thoughtbot / json_matchers

Validate your JSON APIs
MIT License
384 stars 40 forks source link

Local refs don't work in relatively referenced schema #90

Closed musaffa closed 5 years ago

musaffa commented 5 years ago

Here's a failing spec. posts/index schema refers to post schema as its item. In the referenced schema (post), we are trying to use attribute definition (local to the post schema) from attribute property by "$ref": "#/definitions/attributes". In the attribute definition, "id" type has intentionally been set to "string". In our test json, we use an integer for that id attribute. It shouldn't match but it does.

If I replace the atttribute definition with the inline definition(as in the commented out section), then the schema doesn't match and the test passes. This is the expected behaviour.

In a nutshell, inline definitions in a referenced schema work, but local ref definitions in that referenced schema don't work.

it "Fails properly in local refs in referenced schema" do
  create(:schema, name: "post", json: {
    "$schema": "https://json-schema.org/draft-04/schema#",
    "id": "file:/post.json#",
    "definitions": {
      "attributes": {
        "type": "object",
        "required": [
          "id",
          "name",
        ],
        "properties": {
          "id": { "type": "string" },
          "name": { "type": "string" }
        }
      }
    },
    "type": "object",
    "required": [
      "id",
      "type",
      "attributes"
    ],
    "properties": {
      "id": { "type": "string" },
      "type": { "type": "string" },
      "attributes": {
        "$ref": "#/definitions/attributes"
        # "type": "object",
        # "required": [
        #   "id",
        #   "name",
        # ],
        # "properties": {
        #   "id": { "type": "string" },
        #   "name": { "type": "string" }
        # }
      }
    }
  })
  posts_index = create(:schema, name: "posts/index", json: {
    "$schema": "https://json-schema.org/draft-04/schema#",
    "id": "file:/posts/index.json#",
    "type": "object",
    "required": [
      "data"
    ],
    "definitions": {
      "posts": {
        "type": "array",
        "items": {
          "$ref": "file:/post.json#"
        }
      }
    },
    "properties": {
      "data": {
        "$ref": "#/definitions/posts"
      }
    }
  })

  json = build(:response, {
    "data": [{
      "id": "1",
      "type": "Post",
      "attributes": {
        "id": 1,
        "name": "The Post's Name"
      }
    }]
  })

  expect(json).not_to match_json_schema(posts_index)
end
seanpdoyle commented 5 years ago

@musaffa I've pushed up https://github.com/thoughtbot/json_matchers/pull/89 to address a related Issue (#88).

I've pushed up https://github.com/thoughtbot/json_matchers/pull/89/commits/4b60536dee109717e3057191bbcc12d6d7457f52 to include the test you shared as failing. I've changed the id to be consistently declared as a "number" attribute, and consistently declared as a "string" attribute in the payload.

The test passes. Could you leave some comments on the diff to help us better understand the underlying issue?

musaffa commented 5 years ago

Let me clarify the test again.

The above schema requires all the ids to be of type string. If any id in the payload is an integer then the payload will be incorrect according to this schema. In fact, we pass an incorrect payload (see "id": 1 in attributes), so we expect that the json schema shouldn't match with this payload. That's why we are using not_to match. But it actually matches, hence the test fails.

Setting all the ids as string attribute in the payload defeats the purpose of this test.

seanpdoyle commented 5 years ago

The above schema requires all the ids to be of type string.

@musaffa I've pushed up 42d6c6ea7f07c1dbb27e62c970d0c62342cc3485 to https://github.com/thoughtbot/json_matchers/pull/89 to change the two tests to ensure all "id" are declared as "type": "string". If what I've pushed is incorrect, could you please comment on that commit with corrections?

If any id in the payload is an integer then the payload will be incorrect according to this schema. ... That's why we are using not_to match. But it actually matches, hence the test fails.

89 adds tests that use both to match_response_schema and not_tomatch_response_schema` to cover both scenarios. Those tests pass.

Could you add some details that that PR or any of the commits to help us better reproduce the issues you're experiencing?