Tech : Ajout de données contextuelles dynamique pour les trigger FieldsHistory()

[rsebille]

🤔 Pourquoi ?

Afin d'avoir une solution générique à plusieurs problèmes :

1. Pour la carte ETQ PH je peux signaler que le candidat n’est plus sans solution on veux savoir par qui et quand la modification du nouveau drapeau a été fait, ça aurais pu être fait de plein d'autre manière mais je me suis dit que c'était aussi bien de réutiliser (et améliorer) le FieldsHistory() qui avais déjà été mis en place pour asp_uid
2. Discussion récente pendant la descente sur le fait d'avoir un historique des modifications, en particulier le qui, pour certain champs de JobSeekerProfile().

🍰 Comment ?

Utilisation de la fonction PG set_config() pour avoir une "variable" locale aux transactions.
Voir aussi https://www.postgresql.org/docs/17/runtime-config-custom.html.

Utilisation de cette variable (avec gestion d'erreur) dans la fonction appelée par le trigger.
Voir aussi : https://www.postgresql.org/docs/17/errcodes-appendix.html

Le contexte est enregistré ou mis à jour automatiquement grace à connection.execute_wrapper() de Django.

Un middleware qui initialise le contexte de base.

NB : Je me suis pas mal appuyé sur ce qui est fait dans https://github.com/AmbitionEng/django-pghistory, c'est beaucoup plus complexe mais la logique de base est proche.

🏁 Reste à faire

• Peut-être de tests plus poussés pour triggers.context()
• Tester proprement la concurrence pour _set_context_connection_wrapper(), j'utilise des threading.Event() donc ça devrait être bon quand on est en multiprocess mais à mon avis faudrait les attacher à la variable _context : threading.local() ou peut-être directement sur connection ?
• Initialisation du contexte pour les managements commands
• Vérifier que les premiers écrits sont toujours pertinents, par exemple test_context_when_it_is_never_set devrait changé car la différence de comportement est normalement maintenant gérer dans la fonction PG directement.

[tonial]

Ce dernier commit donne mal à la tête ^^'

[tonial]

Est-ce que ce ne serait pas plus lisible comme ça:

previous_data = getattr(_context, "data", None)
_context.data = kwargs

[rsebille]

Probablement, j'utilise cette forme par habitude car ça permet [1] d'éviter un problème de concurrence où une des 2 variables change entre l'exécution des deux lignes, et vu que c'est une problématique de ce bout de code je l'ai joué safe :).

Pour les détails croustillants :

>>> dis.dis('previous_data, _context.data = getattr(_context, "data", None), kwargs')
  0           RESUME                   0

  1           LOAD_NAME                0 (getattr)
              PUSH_NULL
              LOAD_NAME                1 (_context)
              LOAD_CONST               0 ('data')
              LOAD_CONST               1 (None)
              CALL                     3
              LOAD_NAME                2 (kwargs)
              SWAP                     2
              STORE_NAME               3 (previous_data)
              LOAD_NAME                1 (_context)
              STORE_ATTR               4 (data)
              RETURN_CONST             1 (None)
>>> dis.dis('previous_data = getattr(_context, "data", None);_context.data = kwargs')
  0           RESUME                   0

  1           LOAD_NAME                0 (getattr)
              PUSH_NULL
              LOAD_NAME                1 (_context)
              LOAD_CONST               0 ('data')
              LOAD_CONST               1 (None)
              CALL                     3
              STORE_NAME               2 (previous_data)
              LOAD_NAME                3 (kwargs)
              LOAD_NAME                1 (_context)
              STORE_ATTR               4 (data)
              RETURN_CONST             1 (None)

[1] Je retrouve pas la source 😞

[xavfernandez]

Ça me chagrine un peu (beaucoup) de rajouter une requête (souvent inutile) à toutes nos vues (aussi rapide soit-elle).

Je me demande si ça ne serait pas acceptable de ne mettre le contexte que si on trouve un request.user (voir un token pour également décorer les appels à l'API ?).
A priori si des requêtes non-authentifiées faisaient des UPDATE sur nos utilisateurs/entreprises on aurait un petit soucis 😅 (bon c'est techniquement le cas pour les vues de login mais le point tient tout de même 😬 :

Une autre restriction serait de se limiter aux requêtes POST qui sont normalement celles qui pourraient occasionner un appel aux triggers.

Sur un (petit) pic d'utilisation de 10 minute, on tourne à:

• 14k requêtes HTTP
• dont 9k requêtes authentifiées
• dont 1,4k requêtes POST authentifiées

Sur les dernières 24h:

• 770 k requêtes
• 242 k requêtes authentifiées
• 38,5 k avec un POST

Une solution encore plus extrême serait de ne pas mettre de middleware et de décorer les vues susceptibles de déclencher un trigger mais bon on finira forcément par en oublier...

La solution de se limiter à certaines requêtes me semble un bon compromis.

[rsebille]

Ça me chagrine un peu (beaucoup) de rajouter une requête (souvent inutile) à toutes nos vues (aussi rapide soit-elle).

Pareil, mais l'alternative que j'ai vu (chez pghistory) c'est qu'il la concatène devant une requête "normale", j'ai beaucoup hésité car ça évitait de toucher à tout les snapshots mais je me suis dit que c'était un peu moche 🙈.

Je me demande si ça ne serait pas acceptable de ne mettre le contexte que si on trouve un request.user (voir un token pour également décorer les appels à l'API ?).

Oui, on peux chercher à fine tuner, là j'ai pas trop réfléchis afin que ça soit impactant et que des choses explosent en test :), mais aussi parce que si on commence ne plus prendre en compte juste certain cas bah on est sûr d'oublier qu'ils existent et rien ne nous le diras :/.

Mais effectivement je pense qu'on pourrais ignorer les utilisateurs non connectés car a priori ils ne devraient rien pouvoir faire et parce qu'on a activé le middleware qui va bien donc les chances de se tirer une balle dans le pied sont plus faibles.

Pour les méthodes HTTP on peux en discuter mais pas sûr que ça soit respecté partout et surtout on a aucun moyen de le faire respecter, après on peux clairement ne pas le faire pour les HEAD mais ça doit pas être énorme 😅.

Une solution encore plus extrême serait de ne pas mettre de middleware et de décorer les vues susceptibles de déclencher un trigger mais bon on finira forcément par en oublier...

J'avais écarté cette idée mais on pourrais totalement, par contre il faut que le trigger explose si il n'y a pas de contexte, c'est peut-être le meilleur compromis en fait 💡 🤔

[rsebille]

Un des commits que je viens d'ajouter n'initialise pas le contexte si c'est un GET ou HEAD, a noter que dans un commit fixup précédent le trigger RAISE si il n'y a pas de contexte.
J'ai explorer la piste du décorateur de vues mais ça m'a semblé extrêmement relou sur le long terme, surtout pour l'admin.

[xavfernandez]

Et très stylée cette utilisation de set_config ! 👍 😁

[francoisfreitag]

La nouvelle implémentation me semble bien plus simple 💯
Je referai une passe sur la PR demain matin.

[francoisfreitag]

Créatif et extrêmement utile ❤️ 👏