Skip to content

Merge-up: Merge v17/dev into main - #24156

Open
iOvergaard wants to merge 3 commits into
mainfrom
v18/task/merge-from-17
Open

iOvergaard wants to merge 3 commits into
mainfrom
v18/task/merge-from-17

Conversation

@iOvergaard

Copy link
Copy Markdown
Contributor

Description

Merges v17/dev into main. It brings two PRs:

1. ContentTypeServiceBase.ComposeContentTypeChanges

main has reworked change detection into granular change types (PropertyAdded, AliasChanged, …). The resolution keeps that model for the saved content type. It replaces the old one-level GetComposedOf propagation with #24126's transitive one (GetDirectReferencingTypes / GetComposedOfTransitive, with the lookup built once per batch):

Question: should deriving types get the granular PropertyAdded instead of a plain RefreshOther? Nothing in core checks PropertyAdded specifically, so for core they behave the same, and I kept #24126's choice.

2. ContentCacheRefresher

#24126 adds a constructor taking IPublishedContentTypeCache. On 18 that constructor has the same length as the obsolete one taking both IPublishStatusManagementService and IDocumentPublishStatusManagementService. Both can be satisfied by the container, so it throws "constructors are ambiguous" at startup. I tried this: 187 integration tests failed. [ActivatorUtilitiesConstructor] doesn't help, because the container resolves the refresher by type.

So on main the constructors are unchanged, and the cache is resolved lazily through StaticServiceProvider the first time a "refresh all" needs it, with a TODO (V19). On v19/dev, which has a single constructor, it will be injected normally when this goes up.

3. Tests adapted to 18

  • DocumentHybridCacheDocumentTypeTests.Batch_Save_Reports_A_Single_Change_For_A_Type_Deriving_From_Several_Saved_Types used the synchronous batch Save, which doesn't exist on 18. It now saves each composition with UpdateAsync and classifies both in one ComposeContentTypeChanges call, the same pattern as the existing alias-rename test in that file. The assertion is unchanged.
  • ContentCacheRefresherIdKeyMapTests goes back to main's constructor.
  • In Content Types: Refresh composing types when a property is added to a composition (closes #24117) #24126's three new ContentTypeEditingServiceTests, the saved content type now expects main's granular flags (PropertyAdded, PropertyVariationChanged, PropertyRemoved | RawDataUnaffected) instead of v17's RefreshOther / RefreshMain. The deriving types' expectations are unchanged and pass as written.

How to test

All of the following pass:

  • Unit tests: ContentCacheRefresher*, EnsureMinimumResponseTimeFilterTests, UriProviderTests.
  • Integration tests: PublishedContentTypeCacheTests, ReloadPublishedCacheTests, ContentTypeEditingServiceTests, DocumentHybridCacheDocumentTypeTests, UserServiceCrudTests, ContentTypeServiceTests.

Merge with a merge commit (not squash), so v17/dev stays an ancestor of main.

🤖 Generated with Claude Code

AndyButland and others added 3 commits October 9, 2026 12:05
…composition (closes #24117) (#24126)

* Refresh composing content types when a property is added.

Adding a property to a content type used as a composition left every type
composed of (or inheriting from) it serving a published content type built
before the property existed, so the value read as null on the front end and
in Preview until the application restarted.

ComposeContentTypeChanges only propagated to deriving types for changes in
hasPropertyMainImpact, which never included a property addition. The deriving
types therefore got no change entry at all, and neither the published content
type cache nor the converted content cache was ever cleared for them.

Propagation now also runs for a property addition, emitting RefreshOther for
the deriving types - enough to clear both caches, without the cmsContentNu
rebuild a pure addition does not need. The traversal is also made transitive
(GetComposedOfTransitive), so a change reaches types further down an
inheritance or composition chain rather than only the direct consumers.

Reloading the published caches now clears IPublishedContentTypeCache as well,
so "Reload Memory Cache" can recover a site left in this state instead of the
rebuilt content being projected through the same stale definitions.

* fix(core): propagate an alias change to deriving types, and address review

A content type's published content type resolves its composition aliases
recursively through its compositions, so renaming a content type left every
type deriving from it reporting the old alias - breaking IsComposedOf on the
front end and CompositionSchemaIds in the Delivery API - until another refresh
or a restart. hasAliasChanged now propagates too; deriving types take
RefreshOther, since the stored blob is keyed by property alias rather than
content type alias and so needs no rebuild.

GetComposedOfTransitive built its reverse edges by rescanning every content
type for each type it found, making a base-type save O(types x descendants).
It now builds the reverse lookup once, as GetPropertyAliasesReservedByDescendants
does, so the traversal is O(types + composition edges).

Also: name the target in both ContentCacheRefresher obsolete messages so the
migration path is unambiguous when two of them are present, and assert in
ReloadPublishedCacheTests that a cache hit re-serves the same instance, so the
reference comparison after the reload cannot pass vacuously.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Include a few more test scenarios for chained inheritance

* For batched updates: Only load the content types once, and ensure that multiple saves (in)directly affecting the same content type only yield a single change

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: kjac <kja@umbraco.dk>
…r lookup and always apply the minimum response time (#24152)

* fix(api): report an unconfigured application URL before the password reset user lookup

ApplicationUrlNotConfigured was only produced once the user had been found,
so the reset endpoint answered a registered email differently from an
unknown one on sites where the application URL could not be resolved.

Add IForgotPasswordUriProvider.CanCreateForgotPasswordUriAsync for failures
that don't depend on the user, and check it before the lookup. The default
implementation succeeds, so existing providers are unaffected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(api): always apply the minimum response time on password reset endpoints

The filter checked TimeSpan.Microseconds, which is only the 0-999 µs
component of the remaining wait, so the delay was skipped whenever that
component happened to be zero. Compare the whole TimeSpan instead, and
measure with TimeProvider so the behaviour can be tested deterministically.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Added docs for EnsureMinimumResponseTimeFilter.

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Andy Butland <abutland73@gmail.com>
Resolves #24126 against main:

- ComposeContentTypeChanges keeps main's granular change types for the saved
  content type, and takes #24126's transitive propagation (property additions
  and alias changes reach every deriving type) in place of the one-level
  GetComposedOf propagation.
- ContentCacheRefresher resolves IPublishedContentTypeCache lazily: a new
  constructor taking it would be ambiguous for the container next to the
  obsolete constructor taking both publish status services.
- #24126's tests are adapted to 18: no synchronous batch Save, the original
  constructor, and main's granular flags for the saved content type.
Copilot AI balanced review requested due to automatic review settings October 9, 2026 12:55
@iOvergaard
iOvergaard requested a review from AndyButland October 9, 2026 12:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @iOvergaard's task in 1m 12s —— View job


PR Review

Target: origin/main · Based on commit: 643da950

Merge-up of v17/dev into main. It brings in #24152 (password reset: config checked before the user lookup, minimum response time always applied) and #24126 (deriving content types refreshed transitively when a composition changes), with conflicts resolved against main's granular ContentTypeChangeTypes.

  • Modified public API: IForgotPasswordUriProvider.CanCreateForgotPasswordUriAsync(), a new member with a default implementation and a TODO (V19) (§6.3 pattern).
  • Other changes:
    • Forgot-password returns ApplicationUrlNotConfigured before the user lookup, so the result is the same for every email.
    • The minimum response time is now applied even when the remaining wait is under 1 µs (fixes the old forceWait.Microseconds > 0 component check).
    • Adding a property, or changing a content type's alias, now sends RefreshOther to all types deriving from it, at any depth.
    • Main-impact changes now propagate RefreshMain transitively instead of one level deep.
    • A "refresh all" ContentCacheRefresher payload now clears IPublishedContentTypeCache.

Resolution check

  • ComposeContentTypeChanges: The resolution is correct.
    • The saved type keeps main's granular flags.
    • When a main-impact change propagates, deriving types still join rebuildRequiredIds / rawDataUnaffectedCandidateIds, so the Id-keyed RawDataUnaffected resolution at the end still covers them.
    • The lookup is built once per batch, which is cheaper than main's per-type GetComposedOf → GetAll().
    • Moving the propagation below the non-structural block is safe, because AddChange ORs the flags.
  • On question 1 (PropertyAdded vs RefreshOther for deriving types): Nothing under src/ checks PropertyAdded, AliasChanged, CompositionAdded or PropertyAliasChanged outside the producer, so the two are equivalent for core.
  • ContentCacheRefresher: The lazy StaticServiceProvider resolution with a TODO (V19) is a pragmatic way around the constructor ambiguity, and it is only reached on RefreshAll.
    • None of the unit tests that construct the refresher send a RefreshAll payload, so they don't depend on StaticServiceProvider.
    • ContentTypeCacheRefresher / DataTypeCacheRefresher already inject the same cache, so the service is always registered.
  • Tests: The adapted expectations match main's semantics.
    • The new ReloadPublishedCacheTests sidesteps the no-op messenger false-green (per tests/Umbraco.Tests.Integration/CLAUDE.md).
    • It also checks that a cache hit returns the same instance, so the "not same" assertion actually tests the clear.

No Critical, Important or Suggestion findings, so there are no inline comments.

Labels: I added category/api because Management API files changed. area/backend was already on the PR.


Approved

This looks good to be merged as-is, but please do a manual sanity check and testing before merging. (Use a merge commit, as noted in the description.)

@claude claude Bot added the category/api label Oct 9, 2026
@sonarqubecloud

sonarqubecloud Bot commented Oct 9, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Property added to a composition reads as empty on composing document types until restart (ModelsMode Nothing)

3 participants