BUGFIX: do not crash when node already disabled/enabled #4284
Conversation
How to reproduce: - User A: log into backend - User B: log into backend - User A: Hide Node X - User B: Hide Node X - User B: Publish - User A: Publish <-- here the crash has occurred during rebase. Fix idea: IMHO we can relax the checks in these command handlers a bit, to simply do nothing in this case.
|
@ahaeslich I was able to reproduce one of the errors on the demo site now :) @bwaidelich please check whether you like the solution idea; or whether you prefer a APIs in detail. Feel free to continue working here in case you want to have something changed; from my side this would be ready :) |
There was a problem hiding this comment.
Apart from the (partly nitpicky) comments and the failing tests this is a pragmatic solution that should be perfectly fine IMO.
I wonder though if we should (in the long term?) solve these kind of issues with a more sophisticated conflict resolution, i.e. avoid/merge the commands if necessary during rebase.
For the disabled case, pseudo code:
foreach ($eventsOfCurrentContentStream as $event) {
if ($event instanceof NodeAggregateWasDisabled) {
// remember disabled node
}
}
foreach ($originalCommands as $command) {
if ($command instanceof DisableNodeAggregate && $this->alreadyDisabled(...)) {
// continue
}
}
..maybe this can even be combined with #4263 ?
| PendingProjections::empty(), | ||
| new CommitResult( | ||
| Version::first(), | ||
| SequenceNumber::none() |
There was a problem hiding this comment.
This could be a bit dangerous/misleading if consumers don't check for the pending projections properly and use the CommitResult to determine the progress of the ES.
Probably not a real issue, but it might be safer to use a union type EmptyCommandResult (or similar) instead?
Neos.ContentRepository.Core/Classes/CommandHandler/PendingProjections.php
Outdated
Show resolved
Hide resolved
Neos.ContentRepository.Core/Classes/Feature/NodeDisabling/NodeDisabling.php
Show resolved
Hide resolved
|
@bwaidelich actually this change avoids storing the command during rebase; so it leads to the "cleaned up behavior". Generally I am all in for further compaction mechanisms . As I am currently not near a laptop, it would be great if we get this merged and somebody of you could take this over? :) All the best, |
|
Thanks! |
…ging Previously, when trying to disable an already disabled node, we just ignored that fact and did not produce any events. That leads to a potentially opaque behavior: ``` User A: log into backend User B: log into backend User A: Hide Node X User B: Hide Node X User B: Publish User A: Rebase User B: Unhide Node X User B: Publish User A: Rebase <--- now Node X is re-enabled even though User A had explicitly disabled it ``` With this change, the sequence is: ``` User A: log into backend User B: log into backend User A: Hide Node X User B: Hide Node X User B: Publish User A: Rebase <--- this leads to a conflict because both users disabled the same node. And User A can now explicitly decide to ignore that fact ``` Note: This mostly reverts #4284 that was introduced because we did not have a conflict resolution in place Related: #4284
How to reproduce:
Fix idea: IMHO we can relax the checks in these command handlers a bit, to simply do nothing in this case.