Skip to content

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Mar 13, 2025

A shallow look into the new dimension space point migrations reveals a (obvious) problem:

When MoveDimensionSpacePoint is run live or anywhere else, workspace will not be able to publish pending changes to live or rebase anymore because all the rebased commands will target the original dimensions which no longer exist according to the variation graph:

Rebase failed: "Dimension space point {"language":"de"} is not yet occupied by node aggregate "sir-david-nodenborough""

The documentation recommends https://docs.neos.io/guide/content-repository/content-dimensions/migrating-dimension-config

Make sure your users do not have unpublished changes

Though this is not enforced via constraint checks.


The new node migrations sometimes warranted for new commands and events because otherwise things would be impossible.
New command and events, that where introduce for that use-case:

introduced via: neos/contentrepository-development-collection#177

  • command AddDimensionShineThrough -> emits DimensionShineThroughWasAdded
  • command MoveDimensionSpacePoint -> emits DimensionSpacePointWasMoved

introduced via: #4093

  • command UpdateRootNodeAggregateDimensions -> emits RootNodeAggregateDimensionsWereUpdated

This PR changes the described behavior by adding further restrictions and the respective tests:

  • all dimension adjustments can now only be run on root workspaces or their direct children
  • MoveDimensionSpacePoint and UpdateRootNodeAggregateDimensions adjustments can no longer be run if there are any changes in any other workspace than the one they are applied to
  • MoveDimensionSpacePoint and UpdateRootNodeAggregateDimensions now fail if there are any fallbacks active that would become invalid after or removed by the change
  • all dimension adjustments now fail if the target dimension space point is already in use in another workspace

It also contains smaller necessary fixes and adjustments:

  • WorkspaceMaintenanceService::rebaseOutdatedWorkspaces now works recursively and in the correct order, properly cascading changes through the workspace graph
  • a new strategy "promisedCascade" for node type change constraint checks to be used when validating node type hierarchies while changing nodes from a non-existing to an existing type

Upgrade instructions

none

Review instructions

See introduction

Checklist

  • Code follows the PSR-12 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

@kitsunet
Copy link
Member

I am confused now, shouldn't this be a failing test?

mhsdesign and others added 18 commits March 13, 2025 17:15
…nterface` and `NodeAggregateBasedTransformationInterface`
…o work for command steps

The PR neos#5361 simplified everything into a simple

```
@when the command :shortCommandName is executed with payload:
```

step which does not provide autocompletion when writing new tests or also allows to find usages of exactly that step which speeds up filling out the required payload
…ild nodes recursively for removed dimension space points
… new specialisations for existing generalisations

This task belongs to the `AddDimensionShineThrough` command and must be run first. Using `UpdateRootNodeAggregateDimensions` would only add that dimension to the root node without any fallback which is wrong!

As shown by test 'Run migration after renaming a new dimension value'

> Cannot add fallback dimensions via update root node aggregate because node lady-eleonode-rootford already covers generalisations [{"language":"mul"}]
… generalisation where specialisation still falls-back to nodes

If we would drop the eges of a generalisation like "de" all fallback node rows would still be connected via the specialisation ("gsw") hierarchy edges but state that their occupation is "de".
This is the nature when dropping edges but not updating the node rows first.

Thus we must ensure that ALL variants must be varied first. Eg "./flow content:createvariantsrecursively" see test.
…emoved` events for root node in special dimension instead of doing so explicitly
…ubtree-tags-on-root-node-aggregate' into taks/constraint-dimension-structure-adjusments
…g dimensions

its ensured by a previous `NodeAggregateWasRemoved` that `RootNodeAggregateDimensionsWereUpdated` only signals that dimensions are ADDED.
Also mark empty dimensions as affected source, but that will be removed wither way with: neos#5516

because its wrong.
- nodes can never be created if the parent doenst cover them, this is an invariant
- the update root node aggregate constraint checks prevent creating a spezi
to allow executing multiple migrations on a workspace
@nezaniel nezaniel marked this pull request as ready for review March 18, 2025 13:54
@nezaniel nezaniel requested review from dlubitz, bwaidelich and kitsunet and removed request for dlubitz and bwaidelich March 18, 2025 14:11
@kitsunet
Copy link
Member

ok but, like, as we discussed:

Make sure your users do not have unpublished changes

Seems like an unrealistic requirement in any real world scenario?

@nezaniel
Copy link
Member

ok but, like, as we discussed:

Make sure your users do not have unpublished changes

Seems like an unrealistic requirement in any real world scenario?

well, for one, this is not a new requirement (it states the same in the documentation), it is now only enforced to prevent conflicts in workspaces that are hard to resolve (mostly by discard).
I agree that this requirement is not trivial to fulfil, but manageable if changes of this size are to be done. That being said: we should relax the constraints by only checking changes that actually conflict with the dimensionspace adjustments - in a later PR

@kitsunet kitsunet merged commit ae26ace into neos:9.0 Mar 25, 2025
12 checks passed
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