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

BUGFIX: Implement document uri path placeholders #5078

Merged
merged 10 commits into from
Jul 30, 2024

Conversation

nezaniel
Copy link
Member

@nezaniel nezaniel commented May 20, 2024

resolves #5077

This introduces the concept of placeholders for document uri paths. If the DocumentUriPathProjection encounters an unknown node type - which may happen during replay - it creates a placeholder instead of ignoring it. Placeholders are not evaluated in routing, but are transformed to actual uri paths once the respective node aggregate has been changed to type document, shortcut or site.

Upgrade instructions

needs both cr:setup and cr:projectionreplay documenturipath

Review instructions

We still need to decide on how to deal with the node type changes

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the 9.0 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

@nezaniel nezaniel marked this pull request as draft May 20, 2024 08:00
@nezaniel nezaniel requested review from bwaidelich and mhsdesign May 20, 2024 08:00
@github-actions github-actions bot added the 9.0 label May 20, 2024
@bwaidelich
Copy link
Member

Clever solution, +1 by reading.
Just 2 nitpicks, potentially as conversation starter – feel free to ignore.

One thing I wonder: IIRC we plan to store the expanded super node type names with the creation event. That might make parts of this redundant!?

@nezaniel
Copy link
Member Author

It will prevent future events from having classification unknown, yes. But sadly we cannot safely apply the SuperNodeTypes to past events because we don't know the past state anymore, so we still need to handle that case

@mhsdesign
Copy link
Member

But sadly we cannot safely apply the SuperNodeTypes to past events because we don't know the past state anymore, so we still need to handle that case

i would like to think further into that direction if its really not possible as this will prevent us from this complexity. But for that i have think a little more :D

@nezaniel
Copy link
Member Author

Proof that this cannot work:

  • have NodeTypes A and B with supertype Document
  • event 1: Create a node of type A
  • event 2: Change type to B
  • event 3: Remove type A

Now try to assign supertypes to event 1

@nezaniel
Copy link
Member Author

Also yet tbd: what to do if something is a site AND a shortcut

@nezaniel nezaniel marked this pull request as ready for review June 25, 2024 18:53
Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

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

Hi @nezaniel,

I was able to verify that your change resolves #5077.

I have one (unfortunately non-trivial) note on the code (see comments below), other than that: 👍

Regarding that note: I'm hesitant to select "Request Changes" here, because my suggestion is in a gray area between nice-to-have and probably-better-for-long-term-comprehensibility, and will require quite some work. I can't judge how relevant that is in the context of this particular projection. So, I'd be fine if you 👎 me on this one. I'll approve regardless of my suggestion in that case.

Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

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

@nezaniel and I talked about my comments above and concluded that these are possible improvements that can be taken on at a later point, and should be postponed to enable a swift beta11 release.

To ensure that the current state is not exposed as API anywhere, @nezaniel has now marked DocumentTypeClassification as @internal.

@nezaniel nezaniel merged commit 4d1ebdc into 9.0 Jul 30, 2024
10 checks passed
@nezaniel nezaniel deleted the 5077-documentUriPlaceholders branch July 30, 2024 15:58
@mhsdesign mhsdesign changed the title Implement document uri path placeholders BUGFIX: Implement document uri path placeholders Aug 3, 2024
@github-actions github-actions bot added the Bug label Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Replaying the document uri path projection can lose data when node types were changed
5 participants