Published Cache: Stop writing unread member rows to the database cache (closes #23070) - #23730
Conversation
|
Claude finished @AndyButland's task in 4m 2s —— View job PR ReviewTarget: Removes the dead write path that serialised member rows into
Suggestions
ApprovedThorough, well-scoped removal of confirmed dead code. The public API handling is correct — |
There was a problem hiding this comment.
Pull request overview
This pull request stops full published-cache rebuilds from writing unused member rows to cmsContentNu, while preserving entity-based member mapping and API compatibility.
Changes:
- Removes member serialization and database-cache rebuild work.
- Retains
IMemberCacheService.Rebuildas an obsolete no-op. - Updates rebuild contracts, callers, tests, and documentation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Summary |
|---|---|
tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/MemberCacheServiceTests.cs |
Verifies member rows are not created or retained after full rebuilds. |
tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentCacheServiceTests.cs |
Updates full-rebuild documentation coverage. |
src/Umbraco.PublishedCache.HybridCache/Services/MemberCacheService.cs |
Removes database-cache dependencies and rebuild logic. |
src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs |
Adapts to the simplified repository signature. |
src/Umbraco.PublishedCache.HybridCache/Services/IMemberCacheService.cs |
Documents entity-based member mapping and retains the obsolete no-op rebuild. |
src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs |
Adapts to the simplified repository signature. |
src/Umbraco.PublishedCache.HybridCache/Persistence/IDatabaseCacheRepository.cs |
Removes member rebuild parameters and overloads. |
src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs |
Removes member serialization and rebuild processing. |
src/Umbraco.PublishedCache.HybridCache/DatabaseCacheRebuilder.cs |
Rebuilds only documents and media. |
src/Umbraco.PublishedCache.HybridCache/CLAUDE.md |
Documents that members are mapped rather than database-cached. |
Suppressed comments (2)
src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs:93
- The regression tests configure
NuCacheSerializerType.JSON, whose factory ignores the entity flags, so they do not exercise thisMember-flag removal in the MessagePack path where it triggers_memberTypeService.GetAll(). A future reintroduction of that lookup would leave the tests green; add a MessagePack full-rebuild case or an assertion/spy proving member types are not loaded.
| ContentCacheDataSerializerEntityType.Media);
src/Umbraco.PublishedCache.HybridCache/Services/IMemberCacheService.cs:15
- There is no lookup or not-found result in this API: every non-null
IMemberis mapped, and the implementation's only null path is a null runtime argument. The return documentation should reflect that behavior instead of implying that an entity may be absent from the cache.
/// <returns>The published member, or <c>null</c> if not found.</returns>
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Andy Butland <abutland73@gmail.com>
…b.com/umbraco/Umbraco-CMS into v17/bugfix/23070-stop-caching-members
|
Zeegaan
left a comment
There was a problem hiding this comment.
Seems like a reasonable change for me 🤔



Description
Members are written into
cmsContentNuby a full published-cache rebuild, but nothing ever reads those rows. This PR removes the write, and fixes a pre-existing bug that the removal exposed.Fixes #23070.
Why this is justified
Nothing reads member rows.
MemberCache.Get(IMember)goes toMemberCacheService.Get, which callsIPublishedContentFactory.ToPublishedMemberand maps the suppliedIMemberentity on the fly.DatabaseCacheRepositoryis the only class in the codebase that queriescmsContentNu, and its only member-scoped code was the write. There is no second reader anywhere, including the ExamineMembersIndexwhich populates fromIMemberService.Digging through the history suggests this is an unfinished or abandoned feature. The comment in
PublishedContentFactory.ToPublishedMemberhas said so since the first HybridCache commit:IPublishedMemberexposes the underlyingIMember, which cannot be reconstructed from a cache row, so the read path needs the entity regardless. That removes the benefit a cached row could provide, which is presumably why the read side was never wired up — even the original 2016 NuCacheMemberCachetookIMemberServicerather than the data source.The rows are also stale, not merely unused. Up to v14 the write side was self-consistent:
PublishedSnapshotServiceEventHandlerhandledMemberRefreshNotificationto refresh a member's row on save, and member-type changes triggered a rebuild. Removing NuCache in v15 took both with it, and HybridCache never replaced them. Since then the only thing writing member rows has been the bulk rebuild, so a row reflects whatever the last full rebuild happened to see and nothing else.Effect. A full rebuild — the Settings → Published status button, the post-migration rebuild, and the boot-time serializer-changed rebuild — no longer serialises and inserts a row per member. On a site with a lot of members, that is a considerable amount of pointless work removed from every rebuild, inside the single transaction the rebuild holds.
No longer requesting the
Memberserializer flag also drops a_memberTypeService.GetAll()from every rebuild, and makes the corresponding branch inMsgPackContentNestedDataSerializerFactoryunreachable — that branch and its injectedIMemberTypeServicego too. The publicContentCacheDataSerializerEntityType.Memberenum value stays, since it is in the package baseline.Why not the change requested in the issue
The issue asks for a setting to disable member caching, or for
IDatabaseCacheRepository/DatabaseCacheRepositoryto become public and unsealed so the rebuild can be overridden as it could in v13. Neither is needed if the work simply isn't done, and a setting would only be a flag over dead code. Making the repository public would also commit us to a large surface for a major, to work around a defect.Existing rows
Not migrated. I considered adding a 17.7/18.2 migration step for this, but it doesn't seem necessary. Other than taking space, the stale records don't really do any harm — they're unread, and
cmsContentNu.nodeIdhas anON DELETE CASCADEforeign key toumbracoContent, so a deleted member takes its row with it.A full rebuild clears the table before repopulating, so that removes them — though only thanks to the fix below, without which it would never have happened on SQL Server. Worth being precise in the release note: an upgrade won't necessarily perform a full rebuild, because the post-migration rebuild only runs when an executed migration sets
RebuildCache, and targeted per-content-type rebuilds never truncate. So the rows can outlive several upgrades until someone rebuilds.Fix to a pre-existing bug in the rebuild's table clear
In local testing, a full rebuild left member rows behind on SQL Server. The method that clears the table was a silent no-op there:
Both are reference comparisons against NPoco's singleton instances. On SQL Server,
SqlServerSyntaxProvider.GetUpdatedDatabaseTypereturnsUmbracoSqlServerDatabaseType— a subclass ofSqlServer2012DatabaseType, added so that bulk inserts keep foreign key constraints trusted. Being a subclass, it is not the singleton, so neither branch matched and nothing was executed.That went unnoticed because each rebuild arm already deletes its own rows via
RemoveByObjectTypeInBatchesbefore repopulating, leaving this as a pure optimisation whose failure had no visible effect. Removing the member arm is what made it load-bearing: with nothing else deleting member rows, they would have survived every rebuild on SQL Server.It is now one provider-agnostic statement, renamed to say what it does:
DELETErather thanTRUNCATEis deliberate.TRUNCATEhas never actually executed on SQL Server previously, so deleting is what SQL Server already does today and introduces no new permission requirement on a backoffice action.Also included
SqliteSyntaxProvidernow overridesTruncateTableasDELETE FROM {0}. Nothing in production callsDatabase.TruncateTable, but the base provider's format isTRUNCATE TABLE {0}and SQLite has no such statement, so the extension would have handed invalid SQL to the first caller that tried it. Happy to drop this if it feels like scope creep, but it seemed worth closing while it was in view.Testing
Automated
Three integration tests in
MemberCacheServiceTestsreplace the two that asserted the removed behaviour.Although now removed from the call site,
TruncateTableTestscoversDatabase.TruncateTablethe fix to truncating tables.Manual
Clear the member data from the cache with a rebuild from the backoffice via Settings > Published Status > Rebuild database cache. Verify no member records exist via:
Then render a member in a template and confirm the values still resolve — including a custom property, which is the part that used to be serialised into the row. Built-in fields such as
EmailandUserNamecome off the member entity's own columns and would work either way.Template code
Merge notes
Umbraco 18 has a four-arm rebuild — content, media, member, element — and element rows are read, so the merge must drop only the member arm. Two specifics:
maincovers four collections, so it needs the same reduction there rather than taking this branch's two-collection version wholesale.