TASK: document and assert Node.nodeName behavior

[mhsdesign]

related #4311

When looking into the php node readmodel it should be super clear that a nodename is guaranteed to be set if its tethered.

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]

Sadly phpstan doenst allow us to annotate this behavior ...

https://phpstan.org/writing-php-code/narrowing-types#custom-type-checking-functions-and-methods

And also the feature like in @return doesnt work here https://phpstan.org/blog/phpstan-1-6-0-with-conditional-return-types:

/** @var ($this->classification is TETHRED ? string : null) */

playground:
https://phpstan.org/r/7c491f32-89df-4b4b-a218-00c2b1889933

[bwaidelich]

+1 for defensive style (even though people will probably not instantiate this class manually) because it makes invalid states impossible.
In the long run we will hopefully be able to remove NodeName and NodeType from the Node read model, and can remove the checks then.

[bwaidelich]

just two mini tweaks, +1 apart from those

[mhsdesign]

Hmm but now our ProjectionIntegrityViolationDetection fails:

neos-development-collection/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/ProjectionIntegrityViolationDetection/TetheredNodesAreNamed.feature

Line 42 in d614486

When the event NodeAggregateWithNodeWasCreated was published with payload:

001 Scenario: Create node variants of different type                             # Features/ProjectionIntegrityViolationDetection/TetheredNodesAreNamed.feature:41
      When the event NodeAggregateWithNodeWasCreated was published with payload: # Features/ProjectionIntegrityViolationDetection/TetheredNodesAreNamed.feature:42
        InvalidArgumentException: The NodeName must be set if the Node is tethered. in /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/Projection/ContentGraph/Node.php:79
        Stack trace:
        #0 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/NodeFactory.php(67): Neos\ContentRepository\Core\Projection\ContentGraph\Node->__construct()
        #1 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/NodeFactory.php(166): Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\NodeFactory->mapNodeRowToNode()
        #2 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/ContentGraph.php(245): Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\NodeFactory->mapNodeRowsToNodeAggregate()
        #3 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Data/Temporary/Testing/SubContextBehat/Cache/Code/Flow_Object_Classes/Neos_Neos_Fusion_Cache_GraphProjectorCatchUpHookForCacheFlushing.php(176): Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\ContentGraph->findNodeAggregateById()
        #4 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/Projection/DelegatingCatchUpHook.php(44): Neos\Neos\Fusion\Cache\GraphProjectorCatchUpHookForCacheFlushing_Original->onAfterEvent()
        #5 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/ContentRepository.php(183): Neos\ContentRepository\Core\Projection\DelegatingCatchUpHook->onAfterEvent()
        #6 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Libraries/neos/eventstore/src/CatchUp/CatchUp.php(148): Neos\ContentRepository\Core\ContentRepository->Neos\ContentRepository\Core\{closure}()
        #7 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/ContentRepository.php(192): Neos\EventStore\CatchUp\CatchUp->run()
        #8 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Data/Temporary/Testing/SubContextBehat/Cache/Code/Flow_Object_Classes/Neos_ContentRepositoryRegistry_Factory_ProjectionCatchUpTrigger_CatchUpTriggerWithSynchronousOption.php(70): Neos\ContentRepository\Core\ContentRepository->catchUpProjection()
        #9 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/EventStore/EventPersister.php(70): Neos\ContentRepositoryRegistry\Factory\ProjectionCatchUpTrigger\CatchUpTriggerWithSynchronousOption_Original->triggerCatchUp()
        #10 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php(145): Neos\ContentRepository\Core\EventStore\EventPersister->publishEvents()
        #11 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/Features/NodeCreation.php(293): FeatureContext->publishEvent()
        #12 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Libraries/behat/behat/src/Behat/Testwork/Call/Handler/RuntimeCallHandler.php(110): FeatureContext->theEventNodeAggregateWithNodeWasCreatedWasPublishedToStreamWithPayload()

because of this (enabled) GraphProjectorCatchUpHookForCacheFlushing

neos-development-collection/Neos.Neos/Classes/Fusion/Cache/GraphProjectorCatchUpHookForCacheFlushing.php

Line 176 in 52be2f0

$nodeAggregate = $this->contentRepository->getContentGraph()->findNodeAggregateById(

A hack would be to disable it temporary? private static bool $enabled = true;

[bwaidelich]

because of this (enabled)

What is the root cause? Can't we fix that (e.g. adjust the test such that it does not create an invalid node)?

[mhsdesign]

What is the root cause? Can't we fix that (e.g. adjust the test such that it does not create an invalid node)?

as far as i understand is that we test that if a the database projection has an invalid node (somehow) we detect it.
For this to achieve we cannot use the normal commands but tell behat that those events are published (hacky! - but one of the few only ways, we should refrain from doing Given the event #4336)
That results in some catchups being triggered that wont work because something is really wrong.

[mhsdesign]

yes after adjusting the catch up the tests pass.

The neos.neos catchup is registers itself here into the default preset

neos-development-collection/Neos.Neos/Configuration/Settings.ContentRepositoryRegistry.yaml

Lines 18 to 21 in cc4a0a6

	'Neos.ContentRepository:ContentGraph':
	catchUpHooks:
	'Neos.Neos:FlushContentCache':
	factoryObjectName: Neos\Neos\Fusion\Cache\GraphProjectorCatchUpHookForCacheFlushingFactory

and since we use the content repository registry in Neos.ContentRepository.BehavioralTests

neos-development-collection/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Bootstrap/FeatureContext.php

Line 164 in a177872

$contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId);

we get everything registered for the default cr for free ;)

this is partially a good thing as we fully e2e test everything (at least that stuff generally works) but might also cause trouble like when doctrine is not setup but a projection needs it or it might slow down tests.

[mhsdesign]

Thanks for all your input today. Inspired from some other tests in the doctrine dbal adapter:

To modifying the db directly to provoke this invalid state, i moved the test over there and did the same ;)

see cd8408c

hopefully this can now be finally merged :D Didnt expect this to blow up that much :P

[bwaidelich]

Nice one, thanks!

[mhsdesign]

Thank you all for the extensive discussion last Friday (@kitsunet @bwaidelich @nezaniel) and sorry for hijacking that weekly partly for this non critical thing :D Im happy that this is older task of mine is now resolved ;)