diff --git a/apps/content/docs/openapi/routing.mdx b/apps/content/docs/openapi/routing.mdx index a6ca1ec68..7b5c1fe77 100644 --- a/apps/content/docs/openapi/routing.mdx +++ b/apps/content/docs/openapi/routing.mdx @@ -27,7 +27,7 @@ In this example, `list` is exposed as `GET /planets` because it overrides the de ## Path Parameters -To define a path parameter, use `{name}` in the `path` and add the same field as a required key in the input schema: +To define a path parameter, use `{name}` in the `path` and add a field with the same name to the input schema: ```ts import { z } from 'zod' @@ -75,7 +75,7 @@ In this example, `listPlanets` is exposed as `GET /api/v2/planets/`. `createPlan ### Path Parameters in Prefixes -Prefixes can also include path parameters, but they must be defined as required fields in the input schema. +Prefixes can also include path parameters, but they must be defined as fields in the input schema. ```ts const base = os diff --git a/packages/openapi/src/openapi-generator-operation.test.ts b/packages/openapi/src/openapi-generator-operation.test.ts index ea18cd68b..075066601 100644 --- a/packages/openapi/src/openapi-generator-operation.test.ts +++ b/packages/openapi/src/openapi-generator-operation.test.ts @@ -112,6 +112,57 @@ describe('openAPIGenerator operation builders', () => { }) }) + it('renders a compact path param as required even when the schema marks it optional', () => { + const { ctx, operation } = createContext() + const path = '/planets/{id}' as const + + buildRequest(ctx, operation, testDef({ + inputs: [testSchema({ + type: 'object', + properties: { + id: { type: 'string' }, + name: { type: 'string' }, + }, + required: ['name'], + })], + }), { method: 'POST', path }, getDynamicPathParams(path)) + + expect(operation.parameters).toEqual([ + { in: 'path', required: true, name: 'id', schema: { type: 'string' } }, + ]) + expect(operation.requestBody).toEqual({ + required: true, + content: { + 'application/json': { + schema: { + type: 'object', + properties: { name: { type: 'string' } }, + required: ['name'], + }, + }, + }, + }) + }) + + it('renders a detailed path param as required even when the schema marks it optional', () => { + const { ctx, operation } = createContext() + const path = '/planets/{id}' as const + + buildRequest(ctx, operation, testDef({ + inputs: [testSchema({ + type: 'object', + properties: { + params: { type: 'object', properties: { id: { type: 'string' } } }, + }, + })], + }), { inputStructure: 'detailed', path }, getDynamicPathParams(path)) + + expect(operation.parameters).toEqual([ + { in: 'path', required: true, name: 'id', schema: { type: 'string' } }, + ]) + expect(operation.requestBody).toBeUndefined() + }) + it('maps compact path params and keeps the remaining fields in the body', () => { const { ctx, operation } = createContext() const path = '/planets/{id}' as const @@ -426,13 +477,6 @@ describe('openAPIGenerator operation builders', () => { path: '/planets/{id}' as const, message: 'Schema keys: (none)', }, - { - name: 'a dynamic param is optional in the input schema', - inputs: [testSchema({ type: 'object', properties: { id: { type: 'string' } } })], - meta: { path: '/planets/{id}' as const }, - path: '/planets/{id}' as const, - message: 'dynamic param "id" is optional in the input schema', - }, ])('throws when $name', ({ inputs, meta, path, message }) => { const { ctx, operation } = createContext() diff --git a/packages/openapi/src/openapi-generator-operation.ts b/packages/openapi/src/openapi-generator-operation.ts index 3a3fd631a..08ce20322 100644 --- a/packages/openapi/src/openapi-generator-operation.ts +++ b/packages/openapi/src/openapi-generator-operation.ts @@ -221,13 +221,7 @@ function renderPathParameters( ) } - if (entry[2]) { - throw new OpenAPIGeneratorError( - `dynamic param "${name}" is optional in the input schema.\n` - + ` OpenAPI requires path params to be required. Make "${name}" required.`, - ) - } - + // Always required, even when the schema marks it optional: the route only matches when the segment is present. const style = paramsStyles?.[name] const parameter: Exclude[number] = { in: 'path', diff --git a/packages/openapi/tests/openapi-generator/crud.test.ts b/packages/openapi/tests/openapi-generator/crud.test.ts index d9e502e26..aaf9cb562 100644 --- a/packages/openapi/tests/openapi-generator/crud.test.ts +++ b/packages/openapi/tests/openapi-generator/crud.test.ts @@ -226,12 +226,33 @@ describe('openAPIGenerator e2e: crud api', () => { })) }) + it('documents an optional path param as required since the route only matches when it is present', async () => { + const doc = await generator.generate({ + find: oc + .meta(openapi({ method: 'GET', path: '/planets/{id}' })) + .input(z.object({ id: z.string().optional(), includeArchived: z.boolean().optional() })), + }) + + expect(doc.paths?.['/planets/{id}']?.get).toEqual(expect.objectContaining({ + parameters: [ + { name: 'id', in: 'path', required: true, schema: { type: 'string' } }, + { + name: 'includeArchived', + in: 'query', + allowEmptyValue: true, + allowReserved: true, + schema: { type: 'boolean' }, + }, + ], + })) + }) + it('rejects invalid contracts with one aggregated error listing every procedure', async () => { const error = await generator.generate({ - // path params must be required + // path params must exist in the input schema find: oc .meta(openapi({ method: 'GET', path: '/planets/{id}' })) - .input(z.object({ id: z.string().optional() })), + .input(z.object({ name: z.string() })), // GET inputs must be objects list: oc.meta(openapi({ method: 'GET' })).input(z.string()), }).then( @@ -240,7 +261,7 @@ describe('openAPIGenerator e2e: crud api', () => { ) expect(error.message).toContain('Procedure at find:') - expect(error.message).toContain('is optional in the input schema') + expect(error.message).toContain('is missing from the input schema') expect(error.message).toContain('Procedure at list:') expect(error.message).toContain('method is GET but the input schema is not an object') }) diff --git a/tests/openapi/reused-procedure-routes.test.ts b/tests/openapi/reused-procedure-routes.test.ts new file mode 100644 index 000000000..0f4ea70dc --- /dev/null +++ b/tests/openapi/reused-procedure-routes.test.ts @@ -0,0 +1,108 @@ +import type { RouterClient } from '@orpc/server' +import { createORPCClient } from '@orpc/client' +import { openapi, OpenAPIGenerator } from '@orpc/openapi' +import { OpenAPIHandler, OpenAPILink } from '@orpc/openapi/fetch' +import { os } from '@orpc/server' +import { z } from 'zod' + +/** + * One procedure exposed under several routes, where each route promotes a different optional + * field to a path param. Covers https://github.com/middleapi/orpc/issues/513. + */ +describe('one procedure reused under multiple routes with optional path params', () => { + const listComments = os + .meta(openapi({ method: 'GET', path: '/comments', tags: ['admin'] })) + .input(z.object({ + user: z.union([z.literal('me'), z.coerce.number()]).optional(), + tasting: z.coerce.number().optional(), + cursor: z.coerce.number().gte(1).default(1), + limit: z.coerce.number().gte(1).lte(100).default(100), + })) + .handler(async ({ input }) => input) + + const router = { + admin: { listComments }, + tastings: { + listComments: listComments.meta(openapi({ path: '/tastings/{tasting}/comments', tags: ['tastings'] })), + }, + users: { + listComments: listComments.meta(openapi({ path: '/users/{user}/comments', tags: ['users'] })), + }, + } + + describe('handler', () => { + const handler = new OpenAPIHandler(router) + + it.each([ + ['/comments?limit=5', { cursor: 1, limit: 5 }], + ['/comments?tasting=42&user=me', { tasting: 42, user: 'me', cursor: 1, limit: 100 }], + ['/tastings/42/comments', { tasting: 42, cursor: 1, limit: 100 }], + ['/users/me/comments?cursor=2', { user: 'me', cursor: 2, limit: 100 }], + ['/users/7/comments', { user: 7, cursor: 1, limit: 100 }], + ])('routes GET %s to the shared handler with %o', async (path, expected) => { + const { matched, response } = await handler.handle(new Request(`https://example.com${path}`)) + + expect(matched).toBe(true) + expect(response!.status).toBe(200) + await expect(response!.json()).resolves.toEqual(expected) + }) + }) + + describe('link', () => { + const handler = new OpenAPIHandler(router) + const requests: string[] = [] + + const link = new OpenAPILink(router, { + url: '/', + origin: 'https://example.com', + async fetch(url, init) { + const request = new Request(url, init) + const { pathname, search } = new URL(request.url) + requests.push(`${request.method} ${pathname}${search}`) + const { response } = await handler.handle(request) + return response ?? new Response('Not Found', { status: 404 }) + }, + }) + + const client = createORPCClient>(link) + + beforeEach(() => { + requests.length = 0 + }) + + it('sends every field as a query param on the route without path params', async () => { + await expect(client.admin.listComments({ tasting: 42, limit: 5 })).resolves.toEqual({ tasting: 42, cursor: 1, limit: 5 }) + expect(requests).toEqual(['GET /comments?tasting=42&limit=5']) + }) + + it('promotes the field to the path segment on the routes that declare it', async () => { + await expect(client.tastings.listComments({ tasting: 42 })).resolves.toEqual({ tasting: 42, cursor: 1, limit: 100 }) + await expect(client.users.listComments({ user: 'me', cursor: 2 })).resolves.toEqual({ user: 'me', cursor: 2, limit: 100 }) + expect(requests).toEqual(['GET /tastings/42/comments', 'GET /users/me/comments?cursor=2']) + }) + + it('rejects at runtime when the optional field backing a path param is omitted', async () => { + await expect(client.tastings.listComments({})).rejects.toThrow('Path param "tasting" cannot be empty') + expect(requests).toEqual([]) + }) + }) + + describe('generator', () => { + it('documents each route with its own path params and query params', async () => { + const doc = await new OpenAPIGenerator().generate(router) + + expect(Object.keys(doc.paths ?? {})).toEqual(['/comments', '/tastings/{tasting}/comments', '/users/{user}/comments']) + + const paramNames = (path: `/${string}`) => doc.paths![path]!.get!.parameters!.map((p: any) => `${p.in}:${p.name}${p.required ? '!' : ''}`) + + expect(paramNames('/comments')).toEqual(['query:user', 'query:tasting', 'query:cursor', 'query:limit']) + expect(paramNames('/tastings/{tasting}/comments')).toEqual(['path:tasting!', 'query:user', 'query:cursor', 'query:limit']) + expect(paramNames('/users/{user}/comments')).toEqual(['path:user!', 'query:tasting', 'query:cursor', 'query:limit']) + + expect(doc.paths!['/comments']!.get!.tags).toEqual(['admin']) + expect(doc.paths!['/tastings/{tasting}/comments']!.get!.tags).toEqual(['admin', 'tastings']) + expect(doc.paths!['/users/{user}/comments']!.get!.tags).toEqual(['admin', 'users']) + expect(doc.paths!['/tastings/{tasting}/comments']!.get!.operationId).toBe('tastings.listComments') + }) + }) +})