diff --git a/packages/cli/package.json b/packages/cli/package.json index a1c3b382..c7173000 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@metabase/cli", - "version": "0.3.0", + "version": "0.3.1", "description": "Metabase CLI", "license": "AGPL-3.0", "repository": { diff --git a/packages/client/src/http/errors.test.ts b/packages/client/src/http/errors.test.ts index 2394121e..44e120f3 100644 --- a/packages/client/src/http/errors.test.ts +++ b/packages/client/src/http/errors.test.ts @@ -43,6 +43,22 @@ function textHeaders(): Headers { } describe("HttpError message extraction", () => { + it("uses a text/plain body as the message when there is no envelope", () => { + const error = buildHttpError({ + responseHeaders: textHeaders(), + rawBody: "Invalid query: missing or invalid Database ID (:database)\n", + }); + expect(error.message).toBe("Invalid query: missing or invalid Database ID (:database)"); + }); + + it("does not read a non-JSON body of another content type as the message", () => { + const error = buildHttpError({ + responseHeaders: new Headers({ "content-type": "text/html" }), + rawBody: "Bad Request", + }); + expect(error.message).toBe("Metabase returned 400."); + }); + it("prefers top-level message over other fields", () => { const body = JSON.stringify({ message: "top-level wins", diff --git a/packages/client/src/http/errors.ts b/packages/client/src/http/errors.ts index e8fba5e8..7bc1cb33 100644 --- a/packages/client/src/http/errors.ts +++ b/packages/client/src/http/errors.ts @@ -21,6 +21,7 @@ interface StatusClassification { const NOT_FOUND_STATUS = 404; +const TEXT_CONTENT_TYPE = "text/plain"; const ROUTE_MISSING_LITERAL = "API endpoint does not exist."; const RESOURCE_MISSING_LITERAL = "Not found."; @@ -103,7 +104,9 @@ export class HttpError extends MetabaseError { const sanitizedBody = sanitizeBody(input.rawBody, input.redactionContext); const redactedHeaders = redactHeaders(input.responseHeaders); const kind = classifyKind(input.status, sanitizedBody, redactedHeaders); - super(input.overrideUserMessage ?? buildUserMessage(kind, input, sanitizedBody)); + super( + input.overrideUserMessage ?? buildUserMessage(kind, input, sanitizedBody, redactedHeaders), + ); const fields = extractFieldErrors(sanitizedBody); this.name = "HttpError"; this.status = input.status; @@ -225,6 +228,7 @@ function buildUserMessage( kind: HttpErrorKind, input: HttpErrorInput, sanitizedBody: string | null, + redactedHeaders: Record, ): string { if (kind === "route-missing") { return buildRouteMissingMessage(input); @@ -241,9 +245,27 @@ function buildUserMessage( if (kind === "auth") { return `Invalid or unauthorized API key (host: ${hostFromUrl(input.url)}).`; } + const fromText = plainTextMessage(sanitizedBody, redactedHeaders); + if (fromText !== null) { + return fromText; + } return defaultMessageForStatus(input.status); } +// Metabase answers some rejections — a query that fails normalization, for one — with a text/plain +// body that is nothing but the message. Only that content type is read as one: an HTML error page +// from whatever sits in front of Metabase is never a message. +function plainTextMessage( + sanitizedBody: string | null, + redactedHeaders: Record, +): string | null { + if (sanitizedBody === null || !redactedHeaders["content-type"]?.includes(TEXT_CONTENT_TYPE)) { + return null; + } + const trimmed = sanitizedBody.trim(); + return trimmed === "" ? null : capLength(trimmed); +} + function buildRouteMissingMessage(input: HttpErrorInput): string { const path = pathFromUrl(input.url); if (!input.serverTag) { diff --git a/tests/e2e/bootstrap-data.ts b/tests/e2e/bootstrap-data.ts index 2c3702e2..86e1a47f 100644 --- a/tests/e2e/bootstrap-data.ts +++ b/tests/e2e/bootstrap-data.ts @@ -37,6 +37,8 @@ export const SeededIds = z.object({ // `library` premium feature; null otherwise. Defaults null for bootstrap files written before // this field existed. Target for `table publish`. libraryDataCollectionId: z.number().int().positive().nullable().default(null), + // The admin's personal collection, the only personal collection a snapshot holds. + adminPersonalCollectionId: z.number().int().positive(), }); export type SeededIds = z.infer; diff --git a/tests/e2e/collection.e2e.test.ts b/tests/e2e/collection.e2e.test.ts index d486d271..fb4dca6a 100644 --- a/tests/e2e/collection.e2e.test.ts +++ b/tests/e2e/collection.e2e.test.ts @@ -10,6 +10,7 @@ import { readBootstrap, type E2EBootstrap } from "./bootstrap-data"; import { cleanupConfigHome, mkTempConfigHome, runCli } from "./run-cli"; import { cliErrorMessage } from "./cli-error"; import { SEEDED } from "./seed/seeded"; +import { serverVersionBelow } from "./server-gate"; const DEFAULT_COLLECTION_NAME = "E2E Default"; @@ -35,6 +36,19 @@ const ROOT_COMPACT = { is_remote_synced: false, } as const; +const ADMIN_PERSONAL_COMPACT = { + id: SEEDED.adminPersonalCollectionId, + name: "Admin E2E's Personal Collection", + description: null, + archived: false, + location: "/", + parent_id: null, + type: null, + authority_level: null, + is_personal: true, + is_remote_synced: false, +} as const; + const TRASH_COMPACT = { id: 1, name: "Trash", @@ -48,6 +62,14 @@ const TRASH_COMPACT = { is_remote_synced: false, } as const; +// Through v61 the items endpoint reads its total off the first row's window column, so an empty +// page reports no total at all; from v62 it reports 0. +const EMPTY_ITEMS_TOTAL_VERSION = 62; + +function emptyItemsPageTotal(): number | null { + return serverVersionBelow(EMPTY_ITEMS_TOTAL_VERSION) ? null : 0; +} + describe("collection e2e", () => { let bootstrap: E2EBootstrap; const tempDirs: string[] = []; @@ -120,7 +142,7 @@ describe("collection e2e", () => { }); }); - it("list --filter personal returns no rows for the synthetic api-key user", async () => { + it("list --filter personal returns only the admin's personal collection", async () => { const result = await runCli({ args: ["collection", "list", "--filter", "personal", "--json"], configHome: await makeIsolatedConfigHome(), @@ -129,10 +151,10 @@ describe("collection e2e", () => { expect(result.exitCode, result.stderr).toBe(0); expect(parseJson(result.stdout, CollectionListEnvelope)).toEqual({ - data: [], - returned: 0, + data: [ADMIN_PERSONAL_COMPACT], + returned: 1, offset: 0, - total: 0, + total: 1, has_more: false, next_offset: null, }); @@ -433,7 +455,7 @@ describe("collection e2e", () => { expect(result.stdout).toBe(""); }); - it("items on a freshly-created empty collection returns an empty envelope (server total: null)", async () => { + it("items on a freshly-created empty collection returns an empty envelope", async () => { const configHome = await makeIsolatedConfigHome(); const createResult = await runCli({ args: ["collection", "create", "--json"], @@ -458,7 +480,7 @@ describe("collection e2e", () => { data: [], returned: 0, offset: 0, - total: null, + total: emptyItemsPageTotal(), has_more: false, next_offset: null, }); diff --git a/tests/e2e/content-translation.e2e.test.ts b/tests/e2e/content-translation.e2e.test.ts index 13a8b959..5b62be2c 100644 --- a/tests/e2e/content-translation.e2e.test.ts +++ b/tests/e2e/content-translation.e2e.test.ts @@ -105,7 +105,6 @@ describe.skipIf(skipReason !== null)("content translation e2e against EE endpoin expect(result.exitCode, result.stderr).toBe(0); expect(result.stdout).toContain("Locale Code,String,Translation"); - expect(result.stderr).toBe(""); }); it("upload replaces the dictionary and reports the server confirmation", async () => { diff --git a/tests/e2e/docker-compose.yml b/tests/e2e/docker-compose.yml index 0fcf031f..b81b6a03 100644 --- a/tests/e2e/docker-compose.yml +++ b/tests/e2e/docker-compose.yml @@ -31,6 +31,9 @@ services: MB_LOAD_SAMPLE_CONTENT: "false" MB_CHECK_FOR_UPDATES: "false" MB_ANON_TRACKING_ENABLED: "false" + # The warehouse lives on the compose network, which the connection-host policy classes as private + # and refuses by default whenever the license token carries `hosting` (the CI token does). + MB_WAREHOUSE_ALLOWED_NETWORKS: allow-all # `dev` so Metabase honors METASTORE_DEV_SERVER_URL for token checks; # the prod default is hard-pinned to https://token-check.metabase.com. MB_RUN_MODE: dev diff --git a/tests/e2e/git-sync.e2e.test.ts b/tests/e2e/git-sync.e2e.test.ts index 3c4f8a96..f791082a 100644 --- a/tests/e2e/git-sync.e2e.test.ts +++ b/tests/e2e/git-sync.e2e.test.ts @@ -237,7 +237,7 @@ describe.skipIf(skipReason !== null)("git-sync e2e against EE git-sync endpoints expect(cliErrorCategory(result.stderr)).toBe("http"); }); - it("has-remote-changes without git-sync configured surfaces a 400 HttpError", async () => { + it("has-remote-changes without git-sync configured surfaces the server's 400 message", async () => { const configHome = await makeIsolatedConfigHome(); const result = await runCli({ args: ["git-sync", "has-remote-changes", "--json"], @@ -245,10 +245,10 @@ describe.skipIf(skipReason !== null)("git-sync e2e against EE git-sync endpoints env: authEnv(), }); expect(result.exitCode).toBe(1); - expect(result.stderr).toContain("Metabase returned 400"); + expect(cliErrorMessage(result.stderr)).toBe("Remote sync is not configured."); }); - it("cancel-task surfaces a 400 HttpError when there is no running task", async () => { + it("cancel-task surfaces the server's 400 message when there is no running task", async () => { const configHome = await makeIsolatedConfigHome(); const result = await runCli({ args: ["git-sync", "cancel-task", "--json"], @@ -256,10 +256,10 @@ describe.skipIf(skipReason !== null)("git-sync e2e against EE git-sync endpoints env: authEnv(), }); expect(result.exitCode).toBe(1); - expect(result.stderr).toContain("Metabase returned 400"); + expect(cliErrorMessage(result.stderr)).toBe("No active task to cancel"); }); - it("stash surfaces a 400 HttpError when remote-sync-type is not read-write", async () => { + it("stash surfaces the server's 400 message when remote-sync-type is not read-write", async () => { const configHome = await makeIsolatedConfigHome(); const result = await runCli({ args: ["git-sync", "stash", "--new-branch", "wip", "--message", "x", "--no-wait", "--json"], @@ -267,7 +267,9 @@ describe.skipIf(skipReason !== null)("git-sync e2e against EE git-sync endpoints env: authEnv(), }); expect(result.exitCode).toBe(1); - expect(result.stderr).toContain("Metabase returned 400"); + expect(cliErrorMessage(result.stderr)).toBe( + "Stash is only allowed when remote-sync-type is set to 'read-write'", + ); }); it("branches surfaces an HttpError when no source URL is configured", async () => { @@ -281,7 +283,7 @@ describe.skipIf(skipReason !== null)("git-sync e2e against EE git-sync endpoints expect(result.stderr).toContain("Failed to clone git repository"); }); - it("add-collection surfaces a 400 HttpError in the default config (read-only or paywall)", async () => { + it("add-collection surfaces the server's read-only 400 message in the default config", async () => { const configHome = await makeIsolatedConfigHome(); const result = await runCli({ args: ["git-sync", "add-collection", "1", "--json"], @@ -289,7 +291,9 @@ describe.skipIf(skipReason !== null)("git-sync e2e against EE git-sync endpoints env: authEnv(), }); expect(result.exitCode).toBe(1); - expect(result.stderr).toContain("Metabase returned 400"); + expect(cliErrorMessage(result.stderr)).toBe( + "Cannot change synced collections when remote-sync-type is read-only.", + ); }); it("remove-collection is idempotent when the collection is not in the sync config", async () => { diff --git a/tests/e2e/measure.e2e.test.ts b/tests/e2e/measure.e2e.test.ts index a6a45d25..f9dc2a57 100644 --- a/tests/e2e/measure.e2e.test.ts +++ b/tests/e2e/measure.e2e.test.ts @@ -28,8 +28,12 @@ const NEW_MEASURE_BODY: MeasureCreateInput = { table_id: SEEDED.tables.orders, description: MEASURE_DESCRIPTION, definition: { - "source-table": SEEDED.tables.orders, - aggregation: [["count"]], + database: SEEDED.warehouseDbId, + type: "query", + query: { + "source-table": SEEDED.tables.orders, + aggregation: [["count"]], + }, }, }; diff --git a/tests/e2e/query.e2e.test.ts b/tests/e2e/query.e2e.test.ts index a3a82cdf..a3a95d91 100644 --- a/tests/e2e/query.e2e.test.ts +++ b/tests/e2e/query.e2e.test.ts @@ -12,6 +12,7 @@ import { readBootstrap, type E2EBootstrap } from "./bootstrap-data"; import { assertCompactColumns, assertCompletedQuery } from "./card-query"; import { cleanupConfigHome, mkTempConfigHome, runCli } from "./run-cli"; import { cliErrorMessage } from "./cli-error"; +import { invalidDatabaseRejection } from "./server-gate"; import { SEEDED } from "./seed/seeded"; const VALID_QUERY = { @@ -181,7 +182,7 @@ describe("query e2e", () => { expect(result.exitCode).toBe(1); expect(cliErrorMessage(result.stderr)).toContain( - 'database: should be an integer, received: "My DB"', + invalidDatabaseRejection('database: should be an integer, received: "My DB"'), ); expect(result.stdout).toBe(""); }); diff --git a/tests/e2e/segment.e2e.test.ts b/tests/e2e/segment.e2e.test.ts index bd480a58..f7075eab 100644 --- a/tests/e2e/segment.e2e.test.ts +++ b/tests/e2e/segment.e2e.test.ts @@ -28,8 +28,12 @@ const NEW_SEGMENT_BODY: SegmentCreateInput = { table_id: SEEDED.tables.orders, description: SEGMENT_DESCRIPTION, definition: { - "source-table": SEEDED.tables.orders, - filter: [">", ["field", SEEDED.fields.ordersId, null], 0], + database: SEEDED.warehouseDbId, + type: "query", + query: { + "source-table": SEEDED.tables.orders, + filter: [">", ["field", SEEDED.fields.ordersId, null], 0], + }, }, }; diff --git a/tests/e2e/server-gate.ts b/tests/e2e/server-gate.ts index 03500ac1..f28849d3 100644 --- a/tests/e2e/server-gate.ts +++ b/tests/e2e/server-gate.ts @@ -128,3 +128,15 @@ export function serverRejectedMessage(): string { ? "Metabase returned 500." : "Metabase returned 400."; } + +// From v62 a query the server cannot normalize — a database id that is not an integer, say — is +// refused with one message for the whole query before any field-level schema check runs; through +// v61 the schema check runs first and names the field it rejected. +const QUERY_NORMALIZATION_VERSION = 62; +const QUERY_NORMALIZATION_MESSAGE = "Invalid query: missing or invalid Database ID (:database)"; + +export function invalidDatabaseRejection(fieldLevelMessage: string): string { + return serverVersionBelow(QUERY_NORMALIZATION_VERSION) + ? fieldLevelMessage + : QUERY_NORMALIZATION_MESSAGE; +} diff --git a/tests/e2e/setup/bootstrap.ts b/tests/e2e/setup/bootstrap.ts index 0640e7ee..14c762dd 100644 --- a/tests/e2e/setup/bootstrap.ts +++ b/tests/e2e/setup/bootstrap.ts @@ -77,6 +77,7 @@ const JSON_CONTENT_TYPE = "application/json"; const SessionPropertiesResponse = z.object({ "setup-token": z.string().nullish() }).loose(); const SessionResponse = z.object({ id: z.string() }); const ApiKeyResponse = z.object({ unmasked_key: z.string() }).loose(); +const AdminUserResponse = z.object({ personal_collection_id: z.number().int().positive() }).loose(); const EntityWithIdResponse = z.object({ id: z.number() }).loose(); const FieldMeta = z.object({ id: z.number().int(), name: z.string() }).loose(); const TableMeta = z @@ -128,6 +129,7 @@ async function main(): Promise { const sessionId = await ensureAdminSessionId(); await assertServerNotSeeded(sessionId); + const adminPersonalCollectionId = await ensureAdminPersonalCollection(sessionId); const adminApiKey = await mintApiKey(sessionId, "e2e-admin-key", E2E_GROUPS.ADMIN); const client = apiKeyClient(adminApiKey); @@ -137,7 +139,7 @@ async function main(): Promise { await enableTransforms(client); await reportTransformsUsable(client); } - const seeded = await seedContent(client, libraryReady(probed)); + const seeded = await seedContent(client, libraryReady(probed), adminPersonalCollectionId); const oauthSupported = (await tryDiscoverMetadata(BASE_URL, USER_AGENT)) !== null; const server = { ...probed, oauthSupported }; @@ -460,7 +462,11 @@ async function enableTransforms(client: Transport): Promise { } } -async function seedContent(client: Transport, libraryEnabled: boolean): Promise { +async function seedContent( + client: Transport, + libraryEnabled: boolean, + adminPersonalCollectionId: number, +): Promise { const warehouseDbId = await createEntityId(client, "/api/database", { name: WAREHOUSE_DB_NAME, engine: "postgres", @@ -532,6 +538,7 @@ async function seedContent(client: Transport, libraryEnabled: boolean): Promise< tables, fields, libraryDataCollectionId, + adminPersonalCollectionId, }; } @@ -710,6 +717,18 @@ async function tryLogin(): Promise | null> { return parsed.success ? parsed.data : null; } +// Metabase creates a user's personal collection on the first request that hydrates it, and a +// collection listing on an instance that has none trips a nil-set bug in the `is_personal` +// hydration (Metabase master, 2026-08) once more than one collection exists. Nothing else here +// would create one: every seed request runs on an api key, and an api-key user gets no personal +// collection. So ask for the admin once on the session before anything lists collections. +async function ensureAdminPersonalCollection(sessionId: string): Promise { + const admin = await fetchJson(`${BASE_URL}/api/user/current`, AdminUserResponse, { + headers: { [SESSION_HEADER]: sessionId }, + }); + return admin.personal_collection_id; +} + async function keyStillWorks(apiKey: string): Promise { const client = apiKeyClient(apiKey); try { diff --git a/tests/e2e/transform.e2e.test.ts b/tests/e2e/transform.e2e.test.ts index 10ecf918..846369ad 100644 --- a/tests/e2e/transform.e2e.test.ts +++ b/tests/e2e/transform.e2e.test.ts @@ -23,7 +23,7 @@ import { readBootstrap, type E2EBootstrap } from "./bootstrap-data"; import { cleanupConfigHome, mkTempConfigHome, runCli } from "./run-cli"; import { cliErrorMessage } from "./cli-error"; import { SEEDED } from "./seed/seeded"; -import { requireServer, serverVersionBelow } from "./server-gate"; +import { invalidDatabaseRejection, requireServer, serverVersionBelow } from "./server-gate"; const FIRST_TRANSFORM_ID = 1; const TRANSFORM_NAME = "e2e_transform"; @@ -484,7 +484,9 @@ describe.skipIf(skipReason !== null)("transform e2e", () => { }); expect(result.exitCode).toBe(1); - expect(result.stderr).toContain("source.query.lib/metadata: missing required key"); + expect(result.stderr).toContain( + invalidDatabaseRejection("source.query.lib/metadata: missing required key"), + ); expect(result.stdout).toBe(""); });