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.

Sign up for GitHub

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

!!! TASK: Streamline ContentGraph::findRootNodeAggregateByType #5158

Merged
merged 3 commits into from
Jul 29, 2024
Merged

!!! TASK: Streamline ContentGraph::findRootNodeAggregateByType #5158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1294040
TASK: Streamline `ContentGraph::findRootNodeAggregateByType`
mhsdesign Jun 22, 2024
18fbd1c
Apply suggestions from code review
mhsdesign Jul 3, 2024
2b411f9
Merge remote-tracking branch 'origin/9.0' into task/streamLineContent…
mhsdesign Jul 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/ContentGraph.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\NodeAggregates;
use Neos\ContentRepository\Core\Projection\ContentGraph\VisibilityConstraints;
use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId;
use Neos\ContentRepository\Core\SharedModel\Exception\RootNodeAggregateDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateClassification;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
use Neos\ContentRepository\Core\SharedModel\Node\NodeName;
Expand Down Expand Up @@ -119,17 +118,15 @@ public function getSubgraph(
return $this->subgraphs[$index];
}

/**
* @throws RootNodeAggregateDoesNotExist
*/
public function findRootNodeAggregateByType(
NodeTypeName $nodeTypeName
): NodeAggregate {
): ?NodeAggregate {
$rootNodeAggregates = $this->findRootNodeAggregates(
FindRootNodeAggregatesFilter::create(nodeTypeName: $nodeTypeName)
);

if ($rootNodeAggregates->count() > 1) {
// todo drop this check as this is enforced by the write side? https://github.com/neos/neos-development-collection/pull/4339
$ids = [];
foreach ($rootNodeAggregates as $rootNodeAggregate) {
$ids[] = $rootNodeAggregate->nodeAggregateId->value;
Expand All @@ -144,12 +141,7 @@ public function findRootNodeAggregateByType(
));
}

$rootNodeAggregate = $rootNodeAggregates->first();
if ($rootNodeAggregate === null) {
throw RootNodeAggregateDoesNotExist::butWasExpectedTo($nodeTypeName);
}

return $rootNodeAggregate;
return $rootNodeAggregates->first();
}

public function findRootNodeAggregates(
Expand Down
11 changes: 2 additions & 9 deletions Neos.ContentGraph.PostgreSQLAdapter/src/Domain/Repository/ContentHypergraph.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\NodeAggregates;
use Neos\ContentRepository\Core\Projection\ContentGraph\VisibilityConstraints;
use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId;
use Neos\ContentRepository\Core\SharedModel\Exception\RootNodeAggregateDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
use Neos\ContentRepository\Core\SharedModel\Node\NodeName;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
Expand Down Expand Up @@ -96,7 +95,7 @@ public function getSubgraph(

public function findRootNodeAggregateByType(
NodeTypeName $nodeTypeName
): NodeAggregate {
): ?NodeAggregate {
$rootNodeAggregates = $this->findRootNodeAggregates(
FindRootNodeAggregatesFilter::create(nodeTypeName: $nodeTypeName)
);
Expand All @@ -113,13 +112,7 @@ public function findRootNodeAggregateByType(
));
}

$rootNodeAggregate = $rootNodeAggregates->first();

if ($rootNodeAggregate === null) {
throw RootNodeAggregateDoesNotExist::butWasExpectedTo($nodeTypeName);
}

return $rootNodeAggregate;
return $rootNodeAggregates->first();
}

public function findRootNodeAggregates(
Expand Down
6 changes: 1 addition & 5 deletions Neos.ContentRepository.Core/Classes/Feature/Common/ConstraintChecks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
use Neos\ContentRepository\Core\SharedModel\Exception\NodeTypeNotFound;
use Neos\ContentRepository\Core\SharedModel\Exception\PropertyCannotBeSet;
use Neos\ContentRepository\Core\SharedModel\Exception\ReferenceCannotBeSet;
use Neos\ContentRepository\Core\SharedModel\Exception\RootNodeAggregateDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Exception\RootNodeAggregateTypeIsAlreadyOccupied;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
use Neos\ContentRepository\Core\SharedModel\Node\NodeName;
Expand Down Expand Up @@ -160,12 +159,9 @@ protected function requireRootNodeTypeToBeUnoccupied(
ContentGraphInterface $contentGraph,
NodeTypeName $nodeTypeName
): void {
try {
$contentGraph->findRootNodeAggregateByType($nodeTypeName);
} catch (RootNodeAggregateDoesNotExist $_) {
if ($contentGraph->findRootNodeAggregateByType($nodeTypeName) === null) {
return;
}

throw RootNodeAggregateTypeIsAlreadyOccupied::butWasExpectedNotTo($nodeTypeName);
}

Expand Down
6 changes: 1 addition & 5 deletions Neos.ContentRepository.Core/Classes/Projection/ContentGraph/ContentGraphInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
use Neos\ContentRepository\Core\Projection\ProjectionStateInterface;
use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId;
use Neos\ContentRepository\Core\SharedModel\Exception\NodeAggregatesTypeIsAmbiguous;
use Neos\ContentRepository\Core\SharedModel\Exception\RootNodeAggregateDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
use Neos\ContentRepository\Core\SharedModel\Node\NodeName;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
Expand Down Expand Up @@ -59,14 +58,11 @@ public function getSubgraph(
): ContentSubgraphInterface;

/**
* Throws exception if no root aggregate of the given type found.
*
* @throws RootNodeAggregateDoesNotExist
* @api
*/
public function findRootNodeAggregateByType(
NodeTypeName $nodeTypeName
): NodeAggregate;
): ?NodeAggregate;

/**
* @api
Expand Down
28 changes: 13 additions & 15 deletions Neos.ContentRepository.Core/Classes/Service/ContentRepositoryBootstrapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,27 +62,25 @@ public function getOrCreateLiveWorkspace(): Workspace
}

/**
* Retrieve the root Node Aggregate ID for the specified $contentStreamId
* Retrieve the root Node Aggregate ID for the specified $workspace
* If no root node of the specified $rootNodeTypeName exist, it will be created
*/
public function getOrCreateRootNodeAggregate(
Workspace $workspace,
NodeTypeName $rootNodeTypeName
): NodeAggregateId {
try {
return $this->contentRepository->getContentGraph($workspace->workspaceName)->findRootNodeAggregateByType(
$rootNodeTypeName
)->nodeAggregateId;

// TODO make this case more explicit
} catch (\Exception $exception) {
$rootNodeAggregateId = NodeAggregateId::create();
$this->contentRepository->handle(CreateRootNodeAggregateWithNode::create(
$workspace->workspaceName,
$rootNodeAggregateId,
$rootNodeTypeName,
));
return $rootNodeAggregateId;
$rootNodeAggregate = $this->contentRepository->getContentGraph($workspace->workspaceName)->findRootNodeAggregateByType(
$rootNodeTypeName
);
if ($rootNodeAggregate !== null) {
return $rootNodeAggregate->nodeAggregateId;
}
$rootNodeAggregateId = NodeAggregateId::create();
$this->contentRepository->handle(CreateRootNodeAggregateWithNode::create(
$workspace->workspaceName,
$rootNodeAggregateId,
$rootNodeTypeName,
));
return $rootNodeAggregateId;
}
}
31 changes: 0 additions & 31 deletions Neos.ContentRepository.Core/Classes/SharedModel/Exception/RootNodeAggregateDoesNotExist.php
View file
Open in desktop

This file was deleted.

11 changes: 3 additions & 8 deletions Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/CRTestSuiteTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\VisibilityConstraints;
use Neos\ContentRepository\Core\Service\ContentStreamPruner;
use Neos\ContentRepository\Core\Service\ContentStreamPrunerFactory;
use Neos\ContentRepository\Core\SharedModel\Exception\RootNodeAggregateDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamState;
Expand Down Expand Up @@ -222,13 +221,9 @@ protected function getRootNodeAggregateId(): ?NodeAggregateId
return $this->currentRootNodeAggregateId;
}

try {
return $this->currentContentRepository->getContentGraph($this->currentWorkspaceName)->findRootNodeAggregateByType(
NodeTypeName::fromString('Neos.Neos:Sites')
)->nodeAggregateId;
} catch (RootNodeAggregateDoesNotExist) {
return null;
}
return $this->currentContentRepository->getContentGraph($this->currentWorkspaceName)->findRootNodeAggregateByType(
NodeTypeName::fromString('Neos.Neos:Sites')
)->nodeAggregateId;
}

/**
Expand Down
9 changes: 4 additions & 5 deletions Neos.Neos/Classes/Controller/Module/Administration/SitesController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,10 @@ public function updateSiteAction(Site $site, $newSiteNodeName)
);
}

try {
$sitesNode = $contentRepository->getContentGraph($liveWorkspace->workspaceName)->findRootNodeAggregateByType(
NodeTypeNameFactory::forSites()
);
} catch (\Exception $exception) {
$sitesNode = $contentRepository->getContentGraph($liveWorkspace->workspaceName)->findRootNodeAggregateByType(
NodeTypeNameFactory::forSites()
);
if ($sitesNode === null) {
throw new \InvalidArgumentException(
'Cannot update a site without the sites note being present.',
1651958452
Expand Down
4 changes: 4 additions & 0 deletions Neos.Neos/Classes/Domain/Service/SiteNodeUtility.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ public function findSiteNodeBySite(
$rootNodeAggregate = $contentGraph->findRootNodeAggregateByType(
NodeTypeNameFactory::forSites()
);
if (!$rootNodeAggregate) {
throw new \RuntimeException(sprintf('No sites root node found in content repository "%s", while fetching site node "%s"', $contentRepository->id->value, $site->getNodeName()), 1719046570);
}

$rootNode = $rootNodeAggregate->getNodeByCoveredDimensionSpacePoint($dimensionSpacePoint);

$siteNode = $subgraph->findNodeByPath(
Expand Down
4 changes: 4 additions & 0 deletions Neos.Neos/Classes/Domain/Service/SiteServiceInternals.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ public function removeSiteNode(SiteNodeName $siteNodeName): void
$sitesNodeAggregate = $contentGraph->findRootNodeAggregateByType(
NodeTypeNameFactory::forSites()
);
if (!$sitesNodeAggregate) {
// nothing to prune, we could probably also return here directly?
Copy link
Member

Choose a reason for hiding this comment

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

That would be the current behavior with the exception I guess? Or rather we might actually throw here to tell people that this site does not exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes we throw currently. But as we want to remove a child node either way it seems fine to say, well we cant remove a thing here.

Copy link
Member

Choose a reason for hiding this comment

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

I think, skipping is the better behavior.
But maybe we could throw at the end of this method if no node was actually removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont think so. But with #4470 we can definitely discuss how the full behaviour should be and adjust edge cases.

continue;
}
$siteNodeAggregate = $contentGraph->findChildNodeAggregateByName(
$sitesNodeAggregate->nodeAggregateId,
$siteNodeName->toNodeName()
Expand Down
5 changes: 1 addition & 4 deletions Neos.TimeableNodeVisibility/Classes/Service/TimeableNodeVisibilityService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,7 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\PropertyValue\Criteria\PropertyValueLessThanOrEqual;
use Neos\ContentRepository\Core\Projection\ContentGraph\Node;
use Neos\ContentRepository\Core\Projection\ContentGraph\VisibilityConstraints;
use Neos\ContentRepository\Core\Projection\Workspace\Workspace;
use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId;
use Neos\ContentRepository\Core\SharedModel\Exception\RootNodeAggregateDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Node\NodeVariantSelectionStrategy;
use Neos\ContentRepository\Core\SharedModel\Node\PropertyName;
use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName;
Expand Down Expand Up @@ -101,7 +98,7 @@ private function getNodesWithExceededDates(ContentRepository $contentRepository,
$sitesNodeTypeName = NodeTypeName::fromString('Neos.Neos:Sites');
$rootNode = $subgraph->findRootNodeByType($sitesNodeTypeName);
if ($rootNode === null) {
throw RootNodeAggregateDoesNotExist::butWasExpectedTo($sitesNodeTypeName);
throw new \RuntimeException(sprintf('No sites root node found in content repository "%s"', $contentRepository->id->value), 1719047148);
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering: Why don't we keep the RootNodeAggregateDoesNotExist exception and throw it here? (not sure if it's needed but I'm curious about your reasons to replace it)

Copy link
Member Author

Choose a reason for hiding this comment

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

we are in a different package here (the timeable stuff) and its imo not needed here to have a special exception class. Its a not ever gonna happen case and just a save guard.

}

$nodes = $subgraph->findDescendantNodes(
Expand Down
Loading