Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#1372 closed Bug (fixed)

Failing requests are not handled properly (jsoncallback, Content-Type, HTTP code)

Reported by: awendt Owned by:
Priority: normal Milestone: Piwik 0.6.3
Component: Core Keywords:
Cc: Sensitive: no

Description

Hi,

I'm trying to access the API from jQuery using JSONP. I've encountered three issues with failing requests:

1) With the current implementation of Piwik, succeeding requests are wrapped around the requested jsoncallback while failing requests are not.

2) Failures are returned to the client with a Content-Type of application/json while others are returned as text/html.

3) 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.

Attachments (6)

1372.patch (8.0 KB) - added by JulienM 4 years ago.
1372.2.patch (9.0 KB) - added by JulienM 4 years ago.
1372.3.patch (9.0 KB) - added by JulienM 4 years ago.
#1372.patch (8.8 KB) - added by JulienM 4 years ago.
Json.php.patch (532 bytes) - added by JulienM 4 years ago.
Tsv.php.patch (448 bytes) - added by JulienM 4 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 4 years ago by vipsoft (robocoder)

  • Milestone set to 0 - Piwik 0.6.2
  • Priority changed from major to normal

comment:2 Changed 4 years ago by JulienM (JulienMoumne)

The jsoncallback parameter is taken into account within the rendering mechanism (render()) of the JSON renderer in /core/DataTable/Renderer/Json.php#41 :

		if(($jsonCallback = Piwik_Common::getRequestVar('jsoncallback', false)) !== false)
		{
			if(preg_match('/^[0-9a-zA-Z]*$/', $jsonCallback) > 0)
			{
				$str = $jsonCallback . "(" . $str . ")";
			}
		}

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:

			case 'json':
				@header( "Content-type: application/json" );
				// we remove the \n from the resulting string as this is not allowed in json
				$message = str_replace("\n","",$message);
				$return = '{"result":"error", "message":"'.$message.'"}';
			break;

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 :

abstract public function renderException();

The exception message could be provided to the renderer when calling renderException($exception) or a setter function could be added to the renderer:

public function setException($exception) {
    $this->exception = $exception;
}

In /core/API/ResponseBuilder.php#105 the method getResponseException() would call a new protected function :

protected function getRenderedException($exception)

Method renderException($exception) would be called from there.

With this solution, each renderer manages the way exceptions are sent back to the browser.

comment:3 Changed 4 years ago by vipsoft (robocoder)

Agree with proposal 2.

comment:4 Changed 4 years ago by JulienM (JulienMoumne)

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 ?

comment:5 Changed 4 years ago by awendt

Julien,

thanks for your detailed analysis. The generic handling was what I suspected. I agree with your 2nd proposal as well.

André

comment:6 Changed 4 years ago by JulienM (JulienMoumne)

Here is a patch proposal without templates.

Comments:

1) I placed the html entity translation :

htmlentities($this->exception->getMessage(), ENT_COMPAT, "UTF-8");

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.
Please advise if my assumption is incorrect.

2) Currently, if you use format 'original' with an incorrect method, the exception is not correctly sent back to the browser. If you go there : http://piwik.org/demo/index.php?module=API&method=itesManager.getSitesWithViewAccess&format=original you'll end up with:

Fatal error: Exception thrown without a stack frame in Unknown on line 0

This happens because of /core/API/ResponseBuilder.php#110:

case 'original':
    throw $e;

I have kept this behavior in the suggested patch.
Please advise if something should be done about it. Maybe a simple fix would do the trick:

case 'original':
    return $e->getMessage();

3) In the suggested patch, there is a try/catch in method getResponseException(Exception $e) of class Piwik_API_ResponseBuilder.

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.

4) After the fix, I have tested every format both with invalid and valid API calls. The only behavior that changes from the previous version is when the API call contains both an invalid method and an invalid format. Before the patch, it is the invalid method exception that is sent back to the browser. After the patch, it's the format exception which is raised.

Changed 4 years ago by JulienM (JulienMoumne)

comment:7 Changed 4 years ago by matt (mattab)

Looks good! code review:

  • could the code renderException() be refactored into a static helper method in the base class, to avoid copy & paste?
  • the line if($format=='php) could maybe be removed from the base renderer now that the logic is in the php renderer?

comment:8 Changed 4 years ago by JulienM (JulienMoumne)

  • Which part of the exception rendering would you like to refactor in the base class ?
  • Concerning special cases, in file /core/API/ResponseBuilder.php, if you look at method getRenderedDataTable (which is called when getResponse is called), there is also special cases for php, html and csv.

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 ?

comment:9 Changed 4 years ago by matt (mattab)

  • the duplicated code is {{{

$exceptionMessage = htmlentities($this->exception->getMessage(), ENT_COMPAT, "UTF-8"); }}}

  • agreed that special cases in getRenderedDataTable() should ideally also be done in each renderer somehow. but at least for now the special case in getResponseException() could be removed I believe

comment:10 Changed 4 years ago by JulienM (JulienMoumne)

  • I have refactored the html entity rendering in the abstract renderer class.
  • I have modified the http code when an exception is raised. It is now 400 Bad Request instead of 200 OK.
  • I have changed the content type of the json response to "Content-Type: application/json" for normal responses and error responses.
  • I have also changed the format of the error message sent in json so that it outputs only the error string instead of an object.

Explanation:
If you look at this code:

jQuery.ajax({
    url: "http://piwik.org/demo/index.php?module=API&method=SitesManager.getSitesWithViewAccess&format=JSON&jsoncallback=a",
    dataType:"json",
    success:function(response){
        console.debug(response);
    },
    error:function (xhr, ajaxOptions, thrownError){
        console.debug(xhr.status);
        console.debug(xhr.responseText);
    }    
});

The lign:

console.debug(xhr.responseText);


Should directly output the error message. Currently it is not the case, it output a complicated object. E.g:

{"result":"error", "message":"The method 'getvailableLanguages' does not exist or is not available in the module 'Piwik_LanguagesManager_API'."}

Changed 4 years ago by JulienM (JulienMoumne)

comment:11 Changed 4 years ago by JulienM (JulienMoumne)

There are 62 tests failing with this patch. E.g:

Exception: ../tests/core/API/DocumentationGenerator.test.php -> Test_Piwik_API_DocumentationGenerator -> test_callableApiMethods_doNotFail -> Unexpected PHP error [Cannot modify header information - headers already sent by (output started at /Users/Ju/Sites/piwik-dev3/trunk/tests/all_tests.php:15)] severity [E_WARNING] in [/Users/Ju/Sites/piwik-dev3/trunk/core/API/ResponseBuilder.php line 302]

It occurs because I have modified the return http code from 200 to 400 in ResponseBuilder.php (method is : getResponseException() and renderBadRequest())

comment:12 follow-up: Changed 4 years ago by JulienM (JulienMoumne)

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.

1) In the jsoncallback method, the caller can test the type of the return data and deduce from it if an error occurred (when an error occurs, the data returned is a string).

2) A better solution would be to add a new parameter "jsonerrorcallback" which would be used with a different callback method dedicated to error handling.

Please advise on what's best.

(In the patch I just submitted I forgot to add quotes around the returned string)

Changed 4 years ago by JulienM (JulienMoumne)

comment:13 in reply to: ↑ 12 Changed 4 years ago by matt (mattab)

Replying to JulienM:

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.

1) In the jsoncallback method, the caller can test the type of the return data and deduce from it if an error occurred (when an error occurs, the data returned is a string).

2) A better solution would be to add a new parameter "jsonerrorcallback" which would be used with a different callback method dedicated to error handling.

Please advise on what's best.

  • Looking at Flickr json API I see they are using the same callback, but with a different parameter. I think we should do the same, and have the same callback but with the object {result:error, message:%s} passed to it. Therefore I would not return the string only, but return the full object (which is not really 'complicated' ;-)
  • I would remove the http 400 error code as it is added complexity for now (it's best to force apps to test the actual response).

otherwise, looks good to me

comment:14 Changed 4 years ago by JulienM (JulienMoumne)

New patch keep original json exception message format and don't override HTTP return code.

Changed 4 years ago by JulienM (JulienMoumne)

comment:15 Changed 4 years ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed

(In [2294]) Fixes #1372 Thanks JulienM for patch
only modified:

  • replaced PiwiK_DataTable_Renderer:: calls to self:: calls (as they are all children classes)
  • removed renderException from Tsv as it extends Csv

comment:16 Changed 4 years ago by JulienM (JulienMoumne)

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.

comment:17 Changed 4 years ago by vipsoft (robocoder)

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:18 Changed 4 years ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [2301]) Fixes #1372

comment:19 Changed 4 years ago by JulienM (JulienMoumne)

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

comment:20 Changed 4 years ago by matt (mattab)

on purpose: Tsv class extends Csv

Note: See TracTickets for help on using tickets.