FEATURE: Introduce NodeIdentity

[mhsdesign]

Resolves partially: #4564
Required for a new NodeUriBuilder api #4552 which in-turn "should" solve cross cr linking: #4441

The NodeIdentity will contain WorkspaceName instead of the ContentStreamId

For most, if not all use cases, like caching a reference to a node in fusions @cache.context or serializing a node into the frontend and passing it around as query parameter, the workspace name should be used.

That guarantees that a node is still findable, when a workspace will be rebased (which can happen anytime, and thus the csid would be changed

Upgrade instructions

Review instructions

• Userland should not need to operate on cs ids
• Events must operate on cs ids
• getSubgraph arguments
  • WorkspaceName → internally resolved to cs id and will do lookup instantly (CACHE)
  • DimensionSpacePoint
  • VisibilityConstraints
• NodeFactory::mapNodeRowToNode must accept WorkspaceName and ContentStreamId from outside.
  • must accept ContentStreamId as parameter see TASK: Pass ContentStreamId to NodeFactory #4941
  • must accept WorkspaceName as parameter
• => Extend SubgraphIdentity
  • add workspaceName
  • add @internal annotation
    • and to its getters like getIdentity FEATURE 4327 - Expose a subgraph's identity #4678
• command handlers should still operate on cs ids via getSubgraphByIdentity - INTERNAL
  • by that both apis always pass the WorkspaceName
• Node should know its
  • ContentRepositoryId
  • WorkspaceName
    • NOT: ContentSubgraphIdentity.
    • NOT: ContentStreamId
  • DimensionSpacePoint it was fetched from
  • VisibilityConstraints
    • so it can be passed as single instance and eel helpers can fetch parents e.t.c in the same context
  • OriginDimensionSpacePoint (as before)
• WorkspaceName::fromString() must be stricter without colons (so that we could allow in the future for extension, e.g. WorkspaceName::detached(ContentStreamid $csId) (with an internal representation like “cs:”)

#4943 (comment) but maybe manual refresh instead of implicit through handle.

TODOs as discussed with @mhsdesign and @kitsunet today:

• add workspaceName <-> contentStreamId table to content graph projection
• add workspaceName -> contentStreamId lookup to ContentGraph::getSubgraph() (with runtime cache in the ContentGraph – the same workspace will resolve to the same CS ID in one request, maybe reset runtime cache via markStale())
• rewrite ContentGraph::find* queries to use workspaceName instead of contentStreamId (IMO it's totally fine to join the workspaceName <-> cs id table here for those queries. Alternatively we could add a dedicated lookup + runtime cache)
• remove SubgraphIdentity

Afterwards we have to find out whether there are places that need to be able to override the worspace name -> cs id mapping for tests or CR constraints. If so we could either:

• Add a separate, internal, ContentGraph::getSubgraphInternal() – but that would be dangerous because it is part of the public interface (even if marked internal)
• Introduce a ContentsubgraphFactoryInterface that can be used via CR service
• Provide a way to hook into the runtime cache, e.g. $contentGraph->dangerousSetContentStreamIdForWorkspaceNameFooBar(...)
• Extract workspaceName<->cs id mapping to external service that can be replaced/hooked into

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

[bwaidelich]

Some initial comments.

My main concern (not with this PR but in general regarding this topic) is the implicit distinction between the "content stream" and the "workspace" view.

E.g. ContentSubgraphIdentity vs NodeIdentity

[bwaidelich]

As recap from the weekly:
I like the NodeIdentity DTO and I think that it really makes sense.

NodeSerializer does not really make sense to me yet and I think it highlights some inconsistencies we currently have.

I could imagine that we could change the Node read model from:

subgraphIdentity:
  contentRepositoryId
  contentStreamId
  dimensionSpacePoint
  visibilityConstraints
nodeAggregateId
originDimensionSpacePoint
...

to

identity
  contentRepositoryId
  workspaceName
  dimensionSpacePoint
  nodeAggregateId
originDimensionSpacePoint
...

But I'm not sure about all implications yet

[kitsunet]

@bwaidelich the suggested properties for node would prevent us from ever addressing a node that is in a contentstream without workspace attachment. I think it's fine to not plan for this from outside, but we might need to load one eg. for conflict resolution?

[bwaidelich]

I just discussed this topic with @skurfuerst and he also agrees to getting rid of the CS id in the public APIs as much as possible.

We do probably still need visibilityConstraints in the Node read model because we use them to "stay in the same visibility context" when passing nodes via Fusion.
Personally I would prefer some required Fusion runtime variable for that (similar to request) but that would probably be too big of a change.
visibilityConstraints should not be part of the NodeIdentity though but it could be a property of the Node itself (and SubgraphIdentity can probably removed afterwards).

@kitsunet re

the suggested properties for node would prevent us from ever addressing a node that is in a contentstream without workspace attachment

That's true and might be a potential issue.
Sebastian suggested to introduce something like a VirtualWorkspaceName (or TransientWorkspaceName or some similar name) that reflects a CS id without a corresponding workspace (a bit similar to a detached head in Git)
This could be a concept of the read model exclusively, i.e.

class NodeIdentity
{
    private function __construct(
        // ...
        public WorkspaceName|VirtualWorkspaceName $workspaceName
// ...

For write operations, i.e. commands, we would always require a WorkspaceName

[mhsdesign]

I implemented some wip stuff, edit moved to https://github.com/mhsdesign/neos-development-collection/tree/feature/NodeIdentityDto-original-draft

Details

ala (my goal was to not have to keep track of the workspaces in the content graph projection)

final class ContentRepository
{
    public function getSubgraph(NodeIdentity $nodeIdentity, VisibilityConstraints $visibilityConstraints): ContentSubgraphInterface
    {
        if ($nodeIdentity->workspaceName instanceof DetachedWorkspaceName) {
            return $this->getContentGraph()->getSubgraph(
                $nodeIdentity->workspaceName->contentStreamId,
                $nodeIdentity->dimensionSpacePoint,
                $visibilityConstraints
            );
        }
        $workspace = $this->getWorkspaceFinder()->findOneByName($nodeIdentity->workspaceName);
        // ...


interface ContentGraphInterface extends ProjectionStateInterface
{
    public function getSubgraph(
        ContentStreamId|Workspace $contentStreamReference,
        DimensionSpacePoint $dimensionSpacePoint,
        VisibilityConstraints $visibilityConstraints
    ): ContentSubgraphInterface;


final readonly class NodeIdentity implements \JsonSerializable
{
    private function __construct(
        public ContentRepositoryId $contentRepositoryId,
        public WorkspaceName|DetachedWorkspaceName $workspaceName,
        public DimensionSpacePoint $dimensionSpacePoint,
        public NodeAggregateId $nodeAggregateId,
    ) {
    }


final class ContentSubgraph implements ContentSubgraphInterface
{
    // todo what if there was already a subgraph with runtime cache constructed and different / no workspace name?
    public function __construct(
        private readonly ContentRepositoryId $contentRepositoryId,
        private readonly ContentStreamId $contentStreamId,
        private readonly ?WorkspaceName $workspaceName,


final class NodeFactory
{
    public function __construct(
        private readonly ContentRepositoryId $contentRepositoryId,
        private readonly NodeTypeManager $nodeTypeManager,
        private readonly PropertyConverter $propertyConverter
    ) {
    }

    /**
     * @param array<string,string> $nodeRow Node Row from projection (<prefix>_node table)
     */
    public function mapNodeRowToNode(
        array $nodeRow,
        DimensionSpacePoint $dimensionSpacePoint,
        VisibilityConstraints $visibilityConstraints,
        ?WorkspaceName $workspaceName,
    ): Node {
        $nodeType = $this->nodeTypeManager->hasNodeType($nodeRow['nodetypename'])
            ? $this->nodeTypeManager->getNodeType($nodeRow['nodetypename'])
            : null;

        return Node::create(
            NodeIdentity::create(
                $this->contentRepositoryId,
                $workspaceName ?? DetachedWorkspaceName::fromContentStreamId(
                    ContentStreamId::fromString($nodeRow['contentstreamid'])
                ),
                $dimensionSpacePoint,
                NodeAggregateId::fromString($nodeRow['nodeaggregateid'])
            ),

[kitsunet]

The VirtualWorkspaceName idea seems fine, I think that should cover most bases, I like it :)

Re the visibility constraints, IMHO it makes sense to know where exactly that node is from, to refer to other nodes, eg. if I get one of those nodes handed in userland code without them being available on the node I would need to pass that in parallel to not suddenly drop into a different visibility if I need a subtree for this node.

[bwaidelich]

eg. if I get one of those nodes handed in userland code without them being available on the node I would need to pass that in parallel

IMO that would be the "better" way to solve this: To traverse the graph from a given node, you'd need the visibility constraints, too.
Merging those into the node read model for convenience blurs the responsibilities (e.g. whether they should be part of some caching identifier depends on the scenario).

But the usecase you describe is exactly why we would need it for now because the alternative would be to introduce the visibility constraints as some kind of context to the fusion runtime

[bwaidelich]

Any update on this one?
It's one of the major outstandig changes AFAIR and it blocks some security related features.
Let me know if I can help!

[mhsdesign]

Jup. #4790 should be merged first as i was about to adjust the node factory and was like wait a second, dont make it harder with the conflicts ;)

[bwaidelich]

@mhsdesign Thanks for the update and let me know if I can help with this one!

[mhsdesign]

NodeAggregates also need to operate on workspace names, but for that we would have to adjust the content graph api. That might go in a separate pr.

public function findNodeAggregateById(
    WorkspaceName $workspaceName,
    NodeAggregateId $nodeAggregateId
): ?NodeAggregate;

the cs id would be looked up internally via the workspaces table.
And the cache would be similar flushed on markStale as the cached subgraphs

[mhsdesign]

The problem with this would be that the write site heavily relies on the ContentGraph and must work on a specific contentstream id (see ContentStreamIdOverride)

unless we create a custom ContentGraph which will use our content stream, or by actually hacky checking against the state in ContentStreamIdOverride we wont get around that.

[mhsdesign]

Would be great to find out why #4942 is how it is.
These were the only commands untouched by bernhards refactoring.

[mhsdesign]

this MUST not be used as a custom repoint via ContentStreamIdOverride would be ignored.
We possibly have to pass both as touple and create a new ContentGraph by hand.

[mhsdesign]

i guess adding the workspace to the AssetUsage will not be trivial, so we should just use the workspace finder beforehand and get a workspace that will be internally resolved again to a content stream?

[mhsdesign]

this is illegal as well because of ContentStreamIdOverride.

[mhsdesign]

Thanks for all the discussion here, but this branch is super outdated. I cherry picked all the changes to a new pr and we can continue on a blank page: #5042