Produits Entr'ouvert » Authentic 2

En relisant la description je vois

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

une recherche sur prénom/nom et email

Et j'avais fait ça dans une première version du patch mais il me semble que ce n'est justement pas ce qu'on veut : jean dupont s'est créé un compte avec supersayan42@pouet.com et bgdu92@hop.com, il ne faut surtout pas prendre en compte l'email lors de la recherche de doublon sinon on passe à côté. Le bon critère important en plus de nom/prénom ça me semble être la date de naissance.

D'ailleurs pour la date de naissance, étant donné qu'elle est stockée sous la forme d'une chaîne de caractère %Y-%m-%d, la manière d'avoir une recherche robuste aux erreurs de saisie ne passe pas par les trigrammes (imo). Par contre la distance de lenvenshtein ça me paraît très adapté, c'est ce que j'ai fait.

Pour la date de naissance on ne fait pas de recherche approximative, si la date est présente, un match donne un bonus dans le calcul de match (genre 60% + 30% * score-trigram-nom/prenom), si pas présent on ne cherche que via trigram sur le nom complet (c'est comme ça que j'ai fait Ǹanterre, avec en plus pour les doublons des heuristiques spécifiques parce qu'ils ont importés des personne à un moment ayant uniquement l'année de naissance ou alors tous né le 1er janvier 1970, on aura pas ça ici).

Pour l'email c'est pareil, il faut gérer un score variable, si ça matche bonus sinon on revient sur du standard nom/prénom. Et généralement il est difficile de régler ces facteurs une fois pour toute, c'est mieux si c'est configurable.

• ne pas matcher sur nom et prénom mais sur la concaténation des deux, ça donne un score plus facilement utilisable (sauf si l'un des deux n'est pas fourni, mais on peut ignorer ce problème je pense),
• je ne vois pas de création d'index, sans index sur cette concaténation ça ne marchera juste jamais; il faut utiliser RunSQL pour le créer en Django 1.11 je ne pense pas que l'API Meta des modèles permettent de créer un index aussi compliqué,
• levenshtein ne peut pas utiliser d'index, oublier, c'est un gadget pas vraiment utilisable en temps réel
• j'imaginais plutôt une API ayant exactement la même forme que l'API de création, charge ensuite pour le endpoint de voir les champs qu'il utilise pour la recherche de doublon; on peut facilement cabler que si un champ de type birthdate est présent, on s'en sert (qu'il s'appelle birthdate, "date-de-naissance" ou quoi que ce soit).
• au niveau de la requête il faut arriver à refiler le calcul du score complètement à postgresql qui s'en sortira mieux que nous en python surtout si on veut utiliser LIMIT pour ne récupérer que les n premiers résultats, voir le code de zoo.

Valentin Deniaud a écrit :

Merci pour les remarques, je me doutais bien que ça n'allait pas convenir du premier coup et je vais reprendre. Je laissais aussi la création d'index de côté en attendant d'être plus sûr des requêtes à faire.

Benjamin Dauvergne a écrit :

• levenshtein ne peut pas utiliser d'index, oublier, c'est un gadget pas vraiment utilisable en temps réel

J'ai l'impression qu'un problème analogue se pose avec unaccent, non ?

• pour pouvoir utiliser un index sur une expression complexe il faut que l'index soit fait sur l'expression complexe elle même donc si recherche trigram sur une chaîne sans accent en minuscule il faut CREATE INDEX ... ON LOWER(UNACCENT(value)) ... (je ne montre pas les modifieurs pour les trigrams c'est dans la doc postgres). Mais souci on ne peut utiliser que des fonction "immutable" ou pure pour utiliser la désignation en programmation fonctionnelle, i.e. qui ne dépendent pas de l'environnement or unaccent dépend de la locale en cours (lower aussi je pense si c'est bien fait) la solution que j'ai trouvé a été de créer une fonction déclarée arbitrairement immutable qui fait le LOWER(UNACCENT(...)), on peut voir ça là : http://git.entrouvert.org/zoo.git/tree/zoo/zoo_meta/migrations/0002_auto_20161214_1545.py#n35

Je suis aller un peu plus loin avec immutable_normalize qui en plus virer les caractères spéciaux par des 'x' (c'est assez arbitraire mais appliqué des deux coté de l'égalité ou de la recherche trigram ça marche bien).

immutable_timestamp c'est pour pouvoir indexer des dates dans un document JSON (via CREATE INDED ... ON immutable_timestamp(document->>'date')).

• j'imaginais plutôt une API ayant exactement la même forme que l'API de création, charge ensuite pour le endpoint de voir les champs qu'il utilise pour la recherche de doublon

Et c'est ce qu'il se passe, si on fournit tous les attributs de création ils seront simplement ignorés.

Sauf qu'il n'y a pas d'attribut birthdate en standard, il peut s'appeler n'importe comment, autant réutiliser BaseUserSerializer et simplement cherche si un champ de type "birthdate" existe.

on peut facilement cabler que si un champ de type birthdate est présent, on s'en sert (qu'il s'appelle birthdate, "date-de-naissance" ou quoi que ce soit).

Ça par contre non, effectivement... Mais je suis quand même davantage pour avoir un truc carré, à mes yeux le petit confort qu'apporte de rebalancer tous les paramètres sans se poser de question ne contrebalance pas 1) la simplicité du code 2) la sûreté du fonctionnement, on ne confond pas la date de naissance avec une autre date éventuellement présente 3) le fait que l'appelant sait ce qui sera utilisé pour déterminer un doublon (parce que du coup il aura dû lire la doc et vu les paramètres réellement utiles).

Sauf qu'authentic est générique et pas ton code, et on aura beaucoup de mal à l'étendre pour dire qu'on cherche aussi les doublons via l'adresse ou le numéro de mobile, je ne dis pas qu'il faut faire tout ça maintenant, mais introduire un serializer et une forme du code inutile qui bloque les évolutions c'est se mettre des batons dans les roues pour rien.

Aussi la recherche par trigram devrait être écrite à coté du code de l'API qu'on ait pas trop de mal à l'utiliser dans le manager mais que ça donne les mêmes résultats (en gros je verrai bien une méthode fuzzy_search sur User ou UserManager qui prend un dico d'attributs ou une instance et l'API n'aurait qu'à l'appeler son boulot étant seulement d'apter le contenu de serializer.validated_data vers ce qu'attend cette méthode, la méthode contenant nos dernières avancée en matière de recherche approchée).

Ça m'a l'air ok, il faudrait voir en combien de temps ça répond avec une centaine de milliers d'utilisateur (on est à 146 000 à Lyon, 80 000 à Nanterre et environ 50000 dans le Nord pour les ordres de grandeur).

Je viens de faire un petit dév sur le coté pour tester ça sur une base un peu plus réelle (ajouter --slow à pytest), #46988.

Je relis mais déjà, sous-classe BaseUserSerializer en DuplicateSerializer pour juste y ajouter duplicate_distance, qu'on ait pas "duplicate_distance": null dans les APIs existantes sur les utilisateurs.

Valentin Deniaud a écrit :

Fait pour la migration.

Et en jouant avec, j'ai découvert un nouveau comportement chelou sur lequel je me casse les dents : RunSQL(reverse_sql='DROP ...') ne droppe rien du tout chez moi, sans erreur aucune (j'enlève bien le IF EXISTS dans mes tests). Si je migrate_schemas -v3, que je copie colle exactement la commande qui est exécutée dans un shell psql, elle marche. J'ai écrit un truc pour préciser le schéma au cas où, DROP INDEX schema_name.index, rien non plus (pareil pour DROP FUNCTION).

C'est SafeExtensionOperation qui cache une erreur SQL mais les opérations ne s'exécutant pas dans une transaction individuelle, ça tue la migration (je pense), plutôt vérifier si l'extension est déjà présente via select * from pg_extension where extname = '<extname>'.

Soit on ne la fait pas, alors il me semble qu'il faut planter, laisser les gens aller installer l'extension, revenir jouer la migration et ça passe.

Il faut commencer dès maintenant alors à contacter les endroits où on n'a pas les permissions suffisantes sur la db pour leur demander d'exécuter telles instructions, ça ne peut pas planter un jeudi de mise à jour.

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

Soit on ne la fait pas, alors il me semble qu'il faut planter, laisser les gens aller installer l'extension, revenir jouer la migration et ça passe.

Il faut commencer dès maintenant alors à contacter les endroits où on n'a pas les permissions suffisantes sur la db pour leur demander d'exécuter telles instructions, ça ne peut pas planter un jeudi de mise à jour.

Le souci c'est que sur combo il me semble que l'ajout des index/extensions vont juste jouer sur les performances, ici si on a pas les nouveaux opérateurs trigram le code ne marche juste pas, et on pourra pas la rejouer une fois qu'elle sera passée sans défaire des migrations qui viendraient après, il faudra joué à la main toutes les commandes et on se prendra le problème à chaque fois dans le futur pour des nouveaux tenant (mais pas au moment d'une mise en prod sûr). Je n'ai pas vraiment de solution mais on ne peut juste pas ignorer des migrations qui foirent dans tous les cas, sinon on le ferait déjà tout le temps.

Valentin comme je ne vois pas de moyen d'avancer en faisant plaisir à tout le monde, je dirai de faire la totalité de la migration en une seule opération manuelle (database_forward()) d'ignorer toutes les erreurs (même sur la création de l'index ou de la fonction) pour ne pas tout péter un jeudi et d'accepter que dans ce cas find_duplicates() plantera, si on se met à l'utiliser sur ces installations on saura d'où ça vient. Mais il faudra donc donner tout le code de la migration à exécuter à la main, pas juste le CREATE EXTENSION....

À noter qu'il me semble qu'on peut poser des questions pendant les migrations ("Cannot create extension pg_trgm and index for duplicate search, do you want to continue ?") ça me semble acceptable pour un jeudi et ça évite que ça passe inaperçu à des endroits où on peut corriger le souci immédiatement.

À noter qu'il me semble qu'on peut poser des questions pendant les migrations ("Cannot create extension pg_trgm and index for duplicate search, do you want to continue ?") ça me semble acceptable pour un jeudi et ça évite que ça passe inaperçu à des endroits où on peut corriger le souci immédiatement.

Les migrations sont lancées via le job systemd qui ne posera pas de question.

            with transaction.atomic():
                try:
                    schema_editor.execute('CREATE EXTENSION IF NOT EXISTS %s SCHEMA public' % self.name)
                except (OperationalError, ProgrammingError):

Ça ne peut logiquement pas marcher, si tu interceptes une exception de l'ORM elle ne passera pas dans le contextmanager atomic() et donc ne provoquera pas l'envoie de la commande ROLLBACK¹.

¹ troisième exemple après https://docs.djangoproject.com/fr/3.1/topics/db/transactions/#django.db.transaction.atomic et le bloc d'aide qui suit

PS: ou plutôt, si ça marche, c'est qu'en fait le atomic() ne sert à rien.

Benjamin Dauvergne a écrit :

ne provoquera pas l'envoie de la commande ROLLBACK¹.

¹ troisième exemple après https://docs.djangoproject.com/fr/3.1/topics/db/transactions/#django.db.transaction.atomic et le bloc d'aide qui suit

Où l'on lit

This is mostly a concern for DatabaseError and its subclasses such as IntegrityError. After such an error, the transaction is broken and Django will perform a rollback at the end of the atomic block. If you attempt to run database queries before the rollback happens, Django will raise a TransactionManagementError.

Ici on a rien à rollback d'une part, rien à exécuter ensuite d'autre part, il me semble qu'on est pas concerné.
Btw cette partie a tourné dans combo sans problème, c'est juste un copier coller de #40024.

Benjamin Dauvergne a écrit :

PS: ou plutôt, si ça marche, c'est qu'en fait le atomic() ne sert à rien.

Il sert pour la InternalError je pense.