feat: handle inline schema in schemas plugin#3663
feat: handle inline schema in schemas plugin#3663malcolm-kee wants to merge 1 commit intohey-api:mainfrom
Conversation
|
|
|
|
@malcolm-kee is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
bot
added
the
size:XXL
|
Reviewed PR #3663. Submitted feedback on 4 issues: unconditional generation of parameter schemas for all operations (breaking change for existing users), Task list (5/5 completed)
 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3663 +/- ##
==========================================
- Coverage 38.99% 38.73% -0.26%
==========================================
Files 515 515
Lines 18901 19030 +129
Branches 5591 5636 +45
==========================================
+ Hits 7370 7372 +2
- Misses 9326 9417 +91
- Partials 2205 2241 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Medium urgency. The feature works for the targeted inline-parameter use case, but it also unconditionally generates parameter schemas for every operation in every spec (visible in the ~1400 new lines across the three default snapshots). This is a breaking change for existing users whose build pipelines, barrel exports, or bundle sizes depend on the current output. Two smaller correctness issues below.
| for (const path in context.spec.paths) { | ||
| if (path.startsWith('x-')) { | ||
| continue; | ||
| } | ||
|
|
||
| const pathItem = context.spec.paths[path as `/${string}`]!; | ||
|
|
||
| for (const method of httpMethods) { | ||
| const operation = pathItem[method as keyof typeof pathItem] as | ||
| | OpenAPIV2.OperationObject | ||
| | undefined; | ||
| if (!operation) { | ||
| continue; | ||
| } | ||
|
|
||
| const irOperation = context.ir.paths?.[path as `/${string}`]?.[method]; | ||
| if (!irOperation) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const { location, suffix } of paramLocations) { | ||
| const params = new Map<string, OpenAPIV2.ParameterObject>(); | ||
|
|
||
| for (const param of pathItem.parameters ?? []) { | ||
| if ('$ref' in param || param.in !== location) { | ||
| continue; | ||
| } | ||
| params.set(param.name, param as OpenAPIV2.ParameterObject); | ||
| } | ||
|
|
||
| for (const param of operation.parameters ?? []) { | ||
| if ('$ref' in param || param.in !== location) { | ||
| continue; | ||
| } | ||
| params.set(param.name, param as OpenAPIV2.ParameterObject); | ||
| } | ||
|
|
||
| if (params.size === 0) { | ||
| continue; | ||
| } | ||
|
|
||
| const properties: Record<string, OpenAPIV2.SchemaObject> = {}; | ||
| const required: Array<string> = []; | ||
| for (const [paramName, param] of params) { | ||
| if (!('type' in param)) { | ||
| continue; | ||
| } | ||
| const propSchema: Record<string, unknown> = {}; | ||
| for (const [key, value] of Object.entries(param)) { | ||
| if (key === 'allowEmptyValue' || key === 'in' || key === 'name' || key === 'required') { | ||
| continue; | ||
| } | ||
| propSchema[key] = value; | ||
| } | ||
| properties[paramName] = propSchema as OpenAPIV2.SchemaObject; | ||
| if (param.required) { | ||
| required.push(paramName); | ||
| } | ||
| } | ||
|
|
||
| const locationSchema = { | ||
| properties, | ||
| ...(required.length && { required }), | ||
| type: 'object', | ||
| } as OpenAPIV2.SchemaObject; | ||
|
|
||
| const name = `${toCase(irOperation.id, 'PascalCase')}${suffix}`; | ||
| const symbol = plugin.symbol(schemaName({ name, plugin, schema: locationSchema }), { | ||
| meta: { | ||
| category: 'schema', | ||
| resource: 'operation', | ||
| resourceId: irOperation.id, | ||
| tool: 'json-schema', | ||
| }, | ||
| }); | ||
| const obj = schemaToJsonSchemaDraft_04({ | ||
| context, | ||
| plugin, | ||
| schema: locationSchema, | ||
| }); | ||
| const statement = $.const(symbol) | ||
| .export() | ||
| .assign( | ||
| $( | ||
| $.fromValue(obj, { | ||
| layout: 'pretty', | ||
| }), | ||
| ).as('const'), | ||
| ); | ||
| plugin.node(statement); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop generates parameter schemas for all operations — not just specs whose parameters lack component definitions. The default test spec already defines its parameters inline (most specs do), so the 2.0.x default snapshot grows by ~390 lines, 3.0.x by ~630, and 3.1.x by ~700.
Every existing user of @hey-api/schemas will see new exports appear the next time they regenerate. Depending on how they consume the output (barrel re-exports, bundle analysis, snapshot tests), this could be disruptive.
Consider gating this behind an opt-in config option (e.g. inlineParameters: true) so the behavior is additive without surprising existing users. Alternatively, if the intent is for this to be on by default, the PR should be framed as a breaking change with a migration note.
| const propSchema: Record<string, unknown> = {}; | ||
| for (const [key, value] of Object.entries(param)) { | ||
| if (key === 'allowEmptyValue' || key === 'in' || key === 'name' || key === 'required') { | ||
| continue; | ||
| } | ||
| propSchema[key] = value; |
There was a problem hiding this comment.
This copies every property on the Swagger 2.0 ParameterObject except allowEmptyValue, in, name, and required — but ParameterObject has several fields that are not valid JSON Schema properties. Notably collectionFormat leaks through (visible in the CollectionFormatQuerySchema snapshot where each property includes e.g. collectionFormat: 'csv'). Other Swagger-only fields like allowEmptyValue are excluded, but collectionFormat is not.
You should either switch to an allowlist of known schema-relevant keys (type, format, items, default, enum, description, minimum, maximum, etc.) or add collectionFormat to the exclusion list.
| const propSchema: Record<string, unknown> = {}; | |
| for (const [key, value] of Object.entries(param)) { | |
| if (key === 'allowEmptyValue' || key === 'in' || key === 'name' || key === 'required') { | |
| continue; | |
| } | |
| propSchema[key] = value; | |
| if (!('type' in param)) { | |
| continue; | |
| } | |
| const propSchema: Record<string, unknown> = {}; | |
| for (const [key, value] of Object.entries(param)) { | |
| if (key === 'allowEmptyValue' || key === 'collectionFormat' || key === 'in' || key === 'name' || key === 'required') { | |
| continue; | |
| } | |
| propSchema[key] = value; | |
| } |
| if (propSchema) { | ||
| properties[paramName] = propSchema; | ||
| } | ||
| if (param.required) { | ||
| required.push(paramName); | ||
| } |
There was a problem hiding this comment.
required.push(paramName) runs even when propSchema is undefined (no schema or content on the parameter). This means a parameter with required: true but no type information ends up in the required array without a corresponding entry in properties — producing an invalid JSON Schema.
The same issue exists in the V3.1 block at line 700.
| if (propSchema) { | |
| properties[paramName] = propSchema; | |
| } | |
| if (param.required) { | |
| required.push(paramName); | |
| } | |
| if (propSchema) { | |
| properties[paramName] = propSchema; | |
| } | |
| if (propSchema && param.required) { | |
| required.push(paramName); | |
| } |
| for (const path in context.spec.paths) { | ||
| if (path.startsWith('x-')) { | ||
| continue; | ||
| } | ||
|
|
||
| const pathItem = context.spec.paths![path as `/${string}`]!; | ||
|
|
||
| for (const method of httpMethods) { | ||
| const operation = pathItem[method as keyof typeof pathItem] as | ||
| | OpenAPIV3_1.OperationObject | ||
| | undefined; | ||
| if (!operation) { | ||
| continue; | ||
| } | ||
|
|
||
| const irOperation = context.ir.paths?.[path as `/${string}`]?.[method]; | ||
| if (!irOperation) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const { location, suffix } of paramLocations) { | ||
| const params = new Map<string, OpenAPIV3_1.ParameterObject>(); | ||
|
|
||
| for (const param of pathItem.parameters ?? []) { | ||
| if ('$ref' in param || param.in !== location) { | ||
| continue; | ||
| } | ||
| params.set(param.name, param); | ||
| } | ||
|
|
||
| for (const param of operation.parameters ?? []) { | ||
| if ('$ref' in param || param.in !== location) { | ||
| continue; | ||
| } | ||
| params.set(param.name, param); | ||
| } | ||
|
|
||
| if (params.size === 0) { | ||
| continue; | ||
| } | ||
|
|
||
| const properties: Record<string, OpenAPIV3_1.SchemaObject> = {}; | ||
| const required: Array<string> = []; | ||
| for (const [paramName, param] of params) { | ||
| let propSchema: OpenAPIV3_1.SchemaObject | undefined; | ||
| if (param.schema) { | ||
| propSchema = { ...param.schema }; | ||
| if (param.description) { | ||
| propSchema.description = param.description; | ||
| } | ||
| } else if (param.content) { | ||
| const mediaType = Object.values(param.content)[0]; | ||
| if (mediaType?.schema) { | ||
| propSchema = { ...mediaType.schema }; | ||
| } | ||
| } | ||
| if (propSchema) { | ||
| properties[paramName] = propSchema; | ||
| } | ||
| if (param.required) { | ||
| required.push(paramName); | ||
| } | ||
| } | ||
|
|
||
| const locationSchema = { | ||
| properties, | ||
| ...(required.length && { required }), | ||
| type: 'object', | ||
| } as OpenAPIV3_1.SchemaObject; | ||
|
|
||
| const name = `${toCase(irOperation.id, 'PascalCase')}${suffix}`; | ||
| const symbol = plugin.symbol(schemaName({ name, plugin, schema: locationSchema }), { | ||
| meta: { | ||
| category: 'schema', | ||
| resource: 'operation', | ||
| resourceId: irOperation.id, | ||
| tool: 'json-schema', | ||
| }, | ||
| }); | ||
| const obj = schemaToJsonSchema2020_12({ | ||
| context, | ||
| plugin, | ||
| schema: locationSchema, | ||
| }); | ||
| const statement = $.const(symbol) | ||
| .export() | ||
| .assign( | ||
| $( | ||
| $.fromValue(obj, { | ||
| layout: 'pretty', | ||
| }), | ||
| ).as('const'), | ||
| ); | ||
| plugin.node(statement); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is ~95 lines that are structurally identical to the V3.0 block (lines 508–604), differing only in type annotations (OpenAPIV3_1.* vs OpenAPIV3.*) and the schema conversion function called (schemaToJsonSchema2020_12 vs schemaToJsonSchemaDraft_05). The V2 block adds a third copy with slightly different extraction logic.
As you noted in the PR description, this duplication is a concern. One concrete approach: extract a generic collectInlineParameterSchemas helper parameterized by spec types and the schema-conversion function, then call it from each version-specific function. This would also make it easier to add the collectionFormat fix and the required guard in a single place.
1 participant
This is second attempt to fix the issue
I previously just let coding agent do it without having time to validate it, so I scraped that.
Summary
#/components/schemas(ordefinitionsin Swagger 2.0). Parameters defined inline on path items or operations (common in FastAPI-generated specs) were silently ignored.GetCatsQuerySchemaorGetCatsByCatIdPathSchema.Caution
The parameter-to-schema conversion logic lives directly in the plugin. Each spec-version function has its own copy of the iteration and assembly logic.
There might be a better place for this parameter parsing. Open to feedback on whether this duplication is acceptable or should be refactored.
Naming convention
Each inline parameter group is exported as
{OperationId}{LocationSuffix}Schema, where:OperationIdcomes from the spec'soperationId(or is auto-generated from path + method by the existing IR layer, e.g.GET /cats/{cat_id}becomesGetCatsByCatId). The IR already deduplicates IDs by appending a counter on collision.LocationSuffixis one ofQuery,Headers,Path,Cookies.Examples:
GetCatsQuerySchema,GetCatsByCatIdHeadersSchema,GetCatsByCatIdPathSchema.How other plugins handle parameters
Other plugins don't face the same naming decision because they don't produce separate top-level exports per parameter location:
{OperationId}Datatype withquery,path,headers,bodyas nested propertiesCallWithParametersData { headers: { ... }; path: { ... }; query: { ... }; }Querystring,Params,Headers,Bodytype.prop('Querystring', ...)referencingData['query']path,query,headers,bodydirectly{location}_when names conflict across locationspath_foo,query_fooThe types plugin avoids the problem entirely by nesting everything under one name. Fastify and NestJS just reference those nested types. None of them emit standalone per-location exports.
Caution
Conflict risk: It's possible for a component schema name to collide with a generated parameter schema name -- e.g. a component named
GetCatsQuerywould produceGetCatsQuerySchema, same as the inline query params forGET /cats. This seems unlikely in practice but there's no deduplication handling for it currently.