Last modified: 2014-09-24 01:10:31 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 31901 - API: action=query with empty title set should still respond with "query" element
API: action=query with empty title set should still respond with "query" element
Status: PATCH_TO_REVIEW
Product: MediaWiki
Classification: Unclassified
API (Other open bugs)
unspecified
All All
: Lowest major (vote)
: Future release
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: api-2.0
  Show dependency treegraph
 
Reported: 2011-10-23 11:50 UTC by Bergi
Modified: 2014-09-24 01:10 UTC (History)
12 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
proposed patch (1.23 KB, patch)
2011-11-01 00:12 UTC, Bergi
Details

Description Bergi 2011-10-23 11:50:32 UTC
When I try to get info for a set of pages specified by a generator and that generator returns nothing, I'd expect to get an empty set. But you just get no set!
Example:
http://de.wikipedia.org/w/api.php?action=query&list=allpages&apprefix=DoesNotExist
<api>
  <query>
    <allpages />
  </query>
</api>
http://de.wikipedia.org/w/api.php?action=query&prop=revisions&generator=allpages&gapprefix=DoesNotExist
Actual result:
<api />
But Expected:
<api>
  <query>
    <pages />
  </query>
</api>

Or is it a feature? I think its hard to detect when parsing the result, because you usually step through these elements when trying to get your pageset. Then you never know whether its an error in your request, an server error (api error) or its just empty results.
Comment 1 Mark A. Hershberger 2011-10-24 17:04:01 UTC
(In reply to comment #0)
> When I try to get info for a set of pages specified by a generator and that
> generator returns nothing, I'd expect to get an empty set. But you just get no
> set!

> Actual result:
> <api />
> But Expected:
> <api>
>   <query>
>     <pages />
>   </query>
> </api>

Looks like you just need to verify that the <query> element exists.  <api/> isn't the same result you would get if there were an error, is it?
Comment 2 Bergi 2011-10-24 17:24:48 UTC
Yes, of course I can look up whether the <query> element exists. But why do I have to? From my point of view there is always a _query_ result, even if it just says "no pages available".
It is as like as retrieving a list (e.g. the allpages request from #0), I will get an empty element there instead of none.
My propose:
# When I have an "action=query" url, there should always be a <query> element.
# When I retrieve any property (prop=xyz), there should always be a <pages> element.
Comment 3 Mark A. Hershberger 2011-10-24 20:17:33 UTC
Reopening, patches welcome!
Comment 4 Bergi 2011-11-01 00:12:55 UTC
Created attachment 9336 [details]
proposed patch

That was quite easy, I think. Or have I missed something? Should have only been that one if-condition.
Comment 5 Brion Vibber 2011-11-03 22:37:47 UTC
Roan, can you take a peek at this patch and confirm it would not cause compatibility problems?
Comment 6 Bergi 2011-11-03 23:17:04 UTC
(In reply to comment #5)
> compatibility problems?

I think compatibility problems mostly are on client side. Implementation which distingish whether there are pages returned or not may get problems.
Until now the function to determine this looked up if there is the <query> element and if there is the <pages> element, else it returns false. But from now on this function will always return true, so it could happen that another function which expects a set of pages is called with a empty set of pages, instead of not to be called.

An example could be line 768 in /pywikipedia/trunk/pywikipedia/wikipedia.py:
if not 'pages' in data['query']:
    raise RuntimeError("API query error, no pages found")
(but it didn't check for >'query' in data< before - anyway, I'm no pywikipedia expert)
Comment 7 Roan Kattouw 2011-11-08 10:45:49 UTC
(In reply to comment #5)
> Roan, can you take a peek at this patch and confirm it would not cause
> compatibility problems?
Compat problems are entirely on the client side, per comment #6, and it's far from crazy to suspect people wrote code that relies on this specific output format. For that reason, this would probably have to be announced as a breaking change, and I'm very wary of making breaking changes (or changes that could be breaking) for aesthetic reasons.
Comment 8 Bryan Tong Minh 2011-11-08 11:30:59 UTC
I'd say no.
Comment 9 Bergi 2011-11-08 12:05:11 UTC
(In reply to comment #7)
> I'm very wary of making breaking changes (or changes that could be
> breaking) for aesthetic reasons.

I can understand that, but I think it's not only for aesthetic reasons. The convention to provide no <pages> element when there are none might be OK, but the missing <query> element only leads to workarounds - and so should be implemented cleaner.
When you query more than only properties (eg. prop and meta and list) there also always will be the <query> element, but maybe no <pages> element. So every implementation would have to check for !query || !pages, and I think it wouldn't cause any problems at least to provide the <query> element every time. What about that?
Comment 10 Mark A. Hershberger 2011-11-08 14:56:52 UTC
Looks like Roan and Bryan are saying WONTFIX even with a patch, right?
(I'll let them set it if they are.)
Comment 11 Sumana Harihareswara 2011-11-09 02:45:55 UTC
(adding "patch" keyword for consistency)
Comment 12 Sumana Harihareswara 2012-05-22 13:49:26 UTC
Bergi, if you believe this is still a problem, maybe you could use Developer access https://www.mediawiki.org/wiki/Developer_access to submit your patch directly into Git and Gerrit, so it'll get reviewed faster.  Thanks.
Comment 13 Max Semenik 2012-05-25 20:04:26 UTC
I concur with Roan and Bryan, b/c sucks but breaking it sucks even more.
Comment 14 Yuri Astrakhan 2013-01-30 21:42:14 UTC
Part of APIv2. See
http://www.mediawiki.org/wiki/Requests_for_comment/API_Future#Cleanup_2 :

* Move all items from 'query' to root in the result. The 'pages', 'normalized', 'continue', and all list/meta elements will be under the root element.
Comment 15 Andre Klapper 2014-02-28 18:20:49 UTC
(In reply to Bergi from comment #4)
> Created attachment 9336 [details]
> proposed patch

Tried to put the patch attached here into Gerrit via http://tools.wmflabs.org/gerrit-patch-uploader/ but neither "git apply < patch" nor "patch -p0 < patch" nor "patch -p1 < patch" worked. Needs rework; setting "patch-reviewed" status.
Comment 16 Krinkle 2014-04-01 04:11:50 UTC
Ran into this again. Fixed broken request handler in https://en.wiktionary.org/w/index.php?title=MediaWiki%3AGadget-DocTabs.js&diff=prev&oldid=26015877.
Comment 17 Gerrit Notification Bot 2014-04-01 04:17:07 UTC
Change 122639 had a related patch set uploaded by Krinkle:
ApiQuery: Respond with "query/pages" even if there are 0 titles

https://gerrit.wikimedia.org/r/122639
Comment 18 Krinkle 2014-04-01 04:21:02 UTC
(In reply to Roan Kattouw from comment #7)
> (In reply to comment #5)
> > Roan, can you take a peek at this patch and confirm it would not cause
> > compatibility problems?
> Compat problems are entirely on the client side, per comment #6, and it's
> far from crazy to suspect people wrote code that relies on this specific
> output format. For that reason, this would probably have to be announced as
> a breaking change, and I'm very wary of making breaking changes (or changes
> that could be breaking) for aesthetic reasons.

(In reply to Max Semenik from comment #13)
> I concur with Roan and Bryan, b/c sucks but breaking it sucks even more.

How would ensuring there is always query/pages a breaking change?

I can imagine a client not explicitly checking the presence of every property (is an API after all, the point is to not have to double check every thing, the root structure should always be in place), in which case this patch fixes that.

But I can't imagine explicitly checking and relying on the the absence of the structure. Roan/Bryan/Max: Please provide a little more details on that use case, this doesn't sound realistic to me and not a breaking change.
Comment 19 Brad Jorsch 2014-04-01 14:25:22 UTC
(In reply to Krinkle from comment #18)
> But I can't imagine explicitly checking and relying on the the absence of
> the structure. Roan/Bryan/Max: Please provide a little more details on that
> use case, this doesn't sound realistic to me and not a breaking change.

In some sort of PHP-ish pseudocode:

 $result = $api->query( array( 'generator' => ... ) );
 if ( !isset( $result['query']['pages'] ) ) {
     // Generator returned no pages, handle that
 } else {
     // Do something with the pages
 }

Yeah, that's a bit dumb. But people do dumb stuff sometimes, particularly when encouraged by the API representing booleans as present/absent rather than as booleans.
Comment 20 Gerrit Notification Bot 2014-04-30 15:44:58 UTC
Change 122639 abandoned by Krinkle:
ApiQuery: Respond with "query/pages" even if there are 0 titles

https://gerrit.wikimedia.org/r/122639
Comment 21 Derk-Jan Hartman 2014-05-19 22:04:22 UTC
Ran into this with popups...

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links