Skip to content
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

[5.6] Handle more JSON errors gracefully when JSON_PARTIAL_OUTPUT_ON_ERROR is set #23410

Merged

Conversation

Jalle19
Copy link
Contributor

@Jalle19 Jalle19 commented Mar 6, 2018

It's fairly common for stack traces to be circular, i.e. they can't be encoded as JSON without limiting the recursion depth somehow. PHP automatically does this if JSON_PARTIAL_OUTPUT_ON_ERROR is true, but we still considered it an error.

By treating JSON_ERROR_RECURSION as okay if JSON_PARTIAL_OUTPUT_ON_ERROR is set we prevent things from blowing up if the user uses a JSON-based exception handler.

Since exceptions thrown in exception handlers are generally a recipe for disaster I added JSON_ERROR_INF_OR_NAN to the list of "acceptable" errors too.

Sam Stenvall added 2 commits March 6, 2018 17:44
…is set

It's fairly common for stack traces to be circular, i.e. they can't be encoded as JSON without limiting the recursion depth somehow. PHP automatically does this if JSON_PARTIAL_OUTPUT_ON_ERROR is true, but we still considered it an error.

By treating JSON_ERROR_RECURSION as okay if JSON_PARTIAL_OUTPUT_ON_ERROR is set we prevent things from blowing up if the user uses a JSON-based exception handler.

Since exceptions thrown in exception handlers are generally a recipe for disaster I added JSON_ERROR_INF_OR_NAN to the list of "acceptable" errors too.
@Jalle19 Jalle19 force-pushed the gracefully-handle-partial-json-errors branch from 1930a19 to 01090f7 Compare March 6, 2018 15:45
@taylorotwell
Copy link
Member

Need some example of what happens before this PR and how to recreate it so we can see what you are talking about. Please resubmit with that.

@taylorotwell taylorotwell reopened this Mar 7, 2018
@Jalle19
Copy link
Contributor Author

Jalle19 commented Mar 8, 2018

@taylorotwell we use a custom exception handler in a Lumen application that renders exceptions as JSON. Without this patch, when a particular exception happens, what you get is this:

[
    {
        "file": "/vagrant/api/vendor/illuminate/http/JsonResponse.php",
        "line": 75,
        "function": null,
        "class": "Symfony\\Component\\Debug\\Exception\\FatalErrorException",
        "args": [
            "Uncaught InvalidArgumentException: Recursion detected in /vagrant/api/vendor/illuminate/http/JsonResponse.php:75\nStack trace:\n#0 /vagrant/api/vendor/symfony/http-foundation/JsonResponse.php(50): Illuminate\\Http\\JsonResponse->setData(Array)\n#1 /vagrant/api/vendor/illuminate/http/JsonResponse.php(31): Symfony\\Component\\HttpFoundation\\JsonResponse->__construct(Array, 500, Array)\n#2 /vagrant/api/app/App/Exceptions/Handler.php(51): Illuminate\\Http\\JsonResponse->__construct(Array, 500, Array, 512)\n#3 /vagrant/api/vendor/nordsoftware/lumen-chained-exception-handler/src/ChainedExceptionHandler.php(57): ALehdet\\ContentApi\\App\\Exceptions\\Handler->render(Object(Illuminate\\Http\\Request), Object(InvalidArgumentException))\n#4 /vagrant/api/vendor/laravel/lumen-framework/src/Concerns/RegistersExceptionHandlers.php(127): Nord\\Lumen\\ChainedExceptionHandler\\ChainedExceptionHandler->render(Object(Illuminate\\Http\\Request), Object(InvalidArgumentException))\n#5 /vagrant/api/vendor/laravel/lumen-framework/src/Concerns/RegistersExcep"
        ]
    },
    {
        "file": "/vagrant/api/vendor/laravel/lumen-framework/src/Concerns/RegistersExceptionHandlers.php",
        "line": 54,
        "function": "handleShutdown",
        "class": "Laravel\\Lumen\\Application",
        "args": []
    },
    {
        "file": "[internal]",
        "line": 0,
        "function": "Laravel\\Lumen\\Concerns\\{closure}",
        "class": "Laravel\\Lumen\\Application",
        "args": []
    }
]

With this patch, the real exception that caused this (and subsequently couldn't be encoded due to recursion) is shown:

{
    "type": "Symfony\\Component\\Debug\\Exception\\FatalThrowableError",
    "message": "Type error: Argument 1 passed to Contentful\\Delivery\\ResourceBuilder::buildField() must be an instance of Contentful\\Delivery\\ContentTypeField, null given, called in /vagrant/api/vendor/contentful/contentful/src/Delivery/ResourceBuilder.php on line 372",
    "file": "/vagrant/api/vendor/contentful/contentful/src/Delivery/ResourceBuilder.php",
    "line": 385,
    "trace": [
        {
            "file": "/vagrant/api/vendor/contentful/contentful/src/Delivery/ResourceBuilder.php",
            "line": 385,
            "function": null,
            "class": "Symfony\\Component\\Debug\\Exception\\FatalThrowableError",
            "args": [
                "Type error: Argument 1 passed to Contentful\\Delivery\\ResourceBuilder::buildField() must be an instance of Contentful\\Delivery\\ContentTypeField, null given, called in /vagrant/api/vendor/contentful/contentful/src/Delivery/ResourceBuilder.php on line 372"
            ]
        },
        ... SNIP
    ]
}

Since the purpose of the the JSON_PARTIAL_OUTPUT_ON_ERROR option seems to be precisely to avoid failing completely on recoverable errors such as this I thought this is the right solution. The error handling remains unchanged for users who don't pass this special option to JsonResponse.

@Jalle19
Copy link
Contributor Author

Jalle19 commented Mar 8, 2018

@taylorotwell for refernce, our error handler looks like this (it uses whoops to format the exception):

public function render($request, Exception $e)
{
    if (env('APP_DEBUG', false)) {
        $data = Formatter::formatExceptionAsDataArray(new Inspector($e), env('APP_DEBUG', false));
    } else {
        $data = $this->createDefaultResponseData($e);
    }

    $status = $this->resolveResponseStatusCode($e);

    return new JsonResponse($data, $status, [], JSON_PARTIAL_OUTPUT_ON_ERROR);
}

The last line is the important one where we say we want partial output whenever possible.

@taylorotwell taylorotwell merged commit 01090f7 into laravel:5.6 Mar 9, 2018
@Jalle19 Jalle19 deleted the gracefully-handle-partial-json-errors branch March 9, 2018 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants