diff --git a/.changeset/no-illogical-composition-keywords.md b/.changeset/no-illogical-composition-keywords.md new file mode 100644 index 0000000000..ce5593dcaa --- /dev/null +++ b/.changeset/no-illogical-composition-keywords.md @@ -0,0 +1,7 @@ +--- +'@redocly/cli': minor +--- + +Added the `no-illogical-composition-keywords` rule. + +**Note**: the rule is set to `warn` in the `recommended` ruleset and to `error` in `recommended-strict`, so existing API descriptions may report new problems. diff --git a/docs/@v2/rules/built-in-rules.md b/docs/@v2/rules/built-in-rules.md index 0578380a79..c7dce4d053 100644 --- a/docs/@v2/rules/built-in-rules.md +++ b/docs/@v2/rules/built-in-rules.md @@ -87,6 +87,7 @@ The rules list is split into sections. - [no-duplicated-enum-values](./common/no-duplicated-enum-values.md): All values in an `enum` must be unique - [no-enum-type-mismatch](./common/no-enum-type-mismatch.md): Enum options must match the data type declared in the schema - [no-example-value-and-externalValue](./oas/no-example-value-and-externalValue.md): Either the `value` or `externalValue` may be present, but not both +- [no-illogical-composition-keywords](./oas/no-illogical-composition-keywords.md): `oneOf`, `anyOf`, and `allOf` must combine schemas a value can actually match - [no-invalid-media-type-examples](./oas/no-invalid-media-type-examples.md): Example request bodies must match the declared schema - [no-mixed-number-range-constraints](./common/no-mixed-number-range-constraints.md): Ensures that schemas do not use both `maximum` and `exclusiveMaximum` (or both `minimum` and `exclusiveMinimum`) at the same time. - [no-invalid-schema-examples](./oas/no-invalid-schema-examples.md): Schema examples must match declared types diff --git a/docs/@v2/rules/configurable-rules.md b/docs/@v2/rules/configurable-rules.md index 133dc8f4b9..2e7e61dd2b 100644 --- a/docs/@v2/rules/configurable-rules.md +++ b/docs/@v2/rules/configurable-rules.md @@ -627,6 +627,19 @@ OpenAPI 3.0, 3.1, and 3.2 mostly share a type tree. Learn more about the [OpenAPI node types](https://redocly.com/docs/openapi-visual-reference/openapi-node-types/). +The `OneOf`, `AnyOf`, and `AllOf` node types each match the list of schemas under the keyword of the same name. +Use them to assert on one composition keyword without matching the other two. + +```yaml +rules: + rule/oneof-needs-two-schemas: + subject: + type: OneOf + message: Use at least two schemas in oneOf + assertions: + minLength: 2 +``` + ### `any` example The following example asserts that the maximum length of each description is 20 characters. diff --git a/docs/@v2/rules/oas/no-illogical-composition-keywords.md b/docs/@v2/rules/oas/no-illogical-composition-keywords.md new file mode 100644 index 0000000000..dd7a5afa46 --- /dev/null +++ b/docs/@v2/rules/oas/no-illogical-composition-keywords.md @@ -0,0 +1,206 @@ +--- +slug: /docs/cli/rules/oas/no-illogical-composition-keywords +--- + +# no-illogical-composition-keywords + +Ensures that `oneOf`, `anyOf`, and `allOf` combine schemas that a value can actually resolve against. + +The rule reports: + +- A `oneOf` or `anyOf` with fewer than two schemas, unless the schema declares a `discriminator`. +- An `allOf` with fewer than two schemas that neither declares another keyword of its own nor extends a discriminated schema. +- A schema repeated inside the same keyword. +- An empty schema (`{}`) used as a member. +- Two `oneOf` schemas that a single value can match at the same time. +- A nullable schema whose `oneOf` also accepts `null`. +- A `discriminator` whose property is missing from `required` in every member schema. +- An inline `oneOf` or `anyOf` member that a `discriminator` cannot select. + +| OAS | Compatibility | +| --- | ------------- | +| 2.0 | ❌ | +| 3.0 | ✅ | +| 3.1 | ✅ | +| 3.2 | ✅ | + +```mermaid +flowchart TD + +Root ==> components --> NamedSchemas --> Schema + +Schema ==> OneOf +Schema ==> AnyOf +Schema ==> AllOf + +style OneOf fill:#codaf9,stroke:#0044d4,stroke-width:5px +style AnyOf fill:#codaf9,stroke:#0044d4,stroke-width:5px +style AllOf fill:#codaf9,stroke:#0044d4,stroke-width:5px +``` + +## API design principles + +`oneOf` means "exactly one". +When a value matches two of the listed schemas, no tool can tell which one was intended, and validators, code generators, and documentation all disagree about the result. + +The most common version of this is nullability: if a referenced schema already accepts `null` and the `oneOf` also lists `type: 'null'`, a null value matches both branches. +The same ambiguity appears one level up, when the schema holding the `oneOf` is itself nullable and a member accepts `null` too. + +Deciding whether two arbitrary schemas overlap is not solvable in general, so the comparison stays deliberately narrow. +It reads `type`, `nullable`, `enum`, `const`, `properties`, `required`, and `additionalProperties: false`. +A `const` counts as a single-value `enum`, so one member can use `enum` and the other `const`. +When a member uses any other constraint, such as `not`, `pattern`, `minimum`, or a nested `allOf`, that constraint may be what separates the schemas, so the rule reports nothing for the pair. + +A `discriminator` names the property that tells the members apart, so the rule trusts it and checks only what the specification requires. +The property must be listed in `required` in every member schema, because a value can otherwise omit it and nothing decides which schema applies. +Every member must also be a `$ref`: a `discriminator` selects a schema by its component name, and the specification states that inline `oneOf` and `anyOf` subschemas are not considered, so an inline member can never be selected. +A member that declares `$id` is exempt, because a `mapping` entry can name it by URI. + +Wrapping one schema in `allOf` to attach sibling keywords, such as `description` or `readOnly` next to a `$ref`, stays common because support for `$ref` siblings is uneven across tools. +Referencing a schema that declares a `discriminator` carries meaning of its own too: the discriminator resolves the subtype by its schema name, so the wrapper declares a subtype even when it adds no properties. +The rule reports an `allOf` wrapper only when neither applies. + +## Configuration + +| Option | Type | Description | +| -------- | ------ | ----------------------------------------------------------------------------------------- | +| severity | string | Possible values: `off`, `warn`, `error`. Default `warn` (in `recommended` configuration). | + +An example configuration: + +```yaml +rules: + no-illogical-composition-keywords: error +``` + +## Examples + +Given this configuration: + +```yaml +rules: + no-illogical-composition-keywords: error +``` + +Example of an **incorrect** `oneOf` where both schemas accept `null`: + +```yaml +components: + schemas: + TimeShift: + type: [object, 'null'] + Invoice: + oneOf: + - $ref: '#/components/schemas/TimeShift' + - type: 'null' +``` + +Example of a **correct** `oneOf`: + +```yaml +components: + schemas: + TimeShift: + type: object + Invoice: + oneOf: + - $ref: '#/components/schemas/TimeShift' + - type: 'null' +``` + +Example of an **incorrect** `discriminator` with an inline member: + +```yaml +components: + schemas: + Cat: + type: object + properties: + petType: + type: string + required: [petType] + Pet: + discriminator: + propertyName: petType + oneOf: + - $ref: '#/components/schemas/Cat' + - type: object + properties: + petType: + type: string + required: [petType] +``` + +> Move the inline schema into `components/schemas` and reference it with a `$ref`. + +Example of an **incorrect** `discriminator`, where `petType` is optional: + +```yaml +components: + schemas: + Pet: + discriminator: + propertyName: petType + oneOf: + - $ref: '#/components/schemas/Cat' + - $ref: '#/components/schemas/Dog' + Cat: + type: object + properties: + petType: + type: string + Dog: + type: object + properties: + petType: + type: string +``` + +> Add `petType` to `required` in both `Cat` and `Dog` to fix this. + +Example of a **correct** single-schema `allOf` that declares a subtype: + +```yaml +components: + schemas: + Pet: + type: object + required: [petType] + properties: + petType: + type: string + discriminator: + propertyName: petType + Cat: + allOf: + - $ref: '#/components/schemas/Pet' +``` + +Example of **incorrect** composition keywords: + +```yaml +components: + schemas: + Pet: + oneOf: + - $ref: '#/components/schemas/Cat' + Animal: + allOf: + - $ref: '#/components/schemas/Cat' + - $ref: '#/components/schemas/Cat' + - {} +``` + +> `Pet` wraps a single schema, and `Animal` repeats one schema and adds an empty one that matches any value. + +## Related rules + +- [no-schema-type-mismatch](../common/no-schema-type-mismatch.md) +- [no-required-schema-properties-undefined](../common/no-required-schema-properties-undefined.md) +- [spec-discriminator-defaultMapping](./spec-discriminator-defaultMapping.md) + +## Resources + +- [Rule source](https://github.com/Redocly/redocly-cli/blob/main/packages/core/src/rules/oas3/no-illogical-composition-keywords.ts) +- [Schema object docs](https://redocly.com/docs/openapi-visual-reference/schemas/) +- [Discriminator object docs](https://redocly.com/docs/openapi-visual-reference/discriminator/) diff --git a/docs/@v2/rules/recommended.md b/docs/@v2/rules/recommended.md index d607d24f85..9df4032702 100644 --- a/docs/@v2/rules/recommended.md +++ b/docs/@v2/rules/recommended.md @@ -51,6 +51,7 @@ Warnings: - [no-ambiguous-paths](./oas/no-ambiguous-paths.md) - [no-duplicated-enum-values](./common/no-duplicated-enum-values.md) - [no-duplicated-tag-names](./oas/no-duplicated-tag-names.md) +- [no-illogical-composition-keywords](./oas/no-illogical-composition-keywords.md) - [no-invalid-media-type-examples](./oas/no-invalid-media-type-examples.md) - [no-invalid-parameter-examples](./oas/no-invalid-parameter-examples.md) - [no-invalid-schema-examples](./oas/no-invalid-schema-examples.md) diff --git a/docs/@v2/rules/ruleset-templates.md b/docs/@v2/rules/ruleset-templates.md index c18cc53b7b..71d53bf384 100644 --- a/docs/@v2/rules/ruleset-templates.md +++ b/docs/@v2/rules/ruleset-templates.md @@ -187,6 +187,7 @@ rules: no-empty-servers: error no-enum-type-mismatch: error no-identical-paths: error + no-illogical-composition-keywords: warn no-invalid-media-type-examples: warn no-invalid-parameter-examples: warn no-invalid-schema-examples: warn @@ -237,6 +238,7 @@ rules: no-enum-type-mismatch: error no-example-value-and-externalValue: error no-identical-paths: error + no-illogical-composition-keywords: warn no-invalid-media-type-examples: warn no-invalid-parameter-examples: warn no-invalid-schema-examples: warn @@ -283,6 +285,7 @@ rules: no-enum-type-mismatch: error no-example-value-and-externalValue: error no-identical-paths: error + no-illogical-composition-keywords: warn no-invalid-media-type-examples: warn no-invalid-parameter-examples: warn no-invalid-schema-examples: warn diff --git a/docs/@v2/v2.sidebars.yaml b/docs/@v2/v2.sidebars.yaml index a7720f844e..f0744958f3 100644 --- a/docs/@v2/v2.sidebars.yaml +++ b/docs/@v2/v2.sidebars.yaml @@ -122,6 +122,7 @@ - page: rules/oas/no-example-value-and-externalValue.md - page: rules/oas/no-http-verbs-in-paths.md - page: rules/oas/no-identical-paths.md + - page: rules/oas/no-illogical-composition-keywords.md - page: rules/oas/no-invalid-media-type-examples.md - page: rules/oas/no-invalid-parameter-examples.md - page: rules/oas/no-invalid-schema-examples.md diff --git a/packages/core/src/__tests__/__snapshots__/redocly-yaml.test.ts.snap b/packages/core/src/__tests__/__snapshots__/redocly-yaml.test.ts.snap index 61d9e0c6fe..67e81cb31f 100644 --- a/packages/core/src/__tests__/__snapshots__/redocly-yaml.test.ts.snap +++ b/packages/core/src/__tests__/__snapshots__/redocly-yaml.test.ts.snap @@ -1384,6 +1384,9 @@ exports[`createConfigTypes > matches snapshot for the default config schema 1`] "EncodingMap", "HeadersMap", "Link", + "AllOf", + "AnyOf", + "OneOf", "DiscriminatorMapping", "Discriminator", "Components", diff --git a/packages/core/src/config/__tests__/__snapshots__/config-resolvers.test.ts.snap b/packages/core/src/config/__tests__/__snapshots__/config-resolvers.test.ts.snap index e131e1d901..16aef564c3 100644 --- a/packages/core/src/config/__tests__/__snapshots__/config-resolvers.test.ts.snap +++ b/packages/core/src/config/__tests__/__snapshots__/config-resolvers.test.ts.snap @@ -172,6 +172,7 @@ exports[`resolveConfig > should ignore minimal from the root and read local file "no-example-value-and-externalValue": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": { "allowAdditionalProperties": false, "severity": "error", @@ -239,6 +240,7 @@ exports[`resolveConfig > should ignore minimal from the root and read local file "no-example-value-and-externalValue": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": "error", "no-invalid-parameter-examples": "warn", "no-invalid-schema-examples": "warn", @@ -302,6 +304,7 @@ exports[`resolveConfig > should ignore minimal from the root and read local file "no-enum-type-mismatch": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": "error", "no-invalid-parameter-examples": "warn", "no-invalid-schema-examples": "warn", @@ -620,6 +623,7 @@ exports[`resolveConfig > should resolve extends with local file config which con "no-example-value-and-externalValue": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": { "allowAdditionalProperties": false, "severity": "warn", @@ -687,6 +691,7 @@ exports[`resolveConfig > should resolve extends with local file config which con "no-example-value-and-externalValue": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "warn", "no-invalid-schema-examples": "warn", @@ -750,6 +755,7 @@ exports[`resolveConfig > should resolve extends with local file config which con "no-enum-type-mismatch": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "warn", "no-invalid-schema-examples": "warn", diff --git a/packages/core/src/config/__tests__/load.test.ts b/packages/core/src/config/__tests__/load.test.ts index f8c15df694..e4b95963b9 100644 --- a/packages/core/src/config/__tests__/load.test.ts +++ b/packages/core/src/config/__tests__/load.test.ts @@ -300,6 +300,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": { "allowAdditionalProperties": false, "severity": "warn", @@ -369,6 +370,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "off", "no-invalid-schema-examples": "off", @@ -434,6 +436,7 @@ describe('loadConfig', () => { "no-enum-type-mismatch": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "off", "no-invalid-schema-examples": "off", @@ -681,6 +684,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": { "allowAdditionalProperties": false, "severity": "warn", @@ -748,6 +752,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "warn", "no-invalid-schema-examples": "warn", @@ -811,6 +816,7 @@ describe('loadConfig', () => { "no-enum-type-mismatch": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "warn", "no-invalid-schema-examples": "warn", @@ -1071,6 +1077,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": { "allowAdditionalProperties": false, "severity": "warn", @@ -1138,6 +1145,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "off", "no-invalid-schema-examples": "error", @@ -1201,6 +1209,7 @@ describe('loadConfig', () => { "no-enum-type-mismatch": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "off", "no-invalid-schema-examples": "error", @@ -1549,6 +1558,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": { "allowAdditionalProperties": false, "severity": "warn", @@ -1618,6 +1628,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "off", "no-invalid-schema-examples": "off", @@ -1683,6 +1694,7 @@ describe('loadConfig', () => { "no-enum-type-mismatch": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "off", "no-invalid-schema-examples": "off", @@ -1930,6 +1942,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": { "allowAdditionalProperties": false, "severity": "warn", @@ -1997,6 +2010,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "warn", "no-invalid-schema-examples": "warn", @@ -2060,6 +2074,7 @@ describe('loadConfig', () => { "no-enum-type-mismatch": "error", "no-http-verbs-in-paths": "off", "no-identical-paths": "error", + "no-illogical-composition-keywords": "warn", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "warn", "no-invalid-schema-examples": "warn", @@ -2320,6 +2335,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": { "allowAdditionalProperties": false, "severity": "warn", @@ -2387,6 +2403,7 @@ describe('loadConfig', () => { "no-example-value-and-externalValue": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "off", "no-invalid-schema-examples": "error", @@ -2450,6 +2467,7 @@ describe('loadConfig', () => { "no-enum-type-mismatch": "warn", "no-http-verbs-in-paths": "off", "no-identical-paths": "warn", + "no-illogical-composition-keywords": "off", "no-invalid-media-type-examples": "warn", "no-invalid-parameter-examples": "off", "no-invalid-schema-examples": "error", diff --git a/packages/core/src/config/all.ts b/packages/core/src/config/all.ts index 6655b7ef1f..23b3ec8f2b 100644 --- a/packages/core/src/config/all.ts +++ b/packages/core/src/config/all.ts @@ -76,6 +76,7 @@ const all: RawGovernanceConfig<'built-in'> = { 'no-enum-type-mismatch': 'error', 'no-required-schema-properties-undefined': 'error', 'no-schema-type-mismatch': 'error', + 'no-illogical-composition-keywords': 'error', 'no-invalid-media-type-examples': 'error', 'no-server-example.com': 'error', 'no-server-trailing-slash': 'error', @@ -144,6 +145,7 @@ const all: RawGovernanceConfig<'built-in'> = { 'no-enum-type-mismatch': 'error', 'no-required-schema-properties-undefined': 'error', 'no-schema-type-mismatch': 'error', + 'no-illogical-composition-keywords': 'error', 'no-invalid-media-type-examples': 'error', 'no-mixed-number-range-constraints': 'error', 'no-server-example.com': 'error', @@ -212,6 +214,7 @@ const all: RawGovernanceConfig<'built-in'> = { 'no-enum-type-mismatch': 'error', 'no-required-schema-properties-undefined': 'error', 'no-schema-type-mismatch': 'error', + 'no-illogical-composition-keywords': 'error', 'no-invalid-media-type-examples': 'error', 'no-mixed-number-range-constraints': 'error', 'no-server-example.com': 'error', diff --git a/packages/core/src/config/minimal.ts b/packages/core/src/config/minimal.ts index bb18b843af..69861c7426 100644 --- a/packages/core/src/config/minimal.ts +++ b/packages/core/src/config/minimal.ts @@ -68,6 +68,7 @@ const minimal: RawGovernanceConfig<'built-in'> = { 'no-example-value-and-externalValue': 'warn', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'warn', + 'no-illogical-composition-keywords': 'off', 'no-invalid-media-type-examples': { severity: 'warn', allowAdditionalProperties: false, @@ -133,6 +134,7 @@ const minimal: RawGovernanceConfig<'built-in'> = { 'no-example-value-and-externalValue': 'warn', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'warn', + 'no-illogical-composition-keywords': 'off', 'no-invalid-media-type-examples': 'warn', 'no-invalid-parameter-examples': 'off', 'no-invalid-schema-examples': 'off', @@ -194,6 +196,7 @@ const minimal: RawGovernanceConfig<'built-in'> = { 'no-enum-type-mismatch': 'warn', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'warn', + 'no-illogical-composition-keywords': 'off', 'no-invalid-media-type-examples': 'warn', 'no-invalid-parameter-examples': 'off', 'no-invalid-schema-examples': 'off', diff --git a/packages/core/src/config/recommended-strict.ts b/packages/core/src/config/recommended-strict.ts index f04c6196d1..5ea44ab8d0 100644 --- a/packages/core/src/config/recommended-strict.ts +++ b/packages/core/src/config/recommended-strict.ts @@ -68,6 +68,7 @@ const recommendedStrict: RawGovernanceConfig<'built-in'> = { 'no-example-value-and-externalValue': 'error', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'error', + 'no-illogical-composition-keywords': 'error', 'no-invalid-media-type-examples': { severity: 'error', allowAdditionalProperties: false, @@ -133,6 +134,7 @@ const recommendedStrict: RawGovernanceConfig<'built-in'> = { 'no-example-value-and-externalValue': 'error', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'error', + 'no-illogical-composition-keywords': 'error', 'no-invalid-media-type-examples': 'error', 'no-invalid-parameter-examples': 'error', 'no-invalid-schema-examples': 'error', @@ -194,6 +196,7 @@ const recommendedStrict: RawGovernanceConfig<'built-in'> = { 'no-enum-type-mismatch': 'error', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'error', + 'no-illogical-composition-keywords': 'error', 'no-invalid-media-type-examples': 'error', 'no-invalid-parameter-examples': 'error', 'no-invalid-schema-examples': 'error', diff --git a/packages/core/src/config/recommended.ts b/packages/core/src/config/recommended.ts index 44fbe65768..d0c8823ce5 100644 --- a/packages/core/src/config/recommended.ts +++ b/packages/core/src/config/recommended.ts @@ -68,6 +68,7 @@ const recommended: RawGovernanceConfig<'built-in'> = { 'no-example-value-and-externalValue': 'error', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'error', + 'no-illogical-composition-keywords': 'warn', 'no-invalid-media-type-examples': { severity: 'warn', allowAdditionalProperties: false, @@ -133,6 +134,7 @@ const recommended: RawGovernanceConfig<'built-in'> = { 'no-example-value-and-externalValue': 'error', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'error', + 'no-illogical-composition-keywords': 'warn', 'no-invalid-media-type-examples': 'warn', 'no-invalid-parameter-examples': 'warn', 'no-invalid-schema-examples': 'warn', @@ -194,6 +196,7 @@ const recommended: RawGovernanceConfig<'built-in'> = { 'no-enum-type-mismatch': 'error', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'error', + 'no-illogical-composition-keywords': 'warn', 'no-invalid-media-type-examples': 'warn', 'no-invalid-parameter-examples': 'warn', 'no-invalid-schema-examples': 'warn', diff --git a/packages/core/src/config/spec.ts b/packages/core/src/config/spec.ts index b6e8056668..4ee2d06a92 100644 --- a/packages/core/src/config/spec.ts +++ b/packages/core/src/config/spec.ts @@ -68,6 +68,7 @@ const spec: RawGovernanceConfig<'built-in'> = { 'no-example-value-and-externalValue': 'error', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'error', + 'no-illogical-composition-keywords': 'off', 'no-invalid-media-type-examples': { severity: 'off', allowAdditionalProperties: false, @@ -133,6 +134,7 @@ const spec: RawGovernanceConfig<'built-in'> = { 'no-example-value-and-externalValue': 'error', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'error', + 'no-illogical-composition-keywords': 'off', 'no-invalid-media-type-examples': 'off', 'no-invalid-parameter-examples': 'off', 'no-invalid-schema-examples': 'off', @@ -194,6 +196,7 @@ const spec: RawGovernanceConfig<'built-in'> = { 'no-enum-type-mismatch': 'off', 'no-http-verbs-in-paths': 'off', 'no-identical-paths': 'error', + 'no-illogical-composition-keywords': 'off', 'no-invalid-media-type-examples': 'off', 'no-invalid-parameter-examples': 'off', 'no-invalid-schema-examples': 'off', diff --git a/packages/core/src/rules/oas3/__tests__/no-illogical-composition-keywords.test.ts b/packages/core/src/rules/oas3/__tests__/no-illogical-composition-keywords.test.ts new file mode 100644 index 0000000000..168ca86719 --- /dev/null +++ b/packages/core/src/rules/oas3/__tests__/no-illogical-composition-keywords.test.ts @@ -0,0 +1,1630 @@ +import { outdent } from 'outdent'; + +import { parseYamlToDocument, replaceSourceWithRef } from '../../../../__tests__/utils.js'; +import { createConfig } from '../../../config/index.js'; +import { lintDocument } from '../../../lint.js'; +import { BaseResolver } from '../../../resolve.js'; + +describe('Oas3 no-illogical-composition-keywords', () => { + describe('oneOf', () => { + it('should report when oneOf has only one schema', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "\`oneOf\` should have at least two schemas. Use the schema directly instead.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report when oneOf has only one schema but the parent declares a discriminator', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + discriminator: + propertyName: petType + oneOf: + - $ref: '#/components/schemas/Cat' + Cat: + type: object + properties: + petType: + type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report duplicated schemas that are not next to each other', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - $ref: '#/components/schemas/Cat' + - type: integer + - $ref: '#/components/schemas/Cat' + Cat: + type: object + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf/2", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`oneOf\` duplicates the schema at position 1.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report distinct refs that resolve to equal schemas as overlapping', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - $ref: '#/components/schemas/Cat' + - $ref: '#/components/schemas/Kitten' + Cat: + type: object + properties: + id: + type: string + Kitten: + type: object + properties: + id: + type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: \`#/components/schemas/Cat\` and \`#/components/schemas/Kitten\`. Both schemas define \`id\` without constraints that exclude each other. Add a discriminator, or constrain the shared properties to different values.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report a discriminator gap when `propertyName` is not a string', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Cat: + type: object + properties: + name: + type: string + required: [name] + Dog: + type: object + properties: + name: + type: string + bark: + type: string + required: [name] + Test: + discriminator: + propertyName: + name: + type: string + oneOf: + - $ref: '#/components/schemas/Cat' + - $ref: '#/components/schemas/Dog' + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report inline members that a discriminator cannot select', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Cat: + type: object + properties: + petType: + type: string + required: [petType] + Test: + discriminator: + propertyName: petType + oneOf: + - $ref: '#/components/schemas/Cat' + - type: object + properties: + petType: + type: string + required: [petType] + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf/1", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`oneOf\` is inline, so the \`discriminator\` cannot select it. Use a \`$ref\` to a named schema.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should compare `const` against `enum` as a single allowed value', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Exclusive: + oneOf: + - type: string + enum: [card] + - type: string + const: bank + Overlapping: + oneOf: + - enum: [card] + - const: card + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Overlapping/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: schema at position 1 and schema at position 2. Both schemas allow the values ["card"].", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report an empty schema inside oneOf', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: string + - {} + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf/1", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`oneOf\` is empty, so it matches any value.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report when a referenced schema and a null schema both accept null', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - $ref: '#/components/schemas/ContainsNull' + - type: 'null' + ContainsNull: + type: + - object + - 'null' + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: \`#/components/schemas/ContainsNull\` and schema at position 2. Both schemas accept \`null\`.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report when type sets overlap', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: string + - type: + - string + - integer + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: schema at position 1 and schema at position 2. Both schemas accept \`string\`.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report when a shared property uses an unmodelled keyword such as `not`', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - $ref: '#/components/schemas/Lizard' + - $ref: '#/components/schemas/OtherPet' + Lizard: + type: object + required: [petType] + properties: + petType: + type: string + enum: ['Lizard'] + OtherPet: + type: object + required: [petType] + properties: + petType: + not: + enum: ['Lizard'] + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should not crash on boolean schemas used as members or property schemas', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Members: + oneOf: + - true + - type: string + Properties: + oneOf: + - type: object + required: [kind] + properties: + kind: true + - type: object + required: [kind] + properties: + kind: + type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report when both members declare an empty required list', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: object + required: [] + properties: + name: + type: string + - type: object + title: Second + required: [] + properties: + name: + type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: schema at position 1 and \`Second\`. Both schemas define \`name\` without constraints that exclude each other. Add a discriminator, or constrain the shared properties to different values.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report when one member forbids a property the other requires', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: object + additionalProperties: false + required: [a] + properties: + a: + type: string + - type: object + required: [a, b] + properties: + a: + type: string + b: + type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report when an exclusive shared property is not required, without a discriminator', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: object + properties: + kind: + type: string + enum: [cat] + - type: object + properties: + kind: + type: string + enum: [dog] + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: schema at position 1 and schema at position 2. Add \`kind\` to \`required\` in every schema; an optional property cannot distinguish the schemas.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report each overlapping pair of a oneOf separately', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: string + - type: string + title: Second + - type: string + title: Third + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: schema at position 1 and \`Second\`. Both schemas accept \`string\`.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: schema at position 1 and \`Third\`. Both schemas accept \`string\`.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: \`Second\` and \`Third\`. Both schemas accept \`string\`.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report the discriminator property as required when members do not declare it', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Pet: + discriminator: + propertyName: petType + oneOf: + - $ref: '#/components/schemas/Cat' + - $ref: '#/components/schemas/Dog' + Cat: + type: object + properties: + petType: + type: string + Dog: + type: object + properties: + petType: + type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Pet/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: \`#/components/schemas/Cat\` and \`#/components/schemas/Dog\`. Add \`petType\` to \`required\` in every schema; the \`discriminator\` cannot read a property a value may omit.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report when the discriminator property is required in every member', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Pet: + discriminator: + propertyName: petType + oneOf: + - $ref: '#/components/schemas/Cat' + - $ref: '#/components/schemas/Dog' + Cat: + type: object + required: [petType, name] + properties: + name: + type: string + Dog: + type: object + required: [petType, name] + properties: + name: + type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report members that differ only by `format`', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: object + required: [id] + properties: + id: + type: string + format: uuid + - type: object + required: [id] + properties: + id: + type: string + format: uri + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: schema at position 1 and schema at position 2. Both schemas define \`id\` without constraints that exclude each other. Add a discriminator, or constrain the shared properties to different values.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report when the schema and a `oneOf` member both accept null', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + NullableParent: + nullable: true + oneOf: + - type: string + nullable: true + - type: integer + PlainParent: + oneOf: + - type: string + nullable: true + - type: integer + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/NullableParent/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "The schema and a schema in \`oneOf\` both accept \`null\`, so nothing decides which one applies to a null value.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report when a required property with exclusive values tells the members apart', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: object + additionalProperties: + type: string + properties: + kind: + type: string + enum: [cat] + required: [kind] + - type: object + additionalProperties: + type: string + properties: + kind: + type: string + enum: [dog] + required: [kind] + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report when members share a property with no constraint that tells them apart', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: object + additionalProperties: false + properties: + name: + type: string + required: [name] + - type: object + properties: + name: + type: string + required: [name] + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: schema at position 1 and schema at position 2. Both schemas define \`name\` without constraints that exclude each other. Add a discriminator, or constrain the shared properties to different values.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report when the discriminator property is not required in every member', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + discriminator: + propertyName: petType + oneOf: + - type: object + properties: + petType: + type: string + enum: [cat] + - type: object + properties: + petType: + type: string + enum: [dog] + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf/0", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`oneOf\` is inline, so the \`discriminator\` cannot select it. Use a \`$ref\` to a named schema.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf/1", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`oneOf\` is inline, so the \`discriminator\` cannot select it. Use a \`$ref\` to a named schema.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: schema at position 1 and schema at position 2. Add \`petType\` to \`required\` in every schema; the \`discriminator\` cannot read a property a value may omit.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report when members have different types', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: string + - type: integer + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + }); + + describe('anyOf', () => { + it('should report when anyOf has only one schema', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + anyOf: + - type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/anyOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "\`anyOf\` should have at least two schemas. Use the schema directly instead.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report duplicated and empty schemas inside anyOf', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + anyOf: + - type: string + - type: string + - {} + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/anyOf/2", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`anyOf\` is empty, so it matches any value.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/components/schemas/Test/anyOf/1", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`anyOf\` duplicates the schema at position 1.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report anyOf used with a discriminator by default', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + discriminator: + propertyName: petType + anyOf: + - $ref: '#/components/schemas/Cat' + - $ref: '#/components/schemas/Dog' + Cat: + type: object + Dog: + type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + }); + + describe('allOf', () => { + it('should report an allOf wrapper that adds nothing', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + allOf: + - $ref: '#/components/schemas/Cat' + Cat: + type: object + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/allOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "\`allOf\` should have at least two schemas. Use the schema directly instead.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should not report a single-schema allOf used to attach sibling keywords', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + type: object + properties: + customerId: + readOnly: true + allOf: + - $ref: '#/components/schemas/CustomerId' + CustomerId: + type: string + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should not report a single-schema allOf declaring a subtype of a discriminated schema', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.3 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Pet: + type: object + required: [petType] + properties: + petType: + type: string + discriminator: + propertyName: petType + Cat: + allOf: + - $ref: '#/components/schemas/Pet' + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); + }); + + it('should report an empty allOf', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + allOf: [] + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/allOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "\`allOf\` should have at least two schemas.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should report duplicated and empty schemas inside allOf', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + allOf: + - $ref: '#/components/schemas/Cat' + - $ref: '#/components/schemas/Cat' + - {} + Cat: + type: object + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/allOf/2", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`allOf\` is empty, so it matches any value.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/components/schemas/Test/allOf/1", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`allOf\` duplicates the schema at position 1.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + }); + + it('should check every composition keyword used on the same schema', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.1.0 + info: + title: Test + version: '1.0' + paths: {} + components: + schemas: + Test: + oneOf: + - type: string + allOf: + - $ref: '#/components/schemas/Cat' + - $ref: '#/components/schemas/Cat' + Cat: + type: object + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ + rules: { 'no-illogical-composition-keywords': 'error' }, + }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Test/allOf/1", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Schema in \`allOf\` duplicates the schema at position 1.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/components/schemas/Test/oneOf", + "reportOnKey": true, + "source": "foobar.yaml", + }, + ], + "message": "\`oneOf\` should have at least two schemas. Use the schema directly instead.", + "ruleId": "no-illogical-composition-keywords", + "severity": "error", + "suggest": [], + }, + ] + `); + }); +}); diff --git a/packages/core/src/rules/oas3/index.ts b/packages/core/src/rules/oas3/index.ts index 05def43644..ce7066f7e2 100644 --- a/packages/core/src/rules/oas3/index.ts +++ b/packages/core/src/rules/oas3/index.ts @@ -49,6 +49,7 @@ import { BooleanParameterPrefixes } from './boolean-parameter-prefixes.js'; import { ComponentNameUnique } from './component-name-unique.js'; import { NoEmptyServers } from './no-empty-servers.js'; import { NoExampleValueAndExternalValue } from './no-example-value-and-externalValue.js'; +import { NoIllogicalCompositionKeywords } from './no-illogical-composition-keywords.js'; import { ValidContentExamples } from './no-invalid-media-type-examples.js'; import { NoServerExample } from './no-server-example.com.js'; import { NoServerTrailingSlash } from './no-server-trailing-slash.js'; @@ -106,6 +107,7 @@ export const rules: Oas3RuleSet<'built-in'> = { 'paths-kebab-case': PathsKebabCase as Oas3Rule, 'boolean-parameter-prefixes': BooleanParameterPrefixes, 'path-http-verbs-order': PathHttpVerbsOrder as Oas3Rule, + 'no-illogical-composition-keywords': NoIllogicalCompositionKeywords, 'no-invalid-media-type-examples': ValidContentExamples, 'no-identical-paths': NoIdenticalPaths as Oas3Rule, 'no-ambiguous-paths': NoAmbiguousPaths as Oas3Rule, diff --git a/packages/core/src/rules/oas3/no-illogical-composition-keywords.ts b/packages/core/src/rules/oas3/no-illogical-composition-keywords.ts new file mode 100644 index 0000000000..b65441e970 --- /dev/null +++ b/packages/core/src/rules/oas3/no-illogical-composition-keywords.ts @@ -0,0 +1,497 @@ +import { isRef } from '../../ref-utils.js'; +import type { + Oas3Discriminator, + Oas3Schema, + Oas3_1Schema, + Referenced, +} from '../../typings/openapi.js'; +import { dequal } from '../../utils/dequal.js'; +import { getOwn } from '../../utils/get-own.js'; +import { isDefined } from '../../utils/is-defined.js'; +import { isPlainObject } from '../../utils/is-plain-object.js'; +import type { Oas3Rule } from '../../visitors.js'; +import type { UserContext, ResolveFn } from '../../walk.js'; + +type CompositionSchema = Oas3Schema | Oas3_1Schema; +type CompositionKeyword = 'oneOf' | 'anyOf' | 'allOf'; + +type SourcedSchema = { + schema: CompositionSchema; + source: string | undefined; +}; + +type SchemaProperties = NonNullable; + +type PropertyComparison = { + leftSource: string | undefined; + rightSource: string | undefined; + leftProperties: SchemaProperties; + rightProperties: SchemaProperties; + leftRequired: Set; + rightRequired: Set; + sharedNames: string[]; +}; + +// Every keyword a schema may carry without stopping the comparison. A keyword outside this set +// could be the one that separates two schemas, so `hasUnsupportedConstraint` gives up on the pair. +// Membership therefore has two reasons: the checks read it, or it constrains no value at all. +const UNDERSTOOD_KEYWORDS: ReadonlySet = new Set([ + 'type', + 'format', + 'enum', + 'const', + 'nullable', + 'properties', + 'required', + 'additionalProperties', + 'discriminator', + '$ref', + '$id', + '$schema', + '$anchor', + '$comment', + 'title', + 'description', + 'default', + 'example', + 'examples', + 'deprecated', + 'readOnly', + 'writeOnly', + 'externalDocs', + 'xml', +]); + +export const NoIllogicalCompositionKeywords: Oas3Rule = () => { + return { + AllOf(members, ctx) { + const parentSchema = getParentSchema(ctx); + if (!Array.isArray(members) || !parentSchema) return; + + if ( + members.length === 0 || + (members.length === 1 && + Object.keys(parentSchema).length === 1 && + !isSubtypeOfDiscriminatedSchema(members[0], ctx.resolve)) + ) { + reportSingleSchema(members, 'allOf', ctx); + } + + reportEmptyMembers(members, 'allOf', ctx); + reportDuplicateMembers(members, 'allOf', ctx); + }, + + AnyOf(members, ctx) { + const parentSchema = getParentSchema(ctx); + if (!Array.isArray(members) || !parentSchema) return; + + const hasDiscriminator = isPlainObject(parentSchema.discriminator); + + if (members.length < 2 && !hasDiscriminator) { + reportSingleSchema(members, 'anyOf', ctx); + } + + reportEmptyMembers(members, 'anyOf', ctx); + reportDuplicateMembers(members, 'anyOf', ctx); + + if (hasDiscriminator) { + reportInlineMembers(members, 'anyOf', ctx); + } + }, + + OneOf(members, ctx) { + const parentSchema = getParentSchema(ctx); + if (!Array.isArray(members) || !parentSchema) return; + + const { discriminator } = parentSchema; + const hasDiscriminator = isPlainObject(discriminator); + + if (members.length < 2 && !hasDiscriminator) { + reportSingleSchema(members, 'oneOf', ctx); + } + + reportEmptyMembers(members, 'oneOf', ctx); + reportDuplicateMembers(members, 'oneOf', ctx); + + if (hasDiscriminator) { + reportInlineMembers(members, 'oneOf', ctx); + } + + if (schemaAllowsNull(parentSchema)) { + reportNullableParent(members, ctx); + } + + reportAmbiguousMembers(members, hasDiscriminator ? discriminator : undefined, ctx); + }, + }; +}; + +// `struct` is not guaranteed to have rejected a malformed composition first. +function getParentSchema(ctx: UserContext): CompositionSchema | undefined { + return isPlainObject(ctx.parent) ? ctx.parent : undefined; +} + +function reportSingleSchema( + members: CompositionSchema[], + keyword: CompositionKeyword, + ctx: UserContext +) { + const suggestion = members.length === 1 ? ' Use the schema directly instead.' : ''; + ctx.report({ + message: `\`${keyword}\` should have at least two schemas.${suggestion}`, + location: ctx.location.key(), + }); +} + +function isSubtypeOfDiscriminatedSchema( + member: Referenced, + resolve: ResolveFn +): boolean { + return isPlainObject(resolveSchema(member, resolve)?.schema.discriminator); +} + +function reportEmptyMembers( + members: CompositionSchema[], + keyword: CompositionKeyword, + ctx: UserContext +) { + for (const [index, member] of members.entries()) { + if (isPlainObject(member) && Object.keys(member).length === 0) { + ctx.report({ + message: `Schema in \`${keyword}\` is empty, so it matches any value.`, + location: ctx.location.child([index]), + }); + } + } +} + +function reportInlineMembers( + members: CompositionSchema[], + keyword: CompositionKeyword, + ctx: UserContext +) { + for (const [index, member] of members.entries()) { + if (isRef(member) || (isPlainObject(member) && '$id' in member)) continue; + + ctx.report({ + message: `Schema in \`${keyword}\` is inline, so the \`discriminator\` cannot select it. Use a \`$ref\` to a named schema.`, + location: ctx.location.child([index]), + }); + } +} + +function reportDuplicateMembers( + members: CompositionSchema[], + keyword: CompositionKeyword, + ctx: UserContext +) { + for (let index = 1; index < members.length; index++) { + const firstIndex = members.findIndex((other) => dequal(other, members[index])); + if (firstIndex < index) { + ctx.report({ + message: `Schema in \`${keyword}\` duplicates the schema at position ${firstIndex + 1}.`, + location: ctx.location.child([index]), + }); + } + } +} + +function reportNullableParent(members: CompositionSchema[], ctx: UserContext) { + const nullableMember = members.some((member) => { + const resolved = resolveSchema(member, ctx.resolve); + return resolved ? schemaAllowsNull(resolved.schema) : false; + }); + if (!nullableMember) return; + + ctx.report({ + message: + 'The schema and a schema in `oneOf` both accept `null`, so nothing decides which one applies to a null value.', + location: ctx.location.key(), + }); +} + +function reportAmbiguousMembers( + members: CompositionSchema[], + discriminator: Oas3Discriminator | undefined, + ctx: UserContext +) { + const { resolve, report, location } = ctx; + const resolvedMembers = members.map((member) => resolveSchema(member, resolve)); + + for (let leftIndex = 0; leftIndex < resolvedMembers.length - 1; leftIndex++) { + const left = resolvedMembers[leftIndex]; + if (!left) continue; + + for (let rightIndex = leftIndex + 1; rightIndex < resolvedMembers.length; rightIndex++) { + const right = resolvedMembers[rightIndex]; + if (!right) continue; + + // Skip only pairs `reportDuplicateMembers` already reported, which compares raw nodes too. + if (dequal(members[leftIndex], members[rightIndex])) continue; + + const reason = findOverlapReason(left, right, resolve, discriminator); + if (!reason) continue; + + report({ + message: `Schemas in \`oneOf\` must be mutually exclusive. Found overlapping schemas: ${describeMember( + members[leftIndex], + leftIndex + )} and ${describeMember(members[rightIndex], rightIndex)}. ${reason}`, + location: location.key(), + }); + } + } +} + +function describeMember(member: CompositionSchema, index: number): string { + if (isRef(member)) return `\`${member.$ref}\``; + if (member.title) return `\`${member.title}\``; + return `schema at position ${index + 1}`; +} + +function findOverlapReason( + left: SourcedSchema, + right: SourcedSchema, + resolve: ResolveFn, + discriminator?: Oas3Discriminator +): string | null { + const leftSchema = left.schema; + const rightSchema = right.schema; + + if (hasUnsupportedConstraint(leftSchema) || hasUnsupportedConstraint(rightSchema)) return null; + + if (areExclusive(leftSchema, rightSchema)) return null; + + if (schemaAllowsNull(leftSchema) && schemaAllowsNull(rightSchema)) { + return 'Both schemas accept `null`.'; + } + + const leftValues = readAllowedValues(leftSchema); + const rightValues = readAllowedValues(rightSchema); + if (leftValues && rightValues) { + const shared = leftValues.filter((value) => rightValues.some((other) => dequal(value, other))); + return `Both schemas allow the values ${JSON.stringify(shared)}.`; + } + + if (discriminator) { + return describeDiscriminatorGap(leftSchema, rightSchema, discriminator); + } + + const propertyOverlap = findPropertyOverlap(left, right, resolve); + if (propertyOverlap) return propertyOverlap; + + if (!leftSchema.properties && !rightSchema.properties) { + const sharedTypes = getSharedTypes(leftSchema, rightSchema); + if (sharedTypes?.length) { + return `Both schemas accept ${sharedTypes.map((type) => `\`${type}\``).join(', ')}.`; + } + } + + return null; +} + +function describeDiscriminatorGap( + left: CompositionSchema, + right: CompositionSchema, + discriminator: Oas3Discriminator +): string | null { + const { propertyName } = discriminator; + // `struct` is not guaranteed to have rejected a non-string `propertyName` first. + if (typeof propertyName !== 'string') return null; + + const isRequiredInBoth = + !!left.required?.includes(propertyName) && !!right.required?.includes(propertyName); + if (isRequiredInBoth) return null; + + return `Add \`${propertyName}\` to \`required\` in every schema; the \`discriminator\` cannot read a property a value may omit.`; +} + +function findPropertyOverlap( + left: SourcedSchema, + right: SourcedSchema, + resolve: ResolveFn +): string | null { + const comparison = collectPropertyComparison(left, right); + if (!comparison) return null; + + if (forbidsPropertyRequiredBy(left.schema, comparison.leftProperties, comparison.rightRequired)) { + return null; + } + if ( + forbidsPropertyRequiredBy(right.schema, comparison.rightProperties, comparison.leftRequired) + ) { + return null; + } + + if (schemasRequireDifferentProperties(comparison)) return null; + if (comparison.sharedNames.length === 0) return null; + + const shared = classifySharedProperties(comparison, resolve); + if (!shared) return null; + + const { optionalDistinguishingNames, ambiguousNames } = shared; + + if (optionalDistinguishingNames.length > 0) { + const alsoShared = + ambiguousNames.length > 0 ? ` Other shared properties: ${quoteAll(ambiguousNames)}.` : ''; + return `Add ${quoteAll( + optionalDistinguishingNames + )} to \`required\` in every schema; an optional property cannot distinguish the schemas.${alsoShared}`; + } + + if (ambiguousNames.length > 0) { + return `Both schemas define ${quoteAll( + ambiguousNames + )} without constraints that exclude each other. Add a discriminator, or constrain the shared properties to different values.`; + } + + return null; +} + +function collectPropertyComparison( + left: SourcedSchema, + right: SourcedSchema +): PropertyComparison | null { + const leftProperties = left.schema.properties; + const rightProperties = right.schema.properties; + if (!leftProperties || !rightProperties) return null; + + return { + leftSource: left.source, + rightSource: right.source, + leftProperties, + rightProperties, + leftRequired: new Set(left.schema.required ?? []), + rightRequired: new Set(right.schema.required ?? []), + sharedNames: Object.keys(leftProperties).filter((name) => getOwn(rightProperties, name)), + }; +} + +function schemasRequireDifferentProperties({ + leftProperties, + rightProperties, + leftRequired, + rightRequired, +}: PropertyComparison): boolean { + const leftRequiresPropertyUnknownToRight = [...leftRequired].some( + (name) => !getOwn(rightProperties, name) && !rightRequired.has(name) + ); + const rightRequiresPropertyUnknownToLeft = [...rightRequired].some( + (name) => !getOwn(leftProperties, name) && !leftRequired.has(name) + ); + return leftRequiresPropertyUnknownToRight && rightRequiresPropertyUnknownToLeft; +} + +function classifySharedProperties( + { + leftSource, + rightSource, + leftProperties, + rightProperties, + leftRequired, + rightRequired, + sharedNames, + }: PropertyComparison, + resolve: ResolveFn +): { optionalDistinguishingNames: string[]; ambiguousNames: string[] } | null { + const optionalDistinguishingNames: string[] = []; + const ambiguousNames: string[] = []; + + for (const name of sharedNames) { + const isRequiredInBoth = leftRequired.has(name) && rightRequired.has(name); + + const leftProperty = resolveSchema(getOwn(leftProperties, name), resolve, leftSource); + const rightProperty = resolveSchema(getOwn(rightProperties, name), resolve, rightSource); + if (!leftProperty || !rightProperty) continue; + if ( + hasUnsupportedConstraint(leftProperty.schema) || + hasUnsupportedConstraint(rightProperty.schema) + ) { + continue; + } + + if (areExclusive(leftProperty.schema, rightProperty.schema)) { + if (isRequiredInBoth) return null; + optionalDistinguishingNames.push(name); + continue; + } + + ambiguousNames.push(name); + } + + return { optionalDistinguishingNames, ambiguousNames }; +} + +function forbidsPropertyRequiredBy( + schema: CompositionSchema, + declaredProperties: SchemaProperties, + required: Set +): boolean { + if (schema.additionalProperties !== false) return false; + return [...required].some((name) => !getOwn(declaredProperties, name)); +} + +function areExclusive(left: CompositionSchema, right: CompositionSchema): boolean { + const leftValues = readAllowedValues(left); + const rightValues = readAllowedValues(right); + if (leftValues && rightValues) { + return !leftValues.some((value) => rightValues.some((other) => dequal(value, other))); + } + + // `format` is an annotation, not an assertion, so differing formats prove nothing. + return getSharedTypes(left, right)?.length === 0; +} + +function resolveSchema( + member: Referenced, + resolve: ResolveFn, + from?: string +): SourcedSchema | undefined { + if (isRef(member)) { + const { node, location } = resolve(member, from); + return isPlainObject(node) ? { schema: node, source: location?.source.absoluteRef } : undefined; + } + // JSON Schema allows `true` and `false` as schemas; these checks only read object schemas. + return isPlainObject(member) ? { schema: member, source: from } : undefined; +} + +function hasUnsupportedConstraint(schema: CompositionSchema): boolean { + return Object.keys(schema).some( + (keyword) => !UNDERSTOOD_KEYWORDS.has(keyword) && !keyword.startsWith('x-') + ); +} + +function isNullable(schema: CompositionSchema): boolean { + return 'nullable' in schema && schema.nullable === true; +} + +function getTypeSet(schema: CompositionSchema): Set | undefined { + const declaredType = schema.type; + if (declaredType === undefined) return undefined; + + const types = new Set(Array.isArray(declaredType) ? declaredType : [declaredType]); + if (isNullable(schema)) types.add('null'); + return types; +} + +function getSharedTypes(left: CompositionSchema, right: CompositionSchema): string[] | undefined { + const leftTypes = getTypeSet(left); + const rightTypes = getTypeSet(right); + if (!leftTypes || !rightTypes) return undefined; + + return [...leftTypes].filter((type) => rightTypes.has(type)); +} + +function schemaAllowsNull(schema: CompositionSchema): boolean { + return isNullable(schema) || getTypeSet(schema)?.has('null') === true; +} + +function readAllowedValues(schema: CompositionSchema): unknown[] | undefined { + if (schema.enum) return schema.enum; + const constValue = 'const' in schema ? schema.const : undefined; + return isDefined(constValue) ? [constValue] : undefined; +} + +function quoteAll(names: string[]): string { + return names.map((name) => `\`${name}\``).join(', '); +} diff --git a/packages/core/src/types/index.ts b/packages/core/src/types/index.ts index 40324e440b..1c1f995907 100755 --- a/packages/core/src/types/index.ts +++ b/packages/core/src/types/index.ts @@ -56,7 +56,7 @@ type NormalizedResolveTypeFn = (value: any, key: string) => NormalizedPropType; export function listOf( typeName: string, - opts: { description?: string; documentationLink?: string } = {} + opts: { name?: string; description?: string; documentationLink?: string } = {} ) { return { name: `${typeName}List`, diff --git a/packages/core/src/types/oas3.ts b/packages/core/src/types/oas3.ts index 2d1efc5bef..0e978da426 100755 --- a/packages/core/src/types/oas3.ts +++ b/packages/core/src/types/oas3.ts @@ -567,6 +567,12 @@ const Link: NodeType = { 'The Link object represents a possible design-time link for a response. The presence of a link does not guarantee the caller’s ability to successfully invoke it, rather it provides a known relationship and traversal mechanism between responses and other operations.', }; +const compositionDescription = + 'Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema.'; +const AllOf = listOf('Schema', { name: 'AllOf', description: compositionDescription }); +const AnyOf = listOf('Schema', { name: 'AnyOf', description: compositionDescription }); +const OneOf = listOf('Schema', { name: 'OneOf', description: compositionDescription }); + // draft-00 const Schema: NodeType = { properties: { @@ -599,18 +605,9 @@ const Schema: NodeType = { enum: ['object', 'array', 'string', 'number', 'integer', 'boolean'], description: 'Value MUST be a string. Multiple types via an array are not supported.', }, - allOf: listOf('Schema', { - description: - 'Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema.', - }), - anyOf: listOf('Schema', { - description: - 'Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema.', - }), - oneOf: listOf('Schema', { - description: - 'Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema.', - }), + allOf: AllOf, + anyOf: AnyOf, + oneOf: OneOf, not: 'Schema', properties: 'SchemaProperties', items: (value: unknown) => { @@ -914,6 +911,10 @@ export const Oas3Types = { Link, Logo, Schema, + // Registered so rules and configurable rules can target these node names. + AllOf, + AnyOf, + OneOf, Xml, SchemaProperties, DiscriminatorMapping, diff --git a/packages/core/src/types/oas3_1.ts b/packages/core/src/types/oas3_1.ts index 2419c84613..79f0a924f5 100755 --- a/packages/core/src/types/oas3_1.ts +++ b/packages/core/src/types/oas3_1.ts @@ -140,9 +140,9 @@ const Schema: NodeType = { }; } }, - allOf: listOf('Schema'), - anyOf: listOf('Schema'), - oneOf: listOf('Schema'), + allOf: listOf('Schema', { name: 'AllOf' }), + anyOf: listOf('Schema', { name: 'AnyOf' }), + oneOf: listOf('Schema', { name: 'OneOf' }), not: 'Schema', if: 'Schema', then: 'Schema', diff --git a/packages/core/src/types/redocly-yaml.ts b/packages/core/src/types/redocly-yaml.ts index 5b6561dacc..01010f44f2 100644 --- a/packages/core/src/types/redocly-yaml.ts +++ b/packages/core/src/types/redocly-yaml.ts @@ -107,6 +107,7 @@ const builtInOAS3Rules = [ 'component-name-unique', 'no-empty-servers', 'no-example-value-and-externalValue', + 'no-illogical-composition-keywords', 'no-invalid-media-type-examples', 'no-server-example.com', 'no-server-trailing-slash', diff --git a/packages/core/src/visitors.ts b/packages/core/src/visitors.ts index 0d54e67bf4..2624817790 100644 --- a/packages/core/src/visitors.ts +++ b/packages/core/src/visitors.ts @@ -215,6 +215,9 @@ type Oas3FlatVisitor = { Response?: VisitFunctionOrObject>; Link?: VisitFunctionOrObject; Schema?: VisitFunctionOrObject; + AllOf?: VisitFunctionOrObject>; + AnyOf?: VisitFunctionOrObject>; + OneOf?: VisitFunctionOrObject>; Xml?: VisitFunctionOrObject; SchemaProperties?: VisitFunctionOrObject>; DiscriminatorMapping?: VisitFunctionOrObject>; diff --git a/tests/e2e/lint/no-invalid-media-type-examples-recursion/snapshot.txt b/tests/e2e/lint/no-invalid-media-type-examples-recursion/snapshot.txt index 15b939a25f..72c84bb2f6 100644 --- a/tests/e2e/lint/no-invalid-media-type-examples-recursion/snapshot.txt +++ b/tests/e2e/lint/no-invalid-media-type-examples-recursion/snapshot.txt @@ -1,4 +1,18 @@ -[1] openapi.json:29:28 at #/paths/~1test/get/responses/202/content/application~1json/example +[1] openapi.json:68:13 at #/components/schemas/PetWithProps/properties/b/oneOf + +`oneOf` should have at least two schemas. Use the schema directly instead. + +66 | }, +67 | "b": { +68 | "oneOf": [{ "$ref": "#/components/schemas/PetWithProps" }] + | ^^^^^^^ +69 | } +70 | } + +Warning was generated by the no-illogical-composition-keywords rule. + + +[2] openapi.json:29:28 at #/paths/~1test/get/responses/202/content/application~1json/example Example value must conform to the schema: must have required property 'c'. @@ -21,7 +35,7 @@ Warning was generated by the no-invalid-media-type-examples rule. Reference: https://redocly.com/docs/cli/rules/oas/no-invalid-media-type-examples -[2] openapi.json:30:24 at #/paths/~1test/get/responses/202/content/application~1json/example/a +[3] openapi.json:30:24 at #/paths/~1test/get/responses/202/content/application~1json/example/a Example value must conform to the schema: `a` property must have required property 'c'. @@ -39,10 +53,52 @@ Warning was generated by the no-invalid-media-type-examples rule. Reference: https://redocly.com/docs/cli/rules/oas/no-invalid-media-type-examples +[4] openapi.json:49:9 at #/components/schemas/Pet/oneOf + +`oneOf` should have at least two schemas. Use the schema directly instead. + +47 | "Pet": { +48 | "type": "object", +49 | "oneOf": [{ "$ref": "#/components/schemas/Dog" }] + | ^^^^^^^ +50 | }, +51 | "Dog": { + +Warning was generated by the no-illogical-composition-keywords rule. + + +[5] openapi.json:53:9 at #/components/schemas/Dog/oneOf + +`oneOf` should have at least two schemas. Use the schema directly instead. + +51 | "Dog": { +52 | "type": "object", +53 | "oneOf": [{ "$ref": "#/components/schemas/Pet" }] + | ^^^^^^^ +54 | }, +55 | "PetDirect": { + +Warning was generated by the no-illogical-composition-keywords rule. + + +[6] openapi.json:57:9 at #/components/schemas/PetDirect/oneOf + +`oneOf` should have at least two schemas. Use the schema directly instead. + +55 | "PetDirect": { +56 | "type": "object", +57 | "oneOf": [{ "$ref": "#/components/schemas/PetDirect" }] + | ^^^^^^^ +58 | }, +59 | "PetWithProps": { + +Warning was generated by the no-illogical-composition-keywords rule. + + validating openapi.json using lint rules for api 'main'... openapi.json: validated in ms Woohoo! Your API description is valid. 🎉 -You have 2 warnings. +You have 6 warnings. diff --git a/tests/e2e/lint/oas3.2/snapshot.txt b/tests/e2e/lint/oas3.2/snapshot.txt index eeebd82515..a7fa1a2d8c 100644 --- a/tests/e2e/lint/oas3.2/snapshot.txt +++ b/tests/e2e/lint/oas3.2/snapshot.txt @@ -303,7 +303,21 @@ Warning was generated by the operation-4xx-response rule. Reference: https://redocly.com/docs/cli/rules/oas/operation-4xx-response -[21] openapi.yaml:149:5 at #/components/schemas/MyResponseType +[21] openapi.yaml:150:7 at #/components/schemas/MyResponseType/oneOf + +Schemas in `oneOf` must be mutually exclusive. Found overlapping schemas: `#/components/schemas/Lizard` and `#/components/schemas/OtherPet`. Add `petType` to `required` in every schema; the `discriminator` cannot read a property a value may omit. + +148 | schemas: +149 | MyResponseType: +150 | oneOf: + | ^^^^^ +151 | - $ref: '#/components/schemas/Lizard' +152 | - $ref: '#/components/schemas/OtherPet' + +Warning was generated by the no-illogical-composition-keywords rule. + + +[22] openapi.yaml:149:5 at #/components/schemas/MyResponseType Component: "MyResponseType" is never used. @@ -319,7 +333,7 @@ Warning was generated by the no-unused-components rule. Reference: https://redocly.com/docs/cli/rules/oas/no-unused-components -[22] openapi.yaml:125:5 at #/components/securitySchemes/petstore_auth +[23] openapi.yaml:125:5 at #/components/securitySchemes/petstore_auth Security scheme: "petstore_auth" is never used. @@ -335,7 +349,7 @@ Warning was generated by the no-unused-components rule. Reference: https://redocly.com/docs/cli/rules/oas/no-unused-components -[23] openapi.yaml:136:5 at #/components/securitySchemes/api_key +[24] openapi.yaml:136:5 at #/components/securitySchemes/api_key Security scheme: "api_key" is never used. @@ -355,6 +369,6 @@ Reference: https://redocly.com/docs/cli/rules/oas/no-unused-components validating openapi.yaml using lint rules for api 'main'... openapi.yaml: validated in ms -❌ Validation failed with 10 errors and 13 warnings. +❌ Validation failed with 10 errors and 14 warnings. run `redocly lint --generate-ignore-file` to add all problems to the ignore file.