Repository navigation
Content Types: Fix stale published cache when property variance changes without content type variance changing - #23445
Conversation
…content type's doesn't ContentTypeBase never cascaded ResetDirtyProperties(bool) into its property groups/types, so a property's dirty snapshot survived every cache round-trip and permanently defeated WasPropertyTypeVariationChanged's new-property guard for any reload-then-edit flow, silently misclassifying the change as non-structural and leaving the published cache stale.
ad09eeb to
18fe8c4
Compare
|
Claude finished @lauraneto's task in 7m 19s —— View job PR ReviewTarget: Fixes a missing
Important
Suggestions
Approved with Suggestions for improvementThe production fix is correct and well-targeted. The tests do validate the dirty-tracking contract and demonstrate 2 of 4 failing before the fix. The suggestions above are polish items that don't block merging. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a dirty-tracking contract gap in ContentTypeBase where the repository cache path calls ResetDirtyProperties(bool rememberDirty), but the type previously only cascaded resets via the parameterless overload. As a result, nested PropertyTypes could remain permanently dirty after round-trips, leading to property-variance changes being misclassified and the published cache potentially serving stale values.
Changes:
- Add
ContentTypeBase.ResetDirtyProperties(bool rememberDirty)override that cascades dirty-reset intoPropertyGroupsandPropertyTypes(and remove reliance on the parameterless override). - Add integration tests covering published output behavior when property variance changes without republishing.
- Wire required distributed-cache notification handlers + local server messenger for the new integration tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Umbraco.Core/Models/ContentTypeBase.cs |
Fixes dirty-reset cascading for the overload actually used by repository cache round-trips. |
tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentHybridCacheVarianceTests.cs |
Adds regression coverage for published output after property-variance changes (no republish). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Force a real repository round-trip before mutating variance so the dirty-tracking contract is actually exercised, assert CreateAsync/GetAsync results instead of discarding or null-forgiving them, rename tests to the Can_/Cannot_ convention used elsewhere in the file, and parameterize the fixture over NoCache and a real DeepCloneAppCache so the regression is proven under both, since a previous version of this bug was only reproducible with caching disabled.
There was a problem hiding this comment.
Looks good to me @lauraneto. Even if it's not reproducible in production today, I read this as a genuine oversight and a correct fix. ContentBase/Content already override that overload and cascade correctly, so it's good to align ContentTypeBase. And if the cache implementation ever changes, this would stops the bug from surfacing.
It looked like we could verify cheaply with a plain unit test on the model itself, so I've added a few cases to ContentTypeTests and checked them in. They assert the reset cascades into every property type (both grouped and un-grouped) and that the rememberDirty flag is threaded through correctly. I confirmed they fail on v17/dev before the fix and pass with it.
Worth keeping your integration tests too though as they cover a broader end-to-end path that the unit test doesn't.
|
Summary
ContentTypeBaseonly overrode the parameterlessResetDirtyProperties(), not theResetDirtyProperties(bool rememberDirty)overload the repository cache actually calls on every cache round-trip. That overload fell back to the non-cascading base implementation, so property types never had their dirty flags reset — permanently defeatingWasPropertyTypeVariationChanged()'s new-property guard. The result: if only a property's variance changed (not the content type's own), the change was misclassified as non-structural and the published cache could keep serving the stale value.Fix: add the missing
ResetDirtyProperties(bool rememberDirty)override, cascading intoPropertyGroups/PropertyTypesthe same way the parameterless one did. Drop the now-redundant parameterless override, sinceBeingDirtyBase.ResetDirtyProperties()already delegates toResetDirtyProperties(true).Caveat found during review: the new tests only catch this because the integration harness runs with
AppCaches.NoCacheby default. With real caching (AppCachesusingDeepCloneAppCache, matching any actual deployment), the tests pass even without this fix —DeepCloneAppCachedeep-clones and resets dirty state on every cache write, independently masking the missing cascade. We could not reproduce the staleness manually against a running instance either. So this is a real gap in the dirty-tracking contract and a correct fix, but its real-world reachability under normal caching is unconfirmed — flagging for you to judge whether that changes scope or priority.Test plan
DocumentHybridCacheVarianceTests.cshas 4 tests covering property-variance changes with the content type's own variance held constant. Run them againstv17/dev(before this fix) — 2 of the 4 fail. Run them again with this branch — all 4 pass.