Incohérence entre User et Profile
GerardPaligot opened this issue · 32 comments
Je viens créer l'issue officielle pour ce problème épineux.
Quel est le problème ?
Historiquement, lorsque nous voulions référencer un utilisateur dans nos modèles, nous spécifions User comme clé étrangère. Cela demande donc :
- Faire une requête supplémentaire à chaque fois que nous voulons récupérer son profile. Cela ne doit pas couter chère mais, au vu du nombre de fois que cette requête est faite, elle prend une réelle importance.
- Pendant un petit moment, nous ne savions pas sur quel pied dansé. Certains nouveaux modèles références non plus
UsermaisProfile. Donc, aujourd'hui, parfois nous avons l'un, parfois l'autre. - Maintenir une cohérence dans leurs identifiants, chose qui n'est plus le cas en production.
Y'a 1 de décalage : les derniers IDs sont de 2717 / 2718.
Le dernier membre avec les IDs communs est le n°756 "Le Berger".
Le premier avec des IDs différents est le 757 (member_profile) / 758 (user_id) "captain28".On parle de gens qui se sont enregistrées le 24 juillet 2014.
Source: SpaceFox
Quels sont les conséquences actuelles ?
Il est difficile de toutes les énumérer mais en voici quelques unes bien chiantes :
- #2295 : On a une incohérence dans les identifiants entre le site et l'API des membres
- API MP : Ce n'est pas encore vérifié mais je soupçonne fortement un futur problème que je n'ai pas pu déceler pendant le développement puisque je n'avais pas cette incohérence dans les identifiants mais les modèles d'un MP ont comme clé étrangère
Userce qui signifie que nous devons fournir des identifiants de cette table et non pas deProfile(comme c'est renvoyé par l'API des membres). En gros, à partir des utilisateurs au dessus de 757, l'API sera inutilisable (sauf s'ils incrémentent de 1 eux-même l'identifiant renvoyé par l'API des membres). Notons que par l'aspect alpha/bêta de l'API, c'est tolérable (d'un poil). - #1577 : Il y a des soucis d'identification des utilisateurs via leurs karmas.
Oui, le problème commence à devenir urgent !
Quelles sont les solutions possibles ?
- Substituer la table d'authentification d'un utilisateur par une personnelle (doc). Cela va demander une migration lourde.
- Fusionner la table
Useravec la tableProfile. En plus de ne pas savoir comment faire, ça va sans doute demander une migration lourde aussi. - Changer toutes les références à
UserparProfilece qui va demander de très gros changement partout dans le code (amen aux ZEP en cours) et une migration lourde.
Note : Sachez que je suis une quiche en SQL et qu'un script de migration me prendra sans doute du temps si c'est moi qui m'en occupe.
J'hésite à mettre cette issue bloquante tant nous avons laissé ce problème pourrir.
La doc parle en gros de 2 manières d'avoir des éléments custom dans les comptes utilisateurs :
- Étendre le modèle utilisateur via un proxy
- C'est le modèle actuellement utilisé.
- Plus simple à créer
- Permet d'éviter de récupérer les données custom quand on en a pas besoin
- Nécessite de mettre des
select_relatedun peu partout pour éviter les requêtes supplémentaires - Nécessite d'être très rigoureux pour éviter de mélanger les 2 modèles (et c'est là qu'on s'est plantés)
- Remplacer le modèle utilisateur par un modèle complètement dédié au projet
- Simple d'utilisation : tout est au même endroit
- Pas de risque de perdre du temps en requêtes en trop
- Plus complexe à mettre en place
- Délicat à migrer si on est pas dessus depuis le début du projet (et c'est notre cas)
- Sans doute plus lourd en consommation de mémoire dans certains cas (ceux où on ne récupérait que le
userstandard sans leprofilecustom)
Il me semblait avoir lu quelque part que Django recommendait fortement la 2ème solution - mais je ne sais plus où.
En réalité, dans les 3 solutions que tu proposes, @GerardPaligot, les 2 premières sont identiques.
Quoiqu'on choisisse (passer à un remplacement de modèle utilisateur ou nettoyer l'extension de modèle actuelle), la migration va être lourd et délicate.
Je peux essayer de prototyper quelque chose ce WE sur le passage au remplacement de modèle, sans garantie de résultat.
Ok, merci pour tous ces détails @SpaceFox. Par contre, j'aimerais t'exposer un autre problème. Je le mentionne pas dans le premier message parce qu'il est fortement lié à notre situation actuelle aujourd'hui.
Prochainement, nous allons mettre en production l'API des MPs, difficilement utilisables à cause de cette incohérence. On pourrait donc être tenté de proposer un fix (sur cette incohérence) sur la branche de release MAIS la classe Profile n'est pas indépendantes par rapport aux autres modules. En plus de retenir les informations complémentaires d'un utilisateur, elle a comme responsabilité de faire des requêtes vers les autres modules par des méthodes utilitaires (les postes de l'utilisateur, le nombre de sujets, etc.).
DONC On ne peut pas faire de corrections puisque ça va faire des conflits avec la PR sur le refactoring du module des forums qui touche fortement à ces concepts mais qui se merge sur la branche dev.
Hmmm... on peut facilement désactiver l'accès à l'API des MPs ?
Tu commentes cette ligne et les URLs de l'API des MPs ne sont plus accessibles. Pourquoi penses-tu à cette solution ?
Pour dire "tant pis, on fait les corrections au propre dans dev et on interdit l'accès à la nouvelle API en attendant". Pour éviter de mettre en prod un truc pas sec.
Le problème ne se situe pas sur l'API (pour ça, rendez-vous sur ce ticket) et cacher l'API des MPs n'est qu'un pansement sur un des nombreux problèmes que nous rencontrons avec celui exposé dans ce ticket. Autant faire un revert de l'API des MPs et la proposer plus tard à ce compte là.
A condition qu'on puisse encore faire un revert de l'API, d'accord. Sinon, il faudra juste la cacher le temps que le problème de base soit résolu.
(PS : "encore" dans le sens "les corrections faites ne nous empêchent pas de faire le commit de revert").
Bon, j'ai étudié les 2 solutions présentées dans ce post.
La 2ème solution (fusion des 2 tables) n'est plus possible avec notre connaissance en Django, puisque Django n'accepte qu'un, et un seul, objet de type User dans une application. Autrement dit : si cette modification doit être faite, aucun test n'est possible, on est obligés de tout faire d'un coup - et une fois qu'on a tout fini on peut voir si ça marche ou pas.
Reste donc la solution de garder le fonctionnement actuel mais de faire ça proprement.
L'idée est donc de n'utiliser que des member.Profile dans notre code. La migration de données est longue, mais facile (cf le début de code ici).
On a 2 types de champs :
- Ceux qui s'appellent explicitement
useret qui donc s'appellerontprofile. - Ceux qui ont un nom significatif (exemple :
author) et qui conserveront ce nom. Dans ce cas, un 2ème champ temporaire (le temps de faire la migration de données) va être créé, de nomtmp_nomduchamp).
Dans l'état de ma branche, les nouveaux champs sont créés mais pas encore utilisés.
Pour ce faire, il faut repasser sur tout le code parce que un User et un Profile ne s'utilisent pas tout à fait pareil.
Ceci implique des modifications très lourdes, qui vont nécessiter probablement un gel complet des merges parce que la PR que ça va générer sera impossible à rebase.
Je m'y attellerai donc quand le refactoring des forums et si possible la ZEP-12 seront mergés.
L'usage de request.user (dans le back et les templates) est à proscrire partout alors ?
À remplacer par request.user.profile partout où c'est utile, et à laisser comme tel là où on a pas besoin du profile.
Si j'ai bien compris cette issue le problème vient de la différence entre les id de la table user et les id de la table profile pour le même utilisateur. Ils s'agit d'identifiants techniques, donc rien ne nous dit qu'ils seront pareil un jour.
Cependant, le problème a été mis en exergue quand on a décidé d'exposer un identifant pour un membre. Le choix a été fait d'exposer l'identifant du profile.
Y'a une raison pour laquelle on ne veut pas exposer l'identifiant du modèle User dans l'API ? Car j'ai l'impression qu'on se force à exposer l'identifiant du Profile sans vraiment de raison.
@SpaceFox vu que c'est long et chiant à faire, ça ne serait pas mieux de le faire module par module pour que ce soit moins difficile pour éviter des rebases monstres ?
La dépendance à User est omniprésente. C'est difficilement possible.
À remplacer par request.user.profile partout où c'est utile, et à laisser comme tel là où on a pas besoin du profile.
Du coup, si on a besoin du nom d'utilisateur, on fait request.user.profile.user.username ?
À remplacer par request.user.profile partout où c'est utile, et à laisser comme tel là où on a pas besoin du profile.
Du coup, si on a besoin du nom d'utilisateur, on fait
request.user.profile.user.username?
La réponse est dans la question ;) "à laisser comme tel là où on a pas besoin du profile." . Pour avoir l'username tu n'as pas besoin du profile, donc je suppose qu'il ne faut pas s’embêter :)
Ah ok, j'ai compris en relisant le thread et ta réponse. Merci.
Je déterre un peu cette issue car, pour moi le problème de départ (à savoir l'incohérence ) a été corrigé. Les deux soucis étaient :
- L'API exposait avant l'id de la table
Profileau lieu de l'id de la tableUser(ce dernier étant utilisé par tout le reste), ce qui entrainait des mauvaises liaisons par la suite (et des choses comme ça). - le upvote/downvote se faisait via la clé primaire du profile au lieu de l'user
Ces problèmes ont été résolus par les PRs:
- Utilises l'id de User dans l'api des membres et publie l'api des MPs
- Corrige l'affichage des status de vote
Est-ce qu'aujourd'hui @GerardPaligot tu rencontres encore ce problème d'incohérence User/Profile quelque part ?
Je remonte ce sujet parce que je me suis renseigné (merci @vhf) sur la question, j'ai eu des retours sur une de mes PRs (merci @firm1) et après discussion avec vhf, nous arrivons à la conclusion suivante :
- Par souci de connexion avec les autres bibliothèques (nous vers eux ou l'inverse), c'est plus pertinent d'avoir User comme clé étrangère dans tous nos modèles.
- Un
Profilne doit pas être récupéré à partir d'un identifiantUser.
Qu'est-ce qu'il faut faire ?
- Basculer tous les modèles vers
User. - Charger le profil que quand c'est nécessaire.
- Ne jamais présumer que
Profile.pk == User.pk, et donc ne jamais faire de trucs du genreUser.get(pk=profile.pk).
@firm1 J'ai vu ton message après. Je pense que mon message que je viens de poster répond à ta question.
Super.
Je précise que ça nécessite de faire attention à ne pas multiplier les requêtes vers Profile par mégarde.
Du coup @GerardPaligot est-ce qu'on a encore des modèles qui se basent sur Profile ? car sinon, l'issue n'a plus lieu d'être
@firm1 Oui.
@GerardPaligot en jetant un oeil rapide sur les modèles, je n'en vois aucun qui référence les Profile dans sa clé étrangère. Tu pensais à quel modèle toi ?
Je pensais aux unes mais ce n'est plus le cas.
Donc on peut fermer ici ?
Pour bien faire, faudrait un coup de clean sur le projet pour vérifier cette propriété Profile.pk == User.pk mais ça me semble difficilement faisable.
Je ferai une passe ce soir si vous voulez.
2016-01-19 16:05 GMT+01:00 Gérard Paligot notifications@github.com:
Pour bien faire, faudrait un coup de clean sur le projet pour vérifier
cette propriété Profile.pk == User.pk mais ça me semble difficilement
faisable.—
Reply to this email directly or view it on GitHub
#2711 (comment)
.
[victor felder
0788301233]
Je crois que c'est tout bon.
@GerardPaligot cette issue sert encore ?
Je pense que nous sommes tous d'accord que nous devons utiliser partout User dans les clés étrangères et non pas Profile. Actuellement, c'est le cas nul part et nous ne semblons pas présumer que Profile.pk == User.pk.
Donc oui, nous pouvons clore cette issue.