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.

Sign up for GitHub

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

Jump to bottom

BUGFIX: Ensure that its not attempted to publish 0 events #5396

Open
wants to merge 9 commits into
base: 9.0
Choose a base branch
from
Open

BUGFIX: Ensure that its not attempted to publish 0 events #5396

wants to merge 9 commits into from

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Dec 5, 2024 •
edited
Loading

followup to #5357

see #5357 (comment)

currently SetNodeProperties with 0 properties attempts to publish 0 events which throws an error since #5357

Writable events must contain at least one event

Instead we throw an exception in SetNodeProperties. We DONT expect PropertyValuesToWrite to be empty as suggested: #5357 (comment)

As there is code using PropertyValuesToWrite as builder and

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

dlubitz
dlubitz previously approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@dlubitz dlubitz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good by reading.

@mhsdesign mhsdesign changed the title BUGFIX: Asset that SetNodeProperties is not handled empty BUGFIX: Assert that SetNodeProperties is not handled empty Jan 6, 2025
@mhsdesign mhsdesign requested a review from bwaidelich January 6, 2025 12:59
@mhsdesign
BUGFIX: Asset that SetNodeProperties is not handled empty
c0c5906
@mhsdesign
TASK: Fix constraint test of failing too early
c6d093b 
> InvalidArgumentException (1733394351): The command "SetNodeProperties" for node lady-eleonode-rootford must contain property values

We want a NodeAggregateIsRoot exception
@mhsdesign
TASK: Ensure that Events are never instantiated empty
f0fc051 
by enforcing this via "non-empty-array" we can better catch runtime type errors beforehand via phpstan. As otherwise the event store will fail with

> Writable events must contain at least one event

Some internal utility methods previously also returned possibly empty "Events" instances which were then transformed via iterator_to_array back to arrays and merged. This was not simplified to just work with arrays directly to denote that they might be empty. In cases were we work with fully sure filled intermediate event arrays we use non-empty-array.

With this now fine-grained type checking aside from the original found set properties error following cases will also break with 0 events:

- handleSetSerializedNodeProperties (known)
- handleSetSerializedNodeReferences
- handlePublishWorkspace and handlePublishIndividualNodesFromWorkspace if there were no applied events that implement PublishableToWorkspaceInterface -> goes into direction of neos#5337
- structure adjustment TETHERED_NODE_WRONGLY_ORDERED
@mhsdesign mhsdesign force-pushed the bugfix/fail-early-if-set-properties-is-empty branch from ce53a0f to f0fc051 Compare January 13, 2025 16:52
@mhsdesign
Copy link
Member Author

mhsdesign commented Jan 13, 2025 •
edited
Loading

Okay before i wanted to merge this i had the feeling we might be missing more such cases ... thats why i attempted to ensure that Events are never instantiated empty via f0fc051

by enforcing this via "non-empty-array" we can better catch runtime type errors beforehand via phpstan. As otherwise the event store will fail with

Writable events must contain at least one event

Some internal utility methods previously also returned possibly empty "Events" instances which were then transformed via iterator_to_array back to arrays and merged. This was simplified to just work with arrays directly to denote that they might be empty. In cases were fully sure we work with intermediate event arrays wich are annotated via non-empty-array.

With this now fine-grained type checking aside from the original found set properties error following cases will also break with 0 events:

  • handleSetSerializedNodeProperties (known)
  • handleSetSerializedNodeReferences
  • handlePublishWorkspace and handlePublishIndividualNodesFromWorkspace if there were no applied events that implement PublishableToWorkspaceInterface -> goes into direction of !!! TASK: Throw exceptions in unnecessary rebase and publish cases #5337
  • structure adjustment TETHERED_NODE_WRONGLY_ORDERED

@mhsdesign
BUGFIX: Prevent commit error for structure adjustment TETHERED_NODE_W…
5e31760 
…RONGLY_ORDERED

EventsToPublish must not contain events with no entries

otherwise this error is thrown

> Writable events must contain at least one event

we prevent that by ensuring reorderNodes is called with at least two entries of nodes to be reordered as otherwise there is nothing to do.
@mhsdesign
BUGFIX: BUGFIX: Asset that SetNodeReferences is not handled empty
86a7d04
@mhsdesign
TASK: Add dummy typeguard to prevent phpstan from complaining
f5b3fd6 
it does not understand that the command fields are never empty ... but we ensure that
@mhsdesign
TASK: Document why Events does not exist empty
35f58a4
@mhsdesign
TASK: Ensure that publishing does not attempt to publish 0 events
6fa883c 
The event store would reject that:

> Writable events must contain at least one event
@mhsdesign mhsdesign changed the title BUGFIX: Assert that SetNodeProperties is not handled empty BUGFIX: Ensure that its not attempted to publish 0 events Jan 13, 2025
@mhsdesign mhsdesign dismissed dlubitz’s stale review January 13, 2025 20:19

much has changed :D

@mhsdesign
Copy link
Member Author

okay phpstan is happy now and helped me get all the edge cases ... so im happy too ... as this got some new lines changed im requesting a second look before merging this but this seems like a solid sane change

@mhsdesign mhsdesign requested a review from dlubitz January 17, 2025 12:01
@mhsdesign mhsdesign mentioned this pull request Jan 20, 2025
Merged
@mhsdesign mhsdesign requested a review from kitsunet January 21, 2025 16:15
@mhsdesign mhsdesign mentioned this pull request Jan 22, 2025
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwaidelich bwaidelich Awaiting requested review from bwaidelich

@dlubitz dlubitz Awaiting requested review from dlubitz

@kitsunet kitsunet Awaiting requested review from kitsunet

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
9.0 Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mhsdesign @dlubitz