diff --git a/packages/nodes-base/nodes/Databricks/actions/lakebase/openApiDocument.ts b/packages/nodes-base/nodes/Databricks/actions/lakebase/openApiDocument.ts index 5849a0e64a9..13791901c44 100644 --- a/packages/nodes-base/nodes/Databricks/actions/lakebase/openApiDocument.ts +++ b/packages/nodes-base/nodes/Databricks/actions/lakebase/openApiDocument.ts @@ -7,7 +7,7 @@ export const OPENAPI_PATH = /\/openapi\.json$/; /** The Databricks UI path, so every message that names the setting stays in step */ export const OPENAPI_SETTING_PATH = 'Data API > API > Advanced settings > OpenAPI specification'; -export const OPENAPI_DISABLED_NOTICE = `Turn on ${OPENAPI_SETTING_PATH} for this Lakebase project, so the node can list its columns.`; +export const OPENAPI_DISABLED_NOTICE = `Turn on ${OPENAPI_SETTING_PATH} for this Lakebase project.`; export function isOpenApiUnavailable(error: unknown, url: string): boolean { if (!OPENAPI_PATH.test(url)) return false; diff --git a/packages/nodes-base/nodes/Databricks/test/lakebase.test.ts b/packages/nodes-base/nodes/Databricks/test/lakebase.test.ts index b6502055973..c35c85def60 100644 --- a/packages/nodes-base/nodes/Databricks/test/lakebase.test.ts +++ b/packages/nodes-base/nodes/Databricks/test/lakebase.test.ts @@ -266,4 +266,21 @@ describe('lakebaseApiRequest', () => { expect(httpRequestWithAuthentication).toHaveBeenCalledTimes(3); expect(sleep).toHaveBeenCalledWith(1000, undefined); }); + + it('explains the failure the caller actually sees', async () => { + httpRequestWithAuthentication.mockRejectedValue(pgrst205()); + + await expect(lakebaseApiRequest(context, request)).rejects.toMatchObject({ + description: expect.stringContaining('schema and table names'), + }); + }); + + it('does not explain an error the retry recovers from', async () => { + const retried = pgrst205(); + httpRequestWithAuthentication.mockRejectedValueOnce(retried).mockResolvedValueOnce([{ id: 1 }]); + + await expect(lakebaseApiRequest(context, request)).resolves.toEqual([{ id: 1 }]); + + expect(retried.description ?? '').not.toContain('schema and table names'); + }); }); diff --git a/packages/nodes-base/nodes/Databricks/test/lakebaseErrors.test.ts b/packages/nodes-base/nodes/Databricks/test/lakebaseErrors.test.ts new file mode 100644 index 00000000000..1558240c000 --- /dev/null +++ b/packages/nodes-base/nodes/Databricks/test/lakebaseErrors.test.ts @@ -0,0 +1,109 @@ +import type { IHttpRequestOptions, INode, JsonObject } from 'n8n-workflow'; +import { NodeApiError } from 'n8n-workflow'; + +import { LAKEBASE_ERROR_HINTS, makeLakebaseErrorLegible } from '../transport/lakebaseErrors'; + +const node: INode = { + id: '1', + name: 'Databricks', + type: 'n8n-nodes-base.databricks', + typeVersion: 1, + position: [0, 0], + parameters: {}, +}; + +class AxiosError extends Error { + constructor( + message: string, + readonly response: { status: number; data: unknown }, + ) { + super(message); + } +} + +const apiError = (status: number, data: unknown) => + new NodeApiError( + node, + new AxiosError(`Request failed with status code ${status}`, { + status, + data, + }) as unknown as JsonObject, + ); + +const TABLE_URL = 'https://ep.example.com/api/2.0/workspace/1/rest/db/public/orders'; +const request = (url = TABLE_URL): IHttpRequestOptions => ({ method: 'GET', url }); + +describe('Lakebase -> error hints', () => { + it.each([...LAKEBASE_ERROR_HINTS.keys()])('explains how to fix %s', (code) => { + const error = apiError(400, { code, message: `the API said something about ${code}` }); + + makeLakebaseErrorLegible(error, request()); + + expect(error.message).toBe(`the API said something about ${code}`); + expect(error.description).toBe(LAKEBASE_ERROR_HINTS.get(code)); + expect(error.description).not.toBe(error.message); + }); + + // Descriptions are rendered through a sanitiser that deletes anything parsing + // as a tag, so a would reach the user with its value removed + it.each([...LAKEBASE_ERROR_HINTS.entries()])('keeps every word of the %s hint', (_code, hint) => { + expect(hint).not.toMatch(/<[^\s>]+>/); + }); + + it('names the OpenAPI setting when the schema document is missing', () => { + const error = apiError(404, { code: 'PGRST205', message: 'Could not find the table' }); + + makeLakebaseErrorLegible(error, request(`${TABLE_URL}/openapi.json`)); + + expect(error.description).toContain('OpenAPI specification'); + }); + + it('names the table when a table read misses', () => { + const error = apiError(404, { code: 'PGRST205', message: 'Could not find the table' }); + + makeLakebaseErrorLegible(error, request()); + + expect(error.description).toBe(LAKEBASE_ERROR_HINTS.get('PGRST205')); + expect(error.description).not.toContain('OpenAPI specification'); + }); + + it('delegates the workspace permission envelope', () => { + const error = apiError(403, { + error_code: 'PERMISSION_DENIED', + message: 'User lacks permission', + }); + + makeLakebaseErrorLegible(error, request()); + + expect(error.message).toBe('User lacks permission'); + expect(error.description).toContain('Grant'); + }); + + it('truncates an over-long API message', () => { + const error = apiError(400, { code: '23505', message: 'x'.repeat(900) }); + + makeLakebaseErrorLegible(error, request()); + + expect(error.message).toHaveLength(500); + }); + + it.each([ + ['an unknown code', { code: 'PGRST999', message: 'something else' }], + ['no code at all', { message: 'something else' }], + ['a string body', 'not json'], + ['a buffer body', Buffer.from('not json')], + ['no body', undefined], + ])('leaves the error alone for %s', (_name, data) => { + const error = apiError(400, data); + const before = { message: error.message, description: error.description }; + + expect(() => makeLakebaseErrorLegible(error, request())).not.toThrow(); + expect({ message: error.message, description: error.description }).toEqual(before); + }); + + it('ignores an error that did not come from the API', () => { + const error = new Error('socket hang up'); + + expect(() => makeLakebaseErrorLegible(error, request())).not.toThrow(); + }); +}); diff --git a/packages/nodes-base/nodes/Databricks/test/lakebasePickers.test.ts b/packages/nodes-base/nodes/Databricks/test/lakebasePickers.test.ts index ddc2d97f929..16da4b6ad57 100644 --- a/packages/nodes-base/nodes/Databricks/test/lakebasePickers.test.ts +++ b/packages/nodes-base/nodes/Databricks/test/lakebasePickers.test.ts @@ -10,7 +10,8 @@ import type { import { mockDeep } from 'vitest-mock-extended'; import { resolveLakebaseSchemaUrl, resolveLakebaseTableUrl } from '../actions/lakebase/helpers'; -import { OPENAPI_SETTING_PATH } from '../actions/lakebase/openApiDocument'; +import { OPENAPI_DISABLED_NOTICE } from '../actions/lakebase/openApiDocument'; +import { LAKEBASE_ERROR_HINTS } from '../transport/lakebaseErrors'; // The shared Lakebase transport waits before its schema-cache retry vi.mock('@n8n/utils/sleep', () => ({ @@ -502,20 +503,19 @@ describe('listSearch -> getLakebaseTables', () => { expect(error.message).toBe( 'Turn on the "OpenAPI specification" setting of the Data API to list tables, or enter the table name By ID', ); - // Pins that the user is told where the setting lives, not the wording around it - expect(error.description).toContain(OPENAPI_SETTING_PATH); + expect(error.description).toBe(OPENAPI_DISABLED_NOTICE); }); - it('rethrows other Data API errors untouched', async () => { + it('rethrows another Data API error with its own hint, not the OpenAPI one', async () => { const context = createLoadOptionsContext(selectedSchema); const error = apiErrorFromBody(401, { code: 'PGRST301', message: 'invalid token permissions' }); - const { message: originalMessage, description: originalDescription } = error; apiMock(context).mockResolvedValueOnce(endpointsResponse).mockRejectedValueOnce(error); await expect(getLakebaseTables.call(context)).rejects.toBe(error); - expect(error.message).toBe(originalMessage); - expect(error.description).toBe(originalDescription); + expect(error.message).toBe('invalid token permissions'); + expect(error.description).toBe(LAKEBASE_ERROR_HINTS.get('PGRST301')); + expect(error.description).not.toContain('OpenAPI'); }); it('surfaces a legible PERMISSION_DENIED from the endpoints lookup', async () => { diff --git a/packages/nodes-base/nodes/Databricks/transport/lakebase.ts b/packages/nodes-base/nodes/Databricks/transport/lakebase.ts index 6637364c18f..fdffc2dd582 100644 --- a/packages/nodes-base/nodes/Databricks/transport/lakebase.ts +++ b/packages/nodes-base/nodes/Databricks/transport/lakebase.ts @@ -4,13 +4,13 @@ import { sleep } from '@n8n/utils/sleep'; import type { IExecuteFunctions, IHttpRequestOptions, JsonObject } from 'n8n-workflow'; import { NodeApiError, NodeError, NodeOperationError } from 'n8n-workflow'; +import { makeLakebaseErrorLegible } from './lakebaseErrors'; import { databricksApiRequest, getActiveCredentialType, makePermissionErrorLegible, type DatabricksContext, } from '../actions/helpers'; -import { OPENAPI_SETTING_PATH } from '../actions/lakebase/openApiDocument'; // ponytail: single fixed delay; lengthen it if a real schema reload is observed // to take more than 1 s. @@ -36,17 +36,14 @@ function isExpiredToken(error: unknown, body: Record | undefine return /expired/i.test(text) && /jwt|token/i.test(text); } -const OPENAPI_SETTING_HINT = `In Databricks open the project, then turn on ${OPENAPI_SETTING_PATH}.`; - /** - * A dropdown shows only an error's message, so a raw PostgREST code leaves the user - * with nothing to act on. Mutates rather than re-wraps, because only a NodeApiError - * carries its description as far as the dropdown. + * A dropdown shows only an error's message, so the hint `lakebaseApiRequest` already + * attached is not visible there. Mutates rather than re-wraps, because only a + * NodeApiError carries its description as far as the dropdown. */ export function explainLakebaseSchemaError(error: unknown, openApiMessage: string): never { if (error instanceof NodeApiError && errorBody(error)?.code === 'PGRST205') { error.message = openApiMessage; - error.description = OPENAPI_SETTING_HINT; throw error; } @@ -106,6 +103,7 @@ export async function lakebaseApiRequest( await sleep(SCHEMA_CACHE_RETRY_DELAY_MS, abortSignal); continue; } + makeLakebaseErrorLegible(error, options); throw error; } } diff --git a/packages/nodes-base/nodes/Databricks/transport/lakebaseErrors.ts b/packages/nodes-base/nodes/Databricks/transport/lakebaseErrors.ts new file mode 100644 index 00000000000..01f5d7fa19f --- /dev/null +++ b/packages/nodes-base/nodes/Databricks/transport/lakebaseErrors.ts @@ -0,0 +1,76 @@ +import { isRecord } from '@n8n/utils/is-record'; +import type { IHttpRequestOptions } from 'n8n-workflow'; +import { NodeApiError } from 'n8n-workflow'; + +import { makePermissionErrorLegible, sanitizeApiMessage } from '../actions/helpers'; +import { OPENAPI_DISABLED_NOTICE, OPENAPI_PATH } from '../actions/lakebase/openApiDocument'; + +/** + * Keyed on the code the Data API returns, never on its prose, because the wording + * changes between versions. A Map so a key like `constructor` cannot match by accident. + */ +export const LAKEBASE_ERROR_HINTS = new Map([ + [ + 'PGRST205', + 'Check the schema and table names. The table also has to be in a schema the Data API exposes.', + ], + [ + 'PGRST204', + 'A column in the mapping is not in the table. Reopen the node to refresh the column list.', + ], + [ + 'PGRST106', + 'The schema is not one the Data API exposes. Add it to the exposed schemas in Databricks.', + ], + [ + 'PGRST301', + "Reconnect the Databricks OAuth2 credential. If it is valid, the identity has no Postgres role on this branch: run `SELECT databricks_create_role('your-identity', 'SERVICE_PRINCIPAL')` in the Databricks SQL editor, using 'USER' for a user identity.", + ], + ['PGRST100', 'Check the filter column, operator and value.'], + [ + 'PGRST102', + 'The Data API could not read the request body. Check the column values being written.', + ], + ['PGRST103', 'The requested range of rows is not valid. Check the limit and offset.'], + [ + '42501', + 'The Postgres role is not granted to the Data API. Run `GRANT "your-role" TO authenticator` as the role\'s creator. A project owner cannot use the Data API at all, so use a non-owner identity.', + ], + ['23505', 'A row with the same key already exists.'], + ['23502', 'A column that cannot be null has no value.'], +]); + +/** + * Replaces the generic status message with what the Data API said, and puts the + * fix in the description. + * + * Mutates rather than re-wraps: passing a NodeApiError back through the + * constructor returns the same instance and discards the new options. + */ +export function makeLakebaseErrorLegible(error: unknown, options: IHttpRequestOptions): void { + if (!(error instanceof NodeApiError)) return; + + const body = error.context.data; + if (!isRecord(body)) return; + + // The workspace API fronts the same host and reports a different shape + if (typeof body.error_code === 'string') { + makePermissionErrorLegible(error); + return; + } + + const code = typeof body.code === 'string' ? body.code : undefined; + if (!code) return; + + const hint = + code === 'PGRST205' && OPENAPI_PATH.test(options.url) + ? OPENAPI_DISABLED_NOTICE + : LAKEBASE_ERROR_HINTS.get(code); + if (!hint) return; + + const apiMessage = typeof body.message === 'string' ? body.message : undefined; + if (apiMessage) error.message = sanitizeApiMessage(apiMessage); + + // A description equal to the message is dropped, which would lose the fix + if (hint !== error.message) error.description = hint; +}