FEATURE: ContentGraphAdapter for write side access #4979

kitsunet · 2024-04-05T07:23:20Z

The write side no longer uses any regular finders
for checks but only accesses projection data via
the new ContentGraphAdapterInterface.

Fixes: #4973

kitsunet · 2024-04-05T07:29:04Z

Few notes here for starters:

I omitted visibility constraints everywhere as we can just assume "see everything" given that that was hardcoded before.
I added csId + workspaceName to all the methods that need it, a tuple might really make this nicer to read, but it works
I originally thought we expose the factory and not the implementation and you can grab an implementation bound to csId + workspace but there is a bunch of methods in there that do not rely on it and so it felt strange as you wouldn't have either information if you need methods that do not need it.
it's huuuuuuge, I haven't gone over and tried to reduce it by merging similar functionality, but maybe that's an option.
The interface and implementation should probably move to another place, I will snatch that from bastis PR.

kitsunet · 2024-04-05T07:40:29Z

Also I realized too late but why is e.g. the workspace projection in CR Core? it should be in the storage adapter implementations (so DBAL, postgres) etc. ? Same for the contentStreams...

bwaidelich · 2024-04-05T08:43:52Z

Also I realized too late but why is e.g. the workspace projection in CR Core? it should be in the storage adapter implementations

Is that related?
But for the record: Those projections work with MySQL and PostgreSQL – If it was part of the storage adapter we would need to copy all of the implementations to both adapters.
But I agree that it should be in separate packages

kitsunet · 2024-04-05T08:44:00Z

More note.... I guess the adapter needs a bunch more dependencies to actually be able to provide Node(s) results, we'll see how that works out, they are obviously all available when building it, but I would rather want to avoid adding them...

kitsunet · 2024-04-05T08:45:08Z

Also I realized too late but why is e.g. the workspace projection in CR Core? it should be in the storage adapter implementations

Is that related?

I tried to get rid of ALL access to finders within the handlers, so now I also took the worksapce and contentstream related methods into the adapter, which doesn't work because the DBAL adapter implementation then doesn't know about those :D

bwaidelich

Thanks so much for your efforts!
I just did a first quick pass as preparation for our weekly.

Apart from the nitpicky comments, I think the adapter should always act on a workspace/contentstream

bwaidelich · 2024-04-05T08:44:21Z

Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/ContentGraphAdapter.php

+use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName;
+
+/**
+ *


This needs some thorough documentation and an @internal annotation

Yep, documentation is missing everywhere just wanted to get this out here for now. Important though, so thanks!

bwaidelich · 2024-04-05T08:45:17Z

Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/ContentGraphAdapter.php

+        // TODO: Implement rootNodeAggregateWithTypeExists() method.
+    }
+
+    public function findParentNodeAggregates(ContentStreamId $contentStreamId, WorkspaceName $workspaceName, NodeAggregateId $childNodeAggregateId): iterable


Not directly related but we should really introduce a NodeAggregates DTO

Yes, can do maybe

Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/ContentGraphAdapterFactory.php

Neos.ContentRepository.Core/Classes/Feature/Common/ConstraintChecks.php

The write side no longer uses any regular finders for checks but only accesses projection data via the new `ContentGraphAdapterInterface`. Fixes: neos#4973

This also replaces the ContentStreamIdOverride

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

Adds the working DBAL implementation as well as some fixes. Also new Factory/Builder concept

kitsunet · 2024-04-13T19:11:58Z

Now come the hard(er) parts in a way. The nitty gritty. Some of the missing methods (see linter errors) were removed by me because phpstorm didn't detect usage. The interesting question is which of those use cases do we consider valid "read" use cases that should use the content graph and which should use the new ContentGraphAdapter. I think the two controllers are probably real use cases and should be re-instated to work. the ContentCacheFlusher for example I would rather solve with the ContentGraphAdapter as well as the SiteServiceInternals...

All resolved for now, but I will probably poke a few other corners. Also now need to deduplicate the queries.

kitsunet · 2024-04-15T08:37:02Z

Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphAdapterFactoryBuilder.php

+ * Builder to combine injected dependencies and ProjectionFActoryDependencies into a ContentGraphAdapterFactory
+ * @internal
+ */
+class ContentGraphAdapterFactoryBuilder implements ContentGraphAdapterFactoryBuilderInterface


I wouldn't mind getting around having this, and am happy for suggestions, I don't really see it though. An ugly option would be to to have something along the lines of:

ContentGraphAdapterFactory::injectProjectionDependencies(ProjectionFactoryDependencies $projectionFactoryDependencies)

and the ContentGraphAdapterFactory::__construct just gets the adapter dependencies injected (eg. DBALClient), but then all methods in the ContentGraphAdapterFactory need to throw a "TooEarly" Exception if the projection dependencies were not injected yet.

AFAIS we only need it to pass in ContentRepositoryId, NodeTypeManager and PropertyConverter – Maybe we can make these explicit dependencies somehow

..or even make the NodeFactory an explicit dependency (via some NodeFactoryInterface) but I don't know... Let's discuss

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

I think the idea is good, just where we create it and how we pass it along needs refinement, given that this PR will not stay as it is anyways, just putting it in to keep the idea intact.

kitsunet · 2024-04-23T08:57:21Z

In our weekly 2024-04-19 we talked about how this adapter is maybe not worth the trouble and if we can solve the ugly contentStreamIdOverride differently without this. And it is indeed difficult to make assumptions for after introduction of node identity, but it is very clear to me that at a higher level commands and API will use workspaceName the queries (at least for the DBAL Adapter) will need a contentStreamId to work, therefore this translation will have to happen somewhere, IMHO the changes for NodeIdentity will just push this deeper into the read side making it even more annoying (e.g. due toContentSubgraphWithRuntimeCaches) to override for the few caess that we have. And those are valid cases as we apply commands with $some workspaceName to a contentStream that has (not yet got) this workspaceName.

This together with the (from the POV of the write side) convoluted API to read stuff lets me think that this change could have merit even IF we cannot at this point avoid using read models (IMHO this could be a later improvement though) and much easier to just change the write side interface for this.

Convoluted because we apply filters, visibility and a lot of other stuff.

I am still not in love with the triple factory thing, but the separation of APIs seems very sensible still.

mhsdesign · 2024-04-23T13:28:22Z

Neos.ContentRepositoryRegistry.DoctrineDbalClient/Configuration/Settings.yaml

          'Neos.ContentRepository:ContentGraph':
            factoryObjectName: Neos\ContentGraph\DoctrineDbalAdapter\DoctrineDbalContentGraphProjectionFactory
+        contentGraphAdapterFactory:
+          factoryObjectName: Neos\ContentGraph\DoctrineDbalAdapter\ContentGraphAdapterFactoryBuilder


optional followup would be to simplify this config

mhsdesign · 2024-04-23T13:29:34Z

Neos.ContentRepositoryRegistry/Classes/ContentRepositoryRegistry.php

+        if (!$contentGraphAdapterFactoryBuilder instanceof ContentGraphAdapterFactoryBuilderInterface) {
+            throw InvalidConfigurationException::fromMessage('contentGraphAdapterFactory.factoryObjectName for content repository "%s" is not an instance of %s but %s.', $contentRepositoryIdentifier->value, ContentGraphAdapterFactoryBuilderInterface::class, get_debug_type($contentGraphAdapterFactoryBuilder));
+        }
+        // TODO: Do we want to add options here? Would then have to be a setter I guess....


todo over here ;)

mhsdesign · 2024-04-23T13:31:49Z

Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphAdapter.php

We should refine this naming

mhsdesign · 2024-04-23T13:33:07Z

Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphAdapter.php

+    public function getWorkspace(): Workspace
+    {
+        $query = $this->dbalConnection->prepare('SELECT * FROM cr_default_p_workspace WHERE workspacename LIKE :workspaceName');


illegal select, and the Workspace tdo is too big

mhsdesign · 2024-04-23T13:33:21Z

Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphAdapter.php

+        // TODO: This table is not allowed here...
+        $query = $this->dbalConnection->prepare('SELECT currentcontentstreamid FROM cr_default_p_workspace WHERE workspacename LIKE :workspaceName');


todo illegal

bwaidelich · 2024-05-08T07:09:41Z

This is to be replaced by #5028 and I suggest to close the PR (we can still keep the branch for reference if needed)

github-actions bot added Feature 9.0 labels Apr 5, 2024

bwaidelich reviewed Apr 5, 2024

View reviewed changes

kitsunet added 4 commits April 5, 2024 22:10

FEATURE: ContentGraphAdapter for write side access

aff3291

The write side no longer uses any regular finders for checks but only accesses projection data via the new `ContentGraphAdapterInterface`. Fixes: neos#4973

Make ContentGraphAdapterFactory configurable

4dd0880

Introduce ContentGraphAdapterProvider

606a85b

Introduce ContentGraphAdapterProviderInterface

c7480fe

This also replaces the ContentStreamIdOverride

kitsunet force-pushed the feature/content-graph-adapter-4973 branch from d0f89bd to c7480fe Compare April 7, 2024 10:42

kitsunet commented Apr 7, 2024

View reviewed changes

Neos.ContentRepository.Core/Classes/Feature/ContentGraphAdapterInterface.php Outdated Show resolved Hide resolved

kitsunet commented Apr 7, 2024

View reviewed changes

Neos.ContentRepository.Core/Classes/Feature/ContentGraphAdapterInterface.php Outdated Show resolved Hide resolved

kitsunet commented Apr 7, 2024

View reviewed changes

Neos.ContentRepository.Core/Classes/Feature/ContentGraphAdapterInterface.php Outdated Show resolved Hide resolved

kitsunet commented Apr 7, 2024

View reviewed changes

Neos.ContentRepository.Core/Classes/Feature/ContentGraphAdapterInterface.php Show resolved Hide resolved

ContentGraphAdapter working implementation

2f05745

Adds the working DBAL implementation as well as some fixes. Also new Factory/Builder concept

kitsunet marked this pull request as ready for review April 13, 2024 14:48

kitsunet marked this pull request as draft April 13, 2024 14:50

kitsunet added 3 commits April 13, 2024 17:22

Merge branch '9.0' into feature/content-graph-adapter-4973

c184bee

Linting and Naming ContentGraphAdapter

590d143

More code cleanup

1b1f386

kitsunet added 6 commits April 13, 2024 21:17

Code renaming and refinement

6d501b2

linter fix

a55e6c8

Use ContentGraphAdapter in SiteServiceInternals

4af5126

Query deduplication part 1

dd30d55

Query deduplication part 2

027cb02

More cleanup

dad8756

Fix that phpstan doesn't understand condition

87d6336

kitsunet commented Apr 15, 2024

View reviewed changes

...TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php Outdated Show resolved Hide resolved

bwaidelich reviewed Apr 15, 2024

View reviewed changes

...TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php Outdated Show resolved Hide resolved

kitsunet added 4 commits April 15, 2024 11:21

Cleanup

8d580b8

Make factory signature clearer

2f8bfe5

Linter fixes

9826f99

Merge branch '9.0' into feature/content-graph-adapter-4973

0bed8c2

This was referenced Apr 19, 2024

WIP: FEATURE: Rework CR CatchUp mechanism #4988

Draft

WIP: FEATURE: Integrate ContentStream projection into ContentGraph #4980

Draft

WIP: FEATURE: Add workspaceName to relevant events #5002

Draft

Add ContentGraphTableNames

bcf77d7

I think the idea is good, just where we create it and how we pass it along needs refinement, given that this PR will not stay as it is anyways, just putting it in to keep the idea intact.

kitsunet requested a review from mhsdesign April 23, 2024 12:37

mhsdesign reviewed Apr 23, 2024

View reviewed changes

Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphAdapter.php

Copy link

Member

mhsdesign Apr 23, 2024

Choose a reason for hiding this comment

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

We should refine this naming

mhsdesign reviewed Apr 23, 2024

View reviewed changes

bwaidelich mentioned this pull request Apr 26, 2024

WIP: ContentGraphAdapter factory #4978

Closed

6 tasks

kitsunet closed this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: ContentGraphAdapter for write side access #4979

FEATURE: ContentGraphAdapter for write side access #4979

kitsunet commented Apr 5, 2024

kitsunet commented Apr 5, 2024 •

edited

kitsunet commented Apr 5, 2024

bwaidelich commented Apr 5, 2024

kitsunet commented Apr 5, 2024

kitsunet commented Apr 5, 2024

bwaidelich left a comment

bwaidelich Apr 5, 2024

kitsunet Apr 5, 2024

bwaidelich Apr 5, 2024

kitsunet Apr 5, 2024

kitsunet commented Apr 13, 2024 •

edited

kitsunet Apr 15, 2024

bwaidelich Apr 15, 2024

bwaidelich Apr 15, 2024

kitsunet commented Apr 23, 2024

mhsdesign Apr 23, 2024

mhsdesign Apr 23, 2024

mhsdesign Apr 23, 2024

mhsdesign Apr 23, 2024

mhsdesign Apr 23, 2024

bwaidelich commented May 8, 2024

		// TODO: This table is not allowed here...
		$query = $this->dbalConnection->prepare('SELECT currentcontentstreamid FROM cr_default_p_workspace WHERE workspacename LIKE :workspaceName');

FEATURE: ContentGraphAdapter for write side access #4979

FEATURE: ContentGraphAdapter for write side access #4979

Conversation

kitsunet commented Apr 5, 2024

kitsunet commented Apr 5, 2024 • edited

kitsunet commented Apr 5, 2024

bwaidelich commented Apr 5, 2024

kitsunet commented Apr 5, 2024

kitsunet commented Apr 5, 2024

bwaidelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsunet commented Apr 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsunet commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwaidelich commented May 8, 2024

kitsunet commented Apr 5, 2024 •

edited

kitsunet commented Apr 13, 2024 •

edited