Project

General

Profile

Development #50861

api memberships roles, ne pas écrire si pas de changement

Added by Frédéric Péters 9 months ago. Updated 9 months ago.

Status:
En cours
Priority:
Normal
Category:
-
Target version:
-
Start date:
04 Feb 2021
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

Parce que ces API sont appelées en masse, et que le .add/.remove entraine visiblement .save() qui entraine provisionning, peut-être pourrait-on imaginer quelque chose de ce type :

@@ -916,12 +916,14 @@ class RoleMembershipAPI(ExceptionHandlerMixin, APIView):
             raise PermissionDenied(u'User not allowed to change role')

     def post(self, request, *args, **kwargs):
-        self.role.members.add(self.member)
+        if self.member not in self.role.members:
+            self.role.members.add(self.member)
         return Response({'result': 1, 'detail': _('User successfully added to role')},
                         status=status.HTTP_201_CREATED)

     def delete(self, request, *args, **kwargs):
-        self.role.members.remove(self.member)
+        if self.member in self.role.members:
+            self.role.members.remove(self.member)
         return Response({'result': 1, 'detail': _('User successfully removed from role')},
                         status=status.HTTP_200_OK)

(pas testé, et peut-être qu'il serait souhaité quelque chose de plus compliqué dans hobo)


Files

History

#2

Updated by Benjamin Dauvergne 9 months ago

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

Parce que ces API sont appelées en masse, et que le .add/.remove entraine visiblement .save() qui entraine provisionning, peut-être pourrait-on imaginer quelque chose de ce type :

[...]

Oui. Ça n'entraîne pas de save() sur User mais sur Role.members.throug. Ces changements d'appartenance à des rôles entraînent effectivement un provisionning, pour que les applications soient à jour.

(pas testé, et peut-être qu'il serait souhaité quelque chose de plus compliqué dans hobo)

Je ne pense pas qu'on puisse faire beaucoup mieux juste dans hobo; on a besoin de ce provisionning. Une seule amélioration possible ce serait avec une API batch pour créer/supprimer plusieurs liaisons rôles/utilisateurs d'un coup, ça ne produira rien si on parle en face de l'action ajouter un rôle. On pourrait aussi créer des messages de provisionning spécifiques moins coûteux à construire mais je ne sais pas si il y aura vraiment un gain.

#3

Updated by Frédéric Péters 9 months ago

Oui.

Juste pour être sûr : oui un patch qui reprendrait le chunk indiqué (après l'avoir testé etc.) peut être attaché à ce ticket ?

#4

Updated by Benjamin Dauvergne 9 months ago

Oui oui si tu as ça sous la main sinon je le fais.

#5

Updated by Frédéric Péters 9 months ago

Non j'ai juste tapé le chunk ici sans tester ni rien.

#6

Updated by Benjamin Dauvergne 9 months ago

  • Assignee set to Benjamin Dauvergne
#7

Updated by Benjamin Dauvergne 9 months ago

Bon finalement après contrôle du code de Django, Role.members.add ne produit pas de provisionning si l'action est inutile, car il contrôle les membres déjà présent pour éviter de faire des INSERT inutile (m2m_changed est quand même émis mais pk_set est vide)[1]. Par contre dans Role.members.delete ce contrôle n'est pas fait, le verbe SQL DELETE étant idempotent.

1 https://git.entrouvert.org/hobo.git/tree/hobo/agent/authentic2/provisionning.py#n390

#8

Updated by Benjamin Dauvergne 9 months ago

#10

Updated by Benjamin Dauvergne 9 months ago

  • Status changed from Solution proposée to En cours

Je vais regarde ce qui est vraiment déclenché comme signaux.

Also available in: Atom PDF