Skip to content

Commit

Permalink
BUGFIX: Simplify change projection to mark aggregate changes as chang…
Browse files Browse the repository at this point in the history 
…e in all occupied dimensions
  • Loading branch information
@mhsdesign
mhsdesign committed Dec 5, 2024
1 parent 0337e96 commit 97f36d2
Show file tree
Hide file tree
Showing 9 changed files with 43 additions and 116 deletions.
42 changes: 12 additions & 30 deletions Neos.Neos/Classes/Domain/Service/WorkspacePublishingService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,38 +387,20 @@ private function isChangePublishableWithinAncestorScope(
}
}

if ($change->originDimensionSpacePoint) {
$subgraph = $contentRepository->getContentGraph($workspaceName)->getSubgraph(
$change->originDimensionSpacePoint->toDimensionSpacePoint(),
VisibilityConstraints::withoutRestrictions()
);

// A Change is publishable if the respective node (or the respective
// removal attachment point) has a closest ancestor that matches our
// current ancestor scope (Document/Site)
$actualAncestorNode = $subgraph->findClosestNode(
$change->removalAttachmentPoint ?? $change->nodeAggregateId,
FindClosestNodeFilter::create(nodeTypes: $ancestorNodeTypeName->value)
);

return $actualAncestorNode?->aggregateId->equals($ancestorId) ?? false;
} else {
return $this->findAncestorAggregateIds(
$contentRepository->getContentGraph($workspaceName),
$change->nodeAggregateId
)->contain($ancestorId);
}
}
$subgraph = $contentRepository->getContentGraph($workspaceName)->getSubgraph(
$change->originDimensionSpacePoint->toDimensionSpacePoint(),
VisibilityConstraints::withoutRestrictions()
);

private function findAncestorAggregateIds(ContentGraphInterface $contentGraph, NodeAggregateId $descendantNodeAggregateId): NodeAggregateIds
{
$nodeAggregateIds = NodeAggregateIds::create($descendantNodeAggregateId);
foreach ($contentGraph->findParentNodeAggregates($descendantNodeAggregateId) as $parentNodeAggregate) {
$nodeAggregateIds = $nodeAggregateIds->merge(NodeAggregateIds::create($parentNodeAggregate->nodeAggregateId));
$nodeAggregateIds = $nodeAggregateIds->merge($this->findAncestorAggregateIds($contentGraph, $parentNodeAggregate->nodeAggregateId));
}
// A Change is publishable if the respective node (or the respective
// removal attachment point) has a closest ancestor that matches our
// current ancestor scope (Document/Site)
$actualAncestorNode = $subgraph->findClosestNode(
$change->removalAttachmentPoint ?? $change->nodeAggregateId,
FindClosestNodeFilter::create(nodeTypes: $ancestorNodeTypeName->value)
);

return $nodeAggregateIds;
return $actualAncestorNode?->aggregateId->equals($ancestorId) ?? false;
}

/**
Expand Down
17 changes: 6 additions & 11 deletions Neos.Neos/Classes/PendingChangesProjection/Change.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,13 @@
*/
final class Change
{
public const AGGREGATE_DIMENSIONSPACEPOINT_HASH_PLACEHOLDER = 'AGGREGATE';

/**
* @param NodeAggregateId|null $removalAttachmentPoint {@see RemoveNodeAggregate::$removalAttachmentPoint} for docs
*/
public function __construct(
public ContentStreamId $contentStreamId,
public NodeAggregateId $nodeAggregateId,
// null for aggregate scoped changes (e.g. NodeAggregateNameWasChanged, NodeAggregateTypeWasChanged)
public ?OriginDimensionSpacePoint $originDimensionSpacePoint,
public OriginDimensionSpacePoint $originDimensionSpacePoint,
public bool $created,
public bool $changed,
public bool $moved,
Expand All @@ -58,8 +55,8 @@ public function addToDatabase(Connection $databaseConnection, string $tableName)
$databaseConnection->insert($tableName, [
'contentStreamId' => $this->contentStreamId->value,
'nodeAggregateId' => $this->nodeAggregateId->value,
'originDimensionSpacePoint' => $this->originDimensionSpacePoint?->toJson(),
'originDimensionSpacePointHash' => $this->originDimensionSpacePoint?->hash ?: self::AGGREGATE_DIMENSIONSPACEPOINT_HASH_PLACEHOLDER,
'originDimensionSpacePoint' => $this->originDimensionSpacePoint->toJson(),
'originDimensionSpacePointHash' => $this->originDimensionSpacePoint->hash,
'created' => (int)$this->created,
'changed' => (int)$this->changed,
'moved' => (int)$this->moved,
Expand All @@ -86,8 +83,8 @@ public function updateToDatabase(Connection $databaseConnection, string $tableNa
[
'contentStreamId' => $this->contentStreamId->value,
'nodeAggregateId' => $this->nodeAggregateId->value,
'originDimensionSpacePoint' => $this->originDimensionSpacePoint?->toJson(),
'originDimensionSpacePointHash' => $this->originDimensionSpacePoint?->hash ?: self::AGGREGATE_DIMENSIONSPACEPOINT_HASH_PLACEHOLDER,
'originDimensionSpacePoint' => $this->originDimensionSpacePoint->toJson(),
'originDimensionSpacePointHash' => $this->originDimensionSpacePoint->hash,
]
);
} catch (DbalException $e) {
Expand All @@ -103,9 +100,7 @@ public static function fromDatabaseRow(array $databaseRow): self
return new self(
ContentStreamId::fromString($databaseRow['contentStreamId']),
NodeAggregateId::fromString($databaseRow['nodeAggregateId']),
$databaseRow['originDimensionSpacePoint'] ?? null
? OriginDimensionSpacePoint::fromJsonString($databaseRow['originDimensionSpacePoint'])
: null,
OriginDimensionSpacePoint::fromJsonString($databaseRow['originDimensionSpacePoint']),
(bool)$databaseRow['created'],
(bool)$databaseRow['changed'],
(bool)$databaseRow['moved'],
Expand Down
71 changes: 14 additions & 57 deletions Neos.Neos/Classes/PendingChangesProjection/ChangeProjection.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -415,21 +415,27 @@ private function whenNodeAggregateTypeWasChanged(NodeAggregateTypeWasChanged $ev
if ($event->workspaceName->isLive()) {
return;
}
$this->markAggregateAsChanged(
$event->contentStreamId,
$event->nodeAggregateId,
);
foreach ($event->affectedOriginDimensionSpacePoints as $originDimensionSpacePoint) {
$this->markAsChanged(
$event->contentStreamId,
$event->nodeAggregateId,
$originDimensionSpacePoint
);
}
}

private function whenNodeAggregateNameWasChanged(NodeAggregateNameWasChanged $event): void
{
if ($event->workspaceName->isLive()) {
return;
}
$this->markAggregateAsChanged(
$event->contentStreamId,
$event->nodeAggregateId,
);
foreach ($event->affectedOriginDimensionSpacePoints as $originDimensionSpacePoint) {
$this->markAsChanged(
$event->contentStreamId,
$event->nodeAggregateId,
$originDimensionSpacePoint
);
}
}

private function whenContentStreamWasRemoved(ContentStreamWasRemoved $event): void
Expand All @@ -452,19 +458,6 @@ static function (Change $change) {
);
}

private function markAggregateAsChanged(
ContentStreamId $contentStreamId,
NodeAggregateId $nodeAggregateId,
): void {
$this->modifyChangeForAggregate(
$contentStreamId,
$nodeAggregateId,
static function (Change $change) {
$change->changed = true;
}
);
}

private function markAsCreated(
ContentStreamId $contentStreamId,
NodeAggregateId $nodeAggregateId,
Expand Down Expand Up @@ -514,23 +507,6 @@ private function modifyChange(
}
}

private function modifyChangeForAggregate(
ContentStreamId $contentStreamId,
NodeAggregateId $nodeAggregateId,
callable $modifyFn
): void {
$change = $this->getChangeForAggregate($contentStreamId, $nodeAggregateId);

if ($change === null) {
$change = new Change($contentStreamId, $nodeAggregateId, null, false, false, false, false);
$modifyFn($change);
$change->addToDatabase($this->dbal, $this->tableNamePrefix);
} else {
$modifyFn($change);
$change->updateToDatabase($this->dbal, $this->tableNamePrefix);
}
}

private function getChange(
ContentStreamId $contentStreamId,
NodeAggregateId $nodeAggregateId,
Expand All @@ -552,25 +528,6 @@ private function getChange(
return $changeRow ? Change::fromDatabaseRow($changeRow) : null;
}

private function getChangeForAggregate(
ContentStreamId $contentStreamId,
NodeAggregateId $nodeAggregateId,
): ?Change {
$changeRow = $this->dbal->executeQuery(
'SELECT n.* FROM ' . $this->tableNamePrefix . ' n
WHERE n.contentStreamId = :contentStreamId
AND n.nodeAggregateId = :nodeAggregateId
AND n.origindimensionspacepointhash = :origindimensionspacepointhash',
[
'contentStreamId' => $contentStreamId->value,
'nodeAggregateId' => $nodeAggregateId->value,
'origindimensionspacepointhash' => Change::AGGREGATE_DIMENSIONSPACEPOINT_HASH_PLACEHOLDER
]
)->fetchAssociative();

return $changeRow ? Change::fromDatabaseRow($changeRow) : null;
}

private function removeChangesForContentStreamId(ContentStreamId $contentStreamId): void
{
$statement = <<<SQL
Expand Down
6 changes: 1 addition & 5 deletions Neos.Neos/Tests/Behavior/Features/Bootstrap/ChangeProjectionTrait.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,7 @@ public function iExpectTheChangeProjectionToHaveTheFollowingChangesInContentStre
|| $change->deleted !== (bool)$tableRow['deleted']
|| $change->changed !== (bool)$tableRow['changed']
|| $change->moved !== (bool)$tableRow['moved']
|| (
($change->originDimensionSpacePoint === null && strtolower($tableRow['originDimensionSpacePoint']) !== "null")
&&
($change->originDimensionSpacePoint !== null && strtolower($tableRow['originDimensionSpacePoint']) !== "null" && !$change->originDimensionSpacePoint->equals(DimensionSpacePoint::fromJsonString($tableRow['originDimensionSpacePoint'])))
)
|| !$change->originDimensionSpacePoint->equals(DimensionSpacePoint::fromJsonString($tableRow['originDimensionSpacePoint']))
) {
continue;
}
Expand Down
3 changes: 1 addition & 2 deletions ...tures/PendingChanges/08-NodeRenaming/01-ChangeNodeAggregateName_WithoutDimensions.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ Feature: Change node aggregate name without dimensions

Then I expect the ChangeProjection to have the following changes in "user-cs-id":
| nodeAggregateId | created | changed | moved | deleted | originDimensionSpacePoint |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | null |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {} |
And I expect the ChangeProjection to have no changes in "cs-identifier"

Scenario: Change the node aggregate name with already applied changes
Expand All @@ -74,6 +74,5 @@ Feature: Change node aggregate name without dimensions

Then I expect the ChangeProjection to have the following changes in "user-cs-id":
| nodeAggregateId | created | changed | moved | deleted | originDimensionSpacePoint |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | null |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {} |
And I expect the ChangeProjection to have no changes in "cs-identifier"
5 changes: 3 additions & 2 deletions ...Features/PendingChanges/08-NodeRenaming/02-ChangeNodeAggregateName_WithDimensions.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ Feature: Change node aggregate name with dimensions

Then I expect the ChangeProjection to have the following changes in "user-cs-id":
| nodeAggregateId | created | changed | moved | deleted | originDimensionSpacePoint |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | null |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {"language":"de"} |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {"language":"fr"} |
And I expect the ChangeProjection to have no changes in "cs-identifier"

Scenario: Change the node aggregate type with already applied changes
Expand All @@ -87,6 +88,6 @@ Feature: Change node aggregate name with dimensions

Then I expect the ChangeProjection to have the following changes in "user-cs-id":
| nodeAggregateId | created | changed | moved | deleted | originDimensionSpacePoint |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | null |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {"language":"de"} |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {"language":"fr"} |
And I expect the ChangeProjection to have no changes in "cs-identifier"
3 changes: 1 addition & 2 deletions ...res/PendingChanges/09-NodeTypeChange/01-ChangeNodeAggregateType_WithoutDimensions.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ Feature: Change node aggregate type without dimensions

Then I expect the ChangeProjection to have the following changes in "user-cs-id":
| nodeAggregateId | created | changed | moved | deleted | originDimensionSpacePoint |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | null |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {} |
And I expect the ChangeProjection to have no changes in "cs-identifier"

Scenario: Change the node aggregate type with already applied changes
Expand All @@ -78,6 +78,5 @@ Feature: Change node aggregate type without dimensions

Then I expect the ChangeProjection to have the following changes in "user-cs-id":
| nodeAggregateId | created | changed | moved | deleted | originDimensionSpacePoint |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | null |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {} |
And I expect the ChangeProjection to have no changes in "cs-identifier"
5 changes: 3 additions & 2 deletions ...atures/PendingChanges/09-NodeTypeChange/02-ChangeNodeAggregateType_WithDimensions.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ Feature: Change node aggregate type with dimensions

Then I expect the ChangeProjection to have the following changes in "user-cs-id":
| nodeAggregateId | created | changed | moved | deleted | originDimensionSpacePoint |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | null |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {"language":"de"} |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {"language":"fr"} |
And I expect the ChangeProjection to have no changes in "cs-identifier"

Scenario: Change the node aggregate type with already applied changes
Expand All @@ -91,6 +92,6 @@ Feature: Change node aggregate type with dimensions

Then I expect the ChangeProjection to have the following changes in "user-cs-id":
| nodeAggregateId | created | changed | moved | deleted | originDimensionSpacePoint |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | null |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {"language":"de"} |
| sir-david-nodenborough | 0 | 1 | 0 | 0 | {"language":"fr"} |
And I expect the ChangeProjection to have no changes in "cs-identifier"
7 changes: 2 additions & 5 deletions Neos.Workspace.Ui/Classes/Controller/WorkspaceController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,9 +672,6 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos
->findByContentStreamId(
$selectedWorkspace->currentContentStreamId
);
$dimensionSpacePoints = iterator_to_array($contentRepository->getVariationGraph()->getDimensionSpacePoints());
/** @var DimensionSpacePoint $arbitraryDimensionSpacePoint */
$arbitraryDimensionSpacePoint = reset($dimensionSpacePoints);

$selectedWorkspaceContentGraph = $contentRepository->getContentGraph($selectedWorkspace->workspaceName);
// If we deleted a node, there is no way for us to anymore find the deleted node in the ContentStream
Expand All @@ -688,7 +685,7 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos
foreach ($changes as $change) {
$contentGraph = $change->deleted ? $baseWorkspaceContentGraph : $selectedWorkspaceContentGraph;
$subgraph = $contentGraph->getSubgraph(
$change->originDimensionSpacePoint?->toDimensionSpacePoint() ?: $arbitraryDimensionSpacePoint,
$change->originDimensionSpacePoint->toDimensionSpacePoint(),
VisibilityConstraints::withoutRestrictions()
);

Expand Down Expand Up @@ -756,7 +753,7 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos
$nodeAddress = NodeAddress::create(
$contentRepository->id,
$selectedWorkspace->workspaceName,
$change->originDimensionSpacePoint?->toDimensionSpacePoint() ?: $arbitraryDimensionSpacePoint,
$change->originDimensionSpacePoint->toDimensionSpacePoint(),
$change->nodeAggregateId
);

Expand Down

0 comments on commit 97f36d2

Please sign in to comment.