Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 8 additions & 0 deletions features/main/operation.feature
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,11 @@ Feature: Operation support
}
}
"""

Scenario: Get the collection of a resource that doesn't have a defined item operation
When I send a "GET" request to "/disable_item_operations"
Then the response status code should be 200

Scenario: Do not get a resource that doesn't have a defined item operation
When I send a "GET" request to "/disable_item_operations/1"
Then the response status code should be 404
29 changes: 29 additions & 0 deletions src/Action/NotFoundAction.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace ApiPlatform\Core\Action;

use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

/**
* Not found action.
*
* @author Antoine Bluchet <soyuka@gmail.com>
*/
final class NotFoundAction
{
public function __invoke()
{
throw new NotFoundHttpException();
}
}
2 changes: 2 additions & 0 deletions src/Bridge/Symfony/Bundle/Resources/config/api.xml
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,8 @@
<service id="api_platform.action.put_item" alias="api_platform.action.placeholder" public="true" />
<service id="api_platform.action.delete_item" alias="api_platform.action.placeholder" public="true" />
<service id="api_platform.action.get_subresource" alias="api_platform.action.placeholder" public="true" />
<service id="api_platform.action.not_found" class="ApiPlatform\Core\Action\NotFoundAction" public="true" />
<service id="ApiPlatform\Core\Action\NotFoundAction" alias="api_platform.action.not_found" public="true" />

<service id="api_platform.action.entrypoint" class="ApiPlatform\Core\Action\EntrypointAction" public="true">
<argument type="service" id="api_platform.metadata.resource.name_collection_factory" />
Expand Down
2 changes: 1 addition & 1 deletion src/JsonSchema/SchemaFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ private function getMetadata(string $resourceClass, string $type = Schema::TYPE_
$inputOrOutput = $resourceMetadata->getTypedOperationAttribute($operationType, $operationName, $attribute, ['class' => $resourceClass], true);
}

if (null === ($inputOrOutput['class'] ?? null)) {
if (false === ($inputOrOutput['class'] ?? false)) {

@teohhanhui teohhanhui Sep 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

// input or output disabled
return null;
}
Expand Down
21 changes: 21 additions & 0 deletions src/Metadata/Resource/Factory/OperationResourceMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

namespace ApiPlatform\Core\Metadata\Resource\Factory;

use ApiPlatform\Core\Action\NotFoundAction;
use ApiPlatform\Core\Metadata\Resource\ResourceMetadata;

/**
Expand Down Expand Up @@ -81,6 +82,13 @@ public function create(string $resourceClass): ResourceMetadata
$resourceMetadata = $this->normalize(false, $resourceClass, $resourceMetadata, $itemOperations);
}

if ($this->needsDefaultGetOperation($resourceMetadata)) {
$resourceMetadata = $resourceMetadata->withItemOperations(array_merge(
$resourceMetadata->getItemOperations(),
['get' => ['method' => 'GET', 'read' => false, 'output' => ['class' => false], 'controller' => NotFoundAction::class]])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe you've made a mistake here, it's supposed to be null:

if (false === $attribute) {
return ['class' => null];
}

);
}

$graphql = $resourceMetadata->getGraphql();
if (null === $graphql) {
$resourceMetadata = $resourceMetadata->withGraphql(['item_query' => [], 'collection_query' => [], 'delete' => [], 'update' => [], 'create' => []]);
Expand Down Expand Up @@ -148,4 +156,17 @@ private function normalizeGraphQl(ResourceMetadata $resourceMetadata, array $ope

return $resourceMetadata->withGraphql($operations);
}

private function needsDefaultGetOperation(ResourceMetadata $resourceMetadata): bool
{
$itemOperations = $resourceMetadata->getItemOperations();

foreach ($itemOperations as $itemOperation) {
if ('GET' === ($itemOperation['method'] ?? false)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not ?? null instead?

@teohhanhui teohhanhui Sep 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, on second thought, this check is unsafe. There might be a custom GET item operation.

The only safe option seems to me to be for the user to declare this get operation themselves:

/**
 * @ApiResource(
 *     itemOperations={
 *         "get"={
 *             "method"="GET",
 *             "controller"=NotFoundAction::class,
 *             "read"=false,
 *             "output"=false,
 *         },
 *     },
 * )
 */

Which to me seems like we should revert and make this a doc entry instead, while providing the NotFoundAction. (I'm of the opinion that we don't need another shortcut to declare this.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could document the custom get operation with one that disables it! I'm fixing what you said!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could document the custom get operation with one that disables it!

What do you mean?

Anyway, I've given it more thought and it's still unsafe anyway. We don't know that there's not already a GET item operation because of route_name. So I really think we must revert this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you said it yourself it's not recommended to use route_name?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, but if there's an operation using route_name which is the GET item operation, we could never tell, and we would be creating this get item operation with NotFoundAction. And there's nothing the user could do to fix that.

return false;
}
}

return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

namespace ApiPlatform\Core\Tests\Bridge\Symfony\Bundle\DependencyInjection;

use ApiPlatform\Core\Action\NotFoundAction;
use ApiPlatform\Core\Api\FilterInterface;
use ApiPlatform\Core\Api\IdentifiersExtractorInterface;
use ApiPlatform\Core\Api\IriConverterInterface;
Expand Down Expand Up @@ -840,6 +841,7 @@ private function getPartialContainerBuilderProphecy()
'api_platform.action.documentation',
'api_platform.action.entrypoint',
'api_platform.action.exception',
'api_platform.action.not_found',
'api_platform.action.placeholder',
'api_platform.cache.identifiers_extractor',
'api_platform.cache.metadata.property',
Expand Down Expand Up @@ -935,23 +937,24 @@ private function getPartialContainerBuilderProphecy()
'api_platform.property_accessor' => 'property_accessor',
'api_platform.property_info' => 'property_info',
'api_platform.serializer' => 'serializer',
Pagination::class => 'api_platform.pagination',
IriConverterInterface::class => 'api_platform.iri_converter',
UrlGeneratorInterface::class => 'api_platform.router',
SerializerContextBuilderInterface::class => 'api_platform.serializer.context_builder',
CollectionDataProviderInterface::class => 'api_platform.collection_data_provider',
ItemDataProviderInterface::class => 'api_platform.item_data_provider',
SubresourceDataProviderInterface::class => 'api_platform.subresource_data_provider',
DataPersisterInterface::class => 'api_platform.data_persister',
ResourceNameCollectionFactoryInterface::class => 'api_platform.metadata.resource.name_collection_factory',
ResourceMetadataFactoryInterface::class => 'api_platform.metadata.resource.metadata_factory',
GroupFilter::class => 'api_platform.serializer.group_filter',
IdentifiersExtractorInterface::class => 'api_platform.identifiers_extractor.cached',
IriConverterInterface::class => 'api_platform.iri_converter',
ItemDataProviderInterface::class => 'api_platform.item_data_provider',
NotFoundAction::class => 'api_platform.action.not_found',
OperationAwareFormatsProviderInterface::class => 'api_platform.formats_provider',
Pagination::class => 'api_platform.pagination',
PropertyFilter::class => 'api_platform.serializer.property_filter',
PropertyNameCollectionFactoryInterface::class => 'api_platform.metadata.property.name_collection_factory',
PropertyMetadataFactoryInterface::class => 'api_platform.metadata.property.metadata_factory',
ResourceClassResolverInterface::class => 'api_platform.resource_class_resolver',
PropertyFilter::class => 'api_platform.serializer.property_filter',
GroupFilter::class => 'api_platform.serializer.group_filter',
OperationAwareFormatsProviderInterface::class => 'api_platform.formats_provider',
IdentifiersExtractorInterface::class => 'api_platform.identifiers_extractor.cached',
ResourceNameCollectionFactoryInterface::class => 'api_platform.metadata.resource.name_collection_factory',
ResourceMetadataFactoryInterface::class => 'api_platform.metadata.resource.metadata_factory',
SerializerContextBuilderInterface::class => 'api_platform.serializer.context_builder',
SubresourceDataProviderInterface::class => 'api_platform.subresource_data_provider',
UrlGeneratorInterface::class => 'api_platform.router',
];

foreach ($aliases as $alias => $service) {
Expand Down
45 changes: 45 additions & 0 deletions tests/Fixtures/TestBundle/Document/DisableItemOperation.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace ApiPlatform\Core\Tests\Fixtures\TestBundle\Document;

use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

/**
* DisableItemOperation.
*
* @author Antoine Bluchet <soyuka@gmail.com>
*
* @ApiResource(itemOperations={}, collectionOperations={"get"})
* @ODM\Document
*/
class DisableItemOperation
{
/**
* @ODM\Id(strategy="INCREMENT", type="integer")
*/
private $id;

/**
* @var string The dummy name
*
* @ODM\Field
*/
public $name;

public function getId()
{
return $this->id;
}
}
49 changes: 49 additions & 0 deletions tests/Fixtures/TestBundle/Entity/DisableItemOperation.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity;

use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ORM\Mapping as ORM;

/**
* DisableItemOperation.
*
* @author Antoine Bluchet <soyuka@gmail.com>
*
* @ApiResource(itemOperations={}, collectionOperations={"get", "post"})
* @ORM\Entity
*/
class DisableItemOperation
{
/**
* @var int The id
*
* @ORM\Column(type="integer", nullable=true)
* @ORM\Id
* @ORM\GeneratedValue(strategy="AUTO")
*/
private $id;

/**
* @var string The dummy name
*
* @ORM\Column
*/
public $name;

public function getId()
{
return $this->id;
}
}
2 changes: 1 addition & 1 deletion tests/JsonSchema/Command/JsonSchemaGenerateCommandTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public function testExecuteWithTooManyOptions()
{
$this->tester->run(['command' => 'api:json-schema:generate', 'resource' => $this->entityClass, '--collectionOperation' => 'get', '--itemOperation' => 'get', '--type' => 'output']);

$this->assertStringStartsWith('[ERROR] You can only use one of "--itemOperation" and "--collectionOperation"', trim(str_replace(["\r", "\n"], '', $this->tester->getDisplay())));
$this->assertStringStartsWith('[ERROR] You can only use one of "--itemOperation" and "--collectionOperation" options at the same time.', trim(preg_replace('/\s+/', ' ', $this->tester->getDisplay())));
}

public function testExecuteWithJsonldFormatOption()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

namespace ApiPlatform\Core\Tests\Metadata\Resource\Factory;

use ApiPlatform\Core\Action\NotFoundAction;
use ApiPlatform\Core\Api\OperationType;
use ApiPlatform\Core\Metadata\Resource\Factory\OperationResourceMetadataFactory;
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
use ApiPlatform\Core\Metadata\Resource\ResourceMetadata;
Expand Down Expand Up @@ -43,29 +45,39 @@ public function getMetadata(): iterable
yield [new ResourceMetadata(null, null, null, null, [], null, [], []), new ResourceMetadata(null, null, null, $this->getOperations(['get', 'put', 'delete']), [], null, [], [])];
yield [new ResourceMetadata(null, null, null, null, [], null, [], []), new ResourceMetadata(null, null, null, $this->getOperations(['get', 'put', 'patch', 'delete']), [], null, [], []), $jsonapi];
yield [new ResourceMetadata(null, null, null, ['get'], [], null, [], []), new ResourceMetadata(null, null, null, $this->getOperations(['get']), [], null, [], [])];
yield [new ResourceMetadata(null, null, null, [], [], null, [], []), new ResourceMetadata(null, null, null, $this->getPlaceholderOperation(), [], null, [], [])];
yield [new ResourceMetadata(null, null, null, ['put'], [], null, [], []), new ResourceMetadata(null, null, null, $this->getOperations(['put']), [], null, [], [])];
yield [new ResourceMetadata(null, null, null, ['delete'], [], null, [], []), new ResourceMetadata(null, null, null, $this->getOperations(['delete']), [], null, [], [])];
yield [new ResourceMetadata(null, null, null, ['patch' => ['method' => 'PATCH', 'route_name' => 'patch']], [], null, [], []), new ResourceMetadata(null, null, null, ['patch' => ['method' => 'PATCH', 'route_name' => 'patch']], [], null, [], [])];
yield [new ResourceMetadata(null, null, null, ['patch' => ['method' => 'PATCH', 'route_name' => 'patch']], [], null, [], []), new ResourceMetadata(null, null, null, ['patch' => ['method' => 'PATCH', 'route_name' => 'patch']], [], null, [], []), $jsonapi];
yield [new ResourceMetadata(null, null, null, ['patch' => ['method' => 'PATCH', 'route_name' => 'patch']], [], null, [], []), new ResourceMetadata(null, null, null, array_merge(['patch' => ['method' => 'PATCH', 'route_name' => 'patch']], $this->getPlaceholderOperation()), [], null, [], [])];
yield [new ResourceMetadata(null, null, null, ['patch' => ['method' => 'PATCH', 'route_name' => 'patch']], [], null, [], []), new ResourceMetadata(null, null, null, array_merge(['patch' => ['method' => 'PATCH', 'route_name' => 'patch']], $this->getPlaceholderOperation()), [], null, [], []), $jsonapi];
yield [new ResourceMetadata(null, null, null, ['untouched' => ['method' => 'GET']], [], null, [], []), new ResourceMetadata(null, null, null, ['untouched' => ['method' => 'GET']], [], null, [], []), $jsonapi];
yield [new ResourceMetadata(null, null, null, ['untouched_custom' => ['route_name' => 'custom_route']], [], null, [], []), new ResourceMetadata(null, null, null, ['untouched_custom' => ['route_name' => 'custom_route']], [], null, [], []), $jsonapi];
yield [new ResourceMetadata(null, null, null, ['untouched_custom' => ['route_name' => 'custom_route']], [], null, [], []), new ResourceMetadata(null, null, null, array_merge(['untouched_custom' => ['route_name' => 'custom_route']], $this->getPlaceholderOperation()), [], null, [], []), $jsonapi];

// Collection operations
yield [new ResourceMetadata(null, null, null, [], null, null, [], []), new ResourceMetadata(null, null, null, [], $this->getOperations(['get', 'post']), null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['get'], null, [], []), new ResourceMetadata(null, null, null, [], $this->getOperations(['get']), null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['post'], null, [], []), new ResourceMetadata(null, null, null, [], $this->getOperations(['post']), null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['options' => ['method' => 'OPTIONS', 'route_name' => 'options']], null, [], []), new ResourceMetadata(null, null, null, [], ['options' => ['route_name' => 'options', 'method' => 'OPTIONS']], null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['untouched' => ['method' => 'GET']], null, [], []), new ResourceMetadata(null, null, null, [], ['untouched' => ['method' => 'GET']], null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['untouched_custom' => ['route_name' => 'custom_route']], null, [], []), new ResourceMetadata(null, null, null, [], ['untouched_custom' => ['route_name' => 'custom_route']], null, [], [])];
yield [new ResourceMetadata(null, null, null, [], null, null, [], []), new ResourceMetadata(null, null, null, $this->getPlaceholderOperation(), $this->getOperations(['get', 'post'], OperationType::COLLECTION), null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['get'], null, [], []), new ResourceMetadata(null, null, null, $this->getPlaceholderOperation(), $this->getOperations(['get'], OperationType::COLLECTION), null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['post'], null, [], []), new ResourceMetadata(null, null, null, $this->getPlaceholderOperation(), $this->getOperations(['post'], OperationType::COLLECTION), null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['options' => ['method' => 'OPTIONS', 'route_name' => 'options']], null, [], []), new ResourceMetadata(null, null, null, $this->getPlaceholderOperation(), ['options' => ['route_name' => 'options', 'method' => 'OPTIONS']], null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['untouched' => ['method' => 'GET']], null, [], []), new ResourceMetadata(null, null, null, $this->getPlaceholderOperation(), ['untouched' => ['method' => 'GET']], null, [], [])];
yield [new ResourceMetadata(null, null, null, [], ['untouched_custom' => ['route_name' => 'custom_route']], null, [], []), new ResourceMetadata(null, null, null, $this->getPlaceholderOperation(), ['untouched_custom' => ['route_name' => 'custom_route']], null, [], [])];
}

private function getOperations(array $names): array
private function getOperations(array $names, $operationType = OperationType::ITEM): array
{
$operations = [];
foreach ($names as $name) {
$operations[$name] = ['method' => strtoupper($name)];
}

if (OperationType::ITEM === $operationType && !isset($operations['get'])) {
return array_merge($operations, $this->getPlaceholderOperation());
}

return $operations;
}

private function getPlaceholderOperation(): array
{
return ['get' => ['method' => 'GET', 'read' => false, 'output' => ['class' => false], 'controller' => NotFoundAction::class]];
}
}