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

Related collections: Performance vs. supporting non-HAL formats #2829

Open
carlobeltrame opened this issue Jun 13, 2022 · 3 comments
Open

Related collections: Performance vs. supporting non-HAL formats #2829

carlobeltrame opened this issue Jun 13, 2022 · 3 comments
Assignees

Comments

@carlobeltrame
Copy link
Member

carlobeltrame commented Jun 13, 2022

Some related collections are expensive to compute, in the sense that they require additional database queries. Examples include Period#getContentNodes() and Day#getScheduleEntries().

With our current implementation, we try to support not only the HAL JSON format, but also JSON+LD and GraphQL. For HAL, we sometimes add an annotation #[RelatedCollectionLink], and the API response therefore only contains a filtered link like /content_nodes?period=/periods/1a2b3c4d, so all the contentNode data which is fetched from the database is completely ignored afterwards. So if we were only supporting HAL JSON, there would be no need to perform these additional database queries. But omitting the queries would break the JSON+LD format and possibly also GraphQL (not sure about that one).
This affects all API calls where a period or day entity is normalized (e.g. when loading or patching a single period, when loading all periods, when loading a camp with embedded periods, etc.)

This issue was originally raised by @usu in #2700 (comment) and the decision might influence how to proceed in a past discussion in #2683 (comment).

We should decide how we want to proceed with such getters on our entities. Probably we should assess the performance impact of these first.

@carlobeltrame carlobeltrame added the Meeting Discuss Am nächsten Core-Meeting besprechen label Jun 13, 2022
@manuelmeister
Copy link
Member

Core Meeting Decision

We will drop JSON+LD "support" when we have performance problems
Currently we have no tests that cover the JSON+LD support anyway.
We see how good the support for GraphQL is in version 2.7

@usu makes a performance analysis of the issue mentioned above

@manuelmeister manuelmeister removed the Meeting Discuss Am nächsten Core-Meeting besprechen label Aug 25, 2022
@usu usu self-assigned this Aug 25, 2022
BacLuc added a commit to BacLuc/ecamp3 that referenced this issue Jun 24, 2023
Else the Get endpoint of camp executes a lot of additonal
queries to fetch the ContentNodes.
This breaks the json-ld support for this endpoint. (See ecamp#2829)

Issue: ecamp#3539
@BacLuc
Copy link
Contributor

BacLuc commented Jun 24, 2023

Now we have performance problems with the additional queries.

Core Meeting Decision

  • we drop jsonld support.

That this is transparent, it would be good to also remove json-ld and hydra from the error formats and the input formats.
That we can remove it from the error formats, we need to add the header Content-Type: application/json in the Create* api tests.
They don't specify the Content-Type currently, and then api-platform falls back to json-ld, which is not uspported.
Plan: Add the headers in the tests step by step.
If this is finished, remove json-ld from config/packages/api_platform.yml and remove it from the Error Serializer

@usu
Copy link
Member

usu commented Sep 12, 2023

Related to this: https://github.com/api-platform/core/pull/5675/files# was merged and should be available in api-platform 3.2

This could be a alternative solution to avoid overloading of data

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

No branches or pull requests

4 participants