/polls/voters.json retornando usuários duplicados em requisições paginadas

Bug
, ,
1

Ao chamar o endpoint /polls/voters.json, usando paginação, descobrimos que a primeira chamada retorna 25 usuários como esperado, mas a 2ª página retorna 26 usuários, um dos quais foi retornado na primeira chamada. Isso é consistente sempre que há mais de 25 usuários.

O problema está aqui no plugin de enquete para discourse:
https://github.com/discourse/poll/blob/main/plugins/poll/lib/poll.rb#L223

O deslocamento é calculado assumindo que o SQL do postgress é exclusivo com um limite da linha usando BETWEEN, quando é inclusivo.

A primeira consulta retorna 25, porque as linhas calculadas realmente começam em 1, não em 0.

Minha correção proposta é tão simples quanto:

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

Ou, uma solução mais elegante pode ser usar LIMIT e OFFSET do 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 curtidas
2

Boa observação, adicionando uma pr-welcome caso alguém queira tentar corrigir isso.

Precisamos confirmar que não quebraremos coisas existentes em enquetes caso o front-end dependa incorretamente de suposições erradas aqui.

1 curtida
3

Obrigado pela rápida resposta!
Vou tentar configurar um ambiente, não queria fazer um PR sem testar. Sinta-se à vontade para me superar. Eu não sou um desenvolvedor Ruby. Também não sei sobre mudar especificações ou testes.

Hilariamente, na interface do usuário, há um código que despeja os resultados em um conjunto para contornar este bug.

2 curtidas
4

Para qualquer outra pessoa que esteja vendo, implementamos uma correção de UI semelhante para nosso software de rifa aqui.

Parabéns a @lukecyca da VHS por identificar a área do problema.

5

Há também um relatório relacionado sobre o carregamento de mais eleitores em enquetes de janeiro.

1 curtida
7

Ótima observação.

É ainda mais engraçado do que isso. Enviei alguns PRs muito grandes há um tempo para modernizar o front-end e adicionar o Ranked Choice.

A enorme escala desses PRs definitivamente contribuiu para que eu perdesse isso e as correções de back-end não estavam realmente no escopo (embora no processo de fazer esses PRs eu tenha resolvido muitas questões que não eram estritamente para esses PRs resolverem).

Admito ter seguido esse precedente de front-end que foi estabelecido em 2018 para o ranked choice sem realmente pensar sobre isso :sweat_smile:. (Que vocês também seguiram, ainda que conscientemente :laughing:)

É incrível como as soluções alternativas podem persistir sem que o problema subjacente seja identificado.

3 curtidas
8

Então pensei em tentar resolver este problema como minha primeira contribuição, mas parece que pode ser um pouco mais complicado do que eu esperava. :wink:

Tenho um Draft PR aberto: FIX: do not return duplicates from /polls/voters.json by clechasseur · Pull Request #1 · clechasseur/discourse · GitHub

Neste PR, primeiro adicionei testes que reproduzem o problema, depois apliquei a correção simples de Rob e isso fez com que os testes passassem.

Também tentei a solução mais elegante (aquela que eu teria preferido), mas embora ela impeça eleitores duplicados, ela também altera qual eleitor é retornado em qual página, incluindo a primeira, o que potencialmente poderia ser considerado uma alteração que quebra a compatibilidade (dependendo de como o front-end lida com isso - ainda não olhei).

Dando um passo atrás, no entanto, fico me perguntando qual é o verdadeiro significado do parâmetro limit ao chamar este endpoint - ele não limita realmente o número de eleitores totais retornados, apenas o número de eleitores retornados para cada opção de enquete. Você pode ver esse efeito no teste que adicionei para a enquete de múltipla escolha aqui - a primeira página é de fato limitada a 2 eleitores por opção, mas no total, três eleitores diferentes são retornados (espalhados entre as opções). Mudar para a solução elegante (ou seja, usar LIMIT :limit OFFSET :offset) resulta no limit sendo aplicado ao número total de votos e não de eleitores. Não tenho 100% de certeza se é melhor ou mais intuitivo.

De qualquer forma, sou novo nisso, então posso estar pensando demais. A solução simples remove eleitores duplicados e não causa muitos estragos, então pode ser o caminho a seguir. Aguardarei feedback antes de enviar um PR para o repositório pai.

–

Parênteses, acho que há outro bug nesta parte do código. A consulta para carregar eleitores é ordenada por digest, rank e username - mas ao ordenar por rank, ela usa esta condição:

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

No entanto, ’Abstain’ na verdade corresponde ao rank 0, não 1 - o rank 1 também pode ser retornado como ’1’. Isso potencialmente torna a ordenação não determinística entre as consultas, o que significa que, dependendo do número de eleitores e do valor de limit usado, pode ser possível perder eleitores ao fazer consultas paginadas. Nos meus novos testes, tive que classificar os eleitores retornados para contornar a natureza não determinística. (Como é não determinístico, presumo que não seja fácil de reproduzir em um teste, mas posso tentar…)

1 curtida
9

Segui o link acima para um relatório anterior, que inclui um link para um relatório anterior anterior que inclui algo que também parece apontar para essa possibilidade:

No entanto, isso foi há muito tempo, então o código pode ter sido totalmente diferente na época. (Tentei voltar no tempo com o Git, mas o histórico parou em 2021, então presumo que o código foi movido em algum momento.)

10

Após alguma experimentação, não consegui criar um teste que causasse problemas com a ordenação não determinística nos eleitores retornados. Acho que isso se deve à forma como a consulta é elaborada e à maneira como os votos da enquete são criados, mas não tenho 100% de certeza.

Criei um PR com meu teste e as correções que proponho: FIX: Evitar retornar eleitores duplicados do endpoint /polls/voters.json por clechasseur · Pull Request #34433 · discourse/discourse

3 curtidas
12

Muito obrigado pela correção e pela sua paciência em nossa revisão.

Aprovei e mesclei. :partying_face:

3 curtidas
13

Este tópico foi automaticamente fechado após 3 dias. Novas respostas não são mais permitidas.