Fix infinite looping bug.

Author: tjprescott

compute-suppressions.yaml

@@ -1,4 +1,4 @@
-# - path: "definitions/OperationListResult/type"
-#   reason: "Common type"
+- path: "definitions/OperationListResult/type"
+  reason: "Common type"
 - path: "definitions/Operation/properties/display/type"
   reason: "Common type"

src/definitions.ts

@@ -8,6 +8,7 @@ import {
   getRegistryName,
 } from "./util.js";
 import path from "path";
+import { DiffClient } from "./diff-client.js";
 
 /** The registry to look up the name within. */
 export enum RegistryKind {
@@ -87,10 +88,10 @@ export class DefinitionRegistry {
   private providedPaths = new Set<string>();
   private referenceStack: string[] = [];
   private referenceMap = new Map<string, Set<string>>();
-  private currentPath: string | undefined;
-  private args: any;
+  private currentPath: string[];
+  private client: DiffClient;
 
-  constructor(map: Map<string, OpenAPIV2.Document>, args: any) {
+  constructor(map: Map<string, OpenAPIV2.Document>, client: DiffClient) {
     this.data = {
       definitions: new CollectionRegistry(
         map,
@@ -116,9 +117,9 @@ export class DefinitionRegistry {
     for (const key of map.keys()) {
       this.providedPaths.add(path.normalize(key));
     }
-    this.currentPath;
+    this.currentPath = [];
+    this.client = client;
     this.#gatherDefinitions(map);
-    this.args = args;
     this.#expandReferences();
   }
 
@@ -131,7 +132,6 @@ export class DefinitionRegistry {
       if (!refResult) {
         return item;
       }
-      // FIXME: Need to proprogate suppressions here too
       let match = this.get(refResult);
       if (match) {
         if (this.referenceStack.includes(refResult.name)) {
@@ -161,7 +161,9 @@ export class DefinitionRegistry {
       this.#expandDerivedClasses(item);
       this.#expandAllOf(item);
       for (const [propName, propValue] of toSorted(Object.entries(item))) {
+        this.currentPath.push(propName);
         expanded[propName] = this.#expand(propValue);
+        this.currentPath.pop();
       }
       return expanded;
     }
@@ -218,7 +220,10 @@ export class DefinitionRegistry {
         const itemVal = item[key];
         switch (key) {
           case "required":
-            base[key] = (baseVal ?? []).concat(itemVal ?? []);
+            const mergedRequired = new Set(
+              (baseVal ?? []).concat(itemVal ?? [])
+            );
+            base[key] = [...mergedRequired];
             break;
           case "properties":
             base[key] = { ...(baseVal ?? {}), ...(itemVal ?? {}) };
@@ -296,10 +301,11 @@ export class DefinitionRegistry {
   #expandReferencesForCollection(collection: CollectionRegistry) {
     this.referenceStack = [];
     for (const [filepath, values] of collection.data.entries()) {
-      this.currentPath = filepath;
       for (const [key, value] of values.entries()) {
+        this.currentPath.push(key);
         let expanded = this.#expand(value, key, filepath);
         collection.data.get(filepath)!.set(key, expanded);
+        this.currentPath.pop();
       }
     }
     // replace $derivedClasses with $anyOf that contains the expansions of the derived classes
@@ -312,10 +318,18 @@ export class DefinitionRegistry {
   }
 
   #expandReferences() {
+    this.currentPath.push("definitions");
     this.#expandReferencesForCollection(this.data.definitions);
+    this.currentPath.pop();
+    this.currentPath.push("parameters");
     this.#expandReferencesForCollection(this.data.parameters);
+    this.currentPath.pop();
+    this.currentPath.push("responses");
     this.#expandReferencesForCollection(this.data.responses);
+    this.currentPath.pop();
+    this.currentPath.push("securityDefinitions");
     this.#expandReferencesForCollection(this.data.securityDefinitions);
+    this.currentPath.pop();
   }
 
   /**

src/diff-client.ts

@@ -13,7 +13,6 @@ import { RegistryKind } from "./definitions.js";
 import * as fs from "fs";
 import path from "path";
 import { SuppressionRegistry } from "./suppression.js";
-import { HtmlDiffClient } from "./html-diff.js";
 
 export interface DiffClientConfig {
   lhs: string | string[];
@@ -53,7 +52,6 @@ export class DiffClient {
     const lhsRoot = client.args["lhs-root"]
       ? path.resolve(client.args["lhs-root"])
       : undefined;
-
     let rhsRoot = client.args["rhs-root"]
       ? path.resolve(client.args["rhs-root"])
       : undefined;
@@ -65,13 +63,14 @@ export class DiffClient {
         rhsRoot = tempRhsRoot;
       }
     }
-    const lhsParser = await SwaggerParser.create(lhs, lhsRoot, client);
-    const rhsParser = await SwaggerParser.create(rhs, rhsRoot, client);
+    // suppression registry must be created before creating parsers
     if (client.args["suppressions"]) {
       client.suppressions = new SuppressionRegistry(
         client.args["suppressions"]
       );
     }
+    const lhsParser = await SwaggerParser.create(lhs, lhsRoot, client);
+    const rhsParser = await SwaggerParser.create(rhs, rhsRoot, client);
     client.lhsParser = lhsParser;
     client.rhsParser = rhsParser;
     return client;

src/suppression.ts

@@ -39,19 +39,15 @@ export class SuppressionRegistry {
   has(path: string | undefined): boolean {
     if (!path) return false;
     path = path.trim().toLowerCase();
-    const value = this.data.has(path);
-    return value;
+    return this.data.has(path);
   }
 
   propagateSuppression(ref: ReferenceMetadata, basePath: string[] | undefined) {
     if (!basePath) throw new Error("basePath is undefined");
     const base = basePath.join("/");
     const target = `${getRegistryName(ref.registry)}/${ref.name}`.toLowerCase();
-    if (target == "definitions/operation") {
-      let test = "best";
-    }
     for (const item of this.data) {
-      if (item.startsWith(target)) {
+      if (item === target || item.startsWith(`${target}/`)) {
         // create a new string suppression propagating the suppression onto the base path
         const newItem = item.replace(target, base);
         this.add(newItem);

test/rest-api-diff.test.ts

@@ -310,14 +310,21 @@ it("should propagate suppressions that are expanded while collecting definitions
     args: {
       suppressions: "test/files/suppressions2.yaml",
     },
-    rules: [],
+    rules: getApplicableRules({}),
   };
   const client = await TestableDiffClient.create(config);
   client.parse();
   client.processDiff();
-  expect(client.diffResults?.flaggedViolations.length).toBe(0);
-  expect(client.diffResults?.noViolations.length).toBe(0);
+  client.buildOutput();
+  const [lhsParser, rhsParser] = client.getParsers();
+  expect(lhsParser.getUnresolvedReferences().length).toBe(0);
+  expect(lhsParser.getUnreferencedTotal()).toBe(0);
+  expect(rhsParser.getUnresolvedReferences().length).toBe(0);
+  expect(rhsParser.getUnreferencedTotal()).toBe(0);
   expect(client.diffResults?.assumedViolations.length).toBe(0);
+  expect(client.diffResults?.flaggedViolations.length).toBe(0);
+  expect(client.diffResults?.suppressedViolations.length).toBe(2);
+  expect(client.diffResults?.noViolations.length).toBe(2);
 });
 
 it("should sort arrays of strings for stable comparison", async () => {