Page MenuHomePhabricator

Migrate usage of Database::select to SelectQueryBuilder in CheckUser's CompareService
Closed, ResolvedPublic3 Estimated Story Points

Description

See the parent task T312479: Migrate usage of Database::select to SelectQueryBuilder in CheckUser for details.

Acceptance criteria
  • SelectQueryBuilder is used where possible in CompareService
Notes

For more information check T243051 and its documentation.

Event Timeline

Change 826954 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/CheckUser@master] Use SelectQueryBuilder in CompareService

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

Tchanders set the point value for this task to 3.Aug 30 2022, 5:26 PM
Testing notes

We probably just need to do some regression testing on CheckUser. You can follow the documentation here: https://meta.wikimedia.org/wiki/Help:Special_Investigate. You will need to give your user the "checkuser" right.

Testing done locally and also ran through the relevant steps in the help page as suggested to test. Have therefore +2'ed.

Change 826954 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Use SelectQueryBuilder in CompareService

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

I compared the SQL queries which were being run on the "IPs & User agents" tab before and after this change. I did this on both a server running SQLite and MySQL (as there can be differences in queries ran on either database). I noticed a few differences which I will note below for @Tchanders (or another developer) to review:

getTotalEditsFromIp

Before:

[DBQuery] MediaWiki\CheckUser\Investigate\Services\CompareService::getTotalEditsFromIp [0s] unknown: SELECT  COUNT(*) AS "rowcount"  FROM (SELECT  1  FROM cu_changes    WHERE cuc_ip_hex = '7F000001' AND cuc_type IN (0,1)   ) "tmp_count"

After:

[DBQuery] MediaWiki\CheckUser\Investigate\Services\CompareService::getTotalEditsFromIp [0s] unknown: SELECT  COUNT(*) AS "rowcount"  FROM (SELECT  1  FROM cu_changes    WHERE cuc_ip_hex = '7F000001' AND cuc_type IN (0,1)  AND (cuc_id IS NOT NULL)  ) "tmp_count"

We now have an additional AND (cuc_id IS NOT NULL). I assume this is benign.

getTargetsOverLimit

(Note, this query does not run on SQLite)

Before:

[DBQuery] MediaWiki\CheckUser\Investigate\Services\CompareService::getTargetsOverLimit [0.001s] localhost: SELECT  cuc_id,cuc_user,cuc_user_text,cuc_ip,cuc_ip_hex,cuc_agent,cuc_timestamp  FROM `cu_changes`    WHERE cuc_user = 1 AND cuc_type IN (0,1,3)   LIMIT 5000,1

After:

[DBQuery] MediaWiki\CheckUser\Investigate\Services\CompareService::getTargetsOverLimit [0.001s] localhost: SELECT  COUNT(*) AS `rowcount`  FROM (SELECT  1  FROM `cu_changes`    WHERE cuc_user = 1 AND cuc_type IN (0,1,3)  AND (cuc_id IS NOT NULL)  LIMIT 5000,1  ) `tmp_count`

Test environments:

  • local docker (SQLite) CheckUser 2.5 (aea1ef3) 06:33, 20 September 2022.
  • local bare-metal (MySQL) CheckUser 2.5 (f595564) 07:28, 21 September 2022.

Thanks for checking these @dom_walden - they're both fine. Although the second one has changed a bit, the new query is more lightweight and all we need for this situation - should probably have mentioned the change in the commit message.

I also tested this query again by lowering the limit set via $wgCheckUserInvestigateMaximumRowCount to lower than the number of total edits presented in the table on the IPs & User agents tab, and ensuring same error is triggered before and after this patch.

Thanks for checking these @dom_walden - they're both fine. Although the second one has changed a bit, the new query is more lightweight and all we need for this situation - should probably have mentioned the change in the commit message.

I also tested this query again by lowering the limit set via $wgCheckUserInvestigateMaximumRowCount to lower than the number of total edits presented in the table on the IPs & User agents tab, and ensuring same error is triggered before and after this patch.

Thanks! Moving along.

@dom_walden does this ticket being in the Done section mean this task can be closed as resolved?

@dom_walden does this ticket being in the Done section mean this task can be closed as resolved?

It can be. I normally leave it for the Product Manager (@Niharika) to resolve it.

I'll resolve this then.