diff --git a/package-lock.json b/package-lock.json index afef424f..5c3428fe 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@azure/oad", - "version": "0.10.5", + "version": "0.10.9", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@azure/oad", - "version": "0.10.5", + "version": "0.10.9", "license": "MIT", "dependencies": { "@ts-common/fs": "^0.2.0", diff --git a/package.json b/package.json index b5d055b5..75aaa196 100644 --- a/package.json +++ b/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", diff --git a/src/lib/util/resolveSwagger.ts b/src/lib/util/resolveSwagger.ts index 8f82340d..adac48f7 100644 --- a/src/lib/util/resolveSwagger.ts +++ b/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[]) { diff --git a/src/test/compatiblePropertiesTest.ts b/src/test/compatiblePropertiesTest.ts index 16cd8365..256d9965 100644 --- a/src/test/compatiblePropertiesTest.ts +++ b/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" diff --git a/src/test/incompatiblePropertiesTest.ts b/src/test/incompatiblePropertiesTest.ts index 2b2ec2ad..7cf13e32 100644 --- a/src/test/incompatiblePropertiesTest.ts +++ b/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` +// ) +// } +// }) diff --git a/src/test/specs/compatible-properties.json b/src/test/specs/compatible-properties.json index df2201d7..d15f1d64 100644 --- a/src/test/specs/compatible-properties.json +++ b/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" } } } diff --git a/src/test/specs/incompatible-properties.json b/src/test/specs/incompatible-properties.json deleted file mode 100644 index 35037219..00000000 --- a/src/test/specs/incompatible-properties.json +++ /dev/null @@ -1,32 +0,0 @@ -{ - "swagger": "2.0", - "info": { - "title": "xms-enum-name", - "version": "1.0" - }, - "paths": { - }, - "definitions": { - "FooBarString": { - "type":"object", - "properties": { - "bar": { - "type":"string" - } - }, - "allOf": [ - { - "$ref": "#/definitions/FooBarObject" - } - ] - }, - "FooBarObject": { - "type":"object", - "properties": { - "bar": { - "type":"object" - } - } - } - } -} diff --git a/src/test/specs/incompatible-properties/refstring-object.json b/src/test/specs/incompatible-properties/refstring-object.json new file mode 100644 index 00000000..63097afa --- /dev/null +++ b/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" + } + } +} diff --git a/src/test/specs/incompatible-properties/refstring-refobject.json b/src/test/specs/incompatible-properties/refstring-refobject.json new file mode 100644 index 00000000..cbd070da --- /dev/null +++ b/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" + } + } +} diff --git a/src/test/specs/incompatible-properties/string-object.json b/src/test/specs/incompatible-properties/string-object.json new file mode 100644 index 00000000..f210e3a4 --- /dev/null +++ b/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" + } + } + } + } +} diff --git a/src/test/specs/incompatible-properties/string-refobject.json b/src/test/specs/incompatible-properties/string-refobject.json new file mode 100644 index 00000000..b9668a9a --- /dev/null +++ b/src/test/specs/incompatible-properties/string-refobject.json @@ -0,0 +1,34 @@ +{ + "swagger": "2.0", + "info": { + "title": "incompatible-properties-string-refobject", + "version": "1.0" + }, + "paths": {}, + "definitions": { + "Foo": { + "type": "object", + "properties": { + "string-refobject": { + "type": "string" + } + }, + "allOf": [ + { + "$ref": "#/definitions/Foo2" + } + ] + }, + "Foo2": { + "type": "object", + "properties": { + "string-refobject": { + "$ref": "#/definitions/MyObject" + } + } + }, + "MyObject": { + "type": "object" + } + } +}