Skip to content

Allow updating nested resource on POST #1625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 13, 2018
Merged

Allow updating nested resource on POST #1625

merged 1 commit into from
Jan 13, 2018

Conversation

jdeniau
Copy link
Contributor

@jdeniau jdeniau commented Jan 3, 2018 •
edited
Loading

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

Allow updating nested resource on POST

@jdeniau jdeniau force-pushed the jd-fix-nestedUpdate branch 2 times, most recently from 5244bea to 966a5eb Compare January 4, 2018 13:22
dunglas
dunglas requested changes Jan 10, 2018
View reviewed changes
features/hydra/error.feature Outdated
And the response should be in JSON
And the JSON node "@id" should be equal to "/related_dummies/1"
And the JSON node "symfony" should be equal to "laravel"

When I add "Content-Type" header equal to "application/ld+json"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be split in several scenarios (a scenario should not have several When steps).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the first lines are to check that the insertion is correct (mainly that the @id is /related_dummies/1). I can change the line 109 from When to Given.

Copy link
Member

@dunglas dunglas Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should follow the format:

Given ...
And ...
When ...
And ...
Then ...
And ...

You can also split it in two scenarios.
Then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dunglas done

src/JsonLd/Serializer/ItemNormalizer.php Outdated
@@ -98,7 +98,9 @@ public function denormalize($data, $class, $format = null, array $context = [])
{
// Avoid issues with proxies if we populated the object
if (isset($data['@id']) && !isset($context[self::OBJECT_TO_POPULATE])) {
if (isset($context['api_allow_update']) && true !== $context['api_allow_update']) {
if (isset($context['api_allow_update']) && true !== $context['api_allow_update']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about if (!($context['api_allow_update'] ?? false) && !($context['api_resource_is_relation'] ?? false)) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean if (!($context['api_allow_update'] ?? true) ... because now true is the default value for api_allow_update (another way of saying is that an unset api_allow_update wont go if the if).
The only difference here is if the value is set but not boolean, but that "should" not append (except if a user set it manually in the serialization context).

I like it but can maybe be hard to read, but why not.

As a matter of fact, api_allow_update is always set in the SerializerContextBuilder so I'm wondering if someone can denormalize an item without going through the context builder ?
If no, the code can be true !== $context['api_allow_update'] (but it may be safer to keep the isset check.

src/Serializer/AbstractItemNormalizer.php Outdated
@@ -297,6 +297,7 @@ private function denormalizeRelation(string $attributeName, PropertyMetadata $pr
($propertyMetadata->isWritableLink() && \is_array($value))
) {
$context['resource_class'] = $className;
$context['api_resource_is_relation'] = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be easier to just set api_allow_update to true for the subcontext in this case instead of introducing a new flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently it works, and probably fixes the src/Serializer/ItemNormalizer.php that triggers the same error that I missed, I will do this.

@jdeniau jdeniau force-pushed the jd-fix-nestedUpdate branch 2 times, most recently from e5376f4 to a378c76 Compare January 11, 2018 12:37
dunglas
dunglas reviewed Jan 11, 2018
View reviewed changes
features/hydra/error.feature Outdated
@@ -103,23 +103,39 @@ Feature: Error handling
And the JSON node "hydra:description" should be equal to "Update is not allowed for this operation."
And the JSON node "trace" should exist

Scenario: Get an error during update of an existing relation with a non-allowed update operation
@dropSchema
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this one, but be sure to add @dropSchema on the last scenario to cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dunglas I missed the fact that @dropSchema is AfterScenario.

I took the liberty of adding a new @recreateSchema that drop and create the schema, because I don't think relying on what did appear on the previous scenario is a good think. (mainly because someone may add another scenario between.

Copy link
Member

@dunglas dunglas Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if I agree that it's cleaner, the problem with recreating the schema often is that is slow down all our builds...
If someone add another scenario between, it's his build that will fail 😄. His responsibility.

Would you mind to just use create/drop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dunglas I did that, and opened #1642 to discuss about the createSchema vs recreateSchema

@jdeniau jdeniau force-pushed the jd-fix-nestedUpdate branch 3 times, most recently from fe5c7df to 10b01ce Compare January 11, 2018 15:02
@jdeniau jdeniau force-pushed the jd-fix-nestedUpdate branch from 10b01ce to 99bc0b0 Compare January 11, 2018 15:16
@jdeniau jdeniau force-pushed the jd-fix-nestedUpdate branch from c76005b to 7f58508 Compare January 12, 2018 22:38
@dunglas dunglas merged commit 2d3f143 into api-platform:2.1 Jan 13, 2018
@dunglas
Copy link
Member

dunglas commented Jan 13, 2018

Thanks @jdeniau

@jdeniau jdeniau deleted the jd-fix-nestedUpdate branch January 13, 2018 11:52
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
@dunglas
Merge pull request api-platform#1625 from jdeniau/jd-fix-nestedUpdate
5829ba8 
Allow updating nested resource on POST
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdeniau @dunglas