feat(state): add headers to comply with LDP specification #6917

LaurentHuzard · 2025-01-14T14:50:53Z

Q	A
Branch?	main
Tickets	-
License	MIT
Doc PR	api-platform/docs#

To ensure compliance with the LDP specification (https://www.w3.org/TR/ldp/):

Added the "Accept-Post" header with the value "text/turtle, application/ld+json".
Added the "Allow" header with values based on the allowed operations on the queried resources.

To ensure compliance with the LDP specification (https://www.w3.org/TR/ldp/): - Added the "Accept-Post" header with the value "text/turtle, application/ld+json". - Added the "Allow" header with values based on the allowed operations on the queried resources.

dunglas · 2025-01-14T18:24:20Z

features/main/ldp_resources.feature

+    When I add "Content-Type" header equal to "application/ld+json"
+    And I send a "GET" request to "/dummy_get_post_delete_operations"
+    Then the header "Accept-Post" should be equal to "text/turtle, application/ld+json"
+    And the header "Allow" should be equal to "OPTIONS, HEAD, GET, POST, DELETE"


Shouldn't the value be different for item and collection URLs?

Also, Allow-Post should only be defined for URLs where POST is enabled (usually collections, but not always).

You’re right, I overlooked that. I’ll make the changes and push them soon.

dunglas · 2025-01-14T18:28:04Z

src/State/Processor/RespondProcessor.php

@@ -88,6 +93,9 @@ public function process(mixed $data, Operation $operation, array $uriVariables =
            $headers['Accept-Patch'] = $acceptPatch;
        }

+        $headers['Accept-Post'] = 'text/turtle, application/ld+json';


This value cannot be hardcoded. For instance Turtle is not supported natively by API Platform and JSON-LD may or may not depending on the config.

You should inject the enabled formats here. You can retrieve this from metadata. Listing all allowed media types (not only Turtle and JSON-LD) is probably OK (and a best practice).

It sounds much better to list all the allowed media types. I'll take care of it.

dunglas · 2025-01-14T18:29:51Z

src/State/Processor/RespondProcessor.php

+
+    private function getAllowedMethods(?string $resourceClass): string
+    {
+        $allowedMethods = self::DEFAULT_ALLOWED_METHOD;


You should filter here the operations with a different URI Template than the one of the current operation to fix the issue described in my 1st comment.

Ah, good point—I’ll sort this out.

… "allow" has now the right method.

… to avoid sending incorrect values.

LaurentHuzard · 2025-01-15T18:02:02Z

src/State/Processor/RespondProcessor.php

@@ -88,6 +93,16 @@ public function process(mixed $data, Operation $operation, array $uriVariables =
            $headers['Accept-Patch'] = $acceptPatch;
        }

+        if (!$exception) {


To avoid returning incorrect values—such as in the case of a 401 error when requesting a resource—I added this condition, although I’m not entirely sure it’s the best approach. The issue arises because, in the case of a 401 error, the allowed methods and the currentUriTemplate correspond to the error itself rather than the requested resource.

The approach looks good to me.

dunglas · 2025-01-15T21:11:46Z

src/State/Processor/RespondProcessor.php

+            }
+        }
+
+        return array_unique($allowedMethods);


I don't think it's necessary because it's not possible to register two different routes for the same URL and the same method.

It would remove this as we are on the hot path.

To be honest, I would inline this code and remove this method. This will also allow you to set a variable (instead of running an O(n) operation) to check if POST is defined or not for Accept-Post, and this would remove a function call.

(Yes, these are mostly a micro optimization, but this also simplify the code IMHO).

dunglas · 2025-01-15T21:15:18Z

src/State/Processor/RespondProcessor.php

+    private function getAllowedMethods(?string $resourceClass, ?string $currentUriTemplate): array
+    {
+        $allowedMethods = self::DEFAULT_ALLOWED_METHOD;
+        if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver->isResourceClass($resourceClass)) {


Suggested change

if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver->isResourceClass($resourceClass)) {

if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver?->isResourceClass($resourceClass)) {

dunglas · 2025-01-15T21:16:39Z

src/State/Processor/RespondProcessor.php

+    {
+        $allowedMethods = self::DEFAULT_ALLOWED_METHOD;
+        if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver->isResourceClass($resourceClass)) {
+            $resourceMetadataCollection = $this->resourceCollectionMetadataFactory ? $this->resourceCollectionMetadataFactory->create($resourceClass) : new ResourceMetadataCollection($resourceClass);


Suggested change

$resourceMetadataCollection = $this->resourceCollectionMetadataFactory ? $this->resourceCollectionMetadataFactory->create($resourceClass) : new ResourceMetadataCollection($resourceClass);

$resourceMetadataCollection = $this->resourceCollectionMetadataFactory ? $this->resourceCollectionMetadataFactory->create($resourceClass) : new ResourceMetadataCollection($resourceClass);

Better not set the extra headers at all if the factory is missing.

It's an optional feature after all.

As it's an optional feature, should I add a configuration variable to enable it?

dunglas · 2025-01-16T08:03:26Z

src/State/Processor/RespondProcessor.php

+            }
+            $headers['Allow'] = implode(', ', $allowedMethods);
+
+            if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) {


Suggested change

if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) {

if ($isPostAllowed && \is_array($outputFormats = $operation->getOutputFormats()) && [] !== $outputFormats) {

soyuka · 2025-01-16T08:20:51Z

src/State/Processor/RespondProcessor.php

    public function __construct(
        private ?IriConverterInterface $iriConverter = null,
        private readonly ?ResourceClassResolverInterface $resourceClassResolver = null,
        private readonly ?OperationMetadataFactoryInterface $operationMetadataFactory = null,
+        private readonly ?ResourceMetadataCollectionFactoryInterface $resourceCollectionMetadataFactory = null,


We could probably create an LinkedDataPlatformProcessor?

Thanks for the suggestion! A LinkedDataPlatformProcessor sounds like a good idea, I'll consider implementing it.

soyuka · 2025-01-16T08:21:57Z

src/State/Processor/RespondProcessor.php

@@ -88,6 +92,27 @@ public function process(mixed $data, Operation $operation, array $uriVariables =
            $headers['Accept-Patch'] = $acceptPatch;
        }

+        if (!$exception) {


if ($operation instanceof ApiPlatform\Metadata\Error)

soyuka · 2025-01-16T08:24:14Z

src/State/Processor/RespondProcessor.php

+                    foreach ($resource->getOperations() as $resourceOperation) {
+                        if ($resourceOperation->getUriTemplate() === $currentUriTemplate) {
+                            $allowedMethods[] = $operationMethod = $resourceOperation->getMethod();
+                            $isPostAllowed = $isPostAllowed || ('POST' === $operationMethod);


you could collect outputFormats here directly? The operation below may not be the one where the formats are defined.

Good catch. Thanks.

soyuka · 2025-01-16T08:24:38Z

src/State/Processor/RespondProcessor.php

+            }
+            $headers['Allow'] = implode(', ', $allowedMethods);
+
+            if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) {


Suggested change

if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) {

if ($isPostAllowed && ($outputFormats = $operation->getOutputFormats())) {

soyuka · 2025-01-16T08:26:25Z

src/State/Tests/Processor/RespondProcessorTest.php

+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+
+class RespondProcessorTest extends TestCase


I think a RespondProcessorTest already exists it must be in tests forgot to move it

OK, i will merge them.

The existing RespondProcessorTest use the old way of doing test with prophecy instead of PHPunit mock system, should i refactor it ?

soyuka · 2025-01-16T08:26:40Z

src/Symfony/Bundle/Resources/config/state/processor.xml

@@ -21,6 +21,7 @@
            <argument type="service" id="api_platform.iri_converter" />
            <argument type="service" id="api_platform.resource_class_resolver" />
            <argument type="service" id="api_platform.metadata.operation.metadata_factory" />
+            <argument type="service" id="api_platform.metadata.resource.metadata_collection_factory" />


on-invalid=ignore

soyuka · 2025-01-16T08:28:40Z

tests/Fixtures/TestBundle/Document/DummyGetPostDeleteOperation.php

+use ApiPlatform\Metadata\Post;
+use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;
+
+#[ApiResource(operations: [new Get(), new GetCollection(), new Post(), new Delete()])]


do you really need an entity/document for this? I think only the DummyGetPostDeleteOperation is enough, if you need a dummy provider you can create a static one.

OK, i probably did that when i was struggling with behat or trying to fix some MongoDB test on CI.

yes with Behat by default they both run, as this feature doesn't impact doctrine we don't need to setup any entities.

LaurentHuzard added 2 commits January 14, 2025 15:49

fix: handle case injected services are null

50098a2

LaurentHuzard marked this pull request as draft January 14, 2025 15:41

fix: handle case injected services are null

b061e53

dunglas requested changes Jan 14, 2025

View reviewed changes

LaurentHuzard added 3 commits January 15, 2025 18:31

fix: added header "allow-post" only on resources with POST and header…

09fcf78

… "allow" has now the right method.

style: apply php-cs-fixer

2f5e432

Headers "Allow" and "Accept-Post" are not added if we return an error…

49ea482

… to avoid sending incorrect values.

LaurentHuzard commented Jan 15, 2025

View reviewed changes

dunglas requested changes Jan 15, 2025

View reviewed changes

misc fixes and optimization

42d61d3

dunglas reviewed Jan 16, 2025

View reviewed changes

soyuka requested changes Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(state): add headers to comply with LDP specification #6917

feat(state): add headers to comply with LDP specification #6917

LaurentHuzard commented Jan 14, 2025

dunglas Jan 14, 2025 •

edited

Loading

LaurentHuzard Jan 15, 2025

dunglas Jan 14, 2025

LaurentHuzard Jan 15, 2025

dunglas Jan 14, 2025

LaurentHuzard Jan 15, 2025

LaurentHuzard Jan 15, 2025

dunglas Jan 15, 2025

dunglas Jan 15, 2025

dunglas Jan 15, 2025

dunglas Jan 15, 2025

LaurentHuzard Jan 20, 2025

dunglas Jan 16, 2025

soyuka Jan 16, 2025

LaurentHuzard Jan 16, 2025

soyuka Jan 16, 2025

soyuka Jan 16, 2025

LaurentHuzard Jan 16, 2025

soyuka Jan 16, 2025

soyuka Jan 16, 2025

LaurentHuzard Jan 16, 2025

LaurentHuzard Jan 20, 2025

soyuka Jan 16, 2025

soyuka Jan 16, 2025

LaurentHuzard Jan 16, 2025

soyuka Jan 17, 2025

	if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver->isResourceClass($resourceClass)) {
	if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver?->isResourceClass($resourceClass)) {

	$resourceMetadataCollection = $this->resourceCollectionMetadataFactory ? $this->resourceCollectionMetadataFactory->create($resourceClass) : new ResourceMetadataCollection($resourceClass);
	$resourceMetadataCollection = $this->resourceCollectionMetadataFactory ? $this->resourceCollectionMetadataFactory->create($resourceClass) : new ResourceMetadataCollection($resourceClass);

	if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) {
	if ($isPostAllowed && \is_array($outputFormats = $operation->getOutputFormats()) && [] !== $outputFormats) {

	if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) {
	if ($isPostAllowed && ($outputFormats = $operation->getOutputFormats())) {

feat(state): add headers to comply with LDP specification #6917

Are you sure you want to change the base?

feat(state): add headers to comply with LDP specification #6917

Conversation

LaurentHuzard commented Jan 14, 2025

dunglas Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Jan 14, 2025 •

edited

Loading