Produits Entr'ouvert » Authentic 2

Au moins,

    def export_json(self):
        data = {
            'uuid': self.uuid,
            'slug': self.slug,
            'name': self.name,
        }

Ça me semble zapper l'existence d'un attribut description.

Le code de import_json apparait gérer une possibilité de service mais je ne vois pas comment ça peut arriver dans l'export.

Josué Kouka a écrit :

Le comportement de l'import dans ce patch.
On s'arrete si:

• l'OU n'est pas trouvé
• le service auquel un role est attaché n'existe pas

Mouais, mais hier je t'ai proposé de tester d'abord la présence de toutes les OU et services d'un export, avant de lancer un import. Finalement non ?

• le role parent d'un role n'existe pas seulement si l'option --stop-on-absent-parent est passé.

Pourquoi ça ? Ça voudrait dire qu'on accepte que l'import n'a pas marché et n'a pas su recréer le graphe des relations ?

Je n'accepterai pas un patch avec Narnia dedans, vraiment.

Thomas Noël a écrit :

Josué Kouka a écrit :

Le comportement de l'import dans ce patch.
On s'arrete si:

• l'OU n'est pas trouvé
• le service auquel un role est attaché n'existe pas

Mouais, mais hier je t'ai proposé de tester d'abord la présence de toutes les OU et services d'un export, avant de lancer un import. Finalement non ?

Le bloc d'import est dans le contexte d'une transaction atomique, je pense que ça devrait le faire

• le role parent d'un role n'existe pas seulement si l'option --stop-on-absent-parent est passé.

Pourquoi ça ? Ça voudrait dire qu'on accepte que l'import n'a pas marché et n'a pas su recréer le graphe des relations ?

T'as raison, on ne devrait pas en fait.

Je n'accepterai pas un patch avec Narnia dedans, vraiment.

Ok je vais changer (j'étais à cours d'idée)

Ya quelques trucs pas très propres : une exception non utilisée, la convention de nommage des attributs (avec ou sans underscore) mal utilisée.
J'ai craqué et j'ai besoin d'une review, ce sera corrigé pour le prochain patch.

Emmanuel Cazenave a écrit :

Il va falloir converger avec #20706 pour la sérialisation des rôles.

Ce n'est pas essentiel.

Vu dans #22377:

Benjamin Dauvergne a écrit :

Les services n'étant pas unique par slug je demande deux paramètres, le slug de l'OU et le slug du service.

Ici je recherche les services (pour les rattacher aux roles que j'importe), par slug puis par nom, avec dans les deux cas une hypothèse d'unicité, bref un beau bug en perspective.

Par nom c'est clairement pas unique, donc tu peux le faire si effectivement tu trouves un unique exemplaire, sinon abandonne mais pour te simplifier la tâche, utilise l'ou oui.

Par slug et par ou t'es sûr d'en trouver un ou rien.

Dans les précédents patchs on avait dit qu'on exportait pas les rôles attachés à un service (ça doit être écrit quelque part) car ils sont créé automatiquement (il n'y a qu'un seul cas actuellement, les rôles nommés 'Administrateur de <service>' et ils sont créé automatiquement par la commande hobo-deploy d'authentic. Je monte à Paris la semaine prochaine, on pourra voir tout ça.

Dans les précédents patchs on avait dit qu'on exportait pas les rôles attachés à un service (ça doit être écrit quelque part) car ils sont créé automatiquement (il n'y a qu'un seul cas actuellement, les rôles nommés 'Administrateur de <service>' et ils sont créé automatiquement par la commande hobo-deploy d'authentic. Je monte à Paris la semaine prochaine, on pourra voir tout ça.

J'ai du louper, il y a eu beaucoup d'échanges, bref ça me va bien puisque ça me simplifie la tache. Cool de voir ça en direct avec toi semaine prochaine.

Vu avec Benjamin, je retranscris ses remarques.

Rajouter import/export des OU (en cherchant sur uuid, slug, name).

Stratégie de matching entre roles à importer et roles existant, par ordre de priorité :
1. sur l'uuid
2. sur slug + ou
3. sur nom + ou

Parenté entre roles : cas particulier des parentés entre roles "normaux" et les roles techniques "administrateur du role X", il faut que l'import recrée ces liens de parenté.

Import/export des permissions sur les roles: utiliser les méthodes natural_foreign_key pour retrouver les ContentType (dans la table ContentType, pas de stabilité sur les ids mais stabilité sur le nom des modèles)

Par défaut : supprimer tous les objets liés à des roles et recréer (attributs, permissions).

En option : dry run, supprimer les objets en trop (role et OU présent dans la base mais pas dans l'export) avec une demande de confirmation pas l'utilisateur.

Je valide.

La partie natural_key, aujourd'hui obligatoire à l'import, crée pour moi quelque chose de trop lié à la structure interne, qui complique terriblement l'utilisation de l'import avec un fichier json produit par ailleurs.

Peut-être qu'il s'agit juste à une série d'endroits d'accepter qu'il n'existe pas de ['natural_key'] et passer à la méthode suivante.

En gros j'aimerais pouvoir donner ça à import_site :

{   
    "roles": [
        {   
            "name": "Debug (42)",
            "slug": "debug-42" 
        }
    ]
}

Ça passerait avec des modifs comme :

@@ -39,11 +39,11 @@ def search_role(role_d):
     Role = get_role_model()
     try:
         return Role.objects.get(uuid=role_d['uuid'])
-    except Role.DoesNotExist:
+    except (Role.DoesNotExist, KeyError):
         pass
     try:
         return Role.objects.get_by_natural_key(*role_d['natural_key'])
-    except Role.DoesNotExist:
+    except (Role.DoesNotExist, KeyError):
         return None

et

@@ -104,7 +104,10 @@ class RoleDeserializer(object):
             else:
                 self._role_d[key] = value

-        ou_natural_key = self._role_d['natural_key'][1]
+        try:
+            ou_natural_key = self._role_d['natural_key'][1]
+        except KeyError:
+            ou_natural_key = None
         if ou_natural_key:
             self._ou = search_ou(ou_natural_key)
             if self._ou is None:

Aussi, je comprends la frilosité mais le comportement par défaut devrait être l'import, et l'option --dry-run, pas l'inverse.

Frédéric Péters a écrit :

La partie natural_key, aujourd'hui obligatoire à l'import, crée pour moi quelque chose de trop lié à la structure interne, qui complique terriblement l'utilisation de l'import avec un fichier json produit par ailleurs.

Dans l'idée, ils viendraient d'où ces fichiers json ? Un peu de contexte sur les cas d'usage ?

Frédéric Péters a écrit :

La partie natural_key, aujourd'hui obligatoire à l'import, crée pour moi quelque chose de trop lié à la structure interne, qui complique terriblement l'utilisation de l'import avec un fichier json produit par ailleurs.

On sort un peu du cadre, import/export d'un site identique à l'autre, et on va dériver. Mais ce que je verrai après ce premier ticket ce serait plutôt de pouvoir importer plus des templates de site plutôt que des exports, par exemple une liste de rôle sans OU spécifiée mais à l'import qu'on puisse mettre --default-ou-slug oullins et on importe une structure basique

Peut-être qu'il s'agit juste à une série d'endroits d'accepter qu'il n'existe pas de ['natural_key'] et passer à la méthode suivante.

En gros j'aimerais pouvoir donner ça à import_site :

[...]

• retourner comme natural_key [role.name, role.slug, ou.natural_key(), role.service and service.natural_key()]
• dans get_by_natural_key() ne rendre que les 3 premiers arguments obligatoires.

Ensuite il faudrait fonctionner comme dumpdata/loaddata qui n'exportent pas la natural_key telle quelle mais la reconstruisent à la volée, en gros ça mange ça:

{
   'uuid': 'xxxx',
   'name': 'efefe',
   'slug': 'slug1',
   'ou': {
       'uuid': 'yyyy',
       'name': 'xxxx',
       'slug': 'ou-slug',
   }
   'service': null,
}

Ma position ce serait donc d'éviter le champ natural_key explicite.

Pour Fred au mieux ça va donner ça:

{
   'roles': [
      {
         'name': 'Role1',
         'slug': 'role1',
         'ou': { 'slug': 'default' },
      }
   ]
}

ou ce qu'il souhaite si on ajoute une option --default-ou.

Aussi, je comprends la frilosité mais le comportement par défaut devrait être l'import, et l'option --dry-run, pas l'inverse.

• un dry-run
• on affiche un rapport de pré-import
• on demande une confirmation
• on applique
  et qu'on ait une option --no-input qui passe la confirmation.

Je continue à lire.

• https://github.com/django/django/blob/master/django/core/serializers/base.py#L268 : FK
• https://github.com/django/django/blob/master/django/core/serializers/base.py#L239 : objet

Par exemple d'un système extérieur qu'on migrerait. Ou d'une version passée ou future d'Authentic qui n'aurait pas exactement la même structure pour natural_key.

Ça vient aussi de mon inquiétude quand on se trouve avec des listes dans le json, plutôt que des dictionnaires, ça rend les évolutions plus compliquées.

Dans mon export local actuel, et c'est un rôle technique donc pas nécessairement représentatif, mais quand même "natural_key":

[ "_a2-hobo-superuser", [ "default" ], [ [ "default" ], "portal" ] ],

je trouve ça bien moins sûr que

"slug": "_a2-hobo-superuser", "ou": {"slug": "default"}, "service": {"slug", "portal", "ou": {"slug": "default"}}

il faut indiquer une OU

Ok mais comme il y a déjà une OU indiquée comme OU par défaut au niveau du site, je me disais que l'information pouvait être utilisée.

Frédéric Péters a écrit :

Par exemple d'un système extérieur qu'on migrerait. Ou d'une version passée ou future d'Authentic qui n'aurait pas exactement la même structure pour natural_key.

Ça vient aussi de mon inquiétude quand on se trouve avec des listes dans le json, plutôt que des dictionnaires, ça rend les évolutions plus compliquées.

Dans mon export local actuel, et c'est un rôle technique donc pas nécessairement représentatif, mais quand même "natural_key":

[ "_a2-hobo-superuser", [ "default" ], [ [ "default" ], "portal" ] ],

je trouve ça bien moins sûr que

"slug": "_a2-hobo-superuser", "ou": {"slug": "default"}, "service": {"slug", "portal", "ou": {"slug": "default"}}

Pour ça on est d'accord.

Frédéric Péters a écrit :

il faut indiquer une OU

Ok mais comme il y a déjà une OU indiquée comme OU par défaut au niveau du site, je me disais que l'information pouvait être utilisée.

Je préfère qu'on évite l'implicite, dans le cas général d'import entre site mono-collectivité, on aura toujour ou_slug=='default' et ça marchera tout seul d'export en import et à la génération depuis un système tiers (ce que vraiment on a pas prévu de faire là maintenant et qu'on ne fait jamais pour l'instant) on pensera à mettre 'ou': {'slug': 'default'}, si on se retrouve sur un site où le slug default n'existe pas ça plantera et ça nous obligera à réfléchir à ce qu'on est en train de faire.

À la rigueur, je veux bien qu'on accepte l'absence d'ou mais uniquement si le site où on charge n'en a qu'une.

Par défaut la commande fait maintenant l'import, avec un prompt de confirmation, désactivable en passant --yes.

Plutôt --noinput pour cette option, pour suivre ce qui est fait pour migrate et collectstatic ? (même si dans les autres modules on n'a pas ce genre d'option et qu'au fond je m'en passerais bien ici aussi, plutôt que devoir aller le mettre partout).

Oui, parce que moi aussi j'apprécie les vertus de la régularité.
Et je veux bien la virer aussi, pour peu qu'on me fasse pas la remettre si je la vire : Benjamin c'est bon pour toi ?

Frédéric Péters a écrit :

Par défaut la commande fait maintenant l'import, avec un prompt de confirmation, désactivable en passant --yes.

Plutôt --noinput pour cette option, pour suivre ce qui est fait pour migrate et collectstatic ? (même si dans les autres modules on n'a pas ce genre d'option et qu'au fond je m'en passerais bien ici aussi, plutôt que devoir aller le mettre partout).

Mais tu imagines déjà des flopées d'import/export automatiques dans tous les sens ou bien ?

Lors des déploiements, qu'hobo ait connaissance de modèles et provoque un import_site whatever pour toutes les applications; là-dessus donc, plutôt qu'avoir à coder dans hobo que authentic est différent et demande un flag --noinput, je préfère un comportement homogène (mais si ça doit passer par l'ajout d'une confirmation et de la prise en charge d'un --noinput dans les autres applications, so be it).

#  Borrowed from https://bugs.python.org/issue10049#msg118599
@contextlib.contextmanager
def provision_contextm(dry_run):
    multitenant = False
    try:
        import hobo.agent.authentic2
        multitenant = True
    except ImportError:
        pass
    if dry_run and multitenant:
        with hobo.agent.authentic2.provisionning.Provisionning():
            yield
    else:
        yield

un peu alambiqué, fais juste un test sur la présence de 'hobo.agent.authentic2' dans INSTALLED_APPS (la situation n'est pas vraiment multitenant ou pas, mais provisionning ou pas, ici malheureusement par le truchement de la Publik-isation d'authentic tout ça est un peu mélangé).

La duplication du code de calcul des natural key n'est pas super DRY, normalement ça irait tout seul si:
1. tu résoud la référence à l'OU, (si on ne trouve pas, boum)
2. puis tu copies la sérialisation et tu remplaces le champ 'ou' par la valeur obtenue en 1,
3. ensuite tu peux directement passer ta sérialisation au constructeur de Role,
4. enfin tu peux appeler la méthode natural_key() sur l'objet temporaire créé en 3 (c'est à peux près ce que fait le code de dé-sérialisation de Django)

• contrairement à Passerelle on a pas encore la visée d'exporter tout et n'importe quoi
• le code n'est pas factorisé ce qui est tout l'intérêt dans passerelle

Je remplacerai volontiers ça

        concrete_fields = [f for f in self.__class__._meta.get_fields()
                           if f.concrete and not f.is_relation]
        for field in concrete_fields:
            if field.name == 'id':
                continue
            value = getattr(self, field.attname)
            if isinstance(field, SIMPLE_SERIALIZABLE_FIELDS):
                d[field.name] = value
            else:
                raise Exception('export_json: field %s of ressource class %s is unsupported' % (
                    field, self.__class__))

d['uuid'] = self.uuid
d['slug'] = self.slug
d['name'] = self.name

et ça:

        d['ou'] = None
        if self.ou:
            d['ou'] = {'uuid': self.ou.uuid, 'slug': self.ou.slug, 'name': self.ou.name}

d['ou'] = self.ou and self.ou.export_json(full=False)  # full enable exporting other attributes uuid, slug and name

Et je ferai la même chose pour Permission (on exporter le truc correctement et on cacul les natural_key à la volée, avec un export_json()/import_json() sur l'objet Permission).

Frédéric Péters a écrit :

Lors des déploiements, qu'hobo ait connaissance de modèles et provoque un import_site whatever pour toutes les applications; là-dessus donc, plutôt qu'avoir à coder dans hobo que authentic est différent et demande un flag --noinput, je préfère un comportement homogène (mais si ça doit passer par l'ajout d'une confirmation et de la prise en charge d'un --noinput dans les autres applications, so be it).

Bon ok alors, on garde la confirmation que si --dry-run est invoqué, et par défaut c'est non.

Bon ok alors, on garde la confirmation que si --dry-run est invoqué, et par défaut c'est non.

La demande de confirmation sur un tir à blanc ce n'est vraiment pas utile, je trouve. Ça me va de garder --noinput ici et quand il y aura des appels depuis hobo, hobo gérera.

Finalement la reprise du code d'export de passerelle ne sert un peu à rien:

• contrairement à Passerelle on a pas encore la visée d'exporter tout et n'importe quoi
• le code n'est pas factorisé ce qui est tout l'intérêt dans passerelle

Mais tu disais exactement l'inverse ici : https://dev.entrouvert.org/issues/12595#note-32 !!! Et je trouve que tu avais raison.
Effectivement ça manque de factorisation, je veux bien faire un effort là dessus.
Mais me faire revenir en arrière alors que j'ai fait l'effort de respecter tes remarques....

Emmanuel Cazenave a écrit :

Finalement la reprise du code d'export de passerelle ne sert un peu à rien:

• contrairement à Passerelle on a pas encore la visée d'exporter tout et n'importe quoi
• le code n'est pas factorisé ce qui est tout l'intérêt dans passerelle

Mais tu disais exactement l'inverse ici : https://dev.entrouvert.org/issues/12595#note-32 !!! Et je trouve que tu avais raison.
Effectivement ça manque de factorisation, je veux bien faire un effort là dessus.
Mais me faire revenir en arrière alors que j'ai fait l'effort de respecter tes remarques....

Le problème c'est que ce n'est pas factorisé, j'aurai du être plus explicite mais l'idée d'un tel code c'est d'avoir une base pour aller vers un export de tous les modèles, là tel que c'est fait ça ne nous avancera pas sur cette voie, donc soit tu factorises soit tu simplifies (j'en ai déjà jeter plus que ça du code tu ne me feras pas pleurer :-) ).

Au passage si ça peut gérer directement les FK et M2M via natural_key, +100 (aussi pour te simplifier la vie, ce code générique devrait prendre en paramètre optionnel la liste des champs qu'on souhaite exporter).

En l'état mon patch est loin du niveau de généricité de passerelle mais permets quand même de pas avoir à toucher au code d'import/export à chaque fois qu'on rajoute un champ 'simple' à OU ou Role, c'était ça mon objectif.

Emmanuel Cazenave a écrit :

En l'état mon patch est loin du niveau de généricité de passerelle mais permets quand même de pas avoir à toucher au code d'import/export à chaque fois qu'on rajoute un champ 'simple' à OU ou Role, c'était ça mon objectif.

Si tu peux simplement faire une fonction, generic_export_json(instance, fields=()) ce sera un début de factorisation, prends tous les champs si fields est vide, sinon uniquement ceux dans fields.

J'ai poussé dans wip/import-export-role

un peu alambiqué, fais juste un test sur la présence de 'hobo.agent.authentic2' dans INSTALLED_APPS (la situation n'est pas vraiment multitenant ou pas, mais provisionning ou pas, ici malheureusement par le truchement de la Publik-isation d'authentic tout ça est un peu mélangé).

Done.

La duplication du code de calcul des natural key n'est pas super DRY, normalement ça irait tout seul si:

....

Tu parles de build_role_natural_key ? Si on s'interdit d'utiliser natural_key dans la sérialisation, il faut bien que je la recalcule quand je déserialise pour pouvoir appeler get_by_natural_key. Donc ta remarque en fait c'est : ne jamais de servir de get_by_natural_key. Mais pourquoi donc ? Ça me mâche le travail et c'est fait pour ça il me semble non ?

Et je ferai la même chose pour Permission (on exporter le truc correctement et on calcule les natural_key à la volée, avec un export_json()/import_json() sur l'objet Permission).

Sûr de sûr ? Là j'ai fait un natural_key d'un coté, un get_by_natural_key de l'autre et ça a marché direct. Le modèle étant un peu compliqué avec entre autres sa généric relation, ça va faire pas mal de code pour arriver au même résultat. Le jeu en vaut-il la chandelle ? Pitié.

Finalement la reprise du code d'export de passerelle ne sert un peu à rien:

J'ai tout viré et opté pour ta solution simple.

• Tu peux merger le 0001 et le 0002.
• On a oublié un truc dont on a avait parlé au tableau: exporter les fils des rôles d'administration des rôles, dans Role.export_json():
  

  if admin_roles:
      admin_role = self.get_admin_role()
      for relation in admin_role.child_relation.all():
         d.setdefault('admin_roles', []).append(relation.child.natural_key_json())
  

  
  idem à la désérialisation

Ack.