redocly-cli
redocly-cli copied to clipboard
remove-unused-components skips schemas that have a top-level `allOf`
Describe the bug
Given an OpenAPI document that contains something along these lines:
openapi: "3.0.0"
paths:
/pets:
get:
summary: List all pets
operationId: listPets
responses:
'200':
content:
application/json:
schema:
$ref: '#/components/schemas/Used'
components:
schemas:
Unused:
allOf:
- type: object
properties:
one:
type: string
- type: object
properties:
two:
type: string
Used:
type: object
properties:
link:
type: string
child:
$ref: '#/components/schemas/Two'
It would be expected that bundle --remove-unused-components would remove the Unused schema since it isn't used.
However, the current behavior is that after running bundle with --removed-unused-components, Unused is not removed. This appears to be intentional (see https://github.com/Redocly/redocly-cli/blob/cfa31330b04f22e7b855143b2eef0d040b0873fa/packages/core/src/rules/oas3/remove-unused-components.ts#L64) but I have not found any reference to why it was intentional.
To Reproduce Steps to reproduce the behavior:
- Copy the OpenAPI content above to a file.
- Run redocly bundle <file.yaml> -o output.yaml --remove-unused-components
- Check the output.yaml file.
Expected behavior
An unused component gets removed regardless of whether it has an allOf
or not.
OpenAPI definition
Have only tested with 3.0 but the same if
statement is present in the oas2
rule.
Redocly Version(s)
1.0.0 and main.
Node.js
Version(s)
16.17.0, 16.18.0, and 18.16.0
@slavikbez I think this came from your PR (#443). I'd be interested in hearing your feedback on this.
@jedelson-pagerduty I believe those changes mirror this rule: https://github.com/Redocly/redocly-cli/blob/572980cb33e3ff399e035246e8aa2c24e54d9617/packages/core/src/rules/oas3/no-unused-components.ts#L43 Looks like this is a workaround to detect discriminators.
@tatomyr thanks for that pointer, although that leaves me with more questions :) But those probably aren't important.
~Is there an example of a false-positive (i.e. an allOf
schema which triggers the no-unused-components rule when used)? I'd be happy to try to fix that along with changing remove-unused-components
.~
~The one slightly relevant case I can think of with discriminators is where there is a schema referenced in the discriminator but not in the allOf
(or, more likely IMO, oneOf
), e.g.~
Pet:
oneOf:
- $ref: '#/components/schemas/Cat'
- $ref: '#/components/schemas/Dog'
discriminator:
propertyName: petType
mapping:
dog: '#/components/schemas/Dog'
cat: '#/components/schemas/Cat'
lizard: '#/components/schemas/Lizard'
~In this case, Lizard
could be considered unused (assuming nothing else refers to it). But I would suggest that this is (a) arguably correct and (b) something that a different rule should be catching~
I think the problematic scenario is something like this:
Pet:
type: object
required:
- petType
properties:
petType:
type: string
discriminator:
propertyName: petType
Cat:
allOf:
- $ref: '#/components/schemas/Pet'
- type: object
properties:
name:
type: string
Dog:
allOf:
- $ref: '#/components/schemas/Pet'
- type: object
properties:
bark:
type: string
Lizard:
allOf:
- $ref: '#/components/schemas/Pet'
- type: object
properties:
lovesRocks:
type: boolean
Which makes more sense as a problematic case than what I was originally thinking. I'll update my PR to address this.
I haven't found any related tests that can shed some light on what was the case for this 😄. @RomanHotsiy could you maybe help with an example of a false positive?
Yes, as @jedelson-pagerduty noted in the updated comment, the problem is related to implicit discriminator mapping.
Someone can point to Pet
only and the nested schemas are supposed to be kept too. This requires a careful implementation. There are many edge-cases to this.
I think I'm hitting this issue with the the Cloudflare schema. Try tree shaking this schema for example: https://gist.github.com/jokull/2ccf3a492fd6fe35e2b7cd07704f5863