Projet

Général

Profil

Development #36990

python3 : porter authentic2.compat.JSONField

Ajouté par Paul Marillonnet il y a plus de 4 ans. Mis à jour il y a environ 4 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
-
Catégorie:
-
Version cible:
-
Début:
16 octobre 2019
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

Description

SQLite et Django < 1.11 ne sont plus supportés.
Le patch simplifiera le passage à python3.


Fichiers

Historique

#1

Mis à jour par Paul Marillonnet il y a plus de 4 ans

#2

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

  • Statut changé de Solution proposée à Rejeté

Je ne vois pas en quoi ça simplifie le support python3, et je suis attaché au support sqlite même partiel; le mieux c'est me poser la question avant de faire un patch.

#3

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Benjamin Dauvergne a écrit :

Je ne vois pas en quoi ça simplifie le support python3,

Solution de facilité, qui m'épargne d'implémenter des opérateurs de comparaisons entre champs.

et je suis attaché au support sqlite même partiel; le mieux c'est me poser la question avant de faire un patch.

Je ne savais pas. Je vais laisser de côté cette solution de facilité et porter ce bout de code en python3.

#4

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

Je ne vois pas en quoi ça simplifie le support python3,

Solution de facilité, qui m'épargne d'implémenter des opérateurs de comparaisons entre champs.

Pour ?

#5

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Benjamin Dauvergne a écrit :

Pour ?

Le code de bisect.bisect qui semblerait-il a changé depuis :

  File "/home/paul/src/authentic/src/authentic2_auth_oidc/models.py", line 45, in <module>
    class OIDCProvider(models.Model):
  File "/tmp/tox-paul/authentic/py3-coverage-dj111-authentic-pg-/lib/python3.7/site-packages/django/db/models/base.py", line 162, in __new__
    new_class.add_to_class(obj_name, obj)
  File "/tmp/tox-paul/authentic/py3-coverage-dj111-authentic-pg-/lib/python3.7/site-packages/django/db/models/base.py", line 325, in add_to_class
    value.contribute_to_class(cls, name)
  File "/home/paul/src/authentic/src/authentic2/compat.py", line 90, in contribute_to_class
    cls._meta.add_field(self)
  File "/tmp/tox-paul/authentic/py3-coverage-dj111-authentic-pg-/lib/python3.7/site-packages/django/db/models/options.py", line 277, in add_field
    self.local_fields.insert(bisect(self.local_fields, field), field)
TypeError: '<' not supported between instances of 'JSONField' and 'URLField'
ERROR: InvocationError for command /tmp/tox-paul/authentic/py3-coverage-dj111-authentic-pg-/bin/py.test --cov=src --cov-branch --cov-append --cov-report xml --cov-report html tests/ --random-group (exited with code 1)

Je regarde dès que j'ai un peu de temps.

#6

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

On obtient cette trace en faisant quoi ?

#7

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Benjamin Dauvergne a écrit :

On obtient cette trace en faisant quoi ?

Directement à l'initialisation des tests1, après avoir bien sûr adapté le fichier tox.ini :

diff --git a/tox.ini b/tox.ini
index 29f0921ed..34e58e60d 100644
--- a/tox.ini
+++ b/tox.ini
@@ -5,7 +5,7 @@

 [tox]
 toxworkdir = {env:TMPDIR:/tmp}/tox-{env:USER}/authentic/
-envlist = py27-coverage-dj111-authentic-pg-{oldldap,},py27-coverage-dj111-rbac-pg,pylint
+envlist = {py27,py3}-coverage-dj111-authentic-pg-{oldldap,},{py3,py27}-coverage-dj111-rbac-pg,pylint

 [testenv]
 whitelist_externals =
@@ -61,7 +61,8 @@ deps =
   ldaptools>=0.15
   oldldap: python-ldap<3
 commands =
-  ./getlasso.sh
+  py27: ./getlasso.sh
+  py3: ./getlasso3.sh
   rbac,authentic: py.test {env:FAST:} {env:REUSEDB:} {env:COVERAGE:} {env:JUNIT:} {posargs:{env:TESTS} --random-group}

 [testenv:pylint]

La trace complète ici

#8

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

Et avec ce patch ? Si c'est ok ouvre un ticket migration python3 pour ce patch (j'ai du mal à comprendre pourquoi ça n'arrive qu'en python3).

#9

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Benjamin Dauvergne a écrit :

Et avec ce patch ? Si c'est ok ouvre un ticket migration python3 pour ce patch (j'ai du mal à comprendre pourquoi ça n'arrive qu'en python3).

Toujours pas.

#10

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

Et avec ce patch ? Si c'est ok ouvre un ticket migration python3 pour ce patch (j'ai du mal à comprendre pourquoi ça n'arrive qu'en python3).

Toujours pas.

En fait il faudrait juste déléguer eq et lt à .__real_field__()

#11

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Après quelques modifs pour que ça tourne, par dessus ton diff :

diff --git a/src/authentic2/compat.py b/src/authentic2/compat.py
index f7e48e49c..520de2815 100644
--- a/src/authentic2/compat.py
+++ b/src/authentic2/compat.py
@@ -51,16 +51,16 @@ class JSONField(object):
         self.__kwargs = kwargs
         if django.VERSION >= (1, 11):
             try:
-                from django.contrib.postgres.fields import JSONField
-                self.__dj11_field = JSONField(*args, **kwargs)
+                from django.contrib.postgres.fields import JSONField as Dj11Field
+                self.__dj11_field = Dj11Field(*args, **kwargs)
             except ImportError:
                 pass
         try:
-            from jsonfield.fields import JSONField
-            self.__jsonfield_field = JSONField(*args, **kwargs)
+            from jsonfield.fields import JSONField as JsonfieldField
+            self.__jsonfield_field = JsonfieldField(*args, **kwargs)
         except ImportError:
             pass
-        self.creation_counter = Field.creation_counter
+        super(JSONField, self).__setattr__('creation_counter', Field.creation_counter)
         Field.creation_counter += 1

     def __real_field__(self):

on tombe sur une trace qui laisse entendre qu'il va falloir un peu plus de boulot.
(i.e. le authentic2.compat.JSONField en opérande de comparaison à droite.)

Je n'ai pas d'idée pour l'instant, je creuse l'affaire dès que j'ai un peu de temps.

#12

Mis à jour par Paul Marillonnet il y a plus de 4 ans

  • Sujet changé de Retirer la compatibilité JSONField pour SQLite et Django < 1.11 à python3 : porter authentic2.compat.JSONField
#13

Mis à jour par Paul Marillonnet il y a plus de 4 ans

  • Statut changé de Rejeté à En cours
#14

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Après réflexion je pense que le plus simple est d'altérer le comportement du builtin isinstance au moment de la définition des modèles qui ont un champ de type authentic2.compat.JSONField, et ceci afin de leurrer la méthode __lt__ de la classe native Field de django à l'origine de l'erreur lors de la tentative de comparaison à droite.

Peut-être un gestionnaire de contexte qui ferait un truc du genre :

from django.utils.six.moves import builtins as __builtin__

@contextmanager
def fake_isinstance_contextmanager():
    isinstance_orig = __builtin__.isinstance

    def fake_isinstance(obj, clz):
        if isinstance_orig(obj, compat.JSONField) and issubclass(clz, models.Field):
            return True
        else:
            return isinstance_orig(obj, clz)    

    try:
        __builtin__.isinstance = fake_isinstance
        yield
    finally:
        __builtin__.isinstance = isinstance_orig

with fake_isinstance_contextmanager():
    class OIDCProvider(models.Model):
        # […]

#15

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Benj, en réunion dev :

je pense qu'on va dumper, j'abandonne

Je pense qu'on peut en profiter pour délaisser la dépendance à django-jsonfield et se baser sur le champ natif postgresql de django.

#16

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

Tu peux reprendre ton premier patch et y ajouter de virer tout ce qui concerne sqlite dans les tests.

#17

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Sans doute comme ça. Dans le premier patch je passe par un revert tout bête qui réintroduit la dépendance django-jsonfield, pour la virer juste après, dans le second patch. Est-ce que c'est grave docteur ? :)

Autre question que je me pose : aurait-on intérêt à tester que le patch modifiant la migration ne va pas casser les champs jusque-là stockés sous forme de jsonfield.JSONField, et qui à l'insu de leur plein gré deviennent des django.contrib.postgres.fields.JSONField ?

#18

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Paul Marillonnet a écrit :

Autre question que je me pose : aurait-on intérêt à tester que le patch modifiant la migration ne va pas casser les champs jusque-là stockés sous forme de jsonfield.JSONField, et qui à l'insu de leur plein gré deviennent des django.contrib.postgres.fields.JSONField ?

En ayant relu le code de django-jsonfield et du code de contrib de django, je suis rassuré.
On est sur du pg>9.4 sur Debian Stretch, donc la situation où des données jusque là stockées en type pg 'TEXT' ou 'JSON' seraient interprétées comme du 'JSONB' suite à la migration ne se posera pas (et quand bien même elle se poserait, il y a l'intelligence dans psycopg2 pour faire face à ce genre de situation1).

Je remets donc sur la table les trois patches (quasiment) inchangés.

1 https://github.com/psycopg/psycopg2/blob/master/lib/_json.py#L158

#19

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

  • Statut changé de Solution proposée à Solution validée
#20

Mis à jour par Paul Marillonnet il y a plus de 4 ans

  • Statut changé de Solution validée à Résolu (à déployer)
commit 2cc2cf4200b69ddc36b6262456b7afbadfa0d963
Author: Paul Marillonnet <pmarillonnet@entrouvert.com>
Date:   Fri Nov 15 17:16:50 2019 +0100

    tests: drop partial sqlite support

commit 6d064a2c223e900d670b398578e971f73aea6a8b
Author: Paul Marillonnet <pmarillonnet@entrouvert.com>
Date:   Fri Nov 15 17:01:36 2019 +0100

    auth_oidc: drop now-redundant django-jsonfield dependency

commit cef04caf83376793c29cb58d4bc0fc690b251762
Author: Paul Marillonnet <pmarillonnet@entrouvert.com>
Date:   Tue Oct 29 11:18:16 2019 +0100

    Revert "add compatibility layer for support of Django native JSONField (fixes #29193)" 

    This reverts commit d730dba525215d615fa2b974d44183d16909f4d2.
#21

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Et j'avais les bons messages de commit dans une branche locale, que j'ai écrasée par erreur sans m'en rendre compte. Mes excuses pour ces messages de commit qui ne font pas apparaître le numéro de ticket. :/

#22

Mis à jour par Frédéric Péters il y a environ 4 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF