Draft: Ajoute `get_queryset` dans `ModelSearch` #774

Closed

bclement wants to merge 1 commit from bclement/autocomplete-queryset into master

bclement commented

2020-10-23 22:46:56 +02:00

(Migrated from git.eleves.ens.fr)

Quand on utilise les vues d'auto-complétion, on a la possibilité de
configurer des filtres avec get_queryset_filter, mais pas d'autres
transformations sur le queryset. Notamment, cela signifie que pour les
vues qui utilisent des ForeignKey, on ne peut pas spécifier de
select_related pour éviter des requêtes inutiles et répétées lors du
rendering des vues.

Ce patch ajoute une méthode get_queryset, en suivant la convention
habituelle de Django, permettant cette configuration, et l'utilise pour
appeler select_related de façon appropriée dans les vues qui en
bénéficient.

À priori, cela devrait permettre de compenser la potentielle perte de
performance de !469, qui a supprimé les select_related lors de la
sélection des participants.

Attention, je n'ai pas testé ce code car je n'ai pas réussi à installer un environnement de développement sur ma machine (c'est-à-dire que j'ai arrêté d'essayer au bout de plusieurs erreurs). Je fais une MR pour que le code soit visible. Si quelqu'un veut tester, iel est lea bienvenue; sinon, je le ferai le jour où j'ai un environnement de dév qui marche.

Quand on utilise les vues d'auto-complétion, on a la possibilité de configurer des filtres avec `get_queryset_filter`, mais pas d'autres transformations sur le queryset. Notamment, cela signifie que pour les vues qui utilisent des `ForeignKey`, on ne peut pas spécifier de `select_related` pour éviter des requêtes inutiles et répétées lors du rendering des vues. Ce patch ajoute une méthode `get_queryset`, en suivant la convention habituelle de Django, permettant cette configuration, et l'utilise pour appeler `select_related` de façon appropriée dans les vues qui en bénéficient. À priori, cela devrait permettre de compenser la potentielle perte de performance de !469, qui a supprimé les `select_related` lors de la sélection des participants. **Attention, je n'ai pas testé ce code car je n'ai pas réussi à installer un environnement de développement sur ma machine (c'est-à-dire que j'ai arrêté d'essayer au bout de plusieurs erreurs). Je fais une MR pour que le code soit visible. Si quelqu'un veut tester, iel est lea bienvenue; sinon, je le ferai le jour où j'ai un environnement de dév qui marche.**

lstephan commented

2020-10-28 15:02:19 +01:00

(Migrated from git.eleves.ens.fr)

2 remarques :

ça ne marche pas : il y a un problème d'initialisation dans ParticipantAutocomplete parce que la fonction __init__ a besoin de self.q, qui n'est pas encore défini
Notre utilisation de dal.select2 est probablement obsolète : Django supporte maintenant ça nativement dans l'interface admin (voir ce lien).

Intéressament, il y a la phrase Sometimes you don’t want to incur the overhead of selecting all the related instances to display in the dropdown. : je pense que cela implique que Django se charge des select_related à notre place ?

2 remarques : - ça ne marche pas : il y a un problème d'initialisation dans `ParticipantAutocomplete` parce que la fonction `__init__` a besoin de `self.q`, qui n'est pas encore défini - Notre utilisation de `dal.select2` est probablement obsolète : Django supporte maintenant ça nativement dans l'interface admin (voir [ce lien](https://docs.djangoproject.com/en/2.2/ref/contrib/admin/#django.contrib.admin.ModelAdmin.autocomplete_fields)). Intéressament, il y a la phrase `Sometimes you don’t want to incur the overhead of selecting all the related instances to display in the dropdown.` : je pense que cela implique que Django se charge des `select_related` à notre place ?

mpepin commented

2020-12-04 17:08:24 +01:00

(Migrated from git.eleves.ens.fr)

@bclement est-ce que je peux rebaser sur !475 et terminer le patch ?

bclement commented

2020-12-04 17:51:44 +01:00

(Migrated from git.eleves.ens.fr)

Go ahead

mpepin commented

2020-12-04 17:55:29 +01:00

(Migrated from git.eleves.ens.fr)

Merci

mpepin commented

2020-12-04 18:14:42 +01:00

(Migrated from git.eleves.ens.fr)

pourquoi self.get_queryset().model et pas self.model ?

pourquoi `self.get_queryset().model` et pas `self.model` ?

mpepin commented

2020-12-04 18:24:47 +01:00

(Migrated from git.eleves.ens.fr)

On a le problème suivant :

Dans ModelSearch on appelle d'abord la méthode get_queryset() pour obtenir le queryset des objects dans lesquels on veut chercher, puis dans la méthode .search(keywords) on applique des filtre supplémentaires et on renvoie le résultat de la recherche
Dans Select2QuerySetView c'est l'inverse : la classe dal.autocomplete.Select2QuerySetView attend le résultat "final" de la recherche comme valeur de retour de get_queryset

Je propose de se calquer sur le comportement de dal : la méthode get_queryset renvoie le résultat de la recherche

On a le problème suivant : - Dans `ModelSearch` on appelle d'abord la méthode `get_queryset()` pour obtenir le queryset des objects dans lesquels on veut chercher, puis dans la méthode `.search(keywords)` on applique des filtre supplémentaires et on renvoie le résultat de la recherche - Dans `Select2QuerySetView` c'est l'inverse : la classe `dal.autocomplete.Select2QuerySetView` attend le résultat "final" de la recherche comme valeur de retour de `get_queryset` Je propose de se calquer sur le comportement de `dal` : la méthode `get_queryset` renvoie le résultat de la recherche

mpepin commented

2020-12-04 18:29:20 +01:00

(Migrated from git.eleves.ens.fr)

ah mais ça pue parce que du coup on n'a plus accès aux keywords…

bclement commented

2020-12-04 18:47:23 +01:00

(Migrated from git.eleves.ens.fr)

Ah oui ça me revient; j'avais essayé de faire ça et c'est un gros clusterfuck en fait… Genre ça serait plus simple de juste avoir un my_get_queryset parceque en fait là c'est utilisé de façon incompatible par les 2 et c'est pas pratique…

Ou alors, d'avoir une surcouche comme le truc qui gère plusieurs autocomplete (je me souviens plus du nom) plutôt qu'un mixin ?

Ah oui ça me revient; j'avais essayé de faire ça et c'est un gros clusterfuck en fait… Genre ça serait plus simple de juste avoir un `my_get_queryset` parceque en fait là c'est utilisé de façon incompatible par les 2 et c'est pas pratique… Ou alors, d'avoir une surcouche comme le truc qui gère plusieurs autocomplete (je me souviens plus du nom) plutôt qu'un mixin ?

lstephan commented

2020-12-04 18:51:59 +01:00

(Migrated from git.eleves.ens.fr)

J'avais fait la remarque je sais plus où mais est ce qu'on a encore vraiment besoin des vues python de dal ? Il me semble que c'était uniquement utilisé dans l'interface admin, mais elle supporte Select2 nativement maintenant il me semble...

J'avais fait la remarque je sais plus où mais est ce qu'on a encore vraiment besoin des vues python de `dal` ? Il me semble que c'était uniquement utilisé dans l'interface admin, mais elle supporte `Select2` nativement maintenant il me semble...

bclement commented

2020-12-04 19:02:46 +01:00

(Migrated from git.eleves.ens.fr)

Ah ça serait une solution ça :)

Sinon @mpepin en fait ModelSearch c'est un truc complétement custom iirc? Du coup le quick fix c'est juste d'appeler autrement le get_queryset, genre get_model_queryset, ou get_base_queryset, get_unfiltered_queryset, something.

Ah ça serait une solution ça :) Sinon @mpepin en fait `ModelSearch` c'est un truc complétement custom iirc? Du coup le quick fix c'est juste d'appeler autrement le `get_queryset`, genre `get_model_queryset`, ou `get_base_queryset`, `get_unfiltered_queryset`, something.

mpepin commented

2020-12-04 19:17:51 +01:00

(Migrated from git.eleves.ens.fr)

Oui c'est complètement custom, et j'aime bien get_base_queryset 👍

Je vais d'abord regarder la solution de @lstephan et sinon je ferai ça

Oui c'est complètement custom, et j'aime bien `get_base_queryset` :+1: Je vais d'abord regarder la solution de @lstephan et sinon je ferai ça

mpepin commented

2020-12-04 19:34:56 +01:00

(Migrated from git.eleves.ens.fr)

mentioned in merge request !476