Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
musaffa opened this issue Sep 19, 2018 · 3 comments
Closed

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

musaffa opened this issue Sep 19, 2018 · 3 comments

Comments

@musaffa
Copy link

musaffa commented Sep 19, 2018

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 added a commit that referenced this issue Nov 2, 2018
When the `"id"` key is specified as a `"number"` in both attribute and
property definitions, then specified as a `"string"` value in the
payload, the matcher will raise and the test will pass.
@seanpdoyle
Copy link
Collaborator

@musaffa I've pushed up #89 to address a related Issue (#88).

I've pushed up 4b60536 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
Copy link
Author

musaffa commented Nov 6, 2018

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
Copy link
Collaborator

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

@musaffa I've pushed up 42d6c6e to #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_to match_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?

seanpdoyle added a commit that referenced this issue Apr 5, 2019
When the `"id"` key is specified as a `"number"` in both attribute and
property definitions, then specified as a `"string"` value in the
payload, the matcher will raise and the test will pass.
seanpdoyle added a commit that referenced this issue Apr 5, 2019
When the `"id"` key is specified as a `"number"` in both attribute and
property definitions, then specified as a `"string"` value in the
payload, the matcher will raise and the test will pass.
seanpdoyle added a commit that referenced this issue Apr 5, 2019
Closes [#88].
Closes [#90].

Add tests from Issue #90

When the `"id"` key is specified as a `"number"` in both attribute and
property definitions, then specified as a `"string"` value in the
payload, the matcher will raise and the test will pass.

After rebasing off `master` after [`3b66b4d`][3b66b4d] was merged, [the
tests that were once failing][tests] are now passing.

[#88]: #88
[#90]: #90
[3b66b4d]: 3b66b4d
[tests]: #89 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants