Skip to content
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

Add the readOnly field for Swagger #764

Merged
merged 1 commit into from
Sep 28, 2016
Merged

Add the readOnly field for Swagger #764

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
11 changes: 11 additions & 0 deletions src/Swagger/Serializer/DocumentationNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use ApiPlatform\Core\Api\ResourceClassResolverInterface;
use ApiPlatform\Core\Api\UrlGeneratorInterface;
use ApiPlatform\Core\Documentation\Documentation;
use ApiPlatform\Core\Exception\RuntimeException;
use ApiPlatform\Core\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
use ApiPlatform\Core\Metadata\Property\PropertyMetadata;
Expand Down Expand Up @@ -364,6 +365,8 @@ private function getDefinition(\ArrayObject $definitions, bool $collection, bool
* @param ResourceMetadata $resourceMetadata
* @param array|null $serializerContext
*
* @throws RuntimeException
*
* @return \ArrayObject
*/
private function getDefinitionSchema(string $resourceClass, ResourceMetadata $resourceMetadata, array $serializerContext = null) : \ArrayObject
Expand All @@ -384,6 +387,10 @@ private function getDefinitionSchema(string $resourceClass, ResourceMetadata $re
$normalizedPropertyName = $this->nameConverter ? $this->nameConverter->normalize($propertyName) : $propertyName;

if ($propertyMetadata->isRequired()) {
if (false === $propertyMetadata->isWritable()) {
throw new RuntimeException(sprintf('The property "%s" of the resource "%s" can not be required and read-only at the same time.', $propertyName, $resourceClass));
Copy link
Contributor

@teohhanhui teohhanhui Sep 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree.

Copy link
Member

@dunglas dunglas Sep 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why? If a property is required, by definition it should be writeable no?
Swagger does the same thing btw.

Copy link
Contributor

@teohhanhui teohhanhui Sep 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This is really problematic, because read-only properties can be "required" too, i.e. they cannot be set to null or the empty string. Swagger operates in a different context than ours.

For example, we set withRequired(true) here:

core/src/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactory.php

Line 61 in 0f88647

return $propertyMetadata->withRequired(true);

Copy link
Member

Choose a reason for hiding this comment

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

IMO this loader should be fixed and not set required to true if the property isn't writeable. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Our metadata system does not provide enough flexibility to do this correctly. Let's say if writable is set to false after ValidatorPropertyMetadataFactory, then what?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we again explicitly call withRequired(false) each time we call withWritable(false). You can see how this quickly gets out of hand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless this logic can be embedded in PropertyMetadata itself...

Copy link
Member

Choose a reason for hiding this comment

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

Embedding it in PropertyMetadata is a good idea IMO.

}

$definitionSchema['required'][] = $normalizedPropertyName;
}

Expand All @@ -406,6 +413,10 @@ private function getPropertySchema(PropertyMetadata $propertyMetadata) : \ArrayO
{
$propertySchema = new \ArrayObject();

if (false === $propertyMetadata->isWritable()) {
$propertySchema['readOnly'] = true;
}

if (null !== $description = $propertyMetadata->getDescription()) {
$propertySchema['description'] = $description;
}
Expand Down
49 changes: 48 additions & 1 deletion tests/Swagger/Serializer/DocumentationNormalizerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,14 @@ public function testNormalize()
$documentation = new Documentation(new ResourceNameCollection([Dummy::class]), 'Test API', 'This is a test API.', '1.2.3', ['jsonld' => ['application/ld+json']]);

$propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class);
$propertyNameCollectionFactoryProphecy->create(Dummy::class, [])->shouldBeCalled()->willReturn(new PropertyNameCollection(['name']));
$propertyNameCollectionFactoryProphecy->create(Dummy::class, [])->shouldBeCalled()->willReturn(new PropertyNameCollection(['id', 'name']));

$dummyMetadata = new ResourceMetadata('Dummy', 'This is a dummy.', 'http://schema.example.com/Dummy', ['get' => ['method' => 'GET'], 'put' => ['method' => 'PUT']], ['get' => ['method' => 'GET'], 'post' => ['method' => 'POST'], 'custom' => ['method' => 'GET', 'path' => '/foo'], 'custom2' => ['method' => 'POST', 'path' => '/foo']], []);
$resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
$resourceMetadataFactoryProphecy->create(Dummy::class)->shouldBeCalled()->willReturn($dummyMetadata);

$propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class);
$propertyMetadataFactoryProphecy->create(Dummy::class, 'id')->shouldBeCalled()->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_INT), 'This is an id.', true, false));
$propertyMetadataFactoryProphecy->create(Dummy::class, 'name')->shouldBeCalled()->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), 'This is a name.', true, true, true, true, false, false, null, null, []));
$resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class);
$resourceClassResolverProphecy->isResourceClass(Dummy::class)->willReturn(true);
Expand Down Expand Up @@ -224,6 +225,11 @@ public function testNormalize()
'description' => 'This is a dummy.',
'externalDocs' => ['url' => 'http://schema.example.com/Dummy'],
'properties' => [
'id' => new \ArrayObject([
'type' => 'integer',
'description' => 'This is an id.',
'readOnly' => true,
]),
'name' => new \ArrayObject([
'type' => 'string',
'description' => 'This is a name.',
Expand All @@ -236,6 +242,47 @@ public function testNormalize()
$this->assertEquals($expected, $normalizer->normalize($documentation));
}

/**
* @expectedException \ApiPlatform\Core\Exception\RuntimeException
* @expectedExceptionMessage The property "id" of the resource "ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy" can not be required and read-only at the same time.
*/
public function testNormalizeThrowsExceptionWithAReadOnlyAndRequiredProperty()
{
$documentation = new Documentation(new ResourceNameCollection([Dummy::class]), 'Dummy API', 'This is a dummy API', '1.2.3', ['jsonld' => ['application/ld+json']]);

$propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class);
$propertyNameCollectionFactoryProphecy->create(Dummy::class, [])->shouldBeCalled()->willReturn(new PropertyNameCollection(['id']));

$dummyMetadata = new ResourceMetadata('Dummy', 'This is a dummy.', null, ['post' => ['method' => 'POST']], [], []);
$resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
$resourceMetadataFactoryProphecy->create(Dummy::class)->shouldBeCalled()->willReturn($dummyMetadata);

$propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class);
$propertyMetadataFactoryProphecy->create(Dummy::class, 'id')->shouldBeCalled()->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), 'This is an id.', true, false, null, null, true));

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

$operationMethodResolverProphecy = $this->prophesize(OperationMethodResolverInterface::class);
$operationMethodResolverProphecy->getItemOperationMethod(Dummy::class, 'post')->shouldBeCalled()->willReturn('POST');

$urlGeneratorProphecy = $this->prophesize(UrlGeneratorInterface::class);

$operationPathResolver = new CustomOperationPathResolver(new UnderscoreOperationPathResolver());

$normalizer = new DocumentationNormalizer(
$resourceMetadataFactoryProphecy->reveal(),
$propertyNameCollectionFactoryProphecy->reveal(),
$propertyMetadataFactoryProphecy->reveal(),
$resourceClassResolverProphecy->reveal(),
$operationMethodResolverProphecy->reveal(),
$operationPathResolver,
$urlGeneratorProphecy->reveal()
);

$normalizer->normalize($documentation);
}

public function testNormalizeWithNameConverter()
{
$documentation = new Documentation(new ResourceNameCollection([Dummy::class]), 'Dummy API', 'This is a dummy API', '1.2.3', ['jsonld' => ['application/ld+json']]);
Expand Down