`ProviderNotFoundException` overwrote by misleading `NotFoundHttpException`

[Aerendir]

API Platform version(s) affected: < 3.2.21

Description
When setting GET operations, they (may?) require a provider.

When a provider is not found, the exception ProviderNotFoundException is caught and the variable $data is set to null.

Then there is a check that, when the operation is, among the others, a GET, throws a NotFoundHttpException.

This is completely misleading, leading the developer to do a whole series of tests to understand why things are not working as expected.

To make things even worse, for example, running bin/console debug:router, correctly shows the route with the correct operation.

So, a full round of debug, breakpoints, very deep trees of decorators and many more scary things, starts... with countless hours spent going around and around in the code, banging the head against the monitor.... "Why the f*** aren't you working!?🤬"...

In the end, I came to this code:

https://github.com/api-platform/core/blob/6d15e228611875a3c1a41a14ecf26a38bd67639a/src/Symfony/EventListener/ReadListener.php#L130-L145

(it seems to be updated, but the substance didn't change)

In practice, this code overwrites the informative ProviderNotFoundException with a very misleading NotFoundHttpException.

They also return two very different codes: a misleading 404 versus an informative 500.

How to reproduce

A controller:

#[AsController]
#[Route(path: SearchRequest::API_ENDPOINT, name: SearchRequest::API_ROUTE_NAME, methods: [Request::METHOD_GET])]
final readonly class SearchController
{
    public function __invoke(
        #[MapQueryString]
        SearchRequest $searchRequest,
        // Account $account,
    ): JsonResponse {
        ...
    }
}

A request object set as an ApiPlatform resource:

#[Get(
    /**
     * Documented with a decorator.
     *
     * @see SearchOpenApiGet
     */
    controller: SearchController::class,
    uriTemplate: self::API_ENDPOINT,
)]
class SearchRequest
{
...
}

Possible Solution
I think the code should, at least, be changed like this:

            $data = $this->provider->provide($operation, $uriVariables, $context);
        } catch (InvalidIdentifierException|InvalidUriVariableException $e) {
            throw new NotFoundHttpException('Invalid identifier value or configuration.', $e);
        } catch (ProviderNotFoundException $e) {
            $data = null;
        }

        if (
            null === $data
            && 'POST' !== $operation->getMethod()
            && (
                'PUT' !== $operation->getMethod()
                || ($operation instanceof Put && !($operation->getAllowCreate() ?? false))
            )
        ) {
-            throw new NotFoundHttpException('Not Found');
+            isset($e) ? throw $e : throw new NotFoundHttpException('Not Found');
        }

This way, if the problem is a caught ProviderNotFoundException, it is not overwrote but, instead, is thrown and the developer knows (s)he need a StateProvider to make the route work as expected.

But, maybe this is not the best solution at all: in the end, which is the purpose of throwing a generic NotFoundHttpException? The "not found" refers to the result of the found state provider (the state provider didn't find the data it has to return) or the throwing of the ProviderNotFoundException.

In the second case, in my opinion, it's better to re-throw the original ProviderNotFoundException: this informs the developer (s)he has to implement one.

In the first case, instead, may be better to throw a more informative exception: "The data provider returned no value" or something similar.

In both cases, the problem is with the state provider, not with the route: the route exists: is the state provider that is failing to do its job (or isn't configured at all).

Putting the accent on the state provider, helps the developer look in the right place, not looking for, for example, the configuration of the route.

[soyuka]

we should just send the exception to previous on the not found

[Aerendir]

@soyuka , yes, may help… but anyway the real problem would pass behind the 404 error…

But I’d like to better understand why a 404 error is returned when, in fact, the problem is a missing state provider: to me, for this situation, a 500 seems more appropriate: without a state provider, the route cannot work (only if the path has parameters?)… what am I missing? 🤔 why a 404 error was considered better?

[soyuka]

yes it can you don't need a state provider for post requests

[Aerendir]

yes it can you don't need a state provider for post requests

I think I'm not understanding: a GET request requires a state provider.

Do you agree or not that a 500 error may be more appropriate in case the state provider is missed?

[mrossard]

IMO 404 is a correct response : the server was indeed unable to find the requested resource.

That being said, a simple "fix" could be to just log the ProviderNotFoundException, couldn't it? The dev would just look at his logs and see he made a simple mistake...

[Aerendir]

IMO 404 is a correct response : the server was indeed unable to find the requested resource.

@mrossard , beh, technically, the server doesn't even know if it found or not the entity: it only knows it wasn't able to find the state provider and, due to this, it is not able to check for the existence of the requested resource 🤔

Anyway, the real thing I'm interested in is to get a precise error about the cause of the issue: in this case, I'd like to get an immediate error that says "Hey, you forgot to configure the state provider and you have to, otherwise you we'll never be able to search for the entity and the route will never work.".

I see it as something like the messages Symfony returns when you miss something mandatory to make the code work...

Whichever will be the solution, please, let the developer know what is going on, so (s)he can fix the issue fast, without being forced to debug for hours 🙏

[mrossard]

IMO 404 is a correct response : the server was indeed unable to find the requested resource.

@mrossard , beh, technically, the server doesn't even know if it found or not the entity: it only knows it wasn't able to find the state provider and, due to this, it is not able to check for the existence of the requested resource 🤔

Well if there is no provider, IMO the framework has no other choice than to believe it's intentional and the dev actually wanted to send a 404 for everything... I'm not sure what the use case for explicitly defining a 404 this way would be (maybe for a write-only resource of some kind? You'd likely want a 403 instead, but not having a provider would likely be fine...), but if there is one sending a technical message to the end suer wouldn't be great.

Anyway, the real thing I'm interested in is to get a precise error about the cause of the issue: in this case, I'd like to get an immediate error that says "Hey, you forgot to configure the state provider and you have to, otherwise you we'll never be able to search for the entity and the route will never work.".

I see it as something like the messages Symfony returns when you miss something mandatory to make the code work...

Whichever will be the solution, please, let the developer know what is going on, so (s)he can fix the issue fast, without being forced to debug for hours 🙏

Logging it does make sense to me. That could be a simple PR to write ;)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Aerendir]

@mrossard Sorry for the late reply, I missed your...

I'm going to start from the end of your message:

but if there is one sending a technical message to the end suer wouldn't be great.

We both strongly agree on this... And, if we agree on this, as we agree, the next question is: how can we inform the developer it is probably doing something wrong?

Maybe we could distinguish the messages in dev env and in prod env (as it is commonly done)?

Comes to my mind messages that, for example, inform that a component is missed from the environment and that it is required to make some features of the framework work correctly...

Some Examples (also if I'm sure you are already aware of what I'm speaking about: thos are for the other readers):

https://github.com/symfony/symfony/blob/78de59be7e54a651fb9b1a79749274fb2f41fe2c/src/Symfony/Component/Console/Command/LockableTrait.php#L37

https://github.com/symfony/symfony/blob/78de59be7e54a651fb9b1a79749274fb2f41fe2c/src/Symfony/Bundle/FrameworkBundle/Command/ConfigDumpReferenceCommand.php#L103

https://github.com/symfony/symfony/blob/78de59be7e54a651fb9b1a79749274fb2f41fe2c/src/Symfony/Component/Messenger/Transport/TransportFactory.php#L40-L52

Well if there is no provider, IMO the framework has no other choice than to believe it's intentional and the dev actually wanted to send a 404 for everything...

I have URLs like this:

/{resource1Id}/{resource2Id}/...

In these situations, a data provider is required to retrieve the resources from the database and make Api Platform aware of them.

If I forgot a data provider, I actually get the generic 404 error while, what it really happened, was that the data provider is missed, not the resource itself.

If I try to retrieve one of the resources mentioned in the URL and one of them cannot be found, then a 404 is correct.

But in all other situations, what happens is that, also if I'm sure the resource is in the database, I anyway get the 404 error: this is, as told, misleading, as doesn't permit me to distinguish between an actual 404 (the resource actually doesn't exist) and another kind of error (missed data provider).

Maybe a LogicException with a 400 - Bad request error code would fit better.

Thinking "the framework has no other choice than to believe it's intentional and the dev actually wanted to send a 404 for everything" is only one of the possible thoughts; the other, as I'm saying, is that the developer UNintentionally forgot to configure a data provider: this second situation is completely unhandled and, worst, hidden behind a misleading 404.

I'm not sure what the use case for explicitly defining a 404 this way would be (maybe for a write-only resource of some kind? You'd likely want a 403 instead, but not having a provider would likely be fine...),

I'm not sure, too and this is the reason why...

Logging it does make sense to me. That could be a simple PR to write ;)

... I'm asking for the opinion of more experienced developer on Api Platform before doing anything ;)

With "logging", what do you mean exactly?

[mrossard]

I do understand your point, but still think having a "userland" error for this wouldn't be ok for some use cases / could have unwanted side effects.

By logging i mean just that - it should be possible and relatively easy to just write something along the lines of "no provider for this resource, is it intentional?" in the logs when the frameworks tries to read a resource and doesn't find a way to do so...

[soyuka]

Adding a log is a fairly good idea as it can be at a level that would be disabled in most production codes.

[dunglas]

Maybe could we also showcase the problem in the profiler and/or the web debug toolbar?