Closed Bug 966277 Opened 11 years ago Closed 11 years ago

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

Categories

(Bugzilla :: WebService, enhancement)

Product:
Bugzilla
Component:
WebService
Version:
4.5.1
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 5.0

People

(Reporter: emorley, Assigned: emorley)

References

Details

Attachments

(1 file)

Patch v1
(deleted), patch
dkl
: review+
Details | Diff | Splinter Review
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 v1 (deleted) — — Splinter 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: 11 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

Creator:
Created:
Updated:
Size: