diff --git a/e2e/support/start-mock-trino.ts b/e2e/support/start-mock-trino.ts index 83ebff63..8a89241e 100644 --- a/e2e/support/start-mock-trino.ts +++ b/e2e/support/start-mock-trino.ts @@ -60,7 +60,12 @@ const routes: [string, object][] = [ 'SHOULD_ERROR', { id: 'q-err', - error: { message: 'syntax error at position 7', errorCode: 1 }, + error: { + message: 'syntax error at position 7', + errorCode: 1, + errorName: 'SYNTAX_ERROR', + errorType: 'USER_ERROR' + }, stats: { state: 'FAILED' } } ], @@ -113,6 +118,8 @@ const DEFAULT_RESPONSE = { // FROM exact_limit_table exactly MAX_CLIENT_ROWS rows, then an empty trailing page // FAILS_SILENTLY first response FAILED without an error object // FAILS_LATER one page of rows, then FAILED without an error object +// FAILS_INTERNALLY first response FAILED with an INTERNAL_ERROR +// POLL_UNAVAILABLE QUEUED, then GET answers HTTP 502 with an HTML body // HOLD: RUNNING until released via POST /__mock/release/ // GATE: submit response withheld until released (combinable) // @@ -133,6 +140,8 @@ interface MockQuery { /** Release id for HOLD queries, which stay RUNNING until released. */ hold?: string; cancelled?: boolean; + /** GET nextUri answers HTTP 502, as a proxy in front of an unavailable Trino. */ + pollUnavailable?: boolean; } const queries = new Map(); @@ -249,6 +258,21 @@ function startPagedQuery(id: string, sql: string): Page | null { if (sql.includes('FAILS_SILENTLY')) { return { id, stats: { state: 'FAILED' } }; } + if (sql.includes('FAILS_INTERNALLY')) { + return { + id, + error: { + message: 'java.lang.IllegalStateException: metastore thrift://hive.internal:9083', + errorName: 'GENERIC_INTERNAL_ERROR', + errorType: 'INTERNAL_ERROR' + }, + stats: { state: 'FAILED' } + }; + } + if (sql.includes('POLL_UNAVAILABLE')) { + queries.set(id, { sql, pages: [], pollUnavailable: true }); + return { id, nextUri: nextUri(id, 1), stats: { state: 'QUEUED' } }; + } if (sql.includes('FAILS_LATER')) { queries.set(id, { sql, @@ -333,6 +357,11 @@ http return; } if (req.method === 'GET' && executing) { + if (queries.get(executing[1])?.pollUnavailable) { + res.writeHead(502, { 'Content-Type': 'text/html' }); + res.end('502 Bad Gateway from proxy.internal'); + return; + } pollQuery(executing[1], Number(executing[2])).then((page) => sendJson(res, page)); return; } diff --git a/e2e/trino/query-lifecycle.spec.ts b/e2e/trino/query-lifecycle.spec.ts index 0e9b0b04..3cac4f8d 100644 --- a/e2e/trino/query-lifecycle.spec.ts +++ b/e2e/trino/query-lifecycle.spec.ts @@ -4,7 +4,7 @@ import { mockMarker, mockTrinoStatements, releaseMockTrino } from '../support/mo /** * Query lifecycle against the multi-page mock Trino protocol: polling, running - * state, cancellation and failures reported without error details. + * state, cancellation and failures, including which error details reach the user. */ test.describe('Trino query lifecycle', () => { test.use({ locale: 'en-US' }); @@ -110,6 +110,34 @@ test.describe('Trino query lifecycle', () => { await expect(page.getByText('Query failed')).toBeVisible(); }); + test('withholds the message of a Trino internal error', async ({ page }) => { + await openEditor(page, 'SELECT FAILS_INTERNALLY'); + + await runButton(page).click(); + await waitForQueryComplete(page); + + await expect(statusBadge(page)).toHaveAttribute('data-query-state', 'FAILED'); + await expect( + page.getByText( + 'Trino could not run the query because of a server-side error (GENERIC_INTERNAL_ERROR).' + ) + ).toBeVisible(); + await expect(page.getByText('hive.internal')).toHaveCount(0); + }); + + test('withholds the response body when Trino is unavailable during polling', async ({ page }) => { + await openEditor(page, 'SELECT POLL_UNAVAILABLE'); + + await runButton(page).click(); + await waitForQueryComplete(page); + + await expect(statusBadge(page)).toHaveAttribute('data-query-state', 'FAILED'); + await expect( + page.getByText('Trino could not be reached or sent an invalid response.') + ).toBeVisible(); + await expect(page.getByText('proxy.internal')).toHaveCount(0); + }); + test('re-running between statements stops the previous script', async ({ page }) => { const marker = mockMarker(); const gate = releaseId(); diff --git a/e2e/trino/trino.spec.ts b/e2e/trino/trino.spec.ts index 0f7b2ea1..697b09c4 100644 --- a/e2e/trino/trino.spec.ts +++ b/e2e/trino/trino.spec.ts @@ -133,6 +133,7 @@ test.describe('Trino query editor', () => { // Monaco also renders role="alert" nodes for its own accessibility — filter by content. const alert = page.getByRole('alert').filter({ hasText: 'Query error' }); await expect(alert).toBeVisible(); + // Errors in the user's query keep Trino's message. await expect(alert.getByText('syntax error at position 7')).toBeVisible(); }); diff --git a/messages/de.json b/messages/de.json index bd7844fb..542b24db 100644 --- a/messages/de.json +++ b/messages/de.json @@ -64,6 +64,10 @@ "header_sign_out": "Abmelden", "trino_results_empty": "Keine Ergebnisse", "trino_query_error": "Abfragefehler", + "trino_query_error_server": "Trino konnte die Abfrage wegen eines serverseitigen Fehlers nicht ausführen.", + "trino_query_error_server_named": "Trino konnte die Abfrage wegen eines serverseitigen Fehlers nicht ausführen ({name}).", + "trino_query_error_unavailable": "Trino ist nicht erreichbar oder hat eine ungültige Antwort gesendet.", + "trino_query_error_unknown": "Abfrage fehlgeschlagen", "trino_query_connection_lost": "Verbindung zum Server verloren.", "trino_running": "Wird ausgeführt...", "trino_connection_url_placeholder": "https://trino.example.com:8443", diff --git a/messages/en.json b/messages/en.json index 4a25cd94..58851004 100644 --- a/messages/en.json +++ b/messages/en.json @@ -64,6 +64,10 @@ "header_sign_out": "Sign out", "trino_results_empty": "No results", "trino_query_error": "Query error", + "trino_query_error_server": "Trino could not run the query because of a server-side error.", + "trino_query_error_server_named": "Trino could not run the query because of a server-side error ({name}).", + "trino_query_error_unavailable": "Trino could not be reached or sent an invalid response.", + "trino_query_error_unknown": "Query failed", "trino_query_connection_lost": "Lost connection to the server.", "trino_running": "Running...", "trino_connection_url_placeholder": "https://trino.example.com:8443", diff --git a/src/lib/components/trino/StatementResult.svelte b/src/lib/components/trino/StatementResult.svelte index c49cc466..ff3ee162 100644 --- a/src/lib/components/trino/StatementResult.svelte +++ b/src/lib/components/trino/StatementResult.svelte @@ -5,7 +5,7 @@ import IconExpandMore from 'virtual:icons/material-symbols/expand-more'; import IconOpenInNew from 'virtual:icons/material-symbols/open-in-new'; import IconDownload from 'virtual:icons/material-symbols/download'; - import { MAX_CLIENT_ROWS, type QuerySnapshot } from '$lib/types/query'; + import { MAX_CLIENT_ROWS, type QueryError, type QuerySnapshot } from '$lib/types/query'; import { initPageSize, type PageSize } from '$lib/types/pagination.js'; import Pagination from '$lib/components/Pagination.svelte'; @@ -35,7 +35,7 @@ const lastPage = $derived(totalPages - 1); const rowStart = $derived(currentPage * pageSize + 1); const rowEnd = $derived(currentPage * pageSize + displayedRows.length); - const stmtError = $derived(result.error); + const stmtError = $derived(result.error ? formatQueryError(result.error) : null); const showHeader = $derived(totalStatements > 1); const rowLimitWarning = $derived( showHeader && result.rowLimitReached @@ -43,6 +43,21 @@ : null ); + function formatQueryError(error: QueryError): string { + switch (error.kind) { + case 'query': + return error.message; + case 'server': + return error.name + ? m.trino_query_error_server_named({ name: error.name }) + : m.trino_query_error_server(); + case 'unavailable': + return m.trino_query_error_unavailable(); + case 'unknown': + return m.trino_query_error_unknown(); + } + } + const CSV_HEADERS_KEY = 'trino_csv_include_headers'; let includeHeaders = $state(browser ? localStorage.getItem(CSV_HEADERS_KEY) === 'true' : false); diff --git a/src/lib/server/trino/client.test.ts b/src/lib/server/trino/client.test.ts index a7fd9524..1d9aa202 100644 --- a/src/lib/server/trino/client.test.ts +++ b/src/lib/server/trino/client.test.ts @@ -8,6 +8,7 @@ vi.mock('$env/dynamic/private', () => ({ env: mockEnv })); vi.mock('$lib/server/logging', () => import('$lib/test-utils/mock-logger.js')); import { TrinoClient, trinoMetadataQuery } from './client.js'; +import { TrinoHttpError, TrinoQueryFailure } from './errors.js'; describe('TrinoClient X-Trino-User header', () => { let fetchMock: ReturnType; @@ -151,9 +152,24 @@ describe('trinoMetadataQuery (connection test path)', () => { vi.stubGlobal('fetch', fetchMock); const client = new TrinoClient({ serverUrl: 'http://trino:8080' }); await expect(trinoMetadataQuery(client, 'SELECT 1', { user: 'alice' })).rejects.toThrow( - 'Access Denied' + expect.objectContaining({ + constructor: TrinoQueryFailure, + trinoError: { message: 'Access Denied' } + }) ); }); + + it('throws an HTTP error without the response body when polling fails', async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce({ ok: true, json: async () => ({ id: 'q1', nextUri: 'http://n/1' }) }) + .mockResolvedValueOnce({ ok: false, status: 502, text: async () => 'proxy' }); + vi.stubGlobal('fetch', fetchMock); + const client = new TrinoClient({ serverUrl: 'http://trino:8080' }); + const result = trinoMetadataQuery(client, 'SELECT 1', { user: 'alice' }); + await expect(result).rejects.toBeInstanceOf(TrinoHttpError); + await expect(result).rejects.toThrow('Trino poll failed (502)'); + }); }); describe('resolveTrinoPublicUrl', () => { diff --git a/src/lib/server/trino/client.ts b/src/lib/server/trino/client.ts index 0fb801ab..d3ad7aa2 100644 --- a/src/lib/server/trino/client.ts +++ b/src/lib/server/trino/client.ts @@ -2,6 +2,7 @@ import { env } from '$env/dynamic/private'; import { readFileSync } from 'node:fs'; import { Agent } from 'undici'; import { logger } from '$lib/server/logging'; +import { TrinoHttpError, TrinoQueryFailure, type TrinoQueryError } from './errors.js'; import { getUserTrinoClient, getUserTrinoUrl } from './user-clients.js'; const log = logger.child({ module: 'trino-client' }); @@ -20,13 +21,6 @@ export interface TrinoQueryStats { elapsedTimeMillis?: number; } -interface TrinoQueryError { - message: string; - errorCode?: number; - errorName?: string; - errorType?: string; -} - export interface TrinoQueryResult { id: string; nextUri?: string; @@ -91,7 +85,7 @@ export class TrinoClient { if (!res.ok) { const text = await res.text().catch(() => ''); - throw new Error(`Trino POST /v1/statement failed (${res.status}): ${text}`); + throw new TrinoHttpError('POST /v1/statement', res.status, text); } return res.json() as Promise; @@ -112,7 +106,7 @@ export class TrinoClient { if (!res.ok) { const text = await res.text().catch(() => ''); - throw new Error(`Trino poll failed (${res.status}): ${text}`); + throw new TrinoHttpError('poll', res.status, text); } return res.json() as Promise; @@ -139,7 +133,7 @@ export class TrinoClient { // 404 is expected — the query may have already finished and been cleaned up. if (!res.ok && res.status !== 404) { const text = await res.text().catch(() => ''); - throw new Error(`Trino DELETE /v1/query failed (${res.status}): ${text}`); + throw new TrinoHttpError('DELETE /v1/query', res.status, text); } } @@ -154,7 +148,7 @@ export class TrinoClient { if (!res.ok && res.status !== 404 && res.status !== 410) { const text = await res.text().catch(() => ''); - throw new Error(`Trino DELETE nextUri failed (${res.status}): ${text}`); + throw new TrinoHttpError('DELETE nextUri', res.status, text); } } } @@ -267,14 +261,14 @@ export async function trinoMetadataQuery( const rows: unknown[][] = result.data ? [...result.data] : []; if (result.error) { - throw new Error(result.error.message); + throw new TrinoQueryFailure(result.error); } while (result.nextUri) { result = await client.poll(result.nextUri, { user: options.user, signal }); if (result.error) { - throw new Error(result.error.message); + throw new TrinoQueryFailure(result.error); } if (result.columns && columns.length === 0) { columns = result.columns; diff --git a/src/lib/server/trino/errors.test.ts b/src/lib/server/trino/errors.test.ts new file mode 100644 index 00000000..a76661ee --- /dev/null +++ b/src/lib/server/trino/errors.test.ts @@ -0,0 +1,56 @@ +import { describe, it, expect, vi } from 'vitest'; + +vi.mock('$lib/server/logging', () => import('$lib/test-utils/mock-logger.js')); + +import { TrinoHttpError, TrinoQueryFailure, classifyTrinoError, toQueryError } from './errors.js'; + +describe('toQueryError', () => { + it.each(['USER_ERROR', 'INSUFFICIENT_RESOURCES'])('passes on the message of %s', (errorType) => { + expect(toQueryError({ message: 'line 1:8: Column x cannot be resolved', errorType })).toEqual({ + kind: 'query', + message: 'line 1:8: Column x cannot be resolved' + }); + }); + + it.each(['INTERNAL_ERROR', 'EXTERNAL', undefined])( + 'withholds the message of %s and keeps the error name', + (errorType) => { + expect( + toQueryError({ + message: 'Failed connecting to metastore thrift://hive.internal:9083', + errorName: 'HIVE_METASTORE_ERROR', + errorType + }) + ).toEqual({ kind: 'server', name: 'HIVE_METASTORE_ERROR' }); + } + ); + + it.each([undefined, {}, { errorType: 'USER_ERROR' }])( + 'reports a failure without a message as unknown (%o)', + (trinoError) => { + expect(toQueryError(trinoError)).toEqual({ kind: 'unknown' }); + } + ); +}); + +describe('classifyTrinoError', () => { + it('classifies a Trino query failure by its error type', () => { + const err = new TrinoQueryFailure({ message: 'Access Denied', errorType: 'USER_ERROR' }); + expect(classifyTrinoError(err)).toEqual({ kind: 'query', message: 'Access Denied' }); + }); + + it.each([ + ['an HTTP error', new TrinoHttpError('poll', 502, 'proxy.internal')], + ['a network failure', new TypeError('fetch failed')] + ])('reports %s as unavailable', (_, err) => { + expect(classifyTrinoError(err)).toEqual({ kind: 'unavailable' }); + }); +}); + +describe('TrinoHttpError', () => { + it('keeps the response body out of the message and caps it', () => { + const err = new TrinoHttpError('poll', 502, 'x'.repeat(5000)); + expect(err.message).toBe('Trino poll failed (502)'); + expect(err.body).toHaveLength(2000); + }); +}); diff --git a/src/lib/server/trino/errors.ts b/src/lib/server/trino/errors.ts new file mode 100644 index 00000000..7e322d0e --- /dev/null +++ b/src/lib/server/trino/errors.ts @@ -0,0 +1,88 @@ +import { logger } from '$lib/server/logging'; +import type { QueryError } from '$lib/types/query.js'; + +const log = logger.child({ module: 'trino-errors' }); + +/** Error object of a failed Trino query (only fields we read). */ +export interface TrinoQueryError { + message?: string; + errorCode?: number; + errorName?: string; + errorType?: string; +} + +/** Response bodies are kept for logging only; cap them so a proxy's HTML page cannot flood the logs. */ +const MAX_LOGGED_BODY_LENGTH = 2000; + +/** Trino answered with a non-2xx HTTP status. The body never reaches the client. */ +export class TrinoHttpError extends Error { + readonly body: string; + + constructor( + operation: string, + readonly status: number, + body: string + ) { + super(`Trino ${operation} failed (${status})`); + this.name = 'TrinoHttpError'; + this.body = body.slice(0, MAX_LOGGED_BODY_LENGTH); + } +} + +/** Trino reported a failed query. */ +export class TrinoQueryFailure extends Error { + constructor(readonly trinoError: TrinoQueryError) { + super(trinoError.message ?? 'Query failed'); + this.name = 'TrinoQueryFailure'; + } +} + +/** + * Error types whose message describes the user's own query. Trino runs the + * query as the user, so these only mention objects the user may access. + * INTERNAL_ERROR and EXTERNAL messages can contain exception text, hostnames + * or storage paths. + */ +const USER_FACING_ERROR_TYPES = new Set(['USER_ERROR', 'INSUFFICIENT_RESOURCES']); + +/** + * Classify the error object of a failed Trino query. Server errors are logged, + * as their message is withheld from the user. + */ +export function toQueryError( + trinoError: TrinoQueryError | undefined, + trinoQueryId?: string +): QueryError { + if (!trinoError?.message) return { kind: 'unknown' }; + if (USER_FACING_ERROR_TYPES.has(trinoError.errorType ?? '')) { + return { kind: 'query', message: trinoError.message }; + } + log.warn( + { + trino_query_id: trinoQueryId, + trino_error_type: trinoError.errorType, + trino_error_name: trinoError.errorName, + trino_error_message: trinoError.message + }, + 'Trino reported a server error' + ); + return { kind: 'server', name: trinoError.errorName ?? null }; +} + +/** Classify an error thrown while talking to Trino. */ +export function classifyTrinoError(err: unknown): QueryError { + if (err instanceof TrinoQueryFailure) return toQueryError(err.trinoError); + // HTTP errors, network failures and unparsable responses. + return { kind: 'unavailable' }; +} + +/** + * Error body for an API response after a failed Trino call. Only errors in the + * user's query keep their message; others get a generic text with the request ID, + * so the client shows its translated fallback. + */ +export function trinoErrorBody(err: unknown, requestId: string): App.Error { + const queryError = classifyTrinoError(err); + if (queryError.kind === 'query') return { message: queryError.message }; + return { message: 'Trino request failed', requestId }; +} diff --git a/src/lib/server/trino/queries.test.ts b/src/lib/server/trino/queries.test.ts index ebcbbe04..470a45ca 100644 --- a/src/lib/server/trino/queries.test.ts +++ b/src/lib/server/trino/queries.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import type { TrinoClient, TrinoQueryResult } from './client.js'; import type { TrinoQuery } from './queries.js'; +import type { QueryError } from '$lib/types/query.js'; // Stable mock functions shared across module re-imports (each test imports a // fresh queries.ts so its in-memory store starts empty). @@ -48,7 +49,7 @@ function pending(id: string): TrinoQueryResult { } /** Make collectResults drive each polled query to the given terminal state. */ -function collectTo(state: 'FINISHED' | 'FAILED' | 'CANCELLED', error: string | null = null) { +function collectTo(state: 'FINISHED' | 'FAILED' | 'CANCELLED', error: QueryError | null = null) { return async (query: TrinoQuery) => { query.columns = [{ name: 'x', type: 'integer' }]; query.rows = [[1]]; @@ -190,13 +191,13 @@ describe('startScript', () => { }); it('stops at the first statement that does not finish', async () => { - mocks.collectResults.mockImplementationOnce(collectTo('FAILED', 'Table not found')); + mocks.collectResults.mockImplementationOnce(collectTo('FAILED', { kind: 'unknown' })); await q.startScript(trinoClient(), USER, TAB, ['SELECT 1', 'SELECT 2'], OPTS); expect(client.submit).toHaveBeenCalledTimes(1); const [snapshot] = q.getQuerySnapshots(USER, TAB); - expect(snapshot).toMatchObject({ state: 'FAILED', error: 'Table not found' }); + expect(snapshot).toMatchObject({ state: 'FAILED', error: { kind: 'unknown' } }); }); it('marks a statement without nextUri as finished without polling', async () => { @@ -258,7 +259,7 @@ describe('startScript', () => { it('fails and stops on an error in the submit response', async () => { client.submit.mockResolvedValueOnce({ id: 'q1', - error: { message: 'line 1:1: mismatched input' } + error: { message: 'line 1:1: mismatched input', errorType: 'USER_ERROR' } }); await q.startScript(trinoClient(), USER, TAB, ['SELEC 1', 'SELECT 2'], OPTS); // typos:ignore-line @@ -268,7 +269,10 @@ describe('startScript', () => { expect(mocks.activeDec).not.toHaveBeenCalled(); expect(mocks.queryTotalInc).toHaveBeenCalledWith({ outcome: 'failed' }); const [snapshot] = q.getQuerySnapshots(USER, TAB); - expect(snapshot).toMatchObject({ state: 'FAILED', error: 'line 1:1: mismatched input' }); + expect(snapshot).toMatchObject({ + state: 'FAILED', + error: { kind: 'query', message: 'line 1:1: mismatched input' } + }); }); it('treats a FAILED submit response without error details as a failure', async () => { @@ -280,18 +284,10 @@ describe('startScript', () => { expect(mocks.queryTotalInc).not.toHaveBeenCalledWith({ outcome: 'completed' }); expect(q.getQuerySnapshots(USER, TAB)[0]).toMatchObject({ state: 'FAILED', - error: 'Query failed' + error: { kind: 'unknown' } }); }); - it('falls back to a generic message when the submit error has none', async () => { - client.submit.mockResolvedValueOnce({ id: 'q1', error: {} }); - - await q.startScript(trinoClient(), USER, TAB, ['SELECT 1'], OPTS); - - expect(q.getQuerySnapshots(USER, TAB)[0].error).toBe('Query failed'); - }); - it('stops without storing a result when submit throws', async () => { client.submit.mockRejectedValueOnce(new Error('ECONNREFUSED')); @@ -324,7 +320,7 @@ describe('startScript', () => { expect(mocks.queryTotalInc).toHaveBeenCalledWith({ outcome: 'failed' }); expect(q.getQuerySnapshots(USER, TAB)[0]).toMatchObject({ state: 'FAILED', - error: 'Internal poll error' + error: { kind: 'unknown' } }); }); diff --git a/src/lib/server/trino/queries.ts b/src/lib/server/trino/queries.ts index 6161be8c..1ca5f901 100644 --- a/src/lib/server/trino/queries.ts +++ b/src/lib/server/trino/queries.ts @@ -5,11 +5,13 @@ import { INITIAL_PROGRESS, isTerminal, type Column, + type QueryError, type QueryProgress, type QuerySnapshot, type QueryState } from '$lib/types/query.js'; import { type TrinoClient, type TrinoQueryStats, resolveTrinoPublicUrl } from './client.js'; +import { toQueryError } from './errors.js'; import { collectResults } from './result-collector.js'; const log = logger.child({ module: 'trino-queries' }); @@ -28,7 +30,7 @@ export interface TrinoQuery { progress: QueryProgress; columns: Column[]; rows: unknown[][]; - error: string | null; + error: QueryError | null; rowLimitReached: boolean; sql: string; startedAt: number; @@ -218,7 +220,7 @@ async function submitStatement( }; if (submitResult.error || initialState === 'FAILED') { - query.error = submitResult.error?.message ?? 'Query failed'; + query.error = toQueryError(submitResult.error, trinoQueryId); terminateQuery(query, 'FAILED', { decrementGauge: false }); trinoQueryTotal.inc({ outcome: 'failed' }); } @@ -324,7 +326,7 @@ async function runStatements( } catch (err) { log.error({ err, trino_query_id: query.trinoQueryId }, 'poll loop crashed'); if (!isTerminal(query.state)) { - query.error = 'Internal poll error'; + query.error = { kind: 'unknown' }; terminateQuery(query, 'FAILED'); trinoQueryTotal.inc({ outcome: 'failed' }); } diff --git a/src/lib/server/trino/result-collector.test.ts b/src/lib/server/trino/result-collector.test.ts index bdee9d7f..9ab4cceb 100644 --- a/src/lib/server/trino/result-collector.test.ts +++ b/src/lib/server/trino/result-collector.test.ts @@ -10,6 +10,7 @@ vi.mock('$lib/server/metrics.js', () => ({ import { collectResults } from './result-collector.js'; import { MAX_CLIENT_ROWS, INITIAL_PROGRESS } from '$lib/types/query.js'; import type { TrinoQuery } from './queries.js'; +import { TrinoHttpError } from './errors.js'; /** Just over half the limit, so two pages cross MAX_CLIENT_ROWS. */ function page(): unknown[][] { @@ -160,14 +161,26 @@ describe('collectResults failure', () => { const poll = vi.fn(async () => ({ id: 'q1', stats: { state: 'FAILED' }, - error: { message: 'Division by zero' } + error: { message: 'Division by zero', errorType: 'USER_ERROR' } })); const query = makeQuery({ poll }); await collectResults(query); expect(query.state).toBe('FAILED'); - expect(query.error).toBe('Division by zero'); + expect(query.error).toEqual({ kind: 'query', message: 'Division by zero' }); + }); + + it('fails without the response body when polling gets an HTTP error', async () => { + const poll = vi.fn(async () => { + throw new TrinoHttpError('poll', 502, 'internal-proxy.local'); + }); + + const query = makeQuery({ poll }); + await collectResults(query); + + expect(query.state).toBe('FAILED'); + expect(query.error).toEqual({ kind: 'unavailable' }); }); it('fails on a FAILED state without error details', async () => { @@ -185,6 +198,6 @@ describe('collectResults failure', () => { expect(poll).toHaveBeenCalledTimes(1); expect(query.state).toBe('FAILED'); - expect(query.error).toBe('Query failed'); + expect(query.error).toEqual({ kind: 'unknown' }); }); }); diff --git a/src/lib/server/trino/result-collector.ts b/src/lib/server/trino/result-collector.ts index e319d0b0..95419821 100644 --- a/src/lib/server/trino/result-collector.ts +++ b/src/lib/server/trino/result-collector.ts @@ -1,6 +1,7 @@ import { logger } from '$lib/server/logging'; import { trinoQueryTotal } from '$lib/server/metrics.js'; import { MAX_CLIENT_ROWS, isTerminal } from '$lib/types/query.js'; +import { classifyTrinoError, toQueryError } from './errors.js'; import { type TrinoQuery, mapTrinoState, toQueryProgress, terminateQuery } from './queries.js'; const log = logger.child({ module: 'trino-result-collector' }); @@ -30,7 +31,7 @@ export async function collectResults(query: TrinoQuery): Promise { if (isTerminal(query.state)) return; log.error({ err, trino_query_id: query.trinoQueryId }, 'poll failed'); - query.error = err instanceof Error ? err.message : 'Poll failed'; + query.error = classifyTrinoError(err); terminateQuery(query, 'FAILED'); trinoQueryTotal.inc({ outcome: 'failed' }); return; @@ -61,7 +62,7 @@ export async function collectResults(query: TrinoQuery): Promise { } if (result.error || result.stats?.state === 'FAILED') { - query.error = result.error?.message ?? 'Query failed'; + query.error = toQueryError(result.error, query.trinoQueryId); terminateQuery(query, 'FAILED'); trinoQueryTotal.inc({ outcome: 'failed' }); return; diff --git a/src/lib/types/query.ts b/src/lib/types/query.ts index c8a203ab..0ac24176 100644 --- a/src/lib/types/query.ts +++ b/src/lib/types/query.ts @@ -26,6 +26,20 @@ export interface QueryProgress { elapsedTimeMillis: number; } +/** + * Why a query failed, safe to show to the user. Only errors in the user's own + * query carry Trino's message; server-side errors could expose internals. + */ +export type QueryError = + /** Error in the query itself (syntax, missing table, access denied, resource limit). */ + | { kind: 'query'; message: string } + /** Trino or a connector failed; only Trino's error name is passed on. */ + | { kind: 'server'; name: string | null } + /** Trino could not be reached or sent an invalid response. */ + | { kind: 'unavailable' } + /** Failed without details. */ + | { kind: 'unknown' }; + /** Whether the query state is terminal (no further transitions expected). */ export function isTerminal(state: QueryState): boolean { return state === 'FINISHED' || state === 'FAILED' || state === 'CANCELLED'; @@ -40,7 +54,7 @@ export interface QuerySnapshot { rows: unknown[][]; /** Rows fetched so far. Populated even in lightweight snapshots, where `rows` is empty. */ rowCount: number; - error: string | null; + error: QueryError | null; /** Fetching stopped and the query was cancelled because more than MAX_CLIENT_ROWS rows arrived. */ rowLimitReached: boolean; sql: string; diff --git a/src/routes/(app)/api/trino/catalog/+server.ts b/src/routes/(app)/api/trino/catalog/+server.ts index e4ac03b9..59e8a0f5 100644 --- a/src/routes/(app)/api/trino/catalog/+server.ts +++ b/src/routes/(app)/api/trino/catalog/+server.ts @@ -2,6 +2,7 @@ import { json, error } from '@sveltejs/kit'; import { resolveTrinoClient, trinoMetadataQuery } from '$lib/server/trino/client.js'; import { getTrinoUser, getUserId } from '$lib/server/auth-utils.js'; import { quoteIdentifier, quoteStringLiteral } from '$lib/editor/identifiers.js'; +import { trinoErrorBody } from '$lib/server/trino/errors.js'; import type { RequestHandler } from './$types'; export const GET: RequestHandler = async ({ url, locals }) => { @@ -59,8 +60,7 @@ export const GET: RequestHandler = async ({ url, locals }) => { ); return json(rows); } catch (err) { - const msg = err instanceof Error ? err.message : 'unknown error'; log.info({ err, level, catalog, schema, table }, 'catalog metadata query failed'); - error(502, msg); + error(502, trinoErrorBody(err, locals.requestId)); } }; diff --git a/src/routes/(app)/api/trino/catalog/server.test.ts b/src/routes/(app)/api/trino/catalog/server.test.ts index cb6537ac..b8117652 100644 --- a/src/routes/(app)/api/trino/catalog/server.test.ts +++ b/src/routes/(app)/api/trino/catalog/server.test.ts @@ -8,6 +8,7 @@ vi.mock('$lib/server/trino/client.js', () => ({ import { GET } from './+server.js'; import { resolveTrinoClient, trinoMetadataQuery } from '$lib/server/trino/client.js'; import type { TrinoClient } from '$lib/server/trino/client.js'; +import { TrinoQueryFailure } from '$lib/server/trino/errors.js'; const fakeClient = { fake: true } as unknown as TrinoClient; @@ -95,7 +96,9 @@ describe('GET /api/trino/catalog', () => { }); it('throws 502 with the Trino error message when the query fails', async () => { - vi.mocked(trinoMetadataQuery).mockRejectedValue(new Error('Catalog does not exist')); + vi.mocked(trinoMetadataQuery).mockRejectedValue( + new TrinoQueryFailure({ message: 'Catalog does not exist', errorType: 'USER_ERROR' }) + ); await expect(GET(mockEvent({ level: 'schemas', catalog: 'nope' }))).rejects.toThrow( expect.objectContaining({ status: 502, body: { message: 'Catalog does not exist' } }) diff --git a/src/routes/(app)/api/trino/completion/metadata/+server.ts b/src/routes/(app)/api/trino/completion/metadata/+server.ts index 880907be..a25969d8 100644 --- a/src/routes/(app)/api/trino/completion/metadata/+server.ts +++ b/src/routes/(app)/api/trino/completion/metadata/+server.ts @@ -4,6 +4,7 @@ import { resolveTrinoClient, trinoMetadataQuery } from '$lib/server/trino/client import { getTrinoUser, getUserId } from '$lib/server/auth-utils.js'; import { completionEnabled } from '$lib/server/feature-flags.js'; import { quoteIdentifier, quoteStringLiteral } from '$lib/editor/identifiers.js'; +import { trinoErrorBody } from '$lib/server/trino/errors.js'; import type { RequestHandler } from './$types'; // Any name Trino returns is valid; values are quoted/escaped before use in SQL. @@ -115,8 +116,7 @@ export const GET: RequestHandler = async ({ url, locals }) => { ); return json(names); } catch (err) { - const msg = err instanceof Error ? err.message : 'unknown error'; log.info({ err, level, catalog, schema, table }, 'completion metadata query failed'); - error(502, msg); + error(502, trinoErrorBody(err, locals.requestId)); } }; diff --git a/src/routes/(app)/api/trino/completion/metadata/server.test.ts b/src/routes/(app)/api/trino/completion/metadata/server.test.ts index 7e2cca86..3af3db40 100644 --- a/src/routes/(app)/api/trino/completion/metadata/server.test.ts +++ b/src/routes/(app)/api/trino/completion/metadata/server.test.ts @@ -16,6 +16,7 @@ vi.mock('$lib/server/trino/client.js', () => ({ import { GET } from './+server.js'; import { resolveTrinoClient, trinoMetadataQuery } from '$lib/server/trino/client.js'; import type { TrinoClient } from '$lib/server/trino/client.js'; +import { TrinoHttpError, TrinoQueryFailure } from '$lib/server/trino/errors.js'; const fakeClient = { fake: true } as unknown as TrinoClient; @@ -24,7 +25,11 @@ function mockEvent(params: Record) { for (const [k, v] of Object.entries(params)) url.searchParams.set(k, v); return { url, - locals: { logger: { debug: vi.fn(), info: vi.fn() }, user: { id: 'u1', trinoUser: 'alice' } } + locals: { + logger: { debug: vi.fn(), info: vi.fn() }, + requestId: 'req-1', + user: { id: 'u1', trinoUser: 'alice' } + } } as unknown as Parameters[0]; } @@ -154,10 +159,25 @@ describe('GET /api/trino/completion/metadata', () => { }); it('throws 502 with the Trino error message when the query fails', async () => { - vi.mocked(trinoMetadataQuery).mockRejectedValue(new Error('Access Denied')); + vi.mocked(trinoMetadataQuery).mockRejectedValue( + new TrinoQueryFailure({ message: 'Access Denied', errorType: 'USER_ERROR' }) + ); await expect(GET(mockEvent({ level: 'catalogs' }))).rejects.toThrow( expect.objectContaining({ status: 502, body: { message: 'Access Denied' } }) ); }); + + it('throws 502 without upstream details when Trino fails', async () => { + vi.mocked(trinoMetadataQuery).mockRejectedValue( + new TrinoHttpError('POST /v1/statement', 503, 'proxy.internal') + ); + + await expect(GET(mockEvent({ level: 'catalogs' }))).rejects.toThrow( + expect.objectContaining({ + status: 502, + body: { message: 'Trino request failed', requestId: 'req-1' } + }) + ); + }); });