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: Remove obsolete findContentStream(s) #5340

Open
wants to merge 4 commits into
base: 9.0
Choose a base branch
from
+33 −157
Open

TASK: Remove obsolete findContentStream(s) #5340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6249a98
TASK: Remove obsolete `findContentStreams`
mhsdesign Nov 2, 2024
9375fd9
TASK: Make `findContentStreamById` fully internal (like `countNodes`)
mhsdesign Nov 2, 2024
55f592d
Merge remote-tracking branch 'origin/9.0' into task/remove-obsolete-f…
mhsdesign Nov 13, 2024
e66da94
TASK: Declare `ContentStream` read model internal
mhsdesign Nov 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
17 changes: 0 additions & 17 deletions Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphReadModelAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStream;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreams;
use Neos\ContentRepository\Core\SharedModel\Workspace\Workspace;
use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName;
use Neos\ContentRepository\Core\SharedModel\Workspace\Workspaces;
Expand Down Expand Up @@ -123,22 +122,6 @@ public function findContentStreamById(ContentStreamId $contentStreamId): ?Conten
return self::contentStreamFromDatabaseRow($row);
}

public function findContentStreams(): ContentStreams
{
$contentStreamsStatement = <<<SQL
SELECT
id, sourceContentStreamId, version, closed
FROM
{$this->tableNames->contentStream()}
SQL;
try {
$rows = $this->dbal->fetchAllAssociative($contentStreamsStatement);
} catch (Exception $e) {
throw new \RuntimeException(sprintf('Failed to load content streams from database: %s', $e->getMessage()), 1716903042, $e);
}
return ContentStreams::fromArray(array_map(self::contentStreamFromDatabaseRow(...), $rows));
}

public function countNodes(): int
{
$countNodesStatement = <<<SQL
Expand Down
9 changes: 1 addition & 8 deletions Neos.ContentGraph.PostgreSQLAdapter/src/ContentHyperGraphReadModelAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,13 @@
use Doctrine\DBAL\Connection;
use Neos\ContentGraph\PostgreSQLAdapter\Domain\Repository\ContentHypergraph;
use Neos\ContentGraph\PostgreSQLAdapter\Domain\Repository\NodeFactory;
use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphReadModelInterface;
use Neos\ContentRepository\Core\NodeType\NodeTypeManager;
use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphReadModelInterface;
use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId;
use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStream;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreams;
use Neos\ContentRepository\Core\SharedModel\Workspace\Workspace;
use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName;
use Neos\ContentRepository\Core\SharedModel\Workspace\Workspaces;
Expand Down Expand Up @@ -60,12 +59,6 @@ public function findContentStreamById(ContentStreamId $contentStreamId): ?Conten
return null;
}

public function findContentStreams(): ContentStreams
{
// TODO: Implement getContentStreams() method.
return ContentStreams::createEmpty();
}

public function countNodes(): int
{
// TODO: Implement countNodes method.
Expand Down
14 changes: 0 additions & 14 deletions Neos.ContentRepository.Core/Classes/ContentRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,10 @@
use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryStatus;
use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\User\UserIdProviderInterface;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStream;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreams;
use Neos\ContentRepository\Core\SharedModel\Workspace\Workspace;
use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName;
use Neos\ContentRepository\Core\SharedModel\Workspace\Workspaces;
use Neos\EventStore\EventStoreInterface;
use Neos\EventStore\Exception\ConcurrencyException;
use Neos\EventStore\Model\EventEnvelope;
use Neos\EventStore\Model\EventStream\VirtualStreamName;
use Psr\Clock\ClockInterface;
Expand Down Expand Up @@ -115,7 +111,7 @@
$eventsToPublish = $this->enrichEventsToPublishWithMetadata($yieldedEventsToPublish);
try {
$this->eventPersister->publishWithoutCatchup($eventsToPublish);
} catch (ConcurrencyException $concurrencyException) {

Check failure on line 114 in Neos.ContentRepository.Core/Classes/ContentRepository.php

View workflow job for this annotation

GitHub Actions / PHP 8.2 Test linting-unit-functionaltests-mysql (deps: highest) 

Caught class Neos\ContentRepository\Core\ConcurrencyException not found.

Check failure on line 114 in Neos.ContentRepository.Core/Classes/ContentRepository.php

View workflow job for this annotation

GitHub Actions / PHP 8.3 Test linting-unit-functionaltests-mysql (deps: highest) 

Caught class Neos\ContentRepository\Core\ConcurrencyException not found.
// we pass the exception into the generator (->throw), so it could be try-caught and reacted upon:
//
// try {
Expand All @@ -128,7 +124,7 @@
if ($yieldedErrorStrategy instanceof EventsToPublish) {
$this->eventPersister->publishWithoutCatchup($yieldedErrorStrategy);
}
throw $concurrencyException;

Check failure on line 127 in Neos.ContentRepository.Core/Classes/ContentRepository.php

View workflow job for this annotation

GitHub Actions / PHP 8.2 Test linting-unit-functionaltests-mysql (deps: highest) 

Throwing object of an unknown class Neos\ContentRepository\Core\ConcurrencyException.

Check failure on line 127 in Neos.ContentRepository.Core/Classes/ContentRepository.php

View workflow job for this annotation

GitHub Actions / PHP 8.3 Test linting-unit-functionaltests-mysql (deps: highest) 

Throwing object of an unknown class Neos\ContentRepository\Core\ConcurrencyException.
}
}
} finally {
Expand Down Expand Up @@ -286,16 +282,6 @@
return $this->contentGraphReadModel->findWorkspaces();
}

public function findContentStreamById(ContentStreamId $contentStreamId): ?ContentStream
{
return $this->contentGraphReadModel->findContentStreamById($contentStreamId);
}

public function findContentStreams(): ContentStreams
{
return $this->contentGraphReadModel->findContentStreams();
}

public function getNodeTypeManager(): NodeTypeManager
{
return $this->nodeTypeManager;
Expand Down
6 changes: 3 additions & 3 deletions ...ContentRepository.Core/Classes/Projection/ContentGraph/ContentGraphReadModelInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStream;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreams;
use Neos\ContentRepository\Core\SharedModel\Workspace\Workspace;
use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName;
use Neos\ContentRepository\Core\SharedModel\Workspace\Workspaces;
Expand All @@ -42,10 +41,11 @@ public function findWorkspaceByName(WorkspaceName $workspaceName): ?Workspace;

public function findWorkspaces(): Workspaces;

/**
* @internal only used for constraint checks and in testcases, the public API must only use workspaces {@see findWorkspaceByName}.
*/
public function findContentStreamById(ContentStreamId $contentStreamId): ?ContentStream;

public function findContentStreams(): ContentStreams;

/**
* Provides the total number of projected nodes regardless of workspace or content stream.
*
Expand Down
5 changes: 1 addition & 4 deletions Neos.ContentRepository.Core/Classes/SharedModel/Workspace/ContentStream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
/**
* Content Stream Read Model
*
* @api Note: The constructor is not part of the public API
* @internal the public API is limited to the {@see Workspace} read model
*/
final readonly class ContentStream
{
Expand All @@ -31,9 +31,6 @@ private function __construct(
) {
}

/**
* @internal
*/
public static function create(
ContentStreamId $id,
?ContentStreamId $sourceContentStreamId,
Expand Down
95 changes: 0 additions & 95 deletions Neos.ContentRepository.Core/Classes/SharedModel/Workspace/ContentStreams.php
View file
Open in desktop

This file was deleted.

44 changes: 28 additions & 16 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 @@ -108,7 +108,7 @@ protected function readPayloadTable(TableNode $payloadTable): array
*/
public function iExpectTheContentStreamToExist(string $rawContentStreamId): void
{
$contentStream = $this->currentContentRepository->findContentStreamById(ContentStreamId::fromString($rawContentStreamId));
$contentStream = $this->getContentGraphReadModel()->findContentStreamById(ContentStreamId::fromString($rawContentStreamId));
Assert::assertNotNull($contentStream, sprintf('Content stream "%s" was expected to exist, but it does not', $rawContentStreamId));
}

Expand All @@ -117,7 +117,7 @@ public function iExpectTheContentStreamToExist(string $rawContentStreamId): void
*/
public function iExpectTheContentStreamToNotExist(string $rawContentStreamId, string $not = ''): void
{
$contentStream = $this->currentContentRepository->findContentStreamById(ContentStreamId::fromString($rawContentStreamId));
$contentStream = $this->getContentGraphReadModel()->findContentStreamById(ContentStreamId::fromString($rawContentStreamId));
Assert::assertNull($contentStream, sprintf('Content stream "%s" was not expected to exist, but it does', $rawContentStreamId));
}

Expand All @@ -142,20 +142,7 @@ public function workspaceStatusMatchesExpected(string $rawWorkspaceNames, string
*/
public function iExpectTheGraphProjectionToConsistOfExactlyNodes(int $expectedNumberOfNodes): void
{
// HACK to access
$contentGraphReadModelAccess = new class implements ContentRepositoryServiceFactoryInterface {
public ContentGraphReadModelInterface|null $instance;
public function build(ContentRepositoryServiceFactoryDependencies $serviceFactoryDependencies): ContentRepositoryServiceInterface
{
$this->instance = $serviceFactoryDependencies->projectionsAndCatchUpHooks->contentGraphProjection->getState();
return new class implements ContentRepositoryServiceInterface
{
};
}
};
$this->getContentRepositoryService($contentGraphReadModelAccess);

$actualNumberOfNodes = $contentGraphReadModelAccess->instance->countNodes();
$actualNumberOfNodes = $this->getContentGraphReadModel()->countNodes();
Assert::assertSame($expectedNumberOfNodes, $actualNumberOfNodes, 'Content graph consists of ' . $actualNumberOfNodes . ' nodes, expected were ' . $expectedNumberOfNodes . '.');
}

Expand Down Expand Up @@ -252,6 +239,31 @@ abstract protected function getContentRepositoryService(
ContentRepositoryServiceFactoryInterface $factory
): ContentRepositoryServiceInterface;

final protected function getContentRepositoryServiceFactoryDependencies(): ContentRepositoryServiceFactoryDependencies
{
$accessorFactory = new class implements ContentRepositoryServiceFactoryInterface {
public function build(ContentRepositoryServiceFactoryDependencies $serviceFactoryDependencies): ContentRepositoryServiceInterface
{
return new class ($serviceFactoryDependencies) implements ContentRepositoryServiceInterface
{
public function __construct(
public readonly ContentRepositoryServiceFactoryDependencies $dependencies
) {
}
};
}
};
return $this->getContentRepositoryService($accessorFactory)->dependencies;
}

final protected function getContentGraphReadModel(): ContentGraphReadModelInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

or maybe, if we like it or not, we should just expose the ContentGraphReadModel ON the content repository directly as @internal method with note to be careful. We cannot avoid people ditching security (see $context->withoutAuthorisationChecks) and as this ContentGraphReadModel will soon to be exposed for catchup hooks as api already ...

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's not about preventing folks to intentionally circumvent security – they always can, as you pointed out.
But the API should make that cumbersome or at least very very explicit. And exposing a internal concept in an api-object is a code smell IMO (even if it is marked with warnings)

I think, we should just have some custom "accessor factory" in our CRTestSuiteTrait that is instantiated once and provides read-access to all required dependencies.

Copy link
Member Author

@mhsdesign mhsdesign Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

that is instantiated once

Okay agreed, but that escalated to a (possibly) bigger change: #5341 so lets have this optimisation separate for later.

Copy link
Member

Choose a reason for hiding this comment

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

I would rather merge many small changes than always having these huge refactor everyhting things...

Copy link
Member Author

Choose a reason for hiding this comment

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

oke so are you fine with the code now? @kitsunet

{
return $this->getContentRepositoryServiceFactoryDependencies()
->projectionsAndCatchUpHooks
->contentGraphProjection
->getState();
}

/**
* @When I replay the :projectionName projection
*/
Expand Down
Loading