This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-5673-a5d8602b44f8297a15cf7800dde468d7d784b235 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 559137f2c3b50d3d6dec371611001c909582a5fa Author: Matthew B. <[email protected]> AuthorDate: Mon Jun 22 01:35:25 2026 -0700 refactor(frontend): preserve config source in GuiConfigService instead of one flat map (#5673) > Stacked on #5545 (adds the `/config/amber` endpoint and the `amber` config source this PR namespaces). Base diff includes #5545 until it merges; review the last commit only, or after #5545 lands. ### What changes were proposed in this PR? - Changed `GuiConfigService` to store each endpoint's payload under its own key in a `configBySource` map (`preLogin`, `gui`, `amber`, `userSystem`) instead of spreading them all into one flat object, so the origin of every config value is preserved (addresses the review comment on #5545). - Derived the existing flat `env` getter from `configBySource` on read, keeping its shape identical so the ~50 existing `guiConfigService.env.<flag>` call sites are untouched. - Memoized `env` so it returns a stable object reference (rebuilt only when a source loads). This preserves the two-way `[(ngModel)]` write-through at `menu.component.html`, which only persists when the same object identity is held across change-detection reads. - Added a `source(name)` accessor that returns one endpoint's payload in isolation. - Added unit tests for: source isolation (each source retrievable alone, no key bleed across sources, empty object for an unloaded source), env reference stability (stable across reads, rebuilt after a new source loads, in-memory write discarded on reload), and merge precedence (later source wins on a shared key). ### Any related issues, documentation, discussions? Closes: #5669 ### How was this PR tested? - Run `cd frontend && npx ng test --include="src/app/common/service/gui-config.service.spec.ts"` and expect 15 passed (the 8 existing merge/orchestration tests plus 7 new tests covering source isolation, env reference stability, and merge precedence). - Confirm the merged view is unchanged: `service.env.defaultDataTransferBatchSize` still resolves after post-login load, while `service.source("amber")` returns only the amber payload and `service.source("gui")` does not contain `defaultDataTransferBatchSize`. ### Was this PR authored or co-authored using generative AI tooling? Co-authored with Claude Opus 4.8 in compliance with ASF --- .../app/common/service/gui-config.service.spec.ts | 115 +++++++++++++++++++++ .../src/app/common/service/gui-config.service.ts | 56 ++++++++-- 2 files changed, 160 insertions(+), 11 deletions(-) diff --git a/frontend/src/app/common/service/gui-config.service.spec.ts b/frontend/src/app/common/service/gui-config.service.spec.ts index 92a0d5a615..9dad79e471 100644 --- a/frontend/src/app/common/service/gui-config.service.spec.ts +++ b/frontend/src/app/common/service/gui-config.service.spec.ts @@ -207,4 +207,119 @@ describe("GuiConfigService", () => { http.expectNone(`${API}/config/user-system`); await expect(pending).rejects.toThrow(/pre-login configuration/); }); + + // ─── source() isolation ─────────────────────────────────────────────────── + + it("source() keeps each endpoint's payload retrievable in isolation", async () => { + const preLoginPending = firstValueFrom(service.loadPreLogin()); + http.expectOne(`${API}/config/pre-login`).flush(PRE_LOGIN_PAYLOAD); + await preLoginPending; + + const postLoginPending = firstValueFrom(service.loadPostLogin()); + http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD); + http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD); + http.expectOne(`${API}/config/user-system`).flush(USER_SYSTEM_PAYLOAD); + await postLoginPending; + + expect(service.source("preLogin")).toEqual(PRE_LOGIN_PAYLOAD); + expect(service.source("gui")).toEqual(GUI_PAYLOAD); + expect(service.source("amber")).toEqual(AMBER_PAYLOAD); + expect(service.source("userSystem")).toEqual(USER_SYSTEM_PAYLOAD); + }); + + it("source() does not bleed keys across sources", async () => { + // A value from one endpoint must not appear under another. + const pending = firstValueFrom(service.loadPostLogin()); + http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD); + http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD); + http.expectOne(`${API}/config/user-system`).flush(USER_SYSTEM_PAYLOAD); + await pending; + + expect(service.source("amber")).toHaveProperty("defaultDataTransferBatchSize"); + expect(service.source("gui")).not.toHaveProperty("defaultDataTransferBatchSize"); + expect(service.source("gui")).not.toHaveProperty("inviteOnly"); + // env still flattens them together for the common single-flag call sites. + expect(service.env.defaultDataTransferBatchSize).toBe(400); + }); + + it("source() returns an empty object for an endpoint that has not loaded", () => { + expect(service.source("amber")).toEqual({}); + }); + + // ─── env reference stability ─────────────────────────────────────────────── + + it("env returns a stable reference between loads so two-way bindings persist", async () => { + const pending = firstValueFrom(service.loadPreLogin()); + http.expectOne(`${API}/config/pre-login`).flush(PRE_LOGIN_PAYLOAD); + await pending; + + // Repeated reads must return the same object (relied on by change detection + // and by the [(ngModel)] write-through at menu.component.html). + expect(service.env).toBe(service.env); + + // A write through env persists across subsequent reads. + (service.env as any).workflowEmailNotificationEnabled = true; + expect(service.env.workflowEmailNotificationEnabled).toBe(true); + }); + + it("env reference is rebuilt after a new source loads", async () => { + const preLoginPending = firstValueFrom(service.loadPreLogin()); + http.expectOne(`${API}/config/pre-login`).flush(PRE_LOGIN_PAYLOAD); + await preLoginPending; + const before = service.env; + + const postLoginPending = firstValueFrom(service.loadPostLogin()); + http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD); + http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD); + http.expectOne(`${API}/config/user-system`).flush(USER_SYSTEM_PAYLOAD); + await postLoginPending; + + expect(service.env).not.toBe(before); + expect(service.env.defaultDataTransferBatchSize).toBe(400); + }); + + it("a write through env is discarded when a source reloads", async () => { + // Documents the boundary of the memoized write-through: in-memory edits made + // via [(ngModel)] land on the merged cache only, not on any source payload. + // A subsequent source load invalidates the cache, so the edit does not + // survive a re-fetch (e.g. a fresh login re-running loadPostLogin). This is + // intentional (config reloads are authoritative) and is locked in here so a + // future "persist edits across reloads" change is a deliberate decision. + const preLoginPending = firstValueFrom(service.loadPreLogin()); + http.expectOne(`${API}/config/pre-login`).flush(PRE_LOGIN_PAYLOAD); + await preLoginPending; + + (service.env as any).workflowEmailNotificationEnabled = true; + expect(service.env.workflowEmailNotificationEnabled).toBe(true); + + const postLoginPending = firstValueFrom(service.loadPostLogin()); + http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD); + http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD); + http.expectOne(`${API}/config/user-system`).flush(USER_SYSTEM_PAYLOAD); + await postLoginPending; + + expect(service.env.workflowEmailNotificationEnabled).toBeUndefined(); + }); + + // ─── merge precedence ─────────────────────────────────────────────────────── + + it("env merges sources in MERGE_ORDER so a later source wins on a shared key", async () => { + // The typed source payloads are disjoint, so precedence never fires through + // the public API, but the merge reduce implements "later source wins" and + // nothing else covers it. Inject an overlapping key via the raw payloads to + // lock the documented order (preLogin, gui, amber, userSystem). + const preLoginPending = firstValueFrom(service.loadPreLogin()); + http.expectOne(`${API}/config/pre-login`).flush({ ...PRE_LOGIN_PAYLOAD, inviteOnly: false }); + await preLoginPending; + // userSystem comes after preLogin in MERGE_ORDER, so its value must win. + expect(service.env.inviteOnly).toBe(false); + + const postLoginPending = firstValueFrom(service.loadPostLogin()); + http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD); + http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD); + http.expectOne(`${API}/config/user-system`).flush({ inviteOnly: true }); + await postLoginPending; + + expect(service.env.inviteOnly).toBe(true); + }); }); diff --git a/frontend/src/app/common/service/gui-config.service.ts b/frontend/src/app/common/service/gui-config.service.ts index 28695f7d06..7806f1a680 100644 --- a/frontend/src/app/common/service/gui-config.service.ts +++ b/frontend/src/app/common/service/gui-config.service.ts @@ -34,23 +34,36 @@ type AmberConfig = Pick<GuiConfig, "defaultDataTransferBatchSize">; type GuiOnlyConfig = Omit<GuiConfig, keyof PreLoginConfig | keyof AmberConfig | "inviteOnly">; type UserSystemConfig = Pick<GuiConfig, "inviteOnly">; +// One entry per backend config endpoint. +export type ConfigSource = "preLogin" | "gui" | "amber" | "userSystem"; + @Injectable({ providedIn: "root" }) export class GuiConfigService { - private config: Partial<GuiConfig> = {}; + // Each endpoint's payload, kept under its own key. + private configBySource: Partial<Record<ConfigSource, Partial<GuiConfig>>> = {}; + + // Memoized flat merge. Rebuilt only when a source is written so that env + // returns a stable reference: callers read env on every change-detection + // cycle, and one call site mutates env through a two-way [(ngModel)] binding, + // which only persists when the object identity is held across reads. + private mergedCache: GuiConfig | null = null; + + // Merge precedence when a key appears in multiple sources (later wins). + private static readonly MERGE_ORDER: ConfigSource[] = ["preLogin", "gui", "amber", "userSystem"]; constructor(private http: HttpClient) {} /** * APP_INITIALIZER entry point. Always loads /config/pre-login (anonymous). If * a JWT is already in localStorage (browser reload while logged in), chains - * /config/gui + /config/user-system in the same await so the full config is - * available before any post-login component mounts. + * /config/gui + /config/amber + /config/user-system in the same await so the + * full config is available before any post-login component mounts. */ load(): Observable<Partial<GuiConfig>> { return this.loadPreLogin().pipe( switchMap(() => { if (!GuiConfigService.hasStoredAccessToken()) { - return of(this.config); + return of(this.env); } return this.loadPostLogin().pipe( // Expired or malformed token → /config/gui returns 403. Continue @@ -58,7 +71,7 @@ export class GuiConfigService { // expiry on its own, so we shouldn't block bootstrap on it. catchError((err: unknown) => { console.warn("Failed to load authenticated config; continuing with pre-login only.", err); - return of(this.config); + return of(this.env); }) ); }) @@ -68,9 +81,9 @@ export class GuiConfigService { loadPreLogin(): Observable<Partial<GuiConfig>> { return this.http.get<PreLoginConfig>(`${AppSettings.getApiEndpoint()}/config/pre-login`).pipe( tap(preLogin => { - this.config = { ...this.config, ...preLogin }; + this.setSource("preLogin", preLogin); }), - map(() => this.config), + map(() => this.env), catchError((error: unknown) => { console.error("Failed to load pre-login configuration:", error); return throwError(() => new Error(`Failed to load pre-login configuration from backend: ${error}`)); @@ -80,7 +93,7 @@ export class GuiConfigService { /** * Fetches the authenticated portion of the configuration. Runs after the - * frontend has a valid JWT — called from APP_INITIALIZER on bootstrap when a + * frontend has a valid JWT, called from APP_INITIALIZER on bootstrap when a * stored token exists, and from UserService.handleAccessToken on fresh login. */ loadPostLogin(): Observable<Partial<GuiConfig>> { @@ -89,14 +102,35 @@ export class GuiConfigService { const userSystemConfig$ = this.http.get<UserSystemConfig>(`${AppSettings.getApiEndpoint()}/config/user-system`); return forkJoin([guiConfig$, amberConfig$, userSystemConfig$]).pipe( tap(([guiConfig, amberConfig, userSystemConfig]) => { - this.config = { ...this.config, ...guiConfig, ...amberConfig, ...userSystemConfig }; + this.setSource("gui", guiConfig); + this.setSource("amber", amberConfig); + this.setSource("userSystem", userSystemConfig); }), - map(() => this.config) + map(() => this.env) ); } + // Flat merge of all sources, memoized so reads return a stable reference. get env(): GuiConfig { - return this.config as GuiConfig; + if (this.mergedCache === null) { + this.mergedCache = GuiConfigService.MERGE_ORDER.reduce( + (merged, source) => ({ ...merged, ...this.configBySource[source] }), + {} as Partial<GuiConfig> + ) as GuiConfig; + } + return this.mergedCache; + } + + // One endpoint's payload, in isolation. Returns a shallow copy so callers + // cannot mutate the stored source and desync it from the memoized env view. + source(name: ConfigSource): Partial<GuiConfig> { + return { ...(this.configBySource[name] ?? {}) }; + } + + // Store a source payload and invalidate the merged view. + private setSource(name: ConfigSource, payload: Partial<GuiConfig>): void { + this.configBySource[name] = payload; + this.mergedCache = null; } private static hasStoredAccessToken(): boolean {
