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

[jedelson-pagerduty]

fixes #1209

What/Why/How?

Details #1209 but essentially a schema like:

Unused:
  allOf:
    - type: object
      properties:
        one:
          type: string
    - type: object
      properties:
        two:
          type: string

will never be removed

Testing

Added a unit test for this case. Have also tested this change on a real-world document with this situation.

Check yourself

• Code is linted
• Tested with redoc/reference-docs/workflows
• All new/updated code is covered with tests

Security

• Security impact of change has been considered
• Code follows company security practices and guidelines

[changeset-bot]

🦋 Changeset detected

Latest commit: 9277248

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@redocly/openapi-core	Minor
@redocly/cli	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jedelson-pagerduty]

@tatomyr I've updated the code to deal with using discriminator from a referenced allOf. Still not 100% sure this is case that drew the original concern.

I should still add a test for the rule change but I'm less familiar with how rules are tested (vs. decorators) but wanted to push this up to see if this was what you were referring to.

[jedelson-pagerduty]

@tatomyr I've added the rule test. To summarize:

• If an "allOf" schema references some other schema and that schema has a discriminator the "allOf" schema is considered used (even though it may not actually be used... it is potentially used). no-unused-components will not create an issue and remove-unused-components will not remove it.
• If an "allOf" schema references some other schema and none of the referenced schemas have a discriminator, the "allOf" schema is considered used following the normal usage rules (i.e. is ref'd somewhere else).
• If an "allOf" schema doesn't reference other schemas, the "allOf" schema is considered used following the normal usage rules (i.e. is ref'd somewhere else).

I believe this addresses the original FIXME.

[jedelson-pagerduty]

Open questions:

• Should this be a minor change instead of a patch?
• Should I create two separate changeset files?

[tatomyr]

Should this be a minor change instead of a patch?
Should I create two separate changeset files?

Please use minor (replace the existing one).

[jedelson-pagerduty]

Please use minor (replace the existing one).

Done

[adamaltman]

@tatomyr I'm curious why this is a minor instead of a patch? I thought a bug fix would be a patch? Is this bugfix not considered backwards compatible perhaps? (if not, wouldn't it require a major revision?)

@jedelson-pagerduty overall great contribution 🎉 . Thank you so much for your contribution.

[tatomyr]

@adamaltman perhaps we decided this is more of an improvement rather than a fix. I don’t recall the exact reason though.
@IgorKarpiuk could you take a look and finalise this once you’re done with the current release?

[tatomyr]

Hi @jedelson-pagerduty! Sorry that the review took so long 😅

I believe your approach is correct. However, there are some corner cases when we are removing actually used components (which is worse than leaving unused ones). I found one. Do you see anything else? Maybe @lornajane has more to suggest?

[tatomyr]

I'm afraid this won't work correctly with a transient allOf-discriminator pair. Consider this example:

openapi: '3.0.0'

components: 
  schemas: 
    BaseVehicle: 
      type: object
      description: Vehicle
      discriminator:
        propertyName: powerSource
      properties:
        powerSource:
          description: How is the vehicle powered.
          type: string
          example: pedaling

    Vehicle:
      allOf: 
        - $ref: "#/components/schemas/BaseVehicle"
        - type: object 
          properties:
            vehicleType:
              description: The type of vehicle.
              type: string
              example: bicycle

    PedaledVehicle:
      allOf:
        - $ref: "#/components/schemas/Vehicle"
        - type: object
          description: Pedaled Vehicle
          properties:
            range:
              description: The 95th percentile range of a trip in kilometers.
              type: integer
              example: 100

The PedaledVehicle will be considered unused despite Vehicle resolves to BaseVehicle which actually contains the discriminator. I'm not sure this is a common use case but technically it looks like a valid one.

[jedelson-pagerduty]

@tatomyr Appreciate the feedback! And yes, I suppose that is valid in the general case.

I'm wondering if this is fruitless -- I am trying to apply a very strict definition of "used" that works for my use case (in this example, I would actually expect all those schemas to be removed) but wouldn't be generally applicable.

I can look at taking another run at this (although it might be a few days) but I am starting to think a different approach is necessary.

[tatomyr]

Suggested change

	schema.allOf && schema.allOf.some((v) => v.$ref && resolve(v).node?.discriminator)
	schema.allOf?.some((v) => v.$ref && resolve(v).node?.discriminator)

[tatomyr]

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

[tatomyr]

@jedelson-pagerduty do you have any updates on this?

FYI we've recently moved the remove-unused-components decorator, so that's why this MR got the conflicts 😅.