remove-unused-components skips schemas that have a top-level `allOf`

[jedelson-pagerduty]

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:

1. Copy the OpenAPI content above to a file.
2. Run redocly bundle <file.yaml> -o output.yaml --remove-unused-components
3. 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

[jedelson-pagerduty]

@slavikbez I think this came from your PR (#443). I'd be interested in hearing your feedback on this.

[tatomyr]

@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.

[jedelson-pagerduty]

@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.

[tatomyr]

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?

[RomanHotsiy]

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.

[jokull]

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