Skip to content

throw NotFound exception when use plain identifiers #1716

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

Closed
ewgRa wants to merge 1 commit into from
Closed

throw NotFound exception when use plain identifiers #1716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/Serializer/AbstractItemNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,13 @@ protected function denormalizeRelation(string $attributeName, PropertyMetadata $
// repeat the code so that IRIs keep working with the json format
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]);

@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.

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

@ewgRa ewgRa Feb 19, 2018

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 •
edited
Loading

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!

@ewgRa ewgRa Feb 19, 2018

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.

@ewgRa ewgRa Feb 22, 2018

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?

@soyuka soyuka Feb 22, 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.

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:

core/src/EventListener/ReadListener.php

Lines 110 to 124 in 45c537f

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?


if (null === $item) {
throw new ItemNotFoundException(sprintf('Item with id "%s" not found for class "%s".', $value, $className));
}

return $item;
} catch (ItemNotFoundException $e) {
throw new InvalidArgumentException($e->getMessage(), $e->getCode(), $e);
} catch (InvalidArgumentException $e) {
Expand Down
121 changes: 120 additions & 1 deletion tests/Serializer/AbstractItemNormalizerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use ApiPlatform\Core\Api\ResourceClassResolverInterface;
use ApiPlatform\Core\DataProvider\ItemDataProviderInterface;
use ApiPlatform\Core\Exception\InvalidArgumentException;
use ApiPlatform\Core\Exception\ItemNotFoundException;
use ApiPlatform\Core\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
use ApiPlatform\Core\Metadata\Property\PropertyMetadata;
Expand Down Expand Up @@ -287,6 +288,73 @@ public function testDenormalize()
], Dummy::class);
}

/**
* @expectedException \ApiPlatform\Core\Exception\InvalidArgumentException
* @expectedExceptionMessage NotFound exception
*/
public function testDenormalizeNotFound()
{
$propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class);
$propertyNameCollectionFactoryProphecy->create(Dummy::class, [])->willReturn(
new PropertyNameCollection(['name', 'relatedDummy', 'relatedDummies'])
)->shouldBeCalled();

$propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class);
$propertyMetadataFactoryProphecy->create(Dummy::class, 'name', [])->willReturn(
new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', false, true)
)->shouldBeCalled();
$propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummy', [])->willReturn(
new PropertyMetadata(
new Type(Type::BUILTIN_TYPE_OBJECT, false, RelatedDummy::class),
'',
false,
true,
false,
false
)
)->shouldBeCalled();
$propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummies', [])->willReturn(
new PropertyMetadata(
new Type(Type::BUILTIN_TYPE_OBJECT,
false,
ArrayCollection::class,
true,
new Type(Type::BUILTIN_TYPE_INT),
new Type(Type::BUILTIN_TYPE_OBJECT, false, RelatedDummy::class)
),
'',
false,
true,
false,
false
)
)->shouldBeCalled();

$iriConverterProphecy = $this->prophesize(IriConverterInterface::class);
$iriConverterProphecy->getItemFromIri('/dummies/1', Argument::type('array'))->willThrow(new ItemNotFoundException('NotFound exception'))->shouldBeCalled();

$propertyAccessorProphecy = $this->prophesize(PropertyAccessorInterface::class);

$resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class);

$serializerProphecy = $this->prophesize(SerializerInterface::class);
$serializerProphecy->willImplement(NormalizerInterface::class);

$normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [
$propertyNameCollectionFactoryProphecy->reveal(),
$propertyMetadataFactoryProphecy->reveal(),
$iriConverterProphecy->reveal(),
$resourceClassResolverProphecy->reveal(),
$propertyAccessorProphecy->reveal(),
]);
$normalizer->setSerializer($serializerProphecy->reveal());

$normalizer->denormalize([
'name' => 'foo',
'relatedDummy' => '/dummies/1',
], Dummy::class);
}

public function testDenormalizeWritableLinks()
{
$relatedDummy1 = new RelatedDummy();
Expand Down Expand Up @@ -676,7 +744,58 @@ public function testDenormalizeRelationWithPlainId()
$serializerProphecy->willImplement(DenormalizerInterface::class);

$itemDataProviderProphecy = $this->prophesize(ItemDataProviderInterface::class);
$itemDataProviderProphecy->getItem(RelatedDummy::class, 1, null, Argument::type('array'))->shouldBeCalled();
$itemDataProviderProphecy->getItem(RelatedDummy::class, 1, null, Argument::type('array'))->willReturn(new RelatedDummy())->shouldBeCalled();

$normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [
$propertyNameCollectionFactoryProphecy->reveal(),
$propertyMetadataFactoryProphecy->reveal(),
$iriConverterProphecy->reveal(),
$resourceClassResolverProphecy->reveal(),
$propertyAccessorProphecy->reveal(),
null,
null,
$itemDataProviderProphecy->reveal(),
true,
]);
$normalizer->setSerializer($serializerProphecy->reveal());

$normalizer->denormalize(['relatedDummy' => 1], Dummy::class, 'jsonld');
}

/**
* @expectedException \ApiPlatform\Core\Exception\InvalidArgumentException
* @expectedExceptionMessage Item with id "1" not found for class "ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy".
*/
public function testDenormalizeNotFoundRelationWithPlainId()
{
$propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class);
$propertyNameCollectionFactoryProphecy->create(Dummy::class, [])->willReturn(
new PropertyNameCollection(['relatedDummy'])
)->shouldBeCalled();

$propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class);
$propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummy', [])->willReturn(
new PropertyMetadata(
new Type(Type::BUILTIN_TYPE_OBJECT, false, RelatedDummy::class),
'',
false,
true,
false,
false
)
)->shouldBeCalled();

$iriConverterProphecy = $this->prophesize(IriConverterInterface::class);
$propertyAccessorProphecy = $this->prophesize(PropertyAccessorInterface::class);

$resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class);
$resourceClassResolverProphecy->isResourceClass(RelatedDummy::class)->willReturn(true)->shouldBeCalled();

$serializerProphecy = $this->prophesize(SerializerInterface::class);
$serializerProphecy->willImplement(DenormalizerInterface::class);

$itemDataProviderProphecy = $this->prophesize(ItemDataProviderInterface::class);
$itemDataProviderProphecy->getItem(RelatedDummy::class, 1, null, Argument::type('array'))->willReturn(null)->shouldBeCalled();

$normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [
$propertyNameCollectionFactoryProphecy->reveal(),
Expand Down