Page MenuHomePhabricator

TemporaryAccountHandler::getData throws a database error when using a postgres DB
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • Run experimental tests, which include the PHPUnit tests run using a postgres DB

What happens?:
The postgres tests fail. Some of these failures from TemporaryAccountHandler::getData. Example:

1) MediaWiki\CheckUser\Tests\Integration\Api\Rest\Handler\TemporaryAccountHandlerTest::testExecute
13:25:57 Wikimedia\Rdbms\DBQueryError: Error 42P10: ERROR:  for SELECT DISTINCT, ORDER BY expressions must appear in select list
13:25:57 LINE 1: ...t_cu_changes"    WHERE cuc_actor = 1234  ORDER BY cuc_actor ...
13:25:57                                                              ^
13:25:57 
13:25:57 Function: MediaWiki\CheckUser\Api\Rest\Handler\TemporaryAccountHandler::getData
13:25:57 Query: SELECT DISTINCT cuc_ip AS "value"  FROM "unittest_cu_changes"    WHERE cuc_actor = 1234  ORDER BY cuc_actor DESC,cuc_ip DESC,cuc_timestamp DESC LIMIT 5000

What should have happened?
The tests should not have failed in this way.

Other information
The tests failing indicate that the query being run will not work on a postgres DB. I have not been able to test it myself on a postgres DB.

This might have to be something that is addressed in the SelectQueryBuilder that constructs the query, but in any case this should be fixed.

Event Timeline

As this change that has these tests was recently made by @Tchanders I've added them as a subscriber.

Dreamy_Jazz renamed this task from TemporaryAccountHandler::getData fails throws a database error when using a postgres DB to TemporaryAccountHandler::getData throws a database error when using a postgres DB.Jan 25 2023, 3:59 PM

To make the query work in Postgres will require DISTINCT ON expression (instead of DISTINCT) which SelectQueryBuilder does not seem to support. Alternately, I think you need to grab all the simple select result and manually filter unique values.

Change 883851 had a related patch set uploaded (by Ammarpad; author: Ammarpad):

[mediawiki/extensions/CheckUser@master] Rest: Make TemporaryAccountHandler query work with Postgres

https://gerrit.wikimedia.org/r/883851

Change 883851 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Rest: Make TemporaryAccountHandler query work with Postgres

https://gerrit.wikimedia.org/r/883851

Ran experimental tests on a change that had no DB changes and the tests passed for postgres. As such, this is resolved.