Merge pull request #5357 from neos/bugfix/4284-reintroduce-stricter-c…

…hecks-for-subtree-tagging

BUGFIX: Re-introduce stricter checks for subtree en/disabling and tagging

Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/06-NodeDisabling/01-DisableNodeAggregate_ConstraintChecks.feature

@@ -61,19 +61,12 @@ Feature: Constraint checks on node aggregate disabling
       | coveredDimensionSpacePoint   | {"language": "de"}       |
       | nodeVariantSelectionStrategy | "allVariants"            |
 
-      # Note: The behavior has been changed with https://github.com/neos/neos-development-collection/pull/4284 and the test was adjusted accordingly
-    When the command DisableNodeAggregate is executed with payload:
+    When the command DisableNodeAggregate is executed with payload and exceptions are caught:
       | Key                          | Value                    |
       | nodeAggregateId              | "sir-david-nodenborough" |
       | coveredDimensionSpacePoint   | {"language": "de"}       |
       | nodeVariantSelectionStrategy | "allVariants"            |
-    Then I expect exactly 4 events to be published on stream with prefix "ContentStream:cs-identifier"
-    And event at index 3 is of type "SubtreeWasTagged" with payload:
-      | Key                          | Expected                               |
-      | contentStreamId              | "cs-identifier"                        |
-      | nodeAggregateId              | "sir-david-nodenborough"               |
-      | affectedDimensionSpacePoints | [{"language":"de"},{"language":"gsw"}] |
-      | tag                          | "disabled"                             |
+    Then the last command should have thrown an exception of type "NodeAggregateIsAlreadyDisabled"
 
 
   Scenario: Try to disable a node aggregate in a non-existing dimension space point

Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/06-NodeDisabling/04-EnableNodeAggregate_ConstraintChecks.feature

@@ -44,13 +44,12 @@ Feature: Enable a node aggregate
       | nodeVariantSelectionStrategy | "allVariants"    |
     Then the last command should have thrown an exception of type "NodeAggregateCurrentlyDoesNotExist"
 
-    # Note: The behavior has been changed with https://github.com/neos/neos-development-collection/pull/4284 and the test was adjusted accordingly
   Scenario: Try to enable an already enabled node aggregate
-    When the command EnableNodeAggregate is executed with payload:
+    When the command EnableNodeAggregate is executed with payload and exceptions are caught:
       | Key                          | Value                    |
       | nodeAggregateId              | "sir-david-nodenborough" |
       | nodeVariantSelectionStrategy | "allVariants"            |
-    Then I expect exactly 3 events to be published on stream with prefix "ContentStream:cs-identifier"
+    Then the last command should have thrown an exception of type "NodeAggregateIsAlreadyEnabled"
 
   Scenario: Try to enable a node aggregate in a non-existing dimension space point
     When the command EnableNodeAggregate is executed with payload and exceptions are caught:

Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/SubtreeTagging/TagSubtree_WithoutDimensions.feature

@@ -37,7 +37,7 @@ Feature: Tag subtree without dimensions
       | b               | Neos.ContentRepository.Testing:Document | root                  | b        |
       | b1              | Neos.ContentRepository.Testing:Document | b                     | b1       |
 
-  Scenario: Tagging the same node twice with the same subtree tag is ignored
+  Scenario: Tagging the same node twice with the same subtree tag
     When the command TagSubtree is executed with payload:
       | Key                          | Value         |
       | nodeAggregateId              | "a1"          |
@@ -50,23 +50,23 @@ Feature: Tag subtree without dimensions
       | nodeAggregateId              | "a1"            |
       | affectedDimensionSpacePoints | [[]]            |
       | tag                          | "tag1"          |
-    When the command TagSubtree is executed with payload:
+    When the command TagSubtree is executed with payload and exceptions are caught:
       | Key                          | Value         |
       | nodeAggregateId              | "a1"          |
       | nodeVariantSelectionStrategy | "allVariants" |
       | tag                          | "tag1"        |
-    Then I expect exactly 14 events to be published on stream with prefix "ContentStream:cs-identifier"
+    Then the last command should have thrown an exception of type "SubtreeIsAlreadyTagged"
 
-  Scenario: Untagging a node without tags is ignored
+  Scenario: Untagging a node without tags
     Then I expect exactly 13 events to be published on stream with prefix "ContentStream:cs-identifier"
-    When the command UntagSubtree is executed with payload:
+    When the command UntagSubtree is executed with payload and exceptions are caught:
       | Key                          | Value         |
       | nodeAggregateId              | "a1"          |
       | nodeVariantSelectionStrategy | "allVariants" |
       | tag                          | "tag1"        |
-    Then I expect exactly 13 events to be published on stream with prefix "ContentStream:cs-identifier"
+    Then the last command should have thrown an exception of type "SubtreeIsNotTagged"
 
-  Scenario: Untagging a node that is only implicitly tagged (inherited) is ignored
+  Scenario: Untagging a node that is only implicitly tagged (inherited)
     When the command TagSubtree is executed with payload:
       | Key                          | Value         |
       | nodeAggregateId              | "a1"          |
@@ -79,12 +79,12 @@ Feature: Tag subtree without dimensions
       | nodeAggregateId              | "a1"            |
       | affectedDimensionSpacePoints | [[]]            |
       | tag                          | "tag1"          |
-    When the command UntagSubtree is executed with payload:
+    When the command UntagSubtree is executed with payload and exceptions are caught:
       | Key                          | Value         |
       | nodeAggregateId              | "a1a"         |
       | nodeVariantSelectionStrategy | "allVariants" |
       | tag                          | "tag1"        |
-    Then I expect exactly 14 events to be published on stream with prefix "ContentStream:cs-identifier"
+    Then the last command should have thrown an exception of type "SubtreeIsNotTagged"
 
   Scenario: Tagging subtree with arbitrary strategy since dimensions are not involved
     When the command TagSubtree is executed with payload:

Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W8-IndividualNodePublication/03-MoreBasicFeatures.feature

@@ -182,7 +182,7 @@ Feature: Publishing individual nodes (basics)
       | originDimensionSpacePoint | {}                           |
       | propertyValues            | {"image": "Bla bli blub"}    |
 
-  Scenario: Tag the same node in live and in the user workspace so that a rebase will omit the user change
+  Scenario: Tag the same node in live and in the user workspace so that a rebase will lead to a conflict
     When the command TagSubtree is executed with payload:
       | Key                          | Value                     |
       | workspaceName                | "live"                    |
@@ -195,20 +195,14 @@ Feature: Publishing individual nodes (basics)
       | nodeAggregateId              | "sir-unchanged"           |
       | nodeVariantSelectionStrategy | "allVariants"             |
       | tag                          | "tag1"                    |
-    When the command PublishIndividualNodesFromWorkspace is executed with payload:
+    When the command PublishIndividualNodesFromWorkspace is executed with payload and exceptions are caught:
       | Key                             | Value                                                             |
       | workspaceName                   | "user-test"                                                       |
       | nodesToPublish                  | [{"dimensionSpacePoint": {}, "nodeAggregateId": "sir-unchanged"}] |
       | contentStreamIdForRemainingPart | "user-cs-identifier-remaining"                                    |
-
-    When I am in workspace "live" and dimension space point {}
-    Then I expect node aggregate identifier "sir-unchanged" to lead to node cs-identifier;sir-unchanged;{}
-    And I expect this node to be exactly explicitly tagged "tag1"
-
-    When I am in workspace "user-test" and dimension space point {}
-    Then I expect node aggregate identifier "sir-unchanged" to lead to node user-cs-identifier-remaining;sir-unchanged;{}
-    And I expect this node to be exactly explicitly tagged "tag1"
-    Then workspace user-test has status UP_TO_DATE
+    Then the last command should have thrown the WorkspaceRebaseFailed exception with:
+      | SequenceNumber | Command    | Exception              |
+      | 14             | TagSubtree | SubtreeIsAlreadyTagged |
 
   Scenario: It is possible to publish all nodes
     When the command PublishIndividualNodesFromWorkspace is executed with payload:

Neos.ContentRepository.Core/Classes/CommandHandler/CommandSimulator.php

@@ -101,10 +101,6 @@ private function handle(RebaseableCommand $rebaseableCommand): void
             throw new \RuntimeException(sprintf('%s expects an instance of %s to be returned. Got %s when handling %s', self::class, EventsToPublish::class, get_debug_type($eventsToPublish), $rebaseableCommand->originalCommand::class));
         }
 
-        if ($eventsToPublish->events->isEmpty()) {
-            return;
-        }
-
         $normalizedEvents = Events::fromArray(
             $eventsToPublish->events->map(function (EventInterface|DecoratedEvent $event) use (
                 $rebaseableCommand

Neos.ContentRepository.Core/Classes/EventStore/EventPersister.php

@@ -28,9 +28,6 @@ public function __construct(
      */
     public function publishEvents(ContentRepository $contentRepository, EventsToPublish $eventsToPublish): void
     {
-        if ($eventsToPublish->events->isEmpty()) {
-            return;
-        }
         $normalizedEvents = Events::fromArray(
             $eventsToPublish->events->map($this->eventNormalizer->normalize(...))
         );

Neos.ContentRepository.Core/Classes/EventStore/Events.php

@@ -56,11 +56,6 @@ public function map(\Closure $callback): array
         return array_map($callback, $this->events);
     }
 
-    public function isEmpty(): bool
-    {
-        return empty($this->events);
-    }
-
     public function count(): int
     {
         return count($this->events);

Neos.ContentRepository.Core/Classes/EventStore/EventsToPublish.php

@@ -25,15 +25,6 @@ public function __construct(
     ) {
     }
 
-    public static function empty(): self
-    {
-        return new EventsToPublish(
-            StreamName::fromString("empty"),
-            Events::fromArray([]),
-            ExpectedVersion::ANY()
-        );
-    }
-
     public function withAppendedEvents(Events $events): self
     {
         return new self(

Neos.ContentRepository.Core/Classes/Feature/NodeDisabling/Exception/NodeAggregateIsAlreadyDisabled.php

@@ -0,0 +1,22 @@
+<?php
+
+/*
+ * This file is part of the Neos.ContentRepository package.
+ *
+ * (c) Contributors of the Neos Project - www.neos.io
+ *
+ * This package is Open Source Software. For the full copyright and license
+ * information, please view the LICENSE file which was distributed with this
+ * source code.
+ */
+
+declare(strict_types=1);
+
+namespace Neos\ContentRepository\Core\Feature\NodeDisabling\Exception;
+
+/**
+ * @api
+ */
+final class NodeAggregateIsAlreadyDisabled extends \DomainException
+{
+}

Neos.ContentRepository.Core/Classes/Feature/NodeDisabling/Exception/NodeAggregateIsAlreadyEnabled.php

@@ -0,0 +1,22 @@
+<?php
+
+/*
+ * This file is part of the Neos.ContentRepository package.
+ *
+ * (c) Contributors of the Neos Project - www.neos.io
+ *
+ * This package is Open Source Software. For the full copyright and license
+ * information, please view the LICENSE file which was distributed with this
+ * source code.
+ */
+
+declare(strict_types=1);
+
+namespace Neos\ContentRepository\Core\Feature\NodeDisabling\Exception;
+
+/**
+ * @api
+ */
+final class NodeAggregateIsAlreadyEnabled extends \DomainException
+{
+}

Neos.ContentRepository.Core/Classes/Feature/NodeDisabling/NodeDisabling.php

@@ -19,6 +19,8 @@
 use Neos\ContentRepository\Core\DimensionSpace\Exception\DimensionSpacePointNotFound;
 use Neos\ContentRepository\Core\EventStore\Events;
 use Neos\ContentRepository\Core\EventStore\EventsToPublish;
+use Neos\ContentRepository\Core\Feature\NodeDisabling\Exception\NodeAggregateIsAlreadyDisabled;
+use Neos\ContentRepository\Core\Feature\NodeDisabling\Exception\NodeAggregateIsAlreadyEnabled;
 use Neos\ContentRepository\Core\Feature\RebaseableCommand;
 use Neos\ContentRepository\Core\Feature\ContentStreamEventStreamName;
 use Neos\ContentRepository\Core\Feature\NodeDisabling\Command\DisableNodeAggregate;
@@ -58,8 +60,7 @@ private function handleDisableNodeAggregate(
             $command->coveredDimensionSpacePoint
         );
         if ($nodeAggregate->getDimensionSpacePointsTaggedWith(SubtreeTag::disabled())->contains($command->coveredDimensionSpacePoint)) {
-            // already disabled, so we can return a no-operation.
-            return EventsToPublish::empty();
+            throw new NodeAggregateIsAlreadyDisabled(sprintf('Node aggregate "%s" cannot be disabled because it is already explicitly disabled for dimension space point %s', $nodeAggregate->nodeAggregateId->value, $command->coveredDimensionSpacePoint->toJson()), 1731166196);
         }
 
         $affectedDimensionSpacePoints = $command->nodeVariantSelectionStrategy
@@ -114,8 +115,7 @@ public function handleEnableNodeAggregate(
             $command->coveredDimensionSpacePoint
         );
         if (!$nodeAggregate->getDimensionSpacePointsTaggedWith(SubtreeTag::disabled())->contains($command->coveredDimensionSpacePoint)) {
-            // already enabled, so we can return a no-operation.
-            return EventsToPublish::empty();
+            throw new NodeAggregateIsAlreadyEnabled(sprintf('Node aggregate "%s" cannot be enabled because is not explicitly disabled for dimension space point %s', $nodeAggregate->nodeAggregateId->value, $command->coveredDimensionSpacePoint->toJson()), 1731166142);
         }
 
         $affectedDimensionSpacePoints = $command->nodeVariantSelectionStrategy

Neos.ContentRepository.Core/Classes/Feature/SubtreeTagging/Command/TagSubtree.php

@@ -22,7 +22,6 @@
 use Neos\ContentRepository\Core\Feature\WorkspacePublication\Dto\NodeIdToPublishOrDiscard;
 use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
 use Neos\ContentRepository\Core\SharedModel\Node\NodeVariantSelectionStrategy;
-use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
 use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName;
 
 /**

Neos.ContentRepository.Core/Classes/Feature/SubtreeTagging/Command/UntagSubtree.php

@@ -22,7 +22,6 @@
 use Neos\ContentRepository\Core\Feature\WorkspacePublication\Dto\NodeIdToPublishOrDiscard;
 use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
 use Neos\ContentRepository\Core\SharedModel\Node\NodeVariantSelectionStrategy;
-use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
 use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName;
 
 /**

Neos.ContentRepository.Core/Classes/Feature/SubtreeTagging/Exception/SubtreeIsAlreadyTagged.php

@@ -0,0 +1,22 @@
+<?php
+
+/*
+ * This file is part of the Neos.ContentRepository package.
+ *
+ * (c) Contributors of the Neos Project - www.neos.io
+ *
+ * This package is Open Source Software. For the full copyright and license
+ * information, please view the LICENSE file which was distributed with this
+ * source code.
+ */
+
+declare(strict_types=1);
+
+namespace Neos\ContentRepository\Core\Feature\SubtreeTagging\Exception;
+
+/**
+ * @api
+ */
+final class SubtreeIsAlreadyTagged extends \DomainException
+{
+}

Neos.ContentRepository.Core/Classes/Feature/SubtreeTagging/Exception/SubtreeIsNotTagged.php

@@ -0,0 +1,22 @@
+<?php
+
+/*
+ * This file is part of the Neos.ContentRepository package.
+ *
+ * (c) Contributors of the Neos Project - www.neos.io
+ *
+ * This package is Open Source Software. For the full copyright and license
+ * information, please view the LICENSE file which was distributed with this
+ * source code.
+ */
+
+declare(strict_types=1);
+
+namespace Neos\ContentRepository\Core\Feature\SubtreeTagging\Exception;
+
+/**
+ * @api
+ */
+final class SubtreeIsNotTagged extends \DomainException
+{
+}

Neos.ContentRepository.Core/Classes/Feature/SubtreeTagging/SubtreeTagging.php

@@ -25,6 +25,8 @@
 use Neos\ContentRepository\Core\Feature\SubtreeTagging\Command\UntagSubtree;
 use Neos\ContentRepository\Core\Feature\SubtreeTagging\Event\SubtreeWasTagged;
 use Neos\ContentRepository\Core\Feature\SubtreeTagging\Event\SubtreeWasUntagged;
+use Neos\ContentRepository\Core\Feature\SubtreeTagging\Exception\SubtreeIsAlreadyTagged;
+use Neos\ContentRepository\Core\Feature\SubtreeTagging\Exception\SubtreeIsNotTagged;
 use Neos\EventStore\Model\EventStream\ExpectedVersion;
 
 /**
@@ -47,8 +49,7 @@ private function handleTagSubtree(TagSubtree $command, CommandHandlingDependenci
         );
 
         if ($nodeAggregate->getDimensionSpacePointsTaggedWith($command->tag)->contains($command->coveredDimensionSpacePoint)) {
-            // already explicitly tagged with the same Subtree Tag, so we can return a no-operation.
-            return EventsToPublish::empty();
+            throw new SubtreeIsAlreadyTagged(sprintf('Cannot add subtree tag "%s" because node aggregate "%s" is already explicitly tagged with that tag in dimension space point %s', $command->tag->value, $nodeAggregate->nodeAggregateId->value, $command->coveredDimensionSpacePoint->toJson()), 1731167142);
         }
 
         $affectedDimensionSpacePoints = $command->nodeVariantSelectionStrategy
@@ -93,8 +94,7 @@ public function handleUntagSubtree(UntagSubtree $command, CommandHandlingDepende
         );
 
         if (!$nodeAggregate->getDimensionSpacePointsTaggedWith($command->tag)->contains($command->coveredDimensionSpacePoint)) {
-            // not explicitly tagged with the given Subtree Tag, so we can return a no-operation.
-            return EventsToPublish::empty();
+            throw new SubtreeIsNotTagged(sprintf('Cannot remove subtree tag "%s" because node aggregate "%s" is not explicitly tagged with that tag in dimension space point %s', $command->tag->value, $nodeAggregate->nodeAggregateId->value, $command->coveredDimensionSpacePoint->toJson()), 1731167464);
         }
 
         $affectedDimensionSpacePoints = $command->nodeVariantSelectionStrategy

Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php

@@ -151,6 +151,10 @@ protected function publishEvent(string $eventType, StreamName $streamName, array
      */
     public function theLastCommandShouldHaveThrown(string $shortExceptionName, ?int $expectedCode = null): void
     {
+        if ($shortExceptionName === 'WorkspaceRebaseFailed') {
+            throw new \RuntimeException('Please use the assertion "the last command should have thrown the WorkspaceRebaseFailed exception with" instead.');
+        }
+
         Assert::assertNotNull($this->lastCommandException, 'Command did not throw exception');
         $lastCommandExceptionShortName = (new \ReflectionClass($this->lastCommandException))->getShortName();
         Assert::assertSame($shortExceptionName, $lastCommandExceptionShortName, sprintf('Actual exception: %s (%s): %s', get_class($this->lastCommandException), $this->lastCommandException->getCode(), $this->lastCommandException->getMessage()));