Page MenuHomePhabricator
Create Task
Maniphest T295604

Kartographer mapdata API should return an empty result when the `mpdgroups` parameter is not matched
Open, LowPublic2 Estimated Story Points
Actions

Description

Although this an exceptional condition and it's not supposed to happen unless there's a problem with the software (for example unversioned requests made against a revision-stabilized page), the standard for MediaWiki APIs is to treat the parameter like a failed filter and return an empty result set. Currently the API returns the requested key with a null value.

Example of a such a bogus query vs. how it's supposed to look like.

"mapdata": [
    "{\"_ca5fe2a1449687fe54ae1fdbde7b637cd662d3b7\":null}"
]
Suggestion #1
"mapdata": []
Suggestion #2
"mapdata": [
    "{\"_ca5fe2a1449687fe54ae1fdbde7b637cd662d3b7\":[]}"
]

Make sure that consumers can handle the new response.

Details

SubjectRepoBranchLines +/-
[WIP] Return an empty result for missing groupsmediawiki/extensions/Kartographermaster+7 -7
Customize query in gerrit

Related Objects
Search...

Event Timeline

awight created this task.Nov 12 2021, 2:41 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 12 2021, 2:41 PM
awight mentioned this in T295363: Explore mapdata API log to estimate prevalence of maps and usage.Nov 12 2021, 2:41 PM
awight mentioned this in T293905: Performance review of proposed Kartographer/Kartotherian revid support.Dec 9 2021, 10:39 AM
thiemowmde added a subtask: T295050: Investigate mapdata API caching.Feb 2 2022, 9:46 AM
thiemowmde moved this task from Backlog to In sprint on the WMDE-GeoInfo-FocusArea board.
thiemowmde added a project: WMDE-TechWish-Sprint-2022-02-02.
thiemowmde subscribed.Feb 2 2022, 9:53 AM

We pulled this into our current WMDE-TechWish-Sprint-2022-02-02 to move this forward, investigate, and come up with concrete proposals. But it doesn't need to be 100% resolved in this sprint.

awight moved this task from Sprint Backlog to Doing on the WMDE-TechWish-Sprint-2022-02-02 board.Feb 3 2022, 9:59 AM
awight moved this task from Doing to Sprint Backlog on the WMDE-TechWish-Sprint-2022-02-02 board.
thiemowmde added a project: WMDE-TechWish-Maintenance.Feb 4 2022, 3:17 PM
thiemowmde moved this task from Incoming to In progress on the WMDE-TechWish-Maintenance board.Feb 7 2022, 9:57 AM
awight triaged this task as Low priority.Feb 9 2022, 1:32 PM
awight moved this task from In progress to Backlog on the WMDE-TechWish-Maintenance board.
awight removed a project: WMDE-TechWish-Sprint-2022-02-02.

I'm finding several cases to consider:

  • titles refers to a page that doesn't exist
    • Returned page includes the missing key.
  • revids refers to a revision that doesn't exist
    • Comes back in query.badrevids.NNN.
    • Status 200.
  • titles or revids refers to an entity with no mapdata.
    • With or without the mpdgroups parameter, the result simply doesn't include mapdata.
  • The entity contains mapdata, but not the requested mpdgroups.
    • The result includes mapdata, but the value is null.

We can't change the first three cases, the behavior is already consistent with other APIs such as action=query&prop=images.

We can focus on the last case because it corresponds to the actual queries from Kartotherian. As much as I'd like to return an error as this task requests, it seems inconsistent with other APIs. For example, this query filters all templates transcluded on Main Page, for which the template is in the main namespace. It will usually return empty, which looks like our third case, for a page which doesn't contain mapdata. This result is cached the same as other API responses, so if such a template is added to the Main Page it might not show up in the API for some amount of time (e.g. 15 minutes).

I propose that we make the API consistent by returning an empty result when the mpdgroups don't match the page. We won't set any special cache headers or error codes. This isn't particularly useful to us, just a minor clean-up, so I'll deprioritize the task.

awight renamed this task from Kartographer mapdata API should return an API error when a group is not found to Kartographer mapdata API should return an empty result when the `mpdgroups` parameter is not matched.Feb 9 2022, 1:34 PM
awight updated the task description. (Show Details)
awight set the point value for this task to 2.
gerritbot added a comment.Feb 9 2022, 1:40 PM

Change 761327 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Kartographer@master] [WIP] Return an empty result for missing groups

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

gerritbot added a project: Patch-For-Review.Feb 9 2022, 1:40 PM
thiemowmde closed subtask T295050: Investigate mapdata API caching as Resolved.Feb 15 2022, 9:35 AM
thiemowmde moved this task from In sprint to Ready for story time on the WMDE-GeoInfo-FocusArea board.May 4 2022, 9:57 AM
thiemowmde added a comment.May 5 2022, 1:22 PM

Question: The MediaWiki API does have a way to report errors, even in localized form. Does this already happen in the cases this ticket is about? If not we should first make sure this happens, before making changes to the actual result part of the API response.

awight added a comment.May 5 2022, 2:19 PM
In T295604#7906744, @thiemowmde wrote:

Question: The MediaWiki API does have a way to report errors, even in localized form. Does this already happen in the cases this ticket is about? If not we should first make sure this happens, before making changes to the actual result part of the API response.

I gave some thought to that in T295604#7696841 and an error doesn't seem like the right way to handle this case. Here are some examples illustrating how missing pages or properties are handled now:

It seems better if a missing mapdata group returns a result similar to the latter, above.

thiemowmde added a comment.May 5 2022, 2:50 PM

My argument was more like that we should probably do both: Communicate the reason with an error message like here. It seems like this should be possible without risk. Then do what this ticket suggests.

awight added a comment.EditedMay 5 2022, 2:54 PM

I like the idea of sending a warning. This isn't quite an error, but certainly an unusual scenario that should cause a tool author to take action.

thiemowmde updated the task description. (Show Details)May 5 2022, 4:57 PM
Lena_WMDE moved this task from Ready for story time to Backlog on the WMDE-GeoInfo-FocusArea board.Aug 16 2022, 10:01 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL