Core: Assert\NotBlank() doesn't prevent AbstractItemNormalizer error

[kate-kate]

Hi guys! I have an entity with related field like this:

    /**
     * @Groups({"dashboard","create_manual"})
     * @Assert\NotBlank(groups={"create_manual"})
     *
     * @ORM\ManyToOne(targetEntity="Campaign")
     */
    private $campaign;

where "create_manual" is a denormalization context group.

And then comes PUT request with "campaign":"" (because there is a select in the frontend, which is sending campaign IRI from dropdown or empty string if user doesn't choose any).

I expected validation error for this variant, but I get error: Type error: Argument 2 passed to ApiPlatform\Core\Metadata\Property\Factory\CachedPropertyMetadataFactory::create() must be of the type string, integer given, called in /path-to-my-project/vendor/api-platform/core/src/Serializer/AbstractItemNormalizer.php on line 150.

I saw exactly the same error, once I tried to create embedded entity and have forgotten to use correct denormalization_context.

But I don't expect any objects here. I'm waiting for correct IRI or at least empty string, which may cause validation error.

So, what's wrong?

[bwegrzyn]

I'm guessing the error is caused by strict types being declared on all files in api platform recently. I'm not sure why its trying to pass an integer though.

Can you post the full error?

Edit: Also, do you have the proper config on the @ApiResource annotation if you are using groups with your validators? The validation groups are not the same as groups on the normalization/denormalization context.

/**
 * @ApiResource(attributes={"validation_groups"={"create_manual"}})
 */

[kate-kate]

@bwegrzyn of course, validation groups are not the same as groups on the normalization/denormalization context:) But I named them the same (it seems more simple for me). This is full @ApiResource annotation of my entity:

/**
 * @ApiResource(attributes={
 *     "validation_groups"={"create_manual"},
 *     "normalization_context"={"groups"={"dashboard"}},
 *     "denormalization_context"={"groups"={"create_manual"}},
 *     "pagination_client_enabled"=false,
 *     "filters"={"job.search", "job.date", "job.order"}
 * })
 */

And here is the full error response: https://gist.github.com/Simperfit/73d7d55191248910f7110be8891a756a

[kate-kate]

Also I've created test with empty string passed to campaign and debugged it. At first setAttributeValue() method receive "campaign" as well, but then something happens, and it really get 0 as attribute. Maybe I will found what's wrong, looks like an empty string was deserialized as attribute named 0.

UPD: Yes, denormalizer "understands" that campaign is an object with attribute 0 and its value is "".

Since AbstractObjectNormalizer on 180 line checks that "campaign" is a className, it tries to denormalizeRelation().

Then we try to getItemFromIri() where IRI is an empty string, so method throws InvalidArgumentException, but catch does nothing and there is comment //Give a chance to other normalizers (e.g.: DateTimeNormalizer):)

So we give it a chance and on 288 line in AbstractItemNormalizer we try to denormalize it, data is equals to "", after prepareForDenormalization() data equals to [0=>""] and we got this 0-attribute

UPD2: Also I made few experiments and found, that if I just add

private function denormalizeRelation(string $attributeName, PropertyMetadata $propertyMetadata, string $className, $value, string $format = null, array $context)
{
    if (is_string($value)) {
        try {
            return $this->iriConverter->getItemFromIri($value, $context + ['fetch_data' => true]);
        } catch (ItemNotFoundException $e) {
            throw new InvalidArgumentException($e->getMessage(), $e->getCode(), $e);
        } catch (InvalidArgumentException $e) {
            // If value is an empty string and property is a writable link or resource class it should be set to null
            // so we return null and it will be transferred to setValue()
            if ($value === "" && ($this->resourceClassResolver->isResourceClass($className) || $propertyMetadata->isWritableLink())) {
                return null;
            }
            // Otherwise give a chance to other normalizers (e.g.: DateTimeNormalizer)
        }
    }
    // rest of the code...
}

everything works well and I finally see validation error "campaign: This value should not be blank."
I have tested it with writable nested entity, with field expecting only IRI, and also with simple not blank field - it works well

[bwegrzyn]

Nice job on debugging the issue. Can you open a pull request with the fix and some additional tests?

[dunglas]

Thanks you for the debugging and the very clear explanation @kate-kate.

[dunglas]

IIUC, the problem is that the type is not enforced here. It should be an array or an IRI, nothing else. Testing for '' only partially fix the problem. In this specific case, we should not let other normalizers a change because only an IRI or an array containing a valid document is expected.

[kate-kate]

hello @dunglas, glad that you noticed this issue. IMO after InvalidArgumentException there should not be "a chance to other normalizers (e.g.: DateTimeNormalizer)", cause we normalizing relation and only string variant is IRI. But also it should raise validation error, not an exception, I think, because in the web in dropdowns default value often will be ''

[Padam87]

I think I have found a related issue.

I have created a test app, just to look at the api platform before my next major project. I have a title field:

    /**
     * @var string
     *
     * @ORM\Column()
     * @Assert\NotBlank()
     * @Serializer\Groups({"test"})
     */
    protected $title;

If the post contains a null value:
The type of the "title" attribute must be "string", "NULL" given.

If an empty string is posted:
Not blank message, and a list of violations, as it should be.

[ihorsamusenko]

Any updates on the issue? Why https://github.com/api-platform/core/pull/1077 is not merged yet?

[kate-kate]

Hi again @dunglas and others! Apparently, I have participated yet in another project on API Platform. After I've created a huge test for my code, I saw this error and was referred to this issue again :)
As I remember, after some code style enhancements and @soyuka advices, my fix has passed all tests and I even made one more test for this exact problem. So, is anything missing to merge it?

[sverraest]

Running into the exact same issue. Anything blocking this merge?

[soyuka]

https://github.com/symfony/validator/commit/e9cf9f4401fa1e6262360a80502b5acede5a937c should fix this issue