Ensure properties are dereferenced before comparing equality

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@azure/oad",
-  "version": "0.10.8",
+  "version": "0.10.9",
   "author": {
     "name": "Microsoft Corporation",
     "email": "azsdkteam@microsoft.com",

src/lib/util/resolveSwagger.ts

@@ -293,24 +293,26 @@ export class ResolveSwagger {
     if (!unwrappedProperty) {
       throw new Error("Null unwrapped property.")
     }
+
+    // Note: per https://editor-next.swagger.io/ tooltip when hovering over '$ref',
+    // other properties must be ignored when $ref is present:
+    //
+    //  $ref: string
+    //  Any time a subschema is expected, a schema may instead use an object containing a "$ref" property.
+    //  The value of the $ref is a URI Reference. Resolved against the current URI base,
+    //  it identifies the URI of a schema to use.
+    //  All other properties in a "$ref" object MUST be ignored.
+    //
+    parentProperty = parentProperty.$ref ? this.dereference(parentProperty.$ref) : parentProperty
+    unwrappedProperty = unwrappedProperty.$ref ? this.dereference(unwrappedProperty.$ref) : unwrappedProperty
+
     if ((!parentProperty.type || parentProperty.type === "object") && (!unwrappedProperty.type || unwrappedProperty.type === "object")) {
-      let parentPropertyToCompare = parentProperty
-      let unwrappedPropertyToCompare = unwrappedProperty
-      if (parentProperty.$ref) {
-        parentPropertyToCompare = this.dereference(parentProperty.$ref)
-      }
-      if (unwrappedProperty.$ref) {
-        unwrappedPropertyToCompare = this.dereference(unwrappedProperty.$ref)
-      }
-      if (parentPropertyToCompare === unwrappedPropertyToCompare) {
-        return true
-      }
-      return false
-    }
-    if (parentProperty.type === "array" && unwrappedProperty.type === "array") {
+      return parentProperty === unwrappedProperty
+    } else if (parentProperty.type === "array" && unwrappedProperty.type === "array") {
       return this.isEqual(parentProperty.items, unwrappedProperty.items)
+    } else {
+      return parentProperty.type === unwrappedProperty.type && parentProperty.format === unwrappedProperty.format
     }
-    return parentProperty.type === unwrappedProperty.type && parentProperty.format === unwrappedProperty.format
   }
 
   private checkCircularAllOf(schema: any, visited: any[] | undefined, referenceChain: string[]) {

src/test/compatiblePropertiesTest.ts

@@ -7,6 +7,11 @@ import { fileUrl } from "./fileUrl"
 // Given a property with given type and name
 // When another property with the same name and compatible type is referenced
 // Then no issue is reported, as this is a valid scenario
+//
+// Also ensures that $ref are resolved when determining if types are compatible.
+// For example, these should be compatible:
+// 1. "bar": { "type":"string" }
+// 2. "bar": { "$ref":"#/definitions/MyString" }, "MyString": { "type": "string" }
 test("compatible-properties", async () => {
   const diff = new OpenApiDiff({})
   const file = "src/test/specs/compatible-properties.json"

src/test/incompatiblePropertiesTest.ts

@@ -7,9 +7,14 @@ import { fileUrl } from "./fileUrl"
 // Given a property with given type and name
 // When another property with the same name but an incompatible type is referenced
 // Then an issue is reported, with output providing the exact source file locations of both of the occurrences of the property.
-test("incompatible-properties", async () => {
+//
+// Also ensures that $ref are resolved when determining if types are compatible.
+// For example, these should be incompatible:
+// 1. "bar": { "type":"string" }
+// 2. "bar": { "$ref":"#/definitions/MyObject" }, "MyObject": { "type": "object" }
+test.each(["string-object", "string-refobject", "refstring-object", "refstring-refobject"])("incompatible-properties-%s", async prop => {
   const diff = new OpenApiDiff({})
-  const file = "src/test/specs/incompatible-properties.json"
+  const file = `src/test/specs/incompatible-properties/${prop}.json`
   const filePath = fileUrl(path.resolve(file))
 
   try {
@@ -19,11 +24,74 @@ test("incompatible-properties", async () => {
     const e = error as Error
     assert.equal(
       e.message,
-      "incompatible properties : bar\n" +
-        "  definitions/FooBarString/properties/bar\n" +
-        `    at ${filePath}#L13:8\n` +
-        "  definitions/FooBarObject/properties/bar\n" +
-        `    at ${filePath}#L26:8`
+      `incompatible properties : ${prop}\n` +
+        `  definitions/Foo/properties/${prop}\n` +
+        `    at ${filePath}#L12:8\n` +
+        `  definitions/Foo2/properties/${prop}\n` +
+        `    at ${filePath}#L25:8`
     )
   }
 })
+
+// test("incompatible-properties-string-refobject", async () => {
+//   const diff = new OpenApiDiff({})
+//   const file = "src/test/specs/incompatible-properties/string-refobject.json"
+//   const filePath = fileUrl(path.resolve(file))
+
+//   try {
+//     await diff.compare(file, file)
+//     assert.fail("expected diff.compare() to throw")
+//   } catch (error) {
+//     const e = error as Error
+//     assert.equal(
+//       e.message,
+//       "incompatible properties : string-refobject\n" +
+//         "  definitions/Foo/properties/string-refobject\n" +
+//         `    at ${filePath}#L12:8\n` +
+//         "  definitions/Foo2/properties/string-refobject\n" +
+//         `    at ${filePath}#L25:8`
+//     )
+//   }
+// })
+
+// test("incompatible-properties-refstring-object", async () => {
+//   const diff = new OpenApiDiff({})
+//   const file = "src/test/specs/incompatible-properties/string-refobject.json"
+//   const filePath = fileUrl(path.resolve(file))
+
+//   try {
+//     await diff.compare(file, file)
+//     assert.fail("expected diff.compare() to throw")
+//   } catch (error) {
+//     const e = error as Error
+//     assert.equal(
+//       e.message,
+//       "incompatible properties : refstring-object\n" +
+//         "  definitions/Foo/properties/refstring-object\n" +
+//         `    at ${filePath}#L12:8\n` +
+//         "  definitions/Foo2/properties/refstring-object\n" +
+//         `    at ${filePath}#L25:8`
+//     )
+//   }
+// })
+
+// test("incompatible-properties-refstring-refobject", async () => {
+//   const diff = new OpenApiDiff({})
+//   const file = "src/test/specs/incompatible-properties/refstring-refobject.json"
+//   const filePath = fileUrl(path.resolve(file))
+
+//   try {
+//     await diff.compare(file, file)
+//     assert.fail("expected diff.compare() to throw")
+//   } catch (error) {
+//     const e = error as Error
+//     assert.equal(
+//       e.message,
+//       "incompatible properties : refstring-refobject\n" +
+//         "  definitions/Foo/properties/refstring-refobject\n" +
+//         `    at ${filePath}#L12:8\n` +
+//         "  definitions/Foo2/properties/refstring-refobject\n" +
+//         `    at ${filePath}#L25:8`
+//     )
+//   }
+// })

src/test/specs/compatible-properties.json

@@ -1,32 +1,53 @@
 {
   "swagger": "2.0",
   "info": {
-    "title": "xms-enum-name",
+    "title": "compatible-properties",
     "version": "1.0"
   },
   "paths": {
   },
   "definitions": {
-    "FooBarString": {
+    "Foo": {
       "type":"object",
       "properties": {
-        "bar": {
+        "string-string": {
           "type":"string"
+        },
+        "string-refstring": {
+          "type":"string"
+        },
+        "refstring-string": {
+          "$ref": "#/definitions/MyString"
+        },
+        "refstring-refstring": {
+          "$ref": "#/definitions/MyString"
         }
       },
       "allOf": [
         {
-          "$ref": "#/definitions/FooBarString2"
+          "$ref": "#/definitions/Foo2"
         }
       ]
     },
-    "FooBarString2": {
+    "Foo2": {
       "type":"object",
       "properties": {
-        "bar": {
+        "string-string": {
+          "type":"string"
+        },
+        "string-refstring": {
+          "$ref": "#/definitions/MyString"
+        },
+        "refstring-string": {
           "type":"string"
+        },
+        "refstring-refstring": {
+          "$ref": "#/definitions/MyString"
         }
       }
+    },
+    "MyString": {
+      "type": "string"
     }
   }
 }

src/test/specs/incompatible-properties/refstring-object.json

@@ -0,0 +1,34 @@
+{
+  "swagger": "2.0",
+  "info": {
+    "title": "incompatible-properties-refstring-object",
+    "version": "1.0"
+  },
+  "paths": {},
+  "definitions": {
+    "Foo": {
+      "type": "object",
+      "properties": {
+        "refstring-object": {
+          "$ref": "#/definitions/MyString"
+        }
+      },
+      "allOf": [
+        {
+          "$ref": "#/definitions/Foo2"
+        }
+      ]
+    },
+    "Foo2": {
+      "type": "object",
+      "properties": {
+        "refstring-object": {
+          "type": "object"
+        }
+      }
+    },
+    "MyString": {
+      "type": "string"
+    }
+  }
+}

src/test/specs/incompatible-properties/refstring-refobject.json

@@ -0,0 +1,37 @@
+{
+  "swagger": "2.0",
+  "info": {
+    "title": "incompatible-properties-refstring-refobject",
+    "version": "1.0"
+  },
+  "paths": {},
+  "definitions": {
+    "Foo": {
+      "type": "object",
+      "properties": {
+        "refstring-refobject": {
+          "$ref": "#/definitions/MyString"
+        }
+      },
+      "allOf": [
+        {
+          "$ref": "#/definitions/Foo2"
+        }
+      ]
+    },
+    "Foo2": {
+      "type": "object",
+      "properties": {
+        "refstring-refobject": {
+          "$ref": "#/definitions/MyObject"
+        }
+      }
+    },
+    "MyObject": {
+      "type": "object"
+    },
+    "MyString": {
+      "type": "string"
+    }
+  }
+}

src/test/specs/incompatible-properties/string-object.json

@@ -0,0 +1,31 @@
+{
+  "swagger": "2.0",
+  "info": {
+    "title": "incompatible-properties-string-object",
+    "version": "1.0"
+  },
+  "paths": {},
+  "definitions": {
+    "Foo": {
+      "type": "object",
+      "properties": {
+        "string-object": {
+          "type": "string"
+        }
+      },
+      "allOf": [
+        {
+          "$ref": "#/definitions/Foo2"
+        }
+      ]
+    },
+    "Foo2": {
+      "type": "object",
+      "properties": {
+        "string-object": {
+          "type": "object"
+        }
+      }
+    }
+  }
+}