BUGFIX: Flush node caches when asset has changed #4788

dlubitz · 2023-12-01T13:29:56Z

Clears the caches when asset has changed.

Also removed adding the "AssetDynamicTag" cache tag, as the occurance of assets within texts is also covered by new AssetUsage projection:

https://github.com/neos/neos-development-collection/blob/207a86d9025ff26c669c678f1[…]eos.Neos/Classes/AssetUsage/Projection/AssetUsageProjection.php

Added behat tests for content cache flushing on asset changes and also for content cache flushing on node and convertUri changes.

Fixes: #4707

dlubitz · 2023-12-01T14:25:52Z

Linting errors are unrelated to this PR, they are already in 9.0 branch

dlubitz · 2023-12-05T11:28:14Z

We need to check if this also works for Nodes that are not cached directly. E.g some uncached elements in a cached collection.

dlubitz · 2023-12-15T17:20:26Z

@kitsunet I fixed the parent-propagation of the cache flush. But it's still broken, as the AssetUsageStrategie will atm just return on node of a random contentstream. We need to fix this separately as this also effects the media module.

…flush

…st suite

bwaidelich

Love it, +1 by 👁️
Just a mini nitpick-comment. Feel free to ignore :)

Neos.Neos/Classes/Fusion/Cache/ContentCacheFlusher.php

… to private

mhsdesign · 2024-05-06T13:34:05Z

Neos.Neos/Tests/Behavior/Features/Bootstrap/FeatureContext.php

+    protected function deserializeProperties(array $properties): PropertyValuesToWrite
+    {
+        $properties = array_map(
+            $this->loadFlowObjectsRecursive(...),
+            $properties
+        );
+
+        return $this->deserializePropertiesCrTestSuiteTrait($properties);
+    }
+
+    public function loadFlowObjectsRecursive(mixed $value): mixed
+    {
+        if (is_array($value) && isset($value['__flow_object_type'])) {
+            return $this->persistenceManager->getObjectByIdentifier($value['__identifier'], $value['__flow_object_type'], true);
+        } elseif (is_array($value)) {
+            return array_map(
+                $this->loadFlowObjectsRecursive(...),
+                $value
+            );
+        }
+        return $value;
+    }


nice trick to fully allow the flow object syntax {"__flow_object_type":"Neos\\Media\\Domain\\Model\\Asset","__identifier":"an-asset-to-change"}

i would slightly prefer the use of a custom synax to not confuse it with the final serialization format like

{ "uriPathSegment": "a1-1", "title": "Node a1-1", "asset": "Asset:an-asset-to-change" }

like we do in

neos-development-collection/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/02-NodeCreation/05-CreateNodeAggregateWithNode_ComplexDefaultAndInitialProperties.feature

Lines 98 to 115 in d0f4ecc

Scenario: Create a node aggregate with complex initial and default values

When the command CreateNodeAggregateWithNode is executed with payload:

| Key | Value |

| nodeAggregateId | "nody-mc-nodeface" |

| nodeTypeName | "Neos.ContentRepository.Testing:Node" |

| parentNodeAggregateId | "lady-eleonode-rootford" |

| initialPropertyValues | {"dayOfWeek":"DayOfWeek:https://schema.org/Friday","postalAddress":"PostalAddress:anotherDummy", "date":"Date:2021-03-13T17:33:17+00:00", "uri":"URI:https://www.neos.io", "price":"PriceSpecification:anotherDummy"} |

And the graph projection is fully up to date

Then I expect a node identified by cs-identifier;nody-mc-nodeface;{} to exist in the content graph

And I expect this node to have the following properties:

| Key | Value |

| dayOfWeek | DayOfWeek:https://schema.org/Friday |

| array | {"givenName":"Nody", "familyName":"McNodeface"} |

| postalAddress | PostalAddress:anotherDummy |

| now | Date:now |

| date | Date:2021-03-13T17:33:17+00:00 |

| uri | URI:https://www.neos.io |

| price | PriceSpecification:anotherDummy |

but this is certainly more versatile and cool :D

I was thinking about that.... But why creating a new syntax if we already have one? And how would you handle arrays of assets?

{ "uriPathSegment": "a1-1", "title": "Node a1-1", "assets": "Assets:an-asset-to-change,some-other-asset" }

yeah exactly that are the things that speak against this ;) only by using a know serialisation format one might think by reading the tests that we write directly to the db which is the opposite.

In the end its fine either way, we have tests now so 🥳

But why creating a new syntax if we already have one?

The one we have is an implementation detail of the Flow persistence really.
Since Behat tests are also meant to be read by humans, I agree to @mhsdesign

And the following CreateNodeAggregateWithNode commands are executed: | nodeAggregateId | parentNodeAggregateId | nodeTypeName | initialPropertyValues | nodeName | | a | root | Neos.Neos:Site | {} | site | | a1 | a | Neos.Neos:Test.DocumentType1 | {"uriPathSegment": "a1", "title": "Node a1", "asset": {"__flow_object_type":"Neos\\Media\\Domain\\Model\\Asset","__identifier":"an-asset-to-change"}} | a1 | | a1-1 | a1 | Neos.Neos:Test.DocumentType1 | {"uriPathSegment": "a1-1", "title": "Node a1-1", "assets": [{"__flow_object_type":"Neos\\Media\\Domain\\Model\\Asset","__identifier":"an-asset-to-change"}]} | a1-1 | | a1-2 | a1 | Neos.Neos:Test.DocumentType1 | {"uriPathSegment": "a1-2", "title": "Node a1-2", "asset": {"__flow_object_type":"Neos\\Media\\Domain\\Model\\Asset","__identifier":"some-other-asset"}} | a1-2 | | a2 | a | Neos.Neos:Test.DocumentType2 | {"uriPathSegment": "a2", "title": "Node a2", "text": "Link to asset://an-asset-to-change."} | a2 | | a3 | a | Neos.Neos:Test.DocumentType2 | {"uriPathSegment": "a2", "title": "Node a2", "text": "Link to asset://some-other-asset."} | a3 |

is much harder to read than

And the following CreateNodeAggregateWithNode commands are executed: | nodeAggregateId | parentNodeAggregateId | nodeTypeName | initialPropertyValues | nodeName | | a | root | Neos.Neos:Site | {} | site | | a1 | a | Neos.Neos:Test.DocumentType1 | {"uriPathSegment": "a1", "title": "Node a1", "asset": "Asset:an-asset-to-change"} | a1 | | a1-1 | a1 | Neos.Neos:Test.DocumentType1 | {"uriPathSegment": "a1-1", "title": "Node a1-1", "assets": ["Asset:an-asset-to-change"]} | a1-1 | | a1-2 | a1 | Neos.Neos:Test.DocumentType1 | {"uriPathSegment": "a1-2", "title": "Node a1-2", "asset": "Asset:some-other-asset"} | a1-2 | | a2 | a | Neos.Neos:Test.DocumentType2 | {"uriPathSegment": "a2", "title": "Node a2", "text": "Link to asset://an-asset-to-change."} | a2 | | a3 | a | Neos.Neos:Test.DocumentType2 | {"uriPathSegment": "a2", "title": "Node a2", "text": "Link to asset://some-other-asset."} | a3 |

But obviously this could be done in a follow-up

Another point that speaks for this is that when its easier to find where the custom Asset:an-asset-to-change logic is implemented. As theres likely a regex or string reference to /Asset:[0-9a-z-]+/ in the behat code base.

The way i see it the ClassName:identifier is our special syntax that means insert a real php object instance in here, but i can only use json in behat ^^

so +1 for a followup ;)

Ok ok, I'll provide a change for this.

mhsdesign · 2024-05-06T13:34:31Z

Neos.Neos/Tests/Behavior/Features/Bootstrap/FeatureContext.php

+        return $this->deserializePropertiesCrTestSuiteTrait($properties);
+    }
+
+    public function loadFlowObjectsRecursive(mixed $value): mixed


Suggested change

public function loadFlowObjectsRecursive(mixed $value): mixed

private function loadFlowObjectsRecursive(mixed $value): mixed

dlubitz · 2024-05-06T13:40:06Z

Ooopsi, you are quite fast... I'll change at least the visibility of loadFlowObjectsRecursive in a new PR

mhsdesign · 2024-05-06T13:41:33Z

no dont worry i deliberately didnt wait as this is just testing infrastructure, its purely cosmetic :D

bwaidelich · 2024-05-06T15:54:08Z

no dont worry i deliberately didnt wait as this is just testing infrastructure, its purely cosmetic :D

Come on. You suggest a change and then ignore your own suggestion?

dlubitz added the 9.0 label Dec 1, 2023

dlubitz self-assigned this Dec 1, 2023

crydotsnake added the Bug label Dec 2, 2023

crydotsnake changed the title ~~BUGFIX Flush node caches when asset has changed~~ BUGFIX: Flush node caches when asset has changed Dec 2, 2023

kitsunet approved these changes Dec 4, 2023

View reviewed changes

dlubitz requested review from bwaidelich and mhsdesign December 5, 2023 11:05

mhsdesign marked this pull request as draft January 14, 2024 15:30

bwaidelich removed their request for review January 17, 2024 14:53

mhsdesign removed their request for review February 15, 2024 10:29

dlubitz force-pushed the 90/bugfix/asset-cache-flush branch 2 times, most recently from 30abcb3 to b994e5f Compare March 16, 2024 08:54

dlubitz added 2 commits March 17, 2024 10:41

BUGFIX Flush cache when asset has changed

23dd565

BUGFIX Flush cache when asset has changed

b353adc

dlubitz force-pushed the 90/bugfix/asset-cache-flush branch from 48af637 to b353adc Compare March 17, 2024 09:44

dlubitz added 12 commits March 17, 2024 13:12

BUGFIX Flush cache when asset has changed

b8c7bcf

BUGFIX Flush cache when asset has changed

e2e3ab3

BUGFIX Flush cache when asset has changed

495183e

Merge remote-tracking branch 'origin/9.0' into 90/bugfix/asset-cache-…

04fceb0

…flush

BUGFIX Flush cache when a linked node has changed

b0bd1fa

BUGFIX Flush cache when asset has changed

2b54e67

Merge remote-tracking branch 'origin/9.0' into 90/bugfix/asset-cache-…

52d4c27

…flush

BUGFIX Flush cache when asset has changed

915a5ee

TASK Test handling of workspaces

f189318

TASK: Refactor ContentCacheFlusher to flush tags after persistence

2e8f97d

TASK: Add tests for content cache flush on asset changes

50d2336

TASK: Update test features, refactoring for asset handling in Neos te…

27f07e3

…st suite

dlubitz requested a review from kitsunet May 5, 2024 15:22

dlubitz marked this pull request as ready for review May 5, 2024 15:22

TASK: Update test features, refactoring for asset handling in Neos te…

6616d30

…st suite

bwaidelich approved these changes May 6, 2024

View reviewed changes

Neos.Neos/Classes/Fusion/Cache/ContentCacheFlusher.php Outdated Show resolved Hide resolved

kitsunet approved these changes May 6, 2024

View reviewed changes

TASK: Change visibility of method collectTagsForChangeOnNodeAggregate…

a24bab3

… to private

mhsdesign reviewed May 6, 2024

View reviewed changes

mhsdesign merged commit 4674309 into neos:9.0 May 6, 2024
9 checks passed

dlubitz deleted the 90/bugfix/asset-cache-flush branch May 6, 2024 16:21

mhsdesign mentioned this pull request May 9, 2024

Fully migrate ContentCacheFlusherTest to behat #5037

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: Flush node caches when asset has changed #4788

BUGFIX: Flush node caches when asset has changed #4788

dlubitz commented Dec 1, 2023 •

edited

dlubitz commented Dec 1, 2023

dlubitz commented Dec 5, 2023

dlubitz commented Dec 15, 2023

bwaidelich left a comment

mhsdesign May 6, 2024

dlubitz May 6, 2024

mhsdesign May 6, 2024 •

edited

bwaidelich May 6, 2024 •

edited

mhsdesign May 6, 2024

dlubitz May 6, 2024

dlubitz May 6, 2024

mhsdesign May 6, 2024

dlubitz commented May 6, 2024

mhsdesign commented May 6, 2024 •

edited

bwaidelich commented May 6, 2024

	Scenario: Create a node aggregate with complex initial and default values
	When the command CreateNodeAggregateWithNode is executed with payload:
	\| Key \| Value \|
	\| nodeAggregateId \| "nody-mc-nodeface" \|
	\| nodeTypeName \| "Neos.ContentRepository.Testing:Node" \|
	\| parentNodeAggregateId \| "lady-eleonode-rootford" \|
	\| initialPropertyValues \| {"dayOfWeek":"DayOfWeek:https://schema.org/Friday","postalAddress":"PostalAddress:anotherDummy", "date":"Date:2021-03-13T17:33:17+00:00", "uri":"URI:https://www.neos.io", "price":"PriceSpecification:anotherDummy"} \|
	And the graph projection is fully up to date
	Then I expect a node identified by cs-identifier;nody-mc-nodeface;{} to exist in the content graph
	And I expect this node to have the following properties:
	\| Key \| Value \|
	\| dayOfWeek \| DayOfWeek:https://schema.org/Friday \|
	\| array \| {"givenName":"Nody", "familyName":"McNodeface"} \|
	\| postalAddress \| PostalAddress:anotherDummy \|
	\| now \| Date:now \|
	\| date \| Date:2021-03-13T17:33:17+00:00 \|
	\| uri \| URI:https://www.neos.io \|
	\| price \| PriceSpecification:anotherDummy \|

	public function loadFlowObjectsRecursive(mixed $value): mixed
	private function loadFlowObjectsRecursive(mixed $value): mixed

BUGFIX: Flush node caches when asset has changed #4788

BUGFIX: Flush node caches when asset has changed #4788

Conversation

dlubitz commented Dec 1, 2023 • edited

dlubitz commented Dec 1, 2023

dlubitz commented Dec 5, 2023

dlubitz commented Dec 15, 2023

bwaidelich left a comment

Choose a reason for hiding this comment

mhsdesign May 6, 2024

Choose a reason for hiding this comment

dlubitz May 6, 2024

Choose a reason for hiding this comment

mhsdesign May 6, 2024 • edited

Choose a reason for hiding this comment

bwaidelich May 6, 2024 • edited

Choose a reason for hiding this comment

mhsdesign May 6, 2024

Choose a reason for hiding this comment

dlubitz May 6, 2024

Choose a reason for hiding this comment

dlubitz May 6, 2024

Choose a reason for hiding this comment

mhsdesign May 6, 2024

Choose a reason for hiding this comment

dlubitz commented May 6, 2024

mhsdesign commented May 6, 2024 • edited

bwaidelich commented May 6, 2024

dlubitz commented Dec 1, 2023 •

edited

mhsdesign May 6, 2024 •

edited

bwaidelich May 6, 2024 •

edited

mhsdesign commented May 6, 2024 •

edited