Feedback on problem-details module

Contributors
1

Hi, all!

I’ve been working on the Apigility on Expressive initiative, and the first step, as it was with Apigility originally, was setting up error handling. To this end, I’ve created a new problem-details module that provides the following:

  • A Problem Details exception interface, providing access to the title, type, detail, status code, and any additional data as key/value pairs (getAdditionaData()); a trait accompanies this interface for implementing the methods, but users would be expected to create their own custom types with named constructors for reporting problems.
  • Problem Details responses, both XML and JSON formats. Each has named constructors for generating the response from either discrete arguments or throwables; an additional, optional flag on the fromThrowable() method controls whether exception backtrace/previous exception details are presented. If the throwable is of the problem details exception type, it pulls information from the exception to create the problem details information.
  • A factory for generating a Problem Details response that uses the value of the PSR-7 Accept header PSR-7 ServerRequestInterface and its composed Accept header (if present) to determine whether XML or JSON should be returned (defaults to XML when unable to match). This has separate named constructors that mirror those of the responses factory methods for generating a response from PHP values vs PHP Throwable instances. The factory composes a flag for $isDebug mode (in which case full exception details are provided), JSON encoding flags, a PSR-7 response prototype, and a callable factory for generating a writable PSR-7 stream for use with the response body.
  • Middleware that performs error handling. Caught errors/exceptions are passed to the the above factory, along with the Accept header request instance. The middleware can compose a flag indicating whether or not to include debug details composes a problem details response factory instance.

Why so many features?

The easiest path is to simply return a problem details response from your middleware. However, this can be tedious, and you may miss the fact that code you are calling raises exceptions, leaving you with error responses in HTML instead of JSON or XML. As such, having error handling middleware specific to your API can be a good fallback.

If you’re going to have error handling middleware anyways, why not just raise exceptions in the first place? Thus, the special exception interface. One advantage of this exception type is that you can create custom exceptions for your application that compose implement it; if you are in an API context, you get excellent problem reporting; if you are in your standard application context, you’ll get your normal errors. Extra context with exceptions is always useful.

Finally, one thing we never tackled with Apigility was using XML. The problem details specification defines both JSON and XML formats, and this seems like a good time to start implementing the latter. Considering we have access to the Accept header in middleware, content negotiation is not difficult, and would give us more possibilities.

Do you have any feedback? Am I missing anything? Does the order of arguments make sense? etc.

For full documentation: https://weierophinney.github.io/problem-details/

1 Like
2

Hi @matthew,

Thank you for your good work on this module. I have gone through the documentation and it looks good to me. I would like to share a few questions that came to my mind. Nothing really a blocker. But just for consideration.

Not sure whether you are aware of GitHub - Crell/ApiProblem: A simple implementation of the api-problem specification. Includes PSR-15 support. . It looks like it is RFC 7809, but the one you created is 7807 . I haven’t read both RFC, so don’t know whether anything super-seeds or not.

Not able to strike through. It looks both implement 7807. It was a typo in the README.md .

  1. Is there any reason you didn’t considered using PSR-17, GitHub - http-interop/http-factory: (DEPRECATED) PSR-17 interfaces for HTTP Factories . In the long run, I am sure other frameworks may be creating its own PSR-7 Response. So this library can be suited easily for them also. It also helps other library / module developers not to rely on classes specific to Diactoros.

Thank you.

3

There was one primary reason: being able to return a response via direct instantiation. That said, since the recommended way to create a response is via the factory (in order to perform content negotiation), the response type could be either (a) removed or (b) optional (as in a “suggest” dependency on zend-diactoros), and the factory could use PSR-17 in order to generate an appropriate response.

I’ll create a branch to experiment with that; thanks for the suggestion!

1 Like
4

Interesting to see how Apigility and Expressive are influencing each other. I remember asking about Apigility 2 a couple of years ago. You were already talking about moving to a middleware core back then. Getting API Problems working is a good first step.

Since this is both a middleware and a service, was this planned on replacing Zend\Stratigility\Middleware\ErrorHandler, or providing an alternative? Were y’all looking to bake this into Expressive to handle internal errors too (i.e. in Application, RouteMiddleware and the like)?

If there was one thing I could suggest to add would be to, somehow, have a middleware pipeline inside ProblemDetailsMiddleware to handle the role listeners perform for Zend\Stratigility\Middleware\ErrorHandler. Otherwise, I’ll have to pipe those before ProblemDetailsMiddleware in the main app pipeline to peek at the response and do any post-error processing.

On a very minor note, you might want to change your Exceptions example to use StatusCode::STATUS_FORBIDDEN instead of 403 as the trait code uses that library. Just for consistency and the like.

5

Hi saw your request for comments. And I’m not sure if this helps: I’m using ApiProblem in my expressive application as a catch all error handler. The main problem was that I couldn’t simply return the ApiProblemResponse as that doesn’t implement \Psr\Http\Message\ResponseInterface. hence the extra toResponse step. If that could be avoided in a refactor… Any Exceptions thrown that implement ZF\ApiProblem\Exception\ProblemExceptionInterface are treated differently.

Additionally I would like to be able to specify more jsonFlags options. Especially pretty print when APP_ENV !== ‘production’.

Thank you Matthew!

Bas

<?php

declare(strict_types=1);

namespace HF\Api\Middleware;

use Error;
use Exception;
use Interop\Http\ServerMiddleware\DelegateInterface;
use Interop\Http\ServerMiddleware\MiddlewareInterface;
use LosMiddleware\LosLog\StaticLogger;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Zend\Diactoros\Response;
use ZF\ApiProblem\ApiProblem as ZendApiProblem;
use ZF\ApiProblem\ApiProblemResponse as ZendApiProblemResponse;
use ZF\ApiProblem\Exception\ProblemExceptionInterface;

final class JsonApiProblem implements MiddlewareInterface
{
    public function process(ServerRequestInterface $request, DelegateInterface $delegate): ResponseInterface
    {
        try {
            return $delegate->process($request);
        } catch (Error $e) {
            // get inner most error
            while ($e->getPrevious()) {
                $e = $e->getPrevious();
            }

            $apiProblem = new ZendApiProblem(500, $e, null, null, [
                'file' => $e->getFile(),
                'line' => $e->getLine(),
            ]);
            $response   = $this->toResponse($apiProblem);

            return $response;
        } catch (Exception $e) {
            // get inner most exception or first that is ProblemExceptionInterface
            while (! ($e instanceof ProblemExceptionInterface) && $e->getPrevious()) {
                $e = $e->getPrevious();
            }

            $apiProblem = new ZendApiProblem(422, $e);
            $response   = $this->toResponse($apiProblem);

            return $response;
        }
    }

    private function toResponse(ZendApiProblem $problem): ResponseInterface
    {
        $problem->setDetailIncludesStackTrace(APP_ENV !== 'production');

        $problemResponse = new ZendApiProblemResponse($problem);
        $response        = new Response();
        $response        = $response->withStatus($problemResponse->getStatusCode());

        foreach ($problemResponse->getHeaders() as $header) {
            $response = $response->withHeader($header->getFieldName(), $header->getFieldValue());
        }

        $response->getBody()->write($problemResponse->getBody());
        StaticLogger::save($problemResponse->getContent(), 'api-problem.log');

        return $response;
    }
}
6

The plan is a completely different package, which I’ve linked above (it’s temporary until we figure out where these new packages will live). As such, the probems you go through above will not be an issue.

I’d not considered adding JSON flags; I’ll start work on that soon!

7

@harikt I’ve started working on this, but there’s a bit of a problem: PSR-17 is still being actively developed, which makes it a moving target.

One idea I’m floating instead is to compose a response prototype in the ProblemDetailsResponseFactory, and fall back to a Diactoros response if none is provided. This would solve the de-coupling problem, as you could compose the factory in your middleware, and provide a response prototype in the factory for the factory.

I’m going to go with that approach for now, and let you know when it’s ready to review.

1 Like
8

Hi @matthew,

I agree with you.

9

For anyone following the discussion, I’ve just pushed version 0.2.0 of the weierophinney/problem-details package. It incorporates the following suggestions:

  • Ability to inject JSON encoding flags.
  • Removal of concrete response types, in favor of a factory approach. The ProblemDetailsResponseFactory now composes a response prototype, and a callable for generating a PSR-7 stream to use with the generated response. This allows usage with any PSR-7 implementation, not just Diactoros.

The core of the package is now the ProblemDetailsResponseFactory, which can be composed in middleware, or used as part of the ProblemDetailsMiddleware for general error handling. Additionally, the ProblemDetailsException provides the ability for users to give consumable error details when the ProblemDetailsResponseFactory is used; other exception handlers can freely ignore that information.

Any other feedback?

10

hey @matthew,
I am currently using a self baked solution that achieves roughly the same functionality of your module, and I stumbled upon a peculiar issue with my response generator.

Basically, using Throwable::getTrace() should not be considered safe for JSON encoding, as it may contain binary strings, which could cause a JSON_ERROR_UTF8.

I was able to replicate this by putting some random char bytes in the payload:
new JsonResponse(['foo' => pack('c', 0x42986) ])

I ended up using getTraceAsString in the response generator instead, but that output is obviously less readable.

Anyway, this detail raises a different issue: I think that there should be a failsafe try catch around the attempt to create a specific ProblemDetailResponse from a Throwable, and return a default response with generic infos.

What do you think?

11

I forgot to mention: obviously the encoding problem also applies to the exception message, but that is quite unlikely to contain a binary string, while the whole exception trace contains call stack invocation arguments, so that’s where the issue was coming from.

12

Hi!
I have created PR with some small fixes (https://github.com/weierophinney/problem-details/pull/6), and I’ve added there also my questions/suggestions. I’ve put these also here:

  • is there any reason why XML is default, not JSON? Apigility 1 was using JSON by default, and I would keep it here as well. As I can see now - to convert arrays to XML we need to us external library, but to use JSON we are using internal methods, so I think it’s better, and we should recommend using JSON if possible. Maybe we can go even further and require to add XML converter dependency only in case someone wants use XML responses?

  • don’t we need to add “ext-json”: “*”, into composer? I remember when you said that in some configuration json is not complied with PHP. Maybe now (with PHP 7.1) is different?

  • maybe - instead of merging “additional” params array with default information - we should add all additional data in a subkey?

  • I’d update everywhere (also in tests) return types, type hinting, class properties and PHPDocs. Now for me in the library is a bit messy in PHPDocs, sometimes we have there something, sometimes just part, sometimes we don’t have it at all. We can synchronize PHPDocs with type hinting and return types using phpcs. (not sure, but I think phpDocumentor is using only PHPDocs to generate docs)

  • maybe we should move exceptions to ProblemDetails\Exception namespace? Maybe even we should think about better struct of the library. I know that is is very simple, but it is not so nice to have all classes on one level. Then we can remove “ProblemDetails” from many of these classes and leave it only in the namespace. Now ProblemDetails is duplicated (namespace and class name).

13

Yes: so that when somebody requests via the browser, they get markup. That said, most browsers have */* as the highest priority, and with the negotiation priority rules we’ve specified, application/json will be selected in these cases.

Additionally, somebody pointed out that absence of an Accept header implies */* per the specification, and so we now make that happen.

As such, there are very few cases where you will get XML; it should only happen at this point if you specifically accept only XML, or if you have it higher priority than JSON, both of which require some effort.

No. This needs to be turn-key, and not require the user to manually add more dependencies. Additionally, doing so adds to the logic substantially, as we then need to have different negotiation rules based on whether or not XML generation is possible.

That’s a good question; I need to look and see how the various distros are handling ext-json at this point. There was indeed a period where it wasn’t included by default, but I’m not sure that’s the case any longer.

No! The whole point of the Problem Details specification is that it is extensible, and allows you to provide additional data at the top level, so long as it does not conflict with any specified keys. If users what data pushed down a level, they can do so via nested structures provided the $additional argument.

This should be in a separate contributors topic, I think.

As I developed this library, I realized that most phpdoc was redundant: parameters and return values did not typically need documentation unless:

  • The value could vary type.
  • The value had additional constraints.

I added docblocks when either of those conditions occurred, but only to document the specific parameters and/or return values that needed documentation. I would also add docblocks when:

  • I was throwing exceptions.
  • I needed to explain the purpose of a method.

I have no idea how IDEs and/or phpdocumentor are currently handling the situation with PHP 7.1… which is why I think this needs to be part of another topic. Could you start that, please?

Why? (truly curious)

You indicate in some cases that we then have duplication in naming; that said, when I import these classes, it’s often nice to have something descriptive: ResponseFactory can be quite ambiguous when you’re importing it!

You also indicate you’d prefer separating the exceptions to their own subnamespace — why? What does that specifically give us?

I’m asking these questions, because I found myself asking why I was separating classes into separate directories as I developed this component; it wasn’t gaining me much, but was making things like importing harder and more ambiguous.

I definitely think that as a component grows and specialized subtypes and subfunctions start appearing, segregating makes sense; I’m doing exactly that in the HAL module now. But I’m starting from a point of “if it’s related, why not keep it together?”

So, I’d love to hear your arguments!

14

@matthew
Thanks for your reply, now it’s more clear for me and I understand more. :blush:

I’ve started separate topic about PHPDocs on PHP 7.1 here:
https://discourse.zendframework.com/t/php-7-1-and-phpdocs/171

Regarding structurize library
I was just thinking that it will be more clearer, easier to find etc. Maybe it’s silly to do in such a small component, and as you said it make sense when component grows. The question is when we should structuruze component? When is big enough to segregate classes into subdirectories and when we can keep all in one? For me three different types of Exceptions are enough to make for them separate namespace. This is why I think we should structurize it always, even if it is small component.

@matthew
You indicate in some cases that we then have duplication in naming; that said, when I import these classes, it’s often nice to have something descriptive: ResponseFactory can be quite ambiguous when you’re importing it!

I know what do you mean. It’s annoying when multiple classes we import are called the same and for all of them we have to add aliases. But still I’m not convinced that it’s good to have duplicated namespace in class name. I think purpose of namespaces is to not do that anymore, and yes, when we import multiple classes with the same name we should add aliases to clarify what is it. Of course not always is possible to avoid duplication, and for clarify it should be there in some cases - and maybe in case of problem-details we should keep it as it is. I just don’t want back to old naming convention as “Zend_Stdlib_Filter_Integer” :slight_smile: when we duplicate namespace in class name.

15

Fair enough on that; I can make that change easily.

I’m not convinced on imposing structure on a component always. I think structure should be added as it is discovered: if concepts are related and would benefit from a separate subnamespace, create it; otherwise, it’s structure for the sake of … what?

There are cases where having the namespace or subnamespace in the name makes sense. As an example, I’d argue validators and filters would be far better to include the verbiage in their name; otherwise, when you import them, you get things like DateRange or StringLength, and it’s unclear what those classes do without looking at the import statement.

But those are isolated cases; if we can avoid it, I agree. However, I also think in this particular case, the verbiage is warranted: the response being generated is a problem details response, so including that name in the factory makes the factory’s purpose clear. It’s not some generic response.

So we need to judge these on an individual basis.

16

I wonder if we could do something like the following:

$trace = json_encode($e->getTrace, $this->jsonFlags);
if (json_last_error() !== JSON_ERROR_OK) {
    // do not include trace
} else {
    // include trace
}

Thoughts, @stefanotorresi?

17

Hmm I’d rather always have the trace key, but in case it cannot be encoded I’d put some kind of message, otherwise the consumer would be left wondering why they cannot see the trace!

18

Alternately, we could go through each item in the trace and see if its encodable; if not, we could pass that string to bin2hex or unpack (though I have doubts about the safety of such an operation). I think such an approach could be prohibitive from a performance point of view, though.