neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 221 forks source link

FEATURE: ContentGraphAdapter for write side access #4979

Closed kitsunet closed 2 months ago

kitsunet commented 3 months ago

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

Fixes: #4973

kitsunet commented 3 months ago

Few notes here for starters:

kitsunet commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

kitsunet commented 3 months ago

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 commented 2 months ago

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.

bwaidelich commented 2 months ago

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