/polls/voters.json renvoie des utilisateurs en double à travers les requêtes paginées

Bug
, ,
1

Lorsque vous appelez le point de terminaison /polls/voters.json, en utilisant la pagination, nous avons constaté que le premier appel renvoie 25 utilisateurs comme prévu, mais la 2ème page renvoie 26 utilisateurs, dont l’un a été renvoyé lors du premier appel. Ceci est cohérent lorsqu’il y a plus de 25 utilisateurs.

Le problème se situe ici dans le plugin poll pour discourse :

Le décalage est calculé en supposant que postgress SQL est exclusif avec une borne de la ligne en utilisant BETWEEN, alors qu’il est inclusif.

La première requête renvoie en fait 25, car les lignes calculées commencent en fait à 1, pas à 0.

Ma correction proposée est aussi simple que :

   params = {
      offset: offset + 1,
      offset_plus_limit: offset + limit,
      option_digest: opts[:option_id].presence,
    }

Ou, une solution plus élégante pourrait être d’utiliser LIMIT et OFFSET de postgres

    params = {
      limit: limit,
      offset: offset,
      option_digest: opts[:option_id].presence,
    }

          WHERE pv.poll_id IN (:poll_ids)
                /* where */
        ) v
        ORDER BY digest, CASE WHEN rank = 'Abstain' THEN 1 ELSE CAST(rank AS integer) END, username
        LIMIT :limit OFFSET :offset
      SQL
2 « J'aime »
2

Bien vu, je mets un pr-welcome au cas où quelqu’un voudrait tenter de corriger ça.

Nous devons nous assurer de ne pas casser les éléments existants dans les sondages au cas où le front-end dépendrait incorrectement de suppositions erronées ici.

1 « J'aime »
3

Merci pour votre réponse rapide !
J’essaierai de configurer un environnement, je ne voulais pas faire de PR sans tester. N’hésitez pas à me devancer si vous le souhaitez. Je ne suis pas du tout un développeur Ruby. Je ne sais pas non plus comment modifier les spécifications ou les tests.

De manière hilarante, dans l’interface utilisateur, il y a du code qui déverse les résultats dans un ensemble pour contourner ce bug.

2 « J'aime »
4

Pour toute autre personne qui consulte, nous avons apporté une correction d’interface utilisateur similaire pour notre logiciel de tombola ici.

Bravo à @lukecyca de VHS pour avoir identifié le problème.

5

Il y a aussi un rapport connexe sur le chargement de plus d’électeurs dans les sondages à partir de janvier.

1 « J'aime »
7

Excellente trouvaille.

C’est encore plus drôle que ça. J’ai soumis quelques très gros PRs il y a quelque temps pour moderniser le front-end et ajouter le vote par classement.

L’énorme échelle de ces PRs a certainement contribué à ce que je manque cela et que les corrections du back-end n’étaient pas vraiment dans le périmètre (bien qu’en cours de réalisation de ces PRs, j’ai effectivement résolu beaucoup de problèmes qui n’étaient pas strictement destinés à être corrigés par ces PRs).

J’admets avoir suivi ce précédent de front-end qui a été établi en 2018 pour le vote par classement sans vraiment y réfléchir :sweat_smile:. (Que vous avez d’ailleurs également suivi, bien qu’en connaissance de cause :laughing:)

Incroyable de voir comment les solutions de contournement peuvent persister sans que le problème sous-jacent ne soit identifié.

3 « J'aime »
8

J’ai donc pensé que je tenterais celui-ci pour ma première contribution, mais il semble que ce soit un peu plus compliqué que prévu. :wink:

J’ai un PR de brouillon en ligne : FIX: do not return duplicates from /polls/voters.json by clechasseur · Pull Request #1 · clechasseur/discourse · GitHub

Dans ce PR, j’ai d’abord ajouté des tests qui reproduisent le problème, puis j’ai appliqué la simple correction de Rob et cela a fait passer les tests.

J’ai également essayé la solution plus élégante (celle que j’aurais préférée), mais bien qu’elle empêche les électeurs en double, elle modifie également l’électeur retourné dans chaque page, y compris la première, ce qui pourrait potentiellement être considéré comme un changement majeur (selon la façon dont le frontend le gère - je n’ai pas encore regardé).

En prenant du recul, cependant, je me demande quelle est la véritable signification du paramètre limit lorsque vous appelez ce point de terminaison - il ne limite pas vraiment le nombre total d’électeurs retournés, mais seulement le nombre d’électeurs retournés pour chaque option de vote. Vous pouvez voir cet effet dans le test que j’ai ajouté pour le sondage à choix multiples ici - la première page est effectivement limitée à 2 électeurs par option, mais au total, trois électeurs différents sont retournés (répartis sur les options). Passer à la solution élégante (c’est-à-dire utiliser LIMIT :limit OFFSET :offset) fait que la limit s’applique au nombre total de votes et non d’électeurs. Je ne suis pas sûr à 100% que ce soit mieux ou plus intuitif.

Quoi qu’il en soit, je suis nouveau dans ce domaine et je réfléchis peut-être trop. La solution simple supprime les électeurs en double et ne cause pas trop de dégâts, donc c’est peut-être la bonne voie à suivre. J’attendrai des commentaires avant de soumettre un PR au dépôt parent.

–

Par ailleurs, je pense qu’il y a un autre bug dans cette partie du code. La requête pour charger les électeurs est triée par digest, rank et username - mais lors du tri par rank, elle utilise cette condition :

CASE WHEN rank = 'Abstain' THEN 1 ELSE CAST(rank AS integer) END

Cependant, 'Abstain' correspond en fait au rang 0, et non au rang 1 - le rang 1 peut également être retourné sous forme de '1'. Cela rend potentiellement le tri non déterministe entre les requêtes, ce qui signifie que, selon le nombre d’électeurs et la valeur de limit utilisée, il pourrait être possible de manquer des électeurs lors des requêtes paginées. Dans mes nouveaux tests, j’ai dû trier les électeurs retournés pour contourner la nature non déterministe. (Parce que c’est non déterministe, je suppose que ce n’est pas facile à reproduire dans un test, mais je peux essayer…)

1 « J'aime »
9

J’ai suivi le lien ci-dessus vers un rapport précédent, qui inclut un lien vers un rapport encore plus ancien qui semble également pointer vers cette possibilité :

Cependant, cela remonte à longtemps, donc le code a peut-être été totalement différent à l’époque. (J’ai essayé de remonter le temps avec Git, mais l’historique s’est arrêté en 2021, donc je suppose que le code a été déplacé à un moment donné.)

10

Après quelques expérimentations, je n’ai pas réussi à créer un test qui causerait un problème avec l’ordre non déterministe des électeurs retournés. Je pense que c’est à cause de la façon dont la requête est construite et de la façon dont les votes du sondage sont créés, mais je ne suis pas sûr à 100 %.

J’ai créé une PR avec mon test et les correctifs que je propose : FIX: Éviter de retourner des électeurs en double depuis le point de terminaison /polls/voters.json par clechasseur · Pull Request #34433 · discourse/discourse

3 « J'aime »
12

Merci beaucoup pour la correction et votre patience lors de notre examen.

Je l’ai approuvé et fusionné. :partying_face:

3 « J'aime »
13

Ce sujet a été automatiquement fermé après 3 jours. Les nouvelles réponses ne sont plus autorisées.