Having an id parameter in the schema messes up schema nesting

[c-lliope]

I discovered this bug after generating schemas with prmd.

Minimal failing spec:

it 'works with an id' do
  response = double(body: '{ "a": "b" }')

  expect(response).to match_response_schema("top_level")
end

top_level.json

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "type": "object",
  "properties": {
    "a": { "$ref": "nested.json" }
  }
}

nested.json

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "type": "string",
  "id": "nested"
}

Removing the id from nested.json fixes the spec. Unfortunately, id is required for compatibility with prmd. Documentation

[seanpdoyle]

@Graysonwright thanks for uncovering this.

What was the test failure message?

[malandrina]

Looks like a PR for fixing an issue related to the supporting the id keyword was opened on json_schema a year ago: voxpupuli/json-schema#178

I am experimenting with using json_schema instead of json-schema in a branch. json_schema is what prmd and committee use and my guess is that our odds of achieving compatibility are higher if we use the same validator.

[c-lliope]

Here's the error:

  1) Failing test case works with an id
     Failure/Error: expect(response).to match_response_schema("top_level")
             expected

             { "a": "b" }

             to match schema "top_level":

             {
         "$schema": "http://json-schema.org/draft-04/schema#",
         "type": "object",
         "properties": {
           "a": { "$ref": "nested.json" }
         }
       }


             ---

             The property '#/a' was not a valid schema in schema file:///Users/grayson/dev/tbot/reproduction/spec/support/schemas/top_level.json#
     # ./spec/failing_test_case.rb:7:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:87:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:86:in `block (2 levels) in <top (required)>'

[malandrina]

I was able to replace json-schema with json_schema with no problem but I haven't succeeded in updating the tests. Structuring the schemata in such a way that the JSON pointers are valid has been a struggle.

A good next step would be for @seanpdoyle to take a look at my branch and review Heroku's guidelines for structuring schemata to determine whether we actually want to take the approach of following their guidelines. If it is, we can take another stab at updating the specs.

[seanpdoyle]

@malandrina that all looks good!

What are the implications of switching gems?

Does the public API change?

Would consumers still be able to write somewhat arbitrary schemata, or would they be restricted to to writing schemata that conform to that standard?

Is our best approach an all-or-nothing change, or could/would/should we gradually migrate?

[malandrina]

@seanpdoyle great questions!

What are the implications of switching gems?

• The public API would change: The strict option is not a thing in json_schema, so we would probably want to move away from supporting that option in the matcher.

• Consumers would have to write schemata that conform to the Heroku conventions

• I hadn’t thought much about the migration path but it’s an important question. The easiest approach for us would be an all-or-nothing change.

  Another idea which just occurred to me is to change the API of our matcher to support a heroku-http-api-toolchain option (only with a better name). That way people who aren’t using prmd, committee etc. and don’t care about compatibility can continue to use the matcher in its current state, while others who do need compatibility with Heroku’s tooling have a way to achieve it.

[malandrina]

Per our discussion IRL today, we've decided it makes sense to switch to json_schema.

Given that, we need to:

• Get tests passing on json_matchers/lw-use-other-json-schema-validator
• Update documentation
  • Communicate that we don’t support options any more
  • Provide guidance on structuring schemata

[ms-ati]

@malandrina @seanpdoyle Is this dead?

[seanpdoyle]

Hey @Graysonwright and @malandrina from the past!

We were hoping for some clarification on this issue. Specifically, we were able
to reproduce this failing test case with the original validator, but we get
entirely different failures with the new validators.

With the old validator, when we specify the nested schema's id as
"nested.json" (instead of "nested"), the test passes:

it 'works with an id' do
  nested = create(:schema, name: "nested", json: {
    "$schema": "http://json-schema.org/draft-04/schema#",
    "type": "string",
    "id": "nested.json"
  })

  schema = create(:schema, {
    "$schema": "http://json-schema.org/draft-04/schema#",
    "type": "object",
    "properties": {
      "a": { "$ref": "nested.json" }
    }
  })

  response = double(body: '{ "a": "b" }')

  expect(response).to match_response_schema(schema)
end

That test passes with v0.8.0.

If you've experimented with the current master, renaming the nested
schema's { "id": "nested" } to { "id": "file:/nested.json" } and the
top-level schema's { "$ref": "nested.json" } to { "$ref": "file:/nested.json#" }, the tests pass:

it 'works with an id' do
  nested = create(:schema, name: "nested", json: {
    "$schema": "http://json-schema.org/draft-04/schema#",
    "type": "string",
    "id": "file:/nested.json"
  })

  schema = create(:schema, {
    "$schema": "http://json-schema.org/draft-04/schema#",
    "type": "object",
    "properties": {
      "a": { "$ref": "file:/nested.json#" }
    }
  })

  response = double(body: '{ "a": "b" }')

  expect(response).to match_response_schema(schema)
end

We are unclear on what the purpose of the nested id is, and why it needs to
correspond in whatever way to the top level reference.

Is there good documentation for this somewhere?

Do these workarounds make sense?

cc: @composerinteralia