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:

core/src/Symfony/EventListener/ReadListener.php

Lines 130 to 145 in 6d15e22

	$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');

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