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.

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

feat(serializer): add ApiProperty::uriTemplate option #5675

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

GregoireHebert
Copy link
Contributor

@GregoireHebert GregoireHebert commented Jul 17, 2023

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

This feature gives control over the operation used for *toOne and
*toMany relations IRI generation.

When defined, API Platform will use the operation declared on the related
resource that matches the uriTemplate string.

In addition, this will override the value returned to be the IRI string
only, not an object in JSONLD formats. For HAL and JSON:API format, the
IRI will be used in links properties, and in the objects embedded or in
relationship properties.

learn more with this guide : https://github.com/GregoireHebert/core/blob/iri-only-collection-property/docs/guides/return-the-iri-of-your-resources-relations.php

  • Need more tests

@usu
Copy link
Contributor

usu commented Jul 18, 2023

Hi @GregoireHebert. This is an interesting PR. We've developed a similar feature for our application & as a matter of fact, we were also thinking about proposing a PR to api-platform on this. Trying to chime in on this PR & dissect where our solutions differs from yours. Maybe we can build the best out of both solutions 😃

Our current solution is built with a custom Normalizer which decorates the built-in Hal\ItemNormalizer (we're currently only using HAL, but the concept is most probably also usable for other formats). In this Normalizer we automatically replace all array of links with a single IRI (similar as your proposal).

Functionally, this method works great for us. However, we ran into an issue with sub-optimal performance: Due to the fact, that our Normalizer decorates the built-in one, too many entities are loaded from DB and normalized (basically a waste of resources, because all these data is loaded & normalized, event if they are only discarded later-on by our custom Normalizer). This is the reason why we were interested into building this into api platform natively.

Some questions & points where our solutions differ:

  • Our solution replaces all IRIs application-wide without the need of specifying iriOnly for each ApiProperty. Did you think about using a global flag, which then could be overriden on property level?

  • Our solution checks for a Search Filter on the target resource and only replaces the Array of IRIs with a single IRI, if such a search filter exists. To utilise the same example you used in your doc PR, this would result in the following response (in case Car resource implements a brands search filter):

    {
      "@context": "/contexts/Brand",
      "@id": "/brands/1",
      "@type": "Brand",
      "cars": "/cars?brand=/brands/1"
    }
  • The property name iriOnly could be misleading in my opinion. Especially in HAL format, the current/original response is also IRI's only (array of IRIs), if the data is not embedded. So the property name could mislead that it is about embedding data (=today's property readableLink). What about collectionLink as a proposal? Basically, we replace a collection of entities with a single collection link, hence the name.

  • As you specified, a current limitation is, that multiple GetCollection operations are not supported (same is true for our current implementation). Having iriOnly as a boolean property makes it difficult to later expand it further to support multiple operations. What about using a separate attribute, which could later be expanded, e.g.:

    #[ApiProperty(link: new CollectionLink())] // does the same as iriOnly: true

    This could later be extended without BC to allow for specific operations or also for route parameters:

    #[ApiProperty(link: new CollectionLink(
        relatedEntity: Cars::class,
        parameters: [
            'brand' => 'getBrand',
            'yearOfConstruction' => 'getYear',
        ],
        operationName: 'nested_subresource_get_collection'
    ))]

    Latter example could then generate the following response:

    {
      "@context": "/contexts/Brand",
      "@id": "/brands/1",
      "@type": "Brand",
      "cars": "/brands/1/cars?year=2021"
    }

    In our current solution, we use a similar attribute to generate manual links, in cases where we cannot derive target entity and search filter from the context automatically.

  • For performance reasons, it would be beneficial, if generation of the collection IRI happens before loading the collection data from DB (basically before [propertyAccessor->getValue()](https://github.com/api-platform/core/pull/5675/files#diff-d87efa25fbf52904daa06047002652f08645e3ad4a7f4985ffeec5a7c8e39c4fR604) is called). This should be possible, because $attributeValue is not needed for IRI generation.

Apologies for throwing all these points into your PR. Kind of want to avoid, we come up with a conflicting/non-compatible PR from our side. Happy to support on implementation of the points above, if they seem sensible to you.

Also looping in @carlobeltrame, who was the initial developer of our applications' solution.

@mrossard
Copy link
Contributor

May I be silly for a minute?

To me this looks a lot like you sometimes have very big collections and want to lighten the output...which actually just seems like a custom ApiResource to me. Building IRIs for your collections in the provider and exposing them with the field name used in the entity shouldn't be a complicated task.

I feel like I'm missing something or severely underestimating the amount of work required in your project(s) to do it "manually" that way...is that it?

@usu
Copy link
Contributor

usu commented Jul 19, 2023

To me this looks a lot like you sometimes have very big collections and want to lighten the output...which actually just seems like a custom ApiResource to me.

Speaking for our use-case, it's mostly about reducing the number of network requests necessary to load a tree of related data.

The following response needs 3+1 network requests to load the car and all its repairs. It's an N+1 problem, so the number of requests scale linearly with the number of repairs (very bad for performance and for frontend UX).
(Wrote the example in HAL format, as I'm more familiar with it)

{
    "_links": {
        "self": {
            "href": "/cars/1"
        },
        "repairs": [
            {
                "href": "/repairs/1"
            },
            {
                "href": "/repairs/2"
            },
            {
                "href": "/repairs/3"
            }
        ]
    },
    "id": "1"
}

The following response always needs exactly 2 network requests, independent of the number of repairs:

{
    "_links": {
        "self": {
            "href": "/cars/1"
        },
        "repairs": {
            "href": "/repairs?car=/cars/1"
        }
    },
    "id": "1"
}

Building IRIs for your collections in the provider and exposing them with the field name used in the entity shouldn't be a complicated task.

I guess the overriding the fields in the provider would work for the top-level item. However, this could get complicated very fast for embedded resources. For example when the brand resource embeds its cars:

{
    "_links": {
        "self": {
            "href": "/brands/1"
        },
        "cars": {
            "href": "/cars?brand=/brands/1"
        }
    },
    "_embedded": {
        "cars": [
            {
                "_links": {
                    "self": {
                        "href": "/cars/1"
                    },
                    "repairs": {
                        "href": "/repairs?car=/cars/1"
                    }
                },
                "id": "1"
            }
        ]
    },
    "id": "1"
}

The beauty of hooking into the Normalizer is that it generates consistent results, independent of whether a resource is top-level resource or a deeply nested embedded resource.

@mrossard
Copy link
Contributor

Ok, so with embedded resources you'd have to have everything as separate ApiResources different from your entities.

That's actually the way i work with APIP right now, but i understand how that could become a problem / a huge amount of work if it's not the case for you!

@GregoireHebert
Copy link
Contributor Author

@usu

Did you think about using a global flag, which then could be overriden on property level?

I did, but for this PR, I introduced it in the ApiProperty level only.
We can add the global option later on :)

Filtering the collection on the current resource
It is definitly something I want to add with the two possible solutions:
"cars": "/cars?brand={BRAND-IRI}" and "cars": "/brands/{brandId}/cars"

collectionLink proposal
Why not.

link: new CollectionLink()
We talked about something similar with @vincentchalamon, but the exact structure isn't clear in my head yet.

Definitely some things to consider.
Thanks a lot for your feedback !

Right off the bat, I am tempted to consider the CollectionLink. I'll give it a thought :)

@dunglas
Copy link
Member

dunglas commented Jul 20, 2023

I love the idea. This approach has many benefits:

  1. Performance/ecodesign: this prevents generating and serializing huge, non-paginated collections
  2. Architecture: this is an incentive for creating an atomic, ESI-like API
  3. Security: this mitigates some DoS attacks

In my opinion, we should allow enabling this option globally, and turn this feature on by default in API Platform 4 (to do so without introducing a breaking change, we can deprecate not setting explicitly the config flag in v3, and set its default value to true in v4).

Instead of relying on the search filter, this feature should be tightly coupled to "subresources".
It should be used only if a corresponding subresource exists, or, better, create automatically the corresponding subresource:

Before:

{
    "@id": "/brands/1",
    "cars": [
        {"@id": "/cars/a", ...},
        {"@id": "/cars/b", ...}
     ]
}

After:

{
    "@id": "/brands/1",
    "cars": "/brands/1/cars"
}

The property name iriOnly could be misleading in my opinion.

I don't like it either, but we already have a similar option named like that. What about uriTemplate: bool|uri-template-id? This is consistent with the name prop of #[ApiResource], as a good DX, and allows selecting a specific route if several are defined for the same resource. We should maybe also allow mapping the current prop with the corresponding URI variable on the other side.

Regarding possible filters, documenting them is already supported by Hydra, so I don't think we have more to do, at least for JSON-LD. Unlike Hydra, I don't think that HAL has support for URI templates.

@usu
Copy link
Contributor

usu commented Jul 22, 2023

Thanks @dunglas for your perspective on this and for the general support of the concept.

I don't like it either, but we already have a similar option named like that. What about uriTemplate: bool|uri-template-id? This is consistent with the name prop of #[ApiResource], as a good DX, and allows selecting a specific route if several are defined for the same resource. We should maybe also allow mapping the current prop with the corresponding URI variable on the other side.

For simple use cases, a boolean flag or a string identifying the route might be sufficient (e.g. simple properties linking to other resources).

For more complex use cases (e.g. manual getters) this is not sufficient, and more information is needed (target resource + route + URI/query parameters). Latter could also be a separate feature, which basically allows overriding links with an arbitrary route. I just though there's a case to combine the 2 use-cases, as they share some similarities.

Providing an example:

#[ApiResource]
class Brand {
    #[ApiProperty(link: new CollectionLink(
        relatedEntity: Repair::class,
        parameters: [
            'finished' => false,
        ],
        callbackParameters: [
            'brand' => 'getBrand',
        ],
    ))]
    public function getUnfinishedRepairs(): array {
        $repairs = array_map(function (Car $car) {
            return array_filter(
                $car->repairs->getValues(),
                function (Repair $repair) { return !$repair->finished; }
            );
        }, $this->cars->getValues());

        return array_merge(...$repairs);
    }

    public function getBrand() {
        return $this;
    }
}

Which then would result in the following response:

{
    "@id": "/brands/1",
    "cars": "/brands/1/cars",
    "unfinishedRepairs": "/repairs?brand=/brands/1&finished=false",
}

Regarding possible filters, documenting them is already supported by Hydra, so I don't think we have more to do, at least for JSON-LD. Unlike Hydra, I don't think that HAL has support for URI templates.

HAL actually has support for URI templates (following RFC6570).
https://datatracker.ietf.org/doc/html/draft-kelly-json-hal#section-5.1:

Its value is either a URI [RFC3986] or a URI Template [RFC6570].

We have some code on our side to create RFC6570 templates for an ApiResource.

@GregoireHebert
Copy link
Contributor Author

I did a few changes from my initial proposal.

using a string to define an uriTemplate in the same fashion as the itemUriTemplate of an operation.
I need to add a test to check a behaviour fixed by @vincentchalamon in the following PR: #5663

It will use a GetCollection Operation IRI matching the template, but this Operation has to be defined through the related Resource class.
I am not in favour of defining this much data from the main resource. The property is not a resource nor a resource representation and should not be the source of truth regarding the Uri generation. In a DX sense, that one one the reasons the SubResource annotation was such a mayhem in 2.*.

Then for the global/true/false value possibility, I think it's a bad idea in the same way as the previous comment, the current ressource should not be the source of truth for the related resource class. It guesses too much, and can allow to select matching operation that are not the desired ones. Plus it will be easier to maintain since it uses the same mechanism than the operations, will use correctly the uriVariable already declared.

Now my current proposal doesn't cover one of your need that is passing default filters values during the IRI generation.
This is not possible at all at the moment. I understand your use case, but It deserves a separate PR/Proposal.

@GregoireHebert
Copy link
Contributor Author

note: last failing tests should be green when 3.1 is merged into main.

@@ -155,8 +155,9 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt
}

$fetchEager = $propertyMetadata->getFetchEager();
$uriTemplate = $propertyMetadata->geturiTemplate();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$uriTemplate = $propertyMetadata->geturiTemplate();
$uriTemplate = $propertyMetadata->getUriTemplate();

composer.json Outdated
@@ -75,6 +75,7 @@
"symfony/maker-bundle": "^1.24",
"symfony/mercure-bundle": "*",
"symfony/messenger": "^6.1",
"symfony/monolog-bundle": "^3.8",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"symfony/monolog-bundle": "^3.8",

Use LoggerInterface from PSR

src/Metadata/ApiProperty.php Show resolved Hide resolved
src/Metadata/ApiProperty.php Show resolved Hide resolved
@@ -638,6 +639,13 @@ protected function getAttributeValue(object $object, string $attribute, string $
$childContext = $this->createChildContext($context, $attribute, $format);
unset($childContext['iri'], $childContext['uri_variables'], $childContext['resource_class'], $childContext['operation']);

if (null !== $itemUriTemplate = $propertyMetadata->getUriTemplate()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (null !== $itemUriTemplate = $propertyMetadata->getUriTemplate()) {
if ($itemUriTemplate = $propertyMetadata->getUriTemplate()) {

@@ -638,6 +639,13 @@ protected function getAttributeValue(object $object, string $attribute, string $
$childContext = $this->createChildContext($context, $attribute, $format);
unset($childContext['iri'], $childContext['uri_variables'], $childContext['resource_class'], $childContext['operation']);

if (null !== $itemUriTemplate = $propertyMetadata->getUriTemplate()) {
$operation = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation($itemUriTemplate, true, true);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$operation = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation($itemUriTemplate, true, true);
$operation = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation($itemUriTemplate, true, true);

you can use named arguments for clarity on these booleans?

@usu
Copy link
Contributor

usu commented Aug 14, 2023

Thanks for progressing on this. I tried to test with our application, but noticed this doesn't work with HAL format.

The code fails on Hal/Serializer/ItemNormalizer.php#L230 with a "Warning: foreach() argument must be of type array|object, string given". Basically this code expects an array (because $relation['cardinality'] is not 'one') but instead gets the URI as a string value. This could be catched earlier in the same function, however, the issue is that I might want to embed the relation as well. So getAttributeValue would need to return the URI when $type==="link", but return the actual array of objects for $type==="embedded".

Might be an issue for other formats as well. I don't know JsonAPI enough, but the code in JsonApi/Serializer/ItemNormalizer looks very similar.

@@ -638,6 +639,17 @@ protected function getAttributeValue(object $object, string $attribute, string $
$childContext = $this->createChildContext($context, $attribute, $format);
unset($childContext['iri'], $childContext['uri_variables'], $childContext['resource_class'], $childContext['operation']);

if ($itemUriTemplate = $propertyMetadata->getUriTemplate()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would actually be cool if we can avoid populating $attributeValue before checking for uriTemplate. This can save quite some DB requests, as the actual data is not needed to generate the uri.

I made a proposal here: GregoireHebert#1

This also includes enabling uriTemplate for non-collections. While we both have mainly collections in mind as the main purpose of this feature, I think there's no harm to also allow overriding the uriTemplate for *:1 relations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am trying to take this into account, and it gives me insomnia and headaches...
I will eventually get there, please bear with me :D

Copy link
Contributor

Choose a reason for hiding this comment

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

🤣 got it. Let me know if I can be of any help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am getting somewhere... I think.
If you could check on your project that I dont break anything 😬
I had to change the HAL json schema that didn't allowed having object embedded (only array of objects)
But I wasn't 100% sure it was alright according to the specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is ok, I'll write/update the documentation guide

@GregoireHebert
Copy link
Contributor Author

@usu thanks for the feedbacks, I'll try to have a look and fix the other formats during this week or the next.

Copy link
Contributor

@usu usu left a comment

Choose a reason for hiding this comment

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

Checked on my project in HAL format and everything works as intended 🚀

Thanks a lot!


if (false === $fetchEager) {
if (false === $fetchEager || null !== $uriTemplate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When items are embedded, they should still be fetched.

Should we change as following?

if (false === $fetchEager || (null !== $uriTemplate && !$propertyMetadata->isReadableLink() )) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try this

continue;
}

$childContext = $this->createChildContext($context, $relationName, $format);
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly is the purpose of this and the next few lines? $relation['operation'] is only populated when $relation['iri'] is populated as well, in which case the for-loop has already continued/returned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll double check that, I may have been carried away x)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for HAL and JSON:API formats, the behaviour is fine.

But for the JSONLD format, I feel that we should not use the uriTemplate acting as an IriOnly option, when there is one that exist for NormalizationContext. Would it make more sense that, unless we use a second ApiProperty::IriOnly option, it only resolves the IRI with uriTemplate within the object(s) themselves ? And if the ApiProperty::IriOnly option is true, then return the IRI string instead of an object? But then what about the IriOnly option on Normalization Context?

ping @vincentchalamon @dunglas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on video call with Vincent and Soyuka, the behaviour is different, but doesn't interfere that much. It is acceptable.

@GregoireHebert GregoireHebert marked this pull request as draft August 28, 2023 07:32
@GregoireHebert GregoireHebert changed the title feat: Add IriOnly Option on Collection properties feat(serializer): add ApiProperty::uriTemplate option Aug 28, 2023
@vincentchalamon vincentchalamon mentioned this pull request Sep 11, 2023
18 tasks
This feature gives control over the operation used for *toOne and
*toMany relations IRI generation.

When defined, API Platform will use the operation declared on the related
resource that matches the uriTemplate string.

In addition, this will override the value returned to be the IRI string
only, not an object in JSONLD formats. For HAL and JSON:API format, the
IRI will be used in links properties, and in the objects embedded or in
relationship properties.
@soyuka soyuka force-pushed the iri-only-collection-property branch 2 times, most recently from 76c6942 to 8550276 Compare September 11, 2023 12:28
@soyuka soyuka marked this pull request as ready for review September 11, 2023 12:31
@soyuka soyuka merged commit e65d2c3 into api-platform:main Sep 11, 2023
41 of 43 checks passed
@soyuka
Copy link
Member

soyuka commented Sep 11, 2023

Merging this as experimental, future updates can be provided as bug fix on 3.2 (currently main). Thanks @usu @GregoireHebert !

@BacLuc
Copy link

BacLuc commented Oct 10, 2023

Thank you very much @GregoireHebert and @usu.
I tested the feature and i could reduce unnecessary db queries by quite a bit:
BacLuc/ecamp3@fe00aa1#diff-e8620ddac3c501cd95265fa71310036a686d6a1376608ed804d001937c88149c
If i expand this to all problematic relations this will have a big impact.

It even documents the property as an iri-reference string instead of an array of iri-reference strings, very cool.
BacLuc/ecamp3@fe00aa1#diff-ad6583babfa03aae1d6fc116bafe6e03eacd4fa1642f875be723a11e58bae455

BacLuc/ecamp3#208

@@ -155,8 +155,9 @@ private function joinRelations(QueryBuilder $queryBuilder, QueryNameGeneratorInt
}

$fetchEager = $propertyMetadata->getFetchEager();
$uriTemplate = $propertyMetadata->getUriTemplate();
Copy link
Contributor

Choose a reason for hiding this comment

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

@GregoireHebert This causes an issue here.

Error: Typed property ApiPlatform\Metadata\ApiProperty::$uriTemplate must not be accessed before initialization
#32 /vendor/api-platform/core/src/Metadata/ApiProperty.php(477): ApiPlatform\Metadata\ApiProperty::getUriTemplate
#31 /vendor/api-platform/core/src/Doctrine/Orm/Extension/EagerLoadingExtension.php(158): ApiPlatform\Doctrine\Orm\Extension\EagerLoadingExtension::joinRelations
#30 /vendor/api-platform/core/src/Doctrine/Orm/Extension/EagerLoadingExtension.php(98): ApiPlatform\Doctrine\Orm\Extension\EagerLoadingExtension::apply
#29 /vendor/api-platform/core/src/Doctrine/Orm/Extension/EagerLoadingExtension.php(61): ApiPlatform\Doctrine\Orm\Extension\EagerLoadingExtension::applyToItem

Copy link
Member

Choose a reason for hiding this comment

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

weird, it's initialized to null inside the constructor. Can you provide a reproducer an open a new issue please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants