Development #57500
Ajouter des champs created et deleted (soft delete) sur RoleParenting
0%
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
Révisions associées
tests_rbac: factorize get_*_model calls (#57500)
django_rbac: add missing constraints (#57500)
tests_rbac: add randomized tests on role parenting (#57500)
django_rbac: new update_transitive_closure algorithm (#57500)
misc: implement soft-delete on RoleParenting (#57500)
Historique
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.
Mis à jour par Paul Marillonnet il y a plus de 2 ans
- Fichier 0001-rbac-add-created-deleted-fields-on-RoleParenting-mod.patch 0001-rbac-add-created-deleted-fields-on-RoleParenting-mod.patch ajouté
- Fichier 0002-rbac-add-soft-created-deleted-model-methods-57500.patch 0002-rbac-add-soft-created-deleted-model-methods-57500.patch ajouté
- Fichier 0003-rbac-add-manager-soft-deletion-shortcut-filters-5750.patch 0003-rbac-add-manager-soft-deletion-shortcut-filters-5750.patch ajouté
- Fichier 0004-rbac-perform-soft-deletion-of-roleparenting-pairings.patch 0004-rbac-perform-soft-deletion-of-roleparenting-pairings.patch ajouté
- Fichier 0005-rbac-update_or_create-roleparenting-for-update_trans.patch 0005-rbac-update_or_create-roleparenting-for-update_trans.patch ajouté
- Fichier 0006-apply-soft-deletion-logic-when-retrieving-roleparent.patch 0006-apply-soft-deletion-logic-when-retrieving-roleparent.patch ajouté
- Tracker changé de Support à Bug
- Statut changé de En cours à Solution proposée
- Patch proposed changé de Non à Oui
Voilà ce qui me paraît être la bonne approche. Si on me valide l’approche, en amont de la relecture et indépendamment du ack sur le code, je peux d’ores et déjà commencer #57499 qui fait la même chose pour Roles.members.through
.
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 )
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'
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.
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).
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.
Mis à jour par Paul Marillonnet il y a plus de 2 ans
- Fichier 0008-apply-soft-deletion-logic-when-retrieving-roleparent.patch 0008-apply-soft-deletion-logic-when-retrieving-roleparent.patch ajouté
- Fichier 0007-rbac-update_or_create-roleparenting-for-update_trans.patch 0007-rbac-update_or_create-roleparenting-for-update_trans.patch ajouté
- Fichier 0006-rbac-perform-soft-deletion-of-roleparenting-pairings.patch 0006-rbac-perform-soft-deletion-of-roleparenting-pairings.patch ajouté
- Fichier 0005-rbac-soft_delete-requires-transitive-closure-update-.patch 0005-rbac-soft_delete-requires-transitive-closure-update-.patch ajouté
- Fichier 0004-rbac-define-post_soft_delete-signal-and-its-handler-.patch 0004-rbac-define-post_soft_delete-signal-and-its-handler-.patch ajouté
- Fichier 0003-rbac-define-custom-manager-for-active-objects-57500.patch 0003-rbac-define-custom-manager-for-active-objects-57500.patch ajouté
- Fichier 0002-rbac-add-soft-created-deleted-manager-methods-57500.patch 0002-rbac-add-soft-created-deleted-manager-methods-57500.patch ajouté
- Fichier 0001-rbac-add-created-deleted-fields-on-RoleParenting-mod.patch 0001-rbac-add-created-deleted-fields-on-RoleParenting-mod.patch ajouté
- Statut changé de En cours à Solution proposée
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).
Mis à jour par Benjamin Dauvergne il y a plus de 2 ans
- Statut changé de Solution proposée à En cours
Mis à jour par Benjamin Dauvergne il y a plus de 2 ans
- Statut changé de En cours à Solution proposée
Mis à jour par Benjamin Dauvergne il y a environ 2 ans
- Fichier 0002-tests_rbac-factorize-get_-_model-calls-57500.patch 0002-tests_rbac-factorize-get_-_model-calls-57500.patch ajouté
- Fichier 0005-django_rbac-new-update_transitive_closure-algorithm-.patch 0005-django_rbac-new-update_transitive_closure-algorithm-.patch ajouté
- Fichier 0004-tests_rbac-add-randomized-tests-on-role-parenting-57.patch 0004-tests_rbac-add-randomized-tests-on-role-parenting-57.patch ajouté
- Fichier 0003-django_rbac-add-missing-constraints-57500.patch 0003-django_rbac-add-missing-constraints-57500.patch ajouté
- Fichier 0006-misc-implement-soft-delete-on-RoleParenting-57500.patch 0006-misc-implement-soft-delete-on-RoleParenting-57500.patch ajouté
- Fichier 0001-tests_rbac-test-with-postgresql-57500.patch 0001-tests_rbac-test-with-postgresql-57500.patch ajouté
- Tracker changé de Bug à Development
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é.
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()
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.
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à.
Mis à jour par Benjamin Dauvergne il y a environ 2 ans
- Assigné à changé de Paul Marillonnet à Benjamin Dauvergne
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)
Mis à jour par Transition automatique il y a environ 2 ans
- Statut changé de Résolu (à déployer) à Solution déployée
tests_rbac: test with postgresql (#57500)