From 652c36f34eb1b1c1ac21c432ca9599fb62a954e9 Mon Sep 17 00:00:00 2001 From: Aryaman Dhingra Date: Tue, 30 Jul 2024 16:22:55 -0400 Subject: [PATCH 1/3] feat: consolidate comments from nested levels of schemas DX-592 --- packages/openapi-generator/src/comments.ts | 67 +++ packages/openapi-generator/src/optimize.ts | 49 +-- .../openapi-generator/test/openapi.test.ts | 383 +++++++++++++++++- 3 files changed, 470 insertions(+), 29 deletions(-) diff --git a/packages/openapi-generator/src/comments.ts b/packages/openapi-generator/src/comments.ts index d247d1a5..54723da8 100644 --- a/packages/openapi-generator/src/comments.ts +++ b/packages/openapi-generator/src/comments.ts @@ -1,4 +1,5 @@ import { parse as parseComment, Block } from 'comment-parser'; +import { Schema } from './ir'; export function leadingComment( src: string, @@ -42,3 +43,69 @@ export function leadingComment( const parsedComment = parseComment(commentString); return parsedComment; } + +/** + * + * @param schema the schema to get all comments from + * @returns an array of all comments in the schema + */ +export function getAllSchemaComments(schema: Schema): Block[] { + const result = []; + + /** Push the first comment */ + if (schema.comment) { + result.push(schema.comment); + } + + /** Push the comments of the subschemas in CombinedTypes (union, intersection, etc) */ + if ('schemas' in schema) { + // combined type + for (const s of schema.schemas) { + result.push(...getAllSchemaComments(s)); + } + } + + return result; +} + +/** + * + * @param schema the schema to combine comments from + * @returns a combined comment from all comments in the schema + */ +export function combineComments(schema: Schema): Block | undefined { + const comments = getAllSchemaComments(schema); + + const tagSet = new Set(); + + // Empty comment block where we will build the result + const result: Block = { + tags: [], + description: '', + problems: [], + source: [], + }; + + if (comments.length === 0) return undefined; + + // Only use the first description if it exists, we don't wanna accidentally pull a description from a lower level schema + if (comments[0]?.description && comments[0].description !== '') { + result.description = comments[0].description; + } + + // Add all seen tags, problems, and source comments to the result + for (const comment of comments) { + for (const tag of comment.tags) { + // Only add the tag if we haven't seen it before. Otherwise, the higher level tag is 'probably' the more relevant tag. + if (!tagSet.has(tag.tag)) { + result.tags.push(tag); + tagSet.add(tag.tag); + } + } + + result.problems.push(...comment.problems); + result.source.push(...comment.source); + } + + return result; +} diff --git a/packages/openapi-generator/src/optimize.ts b/packages/openapi-generator/src/optimize.ts index ecc44273..88a37285 100644 --- a/packages/openapi-generator/src/optimize.ts +++ b/packages/openapi-generator/src/optimize.ts @@ -1,3 +1,4 @@ +import { combineComments } from './comments'; import { isPrimitive, type Primitive, type Schema } from './ir'; export type OptimizeFn = (schema: Schema) => Schema; @@ -173,6 +174,15 @@ export function filterUndefinedUnion(schema: Schema): [boolean, Schema] { } } +// This function is a helper that adds back any comments that were removed during optimization +function withComment(newSchema: Schema, oldSchema: Schema): Schema { + if (oldSchema.comment) { + newSchema.comment = combineComments(oldSchema); + } + + return newSchema; +} + export function optimize(schema: Schema): Schema { if (schema.type === 'object') { const properties: Record = {}; @@ -184,8 +194,8 @@ export function optimize(schema: Schema): Schema { } const [isOptional, filteredSchema] = filterUndefinedUnion(optimized); - if (prop.comment) { - filteredSchema.comment = prop.comment; + if (optimized.comment) { + filteredSchema.comment = optimized.comment; } properties[key] = filteredSchema; @@ -197,48 +207,31 @@ export function optimize(schema: Schema): Schema { const schemaObject: Schema = { type: 'object', properties, required }; - // only add comment field if there is a comment - if (schema.comment) { - return { ...schemaObject, comment: schema.comment }; - } - - return schemaObject; + return withComment(schemaObject, schema); } else if (schema.type === 'intersection') { const newSchema = foldIntersection(schema, optimize); - if (schema.comment) { - return { ...newSchema, comment: schema.comment }; - } - return newSchema; + + return withComment(newSchema, schema); } else if (schema.type === 'union') { const consolidated = consolidateUnion(schema); const simplified = simplifyUnion(consolidated, optimize); const merged = mergeUnions(simplified); - if (schema.comment) { - return { ...merged, comment: schema.comment }; - } - - return merged; + return withComment(merged, schema); } else if (schema.type === 'array') { const optimized = optimize(schema.items); - if (schema.comment) { - return { type: 'array', items: optimized, comment: schema.comment }; - } - return { type: 'array', items: optimized }; + + return withComment({ type: 'array', items: optimized }, schema); } else if (schema.type === 'record') { - return { + return withComment({ type: 'record', ...(schema.domain ? { domain: optimize(schema.domain) } : {}), codomain: optimize(schema.codomain), - ...(schema.comment ? { comment: schema.comment } : {}), - }; + }, schema) } else if (schema.type === 'tuple') { const schemas = schema.schemas.map(optimize); - return { type: 'tuple', schemas }; + return withComment({ type: 'tuple', schemas }, schema); } else if (schema.type === 'ref') { - if (schema.comment) { - return { ...schema, comment: schema.comment }; - } return schema; } else { return schema; diff --git a/packages/openapi-generator/test/openapi.test.ts b/packages/openapi-generator/test/openapi.test.ts index d65d3159..9705bb7a 100644 --- a/packages/openapi-generator/test/openapi.test.ts +++ b/packages/openapi-generator/test/openapi.test.ts @@ -55,6 +55,7 @@ async function testCase( schemas, ); + assert.deepEqual(errors, expectedErrors); assert.deepEqual(actual, expected); }); @@ -3935,6 +3936,385 @@ testCase("route with nested array examples", ROUTE_WITH_NESTED_ARRAY_EXAMPLES, { } }); +const ROUTE_WITH_OVERRIDING_COMMENTS = ` +import * as t from 'io-ts'; +import * as h from '@api-ts/io-ts-http'; + +/** + * @example "abc" + */ +const TargetSchema = t.string; + +const ParentSchema = t.type({ + /** This description should show with the example */ + target: h.optional(TargetSchema) +}) + +export const route = h.httpRoute({ + path: '/foo', + method: 'POST', + request: h.httpRequest({ + params: {}, + body: ParentSchema + }), + response: { + 200: t.literal('OK'), + }, +}); +` + +testCase("route with overriding comments", ROUTE_WITH_OVERRIDING_COMMENTS, { + openapi: "3.0.3", + info: { + title: "Test", + version: "1.0.0" + }, + paths: { + "/foo": { + post: { + parameters: [], + requestBody: { + content: { + "application/json": { + schema: { + type: "object", + properties: { + target: { + type: "string", + description: "This description should show with the example", + example: "abc" + } + } + } + } + } + }, + responses: { + 200: { + description: "OK", + content: { + "application/json": { + schema: { + type: "string", + enum: [ + "OK" + ] + } + } + } + } + } + } + } + }, + components: { + schemas: { + TargetSchema: { + title: "TargetSchema", + type: "string", + example: "abc" + }, + ParentSchema: { + title: "ParentSchema", + type: "object", + properties: { + target: { + type: "string", + description: "This description should show with the example", + example: "abc" + } + } + } + } + } +}); + +const ROUTE_WITH_NESTED_OVERRIDEN_COMMENTS = ` +import * as t from 'io-ts'; +import * as h from '@api-ts/io-ts-http'; + +/** + * @example "abc" + */ +const TargetSchema = t.string; + +const ParentSchema = t.type({ + /** This description should show with the example */ + target: h.optional(TargetSchema) +}) + +const GrandParentSchema = t.type({ + /** This description should override the previous description */ + parent: ParentSchema +}) + +export const route = h.httpRoute({ + path: '/foo', + method: 'POST', + request: h.httpRequest({ + params: {}, + body: GrandParentSchema + }), + response: { + 200: t.literal('OK'), + }, +}); +` + + +testCase("route with nested overriding comments", ROUTE_WITH_NESTED_OVERRIDEN_COMMENTS, { + openapi: "3.0.3", + info: { + title: "Test", + version: "1.0.0" + }, + paths: { + "/foo": { + post: { + parameters: [], + requestBody: { + content: { + "application/json": { + schema: { + type: "object", + properties: { + parent: { + allOf: [ + { + '$ref': '#/components/schemas/ParentSchema' + } + ], + description: 'This description should override the previous description', + }, + }, + required: ['parent'] + } + } + } + }, + responses: { + 200: { + description: "OK", + content: { + "application/json": { + schema: { + type: "string", + enum: [ + "OK" + ] + } + } + } + } + } + } + } + }, + components: { + schemas: { + TargetSchema: { + title: "TargetSchema", + type: "string", + example: "abc" + }, + ParentSchema: { + title: "ParentSchema", + type: "object", + properties: { + target: { + type: "string", + description: "This description should show with the example", + example: "abc" + } + } + }, + GrandParentSchema: { + title: "GrandParentSchema", + type: "object", + properties: { + parent: { + allOf: [ + { + '$ref': '#/components/schemas/ParentSchema' + } + ], + description: 'This description should override the previous description' + } + }, + required: ['parent'] + } + } + } +}); + +const ROUTE_WITH_OVERRIDEN_COMMENTS_IN_UNION = ` +import * as t from 'io-ts'; +import * as h from '@api-ts/io-ts-http'; + +/** + * @example "abc" + */ +const TargetSchema = t.string; + +/** + * @example "def" + */ +const TargetSchema2 = t.string; + +const ParentSchema = t.type({ + /** This description should show with the example */ + target: h.optional(t.union([TargetSchema, TargetSchema2])) +}) + +const SecondaryParentSchema = t.type({ + /** + * This description should show with the overriden example + * @example "overridden example" + */ + target: h.optional(t.union([TargetSchema, TargetSchema2])) +}) + +/** + * This is grandparent schema description + * @title Grand Parent Schema + */ +const GrandParentSchema = t.type({ + parent: ParentSchema, + secondaryParent: SecondaryParentSchema +}); + +export const route = h.httpRoute({ + path: '/foo', + method: 'POST', + request: h.httpRequest({ + params: {}, + body: GrandParentSchema + }), + response: { + 200: t.literal('OK'), + }, +}); +` + +testCase("route with overriden comments in union", ROUTE_WITH_OVERRIDEN_COMMENTS_IN_UNION, { + openapi: "3.0.3", + info: { + title: "Test", + version: "1.0.0" + }, + paths: { + "/foo": { + post: { + parameters: [], + requestBody: { + content: { + "application/json": { + schema: { + title: "Grand Parent Schema", + description: 'This is grandparent schema description', + type: "object", + properties: { + parent: { + "$ref": "#/components/schemas/ParentSchema" + }, + secondaryParent: { + "$ref": "#/components/schemas/SecondaryParentSchema" + } + }, + required: [ + "parent", + "secondaryParent" + ] + } + } + } + }, + responses: { + 200: { + description: "OK", + content: { + "application/json": { + schema: { + type: "string", + enum: [ + "OK" + ] + } + } + } + } + } + } + } + }, + components: { + schemas: { + TargetSchema: { + title: "TargetSchema", + type: "string", + example: "abc" + }, + TargetSchema2: { + title: "TargetSchema2", + type: "string", + example: "def" + }, + ParentSchema: { + title: "ParentSchema", + type: "object", + properties: { + target: { + oneOf: [ + { + "$ref": "#/components/schemas/TargetSchema" + }, + { + "$ref": "#/components/schemas/TargetSchema2" + } + ], + description: "This description should show with the example" + } + } + }, + SecondaryParentSchema: { + title: "SecondaryParentSchema", + type: "object", + properties: { + target: { + oneOf: [ + { + "$ref": "#/components/schemas/TargetSchema" + }, + { + "$ref": "#/components/schemas/TargetSchema2" + } + ], + description: "This description should show with the overriden example", + example: "\"overridden example\"" + } + } + }, + GrandParentSchema: { + title: "Grand Parent Schema", + description: 'This is grandparent schema description', + type: "object", + properties: { + parent: { + "$ref": "#/components/schemas/ParentSchema" + }, + secondaryParent: { + "$ref": "#/components/schemas/SecondaryParentSchema" + } + }, + required: [ + "parent", + "secondaryParent" + ] + } + } + } +}); + const ROUTE_WITH_PRIVATE_PROPERTIES = ` import * as t from 'io-ts'; import * as h from '@api-ts/io-ts-http'; @@ -4220,4 +4600,5 @@ testCase("route with record types", ROUTE_WITH_RECORD_TYPES, { } } } -}); \ No newline at end of file +}); + From 09f6bebc1589c2b5acb82abec59889821098be7e Mon Sep 17 00:00:00 2001 From: Aryaman Dhingra Date: Wed, 31 Jul 2024 10:52:55 -0400 Subject: [PATCH 2/3] feat: rework more functions to resuse withComment step DX-592 --- packages/openapi-generator/src/optimize.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/openapi-generator/src/optimize.ts b/packages/openapi-generator/src/optimize.ts index 88a37285..15849507 100644 --- a/packages/openapi-generator/src/optimize.ts +++ b/packages/openapi-generator/src/optimize.ts @@ -168,9 +168,9 @@ export function filterUndefinedUnion(schema: Schema): [boolean, Schema] { if (schemas.length === 0) { return [true, { type: 'undefined' }]; } else if (schemas.length === 1) { - return [true, schemas[0]!]; + return [true, withComment(schemas[0]!, schema)]; } else { - return [true, { type: 'union', schemas }]; + return [true, withComment({ type: 'union', schemas }, schema)]; } } @@ -193,11 +193,6 @@ export function optimize(schema: Schema): Schema { continue; } const [isOptional, filteredSchema] = filterUndefinedUnion(optimized); - - if (optimized.comment) { - filteredSchema.comment = optimized.comment; - } - properties[key] = filteredSchema; if (schema.required.indexOf(key) >= 0 && !isOptional) { From 8b220449fcf803bc381b322aaf139909caf3cb59 Mon Sep 17 00:00:00 2001 From: Aryaman Dhingra Date: Wed, 31 Jul 2024 10:53:16 -0400 Subject: [PATCH 3/3] chore: fix linter errors in tests DX-592 --- packages/openapi-generator/src/optimize.ts | 13 ++++++++----- packages/openapi-generator/test/openapi.test.ts | 6 +++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/openapi-generator/src/optimize.ts b/packages/openapi-generator/src/optimize.ts index 15849507..1b85ee9b 100644 --- a/packages/openapi-generator/src/optimize.ts +++ b/packages/openapi-generator/src/optimize.ts @@ -218,11 +218,14 @@ export function optimize(schema: Schema): Schema { return withComment({ type: 'array', items: optimized }, schema); } else if (schema.type === 'record') { - return withComment({ - type: 'record', - ...(schema.domain ? { domain: optimize(schema.domain) } : {}), - codomain: optimize(schema.codomain), - }, schema) + return withComment( + { + type: 'record', + ...(schema.domain ? { domain: optimize(schema.domain) } : {}), + codomain: optimize(schema.codomain), + }, + schema, + ); } else if (schema.type === 'tuple') { const schemas = schema.schemas.map(optimize); return withComment({ type: 'tuple', schemas }, schema); diff --git a/packages/openapi-generator/test/openapi.test.ts b/packages/openapi-generator/test/openapi.test.ts index 9705bb7a..4bac5c7b 100644 --- a/packages/openapi-generator/test/openapi.test.ts +++ b/packages/openapi-generator/test/openapi.test.ts @@ -3961,7 +3961,7 @@ export const route = h.httpRoute({ 200: t.literal('OK'), }, }); -` +`; testCase("route with overriding comments", ROUTE_WITH_OVERRIDING_COMMENTS, { openapi: "3.0.3", @@ -4059,7 +4059,7 @@ export const route = h.httpRoute({ 200: t.literal('OK'), }, }); -` +`; testCase("route with nested overriding comments", ROUTE_WITH_NESTED_OVERRIDEN_COMMENTS, { @@ -4194,7 +4194,7 @@ export const route = h.httpRoute({ 200: t.literal('OK'), }, }); -` +`; testCase("route with overriden comments in union", ROUTE_WITH_OVERRIDEN_COMMENTS_IN_UNION, { openapi: "3.0.3",