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.

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:

1. I placed the html entity translation :

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.

Attachment:
1372.patch

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?

• 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 ?

• 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

• 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'."}

Attachment:
1372.2.patch

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())

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)

Attachment:
1372.3.patch

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

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

Attachment:
#1372.patch

(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

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:
Json.php.patch

(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:
Tsv.php.patch

on purpose: Tsv class extends Csv