Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Closed
anonymous-piwik-user opened this Issue · 24 comments

4 participants

@anonymous-piwik-user

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.

@JulienMoumne
Collaborator

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.

@robocoder
Collaborator

Agree with proposal 2.

@JulienMoumne
Collaborator

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 ?

@anonymous-piwik-user

Julien,

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

Andr

@JulienMoumne
Collaborator

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](https://github.com/piwik/piwik/blob/master/core/API/ResponseBuilder.php):

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.
@JulienMoumne
Collaborator

Attachment:
1372.patch

@mattab
Owner

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?

@JulienMoumne
Collaborator
  • 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 ?

@mattab
Owner
  • 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
@JulienMoumne
Collaborator
  • 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'."}
@JulienMoumne
Collaborator

Attachment:
1372.2.patch

@JulienMoumne
Collaborator

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

@JulienMoumne
Collaborator

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)

@JulienMoumne
Collaborator

Attachment:
1372.3.patch

@mattab
Owner

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

@JulienMoumne
Collaborator

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

@JulienMoumne
Collaborator

Attachment:
#1372.patch

@mattab
Owner

(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
@JulienMoumne
Collaborator

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.

@JulienMoumne
Collaborator

Attachment:
Json.php.patch

@mattab
Owner

(In [2301]) Fixes #1372

@JulienMoumne
Collaborator

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

@JulienMoumne
Collaborator

Attachment:
Tsv.php.patch

@mattab
Owner

on purpose: Tsv class extends Csv

@anonymous-piwik-user anonymous-piwik-user added this to the Piwik 0.6.3 milestone
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.