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.

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

Workspace aware ContentGraph / Node followup #5038

Closed
6 of 15 tasks
mhsdesign opened this issue May 9, 2024 · 3 comments
Closed
6 of 15 tasks

Workspace aware ContentGraph / Node followup #5038

mhsdesign opened this issue May 9, 2024 · 3 comments
Labels

Comments

@mhsdesign
Copy link
Member

mhsdesign commented May 9, 2024

Followup for #5028

Things for #5042

@mhsdesign mhsdesign changed the title Workspace aware ContentGraph followup Workspace aware ContentGraph / Node followup May 13, 2024
@mhsdesign
Copy link
Member Author

older notes just for reference :)

  • Userland should not need to operate on cs ids
    -> Events must operate on cs ids
  • getSubgraph arguments
  • NodeFactory::mapNodeRowToNode must accept WorkspaceName and ContentStreamId from outside.
  • => Extend SubgraphIdentity
  • 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:”)

further todos from christian an me:

  • 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 workspace 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

bwaidelich added a commit that referenced this issue May 25, 2024
Previously the `DoctrineDbalContentGraphProjection` accessed the `workspace` table of a different projection in order to resolve the workspace<->content stream mapping.

This change adds the `workspace` table to the content graph projection and uses that instead for the resolution.

**Note:** This is not a breaking change because it comes with a migration and does not require a replay, but a
    ./flow cr:setup
is needed in order to apply that!

Related: #5038
@mhsdesign mhsdesign added the 9.0 label Jun 16, 2024
mhsdesign pushed a commit that referenced this issue Oct 1, 2024
Previously the `DoctrineDbalContentGraphProjection` accessed the `workspace` table of a different projection in order to resolve the workspace<->content stream mapping.

This change adds the `workspace` table to the content graph projection and uses that instead for the resolution.

**Note:** This is not a breaking change because it comes with a migration and does not require a replay, but a
    ./flow cr:setup
is needed in order to apply that!

Related: #5038
@kitsunet
Copy link
Member

What is still missing for this now?

@mhsdesign
Copy link
Member Author

Indeed most things are resolved with the merge of #5096 and the leftovers are either outdated or will be fixed via #5272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants