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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 15 commits into from
May 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e70d5e2
chore(performance): use subresources & uriTemplate property to avoid …
usu Apr 13, 2024
b4a1a61
update snapshots
usu Apr 14, 2024
e84147a
use fix-branch for testing
usu Apr 14, 2024
2e3c80d
fix: posting day_reponsibles on correct endpoint
usu Apr 14, 2024
8c5cc9f
switch back to main api-platform/core main repo
usu Apr 20, 2024
e82b462
add test for https://github.com/api-platform/core/pull/6313
usu Apr 20, 2024
1169043
merge devel
usu Apr 21, 2024
58e2e4f
merge devel
usu Apr 29, 2024
28e6169
upgrade to api-plaftform/core 3.2.22
usu Apr 29, 2024
ba4d615
fix normalizaiton context: embed dayResponsibles in day
usu Apr 30, 2024
56a6900
update snapshots
usu Apr 30, 2024
4133ff9
merge devel
usu May 4, 2024
0d4f1b4
update snapshots
usu May 4, 2024
7e97257
use link security
usu May 5, 2024
ed8433d
fix UriTemplateFactory: ignore uriTemplates with different base
usu May 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"php": ">=8.1.0",
"ext-ctype": "*",
"ext-iconv": "*",
"api-platform/core": "3.2.20",
"api-platform/core": "3.2.22",
"composer/package-versions-deprecated": "1.11.99",
"cweagans/composer-patches": "1.7.3",
"doctrine/doctrine-bundle": "2.12.0",
Expand Down
19 changes: 10 additions & 9 deletions api/composer.lock
View file
Open in desktop

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

21 changes: 19 additions & 2 deletions api/src/Entity/Day.php
View file
Open in desktop
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,13 @@
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_fully_authenticated()',
),
],
denormalizationContext: ['groups' => ['write']],
normalizationContext: ['groups' => ['read']],
Expand All @@ -43,6 +51,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 +71,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 +175,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
10 changes: 10 additions & 0 deletions api/src/Entity/DayResponsible.php
View file
Open in desktop
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,13 @@
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_fully_authenticated()',
),
new Post(
securityPostDenormalize: 'is_granted("CAMP_MEMBER", object) or is_granted("CAMP_MANAGER", object)'
),
Expand All @@ -46,6 +54,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
View file
Open in desktop
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
10 changes: 10 additions & 0 deletions api/src/Entity/ScheduleEntry.php
View file
Open in desktop
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,13 @@
new GetCollection(
security: 'is_authenticated()'
),
new GetCollection(
uriTemplate: self::PERIOD_SUBRESOURCE_URI_TEMPLATE,
uriVariables: [
'periodId' => new Link(toProperty: 'period', fromClass: Period::class),
],
security: 'is_fully_authenticated()',
),
new Post(
denormalizationContext: ['groups' => ['write', 'create']],
normalizationContext: self::ITEM_NORMALIZATION_CONTEXT,
Expand All @@ -61,6 +69,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
8 changes: 7 additions & 1 deletion api/tests/Api/Camps/ReadCampTest.php
View file
Open in desktop
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.

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

}

public function testGetSingleCampIsAllowedForMember() {
Expand Down
30 changes: 30 additions & 0 deletions api/tests/Api/DayResponsibles/ListDayResponsiblesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,4 +101,34 @@ 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(200);

$this->assertJsonContains(['totalItems' => 0]);
$this->assertArrayNotHasKey('items', $response->toArray()['_links']);
}
}
32 changes: 32 additions & 0 deletions api/tests/Api/Days/ListDaysTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,36 @@ 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(200);

$this->assertJsonContains(['totalItems' => 0]);
$this->assertArrayNotHasKey('items', $response->toArray()['_links']);
}
}
8 changes: 4 additions & 4 deletions api/tests/Api/Days/ReadDayTest.php
View file
Open in desktop
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
View file
Open in desktop
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