refactor(api)!: improve doctrine performance; remove AbstractContentNodeOwner by usu · Pull Request #2683 · ecamp/ecamp3

usu · 2022-05-01T11:01:19Z

Implements improvements discussed in #2569 and agreed in #2569 (comment)

Implemented

Agreed refactoring
- Root is always of type ColumnLayout
- Remove owner relation from ContentNode Entity
- Use new interface to detect ContentNodes and execute necessary DB queries in security voters
- Remove AbstractContentNodeOwner
Fixed existing API tests
Fixed migrations

To do

Fix frontend
Add tests for new/additional logic

Time measurement

Endpoint	Collection ms	Entity ms
activities	283	110
contentNodes	363	72
multiSelects	94	82
storyboards	144	67
columnLayouts	132	71
multiSelectOptions	80	74
materialNodes	110	81
singleTexts	123	74
storyboardSections	101	69
days	72	66
categories	106	84
contentTypes	53	48
camps	74	83
periods	76	77
materialLists	76	60
scheduleEntries	100	79
materialItems	84	65
activityResponsibles	66	67
dayResponsibles	60	68
campCollaborations	121	71

…--> ContentNode relation

…-option1

BacLuc

Ist mir leider zu gross für jetzt.
Mache später weiter

…face + minor changes as per review

carlobeltrame

Did another pass. I can't say I really understand how of all the many changes you did here fit into one single big picture, so I am trying to instead review the individual files.

carlobeltrame · 2022-05-13T14:41:44Z

    collectionOperations: [
        'get' => [
-            'normalization_context' => ['groups' => ['read', 'Activity:ActivityResponsibles']],
+            'normalization_context' => ['groups' => ['read', 'Activity:ActivityResponsibles', 'Activity:ScheduleEntries']],


We have ScheduleEntry:Activity in the item normalization context of ScheduleEntry. Can this never lead to an infinite loop? Why did you need to add this?

In ScheduleEntryLinks.vue https://github.com/ecamp/ecamp3/pull/2683/files#diff-88775c426170bcdeb7ef38ff0453b604a09d3f830d372c911e33286d568ac63aR5 we list/display all ScheduleEntries of an Activity.

Before this PR, a separate network request was necessary for every activity. With embedding ScheduleEntries into Activity collection, I can load all Activities and ScheduleEntries of a camp with a single network request and traverse in both sides without additional network request.

Infinite Loop should not happen with our setup of normalization groups, unless you start to explicitly embedded the return relations in the normalization context, i.e.:

'normalization_context' => ['groups' => ['read', 'Activity:ActivityResponsibles', 'Activity:ScheduleEntries', 'ScheduleEntry:Activity']],

A potential (and probably more performant?) alternative could be to avoid RelatedCollectionLink on activity->scheduleEntries (similar as for contentNode->children). In that case, I can load all data of a camp with 2 network request (1 for all activities + 1 for all scheduleEntries) and I can still traverse freely in both directions.

carlobeltrame · 2022-05-13T14:48:19Z

-        messageNoneNull: 'Must be null on root content nodes.'
-    )]
-    #[AssertBelongsToSameOwner(groups: ['update'])]
+    #[Assert\NotNull(groups: ['create'])] // Root nodes have parent:null, but manually creating root nodes is not allowed


You now allow changing the parent of an existing ContentNode to null. I think you need to revert this to use AssertEitherIsNull again.

I think #[AssertBelongsToSameRoot(groups: ['update'])] already disallows changing parent to null (because that would implicitly change the root). There's also a test for this (testPatchColumnLayoutValidatesMissingParent).

But I think it doesn't hurt to #[Assert\NotNull], that would make it more explicit.

Ah, now I remember why this was not possible. If #[Assert\NotNull] is enabled for update, I cannot modify root-nodes anymore. So I decided to to verify this in #[AssertBelongsToSameRoot]. Basically, changing the parent property from non-null to null would make the ContentNode a root and therefore implictly also changing the root (which is not allowed according to #[AssertBelongsToSameRoot]).

Happy to discuss counterproposals.

I think you need to revert this to use AssertEitherIsNull again.

There's no owner anymore. Before there were 2 evidences that a ContentNode is a root (parent is null and owner is not-null). They had to be in sync with AssertEitherIsNull. Now there's only parent and nothing to verify/sync against.

carlobeltrame · 2022-05-13T14:49:27Z

-    #[ApiProperty(writable: false, example: '/activities/1a2b3c4d')]
-    #[Groups(['read'])]
-    public function getRootOwner(): Activity|Category|AbstractContentNodeOwner|null {
+    #[ApiProperty(readable: false)]


Is it writable now? Otherwise, you should add writable: false back, no?

I think it should not be an ApiProperty at all, will fix.

Fixed with 90264e2

carlobeltrame · 2022-05-13T14:52:14Z

+    public function getRootDescendants(): array {
+        return $this->rootDescendants->getValues();
+    }
+
+    public function addRootDescendant(ContentNode $rootDescendant): self {
+        if (!$this->rootDescendants->contains($rootDescendant)) {
+            $this->rootDescendants[] = $rootDescendant;
+            $rootDescendant->root = $this;
+        }
+
+        return $this;
+    }
+
+    public function removeRootDescendant(ContentNode $rootDescendant): self {
+        if ($this->rootDescendants->removeElement($rootDescendant)) {
+            // reset the owning side (unless already changed)
+            if ($rootDescendant->root === $this) {
+                $rootDescendant->root = null;
+            }
+        }
+
+        return $this;
+    }


These will also be available on nested column layouts, and appear in the API (?). We need to take care to only use these methods on root content nodes. Or maybe it would help to create another dedicated root content node type instead of using ColumnLayout for it.

These will also be available on nested column layouts, and appear in the API (?). We need to take care to only use these methods on root content nodes.

rootDescendants will be available on all ColumnLayout entities, but empty for nested ones (non-root entities). Not displaying this for nested ColumnLayouts would break the rule, that an entity of the same type always looks the same.

Or maybe it would help to create another dedicated root content node type instead of using ColumnLayout for it.

We discussed this as an option in #2569 and, yes, I have the opinion it would be cleaner. In the meeting we settled on using ColumnLayout as root node for the first shot.

This would however become irrelevant, in case we decide to implement #2633

carlobeltrame · 2022-05-13T14:55:13Z

    )]
    #[Groups(['create'])]
-    #[ORM\OneToOne(targetEntity: Profile::class, inversedBy: 'user', cascade: ['persist'])]
+    #[ORM\OneToOne(targetEntity: Profile::class, inversedBy: 'user', cascade: ['persist'], fetch: 'EAGER')]


What does this have to do with this PR?

This was a small general performance improvement: With every call User is loaded and then Profile for access verification. The latter is because of this line: https://github.com/ecamp/ecamp3/blob/devel/api/src/Entity/User.php#L208 (roles stored on profile entity)

Eager loading combines both entities in 1 DB query instead of 2 separate ones.

carlobeltrame · 2022-05-13T15:19:19Z

+        $previousObject = $this->requestStack->getCurrentRequest()?->attributes?->get('previous_data');
+        $previousValue = $previousObject?->{$this->context->getPropertyName()};
+
+        // root nodes have parent:null (before and after)
+        if ($previousObject instanceof ContentNode && null === $value && null === $previousValue) {
+            return;
+        }


Does a root content node have $this->root === null? Would things be easier if we had a dedicated RootContentNode class which just returns $this when asked for getRoot()?

Root nodes have $parent===null and $root === $this (same as before).

$root === null could also be an option for root nodes.

Implementing a dedicated RootContentNode could be an option, yes.

carlobeltrame · 2022-05-13T15:20:33Z

        }

-        if ($value->entityClass !== get_class($object)) {
+        if ($value->entityClass !== $this->getObjectClass($object)) {


Why is getObjectClass better than get_class? Should we start using that everywhere now?

Yes, we should. get_class() could return the doctrine proxy class, if for any reason the object is not yet fully resolved form the DB. Instanceof comparisons would still be fine, but direct comparisons could fail.

It probably shouldn't happen here (if I'm validating an object, that object should already be resolved). But I stumbled over this in another code fragment and just felt safer to use getObjectClass everywhere, when $object is a Doctrine entity.

carlobeltrame · 2022-05-13T15:31:23Z

+      const root = await contentNode.$href('root')
+
+      return this.camp.activities().items.find(activity => {
+        return activity.rootContentNode()._meta.self === root


Do we have any mechanisms in place now to make sure that every root content node can only belong to one activity or category? And to make sure that no sub-column-layout is the root of some other activity or category?

Not in DB, no.

The protection is "only" from API side, because rootCotentNode is not writable and always a new one is created, when an activity or a category is created.

So, could we add a unique index in the DB to the activity#root_content_node_id and category#root_content_node_id DB columns? Do you see any drawbacks with that?

Is Doctrine and Postgres able to handle unique constraints across multiple tables? If so, sure. No drawback I see.

Discussion: Add at least a unique index to each table (activity + category)

This was already implemented. See unique: true on JoinColumn Attribute:
https://github.com/ecamp/ecamp3/pull/2683/files#diff-6c5bf17f3740f1a2190f671331eb17d4f1b4ea14ff04d2883f63f3ec379093a1L20

and the actual indexes in the migration files:
https://github.com/ecamp/ecamp3/pull/2683/files#diff-7f44091fbb4175265ebac8238f7aebed816b76abaf3bbcf6a153a0eb46413cecR38

https://github.com/ecamp/ecamp3/pull/2683/files#diff-7f44091fbb4175265ebac8238f7aebed816b76abaf3bbcf6a153a0eb46413cecR46

carlobeltrame · 2022-05-13T15:36:46Z

+      contentNodeOwner: {
+        preferredContentTypes: () => this.preferredContentTypes,
+        contentNodes: () => this.contentNodes,
+        camp: () => this.camp
+      }


Ah, I was surprised to read that the provided contentNodeOwner is not an entity from the API. Don't you think it would be better for developer experience if we passed a real entity in here, so that nobody unexpectedly gets errors when they try to access e.g. the self link of contentNodeOwner?

Maybe an other name for this object could be a solution. I struggled here a bit to make the provide/inject data reactive and it finally worked when I encapsulated everything in this contentNodeOwner object.

I don't necessarily want to provide the real owner (which is a category or an activity). Otherwise the components below have to include separate logic to figure out whether the owner is a category or an activity. This way I can just inject the preferredContentTypes and the subcomponents don't have to care, whether they live inside a category or inside an activity.

So far the only relevant difference between activity and category is the preferredContentTypes. If we added a #[RelatedCollectionLink()] function getPreferredContentTypes() { ... } to Activity, wouldn't that allow us to provide a real API entity from the store here?
Might involve the same discussion as in #2700 (comment) though.

Improved with 0745c22

carlobeltrame · 2022-05-13T15:38:03Z

        .filter(idx => idx !== null)
    })
+
+    this.camp().activities().$reload()


Why is this needed now?

A small improvement to avoid several network requests.
In the component MaterialTable.vue https://github.com/ecamp/ecamp3/pull/2683/files#diff-850ca229cd293729e94d2201607458788fe4d0e9369f0fa45a605374f75c529dR369 the activities are needed to match materialItem.materialNode.root with activity.rootContentNode

usu · 2022-05-14T04:34:16Z

Did another pass. I can't say I really understand how of all the many changes you did here fit into one single big picture, so I am trying to instead review the individual files.

Nice thanks. I can imagine not everything is self-explanatory. We spent quite some time in the last meeting to discuss the way forward.
Will try to have a look at the comments. From my perspective, there's no need to merge before the next meeting, so we can also discuss there. But would we good if, by then, everyone has an understanding of what is included in the PR.

BacLuc

Funktioniert alles noch: (Neue Kategorie erstellen, Activity von Category erstellen, Camp von CampPrototype erstellen, drucken, ContentNodes herumschieben).
Performance fühlt sich besser an.

Ein kleines Problem mit der Migration, es ist aber für mich auch ok, wenn wir meinen komischen stand lokal nicht fixen mit der migration.

Wenn die kommentare von cosinus geklärt sind, kann das rein.

Danke vielmals

BacLuc · 2022-05-15T13:12:24Z

@@ -0,0 +1,43 @@
+<?php


Diese migration is fehlgeschlagen bei meinem lokalen stand:

In ExceptionConverter.php line 54: ecamp3-api | ecamp3-api | An exception occurred while executing a query: SQLSTATE[23503]: Foreign key ecamp3-api | violation: 7 ERROR: insert or update on table "abstract_content_node_owne ecamp3-api | r" violates foreign key constraint "fk_8e710ab4f886581c" ecamp3-api | DETAIL: Key (rootcontentnodeid)=(3e67389a5898) is not present in table "co ecamp3-api | ntent_node_columnlayout". ecamp3-api | ecamp3-api | ecamp3-api | In Exception.php line 30: ecamp3-api | ecamp3-api | SQLSTATE[23503]: Foreign key violation: 7 ERROR: insert or update on table ecamp3-api | "abstract_content_node_owner" violates foreign key constraint "fk_8e710ab4 ecamp3-api | f886581c" ecamp3-api | DETAIL: Key (rootcontentnodeid)=(3e67389a5898) is not present in table "co ecamp3-api | ntent_node_columnlayout". ecamp3-api | ecamp3-api | ecamp3-api | In Connection.php line 72: ecamp3-api | ecamp3-api | SQLSTATE[23503]: Foreign key violation: 7 ERROR: insert or update on table ecamp3-api | "abstract_content_node_owner" violates foreign key constraint "fk_8e710ab4 ecamp3-api | f886581c" ecamp3-api | DETAIL: Key (rootcontentnodeid)=(3e67389a5898) is not present in table "co ecamp3-api | ntent_node_columnlayout". ecamp3-api |

Version202205150310_data.txt

Ist die Frage ob man das noch anschauen will.
Sobald alle ihre db's gespühlt haben, ist das wieder ok.

…lls)

…-option1

pmattmann

minor changes

pmattmann · 2022-05-23T17:19:17Z

 namespace eCamp\Core\Entity;

-interface BelongsToContentNodeInterface {
+interface BelongsToContentNodeTreeInterface {


Please also rename the file

hmm, weird. I double checked and noticed that I renamed the file already...
Then I noticed that this is in the old backend --> victim of mass replace😆

If ok for you, I'd rather not fix this, as we're going to delete the old backend code soon anyway.

usu · 2022-05-23T19:15:07Z

-    )]
-    #[AssertBelongsToSameOwner(groups: ['update'])]
+    #[Assert\NotNull(groups: ['create'])] // Root nodes have parent:null, but manually creating root nodes is not allowed
+    #[AssertBelongsToSameRoot(groups: ['update'])]


Alternative name: AssertNoRootChange
// cannot change non-null value to null value

Fixed with 90264e2#diff-3def8e7c7777a7825957abaaedad9d242fd2e421e26c8925f7fd50ff0cb8c146

usu · 2022-05-27T07:55:16Z

@carlobeltrame & @pmattmann:
I've implemented the changes discussed in the last call. Can you have another look at it to verify, if your comments are properly covered?

manuelmeister · 2022-05-29T12:53:14Z

  computed: {
-    preferredContentTypes () {
-      return this.contentNodeOwner.preferredContentTypes().items.map(this.contentTypeMap)
+    preferredContentTypesList () {


Wieso wurde das umbenannt?

Name conflict mit dem injected property preferredContentTypes

manuelmeister · 2022-05-29T12:53:37Z

      <v-list>
        <!-- preferred content types -->
-        <v-list-item v-for="act in preferredContentTypes"
+        <v-list-item v-for="act in preferredContentTypesList"


für was steht act?

Müsste ich raten... 😆 ActivityContentType vielleicht?
Habs vereinfacht mit eab7b42

carlobeltrame · 2022-05-31T11:31:05Z

+  provide () {
+    return {
+      preferredContentTypes: () => this.preferredContentTypes,
+      rootContentNodes: () => this.contentNodes,


I like it this way 👍
Only the name rootContentNodes is not optimal I think. When I read that in a component which gets it injected, I think of a set of root content nodes, i.e. the roots of multiple trees. How about allContentNodes or contentNodesInTree?

Renamed to allContentNodes in eab7b42

Much better name!

…o allContentNodes

usu added 9 commits April 15, 2022 14:22

performance measurement for all endpoints

2b86b29

add indexes for searchable properties

f181d4a

ColumnLayout always as root; workarund for AbstractContentNodeOwner <…

8d2a332

…--> ContentNode relation

remove rootOwner and ownerCategory from serializaiton

341d237

major refactor: remove owner relation from ContentNode entity

27b006c

Merge remote-tracking branch 'ecamp/devel' into chore/api-performance…

40ff86f

…-option1

fix migration

4cba81a

remove AbstractContentNodeOwner

8896cd5

fix phpstan/psalm errors; move rootDescendants to ColumnLayout

1afab4a

usu requested review from BacLuc, carlobeltrame and pmattmann May 1, 2022 11:05

usu added 3 commits May 1, 2022 16:19

fix frontend

a1f15ec

fix react print

3157cf2

add additional test coverage

5f85e8f

BacLuc reviewed May 3, 2022

View reviewed changes

Comment thread api/migrations/schema/Version20220416125104.php Outdated

Comment thread api/migrations/schema/Version20220416125104.php Outdated

Comment thread api/migrations/schema/Version20220501060227.php Outdated

Comment thread api/migrations/schema/Version20220501101420.php Outdated

carlobeltrame reviewed May 5, 2022

View reviewed changes

Comment thread api/src/Entity/BelongsToContentNodeInterface.php Outdated

usu mentioned this pull request May 8, 2022

React print performance adjustment #2700

Merged

rename BelongsToContentNodeInterface to BelongsToContentNodeTreeInter…

27c478d

…face + minor changes as per review

carlobeltrame reviewed May 13, 2022

View reviewed changes

BacLuc approved these changes May 15, 2022

View reviewed changes

usu added the Meeting Discuss Am nächsten Core-Meeting besprechen label May 17, 2022

usu added 2 commits May 20, 2022 14:31

in-memory calculation of scheduleEntry number (avoid additional DB ca…

37abf91

…lls)

Merge remote-tracking branch 'ecamp/devel' into chore/api-performance…

b782ed4

…-option1

pmattmann requested changes May 23, 2022

View reviewed changes

usu commented May 23, 2022

View reviewed changes

usu added 2 commits May 27, 2022 08:39

minor fixes as per review

90264e2

simplify provide/inject

0745c22

usu removed the Meeting Discuss Am nächsten Core-Meeting besprechen label May 27, 2022

usu mentioned this pull request May 28, 2022

chore(frontend): improve camp dashboard performance #2763

Merged

manuelmeister reviewed May 29, 2022

View reviewed changes

manuelmeister approved these changes May 29, 2022

View reviewed changes

carlobeltrame reviewed May 31, 2022

View reviewed changes

BacLuc requested a review from pmattmann June 9, 2022 16:56

simplified ButtonNestedContentNodeAdd.vue; renamed rootContentNodes t…

eab7b42

…o allContentNodes

usu mentioned this pull request Jun 11, 2022

refactor(api)!: json data for ContentNode (new approach) #2825

Merged

10 tasks

merge devel

e286970

usu mentioned this pull request Jun 13, 2022

Format frontend projects with prettier #2769

Merged

usu added the Meeting Discuss Am nächsten Core-Meeting besprechen label Jun 13, 2022

carlobeltrame mentioned this pull request Jun 13, 2022

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

Open

pmattmann approved these changes Jun 13, 2022

View reviewed changes

usu removed the Meeting Discuss Am nächsten Core-Meeting besprechen label Jun 13, 2022

usu merged commit 5c101eb into ecamp:devel Jun 13, 2022

usu deleted the chore/api-performance-option1 branch November 6, 2022 05:50

usu mentioned this pull request Feb 14, 2023

Bad performance of /camp endpoint #2569

Closed

Uh oh!

Conversation

usu commented May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implemented

To do

Time measurement

Uh oh!

BacLuc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlobeltrame left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usu commented May 14, 2022

Uh oh!

BacLuc left a comment

usu commented May 1, 2022 •

edited

Loading