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

FEATURE: Introduce Workspace::hasPublishableChanges #5332

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Oct 31, 2024

Introduces Workspace::hasPublishableChanges which will be used for following cases:

  • to make Discard a noop (instead of looking into the event stream)
  • to make ChangeBaseWorkspace fail if the workspace has changes (instead of looking into the event stream)
  • to make DeleteWorkspace fail if the workspace has changes (future pr)
  • for node migrations as it should be standalone package and NOT use the change finder (link)

Also its good to have some rudimentary changes concept already low level which will hopefully improved at somepoint in Neos X by diffing two content graphs or something.

Note also that the hasPublishableChanges will also return true for the case a NodeMigration was run on the workspace and created events like DimensionShineThroughWasAdded which are in fact publish able (which is NOT detected via the change projection !!!).

Implementation:

We decided for a simple hasChanges boolean flag on the content stream for now which will be set if there is at least one event that is considered a change: PublishableToWorkspaceInterface.
Technically an increment to count the events that are PublishableToWorkspaceInterface could also work but as there is no grouping of events per node aggregate id it would be hard to reason about the number and its meaning (it would be completely different to the change projections count)

We leverage the PublishableToWorkspaceInterface to ignore events like forking, closing and reopening, as they are not published to another stream.

Upgrade instructions

Review instructions

Checklist

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

@bwaidelich
Copy link
Member

bwaidelich commented Oct 31, 2024

@mhsdesign wrote in slack
But instead of having a boolean flag i rather have already some simple incrementing count no? That would allow us to get rid of usages of the change projection like when deleting workspaces and saying how many nodes are in the workspace:
publishableEvents=publishableEvents+1
To be true a PublishableToWorkspaceInterface does NOT equal a change in the change projection as multiple set properties are deduplicated but i think it would still be a good think to have this low level count available.
The workspace would get a method hasPublishableChanges (wich never true for root workspaces) and maybe something countPublishableChanges though we should probably refrain from the whole changes term here as its already claimed by the change projection.
For example a DimensionShineThroughWasAdded event is also a change in terms for the workspace model but NOT respected by the change projection which only handles nodes.
Maybe we should use hasPublishableEvents and countPublishableEvents to be fully distinct?

One note regarding count vs flag:
A counter would only replace the change projection if we kept an individual count per type of edit (create, update, delete) per document node which would require a separate table.
For the time being I would go with the simple boolean flag and expose only hasPublishableChanges() on the workspace (since that can always be derived from the separated changes if we track them at some point).
Or do we need Workspace::countPublishableChanges() for anything else?

@mhsdesign mhsdesign force-pushed the feature/workspace-has-changes-flag branch from c775b11 to 82daa7c Compare October 31, 2024 17:03
@mhsdesign mhsdesign marked this pull request as ready for review October 31, 2024 17:03
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!
Except for the implicit no-ops for invalid publish/discards, but you wanted to change that in a follow-up IIRC

@mhsdesign mhsdesign changed the title FEATURE: Introduce publishableEvents count on content stream FEATURE: Introduce Workspace::hasPublishableChanges Oct 31, 2024
... because it would lead to hard to debug code and is not expected to fail
@mhsdesign mhsdesign force-pushed the feature/workspace-has-changes-flag branch from 82daa7c to 489db4c Compare October 31, 2024 19:06
@mhsdesign
Copy link
Member Author

Removed the unrelated rabbit hole changes into a new pr: #5333

@mhsdesign
Copy link
Member Author

we discussed in the weekly today that we want name the column hasChanges instead and merge it :)

@kitsunet
Copy link
Member

kitsunet commented Nov 1, 2024

interesting test failure because the test doesn#t say anytghing about no changes....

@kitsunet
Copy link
Member

kitsunet commented Nov 1, 2024

you know what, not following my own advice here. This has not too much to do with this change, I will revert my changes and merge this. WE can throw later... (as these changes are now gone here, I added throws for the no ops)

@kitsunet kitsunet force-pushed the feature/workspace-has-changes-flag branch from 110020d to 2e5eb47 Compare November 1, 2024 12:55
@kitsunet kitsunet merged commit 67faf46 into neos:9.0 Nov 1, 2024
17 checks passed
@mhsdesign mhsdesign deleted the feature/workspace-has-changes-flag branch November 1, 2024 13:05
@mhsdesign
Copy link
Member Author

jup thanks for attempting the change though... ill continue it in #5337 for beta16 :)

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.

3 participants