/polls/voters.json returning duplicate users across paged requests

Bug
, ,
1

When calling the /polls/voters.json endpoint, using pagination, we’ve found that the first call returns 25 users as expected, but the 2nd page returns 26 users, one of which was returned in the first call. This is consistent whenever there are over 25 users.

The problem is here in the poll plugin for discourse:

The offset is calculated assuming that postgress SQL is exclusive with a bound of the row using BETWEEN, when it is inclusive.

The first query actually returns 25, because the calculated rows actually start a 1, not 0.

My proposed fix is as simple as:

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

Or, a more elegant solution might be to use postgres LIMIT and OFFSET

    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 Likes
2

Nice catch, putting a pr-welcome on this in case anyone wants to give fixing this a go.

We need to confirm we don’t break existing stuff in polls in case the front end incorrectly depends on wrong assumptions here.

1 Like
3

Thanks for quick reply!
I’ll try to set up an env, didn’t wanna PR without testing. Anyone feel free to beat me to it. I’m not a ruby dev at all. I also don’t know about changing spec or tests.

Hilariously in the UI there is code that dumps the results into a set to bypass this bug.

2 Likes
4

For anyone else viewing, we put in a similar UI fix for our raffle software here.

Cheers to @lukecyca from VHS for identifying the area of issue.

5

There is also a related report about the loading of more voters in polls from January.

1 Like
7

Great catch.

It’s even more funny than that. I submitted a couple of very large PRs a while back to modernise the front end and add Ranked Choice.

The huge scale of those PRs definitely contributed to me missing this and back end fixes were not really in scope (though in the process of doing those PRs I did actually resolve a lot of issues that weren’t strictly for those PRs to fix)

I admit to matching that front end precedent that was set back in 2018 for ranked choice without really thinking about it :sweat_smile:. (Which you guys just also followed, albeit knowingly :laughing:)

Amazing how workarounds can persist without the underlying issue being identified.

3 Likes