ExpandAllOf() throws "incompatible properties" if schemas are compatible but not equal

[mikeharder]

The following spec should be valid per JSON Schema and Swagger. However, we throw error incompatible properties when trying to process it, since DogOwner.pet is type Dog, which is compatible with but not equal to PetOwner.pet of type Pet.

openapi-diff/src/lib/util/resolveSwagger.ts

Line 250 in 8850294

if (!this.isEqual(allOfSchema.properties[key], schemaList[key])) {

@JeffreyRichter, @mikekistler: Do you think the code should be changed to allow types that are compatible but not equal? If so, should it be conditioned on whether the swagger is handwritten or typespec-generated, under the assumption that SDKs for TypeSpec will be generated directly from TypeSpec and not the intermediate Swagger?

We recently fixed a similar issue in #329, although in this case the types were identical with just an extra step of indirection.

@markcowl and I believe this is valid swagger, but some of our SDK generators may not handle it correctly, which might be why it's not allowed in openapi-diff. However, openapi-diff should not be responsible for this. It should be tested directly by the SDK generation tests.

Repro Steps

Save spec below to file test.json, then run npx @azure/oad test.json test.json.

Error: incompatible properties : pet
  definitions/DogOwner/properties/pet
    at file:///home/mharder/tmp/oad/test.json#L61:8
  definitions/PetOwner/properties/pet
    at file:///home/mharder/tmp/oad/test.json#L53:8

{
  "swagger": "2.0",
  "info": {
    "version": "1.0.0",
    "title": "title",
  },
  "paths": {
  },
  "definitions": {
    "Pet": {
      "type": "object",
      "properties": {
        "name": {
          "type": "string"
        } 
      }
    },
    "Dog": {
      "type": "object",
      "properties": {
        "bones": {
          "type": "integer"
        }
      },
      "allOf": [{"$ref": "#/definitions/Pet"}]
    },
    "PetOwner": {
      "type": "object",
      "properties": {
        "ssn": {
          "type": "integer"
        },
        "pet": {
          "$ref": "#/definitions/Pet"
        }
      }
    },
    "DogOwner": {
      "type": "object",
      "properties": {
        "pet": {
          "$ref": "#/definitions/Dog"
        }
      },
      "allOf": [{"$ref": "#/definitions/PetOwner"}]
    }
  }
}

[mikekistler]

It might be dangerous to compare on content rather than name, because schemas that are the same today might not be the same tomorrow. In fact, it's possible that two different schemas exist simply to allow them to evolve independently.

[mikeharder]

It might be dangerous to compare on content rather than name, because schemas that are the same today might not be the same tomorrow. In fact, it's possible that two different schemas exist simply to allow them to evolve independently.

Can you elaborate on this, or provide an example to illustrate what you mean? Why do you think it might be dangerous to allow my example spec above? If either schema changes, the BreakingChanges tool will run again on the PR.

Note this error happens when we are pre-processing the spec, before we even get to comparing two specs. The chain of events:

1. DogOwner has a property pet of type Dog
2. DogOwner is also allOf PetOwner
3. PetOwner has a property pet of type Pet
4. Our code determines the properties are "incompatible", because the types are not the exact same
5. However, since type Dog satisfies type Pet, both JSON Schema and Swagger would consider this a semantically valid schema

I also think this is a more general case of the problem we fixed in #329, where we decided to allow $ref to be considered equivalent to the thing being referenced. All of the properties here are compatible with each other:

openapi-diff/src/test/specs/compatible-properties.json

Lines 10 to 51 in 8850294

	"Foo": {
	"type":"object",
	"properties": {
	"string-string": {
	"type":"string"
	},
	"string-refstring": {
	"type":"string"
	},
	"refstring-string": {
	"$ref": "#/definitions/MyString"
	},
	"refstring-refstring": {
	"$ref": "#/definitions/MyString"
	}
	},
	"allOf": [
	{
	"$ref": "#/definitions/Foo2"
	}
	]
	},
	"Foo2": {
	"type":"object",
	"properties": {
	"string-string": {
	"type":"string"
	},
	"string-refstring": {
	"$ref": "#/definitions/MyString"
	},
	"refstring-string": {
	"type":"string"
	},
	"refstring-refstring": {
	"$ref": "#/definitions/MyString"
	}
	}
	},
	"MyString": {
	"type": "string"
	}

[mikekistler]

Okay ... now I understand. Anything that is a Dog must also be a Pet because Dog allOf's Pet. I suppose this is safe.

[mikeharder]

Okay ... now I understand. Anything that is a Dog must also be a Pet because Dog allOf's Pet. I suppose this is safe.

Sounds good, let me try to create a PR to see how hard this will be to fix.