Merge pull request #47 from tjprescott/suppressions

Add Supression Mechanism

Author: tjprescott

CHANGELOG.md

@@ -1,5 +1,9 @@
 # @azure-tools/rest-api-diff
 
+## 0.2.1 (TBD)
+
+- Added `--suppressions` option to point to a filing containing point suppressions of violations.
+
 ## 0.2.0 (2025-03-11)
 
 - Fixed issue where relative references would sometimes be resolved incorrectly. `--lhs-root`

README.md

@@ -61,6 +61,7 @@ references as if it had been generated in the "correct" folder. This allows the
   when compiling TypeSpec files. If omitted, the tool will attempt to resolve the references without it.
 - `--rhs-root`: The root path for the right-hand side Swagger files. This is used to resolve references
   when compiling TypeSpec files. If omitted, the tool will attempt to resolve the references without it.
+- `--suppressions`: Path to a YAML file containing suppressions.
 
 ### .env File
 
@@ -141,3 +142,14 @@ rule can make one of three determinations:
 - `RuleResult.ContinueProcessing`: the logic of the rule doesn't apply to this diff and thus the tool should continue processing rules.
 
 When processing a diff item against the rules, if `NoViolation` is returned by a rule, it immediately suspends processing additional rules. If `FlaggedViolation` is returned, rules will continue to be processed in case another rule marks it as `NoViolation`. If all rules are run and no determination is made, then the diff is assumed to affect the API surface area and is tracked as an assumed violation. It will be reported by the tool and will appear in a visual diff. When violations are grouped, these violations will appear in the `UNGROUPED` group.
+
+## Suppressions
+
+It is possible that there will be differences between Swaggers that do matter but must be accepted. For this, you can point to a suppression file using the `--suppressions` option. This file should be a YAML file with the following schema:
+
+```yaml
+- path: "path/to/target/"
+  reason: "This is a reason why this path is suppressed."
+```
+
+You can target paths directly from the `diff.json` file assuming `--flatten-paths` is used; however, often violations originate in definitions or parameters, and the way `rest-api-diff` expands these references to create the transformation, the result is that these diffs will be multiplied. In this case, you can use the path to the element in question from the Swagger and it will automatically resolve all diffs that might be a result of reference expansion. Because `rest-api-diff` compares these as Swagger, you cannot use a TypeSpec path even if you are targeting TypeSpec. You must use the Swagger path associated with the TypeSpec-generated Swagger.

compute-suppressions.yaml

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

package.json

@@ -34,6 +34,7 @@
     "@azure-tools/typespec-azure-resource-manager": ">=0.44.0, <1.0.0",
     "@azure/avocado": "^0.9.1",
     "@types/deep-diff": "^1.0.5",
+    "@types/diff": "^7.0.0",
     "@types/node": "^20.14.7",
     "@types/yargs": "^17.0.32",
     "@typespec/compiler": ">=0.57.0, <1.0.0",
@@ -59,7 +60,8 @@
     "arg": "^4.1.3",
     "create-require": "^1.1.1",
     "deep-diff": "^1.0.2",
-    "diff": "^4.0.2",
+    "diff": "^7.0.0",
+    "diff2html": "^3.4.0",
     "dotenv": "^16.4.5",
     "make-error": "^1.3.6",
     "undici-types": "^5.26.5",

pnpm-lock.yaml

@@ -0,0 +1,28 @@
+from collections import Counter
+import json
+
+# open output/diff.json
+with open('output/diff.json') as f:
+    diff = json.load(f)
+
+    # gather the items into a flat list
+    items = []
+    if isinstance(diff, list):
+        items = diff
+    else:
+        for value in diff.values():
+            items.extend(value["items"])
+
+    keys = []
+    for item in items:
+        diff_item = item['diff']
+        kind = diff_item['kind']
+        if kind == 'A':
+            index = diff_item['index']
+            keys.append(f'{diff_item["path"]}/{index}')
+        else:
+            keys.append(diff_item['path'])
+
+    counter = Counter(keys)
+    assert all(val == 1 for val in counter.values())
+    print(f'{len(counter)} unique paths')

scripts/test-paths.py

@@ -5,8 +5,10 @@ import {
   parseReference,
   ReferenceMetadata,
   toSorted,
+  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 {
@@ -31,7 +33,7 @@ class CollectionRegistry {
       if (subdata !== undefined) {
         for (const [name, _] of toSorted(Object.entries(subdata))) {
           const resolvedPath = getResolvedPath(filepath).replace(/\\/g, "/");
-          const pathKey = `${resolvedPath}#/${this.getRegistryName()}/${name}`;
+          const pathKey = `${resolvedPath}#/${getRegistryName(this.kind)}/${name}`;
           // we don't care about unreferenced common-types
           if (!resolvedPath.includes("common-types")) {
             this.unreferenced.add(pathKey);
@@ -41,19 +43,6 @@ class CollectionRegistry {
     }
   }
 
-  getRegistryName(): string {
-    switch (this.kind) {
-      case RegistryKind.Definition:
-        return "definitions";
-      case RegistryKind.Parameter:
-        return "parameters";
-      case RegistryKind.Response:
-        return "responses";
-      case RegistryKind.SecurityDefinition:
-        return "securityDefinitions";
-    }
-  }
-
   /** Add or update an item. */
   add(itemPath: string, name: string, value: any) {
     const resolvedPath = getResolvedPath(itemPath);
@@ -75,7 +64,7 @@ class CollectionRegistry {
   countReference(path: string, name: string, kind: RegistryKind) {
     // convert backslashes to forward slashes
     path = path.replace(/\\/g, "/");
-    const pathKey = `${path}#/${this.getRegistryName()}/${name}`;
+    const pathKey = `${path}#/${getRegistryName(this.kind)}/${name}`;
     this.unreferenced.delete(pathKey);
   }
 
@@ -99,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,
@@ -128,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();
   }
 
@@ -155,6 +144,10 @@ export class DefinitionRegistry {
           for (const [key, value] of toSorted(Object.entries(itemCopy))) {
             matchCopy[key] = value;
           }
+          this.client?.suppressions?.propagateSuppression(
+            refResult,
+            this.currentPath
+          );
           return this.#expand(matchCopy, refResult.name);
         }
       } else {
@@ -168,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;
     }
@@ -225,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 ?? {}) };
@@ -302,11 +300,12 @@ export class DefinitionRegistry {
 
   #expandReferencesForCollection(collection: CollectionRegistry) {
     this.referenceStack = [];
-    for (const [path, values] of collection.data.entries()) {
-      this.currentPath = path;
+    for (const [filepath, values] of collection.data.entries()) {
       for (const [key, value] of values.entries()) {
-        let expanded = this.#expand(value, key, path);
-        collection.data.get(path)!.set(key, expanded);
+        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
@@ -319,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();
   }
 
   /**