/polls/voters.json возвращает дубликаты пользователей при постраничных запросах

Внести вклад Ошибка
, ,
1

При вызове конечной точки /polls/voters.json с использованием пагинации мы обнаружили, что первый запрос возвращает 25 пользователей, как и ожидалось, но вторая страница возвращает 26 пользователей, один из которых уже был возвращён в первом запросе. Это происходит всегда, когда пользователей больше 25.

Проблема находится в плагине опросов для Discourse:

Смещение рассчитывается с предположением, что в PostgreSQL оператор BETWEEN с границами по строкам является исключающим, тогда как он инклюзивный.

Первый запрос фактически возвращает 25 записей, потому что рассчитанные строки на самом деле начинаются с 1, а не с 0.

Предлагаемое мною исправление предельно простое:

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

Или более элегантное решение — использовать LIMIT и OFFSET в PostgreSQL:

    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 лайка
2

Отлично подмечено! Добавляю тег pr-welcome, на случай если кто-то захочет взяться за исправление.

Нам нужно убедиться, что мы не нарушим существующую функциональность опросов, на случай если фронтенд ошибочно полагается на неверные предположения здесь.

1 лайк
3

Спасибо за быстрый ответ!
Я попробую настроить окружение, не хотелось бы создавать PR без тестирования. Кто угодно, пожалуйста, делайте это быстрее меня. Я вообще не разработчик на Ruby. Также я не знаком с изменением спецификаций или тестов.

Смешно, но в UI есть код, который выгружает результаты в множество, чтобы обойти эту ошибку.

2 лайка
4

Для всех остальных, кто просматривает, мы внедрили аналогичное исправление интерфейса для нашего программного обеспечения для розыгрышей здесь.

Благодарим @lukecyca из VHS за выявление проблемной области.

5

Также есть связанный отчёт о загрузке дополнительных голосователей в опросах с января.

1 лайк
7

Отличное замечание.

Это даже смешнее, чем кажется. Я некоторое время назад отправил несколько очень больших PR, чтобы модернизировать фронтенд и добавить голосование по методу Ранжированного выбора.

Огромный масштаб этих PR, безусловно, способствовал тому, что я упустил это, а исправления на бэкенде не входили в задачи (хотя в процессе выполнения этих PR я действительно решил множество проблем, которые не были строго связаны с этими PR).

Я признаю, что без особого размышления последовал тому прецеденту во фронтенде, который был установлен ещё в 2018 году для Ранжированного выбора (FIX: Show every voter only once. by nbianca · Pull Request #6746 · discourse/discourse · GitHub). (Который вы, ребята, также только что повторили, хотя и осознанно :laughing:).

Удивительно, как обходные пути могут сохраняться, не будучи выявленными коренные проблемы.

3 лайка
8

Итак, я решил попробовать внести свой вклад в этот проект, но, похоже, всё окажется сложнее, чем я ожидал. :wink:

Я создал черновик PR: FIX: do not return duplicates from /polls/voters.json by clechasseur · Pull Request #1 · clechasseur/discourse · GitHub

В этом PR я сначала добавил тесты, воспроизводящие проблему, затем применил простое решение от Робa, и тесты начали проходить.

Также я попробовал более элегантное решение (то, которое мне больше понравилось), но, хотя оно и предотвращает дублирование голосующих, оно также меняет порядок возврата голосующих на разных страницах, включая первую. Это потенциально может считаться ломающим изменением (в зависимости от того, как с этим справляется фронтенд — я ещё не смотрел).

Однако, если отойти от деталей, меня остаётся вопрос: что на самом деле означает параметр limit при вызове этого эндпоинта? Он не ограничивает общее количество возвращаемых голосующих, а лишь количество голосующих для каждого варианта опроса. Этот эффект виден в тесте, который я добавил для опроса с множественным выбором здесь — первая страница действительно ограничена двумя голосующими на вариант, но в общей сложности возвращаются три разных голосующих (распределённых по вариантам). Переход к элегантному решению (то есть использование LIMIT :limit OFFSET :offset) приводит к тому, что limit применяется к общему количеству голосов, а не голосующих. Я не на 100% уверен, что это лучше или более интуитивно понятно.

В любом случае, я новичок в этом, поэтому, возможно, слишком сильно углубляюсь. Простое решение действительно убирает дубликаты голосующих и не создаёт слишком много проблем, поэтому, возможно, стоит выбрать его. Я подожду отзывов, прежде чем отправлять PR в основной репозиторий.

—

Кстати, я думаю, что в этой части кода есть ещё одна ошибка. Запрос для загрузки голосующих сортируется по digest, rank и username, но при сортировке по rank используется такое условие:

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

Однако ’Abstain’ на самом деле соответствует рангу 0, а не 1 — ранг 1 также может возвращаться как ’1’. Это потенциально делает сортировку недетерминированной между запросами, что означает, что в зависимости от количества голосующих и используемого значения limit при пагинированных запросах можно фактически пропустить некоторых голосующих. В моих новых тестах мне пришлось сортировать возвращённых голосующих, чтобы обойти недетерминированность. (Поскольку это недетерминировано, я предполагаю, что воспроизвести это в тесте непросто, но я могу попробовать…)

1 лайк
9

Я перешёл по ссылке выше на предыдущий отчёт, который содержит ссылку на предыдущий предыдущий отчёт, где также упоминается возможность, похожая на описанную:

Однако это было очень давно, и код в то время мог быть совершенно другим. (Я пытался посмотреть историю изменений через Git, но история обрывается в 2021 году, поэтому предполагаю, что код где-то был перемещён.)

10

После некоторых экспериментов мне не удалось создать тест, который бы вызвал проблему с возвращаемыми избирателями из-за недетерминированного порядка сортировки. Я полагаю, что это связано с тем, как сформирован запрос и как создаются голоса в опросе, но я не уверен на 100%.

Я создал PR с моим тестом и предлагаемыми исправлениями: FIX: Избежать возврата дублирующихся избирателей из эндпоинта `/polls/voters.json` by clechasseur · Pull Request #34433 · discourse/discourse

3 лайка
12

Огромное спасибо за исправление и за ваше терпение во время нашего ревью.

Я утвердил и слил его. :partying_face:

3 лайка
13

Эта тема была автоматически закрыта через 3 дня. Новые ответы больше не принимаются.