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: Serializable Commands #5348

Merged
merged 21 commits into from
Nov 12, 2024
Merged

!!! TASK: Serializable Commands #5348

merged 21 commits into from
Nov 12, 2024

Conversation

bwaidelich
Copy link
Member

@bwaidelich bwaidelich commented Nov 7, 2024

Turns the low-level commands CreateNodeAggregateWithNodeAndSerializedProperties, SetSerializedNodeProperties and SetSerializedNodeReferences into implementations of a new SerializedCommandInterface in order to prevent them from being dispatched "from the outside".

This is a breaking change in case that someone instantiated and sent one of those lowlevel (and @internal) commands to ContentRepository::handle(). In that case the corresponding high level command should be used instead.

Related: #5353

Neos ui neos/neos-ui#3881

@github-actions github-actions bot added the 9.0 label Nov 7, 2024
@bwaidelich
Copy link
Member Author

With the current WIP tests will fail because of the TransformationsFactory working on the ContentRepository instance (solely to call handle() and in one case to access the content graph). We could probably fix that by passing it the CommandBus instead such that it can trigger those low level commands (or maybe don't use commands but events for node migrations.. but that's a bigger rabbit hole I guess)..

But basically the PoC works with Neos

@mhsdesign
Copy link
Member

Thanks, the idea is also that we centralise RebaseableCommand::enrichEvents which would be a real gain as this is prone to be wrong:

#5301 (comment)

Alright, at least according to the tests this works now. I also went through all Rebasable commands to check if the events get enriched. the Dimension ones were the only missing. IMHO we should centralize this behavior, I opted against doing it here though as I think we need to consider if there would have to be any more logic involved to decide what gets enriched with commands, in what way, when we override the command if there is already metadata and finally what the causation ids are. I guess we could ignore all these questions and centralize it, but it warrants a closer look and is therefore out of scope of this change.

@kitsunet
Copy link
Member

kitsunet commented Nov 7, 2024

Interesting idea, probably the right direction. i am not madly in love with the way the interfaces interact though... This is really .... something:

(self&CommandInterface)|(self&SerializedCommandInterface);

But I have no great alternative atm...

@mhsdesign
Copy link
Member

maybe using @return $this to "fake" it for phpstan? And just using self in php? THe union is crazy :D

@bwaidelich
Copy link
Member Author

i am not madly in love with the way the interfaces interact though

I agree, it's at least uncommon. But it just affects very few very internal parts of the code – does it really matter?

@bwaidelich
Copy link
Member Author

There are 5 NodeMigration implementations that use SetSerializedNodeProperties – it would be interesting to find out why they shouldn't use the "official" commands.
For the RemoveProperty, RenameProperty, StripTagsOnProperty transformations it should be easily possible to use the "official" SetNodeProperties command.
But AddNewProperty and ChangePropertyValue expect serialized property values in the transformation and there is even a test with an example for adding a property that is not defined in the schema – I'm not sure what the usecase is for that..

In any case: If we need to keep it like that but want to make SetSerializedNodeProperties an internal concept, we can either expose the CommandBus to the transformations (but that would make it a new service factory dependency and.. uh..) or just pass the PropertyConverter to the transformations such that they can serialize node properties themselves..
However that would still not allow a transformation to set a node property that is not defined (or invalid) according to the schema

@bwaidelich bwaidelich changed the title WIP: PoC: Serializable Commands !!! TASK: Serializable Commands Nov 8, 2024
@bwaidelich bwaidelich marked this pull request as ready for review November 8, 2024 15:17
@bwaidelich
Copy link
Member Author

@mhsdesign @kitsunet Thanks a lot for your continuous efforts!
I have to say I lost track of the discussions in this and related PRs. So allow me to summarize how I see the issues and especially the blockers for a 9.0 (and please let me know if I miss something important – it's quite possible):

Recap

During a rebase (i.e when I sync changes from a target workspace with new changes to my workspace that also has pending changes) we currently re-apply the original commands to find out wether my pending changes are in conflict with the synced changes.

But because there are some non-idempotent commands we introduced lower-level commands for those that contain "serialized" information about the state of the system at the time of the initial command handling in order to make it (more) idempotent (e.g.: the CreateNodeAggregateWithNodeAndSerializedProperties command contains the explicitly set + the default property values of the node type so that they are consistent even if the node type schema changed)

Blocker

The main issue with that is: Now users can send those low-level commands directly to the CR, skipping some constraint checks (and making the API more complex than it'd need to be).
Furthermore, the low-level commands would/should not trigger Command Hooks because they are sent to the handlers directly, creating yet another implicit divide between the two types of commands.

That is the issue, this PR tries to solve by making the low-level commands an internal concept.

Discussions

I'm happy to fine-tune the namings & types of course, but personally I don't care about those (now called) serialized commands because they are now just an internal thing and nobody should have to deal with them (apart from the core) – or am I missing something here?

Further thoughts

Some more ideas that should not distract us from the important issues, but maybe help to find better in-between-solutions:

I looked at the events and realized that we store the FQN of the commandClass in the metadata.
We should avoid that for the same reason not to store the FQN of the event class names.
Also commandClass is not necessarily the right wording. It is more like: A "snapshot" of information that is needed to re-evaluate constraints.

And, a little more controversial maybe:

The information for the "command" is mostly already contained in the corresponding event. So I wonder why we decided to store information about the command in the metadata.
E.g. for an event of NodePropertiesWereSet the corresponding commandClass will always be SetSerializedNodeProperties so we would not have to store that in the events (let alone the FQN).
Also the command payload should be extractable from the event payload in most cases (and where it is not we could add it to the event because it was apparently missing some information).

As a result we would completely get rid of the command metadata hack and instead re-apply events + checking (some) constraints.

TASK: Remove `SerializedCommandInterface` again and use `RebasableToO…
@bwaidelich
Copy link
Member Author

bwaidelich commented Nov 9, 2024

As discussed with @mhsdesign and @kitsunet

A result of this PR is that the "lowlevel commands" are now completely internal and should not be exposed to the userland at all (that's a good thing IMO).

As a result, we need to tweak the CommandThatFailedDuringRebase construct that was introduced to render details about a conflict in the Neos UI because that previously exposed those.
Instead we decided to expose the events that led to a conflict (for now. A cleaner solution might be to expose the affected node and some higher level "type of change" – but that would require yet another concept and should be thought through carefully)

  • rename CommandsThatFailedDuringRebase to EventsThatFailedDuringRebase
  • rename CommandThatFailedDuringRebase to EventThatFailedDuringRebase and change its signature from
final readonly class CommandThatFailedDuringRebase
{
    public function __construct(
        public CommandInterface $command,
        public \Throwable $exception,
        private SequenceNumber $sequenceNumber,
    ) {
    }

    public function getSequenceNumber(): SequenceNumber;
}

to

final readonly class CommandThatFailedDuringRebase
{
    public function __construct(
        private EventInterface $event,
        private \Throwable $exception,
        private SequenceNumber $sequenceNumber,
    ) {
    }

    /** @internal **/
    public function getEvent(): EventInterface;
    public function getException(): \Throwable;
    public function getSequenceNumber(): SequenceNumber;
}

...and change the UI code accordingly

@@ -64,7 +66,7 @@ Feature: Add New Property
-
type: 'AddNewProperty'
settings:
newPropertyName: 'aDateOutsideSchema'
Copy link
Member Author

Choose a reason for hiding this comment

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

@skurfuerst you had originally introduced this test case with 0f88ed6 but in our opinion node migrations should not be allowed to set properties that are not allowed according to the node type schema. Do you agree to that assumption?

This is an implementation detail, and we might change this concept and rather do constraint checks for events on the read model and then publish those.
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Thank you both for polishing this so well :)

@bwaidelich
Copy link
Member Author

I'm happy with this one now, but I can't +1 it. @mhsdesign @kitsunet shall we merge this?

@mhsdesign
Copy link
Member

@bwaidelich please have a look how i handled things in the Neos ui now neos/neos-ui#3881

Chances that its okay to just be interested in the first dsp are low :D
bwaidelich and others added 6 commits November 12, 2024 12:15
# Conflicts:
#	Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/02-RebasingWithAutoCreatedNodes.feature
#	Neos.ContentRepository.Core/Classes/CommandHandler/CommandInterface.php
#	Neos.ContentRepository.Core/Classes/Feature/NodeModification/Command/SetNodeProperties.php
#	Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php
Copy link
Member

Choose a reason for hiding this comment

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

we discussed that for the case we would want to replay events actually that also a succeeding event might appear here. Currently only the initial event will be available via $event, but we could still later decide to add an additional events list here.
If we were to replay events we can not allow only one to pass. All correlated must pass i assume (as otherwise tethered nodes could be missing), so the first event - if really the part of the conflict or not - will always be $event.

@mhsdesign mhsdesign disabled auto-merge November 12, 2024 11:44
@mhsdesign mhsdesign merged commit 7a5a7c4 into 9.0 Nov 12, 2024
7 of 9 checks passed
@mhsdesign mhsdesign deleted the task/serializable-commands branch November 12, 2024 11:45
@mhsdesign
Copy link
Member

Fyi just for completeness, there is a todo comment in the Ui do to handle serialised commands directly:

https://github.com/neos/neos-ui/blob/c4dcecce0722748c6fe40a164ceb83c837d309d7/Classes/Domain/Service/NodePropertyConverterService.php#L39

but that doesnt seem like a good idea - especially not since its now impossible, will update that todo.

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

Successfully merging this pull request may close these issues.

3 participants