From f9387fbc539383fc0730a2138d569e41704d659e Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:45:56 +0200 Subject: [PATCH 1/6] fix(athena): pass the default workgroup as a string, not the preferences object `Model.Provider` takes an optional `preferences` prop and hands `ui.athena .defaultWorkgroup` to `useWorkgroup`, which now takes that one string instead of the whole `AthenaPreferences` object. The parsed preferences are rebuilt on every provider render, so an object here would re-fire the workgroup effect throughout the probe. A no-op on its own: nothing passes the new prop yet, so behavior is identical to before. It also takes `utils/BucketPreferences` out of the AWS-call layer entirely. Co-Authored-By: Claude Opus 5 --- .../Queries/Athena/model/requests.spec.ts | 3 +-- .../containers/Queries/Athena/model/requests.ts | 10 ++++++---- .../app/containers/Queries/Athena/model/state.tsx | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/catalog/app/containers/Queries/Athena/model/requests.spec.ts b/catalog/app/containers/Queries/Athena/model/requests.spec.ts index ff41f2b0018..d44ce0cb1a8 100644 --- a/catalog/app/containers/Queries/Athena/model/requests.spec.ts +++ b/catalog/app/containers/Queries/Athena/model/requests.spec.ts @@ -1266,10 +1266,9 @@ describe('containers/Queries/Athena/model/requests', () => { data: { list: ['foo', 'bar'] }, loadMore: noop, } - const preferences = { defaultWorkgroup: 'bar' } const { result, waitFor, unmount } = renderHook(() => - useWrapper([workgroups, undefined, preferences]), + useWrapper([workgroups, undefined, 'bar']), ) await act(async () => { diff --git a/catalog/app/containers/Queries/Athena/model/requests.ts b/catalog/app/containers/Queries/Athena/model/requests.ts index c7ecb8b3c02..8f61c440872 100644 --- a/catalog/app/containers/Queries/Athena/model/requests.ts +++ b/catalog/app/containers/Queries/Athena/model/requests.ts @@ -4,7 +4,6 @@ import * as React from 'react' import * as Sentry from '@sentry/react' import * as AWS from 'utils/AWS' -import * as BucketPreferences from 'utils/BucketPreferences' import Log from 'utils/Logging' import noop from 'utils/noop' @@ -190,7 +189,10 @@ export function useWorkgroups(): Model.DataController> { export function useWorkgroup( workgroups: Model.DataController>, requestedWorkgroup?: Workgroup, - preferences?: BucketPreferences.AthenaPreferences, + // The one preference this needs, as a string rather than the `ui.athena` + // object: the parsed preferences are rebuilt on every provider render, so an + // object here would re-fire this effect throughout the workgroup probe. + defaultWorkgroup?: string, ): Model.DataController { const [data, setData] = React.useState>() React.useEffect(() => { @@ -203,7 +205,7 @@ export function useWorkgroup( } // Stored or default workgroup - const initialWorkgroup = storage.getWorkgroup() || preferences?.defaultWorkgroup + const initialWorkgroup = storage.getWorkgroup() || defaultWorkgroup if (initialWorkgroup && listIncludes(workgroups.data.list, initialWorkgroup)) { setData(initialWorkgroup) return @@ -212,7 +214,7 @@ export function useWorkgroup( // First available workgroup or error. Producer drains to exhaustion, so // an accessible workgroup that exists is in this list. setData(workgroups.data.list[0] || new Error('Workgroup not found')) - }, [preferences, requestedWorkgroup, workgroups]) + }, [defaultWorkgroup, requestedWorkgroup, workgroups]) return React.useMemo(() => ({ data, loadMore: noop }), [data]) } diff --git a/catalog/app/containers/Queries/Athena/model/state.tsx b/catalog/app/containers/Queries/Athena/model/state.tsx index c46200f64b7..f1ec99012cf 100644 --- a/catalog/app/containers/Queries/Athena/model/state.tsx +++ b/catalog/app/containers/Queries/Athena/model/state.tsx @@ -2,6 +2,7 @@ import invariant from 'invariant' import * as React from 'react' import * as RRDom from 'react-router-dom' +import type * as BucketPreferences from 'utils/BucketPreferences' import * as NamedRoutes from 'utils/NamedRoutes' import * as requests from './requests' @@ -62,10 +63,15 @@ export interface State { export const Ctx = React.createContext(null) interface ProviderProps { + /** + * `ui.athena` from the `?bucket=` scope's preferences. The console is + * workspace-global, so this only arrives when a bucket is in scope. + */ + preferences?: BucketPreferences.AthenaPreferences children: React.ReactNode } -export function Provider({ children }: ProviderProps) { +export function Provider({ preferences, children }: ProviderProps) { const { urls } = NamedRoutes.use() const location = RRDom.useLocation() @@ -77,7 +83,11 @@ export function Provider({ children }: ProviderProps) { const execution = requests.useWaitForQueryExecution(queryExecutionId) const workgroups = requests.useWorkgroups() - const workgroup = requests.useWorkgroup(workgroups, workgroupId) + const workgroup = requests.useWorkgroup( + workgroups, + workgroupId, + preferences?.defaultWorkgroup, + ) const queries = requests.useQueries(workgroup.data) const query = requests.useQuery(queries.data, execution) const queryBody = requests.useQueryBody(query.value, query.setValue, execution) From f5cdc7f595f046cca6425b1e26020be1c56a845d Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:47:15 +0200 Subject: [PATCH 2/6] fix(athena): resolve the workgroup to AWS's spelling, and try the default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit addresses review findings f34, f16, f33 Two bugs in one expression, both reachable through the default this branch restores. **f34 — the console errors instead of honoring the default.** The workgroup was matched case-insensitively and then stored and used verbatim. An admin writing `ui.athena.defaultWorkgroup: Analytics-Prod` against a workgroup named `analytics-prod` matched here, and then `listNamedQueries` and `startQueryExecution` sent the admin's casing and AWS rejected it with `InvalidRequestException`. Same hole for a mis-cased `:workgroup` URL segment. Candidates now resolve to the list's own spelling before being stored. **f16 — an unavailable stored workgroup masked a valid default.** With `storage.getWorkgroup() || defaultWorkgroup`, a stored workgroup that had been deleted or lost access was picked by the `||`, failed the availability check, and dropped through to the first workgroup in the list — never trying a bucket default that was perfectly valid. The candidates are tried in turn instead. Storage still outranks the bucket default; that ordering is not changed here (see f33 in the PR body). It is now pinned by a test in whichever direction it is set, which nothing did before. Co-Authored-By: Claude Opus 5 --- .../Queries/Athena/model/requests.spec.ts | 87 +++++++++++++++++++ .../Queries/Athena/model/requests.ts | 53 ++++++++--- 2 files changed, 127 insertions(+), 13 deletions(-) diff --git a/catalog/app/containers/Queries/Athena/model/requests.spec.ts b/catalog/app/containers/Queries/Athena/model/requests.spec.ts index d44ce0cb1a8..d09f14d3b6f 100644 --- a/catalog/app/containers/Queries/Athena/model/requests.spec.ts +++ b/catalog/app/containers/Queries/Athena/model/requests.spec.ts @@ -1278,6 +1278,93 @@ describe('containers/Queries/Athena/model/requests', () => { unmount() }) + it('stores the workgroup as AWS spells it, not as the caller did', async () => { + // The match is case-insensitive, but `listNamedQueries` and + // `startQueryExecution` send this value verbatim: an admin writing + // `ui.athena.defaultWorkgroup: Analytics-Prod` against a workgroup named + // `analytics-prod` used to match here and then have AWS reject every call, + // so the console errored instead of honoring the default. + const workgroups = { + data: { list: ['foo', 'analytics-prod'] }, + loadMore: noop, + } + + const { result, waitFor, unmount } = renderHook(() => + useWrapper([workgroups, undefined, 'Analytics-Prod']), + ) + + await act(async () => { + await waitFor(() => typeof result.current.data === 'string') + expect(result.current.data).toBe('analytics-prod') + }) + unmount() + }) + + it('canonicalises a mis-cased workgroup from the URL too', async () => { + const workgroups = { + data: { list: ['foo', 'analytics-prod'] }, + loadMore: noop, + } + + const { result, waitFor, unmount } = renderHook(() => + useWrapper([workgroups, 'ANALYTICS-PROD', undefined]), + ) + + await act(async () => { + await waitFor(() => typeof result.current.data === 'string') + expect(result.current.data).toBe('analytics-prod') + }) + unmount() + }) + + it('falls through to the bucket default when the stored workgroup is gone', async () => { + // A stored workgroup that has been deleted, or whose access was revoked, + // used to be picked by `stored || default`, fail the availability check, + // and drop through to the first workgroup in the list -- masking a bucket + // default that was perfectly valid. + const storageMock = getStorageKey.getMockImplementation() + getStorageKey.mockImplementation(() => 'deleted') + const workgroups = { + data: { list: ['alpha', 'team'] }, + loadMore: noop, + } + + const { result, waitFor, unmount } = renderHook(() => + useWrapper([workgroups, undefined, 'team']), + ) + + await act(async () => { + await waitFor(() => typeof result.current.data === 'string') + expect(result.current.data).toBe('team') + }) + getStorageKey.mockImplementation(storageMock!) + unmount() + }) + + it('keeps the stored workgroup ahead of the bucket default', async () => { + // Pinned in whichever direction it is set: nothing tested this precedence + // either way, and the two candidates are read in one expression. Storage + // first predates this code path, and reordering it is a product decision + // rather than a bug fix -- so it is asserted here rather than changed. + const storageMock = getStorageKey.getMockImplementation() + getStorageKey.mockImplementation(() => 'alpha') + const workgroups = { + data: { list: ['alpha', 'team'] }, + loadMore: noop, + } + + const { result, waitFor, unmount } = renderHook(() => + useWrapper([workgroups, undefined, 'team']), + ) + + await act(async () => { + await waitFor(() => typeof result.current.data === 'string') + expect(result.current.data).toBe('alpha') + }) + getStorageKey.mockImplementation(storageMock!) + unmount() + }) + it('select the first available workgroup if no requested or default', async () => { await act(async () => { const workgroups = { diff --git a/catalog/app/containers/Queries/Athena/model/requests.ts b/catalog/app/containers/Queries/Athena/model/requests.ts index 8f61c440872..87117a71131 100644 --- a/catalog/app/containers/Queries/Athena/model/requests.ts +++ b/catalog/app/containers/Queries/Athena/model/requests.ts @@ -28,8 +28,20 @@ function parseNamedQuery(query: Athena.NamedQuery): Query { } } +/** + * The list's own spelling of `value`, matched case-insensitively. + * + * Returns the entry rather than a boolean, because the caller must go on to use + * the *canonical* name: the match ignores case, but every AWS call downstream + * sends the workgroup name verbatim and rejects a mis-cased one with + * `InvalidRequestException`. + */ +function canonical(list: string[], value: string): string | undefined { + return list.find((x) => x.toLowerCase() === value.toLowerCase()) +} + function listIncludes(list: string[], value: string): boolean { - return list.map((x) => x.toLowerCase()).includes(value.toLowerCase()) + return canonical(list, value) !== undefined } export type Workgroup = string @@ -197,23 +209,38 @@ export function useWorkgroup( const [data, setData] = React.useState>() React.useEffect(() => { if (!Model.hasData(workgroups.data)) return - - // URL parameter workgroup (user navigation) - if (requestedWorkgroup && listIncludes(workgroups.data.list, requestedWorkgroup)) { - setData(requestedWorkgroup) - return + const { list } = workgroups.data + + // Each candidate is resolved to the list's own spelling before it is stored: + // the match is case-insensitive, but `listNamedQueries` and + // `startQueryExecution` send this value verbatim and AWS rejects a mis-cased + // workgroup outright. + // + // Tried in turn rather than `stored || default`: an unavailable stored + // workgroup -- deleted, or access revoked -- used to be picked by the `||`, + // fail the availability check, and drop through to the first workgroup in the + // list, masking a bucket default that was perfectly valid. + // + // Storage still outranks the bucket default. That precedence predates this + // code path and reordering it is a product decision, not a bug fix. + const pick = (candidate?: string | null): boolean => { + if (!candidate) return false + const found = canonical(list, candidate) + if (!found) return false + setData(found) + return true } - // Stored or default workgroup - const initialWorkgroup = storage.getWorkgroup() || defaultWorkgroup - if (initialWorkgroup && listIncludes(workgroups.data.list, initialWorkgroup)) { - setData(initialWorkgroup) - return - } + // URL parameter workgroup (user navigation), then the stored one, then the + // bucket default. Read in that order and only as far as needed, so a URL + // that names a workgroup never touches storage. + if (pick(requestedWorkgroup)) return + if (pick(storage.getWorkgroup())) return + if (pick(defaultWorkgroup)) return // First available workgroup or error. Producer drains to exhaustion, so // an accessible workgroup that exists is in this list. - setData(workgroups.data.list[0] || new Error('Workgroup not found')) + setData(list[0] || new Error('Workgroup not found')) }, [defaultWorkgroup, requestedWorkgroup, workgroups]) return React.useMemo(() => ({ data, loadMore: noop }), [data]) } From 24ff655152b408d51eddb0a7907a0443091ae98d Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:50:47 +0200 Subject: [PATCH 3/6] test(athena): cross the Provider to useWorkgroup seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit addresses review finding f46 The seam this branch exists to restore — `Model.Provider` taking `ui.athena` from the `?bucket=` scope and handing the default workgroup down — was crossed by no test. The prop could have been dropped on the floor and everything else here would still have passed. Drives it through the provider with a URL naming a workgroup the user cannot reach, which is also the only shape in which the default is observable: with no workgroup in the URL the provider redirects rather than rendering its children. The list's first entry is deliberately not the default, so the assertion cannot pass by the fallback happening to agree. Co-Authored-By: Claude Opus 5 --- .../Queries/Athena/model/state.spec.tsx | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/catalog/app/containers/Queries/Athena/model/state.spec.tsx b/catalog/app/containers/Queries/Athena/model/state.spec.tsx index d2d9ec47b61..ddd0e729d08 100644 --- a/catalog/app/containers/Queries/Athena/model/state.spec.tsx +++ b/catalog/app/containers/Queries/Athena/model/state.spec.tsx @@ -102,4 +102,58 @@ describe('app/containers/Queries/Athena/model/state', () => { expect(result.current.workgroup.data).toBe('w') unmount() }) + + it('threads the bucket default workgroup through to the model', async () => { + // The seam this branch exists to restore: `Provider` takes `ui.athena` from + // the `?bucket=` scope's preferences and hands the default workgroup to + // `useWorkgroup`. Nothing crossed it, so the prop could have been dropped on + // the floor and every test here would still have passed. + // A workgroup *is* named, but it is one this user cannot reach — a bookmark + // from before access changed. Without a named workgroup the provider + // redirects instead of rendering, so this is also the shape that lets the + // default be observed at all. + useParams.mockImplementation(() => ({ workgroup: 'gone' }) as Record) + listWorkGroups.mockImplementation(() => ({ + promise: () => + Promise.resolve({ + WorkGroups: [{ Name: 'alpha' }, { Name: 'team' }], + }), + })) + getWorkGroup.mockImplementation(({ WorkGroup: Name }: { WorkGroup: string }) => ({ + promise: () => + Promise.resolve({ + WorkGroup: { + Configuration: { ResultConfiguration: { OutputLocation: 'any' } }, + State: 'ENABLED', + Name, + }, + }), + })) + listNamedQueries.mockImplementation((_x, cb) => { + cb(undefined, { NamedQueryIds: [] }) + return { abort: noop } + }) + listQueryExecutions.mockImplementation((_x, cb) => { + cb(undefined, { QueryExecutionIds: [] }) + return { abort: noop } + }) + listDataCatalogs.mockImplementation(() => ({ + promise: () => Promise.resolve({ DataCatalogsSummary: [] }), + })) + + const wrapper = ({ children }: { children: React.ReactNode }) => ( + + {children} + + ) + const { result, waitFor, unmount } = renderHook(() => Model.useState(), { wrapper }) + await act(async () => { + await waitFor(() => typeof result.current.workgroup.data === 'string') + }) + // 'alpha' is first in the list, so this is the default being honored rather + // than the fallback happening to agree. + expect(result.current.workgroup.data).toBe('team') + unmount() + useParams.mockImplementation(() => ({ workgroup: 'w' }) as Record) + }) }) From b7b81ebed2e2cf98b73aed5132bf73cee3e3e830 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:51:31 +0200 Subject: [PATCH 4/6] docs(changelog): entry for the workgroup resolution fixes addresses review findings f44, f27 The plan for this split had no changelog line here, on the grounds that the seam is a no-op. The two bug fixes folded in are not: a mis-cased workgroup erroring and a stale stored workgroup masking the bucket default are both user-visible, so they get an entry. Co-Authored-By: Claude Opus 5 --- catalog/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/catalog/CHANGELOG.md b/catalog/CHANGELOG.md index cf1a58c62f6..2588fcb3afe 100644 --- a/catalog/CHANGELOG.md +++ b/catalog/CHANGELOG.md @@ -21,6 +21,7 @@ complete sentence without it. ## Changes +- [Fixed] Athena Queries: a workgroup named in `ui.athena.defaultWorkgroup` or in the URL is honored whatever its capitalisation — a mis-cased name used to match and then have every AWS call rejected — and a stored workgroup that has been deleted or lost access no longer hides a valid bucket default ([#5265](https://github.com/quiltdata/quilt/pull/5265)) - [Fixed] Admin Users: the roles dialog and the Role column are read-only for service users, rather than offering a Save the registry refuses ([#5264](https://github.com/quiltdata/quilt/pull/5264)) - [Fixed] Admin Users: a disabled Enabled or Admin switch says why on hover and on keyboard focus — your own account, a service user managed by the stack, or admin capabilities managed by the SSO configuration — instead of rendering as the same unexplained dead control; the Admin switch now also refuses service users ([#5263](https://github.com/quiltdata/quilt/pull/5263)) - [Fixed] Search sidebar: the facet "Sort by" control no longer disappears while you type in "Find metadata" on stacks with truncated facet lists, and it is withheld when the query matches nothing rather than offering to sort an empty list ([#5262](https://github.com/quiltdata/quilt/pull/5262)) From f440009fcb292d845cf65c3de33a77e27c8bcfdf Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 23:22:25 +0200 Subject: [PATCH 5/6] fix(athena): prefer an exact workgroup match, and survive a non-string default Case-insensitive resolution alone picks whichever of 'Alpha' and 'alpha' sorts first, so an exactly-spelled request could land on the other workgroup. And ui.athena is unconstrained by the bucket-config schema, so an all-digit workgroup name arrives from YAML as a number and threw out of the effect. --- .../Queries/Athena/model/requests.spec.ts | 40 +++++++++++++++++++ .../Queries/Athena/model/requests.ts | 15 +++++-- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/catalog/app/containers/Queries/Athena/model/requests.spec.ts b/catalog/app/containers/Queries/Athena/model/requests.spec.ts index d09f14d3b6f..a0e901e8590 100644 --- a/catalog/app/containers/Queries/Athena/model/requests.spec.ts +++ b/catalog/app/containers/Queries/Athena/model/requests.spec.ts @@ -1300,6 +1300,46 @@ describe('containers/Queries/Athena/model/requests', () => { unmount() }) + it('prefers an exact match over a differently-cased sibling', async () => { + // Workgroup names are case-sensitive, so both can exist. `fetchWorkgroups` + // sorts, which puts 'Alpha' first -- a case-insensitive match alone would + // hand the user the workgroup they did not ask for. + const workgroups = { + data: { list: ['Alpha', 'alpha'] }, + loadMore: noop, + } + + const { result, waitFor, unmount } = renderHook(() => + useWrapper([workgroups, 'alpha', undefined]), + ) + + await act(async () => { + await waitFor(() => typeof result.current.data === 'string') + expect(result.current.data).toBe('alpha') + }) + unmount() + }) + + it('falls back rather than throwing when the default is not a string', async () => { + // `ui.athena` is unconstrained by the bucket-config schema, so an all-digit + // workgroup name in the YAML arrives as a number. Reaching `toLowerCase` + // with it threw out of the effect and took the console down. + const workgroups = { + data: { list: ['foo', '2024'] }, + loadMore: noop, + } + + const { result, waitFor, unmount } = renderHook(() => + useWrapper([workgroups, undefined, 2024 as unknown as string]), + ) + + await act(async () => { + await waitFor(() => typeof result.current.data === 'string') + expect(result.current.data).toBe('foo') + }) + unmount() + }) + it('canonicalises a mis-cased workgroup from the URL too', async () => { const workgroups = { data: { list: ['foo', 'analytics-prod'] }, diff --git a/catalog/app/containers/Queries/Athena/model/requests.ts b/catalog/app/containers/Queries/Athena/model/requests.ts index 87117a71131..4727c24efb8 100644 --- a/catalog/app/containers/Queries/Athena/model/requests.ts +++ b/catalog/app/containers/Queries/Athena/model/requests.ts @@ -35,9 +35,16 @@ function parseNamedQuery(query: Athena.NamedQuery): Query { * the *canonical* name: the match ignores case, but every AWS call downstream * sends the workgroup name verbatim and rejects a mis-cased one with * `InvalidRequestException`. + * + * Names are case-sensitive, so `alpha` and `Alpha` can both exist -- an exact + * hit has to win over the case-insensitive one, which the sorted list would + * otherwise resolve to whichever sorts first. */ function canonical(list: string[], value: string): string | undefined { - return list.find((x) => x.toLowerCase() === value.toLowerCase()) + return ( + list.find((x) => x === value) ?? + list.find((x) => x.toLowerCase() === value.toLowerCase()) + ) } function listIncludes(list: string[], value: string): boolean { @@ -223,8 +230,10 @@ export function useWorkgroup( // // Storage still outranks the bucket default. That precedence predates this // code path and reordering it is a product decision, not a bug fix. - const pick = (candidate?: string | null): boolean => { - if (!candidate) return false + // `unknown`, not `string`: `defaultWorkgroup` comes from a user-authored YAML + // document, and the bucket-config schema does not constrain `ui.athena`. + const pick = (candidate: unknown): boolean => { + if (typeof candidate !== 'string' || !candidate) return false const found = canonical(list, candidate) if (!found) return false setData(found) From 5a2b9961e08f7d6e6e2e950885a504a272d49720 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 23:23:06 +0200 Subject: [PATCH 6/6] docs(athena): trim the workgroup narration to the constraints The comments retold what the change did and what the old code got wrong; the tests already pin that. Keep only what the code cannot show: why the canonical spelling has to be used, why the preference crosses as a string, and that storage-before-default is inherited product behaviour. Restore the stubbed mocks in a finally: afterEach only clears call data, so a failing assertion leaked the stub into every later test in the file. The changelog entry now claims only what this layer ships on its own -- the bucket default it also mentioned is not wired until #5267, which announces it. --- catalog/CHANGELOG.md | 2 +- .../Queries/Athena/model/requests.spec.ts | 57 +++++++++---------- .../Queries/Athena/model/requests.ts | 36 +++--------- .../Queries/Athena/model/state.spec.tsx | 32 +++++------ 4 files changed, 51 insertions(+), 76 deletions(-) diff --git a/catalog/CHANGELOG.md b/catalog/CHANGELOG.md index 2588fcb3afe..7e89b662e81 100644 --- a/catalog/CHANGELOG.md +++ b/catalog/CHANGELOG.md @@ -21,7 +21,7 @@ complete sentence without it. ## Changes -- [Fixed] Athena Queries: a workgroup named in `ui.athena.defaultWorkgroup` or in the URL is honored whatever its capitalisation — a mis-cased name used to match and then have every AWS call rejected — and a stored workgroup that has been deleted or lost access no longer hides a valid bucket default ([#5265](https://github.com/quiltdata/quilt/pull/5265)) +- [Fixed] Athena Queries: a workgroup named in the URL is honored whatever its capitalisation, instead of matching a workgroup you can see and then having every AWS call rejected ([#5265](https://github.com/quiltdata/quilt/pull/5265)) - [Fixed] Admin Users: the roles dialog and the Role column are read-only for service users, rather than offering a Save the registry refuses ([#5264](https://github.com/quiltdata/quilt/pull/5264)) - [Fixed] Admin Users: a disabled Enabled or Admin switch says why on hover and on keyboard focus — your own account, a service user managed by the stack, or admin capabilities managed by the SSO configuration — instead of rendering as the same unexplained dead control; the Admin switch now also refuses service users ([#5263](https://github.com/quiltdata/quilt/pull/5263)) - [Fixed] Search sidebar: the facet "Sort by" control no longer disappears while you type in "Find metadata" on stacks with truncated facet lists, and it is withheld when the query matches nothing rather than offering to sort an empty list ([#5262](https://github.com/quiltdata/quilt/pull/5262)) diff --git a/catalog/app/containers/Queries/Athena/model/requests.spec.ts b/catalog/app/containers/Queries/Athena/model/requests.spec.ts index a0e901e8590..a6dd822e4a2 100644 --- a/catalog/app/containers/Queries/Athena/model/requests.spec.ts +++ b/catalog/app/containers/Queries/Athena/model/requests.spec.ts @@ -1279,11 +1279,6 @@ describe('containers/Queries/Athena/model/requests', () => { }) it('stores the workgroup as AWS spells it, not as the caller did', async () => { - // The match is case-insensitive, but `listNamedQueries` and - // `startQueryExecution` send this value verbatim: an admin writing - // `ui.athena.defaultWorkgroup: Analytics-Prod` against a workgroup named - // `analytics-prod` used to match here and then have AWS reject every call, - // so the console errored instead of honoring the default. const workgroups = { data: { list: ['foo', 'analytics-prod'] }, loadMore: noop, @@ -1358,10 +1353,6 @@ describe('containers/Queries/Athena/model/requests', () => { }) it('falls through to the bucket default when the stored workgroup is gone', async () => { - // A stored workgroup that has been deleted, or whose access was revoked, - // used to be picked by `stored || default`, fail the availability check, - // and drop through to the first workgroup in the list -- masking a bucket - // default that was perfectly valid. const storageMock = getStorageKey.getMockImplementation() getStorageKey.mockImplementation(() => 'deleted') const workgroups = { @@ -1369,23 +1360,24 @@ describe('containers/Queries/Athena/model/requests', () => { loadMore: noop, } - const { result, waitFor, unmount } = renderHook(() => - useWrapper([workgroups, undefined, 'team']), - ) + try { + const { result, waitFor, unmount } = renderHook(() => + useWrapper([workgroups, undefined, 'team']), + ) - await act(async () => { - await waitFor(() => typeof result.current.data === 'string') - expect(result.current.data).toBe('team') - }) - getStorageKey.mockImplementation(storageMock!) - unmount() + await act(async () => { + await waitFor(() => typeof result.current.data === 'string') + expect(result.current.data).toBe('team') + }) + unmount() + } finally { + getStorageKey.mockImplementation(storageMock!) + } }) it('keeps the stored workgroup ahead of the bucket default', async () => { - // Pinned in whichever direction it is set: nothing tested this precedence - // either way, and the two candidates are read in one expression. Storage - // first predates this code path, and reordering it is a product decision - // rather than a bug fix -- so it is asserted here rather than changed. + // Precedence pinned, not chosen here: storage-before-default predates this + // code path, so changing it is a product decision. const storageMock = getStorageKey.getMockImplementation() getStorageKey.mockImplementation(() => 'alpha') const workgroups = { @@ -1393,16 +1385,19 @@ describe('containers/Queries/Athena/model/requests', () => { loadMore: noop, } - const { result, waitFor, unmount } = renderHook(() => - useWrapper([workgroups, undefined, 'team']), - ) + try { + const { result, waitFor, unmount } = renderHook(() => + useWrapper([workgroups, undefined, 'team']), + ) - await act(async () => { - await waitFor(() => typeof result.current.data === 'string') - expect(result.current.data).toBe('alpha') - }) - getStorageKey.mockImplementation(storageMock!) - unmount() + await act(async () => { + await waitFor(() => typeof result.current.data === 'string') + expect(result.current.data).toBe('alpha') + }) + unmount() + } finally { + getStorageKey.mockImplementation(storageMock!) + } }) it('select the first available workgroup if no requested or default', async () => { diff --git a/catalog/app/containers/Queries/Athena/model/requests.ts b/catalog/app/containers/Queries/Athena/model/requests.ts index 4727c24efb8..11ab68be28c 100644 --- a/catalog/app/containers/Queries/Athena/model/requests.ts +++ b/catalog/app/containers/Queries/Athena/model/requests.ts @@ -29,16 +29,13 @@ function parseNamedQuery(query: Athena.NamedQuery): Query { } /** - * The list's own spelling of `value`, matched case-insensitively. + * The list's own spelling of `value`, preferring an exact match. * - * Returns the entry rather than a boolean, because the caller must go on to use - * the *canonical* name: the match ignores case, but every AWS call downstream - * sends the workgroup name verbatim and rejects a mis-cased one with - * `InvalidRequestException`. - * - * Names are case-sensitive, so `alpha` and `Alpha` can both exist -- an exact - * hit has to win over the case-insensitive one, which the sorted list would - * otherwise resolve to whichever sorts first. + * Callers must use the spelling this returns: AWS calls send the workgroup name + * verbatim and reject a mis-cased one with `InvalidRequestException`. Names are + * case-sensitive, so `alpha` and `Alpha` can both exist -- an exact hit has to + * win over the case-insensitive one, which the sorted list would otherwise + * resolve to whichever sorts first. */ function canonical(list: string[], value: string): string | undefined { return ( @@ -208,9 +205,9 @@ export function useWorkgroups(): Model.DataController> { export function useWorkgroup( workgroups: Model.DataController>, requestedWorkgroup?: Workgroup, - // The one preference this needs, as a string rather than the `ui.athena` - // object: the parsed preferences are rebuilt on every provider render, so an - // object here would re-fire this effect throughout the workgroup probe. + // A string rather than the `ui.athena` object: the parsed preferences are + // rebuilt on every provider render, so an object would re-fire this effect + // throughout the workgroup probe. defaultWorkgroup?: string, ): Model.DataController { const [data, setData] = React.useState>() @@ -218,18 +215,6 @@ export function useWorkgroup( if (!Model.hasData(workgroups.data)) return const { list } = workgroups.data - // Each candidate is resolved to the list's own spelling before it is stored: - // the match is case-insensitive, but `listNamedQueries` and - // `startQueryExecution` send this value verbatim and AWS rejects a mis-cased - // workgroup outright. - // - // Tried in turn rather than `stored || default`: an unavailable stored - // workgroup -- deleted, or access revoked -- used to be picked by the `||`, - // fail the availability check, and drop through to the first workgroup in the - // list, masking a bucket default that was perfectly valid. - // - // Storage still outranks the bucket default. That precedence predates this - // code path and reordering it is a product decision, not a bug fix. // `unknown`, not `string`: `defaultWorkgroup` comes from a user-authored YAML // document, and the bucket-config schema does not constrain `ui.athena`. const pick = (candidate: unknown): boolean => { @@ -240,9 +225,6 @@ export function useWorkgroup( return true } - // URL parameter workgroup (user navigation), then the stored one, then the - // bucket default. Read in that order and only as far as needed, so a URL - // that names a workgroup never touches storage. if (pick(requestedWorkgroup)) return if (pick(storage.getWorkgroup())) return if (pick(defaultWorkgroup)) return diff --git a/catalog/app/containers/Queries/Athena/model/state.spec.tsx b/catalog/app/containers/Queries/Athena/model/state.spec.tsx index ddd0e729d08..fd9b280148f 100644 --- a/catalog/app/containers/Queries/Athena/model/state.spec.tsx +++ b/catalog/app/containers/Queries/Athena/model/state.spec.tsx @@ -104,14 +104,9 @@ describe('app/containers/Queries/Athena/model/state', () => { }) it('threads the bucket default workgroup through to the model', async () => { - // The seam this branch exists to restore: `Provider` takes `ui.athena` from - // the `?bucket=` scope's preferences and hands the default workgroup to - // `useWorkgroup`. Nothing crossed it, so the prop could have been dropped on - // the floor and every test here would still have passed. - // A workgroup *is* named, but it is one this user cannot reach — a bookmark - // from before access changed. Without a named workgroup the provider - // redirects instead of rendering, so this is also the shape that lets the - // default be observed at all. + // The workgroup named in the URL is deliberately one this user cannot reach: + // without a named workgroup the provider redirects instead of rendering, so + // this is the only shape in which the default can be observed. useParams.mockImplementation(() => ({ workgroup: 'gone' }) as Record) listWorkGroups.mockImplementation(() => ({ promise: () => @@ -146,14 +141,17 @@ describe('app/containers/Queries/Athena/model/state', () => { {children} ) - const { result, waitFor, unmount } = renderHook(() => Model.useState(), { wrapper }) - await act(async () => { - await waitFor(() => typeof result.current.workgroup.data === 'string') - }) - // 'alpha' is first in the list, so this is the default being honored rather - // than the fallback happening to agree. - expect(result.current.workgroup.data).toBe('team') - unmount() - useParams.mockImplementation(() => ({ workgroup: 'w' }) as Record) + try { + const { result, waitFor, unmount } = renderHook(() => Model.useState(), { wrapper }) + await act(async () => { + await waitFor(() => typeof result.current.workgroup.data === 'string') + }) + // 'alpha' is first in the list, so this is the default being honored rather + // than the fallback happening to agree. + expect(result.current.workgroup.data).toBe('team') + unmount() + } finally { + useParams.mockImplementation(() => ({ workgroup: 'w' }) as Record) + } }) })