Skip to content

Commit edeaf8a

Browse files
mikehardermikeharder
authored
Ensure properties are dereferenced before comparing equality (#329)
1 parent cb1ed36 commit edeaf8a

11 files changed

Lines changed: 263 additions & 63 deletions

‎package-lock.json‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@azure/oad",
3-
"version": "0.10.8",
3+
"version": "0.10.9",
44
"author": {
55
"name": "Microsoft Corporation",
66
"email": "azsdkteam@microsoft.com",

‎src/lib/util/resolveSwagger.ts‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -293,24 +293,26 @@ export class ResolveSwagger {
293293
if (!unwrappedProperty) {
294294
throw new Error("Null unwrapped property.")
295295
}
296+
297+
// Note: per https://editor-next.swagger.io/ tooltip when hovering over '$ref',
298+
// other properties must be ignored when $ref is present:
299+
//
300+
// $ref: string
301+
// Any time a subschema is expected, a schema may instead use an object containing a "$ref" property.
302+
// The value of the $ref is a URI Reference. Resolved against the current URI base,
303+
// it identifies the URI of a schema to use.
304+
// All other properties in a "$ref" object MUST be ignored.
305+
//
306+
parentProperty = parentProperty.$ref ? this.dereference(parentProperty.$ref) : parentProperty
307+
unwrappedProperty = unwrappedProperty.$ref ? this.dereference(unwrappedProperty.$ref) : unwrappedProperty
308+
296309
if ((!parentProperty.type || parentProperty.type === "object") && (!unwrappedProperty.type || unwrappedProperty.type === "object")) {
297-
let parentPropertyToCompare = parentProperty
298-
let unwrappedPropertyToCompare = unwrappedProperty
299-
if (parentProperty.$ref) {
300-
parentPropertyToCompare = this.dereference(parentProperty.$ref)
301-
}
302-
if (unwrappedProperty.$ref) {
303-
unwrappedPropertyToCompare = this.dereference(unwrappedProperty.$ref)
304-
}
305-
if (parentPropertyToCompare === unwrappedPropertyToCompare) {
306-
return true
307-
}
308-
return false
309-
}
310-
if (parentProperty.type === "array" && unwrappedProperty.type === "array") {
310+
return parentProperty === unwrappedProperty
311+
} else if (parentProperty.type === "array" && unwrappedProperty.type === "array") {
311312
return this.isEqual(parentProperty.items, unwrappedProperty.items)
313+
} else {
314+
return parentProperty.type === unwrappedProperty.type && parentProperty.format === unwrappedProperty.format
312315
}
313-
return parentProperty.type === unwrappedProperty.type && parentProperty.format === unwrappedProperty.format
314316
}
315317

316318
private checkCircularAllOf(schema: any, visited: any[] | undefined, referenceChain: string[]) {

‎src/test/compatiblePropertiesTest.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ import { fileUrl } from "./fileUrl"
77
// Given a property with given type and name
88
// When another property with the same name and compatible type is referenced
99
// Then no issue is reported, as this is a valid scenario
10+
//
11+
// Also ensures that $ref are resolved when determining if types are compatible.
12+
// For example, these should be compatible:
13+
// 1. "bar": { "type":"string" }
14+
// 2. "bar": { "$ref":"#/definitions/MyString" }, "MyString": { "type": "string" }
1015
test("compatible-properties", async () => {
1116
const diff = new OpenApiDiff({})
1217
const file = "src/test/specs/compatible-properties.json"

‎src/test/incompatiblePropertiesTest.ts‎

Lines changed: 75 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,14 @@ import { fileUrl } from "./fileUrl"
77
// Given a property with given type and name
88
// When another property with the same name but an incompatible type is referenced
99
// Then an issue is reported, with output providing the exact source file locations of both of the occurrences of the property.
10-
test("incompatible-properties", async () => {
10+
//
11+
// Also ensures that $ref are resolved when determining if types are compatible.
12+
// For example, these should be incompatible:
13+
// 1. "bar": { "type":"string" }
14+
// 2. "bar": { "$ref":"#/definitions/MyObject" }, "MyObject": { "type": "object" }
15+
test.each(["string-object", "string-refobject", "refstring-object", "refstring-refobject"])("incompatible-properties-%s", async prop => {
1116
const diff = new OpenApiDiff({})
12-
const file = "src/test/specs/incompatible-properties.json"
17+
const file = `src/test/specs/incompatible-properties/${prop}.json`
1318
const filePath = fileUrl(path.resolve(file))
1419

1520
try {
@@ -19,11 +24,74 @@ test("incompatible-properties", async () => {
1924
const e = error as Error
2025
assert.equal(
2126
e.message,
22-
"incompatible properties : bar\n" +
23-
" definitions/FooBarString/properties/bar\n" +
24-
` at ${filePath}#L13:8\n` +
25-
" definitions/FooBarObject/properties/bar\n" +
26-
` at ${filePath}#L26:8`
27+
`incompatible properties : ${prop}\n` +
28+
` definitions/Foo/properties/${prop}\n` +
29+
` at ${filePath}#L12:8\n` +
30+
` definitions/Foo2/properties/${prop}\n` +
31+
` at ${filePath}#L25:8`
2732
)
2833
}
2934
})
35+
36+
// test("incompatible-properties-string-refobject", async () => {
37+
// const diff = new OpenApiDiff({})
38+
// const file = "src/test/specs/incompatible-properties/string-refobject.json"
39+
// const filePath = fileUrl(path.resolve(file))
40+
41+
// try {
42+
// await diff.compare(file, file)
43+
// assert.fail("expected diff.compare() to throw")
44+
// } catch (error) {
45+
// const e = error as Error
46+
// assert.equal(
47+
// e.message,
48+
// "incompatible properties : string-refobject\n" +
49+
// " definitions/Foo/properties/string-refobject\n" +
50+
// ` at ${filePath}#L12:8\n` +
51+
// " definitions/Foo2/properties/string-refobject\n" +
52+
// ` at ${filePath}#L25:8`
53+
// )
54+
// }
55+
// })
56+
57+
// test("incompatible-properties-refstring-object", async () => {
58+
// const diff = new OpenApiDiff({})
59+
// const file = "src/test/specs/incompatible-properties/string-refobject.json"
60+
// const filePath = fileUrl(path.resolve(file))
61+
62+
// try {
63+
// await diff.compare(file, file)
64+
// assert.fail("expected diff.compare() to throw")
65+
// } catch (error) {
66+
// const e = error as Error
67+
// assert.equal(
68+
// e.message,
69+
// "incompatible properties : refstring-object\n" +
70+
// " definitions/Foo/properties/refstring-object\n" +
71+
// ` at ${filePath}#L12:8\n` +
72+
// " definitions/Foo2/properties/refstring-object\n" +
73+
// ` at ${filePath}#L25:8`
74+
// )
75+
// }
76+
// })
77+
78+
// test("incompatible-properties-refstring-refobject", async () => {
79+
// const diff = new OpenApiDiff({})
80+
// const file = "src/test/specs/incompatible-properties/refstring-refobject.json"
81+
// const filePath = fileUrl(path.resolve(file))
82+
83+
// try {
84+
// await diff.compare(file, file)
85+
// assert.fail("expected diff.compare() to throw")
86+
// } catch (error) {
87+
// const e = error as Error
88+
// assert.equal(
89+
// e.message,
90+
// "incompatible properties : refstring-refobject\n" +
91+
// " definitions/Foo/properties/refstring-refobject\n" +
92+
// ` at ${filePath}#L12:8\n` +
93+
// " definitions/Foo2/properties/refstring-refobject\n" +
94+
// ` at ${filePath}#L25:8`
95+
// )
96+
// }
97+
// })

‎src/test/specs/compatible-properties.json‎

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,53 @@
11
{
22
"swagger": "2.0",
33
"info": {
4-
"title": "xms-enum-name",
4+
"title": "compatible-properties",
55
"version": "1.0"
66
},
77
"paths": {
88
},
99
"definitions": {
10-
"FooBarString": {
10+
"Foo": {
1111
"type":"object",
1212
"properties": {
13-
"bar": {
13+
"string-string": {
1414
"type":"string"
15+
},
16+
"string-refstring": {
17+
"type":"string"
18+
},
19+
"refstring-string": {
20+
"$ref": "#/definitions/MyString"
21+
},
22+
"refstring-refstring": {
23+
"$ref": "#/definitions/MyString"
1524
}
1625
},
1726
"allOf": [
1827
{
19-
"$ref": "#/definitions/FooBarString2"
28+
"$ref": "#/definitions/Foo2"
2029
}
2130
]
2231
},
23-
"FooBarString2": {
32+
"Foo2": {
2433
"type":"object",
2534
"properties": {
26-
"bar": {
35+
"string-string": {
36+
"type":"string"
37+
},
38+
"string-refstring": {
39+
"$ref": "#/definitions/MyString"
40+
},
41+
"refstring-string": {
2742
"type":"string"
43+
},
44+
"refstring-refstring": {
45+
"$ref": "#/definitions/MyString"
2846
}
2947
}
48+
},
49+
"MyString": {
50+
"type": "string"
3051
}
3152
}
3253
}

‎src/test/specs/incompatible-properties.json‎

Lines changed: 0 additions & 32 deletions
This file was deleted.

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
{
2+
"swagger": "2.0",
3+
"info": {
4+
"title": "incompatible-properties-refstring-object",
5+
"version": "1.0"
6+
},
7+
"paths": {},
8+
"definitions": {
9+
"Foo": {
10+
"type": "object",
11+
"properties": {
12+
"refstring-object": {
13+
"$ref": "#/definitions/MyString"
14+
}
15+
},
16+
"allOf": [
17+
{
18+
"$ref": "#/definitions/Foo2"
19+
}
20+
]
21+
},
22+
"Foo2": {
23+
"type": "object",
24+
"properties": {
25+
"refstring-object": {
26+
"type": "object"
27+
}
28+
}
29+
},
30+
"MyString": {
31+
"type": "string"
32+
}
33+
}
34+
}

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
{
2+
"swagger": "2.0",
3+
"info": {
4+
"title": "incompatible-properties-refstring-refobject",
5+
"version": "1.0"
6+
},
7+
"paths": {},
8+
"definitions": {
9+
"Foo": {
10+
"type": "object",
11+
"properties": {
12+
"refstring-refobject": {
13+
"$ref": "#/definitions/MyString"
14+
}
15+
},
16+
"allOf": [
17+
{
18+
"$ref": "#/definitions/Foo2"
19+
}
20+
]
21+
},
22+
"Foo2": {
23+
"type": "object",
24+
"properties": {
25+
"refstring-refobject": {
26+
"$ref": "#/definitions/MyObject"
27+
}
28+
}
29+
},
30+
"MyObject": {
31+
"type": "object"
32+
},
33+
"MyString": {
34+
"type": "string"
35+
}
36+
}
37+
}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
{
2+
"swagger": "2.0",
3+
"info": {
4+
"title": "incompatible-properties-string-object",
5+
"version": "1.0"
6+
},
7+
"paths": {},
8+
"definitions": {
9+
"Foo": {
10+
"type": "object",
11+
"properties": {
12+
"string-object": {
13+
"type": "string"
14+
}
15+
},
16+
"allOf": [
17+
{
18+
"$ref": "#/definitions/Foo2"
19+
}
20+
]
21+
},
22+
"Foo2": {
23+
"type": "object",
24+
"properties": {
25+
"string-object": {
26+
"type": "object"
27+
}
28+
}
29+
}
30+
}
31+
}

0 commit comments

Comments
 (0)
⚡