chore(performance): use subresources & uriTemplate property #4968
Conversation
…large unnecessary join tables
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
|
@BacLuc: Now that we merged api-platform 3.3, I also enabled link security. Users without access to the parent object now receive a 404 instead of an empty list (similar as already implemented in #3610). Just in case, you want to have another look at it. This means, we now don't rely on |
| $this->assertEquals("/periods/{$period2->getId()}/days", $responseArray['_embedded']['periods'][0]['_links']['days']['href']); | ||
| $this->assertEquals("/periods/{$period1->getId()}/days", $responseArray['_embedded']['periods'][1]['_links']['days']['href']); |
There was a problem hiding this comment.
Is the order of these two periods in the response guaranteed? Or should you use $this->assertEqualsCanonicalizing here?
There was a problem hiding this comment.
Yeah, should always be sorted by period start date:
https://github.com/ecamp/ecamp3/blob/devel/api/src/Entity/Camp.php#L98
period2 starts before period1:
https://github.com/ecamp/ecamp3/blob/devel/api/fixtures/periods.yml#L10
| new GetCollection( | ||
| security: 'is_authenticated()' | ||
| ), |
There was a problem hiding this comment.
As far as I see, you added the new GetCollections, but also kept the old, unfiltered ones everywhere. Shouldn't we switch to the new endpoints completely where possible? I think performance-wise we also found out that it doesn't make sense to fetch e.g. all schedule entries across all accessible camps, so this would enable us to disallow fetching the unfiltered collection, no?
There was a problem hiding this comment.
I'd agree, that eventually, we should remove the unfiltered GetCollection (if everyone else agrees as well).
However, some additional works is necessary.
DayResponsibles is also linked from
- CampCollaboration
- Period
ScheduleEntry is also linked from
- Activity
- Day
Day is not linked from any other entity in the backend, but in the frontend we use the /days endpoint to load all days of a camp.
Overall, I'd vote for keeping this PR limited to introducing subresources as a concept. And further down the road, as we introduce more subresources we can (carefully) start to remove the main GetCollection endpoints.
BacLuc
added
the
test-flaky-e2e!
Core Meeting DecisionWe want to replace our filters with sub resources where ever we can. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
usu
removed
test-flaky-e2e!
Reopening #4940
in order to test the fix on api-platform api-platform/core#6313