ES CR: Reference properties #3849

nezaniel · 2022-07-23T16:06:45Z

This introduces references as first class citizens:

Reference{PropertyName $name, PropertyCollection $properties, Node $node}
ContentSubgraphInterface::findReferencingNodes and findReferencedNodes now return References instead of Nodes
all entry points (FlowQuery, Change models,...) have been adjusted to extract the Nodes from the References for now

Checklist

[ X ] Code follows the PSR-12 coding style
[ X ] Tests have been created, run and adjusted as needed
[ X ] The PR is created against the lowest maintained branch
Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
Reviewer - The first section explains the change briefly for change-logs
Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

skurfuerst

@nezaniel I love this feature and the change :) I have some nitpick comments / questions, which we can as well discuss via a call next week.

All the best,
Sebastian

skurfuerst · 2022-07-23T18:43:10Z

Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/GraphProjector.php

+                        'name' => $event->referenceName,
+                        'position' => $position,
+                        'nodeanchorpoint' => $nodeAnchorPoint,
+                        'destinationnodeaggregateidentifier' => $reference->targetNodeAggregateIdentifier,


should we unify the naming here or is there a reason for 2 different names?

Let's unify it, should be consistent from command to read model imho

Done, it's now source->target everywhere

skurfuerst · 2022-07-23T18:56:59Z

Neos.ContentRepository/src/Feature/NodeReferencing/Event/NodeReferencesWereSet.php

@@ -27,30 +27,30 @@ final class NodeReferencesWereSet implements
    public function __construct(
        public readonly ContentStreamIdentifier $contentStreamIdentifier,
        public readonly NodeAggregateIdentifier $sourceNodeAggregateIdentifier,
-        public readonly OriginDimensionSpacePoint $sourceOriginDimensionSpacePoint,
-        public readonly NodeAggregateIdentifiers $destinationNodeAggregateIdentifiers,
+        public readonly OriginDimensionSpacePointSet $affectedSourceOriginDimensionSpacePoints,


I don't fully understand why this now has an OriginDimensionSpacePointSet here :) -> Comment?

That's for property scopes, a comment seems to be helpful

Comment has been added

skurfuerst · 2022-07-23T19:00:03Z

Neos.ContentRepository/Tests/Behavior/Features/Bootstrap/CurrentSubgraphTrait.php

@@ -125,6 +125,7 @@ protected function getCurrentSubgraphs(): ContentSubgraphs
    {
        $currentSubgraphs = [];
        foreach ($this->getActiveContentGraphs() as $adapterName => $contentGraph) {
+            $contentGraph->disableCache();


uh, why is this needed? I am not sure this is correct actually, but it might mask actual errors during the tests.

Now that you mention it, there seem to be caching issues that should be resolved instead. Will have a look at it

skurfuerst · 2022-07-23T19:01:16Z

Neos.ContentRepository/src/Feature/Common/ConstraintChecks.php

+    ): void {
+        $nodeType = $this->nodeTypeManager->getNodeType((string)$nodeTypeName);
+        // initialize node type
+        $nodeType->getOptions();


do we still need this initialization?

Nope, we don't :)

Has been removed

skurfuerst · 2022-07-23T19:04:54Z

Neos.ContentRepository/src/Feature/NodeReferencing/NodeReferencing.php

+                    new NodeReferencesWereSet(
+                        $command->contentStreamIdentifier,
+                        $command->sourceNodeAggregateIdentifier,
+                        $affectedOrigins,


ah, now I understand it. Still I guess we should add a comment in the event what the semantics of multiple affectedOrigins are.

skurfuerst · 2022-07-23T19:06:07Z

Neos.ContentRepository/src/Infrastructure/Property/PropertyConverter.php

+                return new SerializedPropertyValue(null, (string)$propertyType);
+            }
+        } catch (PropertyTypeIsInvalid $exception) {
+            if ($exception->getCode() === 1630063406 && $propertyValue === null) {


can we somehow change this code? Especially checking for an exception code is weird here IMHO

I actually resurrected that part of code from a dead path...
Maybe we can simply remove the outer try/catch.
The original idea from $back then is that by setting a non existent property to null we would be able to unset the property altogether. That didn't survive the introduction of property checks though and has zero test coverage. So... should we just drop it? Unsetting obsolete properties is covered by integrity checks I think

The "unset properties" feature has been restored on command level, tests will follow in a later PR

@nezaniel awesome :) Thanks!

skurfuerst

GREAT :) :) :)

Bernhard Schmitt added 9 commits July 21, 2022 09:46

WIP - Prepare reference properties

c0ab2e4

Merge branch 'psr12' into referenceProperties

13ea3ba

fix code style

834a7c8

WIP: implement NodeReferencing

736eb71

Adjust code base to deal with reference properties

44b5d55

WIP - Properly de/serialize reference properties

7d852c4

Adjust property converter to deal with reference properties

4ddea43

Add missing initialization calls in NodeType

9c5b518

Remove obsolete method

647e093

skurfuerst requested changes Jul 23, 2022

View reviewed changes

skurfuerst changed the title ~~Reference properties~~ ES CR: Reference properties Jul 23, 2022

Bernhard Schmitt added 13 commits July 24, 2022 21:40

Properly handle node property unset

7b915a4

Try to json decode property values in tests

f838b53

Minor cleanup

c024e8f

Add description comment to NodeReferencesWereSet

c6f648b

Resolve caching issue in test case "create underneath disabled node"

36b794a

Minor code cleanup

58443e5

Replace deprecated doctrine method calls

6c5ec53

Fix typo in internal variable names

4186723

Fix property deserialization in tests

01d488e

Enforce the DBAL content graph to be singleton

2cd0623

Fix PropertyConverter return type

f7550bb

Rename destination to target for references in postgres

a5c823b

Properly deserialize JSON strings in tests

8eb8527

nezaniel requested review from skurfuerst, bwaidelich and kitsunet July 25, 2022 07:04

skurfuerst approved these changes Jul 25, 2022

View reviewed changes

skurfuerst merged commit 9864b2f into 9.0 Jul 25, 2022

skurfuerst deleted the referenceProperties branch July 25, 2022 07:35

Uh oh!

ES CR: Reference properties #3849

ES CR: Reference properties #3849

Uh oh!

Conversation

nezaniel commented Jul 23, 2022

Uh oh!

skurfuerst 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!

skurfuerst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!