Skip to content

throw NotFound exception when use plain identifiers#1716

Closed
ewgRa wants to merge 1 commit into
api-platform:masterfrom
ewgRa:item-not-found-for-plain-id
Closed

throw NotFound exception when use plain identifiers#1716
ewgRa wants to merge 1 commit into
api-platform:masterfrom
ewgRa:item-not-found-for-plain-id

Conversation

@ewgRa

@ewgRa ewgRa commented Feb 19, 2018

Copy link
Copy Markdown
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

There is PR #1365 that introduce "allow_plain_identifiers". But there is a problem, if you send identifier for entity that not present in database, it try to set it to object as "null", and if this field is required like "setFoo(Foo $foo)" - than we have 500 error.

Default DataProvider never throw ItemNotFound, since have "return $queryBuilder->getQuery()->getOneOrNullResult();", and AbstractItemNormalizer never have "} catch (ItemNotFoundException $e) {"

This PR fix this and behavior when we use "allow_plain_identifiers" becomes same as with iriConverter.

@ewgRa

ewgRa commented Feb 19, 2018

Copy link
Copy Markdown
Author

travis fail looks like not related

if (true === $this->allowPlainIdentifiers && $this->itemDataProvider) {
try {
return $this->itemDataProvider->getItem($className, $value, null, $context + ['fetch_data' => true]);
$item = $this->itemDataProvider->getItem($className, $value, null, $context + ['fetch_data' => true]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's the itemDataProvider that should be throwing th ItemNotFoundException not the Normalizer.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think about it, but it can brake backward compatibility. I actually don't know very well api-platform and where ItemDataProvider used and can't predict what happens with all such places if ItemDataProvider start throwing this exception. Is there everything will be fine?

If ok, I will make change tomorrow.

@soyuka soyuka Feb 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh my bad you're right, seems we're using doing this in the ReadListener. I'll take a closer look tomorrow!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If it is only one place, than we can change it too. Problem only with custom data providers and who use it with "allow_plain_identifiers". They need to change their DataProviders and add throw for this exception. But since nobody complain about this, looks like it will be not a problem. But anyway, here is potential brake of BC for such developers, decision up to you, I don't know how strict you are with BC.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ping @soyuka , so, throw exception at ItemDataProvider and change ReadListener also or leave it as is?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave it as is for now.

@dunglas I thought that we could add a method in the IriConverter that would do this portion of code:

private function getItemData(Request $request, array $attributes, array $context)
{
$id = $request->attributes->get('id');
try {
$data = $this->itemDataProvider->getItem($attributes['resource_class'], $id, $attributes['item_operation_name'], $context);
} catch (PropertyNotFoundException $e) {
$data = null;
}
if (null === $data) {
throw new NotFoundHttpException('Not Found');
}
return $data;
}

We could then deduplicate some code, wdyt?

@Simperfit Simperfit requested a review from dunglas April 6, 2018 14:01
@Simperfit

Copy link
Copy Markdown
Contributor

Could you please rebase with master ?

@soyuka

soyuka commented Apr 6, 2018

Copy link
Copy Markdown
Member

I think that this has drastically change with the new identifier normalizer stuff. Maybe it's not even needed anymore?

Please rebase and review if the bug still exists but I doubt it and #1478 should fix it.

@ewgRa

ewgRa commented Apr 6, 2018

Copy link
Copy Markdown
Author

@soyuka rebased

If I rollback my changes in AbstractItemNormalizer then testDenormalizeNotFoundRelationWithPlainId test still fail

@soyuka

soyuka commented Apr 6, 2018

Copy link
Copy Markdown
Member

That's okay for now but we really need to improve this. Having itemDataProvider in this class is wrong, I have another fix in mind but I'm not sure if it would work.

@flug

flug commented Nov 14, 2019

Copy link
Copy Markdown
Contributor

@ewgRa Up is this pull request still valid?

@ewgRa

ewgRa commented Nov 14, 2019

Copy link
Copy Markdown
Author

Actually I don't know, do not follow latest changes.

Since it is more than one year old, I think it can be closed without pain.

@soyuka

soyuka commented Nov 15, 2019

Copy link
Copy Markdown
Member

Closing let's keep track of this in #2686

@soyuka soyuka closed this Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants