From 240530b553fb61a6011123fc47d181c1a94ee422 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Mon, 14 Sep 2026 14:52:19 +0200 Subject: [PATCH 1/4] fix(provisioning): a per-App settings section turned off is a change, and the plan says so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An App's Settings is the COMPLETE desired override: AppAdminService rebuilds the whole thing from it, so a null section clears that override (inherit the realm) and an empty subdomain drops the global host route. The planner, though, diffed it as merge-patch off a serialization using the API's WhenWritingNull option — which erases exactly the nulls that carry the intent. Unchecking "Dedicated subdomain for this app" therefore staged a real clear that the draft reported as NO changes at all ("22 unchanged"), while the apply went ahead and cleared it. A plan that under-reports is worse than no plan: the whole confirmation UX rests on it. SectionPolicy gains NestedReplace next to NestedPatch. A replace section is serialized a second time with nulls kept and diffed off that, so Settings.Origin.Subdomain and a switched-off Settings.Branding show up like any other change. Optionals keep their absence — the resolver suppresses a None through ShouldSerialize, independently of the ignore condition. The options instance is cached per source options (building one is expensive and the diff runs per entity). Not a regression of this wave: the apps section has been NestedPatch since #214 (the always-staged admin UI), and the apply was always right — only the preview lied. No stored draft needs fixing; the plan is recomputed on every read. Test: Turning_off_a_per_app_settings_section_shows_in_the_plan_as_the_clear_it_is — asserts the plan reports both clears, that the apply really drops the override and the host route, and that re-planning the cleared state is then genuinely unchanged. Co-Authored-By: Claude Opus 5 --- .../ColdStart/RealmManifestSectionsTests.cs | 66 +++++++++++++++++++ .../Provisioning/RealmManifestPlanner.cs | 66 ++++++++++++++++++- 2 files changed, 129 insertions(+), 3 deletions(-) diff --git a/src/dotnet/Modgud.Api.Tests/ColdStart/RealmManifestSectionsTests.cs b/src/dotnet/Modgud.Api.Tests/ColdStart/RealmManifestSectionsTests.cs index df63ef9e..cb2430a6 100644 --- a/src/dotnet/Modgud.Api.Tests/ColdStart/RealmManifestSectionsTests.cs +++ b/src/dotnet/Modgud.Api.Tests/ColdStart/RealmManifestSectionsTests.cs @@ -239,6 +239,72 @@ await InTenantAsync(factory, slug, async sp => Assert.DoesNotContain(afterPrune.ApplicationDomains, kv => kv.Value == shopAppId); } + [Fact] + public async Task Turning_off_a_per_app_settings_section_shows_in_the_plan_as_the_clear_it_is() + { + await using var host = await Fixture.CreateIsolatedHostAsync(); + var factory = host.Factory; + var ct = TestContext.Current.CancellationToken; + var applier = factory.Services.GetRequiredService(); + var planner = factory.Services.GetRequiredService(); + + const string slug = "appclear"; + var appId = new ShortGuid(Guid.NewGuid()).ToString(); + RealmManifest Manifest(ApplicationSettingsDto settings) => new() + { + Apps = + [ + new RealmManifestApp + { + Slug = "shop", Id = appId, DisplayName = "Shop", + Permissions = [new RealmManifestPermission("order", "read")], + Settings = settings, + }, + ], + }; + + Assert.False((await ProvisionRealmAsync(factory, Shell(slug), Manifest(new ApplicationSettingsDto + { + Origin = new ApplicationOriginDto { Subdomain = $"shop.{slug}.localhost" }, + Branding = new ApplicationBrandingDto { ProductName = "Shop!" }, + }), ct)).IsError); + + // What the admin UI stages when both toggles go off: the section objects are the + // COMPLETE desired override state, so a cleared subdomain is an explicit null and a + // switched-off section is a null section. Both mean "clear" to the apply — and the + // plan has to say so, or the draft looks empty and the admin applies blind. + var cleared = Manifest(new ApplicationSettingsDto + { + Origin = new ApplicationOriginDto { Subdomain = null }, + Branding = null, + }); + + var plan = await planner.PlanAsync(slug, cleared, prune: false, ct: ct); + Assert.False(plan.IsError, plan.IsError ? plan.FirstError.Description : string.Empty); + var entry = Assert.Single(plan.Value.Sections.Single(s => s.Name == "apps").Entries); + Assert.Equal("update", entry.Action); + Assert.Contains(entry.Changes, c => c.Field == "Settings.Origin.Subdomain"); + Assert.Contains(entry.Changes, c => c.Field == "Settings.Branding"); + + // …and the apply really does clear both, including the global host route. + Assert.False((await applier.UpdateRealmAsync(slug, cleared, ct: ct)).IsError); + await InTenantAsync(factory, slug, async sp => + { + var settings = await sp.GetRequiredService() + .GetAsync(new ShortGuid(appId).Guid, ct); + Assert.False(settings.IsError); + Assert.Null(settings.Value.Origin?.Subdomain); + Assert.Null(settings.Value.Branding); + }); + var realm = (await factory.Services.GetRequiredService() + .GetRealmBySlugAsync(slug, ct))!; + Assert.DoesNotContain(realm.ApplicationDomains, kv => kv.Key == $"shop.{slug}.localhost"); + + // Re-planning the cleared state is now genuinely unchanged (no phantom diff). + var again = await planner.PlanAsync(slug, cleared, prune: false, ct: ct); + Assert.Equal("unchanged", Assert.Single(again.Value.Sections.Single(s => s.Name == "apps").Entries).Action); + } + [Fact] public async Task Positions_are_feature_gated_and_import_apply_export_prune() { diff --git a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestPlanner.cs b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestPlanner.cs index 5ca97547..10d0edff 100644 --- a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestPlanner.cs +++ b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestPlanner.cs @@ -1,5 +1,7 @@ +using System.Runtime.CompilerServices; using System.Text.Json; using System.Text.Json.Nodes; +using System.Text.Json.Serialization; using ErrorOr; using Marten; using Microsoft.AspNetCore.Http.Json; @@ -141,7 +143,9 @@ List CanonApps(List apps) => [.. apps.Select KeyField = "Slug", PinnedId = a => a.Id, PinnedIdCheck = PinnedIdLookup(session, a => a.IsDeleted, a => a.Slug, ct), - NestedPatch = ["Settings"], + // REPLACE, not merge-patch: AppAdminService rebuilds the whole override from + // this object, so a null section clears it (→ inherit the realm). + NestedReplace = ["Settings"], DeleteNote = "Deleting fails at apply while the app is still referenced by a kept role, API or scope.", })); @@ -807,10 +811,18 @@ private sealed class SectionPolicy where T : class /// this entry (compared case-insensitively, matching the applier). public HashSet ImmutableFails { get; init; } = []; - /// Nested option-objects with their own patch semantics (app Settings, - /// position TerminalPolicy) — diffed recursively with dotted paths. + /// Nested option-objects with MERGE-PATCH semantics (a position's + /// TerminalPolicy) — diffed recursively with dotted paths; an absent property means + /// "unchanged", so only present ones are compared. public HashSet NestedPatch { get; init; } = []; + /// Nested objects with REPLACE semantics (an App's Settings): the + /// object is the COMPLETE desired override state, so a null section means "clear + /// it" rather than "leave it alone". Diffed like but off a + /// serialization that KEEPS nulls — otherwise turning a section off is invisible in + /// the plan while the apply happily clears it. + public HashSet NestedReplace { get; init; } = []; + /// Extra note appended to every delete candidate of the section. public string? DeleteNote { get; init; } @@ -1106,6 +1118,22 @@ private static RealmPlanEntry DiffEntry( ? null : JsonSerializer.SerializeToNode(baselineItem, json)!.AsObject(); + // A REPLACE section is serialized a second time with nulls KEPT: in the options + // above an explicit null is written as nothing at all, which is exactly right where + // absent means "unchanged" — and exactly wrong here, where it means "clear". + JsonObject? desiredKeepNulls = null, currentKeepNulls = null, baselineKeepNulls = null; + if (policy.NestedReplace.Count > 0) + { + var keepNulls = WithNulls(json); + desiredKeepNulls = JsonSerializer.SerializeToNode(item, keepNulls)!.AsObject(); + currentKeepNulls = existing is null + ? null + : JsonSerializer.SerializeToNode(existing, keepNulls)!.AsObject(); + baselineKeepNulls = baselineItem is null + ? null + : JsonSerializer.SerializeToNode(baselineItem, keepNulls)!.AsObject(); + } + var entry = new RealmPlanEntry { Key = key, Action = existing is null ? "create" : "update" }; var failed = false; @@ -1154,6 +1182,25 @@ private static RealmPlanEntry DiffEntry( // On create, an explicit null / empty list only restates the shipped default. if (currentNode is null && !Carries(value)) continue; + if (policy.NestedReplace.Contains(field)) + { + // The object itself still follows the outer contract: absent (or null) means + // "don't touch this App's override at all". Present means "this IS the + // override", and every section inside then carries — nulls included. + if (desiredKeepNulls?[field] is not JsonObject replace) continue; + if (currentNode is null) + { + entry.Changes.Add(new RealmPlanChange(field, null, replace.DeepClone())); + continue; + } + NestedPatchDiff(field, replace, currentKeepNulls?[field] as JsonObject ?? new JsonObject(), + conflictMode && baselineKeepNulls is not null + ? baselineKeepNulls[field] as JsonObject ?? new JsonObject() + : null, + entry.Changes, entry.Conflicts); + continue; + } + if (policy.NestedPatch.Contains(field)) { if (value is not JsonObject nested) continue; @@ -1238,6 +1285,19 @@ private static void NestedPatchDiff( } } + /// The diff options with WhenWritingNull lifted, for REPLACE sections + /// whose nulls are the payload. Cached per options instance — building a + /// is expensive and the diff runs per entity. + /// Optionals keep their absence: the resolver suppresses a None through + /// ShouldSerialize, independently of the ignore condition. + private static readonly ConditionalWeakTable KeepNullOptions = new(); + + private static JsonSerializerOptions WithNulls(JsonSerializerOptions json) + => KeepNullOptions.GetValue(json, source => new JsonSerializerOptions(source) + { + DefaultIgnoreCondition = JsonIgnoreCondition.Never, + }); + // ── Canonical JSON comparison — null == missing, arrays are order-insensitive // multisets, object properties with null values are treated as absent. ────────── From ef20bcc58a1f08eafe9f318a204d119cd48084c8 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Mon, 14 Sep 2026 15:21:37 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(apps):=20the=20subdomain=20field=20is?= =?UTF-8?q?=20the=20switch=20=E2=80=94=20drop=20the=20redundant=20Origin?= =?UTF-8?q?=20checkbox?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Dedicated subdomain for this app" carried no information. build() produced `Origin: { Subdomain: override ? (value || null) : null }`, so unticking the box and ticking it with an empty field were the same payload — a second clear button next to the one the input already has, and two controls for one fact can contradict each other (ticked but empty). It also greyed the input out against an "inherited" value that does not exist: a realm has a primary domain, not a subdomain to inherit. The value is the switch now: a subdomain routes that host to the app, an empty field means no own subdomain (the app is reached through the realm URL), and clearing it removes the existing route — which is what the backend already did with a null. Deliberately NOT modelled: a disabled-but-remembered subdomain. That needs a real second property, and it raises the question of whether a parked host keeps blocking the cross-realm uniqueness check while it routes nowhere. Worth deciding on its own. Co-Authored-By: Claude Opus 5 --- .../views/admin/apps/AppSettingsSections.vue | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/frontend-vue/src/views/admin/apps/AppSettingsSections.vue b/src/frontend-vue/src/views/admin/apps/AppSettingsSections.vue index a4e44845..12023abd 100644 --- a/src/frontend-vue/src/views/admin/apps/AppSettingsSections.vue +++ b/src/frontend-vue/src/views/admin/apps/AppSettingsSections.vue @@ -47,8 +47,13 @@ const loginProviderOptions = ref<{ value: string; label: string }[]>([]) // Per-section "override" toggle (on → this App overrides the realm; off → inherit). // Numbers are kept as strings (empty = inherit that field). +// +// Origin has NO toggle: the section is a single value, so a toggle would say exactly +// what an empty field already says — and two controls for one fact can contradict each +// other (ticked but empty). The subdomain IS the switch: a value routes the host, empty +// drops the route. const f = reactive({ - origin: { override: false, subdomain: '' }, + origin: { subdomain: '' }, branding: { override: false, productName: '', primaryColor: '', logoAssetId: null as string | null, logoUrl: null as string | null, @@ -139,8 +144,6 @@ const appRateLimitModeOptions = computed(() => [ const inh = computed(() => { const r = realmSettingsStore.settings return { - // No realm equivalent — inheriting means "realm primary domain / realm default". - origin: { subdomain: '' }, branding: { productName: r?.Branding?.ProductName ?? '', primaryColor: r?.Branding?.PrimaryColor ?? '', @@ -222,7 +225,7 @@ function fieldBind(section: string, field: string): any { } function resetForm() { - f.origin.override = false; f.origin.subdomain = '' + f.origin.subdomain = '' f.branding.override = false; f.branding.productName = ''; f.branding.primaryColor = '' f.branding.logoAssetId = null; f.branding.faviconAssetId = null f.branding.logoUrl = null; f.branding.faviconUrl = null @@ -253,7 +256,7 @@ function resetForm() { function populate(s?: ApplicationSettingsDto | null) { resetForm() if (!s) return - if (s.Origin) { f.origin.override = true; f.origin.subdomain = s.Origin.Subdomain ?? '' } + f.origin.subdomain = s.Origin?.Subdomain ?? '' if (s.Branding) { f.branding.override = true f.branding.productName = s.Branding.ProductName ?? '' @@ -426,10 +429,10 @@ const emailPreviewOverlay = computed(() => ({ /** Build the override DTO as the COMPLETE desired state (the App PUT is a replace): * an overridden section sends its values, a non-overridden section sends `null` so the * backend clears that override (→ inherit the realm). Origin always sends a - * section so turning the toggle off explicitly removes any existing route. */ + * section so an emptied field explicitly removes any existing route. */ function build(): ApplicationSettingsDto { return { - Origin: { Subdomain: f.origin.override ? (f.origin.subdomain.trim() || null) : null }, + Origin: { Subdomain: f.origin.subdomain.trim() || null }, Branding: f.branding.override ? { ProductName: f.branding.productName.trim() || null, @@ -614,9 +617,10 @@ watch(() => [activeTab.value, props.applicationId] as const, ([tab]) => {
- - - + + + From 6a243d0396b55ac27a3c784d300151de3289f2fe Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Mon, 14 Sep 2026 15:22:35 +0200 Subject: [PATCH 3/4] i18n(de): translate the app subdomain field, drop the removed toggle's key The Origin override checkbox is gone, so its key is dead. The field's own label had no German entry at all (it fell back to the source string, which happened to be German) and the new hint needs one. Co-Authored-By: Claude Opus 5 --- src/frontend-vue/public/i18n/de.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/frontend-vue/public/i18n/de.json b/src/frontend-vue/public/i18n/de.json index 9e2f3c05..5899cfd9 100644 --- a/src/frontend-vue/public/i18n/de.json +++ b/src/frontend-vue/public/i18n/de.json @@ -2571,7 +2571,8 @@ "admin.appSettings.inherit": "(vom Realm erben)", "admin.appSettings.posture.off": "Off — keine Selbstregistrierung", "admin.appSettings.hint": "Diese Einstellungen überschreiben die Realm-Defaults nur für diese App. Ein deaktivierter Abschnitt erbt vom Realm.", - "admin.appSettings.origin.override": "Eigene Subdomain für diese App", + "admin.appSettings.origin.subdomain": "Eigene Subdomain (Child der Realm-Primary-Domain)", + "admin.appSettings.origin.subdomainHint": "Ein Wert leitet diesen Host auf die App. Leer = keine eigene Subdomain, die App wird über die Realm-URL erreicht — das Feld zu leeren entfernt die bestehende Route.", "admin.appSettings.branding.primaryColor": "Primärfarbe (CSS)", "admin.appSettings.branding.override": "Eigenes Branding (Login/SPA)", "admin.appSettings.email.override": "Eigenes E-Mail-Branding", From f25d53b94e80c29216c73e75b1007c2f011031ae Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Mon, 14 Sep 2026 15:28:54 +0200 Subject: [PATCH 4/4] fix(apps): don't invent a ChangeFeed override just by opening the settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build() always emitted a ChangeFeed section from the reset defaults, because unlike every other section it has no "is there an override" state. Per-App settings are REPLACE, so saving any App materialized {Enabled:false, 7, 1000} on an App that had nothing — an override that says exactly what no override already says (AppChangeFeedSubscription reads a missing section as ApplicationChangeFeedSettings.Disabled). Every draft plan then listed three ChangeFeed changes nobody made, which is noise in the one place that has to be trustworthy. No new toggle: "Enable consumer change feed" already IS the switch, the same argument that just removed the Origin checkbox. The section is sent when the feed is on, or when this App already has one — the second half is what keeps a configured retention window from being dropped when an admin switches the feed off. An App that never had a feed and is not getting one now sends nothing. Verified in the browser against the dev realm: staging a branding edit on an App without a feed reports only Settings.Branding.ProductName; switching the feed off on an App with 30 days / 5000 events stages Enabled=false with both numbers intact and plans exactly one change, Settings.ChangeFeed.Enabled: true -> false. Co-Authored-By: Claude Opus 5 --- .../views/admin/apps/AppSettingsSections.vue | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/frontend-vue/src/views/admin/apps/AppSettingsSections.vue b/src/frontend-vue/src/views/admin/apps/AppSettingsSections.vue index 12023abd..97125e51 100644 --- a/src/frontend-vue/src/views/admin/apps/AppSettingsSections.vue +++ b/src/frontend-vue/src/views/admin/apps/AppSettingsSections.vue @@ -98,8 +98,14 @@ const f = reactive({ reservedNames: [] as string[], perIp: '', perRealm: '', }, cimd: { override: false, enabled: false, access: '', refresh: '' }, + // ChangeFeed has no override toggle either: "Enabled" already IS the switch — a stored + // section with Enabled=false and no section at all mean the same thing to the reader + // (AppChangeFeedSubscription falls back to ApplicationChangeFeedSettings.Disabled). + // `configured` is not a control, it remembers whether this App already HAS a section, so + // switching the feed off keeps the retention numbers instead of silently dropping them. changeFeed: { enabled: false, + configured: false, retentionAgeDays: 7 as number | null, minimumEventCount: 1000 as number | null, }, @@ -250,7 +256,8 @@ function resetForm() { f.dcr.override = false; f.dcr.enabled = false; f.dcr.access = ''; f.dcr.refresh = '' f.dcr.reservedNames = []; f.dcr.perIp = ''; f.dcr.perRealm = '' f.cimd.override = false; f.cimd.enabled = false; f.cimd.access = ''; f.cimd.refresh = '' - f.changeFeed.enabled = false; f.changeFeed.retentionAgeDays = 7; f.changeFeed.minimumEventCount = 1000 + f.changeFeed.enabled = false; f.changeFeed.configured = false + f.changeFeed.retentionAgeDays = 7; f.changeFeed.minimumEventCount = 1000 } function populate(s?: ApplicationSettingsDto | null) { @@ -346,6 +353,7 @@ function populate(s?: ApplicationSettingsDto | null) { } if (s.ChangeFeed) { f.changeFeed.enabled = s.ChangeFeed.Enabled + f.changeFeed.configured = true f.changeFeed.retentionAgeDays = s.ChangeFeed.MinimumRetentionAgeDays ?? 7 f.changeFeed.minimumEventCount = s.ChangeFeed.MinimumEventCount ?? 1000 } @@ -515,11 +523,17 @@ function build(): ApplicationSettingsDto { Cimd: f.cimd.override ? { Enabled: f.cimd.enabled, AccessTokenLifetimeMinutes: parseNum(f.cimd.access), RefreshTokenLifetimeDays: parseNum(f.cimd.refresh) } : null, - ChangeFeed: { - Enabled: f.changeFeed.enabled, - MinimumRetentionAgeDays: f.changeFeed.retentionAgeDays ?? 7, - MinimumEventCount: f.changeFeed.minimumEventCount ?? 1000, - }, + // An App that never had a change feed and is not getting one now sends NO section: + // these settings are REPLACE, so emitting the defaults would materialize an override + // that says exactly what no override already says — and show up in every draft plan as + // a change nobody made. A section the App already has is kept (see `configured`). + ChangeFeed: f.changeFeed.enabled || f.changeFeed.configured + ? { + Enabled: f.changeFeed.enabled, + MinimumRetentionAgeDays: f.changeFeed.retentionAgeDays ?? 7, + MinimumEventCount: f.changeFeed.minimumEventCount ?? 1000, + } + : null, } }