Request must be validated, but not Object after deserialization

errogaht commented 6 years ago

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 commented 6 years ago

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

maks-rafalko commented 6 years ago

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

errogaht commented 6 years ago

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"
        }
    ]
}

maks-rafalko commented 6 years ago

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."
    }
  ]
}

errogaht commented 6 years ago

@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

liorchamla commented 5 years ago

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 :'(

dunglas commented 5 years ago

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.

liorchamla commented 5 years ago

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 ?

dunglas commented 5 years ago

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.

liorchamla commented 5 years ago

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"}}
 *  }}
 * )
 */

dunglas commented 5 years ago

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

liorchamla commented 5 years ago

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 commented 5 years ago

Up ?

dawsza commented 5 years ago

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"
                ]
            ]
        },

liorchamla commented 5 years ago

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 !

teohhanhui commented 5 years ago

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

Deserialize to DTO.
Validate DTO.
Map from DTO to domain model.
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 commented 5 years ago

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?)

jmwri commented 5 years ago

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.