Closed Bug 966277 Opened 10 years ago Closed 10 years ago

Bugzilla native REST API should default to application/json if no Accept header was set

Categories

(Bugzilla :: WebService, enhancement)

4.5.1
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: emorley, Assigned: emorley)

References

Details

Attachments

(1 file)

In bug 930410 I was having issues getting results returned using the native REST API - a dump of the php file_get_contents response showed html markup wrapping the json. The same occurs using python's urllib2:

eg:

>>> oldAPI = "https://api-dev.bugzilla.mozilla.org/latest/bug?keywords=intermittent-failure&include_fields=id,summary,status,resolution&changed_after=-3m&summary=test_bug437844.xul"

>>> newAPI = "https://bugzilla.mozilla.org/rest/bug?keywords=intermittent-failure&keywords_type=allwords&chfieldfrom=-3m&chfieldto=Now&short_desc=test_bug437844.xul&short_desc_type=allwordssubstr&include_fields=id,summary,status,resolution&order=bug_status,bug_id"

>>> urllib2.urlopen(oldAPI).read()
'{"bugs":[{"status":"NEW","summary":"Intermittent test_bug437844.xul | Assertion count 4 is less than expected range 5-6 assertions.","resolution":"","id":934830},{"status":"RESOLVED","summary":"Intermittent timeout in test_bug437844.xul, followed by test_bug557987.xul | Correct number of command events received, and many more, roughly 218","resolution":"WORKSFORME","id":563922}]}'

>>> urllib2.urlopen(newAPI).read()
'<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"\n                      "http://www.w3.org/TR/html4/loose.dtd">\n<html>\n  <head>\n    <title>Bugzilla::REST::API</title>\n    <link href="https://bugzilla.mozilla.org/skins/standard/global.css?1383639130"\n          rel="stylesheet" type="text/css">\n  </head>\n  <body>\n    <pre>{\n   &quot;bugs&quot; : [\n      {\n         &quot;id&quot; : 934830,\n         &quot;resolution&quot; : &quot;&quot;,\n         &quot;status&quot; : &quot;NEW&quot;,\n         &quot;summary&quot; : &quot;Intermittent test_bug437844.xul | Assertion count 4 is less than expected range 5-6 assertions.&quot;\n      },\n      {\n         &quot;id&quot; : 563922,\n         &quot;resolution&quot; : &quot;WORKSFORME&quot;,\n         &quot;status&quot; : &quot;RESOLVED&quot;,\n         &quot;summary&quot; : &quot;Intermittent timeout in test_bug437844.xul, followed by test_bug557987.xul | Correct number of command events received, and many more, roughly 218&quot;\n      }\n   ]\n}\n</pre>\n  </body>\n</html>'

Further poking suggests that unlike BzAPI, the native REST API requires one to explicitly specify 'Accepts: application/json' otherwise you get the html response (including when the Accept request header is not specified at all, which is what happens with file_get_contents and urllib2.urlopen). BzAPI however returns html iff 'Accepts: text/html' (otherwise json), which seems like a more sensible default.

For bug 930410 I could make sure the get_file_contents call sets the header, but I imagine this will catch other consumers as well so would prefer we change the default when the Accept request header is not set.
No longer blocks: 866927
Depends on: 866927
Version: unspecified → 4.5.1
If the accepts header wasn't set, _get_content_prefs() returns '':
http://bzr.mozilla.org/bugzilla/trunk/view/head:/Bugzilla/WebService/Server/REST.pm#L463

So the scoring used by sort returns 0 each time, since the for loop was a no-op:
http://bzr.mozilla.org/bugzilla/trunk/view/head:/Bugzilla/WebService/Server/REST.pm#L450

So the REST_CONTENT_TYPE_WHITELIST list remains in it's original order:
http://bzr.mozilla.org/bugzilla/trunk/view/head:/Bugzilla/WebService/Constants.pm#L254

And we just pick the first item in the list:
http://bzr.mozilla.org/bugzilla/trunk/view/head:/Bugzilla/WebService/Server/REST.pm#L437

...which is 'text/html'.

Seems like we should:
1) Make application/json the first item in the whitelist.
2) (Optionally) Return early with the first item in the whitelist if the accepts header wasn't set (since the sort will always be a no-op), to make the behaviour more explicit.
Attached patch Patch v1Splinter Review
Patch against bugzilla trunk. 

Few notes:
* Re-orders the whitelist putting application/json first, since text/html shouldn't be default (IMO) and application/json is what the RFC states is preferred vs the other json formats [1].
* _get_content_prefs() now no longer appends '*/*' to the accept header values found. Browsers typically add '*/*' themselves [2], so the existing function was causing duplicates in the browser case and I think any fallback logic should be higher up - with _get_content_prefs() only returning what was explicitly found so consumers can differentiate between no header sent and '*/*' sent. (Side note: Also the comment just didn't make sense, so not sure what the intent was here - this code seems to have been copied from [3] but there is no more context there).
* Avoids the unnecessary sort if no accept header was found.
* Few comments to document the existing "default to the first item in the whitelist" behaviour more clearly.

There doesn't appear a way to get bzr to give more more context unless I build bzr trunk from source in order to use the newly added feature (!!) - sorry. There also appears no way to specify author info in the patch. I have to say my new-to-contributing experience with bzr was one of the worse I've experienced from a DVCS ("UnexpectedSmartServerResponse: Could not understand response from smart server" errors until I switched to using http) - I can't believe I'd ever say it, but roll on a project switching to git! :-)

[1] http://www.ietf.org/rfc/rfc4627.txt
[2] https://developer.mozilla.org/en-US/docs/HTTP/Content_negotiation#Default_values
[3] http://search.cpan.org/~moconnor/REST-Application-0.94/lib/REST/Application.pm ; source: http://search.cpan.org/src/MOCONNOR/REST-Application-0.94/lib/REST/Application.pm
Assignee: webservice → emorley
Status: NEW → ASSIGNED
Comment on attachment 8369381 [details] [diff] [review]
Patch v1

(Notes in previous comment, forgot the r? sorry!)
Attachment #8369381 - Flags: review?(glob)
Comment on attachment 8369381 [details] [diff] [review]
Patch v1

Review of attachment 8369381 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl
Attachment #8369381 - Flags: review+
Flags: approval?
Flags: approval? → approval+
Severity: normal → enhancement
Target Milestone: --- → Bugzilla 5.0
Attachment #8369381 - Flags: review?(glob)
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/Server/REST.pm
Committed revision 8907.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Thank you :-)

>>> import urllib2
>>> newAPI = "https://bugzilla.mozilla.org/rest/bug?keywords=intermittent-failure&keywords_type=allwords&chfieldfrom=-3m&chfieldto=Now&short_desc=test_bug437844.xul&short_desc_type=allwordssubstr&include_fields=id,summary,status,resolution&order=bug_status,bug_id"
>>> urllib2.urlopen(newAPI).read()
'{"bugs":[{"summary":"Intermittent test_bug437844.xul | Assertion count 4 is less than expected range 5-6 assertions.","status":"NEW","resolution":"","id":934830},{"summary":"Intermittent timeout in test_bug437844.xul, followed by test_bug557987.xul | Correct number of command events received, and many more, roughly 218","status":"RESOLVED","resolution":"WORKSFORME","id":563922}]}'
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: