Fix relations denormalization with plain ids #1855

Taluu · 2018-04-12T10:19:28Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1840
License	MIT
Doc PR	~

#1547 introduced an is_array check to prevent the normalize to act on writable relations so that we could use plain identifiers, and was reverted by #1762 because not all relations are arrays, and can be iris.

But this revert triggered also the fact that we cannot use plain ids anymore on writable relations. This PR aims to fix that ; if we encounter the invalid argument exception while trying to denormalize the relation, and that we allowed plain identifiers and we have an item provider, we should try the next block if it's not an array. Now that I think about it, I should probably add a !is_array check or something ?

The tests passes, but I'm not sure how I can add a test to reflect this, so any hint would be welcomed...

BTW, shouldn't we add a more specific exception for the invalid iri ? Testing the message isn't specially clean IMO... and as it's not the scope of this PR, I had to make do with this.

Simperfit · 2018-04-12T10:31:33Z

src/Serializer/AbstractItemNormalizer.php

+            try {
+                return $this->serializer->denormalize($value, $className, $format, $context);
+            } catch (InvalidArgumentException $e) {
+                if ('Invalid value provided (invalid IRI?).' !== $e->getMessage()) {


Maybe it could be better to introduce a new Exception extending the InvalidArgumentException.
That said, we wouldn't have to the check the message, WDYT ?
https://github.com/api-platform/core/blob/master/src/Serializer/AbstractItemNormalizer.php#L164

Yep, added a comment on that on my description, but IMO this is not really the scope of this PR, right ? Or should I change it anyway ?

soyuka · 2018-04-12T13:20:50Z

src/Exception/InvalidValueException.php

+
+namespace ApiPlatform\Core\Exception;
+
+class InvalidValueException extends InvalidArgumentException


What about reusing InvalideIdentifierException (backport it from master)?

It doesn't seem to extend / implement the invalid argument exception, Wouldn't that be a BC Break ?

Don't think so. Anyway I'd be fine with a new exception just trying to avoid multiplying them :p. Maybe we can call this one InvalidIRIException?

I'm not against multiplying exceptions. It's just that in 2.2.4 (and 2.2.5), we should expect a InvalidArgumentException (or a child of it), not something else, e.g Exception.

Unless you meant copying / pasting the code from the InvalidIdentifierException ? Which is not clean too (not saying that testing a reason is better though :P)

soyuka · 2018-04-12T13:21:44Z

src/Serializer/AbstractItemNormalizer.php

+            try {
+                return $this->serializer->denormalize($value, $className, $format, $context);
+            } catch (InvalidValueException $e) {
+                if ('invalid IRI?' !== $e->getReason()) {


Wait what? Shouldn't we have 1 exception per case to avoid this comparison?

Well, wasn't sure if we should have an invalid value exception for the invalid iri, but I could alter that to use one instead ?

Not sure I got that. This exception is only thrown when IRI is invalid right?

Yeah, but the part between parentheses looked variable, that's why I added a reason. I can merge it if needed

Sorry I really don't follow I think I'm missing something, has been a long day haha

Well, we had Invalid value (Invalid IRI?)., so I thought that we could have multiple Invalid Value (reason), and it could sometimes be Invalid Value (Invalid IRI?). That's all, but then, I can make a InvalidIriException and be done with it :P

Yes please :D

Taluu · 2018-04-13T13:55:05Z

Updated @soyuka but not satisfied with the tests, looks like something is wrong, as the result is null for the relatedDummy. Need to check out why :/

soyuka · 2018-04-13T14:42:31Z

src/Exception/InvalidIriException.php

+
+    public function __construct(int $code = 0, \Throwable $previous = null)
+    {
+        parent::__construct("Invalid value provided (Invalid IRI?).", $code, $previous);


Can't we just remove everything in this class and call the exception with a message?

soyuka · 2018-04-13T14:42:55Z

src/Serializer/AbstractItemNormalizer.php

@@ -161,7 +162,7 @@ protected function getAllowedAttributes($classOrObject, array $context, $attribu
    protected function setAttributeValue($object, $attribute, $value, $format = null, array $context = [])
    {
        if (!\is_string($attribute)) {
-            throw new InvalidArgumentException('Invalid value provided (invalid IRI?).');
+            throw new InvalidIriException();


throw new InvalidIriException('Invalid value provided (invalid IRI?).');

What would be the point of the exception if the message is not always the same one ? It's like using a generic exception... look at the exceptions from symfony, they each have somehting customisable but a defined (and thus customizable) message (even though the message is also quite pointless and not used)

Because we can have different reasons for an invalid IRI exception:

bad format

route not found

data not found

for example

Yeah that's why I had the (invalid IRI?) variable, so we would always have Invalid value provider (REASON). But ¯\_(ツ)_/¯ because it's the same thing actually. :}

I agree with @soyuka (and every other exception we have for now work like this). You can also add the message as the default value of the $message parameter.

Yep, changed and removed the constructor altogether, as @soyuka suggested #1855 (comment)

soyuka

Oh tests don't pass though

Taluu · 2018-04-25T23:57:21Z

Indeed, I have some problems with the feature actually. Need some help looking into that, why it doesn't seem to take the plain id into account...

dunglas · 2018-05-09T09:28:12Z

src/Serializer/AbstractItemNormalizer.php

@@ -161,7 +162,7 @@ protected function getAllowedAttributes($classOrObject, array $context, $attribu
    protected function setAttributeValue($object, $attribute, $value, $format = null, array $context = [])
    {
        if (!\is_string($attribute)) {
-            throw new InvalidArgumentException('Invalid value provided (invalid IRI?).');
+            throw new InvalidValueException('Invalid value provided (Invalid IRI?).');


the message should be reverted (why an upper case I)?

If a resource property is writable, we can't use plain identifiers, and end up with a iri error, while we explicit allowed plain identifiers.

Taluu · 2018-05-09T15:17:33Z

Looks like I managed to fix the tests 🎉

dunglas · 2018-05-09T15:57:31Z

Thanks @Taluu

Taluu changed the base branch from master to 2.2 April 12, 2018 10:19

Taluu changed the title ~~Fix plain identifiers iri~~ Fix relations denormalization with plain ids Apr 12, 2018

Simperfit reviewed Apr 12, 2018

View reviewed changes

Simperfit requested review from dunglas and soyuka April 12, 2018 10:31

Simperfit added the [RFR] Ready for Review label Apr 12, 2018

soyuka reviewed Apr 12, 2018

View reviewed changes

Taluu force-pushed the fix-plain-identifiers-iri branch 3 times, most recently from bdfe70f to aae76af Compare April 13, 2018 13:54

soyuka reviewed Apr 13, 2018

View reviewed changes

Taluu force-pushed the fix-plain-identifiers-iri branch 2 times, most recently from c197b46 to 83cbb7a Compare April 25, 2018 14:59

soyuka approved these changes Apr 25, 2018

View reviewed changes

soyuka reviewed Apr 25, 2018

View reviewed changes

dunglas requested changes May 9, 2018

View reviewed changes

Taluu added 2 commits May 9, 2018 16:52

On invalid iri error with plain identifiers, use the data provider

f09647e

If a resource property is writable, we can't use plain identifiers, and end up with a iri error, while we explicit allowed plain identifiers.

Make an appropriare exception for invalid value on item normalizer

28a8259

Taluu force-pushed the fix-plain-identifiers-iri branch from 83cbb7a to b80916b Compare May 9, 2018 14:52

Add a behat test for plain id relations

cd0dd88

Taluu force-pushed the fix-plain-identifiers-iri branch from b80916b to cd0dd88 Compare May 9, 2018 15:01

soyuka approved these changes May 9, 2018

View reviewed changes

dunglas approved these changes May 9, 2018

View reviewed changes

dunglas merged commit 7a680f1 into api-platform:2.2 May 9, 2018

Taluu deleted the fix-plain-identifiers-iri branch May 9, 2018 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix relations denormalization with plain ids #1855

Fix relations denormalization with plain ids #1855

Taluu commented Apr 12, 2018 •

edited

Loading

Simperfit Apr 12, 2018 •

edited

Loading

Taluu Apr 12, 2018

soyuka Apr 12, 2018

Taluu Apr 12, 2018

soyuka Apr 12, 2018

Taluu Apr 13, 2018

Taluu Apr 13, 2018 •

edited

Loading

soyuka Apr 12, 2018

Taluu Apr 12, 2018

soyuka Apr 12, 2018

Taluu Apr 12, 2018

soyuka Apr 12, 2018

Taluu Apr 13, 2018

soyuka Apr 13, 2018

Taluu commented Apr 13, 2018

soyuka Apr 13, 2018

soyuka Apr 13, 2018

Taluu Apr 13, 2018

soyuka Apr 18, 2018

Taluu Apr 18, 2018

dunglas Apr 25, 2018

Taluu Apr 25, 2018

soyuka left a comment

Taluu commented Apr 25, 2018

dunglas May 9, 2018

Taluu commented May 9, 2018

dunglas commented May 9, 2018


		namespace ApiPlatform\Core\Exception;

		class InvalidValueException extends InvalidArgumentException

Fix relations denormalization with plain ids #1855

Fix relations denormalization with plain ids #1855

Conversation

Taluu commented Apr 12, 2018 • edited Loading

Simperfit Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taluu Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taluu commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka left a comment

Choose a reason for hiding this comment

Taluu commented Apr 25, 2018

Choose a reason for hiding this comment

Taluu commented May 9, 2018

dunglas commented May 9, 2018

Taluu commented Apr 12, 2018 •

edited

Loading

Simperfit Apr 12, 2018 •

edited

Loading

Taluu Apr 13, 2018 •

edited

Loading