Repository: Redocly/redocly-cli
Pull request #1472 (2024-03-05 18:07:33)

fix: remove unused components transitively
## What/Why/How?

Removes components if their parent paths were removed. Loops until no more changes were made.

## Reference

Fixes https://github.com/Redocly/redocly-cli/issues/874

## Testing

Tests were updated to cover the new logic.

## Check yourself

- [ ] Code changed? - Tested with redoc/reference-docs/workflows (internal)
- [x] All new/updated code is covered with tests
- [ ] New package installed? - Tested in different environments (browser/node)

## Security

- [x] Security impact of change has been considered
- [x] Code follows company security practices and guidelines


changeset-bot[bot]: ###  ⚠️  No Changeset found

Latest commit: 8987fbf7fa5e9d5d2f180b69480843bfe87c3cdb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. **If these changes should result in a version bump, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/fb55/redocly-cli/new/main?filename=.changeset/big-toys-sniff.md&value=---%0A%22%40redocly%2Fopenapi-core%22%3A%20patch%0A---%0A%0ARemove%20unused%20components%20transitively%0A)


fb55: @tatomyr Please have a look!
tatomyr: @fb55 sure thing, will take a look! 
fb55: Thanks for the review, addressed all comments!

> And one more thing. Whatever changes here should also apply to the corresponding rule [no-unused-components](https://github.com/Redocly/redocly-cli/blob/main/packages/core/src/rules/oas3/no-unused-components.ts).

Do you mean introducing the additional data structures for tracking where a component is used?
tatomyr: > Do you mean introducing the additional data structures for tracking where a component is used?

Actually, at a second glance, I'm not sure we should apply the same logic in the rule itself... We're not removing transitive components but rather checking the current state. 

Let me have another look.
fb55: > WIth the latest changes it doesn't seem to remove transitive references anymore 😅

Tests are passing, and the codebase I am looking at (~1000 auto-generated components with ~20% unused) is not showing any unused component errors after processing. But the reported number of removed components has gone down from ~220 to ~160 — not quite sure about what happened here.
tatomyr: > Tests are passing

Indeed they are. Sorry, mixed up with another errors 🙂. 
fb55: @tatomyr Anything I can help with to make this move forward?
fb55: Realised there was a comment I had not addressed, just did so!

Patch:
diff --git a/.changeset/ten-fireants-reflect.md b/.changeset/ten-fireants-reflect.md
new file mode 100644
index 00000000..3948548e
--- /dev/null
+++ b/.changeset/ten-fireants-reflect.md
@@ -0,0 +1,5 @@
+---
+"@redocly/openapi-core": patch
+---
+
+Process remove-unused-components rule transitively; components are now removed if they were previously referenced by a removed component.
diff --git a/packages/core/src/decorators/oas2/remove-unused-components.ts b/packages/core/src/decorators/oas2/remove-unused-components.ts
index 62975f46..0cd3a9f4 100644
--- a/packages/core/src/decorators/oas2/remove-unused-components.ts
+++ b/packages/core/src/decorators/oas2/remove-unused-components.ts
@@ -2,12 +2,12 @@ import { Location } from '../../ref-utils';
 import { isEmptyObject } from '../../utils';
 
 import type { Oas2Decorator } from '../../visitors';
-import type { Oas2Components } from '../../typings/swagger';
+import type { Oas2Components, Oas2Definition } from '../../typings/swagger';
 
 export const RemoveUnusedComponents: Oas2Decorator = () => {
   const components = new Map<
     string,
-    { used: boolean; componentType?: keyof Oas2Components; name: string }
+    { usedIn: Location[]; componentType?: keyof Oas2Components; name: string }
   >();
 
   function registerComponent(
@@ -16,15 +16,46 @@ export const RemoveUnusedComponents: Oas2Decorator = () => {
     name: string
   ): void {
     components.set(location.absolutePointer, {
-      used: components.get(location.absolutePointer)?.used || false,
+      usedIn: components.get(location.absolutePointer)?.usedIn ?? [],
       componentType,
       name,
     });
   }
 
+  function removeUnusedComponents(root: Oas2Definition, removedPaths: string[]): number {
+    const removedLengthStart = removedPaths.length;
+
+    for (const [path, { usedIn, name, componentType }] of components) {
+      const used = usedIn.some(
+        (location) =>
+          !removedPaths.some(
+            (removed) =>
+              // Check if the current location's absolute pointer starts with the 'removed' path
+              // and either its length matches exactly with 'removed' or the character after the 'removed' path is a '/'
+              location.absolutePointer.startsWith(removed) &&
+              (location.absolutePointer.length === removed.length ||
+                location.absolutePointer[removed.length] === '/')
+          )
+      );
+      if (!used && componentType) {
+        removedPaths.push(path);
+        delete root[componentType]![name];
+        components.delete(path);
+
+        if (isEmptyObject(root[componentType])) {
+          delete root[componentType];
+        }
+      }
+    }
+
+    return removedPaths.length > removedLengthStart
+      ? removeUnusedComponents(root, removedPaths)
+      : removedPaths.length;
+  }
+
   return {
     ref: {
-      leave(ref, { type, resolve, key }) {
+      leave(ref, { location, type, resolve, key }) {
         if (['Schema', 'Parameter', 'Response', 'SecurityScheme'].includes(type.name)) {
           const resolvedRef = resolve(ref);
           if (!resolvedRef.location) return;
@@ -33,32 +64,23 @@ export const RemoveUnusedComponents: Oas2Decorator = () => {
           const componentLevelLocalPointer = localPointer.split('/').slice(0, 3).join('/');
           const pointer = `${fileLocation}#${componentLevelLocalPointer}`;
 
-          components.set(pointer, {
-            used: true,
-            name: key.toString(),
-          });
+          const registered = components.get(pointer);
+
+          if (registered) {
+            registered.usedIn.push(location);
+          } else {
+            components.set(pointer, {
+              usedIn: [location],
+              name: key.toString(),
+            });
+          }
         }
       },
     },
     Root: {
       leave(root, ctx) {
         const data = ctx.getVisitorData() as { removedCount: number };
-        data.removedCount = 0;
-
-        const rootComponents = new Set<keyof Oas2Components>();
-        components.forEach((usageInfo) => {
-          const { used, name, componentType } = usageInfo;
-          if (!used && componentType) {
-            rootComponents.add(componentType);
-            delete root[componentType]![name];
-            data.removedCount++;
-          }
-        });
-        for (const component of rootComponents) {
-          if (isEmptyObject(root[component])) {
-            delete root[component];
-          }
-        }
+        data.removedCount = removeUnusedComponents(root, []);
       },
     },
     NamedSchemas: {
diff --git a/packages/core/src/decorators/oas3/remove-unused-components.ts b/packages/core/src/decorators/oas3/remove-unused-components.ts
index f71e1b67..06f6d1f9 100644
--- a/packages/core/src/decorators/oas3/remove-unused-components.ts
+++ b/packages/core/src/decorators/oas3/remove-unused-components.ts
@@ -2,12 +2,12 @@ import { Location } from '../../ref-utils';
 import { isEmptyObject } from '../../utils';
 
 import type { Oas3Decorator } from '../../visitors';
-import type { Oas3Components } from '../../typings/openapi';
+import type { Oas3Components, Oas3Definition } from '../../typings/openapi';
 
 export const RemoveUnusedComponents: Oas3Decorator = () => {
   const components = new Map<
     string,
-    { used: boolean; componentType?: keyof Oas3Components; name: string }
+    { usedIn: Location[]; componentType?: keyof Oas3Components; name: string }
   >();
 
   function registerComponent(
@@ -16,15 +16,45 @@ export const RemoveUnusedComponents: Oas3Decorator = () => {
     name: string
   ): void {
     components.set(location.absolutePointer, {
-      used: components.get(location.absolutePointer)?.used || false,
+      usedIn: components.get(location.absolutePointer)?.usedIn ?? [],
       componentType,
       name,
     });
   }
 
+  function removeUnusedComponents(root: Oas3Definition, removedPaths: string[]): number {
+    const removedLengthStart = removedPaths.length;
+
+    for (const [path, { usedIn, name, componentType }] of components) {
+      const used = usedIn.some(
+        (location) =>
+          !removedPaths.some(
+            (removed) =>
+              location.absolutePointer.startsWith(removed) &&
+              (location.absolutePointer.length === removed.length ||
+                location.absolutePointer[removed.length] === '/')
+          )
+      );
+
+      if (!used && componentType && root.components) {
+        removedPaths.push(path);
+        const componentChild = root.components[componentType];
+        delete componentChild![name];
+        components.delete(path);
+        if (isEmptyObject(componentChild)) {
+          delete root.components[componentType];
+        }
+      }
+    }
+
+    return removedPaths.length > removedLengthStart
+      ? removeUnusedComponents(root, removedPaths)
+      : removedPaths.length;
+  }
+
   return {
     ref: {
-      leave(ref, { type, resolve, key }) {
+      leave(ref, { location, type, resolve, key }) {
         if (
           ['Schema', 'Header', 'Parameter', 'Response', 'Example', 'RequestBody'].includes(
             type.name
@@ -37,29 +67,24 @@ export const RemoveUnusedComponents: Oas3Decorator = () => {
           const componentLevelLocalPointer = localPointer.split('/').slice(0, 4).join('/');
           const pointer = `${fileLocation}#${componentLevelLocalPointer}`;
 
-          components.set(pointer, {
-            used: true,
-            name: key.toString(),
-          });
+          const registered = components.get(pointer);
+
+          if (registered) {
+            registered.usedIn.push(location);
+          } else {
+            components.set(pointer, {
+              usedIn: [location],
+              name: key.toString(),
+            });
+          }
         }
       },
     },
     Root: {
       leave(root, ctx) {
         const data = ctx.getVisitorData() as { removedCount: number };
-        data.removedCount = 0;
+        data.removedCount = removeUnusedComponents(root, []);
 
-        components.forEach((usageInfo) => {
-          const { used, componentType, name } = usageInfo;
-          if (!used && componentType && root.components) {
-            const componentChild = root.components[componentType];
-            delete componentChild![name];
-            data.removedCount++;
-            if (isEmptyObject(componentChild)) {
-              delete root.components[componentType];
-            }
-          }
-        });
         if (isEmptyObject(root.components)) {
           delete root.components;
         }


Test patch:
diff --git a/packages/core/src/decorators/oas2/__tests__/remove-unused-components.test.ts b/packages/core/src/decorators/oas2/__tests__/remove-unused-components.test.ts
index 9b0febbf..e5af0836 100644
--- a/packages/core/src/decorators/oas2/__tests__/remove-unused-components.test.ts
+++ b/packages/core/src/decorators/oas2/__tests__/remove-unused-components.test.ts
@@ -1,5 +1,5 @@
 import { outdent } from 'outdent';
-import { parseYamlToDocument, replaceSourceWithRef, makeConfig } from '../../../../__tests__/utils';
+import { parseYamlToDocument, makeConfig } from '../../../../__tests__/utils';
 import { bundleDocument } from '../../../bundle';
 import { BaseResolver } from '../../../resolve';
 
@@ -152,4 +152,77 @@ describe('oas2 remove-unused-components', () => {
       },
     });
   });
+
+  it('should remove transitively unused components', async () => {
+    const document = parseYamlToDocument(
+      outdent`
+        swagger: '2.0'
+        paths:
+          /pets:
+            get:
+              produces:
+                - application/json
+              parameters: []
+              responses:
+                '200':
+                  schema:
+                    $ref: '#/definitions/Used'
+              operationId: listPets
+              summary: List all pets
+        definitions:
+          Unused:
+            enum:
+              - 1
+              - 2
+            type: integer
+          UnusedTransitive:
+            type: object
+            properties:
+              link:
+                $ref: '#/definitions/Unused'
+          Used:
+            properties:
+              link:
+                type: string
+            type: object
+        `,
+      'foobar.yaml'
+    );
+
+    const results = await bundleDocument({
+      externalRefResolver: new BaseResolver(),
+      document,
+      config: await makeConfig({}),
+      removeUnusedComponents: true,
+    });
+
+    expect(results.bundle.parsed).toEqual({
+      swagger: '2.0',
+      definitions: {
+        Used: {
+          properties: {
+            link: { type: 'string' },
+          },
+          type: 'object',
+        },
+      },
+      paths: {
+        '/pets': {
+          get: {
+            produces: ['application/json'],
+            parameters: [],
+            summary: 'List all pets',
+            operationId: 'listPets',
+            responses: {
+              '200': {
+                schema: {
+                  $ref: '#/definitions/Used',
+                },
+              },
+            },
+          },
+        },
+      },
+    });
+  });
 });
diff --git a/packages/core/src/decorators/oas3/__tests__/remove-unused-components.test.ts b/packages/core/src/decorators/oas3/__tests__/remove-unused-components.test.ts
index a708be88..a6159c0a 100644
--- a/packages/core/src/decorators/oas3/__tests__/remove-unused-components.test.ts
+++ b/packages/core/src/decorators/oas3/__tests__/remove-unused-components.test.ts
@@ -168,4 +168,146 @@ describe('oas3 remove-unused-components', () => {
       },
     });
   });
+
+  it('should remove transitively 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
+          schemas:
+            Unused:
+              type: integer
+              enum:
+                - 1
+                - 2
+            Transitive:
+              type: object
+              properties:
+                link:
+                  $ref: '#/components/schemas/Unused'
+        `,
+      'foobar.yaml'
+    );
+
+    const results = await bundleDocument({
+      externalRefResolver: new BaseResolver(),
+      document,
+      config: await makeConfig({}),
+      removeUnusedComponents: true,
+    });
+
+    expect(results.bundle.parsed).toEqual({
+      openapi: '3.0.0',
+      paths: {
+        '/pets': {
+          get: {
+            summary: 'List all pets',
+            operationId: 'listPets',
+            parameters: [
+              {
+                $ref: '#/components/parameters/used',
+              },
+            ],
+          },
+        },
+      },
+      components: {
+        parameters: {
+          used: {
+            name: 'used',
+          },
+        },
+      },
+    });
+  });
+
+  it('should remove transitively unused components with colliding paths', async () => {
+    const document = parseYamlToDocument(
+      outdent`
+        openapi: "3.0.0"
+        paths:
+          /pets:
+            get:
+              responses: 
+                200:
+                  content: 
+                    application/json: 
+                      schema: 
+                        $ref: "#/components/schemas/Transitive2"
+        components:
+          schemas:
+            Unused: # <-- this will be removed correctly
+              type: integer
+            Transitive: # <-- this will be removed correctly
+              type: object
+              properties:
+                link:
+                  $ref: '#/components/schemas/Unused'
+            Used:
+              type: integer
+            Transitive2:
+              type: object
+              properties:
+                link:
+                  $ref: '#/components/schemas/Used'
+        `,
+
+      'foobar.yaml'
+    );
+
+    const results = await bundleDocument({
+      externalRefResolver: new BaseResolver(),
+      document,
+      config: await makeConfig({}),
+      removeUnusedComponents: true,
+    });
+
+    expect(results.bundle.parsed).toEqual({
+      openapi: '3.0.0',
+      paths: {
+        '/pets': {
+          get: {
+            responses: {
+              200: {
+                content: {
+                  'application/json': {
+                    schema: {
+                      $ref: '#/components/schemas/Transitive2',
+                    },
+                  },
+                },
+              },
+            },
+          },
+        },
+      },
+      components: {
+        schemas: {
+          Transitive2: {
+            type: 'object',
+            properties: {
+              link: {
+                $ref: '#/components/schemas/Used',
+              },
+            },
+          },
+          Used: {
+            type: 'integer',
+          },
+        },
+      },
+    });
+  });
 });