api-platform icon indicating copy to clipboard operation
api-platform copied to clipboard

Request must be validated, but not Object after deserialization

Open errogaht opened this issue 7 years ago • 42 comments

Hi guys, I believe you have the biggest lack of design in API-PLATFORM: you validate deserialized entity but not request. Your approach results in many exceptions that can be thrown because of user input. Eg. I can add a just simple \DateTime field and if the client sends null or empty string then the client gets Exception, not ValidationConstraint. and many many cases when deserialization will fails because of client input. But I want to cover my API fully with all client cases, I want bulletproof API, not API with 500 errors in each second request. Laravel validation validates request...

errogaht avatar Aug 02 '18 04:08 errogaht

So the problem is about \DateTime field on a resource. How can I return ConstrainVoilation message to client on Bad input? This is impossible in the basic setup. If field marked as \DateTime then a client can send the only parsable string with DateTime, he cant send '' or null or 'as3'. But how React app for eg. can show validation error on date time required field? no server-side validation. most of validation is server side but datetime field need JS validation. By the way, this hack if work fine for me: https://github.com/symfony/symfony/issues/27824#issuecomment-409806881 I can't understand Symfony serializer why it designed in this way - validate object but not request.... my opinion is this is harder

errogaht avatar Aug 02 '18 05:08 errogaht

In my test suite, when I pass '' (blank string), I got the followign 400 validation error:

The data is either an empty string or null, you should pass a string that can be parsed with the passed format or a valid DateTime string.

Symfony 4.1, api-platform/core v2.3.0

maks-rafalko avatar Aug 02 '18 05:08 maks-rafalko

404 is not found code. @borNfreee and you get ConstraintVoilationsList object? I don't think so, I believe that the client receives in your case just serialized exception, that cannot be parsed by client, ~13 kb size. I believe that all validations constraints must be responded to the client in one format that is contract with front-end developer, in this case he can write some validation handler on his side. but serialized exceptions with 13 kb response is not a good idea to give it to front-end developer. this is example with a fine response with validations errors, when I add simply validation rules to entity's fields:

{
	"type": "https:\/\/tools.ietf.org\/html\/rfc2616#section-10",
	"title": "An error occurred",
	"detail": "deliveryAt: This value should not be blank.",
	"violations": [
		{
			"propertyPath": "deliveryAt",
			"message": "This value should not be blank.",
			"code": "c1051bb4-d103-4f74-8988-acbcafc7fdc3"
		}
	]
}

errogaht avatar Aug 02 '18 06:08 errogaht

Apologize, this was a typo, I meant 400 Bad Request.

So the response I get is the following:

400 Bad Request (debug mode)

{
  "type": "https://tools.ietf.org/html/rfc2616#section-10",
  "title": "An error occurred",
  "detail": "The data is either an empty string or null, you should pass a string that can be parsed with the passed format or a valid DateTime string.",
  "trace": [
     " %the very big trace dump%" <----------------
  ]

In comparison, the usual validation error response is much more smaller:

{
  "type": "https://tools.ietf.org/html/rfc2616#section-10",
  "title": "An error occurred",
  "detail": "product: This value should not be blank.",
  "violations": [
    {
      "propertyPath": "product",
      "message": "This value should not be blank."
    }
  ]
}

maks-rafalko avatar Aug 02 '18 06:08 maks-rafalko

@borNfreee but what if one entity have many errors? In this case front-end developer gets violations list and mark all invalid fields in form with red border and show a message beside all of them. In case of invalid datetime field there is no violations lists just one message and it is impossible to recognize which field is invalid. If user provide invalid data for 3 common fields he gets 3 errors for each field, but if he forgot to type in date time field (e other common fields is still with invalid data) the user gets just one exception about datetime field without another errors and without propertyPath

errogaht avatar Aug 02 '18 06:08 errogaht

Sad that there is no aswer, i'm running on the same issue right now and cant figure out how to manage it. Instead of a constraint violation, I got an annoying ORM typing error :'(

liorchamla avatar Jan 29 '19 20:01 liorchamla

Can you paste the error you get and the corresponding entity please? This is probably a specific problem related to how the Symfony Serializer handles datetime objects.

dunglas avatar Jan 29 '19 21:01 dunglas

Kévin, I sent u a DM on twitter with the description of my problem which not only on DateTime, but also on a simple "float" type.

When sending a request containing a string instead of a float for my entity property, the violation is not the @Assert\Type constraint, but an earlier error, coming from the @ORM\Column(type="float").

How would u manage that ?

liorchamla avatar Jan 29 '19 21:01 liorchamla

Are you sure that it's not this feature of the serializer? https://symfony.com/doc/current/components/serializer.html#recursive-denormalization-and-type-safety

Then https://github.com/symfony/symfony/pull/27136 could help.

dunglas avatar Jan 29 '19 21:01 dunglas

Thanks so much for your time and dedication :-)

Ok so if I understand well, we have here the possibility to tell the Serializer that we want the ObjectNormalizer::DISABLE_TYPE_ENFORCEMENT option to be true while denormalizing. But how ? We can see in API Platform that we can customize context's variables like "enable_max_depth" and "groups" and also any other options, directly inside de denormalizationContext option of the ApiResource annotation.

But for this one : ObjectNormalizer::DISABLE_TYPE_ENFORCEMENT, I can't figure out how :'(

Any advice ?

EDIT : by looking at the class itself, I saw that this constant was just "disable_type_enforcement", so I tried this, without success :

/**
 * @ORM\Entity(repositoryClass="App\Repository\InvoiceRepository")
 * @ApiResource(
 *  attributes={"pagination_enabled"=true, "pagination_items_per_page"="5"},
 *  normalizationContext={"groups"={"invoice_read"}},
 *  denormalizationContext={"disable_type_enforcement"=true},
 *  collectionOperations={"get", "post", "api_customers_invoices_get_subresource"={
 *      "normalization_context"={"groups"={"customers_invoices_read"}}
 *  }}
 * )
 */

liorchamla avatar Jan 30 '19 00:01 liorchamla

Your config should work (you can even use constants in Doctrine annotations if you want). Can you post the exact error you have?

dunglas avatar Jan 30 '19 06:01 dunglas

With the code above, here is the json error I receive :

{
    "@context": "/api/contexts/Error",
    "@type": "hydra:Error",
    "hydra:title": "An error occurred",
    "hydra:description": "The type of the \"amount\" attribute must be \"float\", \"string\" given.",
    "trace": [ very big stack trace ]
}

But the code I would like to have is the classical violation format, the ConstraintViolationList code.

liorchamla avatar Jan 30 '19 21:01 liorchamla

Up ?

liorchamla avatar Feb 03 '19 18:02 liorchamla

For me it doesn't works too.

{
    "@context": "/api/contexts/Error",
    "@type": "hydra:Error",
    "hydra:title": "An error occurred",
    "hydra:description": "The type of the \"orderNumber\" attribute must be \"int\", \"NULL\" given.",
    "trace": [
        {
            "namespace": "",
            "short_class": "",
            "class": "",
            "type": "",
            "function": "",
            "file": "/home/vagrant/code/some-domain/vendor/api-platform/core/src/Serializer/AbstractItemNormalizer.php",
            "line": 239,
            "args": []
        },
        {
            "namespace": "ApiPlatform\\Core\\Serializer",
            "short_class": "AbstractItemNormalizer",
            "class": "ApiPlatform\\Core\\Serializer\\AbstractItemNormalizer",
            "type": "->",
            "function": "validateType",
            "file": "/home/vagrant/code/some-domain/vendor/api-platform/core/src/Serializer/AbstractItemNormalizer.php",
            "line": 220,
            "args": [
                [
                    "string",
                    "orderNumber"
                ],
                [
                    "object",
                    "Symfony\\Component\\PropertyInfo\\Type"
                ],
                [
                    "null",
                    null
                ],
                [
                    "string",
                    "json"
                ]
            ]
        },

dawsza avatar Feb 05 '19 09:02 dawsza

To help us all out of this issue, there is all my entity code.

Note that I use denormalizationContext={"disable_type_enforcement"=true}, to try to avoid the problem but it does not seem to be the right thing, it does not work anyway :

/**
 * @ORM\Entity(repositoryClass="App\Repository\InvoiceRepository")
 * @ApiResource(
 *  attributes={"pagination_enabled"=true, "pagination_items_per_page"="5"},
 *  normalizationContext={"groups"={"invoice_read"}},
 *  denormalizationContext={"disable_type_enforcement"=true},
 *  collectionOperations={"get", "post", "api_customers_invoices_get_subresource"={
 *      "normalization_context"={"groups"={"customers_invoices_read"}}
 *  }}
 * )
 */
class Invoice
{
    /**
     * @ORM\Id()
     * @ORM\GeneratedValue()
     * @ORM\Column(type="integer")
     * @Groups({"invoice_read", "customer_read", "customer_item_read", "customers_invoices_read"})
     */
    private $id;

    /**
     * @ORM\Column(type="float")
     * @Groups({"invoice_read", "customer_read", "customer_item_read", "customers_invoices_read"})
     * @Assert\NotBlank(message="Le montant est obligatoire")
     * @Assert\Type(type="numeric", message="Le montant doit être un nombre")
     */
    private $amount;

    ....
}

Now there is my error message when sending a string for the amount field :

{
    "@context": "/api/contexts/Error",
    "@type": "hydra:Error",
    "hydra:title": "An error occurred",
    "hydra:description": "The type of the \"amount\" attribute must be \"float\", \"string\" given.",
    "trace": [
    ....
}

Problem : the validation constraint does not send the error, it seems that the errors comes from the Mapping of the field itself. By the way, the validation constraint works very well if I remove type="float" from the mapping. But I lose the possibility of using migrations etc well.

EDIT : I also tried a thing that "worked", in the Trace, I can see that the exception is thrown by the call to the validateType() method of the AbstractItemNormalizer (line 220) : $this->validateType($attribute, $type, $value, $format);

Commenting this line allows the Validation Constraint to do its job. So I replaced line 220 with that :

if (true !== $context['disable_type_enforcement']) {
     $this->validateType($attribute, $type, $value, $format);
}

And YEAH ! It works !

liorchamla avatar Mar 02 '19 17:03 liorchamla

I agree that this is a design problem. Ideally it should be something like:

  1. Deserialize to DTO.
  2. Validate DTO.
  3. Map from DTO to domain model.
  4. Validate domain model.

And everything will be correct? But "we want RAD", so...

With a single model that's used for both purposes, what you end up with is a ton of compromises, among which is not being able to use proper type hints everywhere.

teohhanhui avatar Mar 05 '19 16:03 teohhanhui

Sorry, that was a bit off topic.

For a RAD use case, perhaps we should look forward to https://github.com/symfony/symfony/pull/27735

Once we have that, we should ship with disable_type_enforcement out of the box. (How?)

teohhanhui avatar Mar 05 '19 16:03 teohhanhui

Is there currently no way to validate a DTO?

The following is a trimmed version of what I am trying to accomplish.

Say I have the following entity:

/**
 * @package App\Entity
 * @ApiResource(
 *     input=CustomerInput::class,
 *     itemOperations={
 *         "get",
 *         "delete"
 *     },
 *     collectionOperations={
 *         "get",
 *         "post"
 *     },
 * )
 */
class Customer
{
    /**
     * @ApiProperty(identifier=true)
     * @Assert\Uuid
     * @var string
     */
    private $id;

    /**
     * @ApiProperty()
     * @Assert\Uuid
     * @var ?string
     */
    private $parentId;

    /**
     * @ApiProperty()
     * @var string
     * @Assert\NotBlank()
     */
    private $name;

    /**
     * Customer constructor.
     * @param string $id
     * @param string|null $parentId
     * @param string $name
     */
    public function __construct(
        string $id,
        ?string $parentId,
        string $name
    ) {
        $this->setId($id);
        $this->setParentId($parentId);
        $this->setName($name);
    }
    .... Getters & setters
}

With the following DTO:

class CustomerInput
{

    /**
     * @var string|null
     */
    public $parentId;

    /**
     * @var string
     * @Assert\NotBlank()
     * @Assert\NotNull()
     */
    public $name;
}

Transformer:

/**
     * @param CustomerInput $data
     * @param string $to
     * @param array $context
     * @return Customer
     * @throws \Exception
     */
    public function transform($data, string $to, array $context = [])
    {
        $id = Uuid::uuid4();
        return new Customer($id, $data->parentId, $data->name);
    }

If I POST /customers with

{
	"parent_id": null,
	"namee": "Test"
}

The following error is thrown:

Argument 3 passed to App\\Customer\\Entity\\Customer::__construct() must be of the type string, null given, called in /......./CustomerInputDataTransformer.php on line 44

The only way I can think to get around this would be to validate myself in the transformer.

jmwri avatar Apr 03 '19 17:04 jmwri

The only way I can think to get around this would be to validate myself in the transformer.

Exactly, I noted this "issue" today as well. I did it in the transformer. I'll look for a better solution in the meantime I would advice you to do the validation inside the transformer.

soyuka avatar Apr 03 '19 22:04 soyuka

I'm new to api-platform. Is the transformer the best place to populate a customer with a new ID considering I am using a custom DataPersister?

jmwri avatar Apr 04 '19 08:04 jmwri

The transformer should only transform the Input to the Entity that represents the end resource (eg tagged with @ApiResource). The DataPersister should handle the persistence for this entity indeed.

soyuka avatar Apr 04 '19 08:04 soyuka

If I don't populate the ID in the transformer then I get: Unable to generate an IRI for the item of type "App\Customer\Entity\Customer"

jmwri avatar Apr 04 '19 16:04 jmwri

Would you be able to give some stack trace? I need to know from where this error comes.

soyuka avatar Apr 04 '19 16:04 soyuka

Sorry please ignore that last comment. I forgot to add the ID in the persister. In the mean time, I'm not really interested in generating an IRI. Is there a way to disable this?

jmwri avatar Apr 04 '19 16:04 jmwri

Override the IriConverter with your own that does nothing I guess :p.

soyuka avatar Apr 05 '19 09:04 soyuka

So, is there any solution to this design problem? Using disable_type_enforcement just makes API platform throw another exception, still without an ability to validate input with the incorrect type.

roman-eonx avatar Sep 09 '19 04:09 roman-eonx

So, is there any solution to this design problem? Using disable_type_enforcement just makes API platform throw another exception, still without an ability to validate input with the incorrect type.

Could you formulate your demand again? I'm not sure to get exactly what you want. Thanks!

soyuka avatar Sep 13 '19 10:09 soyuka

@soyuka, the demand is the same as the author of this issue described: there should be an ability to return a validation error (with the 400 HTTP status code and violations in the response body) instead of a usual exception (with the 500 HTTP status code in the response). Right now, if you have a resource property with some type (eg. float) and pass some value with an incorrect type (eg. a string: 'zzz'), you'll get 500 and The type of the "<prop name>" attribute must be "float", "string" given..

roman-eonx avatar Sep 13 '19 10:09 roman-eonx

Mhh but validation errors do throw a 400 ?!

https://github.com/api-platform/core/blob/345612c913e1aca6da4f4aa1cd885421ca6385ff/src/Bridge/Symfony/Validator/EventListener/ValidationExceptionListener.php

Or the issue here is tight to PHP types where a 500 is thrown because of a php error not a validation error? Maybe that fixing the disable_type_enforcement would do the trick?

soyuka avatar Sep 13 '19 14:09 soyuka

@soyuka, the exception thrown is not a validation error, that's the issue. And disable_type_enforcement doesn't help here, as I mentioned before, we still get a non-validation exception with it.

roman-eonx avatar Sep 16 '19 02:09 roman-eonx