Ensure properties are dereferenced before comparing equality

[mikeharder]

• Fixes #331

@johanste, @markcowl, @JeffreyRichter, @allenjzhang, @mikekistler: Please object if anything about this change looks wrong to you.

In summary, the change always calls dereference() on a property with a $ref, before comparing equality.

This looks pretty safe to me and @konrad-jamrozik, and passes the new unit test we wrote (plus all existing tests).

[konrad-jamrozik]

I see the test fails with incompatible properties : bar.

I took the spec:

{
  "swagger": "2.0",
  "info": {
    "title": "compatible-properties-ref",
    "version": "1.0"
  },
  "paths": {
  },
  "definitions": {
    "FooBarString": {
      "type":"object",
      "properties": {
        "bar": {
          "type":"string"
        }
      },
      "allOf": [
        {
          "$ref": "#/definitions/FooBarStringRef"
        }
      ]
    },
    "FooBarStringRef": {
      "type":"object",
      "properties": {
        "bar": {
          "$ref":"#/definitions/MyString"
        }
      }
    },
    "MyString": {
      "type": "string"
    }
  }
}

and pasted it here:

• https://editor.swagger.io/
• https://editor-next.swagger.io/

I got no errors in both cases.

Looking at that screenshot, both definitions have the same bar. There is no incompatibility anywhere. So the spec appears to be OK. The tool interpretation must be wrong.

Interestingly, when I make MyString have different type and extra key example:

Then FooBarString picks up the example but ignores the different type:

[konrad-jamrozik]

OK I think I know what is the root-cause. The problem lies in openapi-diff logic.

We throw error from here

Which will happen if this line returns true:

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

On the failing test it evaluates to:

this.isEqual({"$ref":"#/definitions/MyString"}, {"type":"string"})

Now inside the this.isEqual logic the problem lies here:

if ((!parentProperty.type || parentProperty.type === "object") && (!unwrappedProperty.type || unwrappedProperty.type === "object")) {

On the failing test it evaluates to:

(!undefined || undefined === "object") && (!string || string === "object")

which is (true && false) which is false.

As a result, the isEqual will do no more dereferencing, so the parentProperty, which is {"$ref":"#/definitions/MyString"}, won't be dereferenced. Hence isEqual will return with the last return:

return parentProperty.type === unwrappedProperty.type && parentProperty.format === unwrappedProperty.format

which will evaluate like that:

parentProperty.type (undefined) === unwrappedProperty.type (string) && parentProperty.format (undefined) === unwrappedProperty.format (undefined) which is undefined === string && undefined === undefined which is false, hence we throw.

Basically, the logic of isEqual is assuming that it has either dereference both parentProperty and unwrappedProperty, or none, but not possibly only one.

[konrad-jamrozik]

@mikeharder I think you may want to change the PR title before merging to mention it fixes the bug.