Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove unused components was skipping schemas with a top-level allOf #1220

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/light-games-clap.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@redocly/openapi-core': minor
'@redocly/cli': minor
---

Fixed an issue where remove-unused-components would not remove schemas which had a top-level allOf and improved detection of unused schemas in no-unused-components rule.
tatomyr marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rephrase the description so it emphasises the improvement rather than the fix?

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { outdent } from 'outdent';
import { parseYamlToDocument, replaceSourceWithRef, makeConfig } from '../../../../__tests__/utils';
import { cloneDeep } from 'lodash';
import { parseYamlToDocument, makeConfig } from '../../../../__tests__/utils';
import { bundleDocument } from '../../../bundle';
import { BaseResolver } from '../../../resolve';

Expand Down Expand Up @@ -152,4 +153,68 @@ describe('oas2 remove-unused-components', () => {
},
});
});

it('should remove unused components even if they have a root allOf', async () => {
const document = parseYamlToDocument(
outdent`
swagger: '2.0'
paths:
/pets:
get:
produces:
- application/json
parameters: []
responses:
'200':
schema:
items:
$ref: '#/definitions/Pet'
type: array
operationId: listPets
summary: List all pets
definitions:
Cat:
allOf:
- $ref: '#/definitions/Pet'
- properties:
name:
type: string
type: object
Dog:
allOf:
- $ref: '#/definitions/Pet'
- properties:
bark:
type: string
type: object
Lizard:
allOf:
- $ref: '#/definitions/Pet'
- properties:
lovesRocks:
type: boolean
type: object
Pet:
discriminator: petType
properties:
petType:
type: string
required:
- petType
type: object
`,
'foobar.yaml'
);

const orig = cloneDeep(document.parsed);

const results = await bundleDocument({
externalRefResolver: new BaseResolver(),
document,
config: await makeConfig({}),
removeUnusedComponents: true,
});

expect(results.bundle.parsed).toEqual(orig);
});
});
16 changes: 10 additions & 6 deletions packages/core/src/rules/oas2/remove-unused-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ export const RemoveUnusedComponents: Oas2Rule = () => {
function registerComponent(
location: Location,
componentType: keyof Oas2Components,
name: string
name: string,
used = false
): void {
components.set(location.absolutePointer, {
used: components.get(location.absolutePointer)?.used || false,
used: components.get(location.absolutePointer)?.used || used,
componentType,
name,
});
Expand Down Expand Up @@ -61,10 +62,13 @@ export const RemoveUnusedComponents: Oas2Rule = () => {
},
},
NamedSchemas: {
Schema(schema, { location, key }) {
if (!schema.allOf) {
registerComponent(location, 'definitions', key.toString());
}
Schema(schema, { location, key, resolve }) {
registerComponent(
location,
'definitions',
key.toString(),
schema.allOf && schema.allOf.some((v) => v.$ref && resolve(v).node?.discriminator)
);
},
},
NamedParameters: {
Expand Down
148 changes: 148 additions & 0 deletions packages/core/src/rules/oas3/__tests__/no-unused-components.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,152 @@ describe('Oas3 no-unused-components', () => {
]
`);
});

it('should properly handle allOf when reporting unused components', async () => {
const document = parseYamlToDocument(
outdent`
openapi: "3.0.0"
paths:
/pets:
get:
summary: List all pets
operationId: listPets
parameters:
- $ref: '#/components/parameters/used'
components:
parameters:
used:
name: used
unused:
name: unused
responses:
unused: {}
examples:
unused: {}
requestBodies:
unused: {}
headers:
unused: {}
schemas:
Unused:
allOf:
- type: object
properties:
one:
type: string
- type: object
properties:
two:
type: string
# this is referenced so is considered used
Pet:
type: object
properties:
petType:
type: string
discriminator:
propertyName: petType
required:
- petType
# this is potentially used
Cat:
allOf:
- $ref: '#/components/schemas/Pet'
- type: object
properties:
name:
type: string
`,
'foobar.yaml'
);

const results = await lintDocument({
externalRefResolver: new BaseResolver(),
document,
config: await makeConfig({ 'no-unused-components': 'error' }),
});

expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`
Array [
Object {
"location": Array [
Object {
"pointer": "#/components/parameters/unused",
"reportOnKey": true,
"source": "foobar.yaml",
},
],
"message": "Component: \\"unused\\" is never used.",
"ruleId": "no-unused-components",
"severity": "error",
"suggest": Array [],
},
Object {
"location": Array [
Object {
"pointer": "#/components/schemas/Unused",
"reportOnKey": true,
"source": "foobar.yaml",
},
],
"message": "Component: \\"Unused\\" is never used.",
"ruleId": "no-unused-components",
"severity": "error",
"suggest": Array [],
},
Object {
"location": Array [
Object {
"pointer": "#/components/responses/unused",
"reportOnKey": true,
"source": "foobar.yaml",
},
],
"message": "Component: \\"unused\\" is never used.",
"ruleId": "no-unused-components",
"severity": "error",
"suggest": Array [],
},
Object {
"location": Array [
Object {
"pointer": "#/components/examples/unused",
"reportOnKey": true,
"source": "foobar.yaml",
},
],
"message": "Component: \\"unused\\" is never used.",
"ruleId": "no-unused-components",
"severity": "error",
"suggest": Array [],
},
Object {
"location": Array [
Object {
"pointer": "#/components/requestBodies/unused",
"reportOnKey": true,
"source": "foobar.yaml",
},
],
"message": "Component: \\"unused\\" is never used.",
"ruleId": "no-unused-components",
"severity": "error",
"suggest": Array [],
},
Object {
"location": Array [
Object {
"pointer": "#/components/headers/unused",
"reportOnKey": true,
"source": "foobar.yaml",
},
],
"message": "Component: \\"unused\\" is never used.",
"ruleId": "no-unused-components",
"severity": "error",
"suggest": Array [],
},
]
`);
});
});