From 4d5b903a16582710a87b02738c06d4ad7cec13f8 Mon Sep 17 00:00:00 2001 From: David de Boer Date: Fri, 14 Aug 2026 12:08:15 +0200 Subject: [PATCH] fix(search-api-graphql): tell the caller which argument they got wrong - throw a GraphQLError with extensions.code BAD_USER_INPUT for the paging bounds and the IRI scalar, so the transport keeps the message instead of masking it as "Unexpected error." - describe every root-field argument in the SDL, stating the perPage bound where the playground shows it --- docs/reference/search-api-graphql.md | 33 ++++++++++ .../search-api-graphql/src/build-schema.ts | 61 ++++++++++++++++--- .../generator-stability.test.ts.snap | 19 +++++- .../test/build-schema.test.ts | 5 ++ .../search-api-graphql/test/handler.test.ts | 43 +++++++++++++ .../search-api-graphql/test/print-sdl.test.ts | 7 ++- 6 files changed, 155 insertions(+), 13 deletions(-) diff --git a/docs/reference/search-api-graphql.md b/docs/reference/search-api-graphql.md index 7c92f729..85790d9b 100644 --- a/docs/reference/search-api-graphql.md +++ b/docs/reference/search-api-graphql.md @@ -310,6 +310,39 @@ option (default 100); a request outside `1 ≤ perPage ≤ maxPerPage` or with are fetched (and `page` pins to 1), so a filter UI can refresh its facet counts without paying for a page of results. +Both bounds are stated in the arguments’ SDL descriptions, so the playground’s +own documentation answers “how large may a page be?” before a request has to +fail to say it. + +## Errors the caller can fix + +An invalid argument comes back as an ordinary GraphQL error carrying the +sentence that says what was wrong, plus the conventional code: + +```json +{ + "errors": [ + { + "message": "perPage must be between 0 and 100; got 150.", + "path": ["datasets"], + "extensions": { "code": "BAD_USER_INPUT" } + } + ] +} +``` + +The code is what lets a client tell “fix your query” from “retry later” without +matching on prose. Everything reported that way is **caller-fixable**: the +paging bounds above, and a value rejected by the [`IRI` +scalar](#finding-which-fields-accept-an-iri). + +Anything else is masked to `"Unexpected error."` – graphql-yoga’s default, and +the right one for a fault the consumer can do nothing about (an unreachable +engine, a bug here). Those are logged server-side with their stack; the caller +gets no detail, because there is no detail they could act on. So a +presentation-layer developer building against a hosted endpoint never has to +read the API container’s log to learn that they sent something invalid. + ## Guarding the contract Why the API, the index and a future REST surface cannot drift apart is the diff --git a/packages/search-api-graphql/src/build-schema.ts b/packages/search-api-graphql/src/build-schema.ts index 2221a034..b4f12322 100644 --- a/packages/search-api-graphql/src/build-schema.ts +++ b/packages/search-api-graphql/src/build-schema.ts @@ -153,7 +153,7 @@ const iriScalar = new GraphQLScalarType({ function assertIri(value: string): string { if (!isAbsoluteIri(value)) { - throw new GraphQLError( + throw userError( `IRI cannot represent “${value}”: an IRI needs a scheme (for example “https:”, “urn:” or “doi:”) and no whitespace. A value like this is usually a label or a token, which selects nothing on a field that keys on identity.`, ); } @@ -161,7 +161,8 @@ function assertIri(value: string): string { } /** Outbound the fault is the index, not the caller, so the message points at - * the fix rather than at the query. */ + * the fix rather than at the query – and it carries no `BAD_USER_INPUT`, + * since there is nothing the query could have done differently. */ function assertIriOut(value: string): string { if (!isAbsoluteIri(value)) { throw new GraphQLError( @@ -171,6 +172,27 @@ function assertIriOut(value: string): string { return value; } +/** + * An error the CALLER can fix, marked as such: a `GraphQLError` carrying + * `extensions.code = 'BAD_USER_INPUT'`. + * + * Both halves matter, and both are about the consumer rather than about us. A + * plain `Error` thrown from a resolver is a server fault as far as the + * transport is concerned, so graphql-yoga masks it to `“Unexpected error.”`; + * the message then survives only in the API container’s log, where a + * presentation-layer developer building against a hosted endpoint cannot read + * it. Throwing a `GraphQLError` keeps the sentence that says what was wrong. + * The `code` says whose fault it is, so a client can distinguish “fix your + * query” from “retry later” without matching on prose – the convention every + * major GraphQL server shares. `“Unexpected error.”` is left for what the name + * says: faults we did not anticipate. + */ +function userError(message: string): GraphQLError { + return new GraphQLError(message, { + extensions: { code: 'BAD_USER_INPUT' }, + }); +} + /** SCREAMING_SNAKE_CASE for an enum value name, e.g. `datePosted` → `DATE_POSTED`. */ function screamingSnake(name: string): string { return name.replace(/([a-z0-9])([A-Z])/g, '$1_$2').toUpperCase(); @@ -737,11 +759,32 @@ export function buildGraphQLSchema( return { type: new GraphQLNonNull(resultType), args: { - query: { type: GraphQLString }, - where: { type: whereInput }, - orderBy: { type: orderByInput }, - page: { type: GraphQLInt, defaultValue: 1 }, - perPage: { type: GraphQLInt, defaultValue: 20 }, + query: { + type: GraphQLString, + description: 'Free-text query. Omit it to browse by filter alone.', + }, + where: { + type: whereInput, + description: 'Conditions every result must satisfy.', + }, + orderBy: { + type: orderByInput, + description: + 'Sort order. Defaults to relevance for a free-text query.', + }, + page: { + type: GraphQLInt, + defaultValue: 1, + // The bound in the SDL, so the playground’s own documentation + // answers “how large may a page be?” before a request has to fail + // to say it. + description: '1-based page number; at least 1.', + }, + perPage: { + type: GraphQLInt, + defaultValue: 20, + description: `Results per page, between 0 and ${maxPerPage}. Use 0 for a facet-only query: no results are fetched, and the facet counts still come back.`, + }, }, resolve: async (_source, args, context: SearchContext) => { const built = argsToQuery( @@ -871,11 +914,11 @@ function argsToQuery( const perPage = args.perPage ?? 20; const page = args.page ?? 1; if (page < 1) { - throw new Error(`page must be at least 1; got ${page}.`); + throw userError(`page must be at least 1; got ${page}.`); } // perPage: 0 is a legitimate facet-only query (no hits, page pins to 1). if (perPage < 0 || perPage > maxPerPage) { - throw new Error( + throw userError( `perPage must be between 0 and ${maxPerPage}; got ${perPage}.`, ); } diff --git a/packages/search-api-graphql/test/__snapshots__/generator-stability.test.ts.snap b/packages/search-api-graphql/test/__snapshots__/generator-stability.test.ts.snap index 9948ab4c..9b794753 100644 --- a/packages/search-api-graphql/test/__snapshots__/generator-stability.test.ts.snap +++ b/packages/search-api-graphql/test/__snapshots__/generator-stability.test.ts.snap @@ -2,7 +2,24 @@ exports[`GraphQL generator stability > emits a stable SDL for a representative schema 1`] = ` "type Query { - things(query: String, where: ThingWhere, orderBy: ThingOrderBy, page: Int = 1, perPage: Int = 20): ThingSearchResult! + things( + """Free-text query. Omit it to browse by filter alone.""" + query: String + + """Conditions every result must satisfy.""" + where: ThingWhere + + """Sort order. Defaults to relevance for a free-text query.""" + orderBy: ThingOrderBy + + """1-based page number; at least 1.""" + page: Int = 1 + + """ + Results per page, between 0 and 100. Use 0 for a facet-only query: no results are fetched, and the facet counts still come back. + """ + perPage: Int = 20 + ): ThingSearchResult! } type ThingSearchResult { diff --git a/packages/search-api-graphql/test/build-schema.test.ts b/packages/search-api-graphql/test/build-schema.test.ts index 7ce6f260..59c8d921 100644 --- a/packages/search-api-graphql/test/build-schema.test.ts +++ b/packages/search-api-graphql/test/build-schema.test.ts @@ -225,6 +225,11 @@ describe('buildGraphQLSchema', () => { expect(badPerPage.errors?.[0]?.message).toMatch( /perPage must be between 0 and 100/, ); + // Marked caller-fixable, so a transport keeps the message rather than + // masking it as a server fault (see handler.test.ts). + for (const error of [badPage.errors?.[0], badPerPage.errors?.[0]]) { + expect(error?.extensions.code).toBe('BAD_USER_INPUT'); + } }); it('orders the output list best-first for the requested language', async () => { diff --git a/packages/search-api-graphql/test/handler.test.ts b/packages/search-api-graphql/test/handler.test.ts index 7c07bad3..3da75326 100644 --- a/packages/search-api-graphql/test/handler.test.ts +++ b/packages/search-api-graphql/test/handler.test.ts @@ -103,6 +103,49 @@ describe('createSearchGraphQLHandler', () => { expect(received().text).toBe('kaart'); }); + // The regression the unit tests cannot catch: argument validation runs inside + // a resolver, and the transport masks a resolver throw as “Unexpected error.” + // unless it is a GraphQLError. Asserted through the handler, since masking is + // exactly what the direct-execution tests bypass. + it('reports an invalid argument to the caller instead of masking it', async () => { + const { engine } = fakeEngine(); + const handler = createSearchGraphQLHandler({ + searchSchema: searchSchema(schema), + engine, + }); + + const response = await post( + handler, + '{ datasets(perPage: 150) { pagination { total } } }', + ); + const { errors } = await response.json(); + + expect(errors[0].message).toBe( + 'perPage must be between 0 and 100; got 150.', + ); + expect(errors[0].extensions.code).toBe('BAD_USER_INPUT'); + }); + + it('masks an engine failure, which the caller cannot fix', async () => { + const { engine } = fakeEngine(); + const handler = createSearchGraphQLHandler({ + searchSchema: searchSchema(schema), + engine: { + ...engine, + search: () => Promise.reject(new Error('connect ECONNREFUSED')), + }, + }); + + const response = await post( + handler, + '{ datasets { pagination { total } } }', + ); + const { errors } = await response.json(); + + expect(errors[0].message).toBe('Unexpected error.'); + expect(errors[0].message).not.toContain('ECONNREFUSED'); + }); + it('orders output languages by the Accept-Language header', async () => { const { engine, received } = fakeEngine(); const handler = createSearchGraphQLHandler({ diff --git a/packages/search-api-graphql/test/print-sdl.test.ts b/packages/search-api-graphql/test/print-sdl.test.ts index 37e5c572..8f373eb3 100644 --- a/packages/search-api-graphql/test/print-sdl.test.ts +++ b/packages/search-api-graphql/test/print-sdl.test.ts @@ -97,9 +97,10 @@ describe('printSchemaModuleSdl', () => { modulePath: fixture('no-options.mjs'), }); - // Prettier breaks the multi-argument root field over several lines, so a - // surface move stays one added line in the committed diff. - expect(unformatted).toContain(' datasets(query: String'); + // Prettier expands every one-line description into a block, so a wording + // change stays one changed line in the committed diff. + expect(unformatted).toContain('"""1-based page number; at least 1."""'); + expect(formatted).toContain(' 1-based page number; at least 1.\n'); expect(formatted).not.toBe(unformatted); });