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

TASK: Use proper version checks where applicable #5456

Open
wants to merge 1 commit into
base: 9.0
Choose a base branch
from

Conversation

bwaidelich
Copy link
Member

Replaces some ExpectedVersion::ANY() occurrences with proper versions the decisions were based on.

Note: With this change there are 8 ExpectedVersion::ANY() left in the WorkspaceCommandHandler that would require the workspace read model to keep track of the version. There are three more occurrences in CommandSimulator and ContentStreamHandling that are kept by intention

Related: #5058

Replaces some `ExpectedVersion::ANY()` occurrences with proper versions the decisions were based on.

**Note:** With this change there are 8 `ExpectedVersion::ANY()` left in the `WorkspaceCommandHandler` that would require the workspace read model to keep track of the version.
There are three more occurrences in `CommandSimulator` and `ContentStreamHandling` that are kept by intention

Related: #5058
@mhsdesign
Copy link
Member

For the following cases in the WorkspaceCommandHandler ExpectedVersion::ANY() is used while we could change that at least to ExpectedVersion::STREAM_EXISTS()? For most cases - because we issue multiple events - we dont want this check to ever fail, and it wont because the stream will continue to exist, even if someone deletes the workspace at the same time - that we have to prevent actually :D.

  • handlePublishWorkspace
  • handleRebaseWorkspace
  • handlePublishIndividualNodesFromWorkspace
  • handleDiscardIndividualNodesFromWorkspace
  • discardWorkspace
  • handleChangeBaseWorkspace
  • handleDeleteWorkspace

Further i found a few missing ExpectedVersion::ANY() checks in the structure adjustments - but i think we dont have access to the content stream version here ...

@@ -148,7 +148,7 @@ private function handleCreateWorkspace(
$command->newContentStreamId,
)
),
ExpectedVersion::ANY()
Copy link
Member

Choose a reason for hiding this comment

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

this slight change has a big impact as now after deleting a workspace a new one with the exact same name cannot be recreated.

And because \Neos\Neos\Domain\Service\WorkspaceService::getUniqueWorkspaceName only prevents naming conflict with existing workspaces that are projected attempting to create a workspace with the same title via Ui fails.

steps:

  1. create workspace 'MyTest'
  2. delete workspace
  3. repeat
image

The error is as expected:

Workspace could not be created.
Expected version: -1 [no stream], actual version: 1

That means imo that getUniqueWorkspaceName needs to handle this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

very good catch

Copy link
Member

Choose a reason for hiding this comment

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

So this seems to hve no test then?

Copy link
Member

Choose a reason for hiding this comment

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

jup the the core's DeleteWorkspace is fully untested - i have that somewhere in my todo list and stumble upon that every once and then. Also the cores CreateWorkspace is only tested implicit but not for such cases.

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