api-platform / core

The server component of API Platform: hypermedia and GraphQL APIs in minutes
https://api-platform.com
MIT License
2.4k stars 856 forks source link

Wrong error code when denormalizing a non existant item #6116

Open KDederichs opened 7 months ago

KDederichs commented 7 months ago

API Platform version(s) affected: 3.2.11

Description
While writing tests I noticed that when you try to denormalize a non existing item it'll throw an exception with statuscode 500.

It shouldn't error out with a 500 error since that's not the expected error code for cases like that.

How to reproduce

#[ApiResource]
class Foo {
   prive ?Bar $bar = null;
}
#[ApiResource]
class Bar {
}

and then try to create a Foo with a reference to a non existing Bar. Possible Solution

There's two solutions for this: Change the error code to 404, to match the not found paradime.

Optionally allow not found items to serialize to null so users can throw their own errors with Validations.

Additional Context

    "title": "An error occurred",
    "detail": "Item not found for \"/api/bar/018d1ca6-da89-77c2-93a9-2b3593055e42\".",
    "status": 500,
    "type": "/errors/500",
    "trace": [
        {
            "file": "/app/vendor/api-platform/core/src/Serializer/AbstractItemNormalizer.php",
            "line": 860,
            "function": "denormalizeRelation",
            "class": "ApiPlatform\\Serializer\\AbstractItemNormalizer",
            "type": "->"
        },
...
soyuka commented 7 months ago

I'm pretty sure we've a test for this, imo it should even send a 400 (I have a deja-vu feeling)

edit: https://github.com/api-platform/core/blob/main/features/main/relation.feature#L357

KDederichs commented 7 months ago

Digging into it, it's running into this exception: https://github.com/api-platform/core/blob/ac8031eb42810e6c538ffaca295cf7b4d35adf06/src/Serializer/AbstractItemNormalizer.php#L546

Maybe the test is the other one with the context?

KDederichs commented 7 months ago

Yeah I don't think that test covers it. I just switched out the exception with a return null and the relation.feature test keeps running the same as before

Skkay commented 7 months ago

Same issue with a backed enum:

Example:

<?php

enum StatusEnum: string {
    case Ready: 'ready';
}

class MyEntity {
    private ?StatusEnum $status = null;

    public function getStatus(): ?StatusEnum
    {
        return $this->status;
    }

    public function setStatus(StatusEnum $status): static
    {
        $this->status = $status;

        return $this;
    }
}

Making a POST request with:

{
    "status": "unknown_status",
}

Will result in an Error 500:

{
    "@context": "\/api\/contexts\/Error",
    "@type": "hydra:Error",
    "hydra:title": "An error occurred",
    "hydra:description": "The data must belong to a backed enumeration of type App\\Enum\\StatusEnum",
    "trace": [
        // ...
    ]
}
webda2l commented 5 months ago

Same issue with a backed enum:

Example:

<?php

enum StatusEnum: string {
    case Ready: 'ready';
}

class MyEntity {
    private ?StatusEnum $status = null;

    public function getStatus(): ?StatusEnum
    {
        return $this->status;
    }

    public function setStatus(StatusEnum $status): static
    {
        $this->status = $status;

        return $this;
    }
}

Making a POST request with:

{
    "status": "unknown_status",
}

Will result in an Error 500:

{
    "@context": "\/api\/contexts\/Error",
    "@type": "hydra:Error",
    "hydra:title": "An error occurred",
    "hydra:description": "The data must belong to a backed enumeration of type App\\Enum\\StatusEnum",
    "trace": [
        // ...
    ]
}

Similar with GraphQl query and latest 3.2.17

But seems like a webonyx/graphql-php related issues even it's a bit weird that no previous issue exist about it. (Tested with its v15 & v14)

{
    "errors": [
        {
            "message": "Internal server error",
            "locations": [
                {
                    "line": 9,
                    "column": 5
                }
            ],
            "path": [
                "lessons",
                "edges",
                0,
                "node",
                "tagOptional"
            ],
            "extensions": {
                "debugMessage": "Expected a value of type PriceTagEnum but received: {\"edges\":[{\"node\":{\"#itemResourceClass\":\"App\\\\Entity\\\\Lesson\",\"#itemIdentifiers\":{\"uuid\":\"9932cbf4-0feb-4901-bfe3-73091e5090fe\"}}},{\"node\":{\"#itemResourceClass\":\"App\\\\Entity\\\\Lesson\",\"#itemIdentifiers\":{\"uuid\":\"5ea647db-b66b-4049-9b6b-b6307c483c45\"}}},{\"node\":{\"#itemResourceClass\":\"App\\\\Entity\\\\Lesson\",\"#itemIdentifiers\":{\"uuid\":\"64084fda-cb2b-493a-83d5-69a87cd6c231\"}}},{\"node\":{\"#itemResourceClass\":\"App\\\\Entity\\\\Lesson\",\"#itemIdentifiers\":{\"uuid\":\"28788760-1394-493b-8eb7-fcfd40a7edd2\"}}},{\"node\":
                [...............]
                {\"#itemResourceClass\":\"App\\\\Entity\\\\Lesson\",\"#itemIdentifiers\":{\"uuid\":\"5ffbcdca-fc3b-4344-8547-d18c8cc7fb88\"}}},{\"node\":{\"#itemResourceClass\":\"App\\\\Entity\\\\Lesson\",\"#itemIdentifiers\":{\"uuid\":\"a6d5f1e6-f39a-46df-9d00-76759dbdf52d\"}}}]}",
                "file": "\/app\/api\/vendor\/webonyx\/graphql-php\/src\/Type\/Definition\/EnumType.php",
                "line": 140,
                "trace": [
                    {
                        "file": "\/app\/api\/vendor\/webonyx\/graphql-php\/src\/Executor\/ReferenceExecutor.php",
                        "line": 1011,
                        "call": "GraphQL\\Type\\Definition\\EnumType::serialize(array(1))"
                    },
                    {
                        "file": "\/app\/api\/vendor\/webonyx\/graphql-php\/src\/Executor\/ReferenceExecutor.php",
                        "line": 884,
                        "call": "GraphQL\\Executor\\ReferenceExecutor::completeLeafValue(GraphQLType: PriceTagEnum, array(1))"
                    },
                    {
                        "file": "\/app\/api\/vendor\/webonyx\/graphql-php\/src\/Executor\/ReferenceExecutor.php",
                        "line": 753,
                        "call": "GraphQL\\Executor\\ReferenceExecutor::completeValue(GraphQLType: PriceTagEnum, instance of ArrayObject(1), instance of GraphQL\\Type\\Definition\\ResolveInfo, array(5), array(1), null)"
                    },
                    {
                        "file": "\/app\/api\/vendor\/webonyx\/graphql-php\/src\/Executor\/ReferenceExecutor.php",
                        "line": 640,
                        "call": "GraphQL\\Executor\\ReferenceExecutor::completeValueCatchingError(GraphQLType: PriceTagEnum, instance of ArrayObject(1), instance of GraphQL\\Type\\Definition\\ResolveInfo, array(5), array(1), null)"
                    },
                    {
                        "file": "\/app\/api\/vendor\/webonyx\/graphql-php\/src\/Executor\/ReferenceExecutor.php",
                        "line": 1317,
                        "call": "GraphQL\\Executor\\ReferenceExecutor::resolveField(GraphQLType: Lesson, array(3), instance of ArrayObject(1), array(5), null)"
                    },
                    [..........]
        }
    ],
    "data": {
        "lessons": {
            "edges": [
                {
                    "node": {
                        "id": "\/v3\/api\/lessons\/93c26666-4ebd-4e2a-8927-ce6164552b19",
                        "tagOptional": null
                    }
                }
            ]
        }
    }
}
soyuka commented 5 months ago

I could not reproduce can you help by improving my test at https://github.com/soyuka/core/commit/2d1a5115a601127dacdfa6b896717e1909cea527 ?

KDederichs commented 5 months ago

@soyuka

When I run your test and add

    And the JSON should be a superset of:
    """
    {
      "@id": "/errors/400",
      "@type": "hydra:Error",
      "title": "An error occurred",
      "detail": "Item not found for \"/issue6116_relation/1\""
    }
    """

to the test it fails with

      +  'detail' => 'Invalid IRI "/issue6116_relation/1".',

So looks like you get the right status code for the wrong reason

soyuka commented 5 months ago

Not sure I understand, 400 is the correct status code and the Invalid IRI error was always there.

KDederichs commented 5 months ago

Sure, but the 500 happens when you get a "Item not found" error, not an "Invalid Iri" error. So it would seem the test doesn't seem to test the actual problem but something else.

KDederichs commented 5 months ago

Like the test setup seems to be flawed in a way. Looking into it more it throws Invalid Iri in AbstractItemNormalizer.php:572 because no route matches "/issue6116_relation/1". The route should exist but shouldn't find the resource.

What you got now seems to be just a non existing route for some reason

soyuka commented 5 months ago

https://github.com/api-platform/core/commit/28019ea48100ed49b2a6062d38d858fd36c4dbcd indeed, now I have the not found

KDederichs commented 5 months ago

Hmm still works, I'm trying to track down what's causing it

KDederichs commented 5 months ago

Ok found the issue @soyuka

The error happens if you define custom exception mappings:

    exception_to_status:
        App\Exception\OrganisationNotSetException: !php/const Symfony\Component\HttpFoundation\Response::HTTP_FORBIDDEN
        App\Exception\ProjectNotSetException: !php/const Symfony\Component\HttpFoundation\Response::HTTP_FORBIDDEN

It maybe overwrites the ones you guys do?

KDederichs commented 5 months ago

Also in the test bundle's config_common.yml this is suppressing it cause you're manually mapping the exception status in the tests:

        Symfony\Component\Serializer\Exception\ExceptionInterface: !php/const Symfony\Component\HttpFoundation\Response::HTTP_BAD_REQUEST

If you remove that line it correctly fails.

If you remove all the mappings it correctly has status 400 again.

soyuka commented 5 months ago

Indeed configuration is not merged, you need to add back the defaults. We should add this to the documentation.

KDederichs commented 5 months ago

Yeah that would be useful, along with what the default actually is. Or maybe just merge it as a new feature?