Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(172)

Issue 123044: [SHINDIG-1180] makeRequest does not properly handle server errors, plus standardizing error handling

Can't Edit
Can't Publish+Mail
Start Review
Created:
16 years, 4 months ago by Jon Weygandt
Modified:
11 years, 2 months ago
Reviewers:
Base URL:
http://svn.apache.org/repos/asf/incubator/shindig/trunk/
Visibility:
Public.

Description

When doing a makeRequest, if the remote server returns an error it is not properly handled. The XMLHttpRequest has a status code of 200, and data is present in the "don't be evil" json object. "rc" is set to an error code, and text is set to some value (generally an HTML page) returned by the server. Inside of io.js the overall 200 code passes through hadError; in transformResponseData, errors is set to [] and rc is set to the value. Then it tries to parse the text, which is probably not JSON and throws an exception. To make matters worse, there is no real documented standard for the behavior of makeRequest. See the thread: http://groups.google.com/group/opensocial-and-gadgets-spec/browse_thread/thread/60f73b51799fd315# . This patch *) fixes the breakage in transformResponseData, *) cleans up all the error handling so it conforms with the OpenSocial thread. *) The errors array is now "<code> <msg>", rather than "<msg> <code>" per open social article. It should be noted that the discussion proposed a more human readable form, that might eliminate the <code> from the message. I'll change it again, if that happens. *) fixes jsonrpccontainer.js so that it uses rc rather than errors. *) fixes makeRequestText.xml (the numeric assertEquals test matches the comment string!) *) updates iotests.js to properly deliver rc in the tests, plus add a test to demonstrate the original bug. This change could cause issues with gadget authors, simply because there was no real specification. As long as users were using simple techniques, like ignoring errors, looking at the errors array as human readable string, looking at rc when present, try/catch things are OK. If like jsonrpccontainer.js, they depended upon the human readable string in errors to be of a specific format, they will break with this change.

Patch Set 1 #

Patch Set 2 : Small changes to superseede the original patch set #

Unified diffs Side-by-side diffs Delta from patch set Stats (+114 lines, -45 lines) Patch
features/src/main/javascript/features/core.io/io.js View 1 6 chunks +19 lines, -9 lines 0 comments Download
features/src/main/javascript/features/opensocial-jsonrpc/jsonrpccontainer.js View 4 chunks +10 lines, -10 lines 0 comments Download
features/src/test/javascript/features/core.io/iotest.js View 1 28 chunks +84 lines, -25 lines 0 comments Download
java/server/src/test/resources/endtoend/makeRequestTest.xml View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 2
Jon Weygandt
16 years, 4 months ago (2009-09-23 21:39:21 UTC) #1
Jon Weygandt
16 years, 4 months ago (2009-09-24 16:39:14 UTC) #2
Small changes to superseede the original patch set
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b