Page MenuHomePhabricator

API calls ignoring cmendsortkeyprefix parameter when continue parameter is present
Open, LowPublic

Description

When listing a subset of pages in a category using list=categorymembers and the cmstartsortkeyprefix and cmendsortkeyprefix parameters behaviour is as expected if all the requested pages are returned without a continuation call. e.g. this sandbox request for all the Gastropods described in 1999 that have a sortkey prefix of H.

https://en.wikipedia.org/wiki/Special:ApiSandbox#action=query&format=json&list=categorymembers&cmtitle=Category%3AGastropods_described_in_1999&cmtype=page&cmlimit=500&cmstartsortkeyprefix=H&cmendsortkeyprefix=I

If cmlimit is reduced below the number of pages that should be returned, so that a continue loop is required, the API returns pages and a continue value in the response until the end of the category is reached.

e.g. this sandbox request https://en.wikipedia.org/wiki/Special:ApiSandbox#action=query&format=json&list=categorymembers&cmtitle=Category%3AGastropods_described_in_1999&cmtype=page&cmlimit=20&cmstartsortkeyprefix=H&cmendsortkeyprefix=I

As expected, the sandbox shows a continue button, but when it is pressed results are returned that go beyond the requested range.

The attached python file shows that same test case in python code.

I have found this issue also affects the gcmendsortkeyprefix parameter when categorymembers is used as a generator.

Event Timeline

WDoranWMF added a project: good first task.

@WDoranWMF @Anomie Hi! As this issue is filed as a good first task, if you could add to the task description, some hints/ideas for implementing the solution, that might make it easier for a new contributor to get started!

Can I start working on this issue and I will try to raise a PR for this as soon I get some idea on how to approach this.

The relevant code is in ApiQueryCategoryMembers.php, lines 127–177. The code needs to be reworked so that the handling of parameters in the "else" branch beginning on line 147 is done unconditionally.

Change 575398 had a related patch set uploaded (by Scheleon; owner: Scheleon):
[mediawiki/core@master] Api: Populate variables startsortkey and endsortkey before conditional check

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

@Anomie I have uploaded a patch, but have not tested it yet. Can you please confirm whether the changes are as requested?
Will test and make another commit by the end of today.

Okay so, I ran phpunit tests using "php tests/phpunit/phpunit.php", although I got some warnings but no errors.

@WDoranWMF: Not sure if I'm supposed to move this out of CPT's "icebox". :) Could this patch by a new contributor please receive a review? Thanks in advance!

@Aklapper: If a patch is submitted and needs review from Core Platform Team, please do move it into the Inbox column on Platform Engineering for us to re-triage. We will then move it from there as appropriate.

(I'll personally get to reviewing the patch here soon; I'm still catching up from internal politics not allowing me to do most reviews for a few weeks)

@Aklapper: If a patch is submitted and needs review from Core Platform Team, please do move it into the Inbox column on Platform Engineering for us to re-triage. We will then move it from there as appropriate.

@Anomie: Hi, I cannot track all patch activity, and humans trying to remember individual team requests or custom workflows does not scale.
I'd expect CPT to monitor its tasks (and check for tasks with Patch-For-Review), and/or to request a Herald rule to (re)tag a task as Platform Engineering when Patch-For-Review gets added to a task with another (!) CPT tag (if that's wanted), or to check out column triggers if under the same project tag (if that's wanted).

You said in T245310#5991274 that you weren't sure if you were supposed to move it out of CPT's "icebox" when a patch needed review, I was telling you that you can and should.

I didn't mean to imply that you should be finding every such task, sorry for the misunderstanding.

@Anomie: Ah, I'm also sorry that I got it wrong! Thanks :)

Change 589134 had a related patch set uploaded (by Scheleon; owner: Scheleon):
[mediawiki/core@master] Api: Populate variables startsortkey and endsortkey before conditional check

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

What is "external code reviews"? CPT performing code review for projects not under CPT's stewardship? CPT not performing code reviews for projects under CPT's stewardship because someone not in CPT (external) already does? Something else? Could you please add a description to https://phabricator.wikimedia.org/project/profile/4614/ (and other boards) which explains what that board is about? Thanks a lot in advance for allowing to understand CPT boards! There are many which are a mystery to me and have no description.

Moved to T253036

Change 589134 abandoned by Scheleon:
Api: Populate variables startsortkey and endsortkey before conditional check

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

Change 589134 restored by Scheleon:
[mediawiki/core@master] Api: Populate variables startsortkey and endsortkey before conditional check

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

Change 589134 abandoned by Scheleon:
[mediawiki/core@master] Api: Populate variables startsortkey and endsortkey before conditional check

Reason:

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

Removing good first task as there's not much sense in advertising tasks when WMF does not review patches.

Removing task assignee due to inactivity as this open task has been assigned for more than two years. See the email sent to the task assignee on August 22nd, 2022.
Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome!
If this task has been resolved in the meantime, or should not be worked on ("declined"), please update its task status via "Add Action… 🡒 Change Status".
Also see https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator. Thanks!