diff --git a/.changeset/vast-kids-add.md b/.changeset/vast-kids-add.md new file mode 100644 index 0000000000..0dddd5229e --- /dev/null +++ b/.changeset/vast-kids-add.md @@ -0,0 +1,8 @@ +--- +'@redocly/cli': minor +'@redocly/openapi-core': minor +--- + +Added a `strategy` option to the `component-name-unique` rule, matching the `--component-names-strategy` option of the `bundle` command. +Set it to `title` to check the component names that bundling derives from each schema's `title`. +With `strategy: title`, the rule also reports referenced schemas that have no `title`, because `bundle` can't name those and fails. diff --git a/docs/@v2/commands/bundle.md b/docs/@v2/commands/bundle.md index cf19cf8246..fb5c68424d 100644 --- a/docs/@v2/commands/bundle.md +++ b/docs/@v2/commands/bundle.md @@ -205,3 +205,6 @@ All other characters, including non-ASCII letters such as `é` or `я`, are repl Schemas without `title` can't be named using the `--component-names-strategy=title` strategy. The bundling process reports an error for such schemas. {% /admonition %} + +To catch name collisions before bundling, set the matching `strategy` option on the +[`component-name-unique`](../rules/oas/component-name-unique.md) rule. diff --git a/docs/@v2/rules/oas/component-name-unique.md b/docs/@v2/rules/oas/component-name-unique.md index b0bdfac425..b30ef45b26 100644 --- a/docs/@v2/rules/oas/component-name-unique.md +++ b/docs/@v2/rules/oas/component-name-unique.md @@ -38,6 +38,7 @@ This clearly is not optimal. Having unique component names prevents these proble | parameters | string | Possible values: `off`, `warn`, `error`. Default: not set. | | responses | string | Possible values: `off`, `warn`, `error`. Default: not set. | | requestBodies | string | Possible values: `off`, `warn`, `error`. Default: not set. | +| strategy | string | Possible values: `basename`, `title`. Default: `basename`. | An example configuration: @@ -48,8 +49,26 @@ rules: parameters: off responses: warn requestBodies: warn + strategy: basename ``` +### Component names strategy + +The rule predicts the component names that `bundle` produces, so `strategy` must match the +[`--component-names-strategy`](../../commands/bundle.md#configure-the-component-names-strategy) option you bundle with. + +With the default `basename`, a schema pulled in from another file is named after the `$ref` fragment or the file name. +Two files both called `Order.yaml` therefore collide, and the rule reports them. + +With `title`, the same schemas are named after their `title` field instead. +Two files called `Order.yaml` with the titles `Order model` and `Order request` become `OrderModel` and `OrderRequest`, so the rule no longer reports them. +Two schemas in differently named files that share a title do collide, and the rule reports those instead. + +The `title` strategy applies to every schema that `bundle` renames, which is every schema reached by a `$ref` that crosses a file boundary. +A referenced schema that has no `title` can't be named under this strategy, and `bundle` fails without producing a file. +The rule reports those schemas so you find them before bundling. +For the uniqueness check itself, such a schema still falls back to its file name, so a name collision is reported as well. + ## Examples Given this configuration: diff --git a/packages/core/src/bundle/bundle-visitor.ts b/packages/core/src/bundle/bundle-visitor.ts index 1753b30c25..10e42535f0 100644 --- a/packages/core/src/bundle/bundle-visitor.ts +++ b/packages/core/src/bundle/bundle-visitor.ts @@ -1,5 +1,5 @@ import { type RuleSeverity } from '../config/types.js'; -import { COMPONENT_NAME_CHARS, type SpecMajorVersion } from '../oas-types.js'; +import { type SpecMajorVersion } from '../oas-types.js'; import { isAbsoluteUrl, replaceRef, @@ -14,11 +14,9 @@ import { import { type ResolvedRefMap, type Document } from '../resolve.js'; import { reportUnresolvedRef } from '../rules/common/no-unresolved-refs.js'; import { type OasRef, type Oas3Discriminator, type Oas3Example } from '../typings/openapi.js'; +import { componentNameFromTitle } from '../utils/component-name-from-title.js'; import { dequal } from '../utils/dequal.js'; -import { isPlainObject } from '../utils/is-plain-object.js'; -import { isString } from '../utils/is-string.js'; import { makeRefId } from '../utils/make-ref-id.js'; -import { toPascalCase } from '../utils/to-pascal-case.js'; import { type Oas3Visitor, type Oas2Visitor } from '../visitors.js'; import { type UserContext, type ResolveResult, type NonUndefined, type Problem } from '../walk.js'; import { type ComponentNamesStrategy } from './bundle-document.js'; @@ -320,14 +318,12 @@ export function makeBundleVisitor({ return dequal(node, target.node); } - function componentNameFromTitle( + function resolveComponentNameFromTitle( target: ComponentTarget, componentsGroup: ComponentsGroup, ctx: UserContext ): { key: string; problem?: Problem } { - const { node } = target; - const title = isPlainObject(node) && isString(node.title) ? node.title.trim() : ''; - const key = toPascalCase(title).replace(new RegExp(`[^${COMPONENT_NAME_CHARS}]`, 'g'), '-'); + const { title, name: key } = componentNameFromTitle(target.node); const titleLocation = target.location.child('title'); if (title === '') { @@ -379,7 +375,7 @@ export function makeBundleVisitor({ const componentsGroup = components[componentType]; if (componentNamesStrategy === 'title' && componentType === schemaComponentType) { - const { key, problem } = componentNameFromTitle(target, componentsGroup, ctx); + const { key, problem } = resolveComponentNameFromTitle(target, componentsGroup, ctx); if (!problem) { firstSchemaLocationByName.set(key, target.location.child('title')); return key; diff --git a/packages/core/src/rules/oas3/__tests__/component-name-unique.test.ts b/packages/core/src/rules/oas3/__tests__/component-name-unique.test.ts index b387202fbf..4f055f783b 100644 --- a/packages/core/src/rules/oas3/__tests__/component-name-unique.test.ts +++ b/packages/core/src/rules/oas3/__tests__/component-name-unique.test.ts @@ -986,4 +986,413 @@ describe('Oas3 component-name-unique', () => { `); }); }); + + describe('strategy: title', () => { + it('should not report on same filenames with different titles', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + components: + schemas: + Test: + type: object + properties: + model: + $ref: '/a/Order.yaml' + request: + $ref: '/b/Order.yaml' + `, + '/foobar.yaml' + ); + const additionalDocuments = [ + { + absoluteRef: '/a/Order.yaml', + body: outdent` + title: Order model + type: object + `, + }, + { + absoluteRef: '/b/Order.yaml', + body: outdent` + title: Order request + type: object + `, + }, + ]; + + const results = await lintDocumentForTest( + { 'component-name-unique': { severity: 'error', strategy: 'title' } }, + document, + additionalDocuments + ); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report on different filenames with the same title', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + components: + schemas: + Test: + type: object + properties: + user: + $ref: '/a/User.yaml' + account: + $ref: '/b/Account.yaml' + `, + '/foobar.yaml' + ); + const additionalDocuments = [ + { + absoluteRef: '/a/User.yaml', + body: outdent` + title: User account + type: object + `, + }, + { + absoluteRef: '/b/Account.yaml', + body: outdent` + title: User account + type: object + `, + }, + ]; + + const results = await lintDocumentForTest( + { 'component-name-unique': { severity: 'error', strategy: 'title' } }, + document, + additionalDocuments + ); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/title", + "reportOnKey": false, + "source": "/a/User.yaml", + }, + ], + "message": "Component 'schemas/UserAccount' is not unique. It is also defined at: + - /b/Account.yaml", + "reference": "https://redocly.com/docs/cli/rules/oas/component-name-unique", + "ruleId": "component-name-unique", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/title", + "reportOnKey": false, + "source": "/b/Account.yaml", + }, + ], + "message": "Component 'schemas/UserAccount' is not unique. It is also defined at: + - /a/User.yaml", + "reference": "https://redocly.com/docs/cli/rules/oas/component-name-unique", + "ruleId": "component-name-unique", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report a root schema whose title matches its own component name', async () => { + const rootBody = outdent` + openapi: 3.0.0 + paths: + /things: + get: + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '/Other.yaml' + components: + schemas: + BarThing: + title: Bar thing + type: object + `; + const document = parseYamlToDocument(rootBody, '/foobar.yaml'); + const additionalDocuments = [ + { absoluteRef: '/foobar.yaml', body: rootBody }, + { + absoluteRef: '/Other.yaml', + body: outdent` + title: Other thing + type: object + properties: + inner: + $ref: '/foobar.yaml#/components/schemas/BarThing' + `, + }, + ]; + + const results = await lintDocumentForTest( + { 'component-name-unique': { severity: 'error', strategy: 'title' } }, + document, + additionalDocuments + ); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report a root schema that another file refers to by title', async () => { + const rootBody = outdent` + openapi: 3.0.0 + paths: + /things: + get: + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '/Other.yaml' + components: + schemas: + Foo: + title: Bar thing + type: object + `; + const document = parseYamlToDocument(rootBody, '/foobar.yaml'); + const additionalDocuments = [ + { absoluteRef: '/foobar.yaml', body: rootBody }, + { + absoluteRef: '/Other.yaml', + body: outdent` + title: Bar thing + type: object + properties: + inner: + $ref: '/foobar.yaml#/components/schemas/Foo' + `, + }, + ]; + + const results = await lintDocumentForTest( + { 'component-name-unique': { severity: 'error', strategy: 'title' } }, + document, + additionalDocuments + ); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Foo/title", + "reportOnKey": false, + "source": "/foobar.yaml", + }, + ], + "message": "Component 'schemas/BarThing' is not unique. It is also defined at: + - /Other.yaml", + "reference": "https://redocly.com/docs/cli/rules/oas/component-name-unique", + "ruleId": "component-name-unique", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/title", + "reportOnKey": false, + "source": "/Other.yaml", + }, + ], + "message": "Component 'schemas/BarThing' is not unique. It is also defined at: + - /foobar.yaml#/components/schemas/Foo", + "reference": "https://redocly.com/docs/cli/rules/oas/component-name-unique", + "ruleId": "component-name-unique", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report schemas that only the root document refers to', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + paths: + /orders: + get: + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '#/components/schemas/Order' + components: + schemas: + Order: + title: Order model + type: object + OrderModel: + title: Something else + type: object + `, + '/foobar.yaml' + ); + + const results = await lintDocumentForTest( + { 'component-name-unique': { severity: 'error', strategy: 'title' } }, + document, + [] + ); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report a referenced schema without a title', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + paths: + /carts: + get: + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '/Cart.yaml' + `, + '/foobar.yaml' + ); + const additionalDocuments = [ + { + absoluteRef: '/Cart.yaml', + body: outdent` + type: object + properties: + total: + type: number + `, + }, + ]; + + const results = await lintDocumentForTest( + { 'component-name-unique': { severity: 'error', strategy: 'title' } }, + document, + additionalDocuments + ); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/", + "reportOnKey": false, + "source": "/Cart.yaml", + }, + ], + "message": "Schema must define a \`title\` when using \`strategy: title\`. Bundling fails without it.", + "reference": "https://redocly.com/docs/cli/rules/oas/component-name-unique", + "ruleId": "component-name-unique", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should fall back to the filename when a schema has no title', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + components: + schemas: + Order: + type: object + Test: + type: object + properties: + order: + $ref: '/a/Order.yaml' + `, + '/foobar.yaml' + ); + const additionalDocuments = [ + { + absoluteRef: '/a/Order.yaml', + body: outdent` + type: object + `, + }, + ]; + + const results = await lintDocumentForTest( + { 'component-name-unique': { severity: 'error', strategy: 'title' } }, + document, + additionalDocuments + ); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Order", + "reportOnKey": false, + "source": "/foobar.yaml", + }, + ], + "message": "Component 'schemas/Order' is not unique. It is also defined at: + - /a/Order.yaml", + "reference": "https://redocly.com/docs/cli/rules/oas/component-name-unique", + "ruleId": "component-name-unique", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/", + "reportOnKey": false, + "source": "/a/Order.yaml", + }, + ], + "message": "Component 'schemas/Order' is not unique. It is also defined at: + - /foobar.yaml#/components/schemas/Order", + "reference": "https://redocly.com/docs/cli/rules/oas/component-name-unique", + "ruleId": "component-name-unique", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/", + "reportOnKey": false, + "source": "/a/Order.yaml", + }, + ], + "message": "Schema must define a \`title\` when using \`strategy: title\`. Bundling fails without it.", + "reference": "https://redocly.com/docs/cli/rules/oas/component-name-unique", + "ruleId": "component-name-unique", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + }); }); diff --git a/packages/core/src/rules/oas3/component-name-unique.ts b/packages/core/src/rules/oas3/component-name-unique.ts index e0be1241c0..95c4824853 100644 --- a/packages/core/src/rules/oas3/component-name-unique.ts +++ b/packages/core/src/rules/oas3/component-name-unique.ts @@ -10,6 +10,7 @@ import type { Oas3_1Schema, OasRef, } from '../../typings/openapi.js'; +import { componentNameFromTitle } from '../../utils/component-name-from-title.js'; import { isSupportedExtension } from '../../utils/is-supported-extension.js'; import type { Oas2Rule, Oas3Rule, Oas3Visitor } from '../../visitors.js'; import type { Problem, UserContext } from '../../walk.js'; @@ -28,10 +29,13 @@ const TYPE_NAME_TO_OPTION_COMPONENT_NAME: { [key: string]: string } = { [TYPE_NAME_REQUEST_BODY]: 'requestBodies', }; -type ComponentsMapValue = { absolutePointers: Set; locations: Location[] }; +type ComponentsMapValue = Map; export const ComponentNameUnique: Oas3Rule | Oas2Rule = (options) => { const components = new Map(); + const schemasWithoutTitle = new Map(); + const useTitleStrategy = options.strategy === 'title'; + let rootSourceRef: string; const typeNames: string[] = []; if (options.schemas !== 'off') { @@ -49,26 +53,46 @@ export const ComponentNameUnique: Oas3Rule | Oas2Rule = (options) => { const rule: Oas3Visitor = { ref: { - leave(ref: OasRef, { type, resolve }: UserContext) { + leave(ref: OasRef, { type, resolve, location }: UserContext) { const typeName = type.name; if (typeNames.includes(typeName)) { const resolvedRef = resolve(ref); if (!resolvedRef.location) return; + if (usesTitleStrategy(typeName, location, resolvedRef.location)) { + const { title, name } = componentNameFromTitle(resolvedRef.node); + if (title) { + addFoundComponent( + typeName, + name, + resolvedRef.location, + resolvedRef.location.child('title') + ); + return; + } + schemasWithoutTitle.set( + resolvedRef.location.absolutePointer.toString(), + resolvedRef.location + ); + } + addComponentFromAbsoluteLocation(typeName, resolvedRef.location); } }, }, Root: { + enter(_: AnyOas3Definition, { location }: UserContext) { + rootSourceRef = location.source.absoluteRef; + }, leave(root: AnyOas3Definition, ctx: UserContext) { - components.forEach((value, key, _) => { - if (value.absolutePointers.size > 1) { + components.forEach((entry, key, _) => { + if (entry.size > 1) { const component = getComponentFromKey(key); const optionComponentName = getOptionComponentNameForTypeName(component.typeName); const componentSeverity = optionComponentName ? options[optionComponentName] : null; - for (const location of value.locations) { - const definitions = Array.from(value.absolutePointers) - .filter((v) => v !== location.absolutePointer.toString()) + for (const [absolutePointer, location] of entry) { + const definitions = Array.from(entry.keys()) + .filter((v) => v !== absolutePointer) .map((v) => `- ${v}`) .join('\n'); const problem: Problem = { @@ -83,6 +107,19 @@ export const ComponentNameUnique: Oas3Rule | Oas2Rule = (options) => { } } }); + + for (const location of schemasWithoutTitle.values()) { + const problem: Problem = { + message: + 'Schema must define a `title` when using `strategy: title`. Bundling fails without it.', + location, + reference: 'https://redocly.com/docs/cli/rules/oas/component-name-unique', + }; + if (options.schemas) { + problem.forceSeverity = options.schemas; + } + ctx.report(problem); + } }, }, }; @@ -129,16 +166,17 @@ export const ComponentNameUnique: Oas3Rule | Oas2Rule = (options) => { return componentName; } - function addFoundComponent(typeName: string, componentName: string, location: Location): void { + function addFoundComponent( + typeName: string, + componentName: string, + location: Location, + reportLocation: Location = location + ): void { const key = getKeyForComponent(typeName, componentName); - const entry: ComponentsMapValue = components.get(key) ?? { - absolutePointers: new Set(), - locations: [], - }; + const entry: ComponentsMapValue = components.get(key) ?? new Map(); const absoluteLocation = location.absolutePointer.toString(); - if (!entry.absolutePointers.has(absoluteLocation)) { - entry.absolutePointers.add(absoluteLocation); - entry.locations.push(location); + if (!entry.has(absoluteLocation)) { + entry.set(absoluteLocation, reportLocation); } components.set(key, entry); } @@ -147,6 +185,19 @@ export const ComponentNameUnique: Oas3Rule | Oas2Rule = (options) => { const componentName = getComponentNameFromAbsoluteLocation(location.absolutePointer.toString()); addFoundComponent(typeName, componentName, location); } + + function usesTitleStrategy( + typeName: string, + refLocation: Location, + targetLocation: Location + ): boolean { + return ( + useTitleStrategy && + typeName === TYPE_NAME_SCHEMA && + (refLocation.source.absoluteRef !== rootSourceRef || + targetLocation.source.absoluteRef !== rootSourceRef) + ); + } }; function getOptionComponentNameForTypeName(typeName: string): string | null { diff --git a/packages/core/src/utils/component-name-from-title.ts b/packages/core/src/utils/component-name-from-title.ts new file mode 100644 index 0000000000..2b83018451 --- /dev/null +++ b/packages/core/src/utils/component-name-from-title.ts @@ -0,0 +1,10 @@ +import { COMPONENT_NAME_CHARS } from '../oas-types.js'; +import { isPlainObject } from './is-plain-object.js'; +import { isString } from './is-string.js'; +import { toPascalCase } from './to-pascal-case.js'; + +export function componentNameFromTitle(node: unknown): { title: string; name: string } { + const title = isPlainObject(node) && isString(node.title) ? node.title.trim() : ''; + const name = toPascalCase(title).replace(new RegExp(`[^${COMPONENT_NAME_CHARS}]`, 'g'), '-'); + return { title, name }; +}