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

FEATURE: AssetUsage as CatchUpHook #5258

Merged
merged 12 commits into from
Oct 1, 2024

Conversation

dlubitz
Copy link
Contributor

@dlubitz dlubitz commented Sep 23, 2024

This is an overhaul of the latest "asset usage" implementation which where using a dedicated projection to keep track of asset usages. As this was not handling all necessary events we tried to improve this, but got stuck due to the need of a lot of information of the graph, workspaces, etc (see #5109).

Therfore we decided to try a new approach to use a CatchUpHook instead, which enables us to access all needed information without re-implementing the whole graph logic.

General approach
The package stores the usage of an asset per node and property in a dimension space point. It only stores the usage in the highest/base workspace (usually "live") as long as there are no changes in the same property in a lower/dependent workspace (e.g. user workspace). This allows to get easily an answer to the following questions:

  1. Which nodes do I need to edit, to remove all usages for a safe asset removal (Neos Asset Usage/Media Browser)
  2. Which cache entries do I need to flush on a change to an asset (this requires an additional traversal over all dependent workspaces).

CatchUpHook
The CatchUpHook is keeping track of all changes to node properties referencing an asset. On replay the CatchUpHook is not handling the events for performance reasons.

Indexing Command
The indexing command fills up the index initially. It can also be used to rebuild the index after projection replays or imports of events.

./flow assetusage:index --contentrepository default

Fixes: #4808 #5084

@@ -244,6 +244,7 @@ jobs:
touch ${{ github.workspace }}/Data/DebugDatabaseDumps/keep

./flow package:list --loading-order
FLOW_CONTEXT=Testing/Behat ./flow doctrine:migrate --quiet
Copy link
Member

Choose a reason for hiding this comment

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

(how) is that related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behat tests are not working without this change, as the CatchUpHook expects the doctrine:migrate command to be executed. But it's not when we run the Neos.ContentRepository.BehavioralTests.

This was not an issue before, as these tests where only relying on the projection tables. But the new CatchUpHook now, gets executed also in these test cases.

https://github.com/neos/neos-development-collection/blob/9.0/.composer.json#L34-L41

Copy link
Member

Choose a reason for hiding this comment

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

That is our long standing: you have to do doctrine migrate sometimes problem otherwise it will not work problem that youll fix with the catchup thing as far as i know. see slack: https://neos-project.slack.com/archives/C04PYL8H3/p1727209924914109?thread_ts=1727117857.409749&cid=C04PYL8H3

@bwaidelich
Copy link
Member

Works like a charm, thanks for the hard work!!
Just a couple of comments, nothing severe!

@skurfuerst skurfuerst merged commit 2a18885 into neos:9.0 Oct 1, 2024
9 checks passed
@mhsdesign
Copy link
Member

Post merge + 0.5 i just skimmed through most places thats why no +1 xD

@dlubitz dlubitz deleted the 90/feature/asset-usage-again branch October 1, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants