Project

General

Profile

Development #36990

python3 : porter authentic2.compat.JSONField

Added by Paul Marillonnet 4 months ago. Updated 6 days ago.

Status:
Solution déployée
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Start date:
16 Oct 2019
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

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

0001-drop-JSONField-compatibility-with-multiple-db-vendor.patch View (7.3 KB) Paul Marillonnet, 16 Oct 2019 02:51 PM

JSONField_cmp.diff View (1.46 KB) Benjamin Dauvergne, 22 Oct 2019 02:29 PM

0002-auth_oidc-drop-now-redundant-django-jsonfield-depend.patch View (2.65 KB) Paul Marillonnet, 15 Nov 2019 05:44 PM

0001-Revert-add-compatibility-layer-for-support-of-Django.patch View (10.2 KB) Paul Marillonnet, 15 Nov 2019 05:44 PM

0003-tests-drop-partial-sqlite-support-36990.patch View (5.41 KB) Paul Marillonnet, 15 Nov 2019 05:44 PM

0002-auth_oidc-drop-now-redundant-django-jsonfield-depend.patch View (3.03 KB) Paul Marillonnet, 21 Nov 2019 05:13 PM

0001-Revert-add-compatibility-layer-for-support-of-Django.patch View (10.1 KB) Paul Marillonnet, 21 Nov 2019 05:13 PM

0003-tests-drop-partial-sqlite-support.patch View (5.4 KB) Paul Marillonnet, 21 Nov 2019 05:13 PM

History

#1 Updated by Paul Marillonnet 4 months ago

#2 Updated by Benjamin Dauvergne 4 months ago

  • Status changed from Solution proposée to 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 Updated by Paul Marillonnet 4 months ago

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 Updated by Benjamin Dauvergne 4 months ago

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 Updated by Paul Marillonnet 4 months ago

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 Updated by Benjamin Dauvergne 4 months ago

On obtient cette trace en faisant quoi ?

#7 Updated by Paul Marillonnet 4 months ago

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 Updated by Benjamin Dauvergne 4 months ago

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 Updated by Paul Marillonnet 4 months ago

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 Updated by Benjamin Dauvergne 4 months ago

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 Updated by Paul Marillonnet 4 months ago

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 Updated by Paul Marillonnet 4 months ago

  • Subject changed from Retirer la compatibilité JSONField pour SQLite et Django < 1.11 to python3 : porter authentic2.compat.JSONField

#13 Updated by Paul Marillonnet 4 months ago

  • Status changed from Rejeté to En cours

#14 Updated by Paul Marillonnet 4 months ago

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 Updated by Paul Marillonnet 4 months ago

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 Updated by Benjamin Dauvergne 4 months ago

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

#17 Updated by Paul Marillonnet 3 months ago

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 Updated by Paul Marillonnet 3 months ago

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 Updated by Benjamin Dauvergne 3 months ago

  • Status changed from Solution proposée to Solution validée

#20 Updated by Paul Marillonnet 3 months ago

  • Status changed from Solution validée to 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 Updated by Paul Marillonnet 3 months ago

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 Updated by Frédéric Péters 6 days ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF