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

BUGFIX: Fix and test tethered node structure adjustment #4969

Merged

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Mar 30, 2024

I found out that this line

is totally untested. Thus i wanted to write a test.

This pr also adds causation ids and meta data for changes made by structure adjustments to easier debug them. See also #3887 -> Moved to a dedicated pr in #5451

Related: #4966
Related: #4832 (comment)

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

@mhsdesign
Copy link
Member Author

Currently fails with

RuntimeException: Failed to update and commit highest applied sequence number for subscriber "Neos\ContentGraph\DoctrineDbalAdapter\DoctrineDbalContentGraphProjection". Please run Neos\ContentRepository\Core\Infrastructure\DbalCheckpointStorage::setUp() in /Users/marchenryschultz/Code/core/neos-manufacture-90/Packages/Neos/Neos.ContentRepository.Core/Classes/Infrastructure/DbalCheckpointStorage.php:125

this is due to the blocking in $this->eventPersister->publishEvents. If we dont catchup everything is fine. So a projection problem?

break;
}
/** @var Node $childNodeSource Node aggregates are never empty */
$occupiedDimensionSpacePoints = $childNodeAggregate->occupiedDimensionSpacePoints->getPoints();
Copy link
Member Author

Choose a reason for hiding this comment

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

idk if we should guard here with this check ... the added tests will pass either way ;)

if (count($occupiedDimensionSpacePoints) !== 1) {
    throw new \RuntimeException('TODO: The ChildNodeAggregate occupies multiple origins.', 1711897662);
}

Copy link
Member

Choose a reason for hiding this comment

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

I don't see an issue with the child node aggregate occupying multiple DSPs, that's totally legit

@mhsdesign mhsdesign requested a review from nezaniel April 2, 2024 13:29
@mhsdesign mhsdesign changed the title TASK: Write test for tethered node structure adjustment BUGFIX: Fix and test tethered node structure adjustment Apr 3, 2024
@github-actions github-actions bot added the Bug label Apr 3, 2024
Copy link
Member

@nezaniel nezaniel left a comment

Choose a reason for hiding this comment

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

Needs to be synced with 9.0, looks otherwise fine so far

break;
}
/** @var Node $childNodeSource Node aggregates are never empty */
$occupiedDimensionSpacePoints = $childNodeAggregate->occupiedDimensionSpacePoints->getPoints();
Copy link
Member

Choose a reason for hiding this comment

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

I don't see an issue with the child node aggregate occupying multiple DSPs, that's totally legit

@mhsdesign
Copy link
Member Author

@mhsdesign mhsdesign requested a review from nezaniel April 27, 2024 08:56
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.

Just some initial thoughts on correlation and causation ids, happy to discuss this further!

@mhsdesign mhsdesign force-pushed the task/writeTestForTetheredNodeStructureAdjustment branch from 1e97147 to 07f429a Compare May 13, 2024 18:51
@mhsdesign mhsdesign marked this pull request as draft May 13, 2024 18:52
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Jan 26, 2025
…events

Applies suggestion originally posted in neos#4969 (comment)

the resulting event log would be similar to:

| event                           | correlation id | metadata                                                                                                                                   |
|---------------------------------|----------------|--------------------------------------------------------------------------------------------------------------------------------------------|
| NodeAggregateWithNodeWasCreated | 123456         | `{structureAdjustment: 'Content Stream: %s; Dimension Space Point: %s, Node Aggregate: %s --- The tethered child node "bar" is missing.'}` |
| NodePropertiesWereSet           | 123456         |                                                                                                                                            |
@mhsdesign mhsdesign force-pushed the task/writeTestForTetheredNodeStructureAdjustment branch from 07f429a to c0e759a Compare January 27, 2025 11:24
see `peerSucceedingSiblings` change
@mhsdesign mhsdesign force-pushed the task/writeTestForTetheredNodeStructureAdjustment branch from c0e759a to 554a95e Compare January 27, 2025 11:42
@mhsdesign mhsdesign marked this pull request as ready for review January 27, 2025 11:43
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Jan 27, 2025
…events

Applies suggestion originally posted in neos#4969 (comment)

the resulting event log would be similar to:

| event                           | correlation id | metadata                                                                                                                                   |
|---------------------------------|----------------|--------------------------------------------------------------------------------------------------------------------------------------------|
| NodeAggregateWithNodeWasCreated | 123456         | `{structureAdjustment: 'Content Stream: %s; Dimension Space Point: %s, Node Aggregate: %s --- The tethered child node "bar" is missing.'}` |
| NodePropertiesWereSet           | 123456         |                                                                                                                                            |
@mhsdesign mhsdesign requested a review from kitsunet January 28, 2025 09:47
neos-bot pushed a commit to neos/contentrepository-structureadjustment that referenced this pull request Jan 28, 2025
…events

Applies suggestion originally posted in neos/neos-development-collection#4969 (comment)

the resulting event log would be similar to:

| event                           | correlation id | metadata                                                                                                                                   |
|---------------------------------|----------------|--------------------------------------------------------------------------------------------------------------------------------------------|
| NodeAggregateWithNodeWasCreated | 123456         | `{structureAdjustment: 'Content Stream: %s; Dimension Space Point: %s, Node Aggregate: %s --- The tethered child node "bar" is missing.'}` |
| NodePropertiesWereSet           | 123456         |                                                                                                                                            |
Copy link
Member

@nezaniel nezaniel left a comment

Choose a reason for hiding this comment

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

now up-to-date with 9.0 again and tests are green.

lgtm

@mhsdesign mhsdesign merged commit 4142aa4 into neos:9.0 Feb 20, 2025
12 checks passed
@mhsdesign mhsdesign deleted the task/writeTestForTetheredNodeStructureAdjustment branch February 20, 2025 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants