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(print): improve performance for data loading #2571

Merged
merged 29 commits into from
Apr 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
7df6555
dummy download button for nuxt print (not yet configurable)
usu Mar 20, 2022
233357f
dummy preview component for nuxt print (not yet configurable)
usu Mar 20, 2022
de3af5a
parse config on nuxt side
usu Mar 22, 2022
4141319
tailwind css + cover page
usu Mar 24, 2022
3a0e911
print configurator translation
usu Mar 24, 2022
d3e32d2
fix linter
usu Mar 24, 2022
cb8b3cd
fix react preview
usu Mar 26, 2022
13c74d7
print(react): add Richtext HTML tags for h1,h2,h3,em,s,u
usu Mar 26, 2022
055741a
print single scheduleEntry
usu Mar 26, 2022
21b87db
simplify routes
usu Mar 26, 2022
e89ed25
remove old print preview (incl. iframe messaging)
usu Mar 26, 2022
fb23a8f
style cleanup
usu Mar 26, 2022
33a72c3
TOC implementation for cover page and Toc
usu Mar 27, 2022
9b0f105
TOC implementation for program
usu Mar 27, 2022
b5b68bb
TOC for Picasso & Story overview
usu Mar 27, 2022
e657423
css cleanup
usu Mar 27, 2022
8894d0c
fix linter
usu Mar 27, 2022
7940e6b
fix server startup
usu Mar 29, 2022
9543a93
fix postCSS/pagedJs in build mode
usu Mar 29, 2022
365bfb1
fix printConfiguratior mobile view
usu Mar 29, 2022
e0820c2
only load selected print preview
usu Mar 29, 2022
a581277
Fix font bundling after folder restructuring
carlobeltrame Mar 30, 2022
f53dc78
fix & cleanup translations
usu Apr 3, 2022
fa18f1a
TOC: improved CSS for leader line
usu Apr 3, 2022
61343a1
printConfigurator: separate component name from type name
usu Apr 4, 2022
684b709
profiling for axios calls
usu Apr 4, 2022
788e1be
smarter eager loading of data
usu Apr 4, 2022
cbe97ee
period filter for ContentNode
usu Apr 4, 2022
69e9674
fix tests
usu Apr 5, 2022
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
82 changes: 82 additions & 0 deletions api/src/Doctrine/Filter/ContentNodePeriodFilter.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
<?php

namespace App\Doctrine\Filter;

use ApiPlatform\Core\Api\IriConverterInterface;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\AbstractContextAwareFilter;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
use App\Entity\Activity;
use App\Entity\ContentNode;
use Doctrine\ORM\Query\Expr\Join;
use Doctrine\ORM\QueryBuilder;
use Doctrine\Persistence\ManagerRegistry;
use Psr\Log\LoggerInterface;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\PropertyInfo\Type;
use Symfony\Component\Serializer\NameConverter\NameConverterInterface;

final class ContentNodePeriodFilter extends AbstractContextAwareFilter {
public const PERIOD_QUERY_NAME = 'period';

public function __construct(
private IriConverterInterface $iriConverter,
ManagerRegistry $managerRegistry,
?RequestStack $requestStack = null,
LoggerInterface $logger = null,
array $properties = null,
NameConverterInterface $nameConverter = null
) {
parent::__construct($managerRegistry, $requestStack, $logger, $properties, $nameConverter);
}

// This function is only used to hook in documentation generators (supported by Swagger and Hydra)
public function getDescription(string $resourceClass): array {
$description = [];
$description['period'] = [
'property' => self::PERIOD_QUERY_NAME,
'type' => Type::BUILTIN_TYPE_STRING,
'required' => false,
];

return $description;
}

protected function filterProperty(
string $property,
$value,
QueryBuilder $queryBuilder,
QueryNameGeneratorInterface $queryNameGenerator,
string $resourceClass,
string $operationName = null
) {
if (ContentNode::class !== $resourceClass) {
throw new \Exception("ContentNodePeriodFilter can only be applied to entities of type ContentNode (received: {$resourceClass}).");
}

if (self::PERIOD_QUERY_NAME !== $property) {
return;
}

// load period from query parameter value
$period = $this->iriConverter->getItemfromIri($value);

// generate alias to avoid interference with other filters
$periodParameterName = $queryNameGenerator->generateParameterName($property);
$rootJoinAlias = $queryNameGenerator->generateJoinAlias('root');
$ownerJoinAlias = $queryNameGenerator->generateJoinAlias('owner');
$activityJoinAlias = $queryNameGenerator->generateJoinAlias('activity');
$scheduleEntryJoinAlias = $queryNameGenerator->generateJoinAlias('scheduleEntry');

$rootAlias = $queryBuilder->getRootAliases()[0];

$queryBuilder
->join("{$rootAlias}.root", $rootJoinAlias)
->join("{$rootJoinAlias}.owner", $ownerJoinAlias)
->join(Activity::class, $activityJoinAlias, Join::WITH, "{$activityJoinAlias}.id = {$ownerJoinAlias}.id")
carlobeltrame marked this conversation as resolved.
Show resolved Hide resolved
->join("{$activityJoinAlias}.scheduleEntries", $scheduleEntryJoinAlias)
->andWhere($queryBuilder->expr()->eq("{$scheduleEntryJoinAlias}.period", ":{$periodParameterName}"))
;

$queryBuilder->setParameter($periodParameterName, $period);
}
}
2 changes: 1 addition & 1 deletion api/src/Entity/Activity.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#[ApiResource(
collectionOperations: [
'get' => [
'normalization_context' => ['groups' => ['read', 'Activity:ActivityResponsibles', 'Activity:ContentNodes']],
'normalization_context' => ['groups' => ['read', 'Activity:ActivityResponsibles']],
Copy link
Member

Choose a reason for hiding this comment

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

This was added to make the react-print load time bearable during development. So we should soon add similar replacements for this to react-print as you did for nuxt-print.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, absolutely. I suggest we merge this first, and then we can use the same data loading concept for react-print (by me or you - both ok for me).

'security' => 'is_authenticated()',
],
'post' => [
Expand Down
4 changes: 3 additions & 1 deletion api/src/Entity/ContentNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use ApiPlatform\Core\Annotation\ApiProperty;
use ApiPlatform\Core\Annotation\ApiResource;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\SearchFilter;
use App\Doctrine\Filter\ContentNodePeriodFilter;
use App\Repository\ContentNodeRepository;
use App\Util\EntityMap;
use App\Validator\AssertEitherIsNull;
Expand Down Expand Up @@ -41,7 +42,8 @@
denormalizationContext: ['groups' => ['write']],
normalizationContext: ['groups' => ['read']],
)]
#[ApiFilter(SearchFilter::class, properties: ['parent', 'contentType', 'root'])]
#[ApiFilter(SearchFilter::class, properties: ['contentType', 'root'])]
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the parent filter? Are you sure that it wasn't used anywhere? I can't remember, but there were a lot of tests for this which you had to remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing the parent filter results in children property to not be a single link but an array of links (basically disabling RelatedCollectionLinkNormalizer). This avoids a lot of above API Calls, because once I have the complete list of ContentNodes I can freely traverse down the child nodes without additional API calls.

hal-json-vuex is now "immune" to this, because it will just create a virtual key for the children array.

It shouldn't break anything. Just notice that in the frontend, we're actually no using the children property at the moment, because we do the filtering manually:
https://github.com/ecamp/ecamp3/blob/devel/frontend/src/components/activity/DraggableContentNodes.vue#L67

Copy link
Member

@carlobeltrame carlobeltrame Apr 6, 2022

Choose a reason for hiding this comment

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

Ah, now I get it. In this specific case (recursive data structure) we actually prefer the array of hrefs children: [{ href: '/content-nodes/1a2b3c4d' }, { href: '/content-nodes/1234abcd' }, ... ] over the shortened, filtered single link children: { href: '/content-nodes?parent=/content-nodes/00000000' }, because we don't want to embed all children, and because the array contains all the information we need for filtering in the frontend, without sending a separate request for /content-nodes?parent=/content-nodes/00000000

When I wrote the RelatedCollectionLinkNormalizer, I did not foresee that the link array could ever be more useful than the single filtered link. Maybe we don't need the parent filter right now. But I think we should introduce a way to explicitly disable the filtered link even when the filter exists. Otherwise, someone might in the future e.g. add filters for all relations, and make the performance worse without noticing.

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Explicitly comment in tests why children needs to be an array (to avoid someone adding the parent filter again in the future)

#[ApiFilter(ContentNodePeriodFilter::class)]
abstract class ContentNode extends BaseEntity implements BelongsToCampInterface, CopyFromPrototypeInterface {
/**
* @ORM\OneToOne(targetEntity="AbstractContentNodeOwner", mappedBy="rootContentNode", cascade={"persist"})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,4 @@ public function setUp(): void {
$this->getIriFor('columnLayout2campPrototype'),
];
}

public function testListColumnLayoutsFilteredByParent() {
$response = static::createClientWithCredentials()->request('GET', '/content_node/column_layouts?parent='.$this->getIriFor('columnLayout1'));
$this->assertResponseStatusCodeSame(200);
$this->assertJsonContainsItems($response, [$this->getIriFor('columnLayoutChild1')]);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public function testGetColumnLayout() {
$this->assertJsonContains([
'columns' => $contentNode->columns,
'_links' => [
'children' => ['href' => '/content_nodes?parent='.$this->getIriFor('columnLayoutChild1')],
'children' => [],
],
]);
}
Expand Down
23 changes: 0 additions & 23 deletions api/tests/Api/ContentNodes/ContentNode/ListContentNodesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,27 +44,4 @@ public function testListContentNodesIsAllowedForCollaborator() {
['href' => $this->getIriFor('multiSelect2')],
], $response->toArray()['_links']['items']);
}

public function testListContentNodesFilteredByParentIsAllowedForCollaborator() {
$parent = static::$fixtures['columnLayout1'];
$response = static::createClientWithCredentials()->request('GET', '/content_nodes?parent='.$this->getIriFor('columnLayout1'));
$this->assertResponseStatusCodeSame(200);
$this->assertJsonContains([
'totalItems' => 6,
'_links' => [
'items' => [],
],
'_embedded' => [
'items' => [],
],
]);
$this->assertEqualsCanonicalizing([
['href' => $this->getIriFor('columnLayoutChild1')],
['href' => $this->getIriFor('singleText1')],
['href' => $this->getIriFor('safetyConcept1')],
['href' => $this->getIriFor('materialNode1')],
['href' => $this->getIriFor('storyboard1')],
['href' => $this->getIriFor('multiSelect1')],
], $response->toArray()['_links']['items']);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public function testGetSingleContentNodeIsAllowedForCollaborator() {
'parent' => ['href' => $this->getIriFor($contentNode->parent)],
'owner' => ['href' => $this->getIriFor('activity1')],
'ownerCategory' => ['href' => $this->getIriFor('category1')],
'children' => ['href' => '/content_nodes?parent='.$this->getIriFor('columnLayoutChild1')],
'children' => [],
'self' => ['href' => $this->getIriFor('columnLayoutChild1')],
],
]);
Expand Down
4 changes: 1 addition & 3 deletions api/tests/Api/ContentNodes/CreateContentNodeTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,7 @@ protected function getExampleReadPayload(ContentNode $self, $attributes = [], $e
'parent' => [
'href' => $this->getIriFor($parent),
],
'children' => [
'href' => '/content_nodes?parent='.$this->getIriFor($self),
],
'children' => [],
'contentType' => [
'href' => $this->getIriFor($contentType),
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,4 @@ public function setUp(): void {
$this->getIriFor('materialNodeCampUnrelated'),
];
}

public function testListMaterialNodesFilteredByParent() {
$response = static::createClientWithCredentials()->request('GET', "{$this->endpoint}?parent=".$this->getIriFor('columnLayout1'));
$this->assertResponseStatusCodeSame(200);

$this->assertJsonContainsItems($response, [$this->getIriFor('materialNode1')]);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,4 @@ public function setUp(): void {
$this->getIriFor('multiSelectCampUnrelated'),
];
}

public function testListMultiSelectsFilteredByParent() {
$response = static::createClientWithCredentials()->request('GET', "{$this->endpoint}?parent=".$this->getIriFor('columnLayout1'));
$this->assertResponseStatusCodeSame(200);

$this->assertJsonContainsItems($response, [$this->getIriFor('multiSelect1')]);
}
}
5 changes: 0 additions & 5 deletions api/tests/Api/ContentNodes/SingleText/ListSingleTextTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,4 @@ public function setUp(): void {
$this->getIriFor('singleTextCampUnrelated'),
];
}

public function testListSingleTextsFilteredByParent() {
$response = static::createClientWithCredentials()->request('GET', "{$this->endpoint}?parent=".$this->getIriFor('columnLayout1'));
$this->assertResponseStatusCodeSame(200);
}
}
7 changes: 0 additions & 7 deletions api/tests/Api/ContentNodes/Storyboard/ListStoryboardTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,4 @@ public function setUp(): void {
$this->getIriFor('storyboardCampUnrelated'),
];
}

public function testListStoryboardsFilteredByParent() {
$response = static::createClientWithCredentials()->request('GET', "{$this->endpoint}?parent=".$this->getIriFor('columnLayout1'));
$this->assertResponseStatusCodeSame(200);

$this->assertJsonContainsItems($response, [$this->getIriFor('storyboard1')]);
}
}
155 changes: 155 additions & 0 deletions api/tests/Doctrine/Filter/ContentNodePeriodFilterTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
<?php

namespace App\Tests\Doctrine\Filter;

use ApiPlatform\Core\Api\IriConverterInterface;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
use App\Doctrine\Filter\ContentNodePeriodFilter;
use App\Entity\ContentNode;
use App\Entity\Period;
use Doctrine\ORM\Query\Expr;
use Doctrine\ORM\QueryBuilder;
use Doctrine\Persistence\ManagerRegistry;
use Exception;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;

/**
* @internal
*/
class ContentNodePeriodFilterTest extends TestCase {
private MockObject|ManagerRegistry $managerRegistryMock;
private MockObject|QueryBuilder $queryBuilderMock;
private MockObject|QueryNameGeneratorInterface $queryNameGeneratorInterfaceMock;
private MockObject|IriConverterInterface $iriConverterMock;

public function setUp(): void {
parent::setUp();
$this->managerRegistryMock = $this->createMock(ManagerRegistry::class);
$this->queryBuilderMock = $this->createMock(QueryBuilder::class);
$this->queryNameGeneratorInterfaceMock = $this->createMock(QueryNameGeneratorInterface::class);
$this->iriConverterMock = $this->createMock(IriConverterInterface::class);

$this->queryBuilderMock
->method('join')
->will($this->returnSelf())
;

$this->queryBuilderMock
->method('andWhere')
->will($this->returnSelf())
;

$this->queryBuilderMock
->method('getRootAliases')
->willReturn(['o'])
;

$expr = new Expr();
$this->queryBuilderMock
->method('expr')
->will($this->returnValue($expr))
;

$this->queryNameGeneratorInterfaceMock
->method('generateParameterName')
->willReturnCallback(fn ($field) => $field.'_a1')
;
$this->queryNameGeneratorInterfaceMock
->method('generateJoinAlias')
->willReturnCallback(fn ($field) => $field.'_j1')
;
}

public function testGetDescription() {
// given
$filter = new ContentNodePeriodFilter($this->iriConverterMock, $this->managerRegistryMock);

// when
$description = $filter->getDescription('Dummy');

// then
$this->assertEquals([
'period' => [
'property' => 'period',
'type' => 'string',
'required' => false,
],
], $description);
}

public function testFailsForResouceClassOtherThanContentNode() {
// given
$filter = new ContentNodePeriodFilter($this->iriConverterMock, $this->managerRegistryMock);

// then
$this->expectException(Exception::class);
$this->expectExceptionMessage('ContentNodePeriodFilter can only be applied to entities of type ContentNode (received: DummyClass).');

// when
$filter->apply($this->queryBuilderMock, $this->queryNameGeneratorInterfaceMock, 'DummyClass', null, ['filters' => [
'period' => '/period/123',
]]);
}

public function testDoesNothingForPropertiesOtherThanPeriod() {
// given
$filter = new ContentNodePeriodFilter($this->iriConverterMock, $this->managerRegistryMock);

// then
$this->queryBuilderMock
->expects($this->never())
->method('getRootAliases')
;

$this->queryBuilderMock
->expects($this->never())
->method('join')
;

$this->queryBuilderMock
->expects($this->never())
->method('andWhere')
;

// when
$filter->apply($this->queryBuilderMock, $this->queryNameGeneratorInterfaceMock, ContentNode::class, null, ['filters' => [
'dummyProperty' => 'abc',
]]);
}

public function testAddsFilterForPeriodProperty() {
// given
$filter = new ContentNodePeriodFilter($this->iriConverterMock, $this->managerRegistryMock);
$period = new Period();

// then
$this->iriConverterMock
->expects($this->once())
->method('getItemfromIri')
->with('/period/123')
->will($this->returnValue($period))
;

$this->queryBuilderMock
->expects($this->once())
->method('andWhere')
;

$this->queryBuilderMock
->expects($this->once())
->method('setParameter')
->with('period_a1', $period)
;

$this->queryBuilderMock
->expects($this->exactly(4))
->method('join')
;

// when
$filter->apply($this->queryBuilderMock, $this->queryNameGeneratorInterfaceMock, ContentNode::class, null, ['filters' => [
'period' => '/period/123',
]]);
}
}
Loading