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

META: CopyNodesRecursively vs Brain #5351

Closed
mhsdesign opened this issue Nov 8, 2024 · 2 comments · Fixed by #5371
Closed

META: CopyNodesRecursively vs Brain #5351

mhsdesign opened this issue Nov 8, 2024 · 2 comments · Fixed by #5371

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Nov 8, 2024

Except the big known issues with CopyNodesRecursively

I think that once you start thinking about it, it does not stop seeing problem with it :D

  • The NodeSubtreeSnapshot is NOT validated and taken for correct, imo it must not be possible for a user to do that accidentally with CopyNodesRecursively.

  • On Rebase there is NO validation of the $childNodes to insert, we just turn the serialised CopyNodesRecursively directly back into NodeAggregateWithNodeWasCreated events, defeating the sense of replaying commands partially as we kinda copy events here:

    • This is problematic because the conditions when a node tree snapshot was created might have changes:
      • The target node that was referenced by a child node might be deleted,
      • A child nodes node type might have been adjusted and the new property schema is not validated
      • ... more?
  • When copying nodes they will lose information about their subtreetags

@mhsdesign
Copy link
Member Author

In todays weekly Christian, Denny, Paula, Basti and me discussed that we should probably have a Serialised version of the command instead of adding the NodeSubtreeSnapshot to the API one.

Instead, the api one would contain:

NodeAggregateId $startNodeAggregateIdToCopy,
PropertyValuesToWrite $additionalPropertyValuesToWrite,

And we probably need to keep the NodeAggregateIdMapping on the api one for testing.

@mhsdesign

This comment was marked as duplicate.

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

Successfully merging a pull request may close this issue.

1 participant