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

chore(performance): use subresources & uriTemplate property #4968

Merged
merged 15 commits into from
May 15, 2024
Merged
6 changes: 3 additions & 3 deletions api/composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions api/config/packages/api_platform.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ api_platform:
version: 1.0.0
show_webby: false
use_symfony_listeners: true
enable_link_security: true
mapping:
paths:
- '%kernel.project_dir%/src/Entity'
Expand Down
26 changes: 24 additions & 2 deletions api/src/Entity/Day.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GetCollection;
use ApiPlatform\Metadata\Link;
use App\Repository\DayRepository;
use App\Serializer\Normalizer\RelatedCollectionLink;
use Doctrine\Common\Collections\ArrayCollection;
Expand All @@ -33,6 +34,18 @@
normalizationContext: self::COLLECTION_NORMALIZATION_CONTEXT,
security: 'is_authenticated()'
),
new GetCollection(
uriTemplate: self::PERIOD_SUBRESOURCE_URI_TEMPLATE,
uriVariables: [
'periodId' => new Link(
toProperty: 'period',
fromClass: Period::class,
security: 'is_granted("CAMP_COLLABORATOR", period) or is_granted("CAMP_IS_PROTOTYPE", period)'
),
],
normalizationContext: self::COLLECTION_NORMALIZATION_CONTEXT,
security: 'is_fully_authenticated()',
),
],
denormalizationContext: ['groups' => ['write']],
normalizationContext: ['groups' => ['read']],
Expand All @@ -43,6 +56,8 @@
#[ORM\Entity(repositoryClass: DayRepository::class)]
#[ORM\UniqueConstraint(name: 'offset_period_idx', columns: ['periodId', 'dayOffset'])]
class Day extends BaseEntity implements BelongsToCampInterface {
public const PERIOD_SUBRESOURCE_URI_TEMPLATE = '/periods/{periodId}/days{._format}';

public const ITEM_NORMALIZATION_CONTEXT = [
'groups' => [
'read',
Expand All @@ -61,7 +76,11 @@ class Day extends BaseEntity implements BelongsToCampInterface {
/**
* The list of people who have a whole-day responsibility on this day.
*/
#[ApiProperty(writable: false, example: '["/day_responsibles/1a2b3c4d"]')]
#[ApiProperty(
writable: false,
uriTemplate: DayResponsible::DAY_SUBRESOURCE_URI_TEMPLATE,
example: '/days/1a2b3c4d/day_responsibles'
)]
#[Groups(['read'])]
#[ORM\OneToMany(targetEntity: DayResponsible::class, mappedBy: 'day', orphanRemoval: true)]
public Collection $dayResponsibles;
Expand Down Expand Up @@ -161,7 +180,10 @@ public function getEnd(): ?\DateTime {
/**
* @return DayResponsible[]
*/
#[ApiProperty(readableLink: true)]
#[ApiProperty(
readableLink: true,
uriTemplate: DayResponsible::DAY_SUBRESOURCE_URI_TEMPLATE,
)]
#[SerializedName('dayResponsibles')]
#[Groups(['Day:DayResponsibles'])]
public function getEmbeddedDayResponsibles(): array {
Expand Down
13 changes: 13 additions & 0 deletions api/src/Entity/DayResponsible.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use ApiPlatform\Metadata\Delete;
use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GetCollection;
use ApiPlatform\Metadata\Link;
use ApiPlatform\Metadata\Post;
use App\Repository\DayResponsibleRepository;
use App\Validator\AssertBelongsToSameCamp;
Expand All @@ -31,6 +32,16 @@
new GetCollection(
security: 'is_authenticated()'
),
Comment on lines 32 to 34
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

new GetCollection(
uriTemplate: self::DAY_SUBRESOURCE_URI_TEMPLATE,
uriVariables: [
'dayId' => new Link(
toProperty: 'day',
fromClass: Day::class,
security: 'is_granted("CAMP_COLLABORATOR", day) or is_granted("CAMP_IS_PROTOTYPE", day)'
),
],
),
new Post(
securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)'
),
Expand All @@ -46,6 +57,8 @@
#[ORM\Entity(repositoryClass: DayResponsibleRepository::class)]
#[ORM\UniqueConstraint(name: 'day_campCollaboration_unique', columns: ['dayId', 'campCollaborationId'])]
class DayResponsible extends BaseEntity implements BelongsToCampInterface {
public const DAY_SUBRESOURCE_URI_TEMPLATE = '/days/{dayId}/day_responsibles{._format}';

/**
* The day on which the person is responsible.
*/
Expand Down
17 changes: 14 additions & 3 deletions api/src/Entity/Period.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,11 @@ class Period extends BaseEntity implements BelongsToCampInterface {
/**
* The days in this time period. These are generated automatically.
*/
#[ApiProperty(writable: false, example: '["/days?period=/periods/1a2b3c4d"]')]
#[ApiProperty(
writable: false,
uriTemplate: Day::PERIOD_SUBRESOURCE_URI_TEMPLATE,
example: '/periods/1a2b3c4d/days'
)]
#[Groups(['read'])]
#[ORM\OneToMany(targetEntity: Day::class, mappedBy: 'period', orphanRemoval: true, cascade: ['persist'])]
#[ORM\OrderBy(['dayOffset' => 'ASC'])]
Expand All @@ -80,7 +84,11 @@ class Period extends BaseEntity implements BelongsToCampInterface {
*
* @var Collection<int, ScheduleEntry>
*/
#[ApiProperty(writable: false, example: '["/schedule_entries/1a2b3c4d"]')]
#[ApiProperty(
writable: false,
uriTemplate: ScheduleEntry::PERIOD_SUBRESOURCE_URI_TEMPLATE,
example: '/periods/1a2b3c4d/schedule_entries'
)]
#[Groups(['read'])]
#[ORM\OneToMany(targetEntity: ScheduleEntry::class, mappedBy: 'period')]
#[ORM\OrderBy(['startOffset' => 'ASC', 'left' => 'ASC', 'endOffset' => 'DESC', 'id' => 'ASC'])]
Expand Down Expand Up @@ -178,7 +186,10 @@ public function __construct() {
/**
* @return Day[]
*/
#[ApiProperty(readableLink: true)]
#[ApiProperty(
readableLink: true,
uriTemplate: Day::PERIOD_SUBRESOURCE_URI_TEMPLATE,
)]
#[SerializedName('days')]
#[Groups('Period:Days')]
public function getEmbeddedDays(): array {
Expand Down
14 changes: 14 additions & 0 deletions api/src/Entity/ScheduleEntry.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use ApiPlatform\Metadata\Delete;
use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GetCollection;
use ApiPlatform\Metadata\Link;
use ApiPlatform\Metadata\Patch;
use ApiPlatform\Metadata\Post;
use App\Doctrine\Filter\ExpressionDateTimeFilter;
Expand Down Expand Up @@ -42,6 +43,17 @@
new GetCollection(
security: 'is_authenticated()'
),
new GetCollection(
uriTemplate: self::PERIOD_SUBRESOURCE_URI_TEMPLATE,
uriVariables: [
'periodId' => new Link(
toProperty: 'period',
fromClass: Period::class,
security: 'is_granted("CAMP_COLLABORATOR", period) or is_granted("CAMP_IS_PROTOTYPE", period)'
),
],
security: 'is_fully_authenticated()',
),
new Post(
denormalizationContext: ['groups' => ['write', 'create']],
normalizationContext: self::ITEM_NORMALIZATION_CONTEXT,
Expand All @@ -61,6 +73,8 @@
#[ORM\Index(columns: ['startOffset'])]
#[ORM\Index(columns: ['endOffset'])]
class ScheduleEntry extends BaseEntity implements BelongsToCampInterface {
public const PERIOD_SUBRESOURCE_URI_TEMPLATE = '/periods/{periodId}/schedule_entries{._format}';

public const ITEM_NORMALIZATION_CONTEXT = [
'groups' => ['read', 'ScheduleEntry:Activity'],
'swagger_definition_name' => 'read',
Expand Down
15 changes: 9 additions & 6 deletions api/src/Metadata/Resource/Factory/UriTemplateFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,11 @@ public function createFromResourceClass(string $resourceClass): array {
$getCollectionOperation = OperationHelper::findOneByType($resourceMetadataCollection, GetCollection::class);

$baseUri = $this->iriConverter->getIriFromResource($resourceClass, UrlGeneratorInterface::ABS_PATH, $getCollectionOperation);
$relativeUri = $this->iriConverter->getIriFromResource($resourceClass, UrlGeneratorInterface::REL_PATH, $getCollectionOperation);

$idParameter = $this->getIdParameter($resourceMetadataCollection);
$queryParameters = $this->getQueryParameters($resourceClass, $resourceMetadataCollection);
$additionalPathParameter = $this->allowsActionParameter($resourceMetadataCollection) ? '{/action}' : '';
$additionalPathParameter = $this->allowsActionParameter($relativeUri, $resourceMetadataCollection) ? '{/action}' : '';

return [
$baseUri.$idParameter.$additionalPathParameter.$queryParameters,
Expand Down Expand Up @@ -150,18 +152,19 @@ protected function getPaginationParameters(ResourceMetadataCollection $resourceM
return $parameters;
}

protected function allowsActionParameter(ResourceMetadataCollection $resourceMetadataCollection): bool {
protected function allowsActionParameter(string $uriTemplateBase, ResourceMetadataCollection $resourceMetadataCollection): bool {
foreach ($resourceMetadataCollection->getIterator()->current()->getOperations() as $operation) {
/*
* Matches:
* {/inviteKey}/find
* users{/id}/activate
* - invitations/{/inviteKey}/find
* - /users{/id}/activate
*
* Does not match:
* profiles{/id}
* - profiles{/id}
* - any uriTemplate, which doesn't start with the same $uriTemplateBase
*/
if ($operation instanceof HttpOperation) {
if (preg_match('/^.*\\/?{.*}\\/.+$/', $operation->getUriTemplate() ?? '')) {
if (preg_match('/^\/?'.preg_quote($uriTemplateBase, '/').'.*\\/?{.*}\\/.+$/', $operation->getUriTemplate() ?? '')) {
return true;
}
}
Expand Down
8 changes: 7 additions & 1 deletion api/tests/Api/Camps/ReadCampTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public function testGetSingleCampIsAllowedForGuest() {
/** @var Camp $camp */
$camp = static::getFixture('camp1');
$user = static::getFixture('user3guest');
static::createClientWithCredentials(['email' => $user->getEmail()])->request('GET', '/camps/'.$camp->getId());
$response = static::createClientWithCredentials(['email' => $user->getEmail()])->request('GET', '/camps/'.$camp->getId());
$this->assertResponseStatusCodeSame(200);
$this->assertJsonContains([
'id' => $camp->getId(),
Expand All @@ -67,6 +67,12 @@ public function testGetSingleCampIsAllowedForGuest() {
'categories' => ['href' => '/categories?camp=%2Fcamps%2F'.$camp->getId()],
],
]);

$responseArray = $response->toArray();
$period1 = static::getFixture('period1');
$period2 = static::getFixture('period2');
$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']);
Comment on lines +74 to +75
Copy link
Member

Choose a reason for hiding this comment

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

Is the order of these two periods in the response guaranteed? Or should you use $this->assertEqualsCanonicalizing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

}

public function testGetSingleCampIsAllowedForMember() {
Expand Down
27 changes: 27 additions & 0 deletions api/tests/Api/DayResponsibles/ListDayResponsiblesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -101,4 +101,31 @@ public function testListDayResponsiblesFilteredByDayInCampPrototypeIsAllowedForU
['href' => $this->getIriFor('dayResponsible1day1period1campPrototype')],
], $response->toArray()['_links']['items']);
}

public function testListDayResponsiblesAsDaySubresourceIsAllowedForCollaborator() {
$day = static::getFixture('day1period1');
$response = static::createClientWithCredentials()->request('GET', '/days/'.$day->getId().'/day_responsibles');
$this->assertResponseStatusCodeSame(200);
$this->assertJsonContains([
'totalItems' => 1,
'_links' => [
'items' => [],
],
'_embedded' => [
'items' => [],
],
]);
$this->assertEqualsCanonicalizing([
['href' => $this->getIriFor('dayResponsible1')],
], $response->toArray()['_links']['items']);
}

public function testListDayResponsiblesAsDaySubresourceIsDeniedForUnrelatedUser() {
$day = static::getFixture('day1period1');
$response = static::createClientWithCredentials(['email' => static::$fixtures['user4unrelated']->getEmail()])
->request('GET', '/days/'.$day->getId().'/day_responsibles')
;

$this->assertResponseStatusCodeSame(404);
}
}
29 changes: 29 additions & 0 deletions api/tests/Api/Days/ListDaysTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,33 @@ public function testListDaysFilteredByPeriodInCampPrototypeIsAllowedForCollabora
['href' => $this->getIriFor('day1period1campPrototype')],
], $response->toArray()['_links']['items']);
}

public function testListDaysAsPeriodSubresourceIsAllowedForCollaborator() {
$period = static::getFixture('period1');
$response = static::createClientWithCredentials()->request('GET', '/periods/'.$period->getId().'/days');
$this->assertResponseStatusCodeSame(200);
$this->assertJsonContains([
'totalItems' => 3,
'_links' => [
'items' => [],
],
'_embedded' => [
'items' => [],
],
]);
$this->assertEqualsCanonicalizing([
['href' => $this->getIriFor('day1period1')],
['href' => $this->getIriFor('day2period1')],
['href' => $this->getIriFor('day3period1')],
], $response->toArray()['_links']['items']);
}

public function testListDaysAsPeriodSubresourceIsDeniedForUnrelatedUser() {
$period = static::getFixture('period1');
$response = static::createClientWithCredentials(['email' => static::$fixtures['user4unrelated']->getEmail()])
->request('GET', '/periods/'.$period->getId().'/days')
;

$this->assertResponseStatusCodeSame(404);
}
}
8 changes: 4 additions & 4 deletions api/tests/Api/Days/ReadDayTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public function testGetSingleDayIsAllowedForGuest() {
'_links' => [
'period' => ['href' => $this->getIriFor('period1')],
'scheduleEntries' => ['href' => '/schedule_entries?period=%2Fperiods%2F'.$day->period->getId().'&start%5Bstrictly_before%5D='.urlencode($end).'&end%5Bafter%5D='.urlencode($start)],
'dayResponsibles' => ['href' => '/day_responsibles?day=%2Fdays%2F'.$day->getId()],
'dayResponsibles' => ['href' => '/days/'.$day->getId().'/day_responsibles'],
],
]);
}
Expand All @@ -83,7 +83,7 @@ public function testGetSingleDayIsAllowedForMember() {
'_links' => [
'period' => ['href' => $this->getIriFor('period1')],
'scheduleEntries' => ['href' => '/schedule_entries?period=%2Fperiods%2F'.$day->period->getId().'&start%5Bstrictly_before%5D='.urlencode($end).'&end%5Bafter%5D='.urlencode($start)],
'dayResponsibles' => ['href' => '/day_responsibles?day=%2Fdays%2F'.$day->getId()],
'dayResponsibles' => ['href' => '/days/'.$day->getId().'/day_responsibles'],
],
]);
}
Expand All @@ -102,7 +102,7 @@ public function testGetSingleDayIsAllowedForManager() {
'_links' => [
'period' => ['href' => $this->getIriFor('period1')],
'scheduleEntries' => ['href' => '/schedule_entries?period=%2Fperiods%2F'.$day->period->getId().'&start%5Bstrictly_before%5D='.urlencode($end).'&end%5Bafter%5D='.urlencode($start)],
'dayResponsibles' => ['href' => '/day_responsibles?day=%2Fdays%2F'.$day->getId()],
'dayResponsibles' => ['href' => '/days/'.$day->getId().'/day_responsibles'],
],
]);
}
Expand All @@ -121,7 +121,7 @@ public function testGetSingleDayFromCampPrototypeIsAllowedForUnrelatedUser() {
'_links' => [
'period' => ['href' => $this->getIriFor('period1campPrototype')],
'scheduleEntries' => ['href' => '/schedule_entries?period=%2Fperiods%2F'.$day->period->getId().'&start%5Bstrictly_before%5D='.urlencode($end).'&end%5Bafter%5D='.urlencode($start)],
'dayResponsibles' => ['href' => '/day_responsibles?day=%2Fdays%2F'.$day->getId()],
'dayResponsibles' => ['href' => '/days/'.$day->getId().'/day_responsibles'],
],
]);
}
Expand Down
12 changes: 6 additions & 6 deletions api/tests/Api/Periods/ReadPeriodTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ public function testGetSinglePeriodIsAllowedForGuest() {
'_links' => [
'camp' => ['href' => $this->getIriFor('camp1')],
'materialItems' => ['href' => '/material_items?period=%2Fperiods%2F'.$period->getId()],
'days' => ['href' => '/days?period=%2Fperiods%2F'.$period->getId()],
'scheduleEntries' => ['href' => '/schedule_entries?period=%2Fperiods%2F'.$period->getId()],
'days' => ['href' => '/periods/'.$period->getId().'/days'],
'scheduleEntries' => ['href' => '/periods/'.$period->getId().'/schedule_entries'],
'contentNodes' => ['href' => '/content_nodes?period=%2Fperiods%2F'.$period->getId()],
'dayResponsibles' => ['href' => '/day_responsibles?day.period=%2Fperiods%2F'.$period->getId()],
],
Expand All @@ -86,8 +86,8 @@ public function testGetSinglePeriodIsAllowedForMember() {
'_links' => [
'camp' => ['href' => $this->getIriFor('camp1')],
'materialItems' => ['href' => '/material_items?period=%2Fperiods%2F'.$period->getId()],
'days' => ['href' => '/days?period=%2Fperiods%2F'.$period->getId()],
'scheduleEntries' => ['href' => '/schedule_entries?period=%2Fperiods%2F'.$period->getId()],
'days' => ['href' => '/periods/'.$period->getId().'/days'],
'scheduleEntries' => ['href' => '/periods/'.$period->getId().'/schedule_entries'],
],
]);
}
Expand All @@ -105,8 +105,8 @@ public function testGetSinglePeriodIsAllowedForManager() {
'_links' => [
'camp' => ['href' => $this->getIriFor('camp1')],
'materialItems' => ['href' => '/material_items?period=%2Fperiods%2F'.$period->getId()],
'days' => ['href' => '/days?period=%2Fperiods%2F'.$period->getId()],
'scheduleEntries' => ['href' => '/schedule_entries?period=%2Fperiods%2F'.$period->getId()],
'days' => ['href' => '/periods/'.$period->getId().'/days'],
'scheduleEntries' => ['href' => '/periods/'.$period->getId().'/schedule_entries'],
],
]);
}
Expand Down
Loading
Loading