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

NodeAggregateNameWasChanged does not create copy on write #4150

Closed
bwaidelich opened this issue Mar 31, 2023 · 6 comments · Fixed by #5018
Closed

NodeAggregateNameWasChanged does not create copy on write #4150

bwaidelich opened this issue Mar 31, 2023 · 6 comments · Fixed by #5018
Assignees
Milestone

Comments

@bwaidelich
Copy link
Member

bwaidelich commented Mar 31, 2023

The DoctrineDbalContentGraphProjection directly updates the hierarchyrelations without creating copies of the affected nodes first.

Edit 12th April 2024:
One of the issues is that the node timestamps are not updated when its name changed.
We decided to tackle this because it would require a projection replay if changed lateron.

@bwaidelich bwaidelich converted this from a draft issue Mar 31, 2023
@bwaidelich bwaidelich added this to the 9.0 (ES CR) milestone Mar 31, 2023
@skurfuerst skurfuerst moved this from Prioritized to In Progress in Neos 9.0 Release Board Apr 2, 2023
@ahaeslich ahaeslich moved this from In Progress 🚧 to Prioritized 🔥 in Neos 9.0 Release Board Nov 10, 2023
@skurfuerst skurfuerst removed their assignment Feb 23, 2024
@nezaniel nezaniel self-assigned this Mar 13, 2024
@nezaniel
Copy link
Member

Just to clarify so that I can write appropriate tests: What exactly are the issues here?
The content graph integrity should be intact, but there might be workspace issues?

@bwaidelich
Copy link
Member Author

sorry.. what a bad bug description of mine..
And, worse, I can't remember the steps to reproduce / expected vs actual behavior..

I think the issue was, that the aggregate name is always changed in all variants

@mhsdesign
Copy link
Member

maybe somehow related to #4942?

@nezaniel
Copy link
Member

Node names are currently aggregate scoped, so name changes across variants are by design.
We should trigger a copy on write anyway though; This probably currently doesn't happen because names are written to the hierarchy relations

@nezaniel nezaniel moved this from Prioritized 🔥 to In Progress 🚧 in Neos 9.0 Release Board Apr 26, 2024
@nezaniel
Copy link
Member

In preparation of this, I stumbled upon an issue.

Currently we have ContentGraphInterface::findChildNodeAggregatesByName which explicitly states that there might be multiple of those.
Then we have e.g. TetheredNodeInternals::createEventsForMissingTetheredNode which throws a RuntimeException in that exact case.

We have to come to a decision whether we allow multiple child node aggregates with the same name - as long as they do not share dimension space points - or not.

This can be shown with given example:

Image

Here, we no longer can translate nody-mc-nodeface to violet as its name is already covered by evil-occupier because it takes precedence by having the same name. While evil-occupier might actually take the same role, this defeats the idea of node aggregates aggregating all variants of the same content.

Thus, I'd opt for enforcing unique child names on aggregate level, which would prevent us to create or move evil-occupier there in the first place. wdyt?

@bwaidelich
Copy link
Member Author

bwaidelich commented Apr 27, 2024

I'd opt for enforcing unique child names on aggregate level

I agree: if we're not 100% sure, we should take the defensive route (i.e. enforce unique child names on aggregate level) since it's easier to relax that constraint if it turns out to yield issues.
Also I think, addressing a tethered node by its name is similar to addressing a node by it's id – which can't be different per variant either

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants