Projet

Général

Profil

Development #57500

Ajouter des champs created et deleted (soft delete) sur RoleParenting

Ajouté par Benjamin Dauvergne il y a plus de 2 ans. Mis à jour il y a environ 2 ans.

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

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

Description

Cf. #57499

  • created : DateField(auto_now_add=True)
  • deleted : DateField(null=True)

Les suppression d'héritage doivent être revus pour faire un soft-delete et les créations pour supprimer la valeur deleted et rétablir created à now().

Les méthodes pour lister parents/enfants doivent ignorer les relations d'héritage supprimées (il faut faire le tour des utilisations de RoleParenting).


Fichiers

0002-rbac-add-soft-created-deleted-model-methods-57500.patch (1,15 ko) 0002-rbac-add-soft-created-deleted-model-methods-57500.patch Paul Marillonnet, 12 octobre 2021 10:52
0003-rbac-add-manager-soft-deletion-shortcut-filters-5750.patch (1,26 ko) 0003-rbac-add-manager-soft-deletion-shortcut-filters-5750.patch Paul Marillonnet, 12 octobre 2021 10:52
0001-rbac-add-created-deleted-fields-on-RoleParenting-mod.patch (3,23 ko) 0001-rbac-add-created-deleted-fields-on-RoleParenting-mod.patch Paul Marillonnet, 12 octobre 2021 10:52
0004-rbac-perform-soft-deletion-of-roleparenting-pairings.patch (1,9 ko) 0004-rbac-perform-soft-deletion-of-roleparenting-pairings.patch Paul Marillonnet, 12 octobre 2021 10:52
0005-rbac-update_or_create-roleparenting-for-update_trans.patch (1,75 ko) 0005-rbac-update_or_create-roleparenting-for-update_trans.patch Paul Marillonnet, 12 octobre 2021 10:52
0006-apply-soft-deletion-logic-when-retrieving-roleparent.patch (13,1 ko) 0006-apply-soft-deletion-logic-when-retrieving-roleparent.patch Paul Marillonnet, 12 octobre 2021 10:52
0007-rbac-update_or_create-roleparenting-for-update_trans.patch (1,87 ko) 0007-rbac-update_or_create-roleparenting-for-update_trans.patch Paul Marillonnet, 15 octobre 2021 10:42
0008-apply-soft-deletion-logic-when-retrieving-roleparent.patch (11,5 ko) 0008-apply-soft-deletion-logic-when-retrieving-roleparent.patch Paul Marillonnet, 15 octobre 2021 10:42
0006-rbac-perform-soft-deletion-of-roleparenting-pairings.patch (1,71 ko) 0006-rbac-perform-soft-deletion-of-roleparenting-pairings.patch Paul Marillonnet, 15 octobre 2021 10:42
0005-rbac-soft_delete-requires-transitive-closure-update-.patch (1,2 ko) 0005-rbac-soft_delete-requires-transitive-closure-update-.patch Paul Marillonnet, 15 octobre 2021 10:42
0004-rbac-define-post_soft_delete-signal-and-its-handler-.patch (1,72 ko) 0004-rbac-define-post_soft_delete-signal-and-its-handler-.patch Paul Marillonnet, 15 octobre 2021 10:42
0003-rbac-define-custom-manager-for-active-objects-57500.patch (1,44 ko) 0003-rbac-define-custom-manager-for-active-objects-57500.patch Paul Marillonnet, 15 octobre 2021 10:42
0002-rbac-add-soft-created-deleted-manager-methods-57500.patch (1,78 ko) 0002-rbac-add-soft-created-deleted-manager-methods-57500.patch Paul Marillonnet, 15 octobre 2021 10:42
0001-rbac-add-created-deleted-fields-on-RoleParenting-mod.patch (3,23 ko) 0001-rbac-add-created-deleted-fields-on-RoleParenting-mod.patch Paul Marillonnet, 15 octobre 2021 10:42
0002-tests_rbac-factorize-get_-_model-calls-57500.patch (2,33 ko) 0002-tests_rbac-factorize-get_-_model-calls-57500.patch Benjamin Dauvergne, 01 février 2022 00:48
0005-django_rbac-new-update_transitive_closure-algorithm-.patch (5,31 ko) 0005-django_rbac-new-update_transitive_closure-algorithm-.patch Benjamin Dauvergne, 01 février 2022 00:48
0004-tests_rbac-add-randomized-tests-on-role-parenting-57.patch (2,36 ko) 0004-tests_rbac-add-randomized-tests-on-role-parenting-57.patch Benjamin Dauvergne, 01 février 2022 00:48
0003-django_rbac-add-missing-constraints-57500.patch (3,01 ko) 0003-django_rbac-add-missing-constraints-57500.patch Benjamin Dauvergne, 01 février 2022 00:48
0006-misc-implement-soft-delete-on-RoleParenting-57500.patch (25,1 ko) 0006-misc-implement-soft-delete-on-RoleParenting-57500.patch Benjamin Dauvergne, 01 février 2022 00:48
0001-tests_rbac-test-with-postgresql-57500.patch (794 octets) 0001-tests_rbac-test-with-postgresql-57500.patch Benjamin Dauvergne, 01 février 2022 00:48

Révisions associées

Révision 11cfc084 (diff)
Ajouté par Benjamin Dauvergne il y a environ 2 ans

tests_rbac: test with postgresql (#57500)

Révision bd900e08 (diff)
Ajouté par Benjamin Dauvergne il y a environ 2 ans

tests_rbac: factorize get_*_model calls (#57500)

Révision 9477928d (diff)
Ajouté par Benjamin Dauvergne il y a environ 2 ans

django_rbac: add missing constraints (#57500)

Révision c7399b45 (diff)
Ajouté par Benjamin Dauvergne il y a environ 2 ans

tests_rbac: add randomized tests on role parenting (#57500)

Révision a8994cfc (diff)
Ajouté par Benjamin Dauvergne il y a environ 2 ans

django_rbac: new update_transitive_closure algorithm (#57500)

Révision c004a566 (diff)
Ajouté par Benjamin Dauvergne il y a environ 2 ans

misc: implement soft-delete on RoleParenting (#57500)

Historique

#1

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

  • Assigné à mis à Paul Marillonnet
#2

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

  • Statut changé de Nouveau à En cours

Hmm, j’ai commencé à pousser un truc dans la branche. Le build laisse à penser que django teste la possibilité d’évaluer certaines des expressions F() du patch avant l’exécution de la migration. Je regarde.

#4

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

L'implémentation de soft_delete() me semble ok, mais soft_create() je ne pense pas qu'on voudrait l'exposer comme ça, mais j'ai pas encore tout lu, je continue.


Sur un "soft create" il faut remettre "deleted" à NULL, et donc pas besoin de filtre "deleted__lte=F('created')" le seul filtre à avoir c'est deleted__isnull=True quand on cherche les parents/enfants d'un rôle.

Le plus simple c'est d'ajouter un

from model_utils.manager import QueryManager
...
class RoleParenting:
...

actives = QueryManager(deleted__isnull=True)


Plutôt que d'ajouter des soft_create/soft_delete partout je mettrais toute la mécanique create/delete dans des méthodes de RoleParentingManager (qui sera instantié sur RoleParenting.objects) :

class RoleParentingManager:
...
    def soft_create(self, *, parent, child):
         with atomic(savepoint=False):
             rp = self.get_or_crate(parent=parent, child=child)
             if rp.deleted:
                 rp.created = now()
                 rp.deleted = None
                 rp.save(update_fields=['created', 'deleted'])
...
    def soft_delete(self, *, parent, child):
        self.filter(parent=parent, child=child, deleted__isnull=True).update(deleted=now())


Le contraire pour update_transitive_closure, je préfère que la mécanique soit visible ici, c'est déjà du code assez compliqué.


0006: voir si on peut faire en sorte que les managers parent/child_relation utilise le Manager "actives" et pas "objects", et aussi remplacer tous les RoleParenting.objects. par RoleParenting.actives. (on peut aussi poser QueryManager sur objects et définir un "all = Manager()", la doc https://docs.djangoproject.com/en/3.2/topics/db/managers/#default-managers )

#5

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

Visiblement avec ça on pourrait s'en sortir :

class RoleParentingManager(Manager):
    def get_queryset(self):
        return super().get_queryset().filter(deleted__isnull=True)
....
class RoleParenting:
    objects = RoleParentingManager()
    all = models.Manager()

    class Meta:
        base_manager_name = 'all'

#6

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

  • Statut changé de Solution proposée à En cours

Yes c’est bien vu, j’ai commencé à pousser des modifications qui vont dans ce sens, je regarde comment intégrer ça de façon cohérente.

#7

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

Paul Marillonnet a écrit :

Yes c’est bien vu, j’ai commencé à pousser des modifications qui vont dans ce sens, je regarde comment intégrer ça de façon cohérente.

C’est intégré dans la branche. Il me reste quelques modifications pour gérer les signaux déclenchant le re-calcul de la fermeture transitive (les soft_create/delete sur le manager ne semble pas le déclencher).

#8

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

Paul Marillonnet a écrit :

C’est intégré dans la branche. Il me reste quelques modifications pour gérer les signaux déclenchant le re-calcul de la fermeture transitive (les soft_create/delete sur le manager ne semble pas le déclencher).

Je n'ai pas (re)regardé la branche mais ça parait normal, .update() ne déclenche pas de signaux.

#9

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

Benjamin Dauvergne a écrit :

Paul Marillonnet a écrit :

C’est intégré dans la branche. Il me reste quelques modifications pour gérer les signaux déclenchant le re-calcul de la fermeture transitive (les soft_create/delete sur le manager ne semble pas le déclencher).

Je n'ai pas (re)regardé la branche mais ça parait normal, .update() ne déclenche pas de signaux.

Voilà, ça m’a l’air mieux comme ça. Patches avec définition d’un signal et son handler (0004) et son usage après le .update() (0005).

#10

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

  • Statut changé de Solution proposée à En cours
#11

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

  • Statut changé de En cours à Solution proposée
#12

Mis à jour par Benjamin Dauvergne il y a environ 2 ans

En voulant tester tes modifications à update_transitive_closure() j'ai pondu un nouveau test qui m'a permis de m'apercevoir que:
1. les tests django_rbac ne tournaient pas sur postgresql :/
2. django_rbac ne déclarait pas la contrainte de base sur RoleParenting d'unicité du triplet (parent, child, direct),
3. que ça faussait les tests et que l'algo actuel pour update_transitive_closure ne marchait pas terriblement bien, dans le sens où il créait des RoleParenting où child == parent et d'autres idioties.

Le nouveau code est plus clair (je trouve) et utilise directement du SQL pour aller vite. Il permet d'introduire plus simplement les modifications pour le soft-delete. Je ne vais pas m'autovalider mais Paul si tu peux jeter un coup d'oeuil on valide ça ensemble. J'ai repairé aussi des cas où on pouvait utilisait le Manage alive qui n'était pas utilisé et aussi un cas dans check_and_repair ou il manquait un deleted__isnull=True mais rien de vraiment important.

Dans le cas d'export des utilisateurs (resources.py) j'ai ajouté un commentaire pour le filtrage en python de rp.deleted soit justifié.

#13

Mis à jour par Paul Marillonnet il y a environ 2 ans

Benjamin Dauvergne a écrit :

En voulant tester tes modifications à update_transitive_closure() j'ai pondu un nouveau test qui m'a permis de m'apercevoir que:
1. les tests django_rbac ne tournaient pas sur postgresql :/
2. django_rbac ne déclarait pas la contrainte de base sur RoleParenting d'unicité du triplet (parent, child, direct),

Ok, je crois que de fait dans le code on vérifiait déjà cette unicité, mais c’est bien qu’elle soit déclarée explicitement oui.

3. que ça faussait les tests et que l'algo actuel pour update_transitive_closure ne marchait pas terriblement bien, dans le sens où il créait des RoleParenting où child == parent et d'autres idioties.

Est-ce que ça et le gain de vitesse justifient le SQL ? Je ne suis pas super convaincu du gain en clarté :/

J'ai repairé aussi des cas où on pouvait utilisait le Manage alive qui n'était pas utilisé

Ok.

aussi un cas dans check_and_repair ou il manquait un deleted__isnull=True mais rien de vraiment important.

Ok, merci.

Dans le cas d'export des utilisateurs (resources.py) j'ai ajouté un commentaire pour le filtrage en python de rp.deleted soit justifié.

Vu, ok.

Deux petites choses qui restent, la nouvelle version de l’algo de clôture transitive dont la partie SQL me laisse un peu perplexe. Est-ce qu’on a des chiffres sur le gain de performance engendré par le passage des requêtes Django à du SQL brut dans ce cas ? Par exemple en faisait tourner un test randomisé façon 0004 mais avec aussi des cas de soft-deletion, et en comparant les temps d’exécution ?

J’ai relu le reste, à part la clôture transitive c’est ok pour moi, juste quelques octets superflus sur 0004:

diff --git a/tests_rbac/test_rbac.py b/tests_rbac/test_rbac.py
index bf91ffe5..ab9f35ad 100644
--- a/tests_rbac/test_rbac.py
+++ b/tests_rbac/test_rbac.py
@@ -305,10 +305,9 @@ def test_random_role_parenting(db):

     c = 15
     roles = [Role.objects.create(id=i, name=f'role{i}') for i in range(c)]
-    m = [[False] * c for i in range(c)]
     m = np.zeros((c, c), dtype=bool)

-    def check(i):
+    def check():
         one = np.identity(c, dtype=bool)
         z = m
         for i in range(c):
@@ -340,4 +339,4 @@ def test_random_role_parenting(db):
             roles[a].remove_child(roles[b])
             m[a][b] = False
         print('duration', time() - t)
-        check(i)
+        check()

#14

Mis à jour par Benjamin Dauvergne il y a environ 2 ans

Paul Marillonnet a écrit :

Ok, je crois que de fait dans le code on vérifiait déjà cette unicité, mais c’est bien qu’elle soit déclarée explicitement oui.

Ben de fait mon test faisait des trucs interdits et ça passait (créé deux RP avec même parent, child, et direct et ça passait, donc non dans les tests django_rbac on ne vérifiait pas le respect de cette unicité).

3. que ça faussait les tests et que l'algo actuel pour update_transitive_closure ne marchait pas terriblement bien, dans le sens où il créait des RoleParenting où child == parent et d'autres idioties.

Est-ce que ça et le gain de vitesse justifient le SQL ? Je ne suis pas super convaincu du gain en clarté :/

Perso je commence à être plus à l'aise avec un INSERT .. ON CONFLICT .. que de jongler avec 3 requêtes filter(), bulk_create() puis bulk_update() (ce dernier est dispo à partir de Django 2.2), mais je peux comprendre le contraire.

Deux petites choses qui restent, la nouvelle version de l’algo de clôture transitive dont la partie SQL me laisse un peu perplexe. Est-ce qu’on a des chiffres sur le gain de performance engendré par le passage des requêtes Django à du SQL brut dans ce cas ? Par exemple en faisait tourner un test randomisé façon 0004 mais avec aussi des cas de soft-deletion, et en comparant les temps d’exécution ?

Ça ne peut-être que strictement plus rapide, l'ORM de Django fait trop de bazar pour que soit plus lent (y qu'à comparer un list(Model.objects.values()) avec list(Model.objects.all() qui passe par l'ORM mais supprime toute gestion des Field), le deuxième va beaucoup plus vite) donc pas la peine de mesurer. Est-ce que la performance est importante ici ? Non je ne pense pas, donc la seule question c'est sur la clarté, et je trouve ça plus clair, surtout qu'on a que trois champs à gérer parent, child et direct mais ça reste un avis personnel et une question d'habitude. La question de supporter autre chose que postgresql on l'a supprimé il y a longtemps.

J’ai relu le reste, à part la clôture transitive c’est ok pour moi, juste quelques octets superflus sur 0004:
[...]

Ok.

#15

Mis à jour par Paul Marillonnet il y a environ 2 ans

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

Benjamin Dauvergne a écrit :

Ben de fait mon test faisait des trucs interdits et ça passait (créé deux RP avec même parent, child, et direct et ça passait, donc non dans les tests django_rbac on ne vérifiait pas le respect de cette unicité).

Oui, pardon, hyperbole, je voulais simplement dire que dans la précédente version de #57500-9 sur le soft_create on vérifiait qu’on ne créait pas de doublon, mais oui ok c’est trop léger et rien n’empêchait par exemple de taper dans un shell django des doublons de RoleParenting. Il faut des contraintes d’unicité.

Perso je commence à être plus à l'aise avec un INSERT .. ON CONFLICT .. que de jongler avec 3 requêtes filter(), bulk_create() puis bulk_update() (ce dernier est dispo à partir de Django 2.2), mais je peux comprendre le contraire.
Ça ne peut-être que strictement plus rapide, l'ORM de Django fait trop de bazar pour que soit plus lent (y qu'à comparer un list(Model.objects.values()) avec list(Model.objects.all() qui passe par l'ORM mais supprime toute gestion des Field), le deuxième va beaucoup plus vite) donc pas la peine de mesurer. Est-ce que la performance est importante ici ? Non je ne pense pas, donc la seule question c'est sur la clarté, et je trouve ça plus clair, surtout qu'on a que trois champs à gérer parent, child et direct mais ça reste un avis personnel et une question d'habitude.

Ok, je comprends l’argument, sans doute une question d’habitude (ou disons de manque d’habitude de taper du SQL dans le code django) qui fait que je préfère la version avec les requêtes django. On va dire que c’est ma faute, laissons comme cela.

La question de supporter autre chose que postgresql on l'a supprimé il y a longtemps.

Oui, pas de question/remarque de ce côté là.

#16

Mis à jour par Benjamin Dauvergne il y a environ 2 ans

  • Assigné à changé de Paul Marillonnet à Benjamin Dauvergne
#17

Mis à jour par Benjamin Dauvergne il y a environ 2 ans

  • Statut changé de Solution validée à Résolu (à déployer)
commit c004a5667368766c1d56a75d490aa3d197678dca
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Mon Jan 31 20:52:46 2022 +0100

    misc: implement soft-delete on RoleParenting (#57500)

commit a8994cfc62a40fc9d5e5c856d26cbeafaa37b743
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Mon Jan 31 22:26:19 2022 +0100

    django_rbac: new update_transitive_closure algorithm (#57500)

commit c7399b452cd5f5d56f2ebf5d682517706357fc77
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Sun Jan 30 19:58:57 2022 +0100

    tests_rbac: add randomized tests on role parenting (#57500)

commit 9477928d2ae5ea4a156c94f99fc0e7409df8286e
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Mon Jan 31 20:49:55 2022 +0100

    django_rbac: add missing constraints (#57500)

commit bd900e081b6b4a6864a2c8ab788197c26d6d35a5
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Sun Jan 30 19:58:36 2022 +0100

    tests_rbac: factorize get_*_model calls (#57500)

commit 11cfc084a26dcbdafa67fb8cef0b453a9108906b
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Mon Jan 31 22:26:31 2022 +0100

    tests_rbac: test with postgresql (#57500)
#18

Mis à jour par Transition automatique il y a environ 2 ans

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

Mis à jour par Transition automatique il y a presque 2 ans

Automatic expiration

Formats disponibles : Atom PDF