New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Failing requests are not handled properly (jsoncallback, Content-Type, HTTP code) #1372
Comments
The jsoncallback parameter is taken into account within the rendering mechanism (render()) of the JSON renderer in /core/DataTable/Renderer/Json.php#41 :
The renderer mechanism is called by method getResponse() from /core/API/ResponseBuilder.php#55. When an exception occurs, the API controller /core/API/Request.php#111 does not call getResponse() from /core/API/ResponseBuilder.php#55. Instead, it calls getResponseException(). Method getResponseException() from /core/API/ResponseBuilder.php#105 does not use the rendering mechanism implemented by the json renderer. Instead, it uses an ad-hoc output string to build the exception result, /core/API/ResponseBuilder.php#121:
Proposal 1: A quick fix I don't recommend is to add the jsoncallback parameter logic inside the case 'json' of method getResponseException(). Proposal 2: Exception rendering shouldn't be performed at a generic level (ie. in /core/API/ResponseBuilder.php. Exception rendering should be performed by each specific renderer to meet each format requirements. Therefore, the abstract class /core/DataTable/Renderer.php should define another abstract method :
The exception message could be provided to the renderer when calling renderException($exception) or a setter function could be added to the renderer:
In /core/API/ResponseBuilder.php#105 the method getResponseException() would call a new protected function :
Method renderException($exception) would be called from there. With this solution, each renderer manages the way exceptions are sent back to the browser. |
Agree with proposal 2. |
Please advise: For renders, I would normally use a template to separate presentation from logic. If agreed, would /core/DataTable/Renderer/templates be a suitable place for them ? |
Julien, thanks for your detailed analysis. The generic handling was what I suspected. I agree with your 2nd proposal as well. Andr |
Here is a patch proposal without templates. Comments:
in each specific render class instead of placing it at a generic level. I suggest this in case one day we develop a render which does not require html entity translation.
This happens because of /core/API/ResponseBuilder.php#110:
I have kept this behavior in the suggested patch.
Its purpose is to handle the case when the provided format is invalid even though the format exception has already been raised. Because all exceptions are instances of Exception I can not distinguish if the exception has been raised because of an invalid format or invalid method. This is the reason why I need to raise the format exception again.
|
Attachment: |
Looks good! code review:
|
If the special case for php would be removed from getResponseException, it means that the php renderer would have to implement the logic of caseRendererPHPSerialize. Currently, it's the /core/API/ResponseBuilder.php who sets the result of caseRendererPHPSerialize to the renderer. I agree with removing special cases but shouldn't it be done for getResponse also ? |
|
Explanation:
The lign:
Should directly output the error message. Currently it is not the case, it output a complicated object. E.g:
|
Attachment: |
There are 62 tests failing with this patch. E.g:
It occurs because I have modified the return http code from 200 to 400 in ResponseBuilder.php (method is : getResponseException() and renderBadRequest()) |
Sorry for the multiple updates. This concerns exception handling while using jsonp. With jsonp method, there is no way to know the http return code. With my proposed patch there are two ways to go from here.
Please advise on what's best. (In the patch I just submitted I forgot to add quotes around the returned string) |
Attachment: |
Replying to JulienM:
otherwise, looks good to me |
New patch keep original json exception message format and don't override HTTP return code. |
Attachment: |
(In [2294]) Fixes #1372 Thanks JulienM for patch
|
I forgot to remove quotes in the json exception message. In trunk, if you try for example to add a user with an already existing mail, there is a json parse error and the loading div doesn't show any message. |
Attachment: |
(In [2301]) Fixes #1372 |
There's one file that has not been updated I don't know if it is on purpose. Here is the patch : Tsv.php.patch |
Attachment: |
on purpose: Tsv class extends Csv |
Hi,
I'm trying to access the API from jQuery using JSONP. I've encountered three issues with failing requests:
With the current implementation of Piwik, succeeding requests are wrapped around the requested
jsoncallback
while failing requests are not.Failures are returned to the client with a Content-Type of
application/json
while others are returned astext/html
.Both kinds of requests are reported as being "HTTP/1.1 200 OK" which is unfortunate, to say the least.
Compare http://piwik.org/demo/index.php?module=API&method=SitesManager.getSitesWithViewAccess&format=JSON&jsoncallback=a
with http://piwik.org/demo/index.php?module=API&method=itesManager.getSitesWithViewAccess&format=JSON&jsoncallback=a (note the missing S in the method name for the second request). The first response is wrapped in a() while the second is not.
I'd have expected Piwik to respond to both requests with a wrapped function call.
This makes it impossible to cope with errors on the client and show messages to users. The way it is implemented now raises "invalid label" errors in Firefox and silent failures in other browsers because of the missing
jsoncallback
.While the inappropriate status code is not that bad (parse the response as a work-around), the missing jsoncallback is a real blocker for me.
The text was updated successfully, but these errors were encountered: