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.

Sign up for GitHub

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

Jump to bottom

Fix id handling #178

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix id handling #178

wants to merge 3 commits into from

Conversation

gabrielg
Copy link
Contributor

@gabrielg gabrielg commented Nov 7, 2014

This introduces a few refactors and changes which fixes handling of what validates as a valid Draft 4 schema:

{
  "id": "#/example",
  "type": "integer",
  "oneOf": [
    {"$ref": "#/definitions/foo"},
    {"$ref": "#/definitions/bar"}],
  "definitions": {
    "foo": {
      "type": "integer",
      "maximum": 1000
    },
    "bar": {
      "type": "integer",
      "minimum": 2000,
      "maximum": 3000
    }
  }
}

json-schema validation to show it's a valid schema:

irb(main):005:0> require 'json-schema'
=> true
irb(main):006:0> puts valid_schema
{
  "id": "#/example",
  "type": "integer",
  "oneOf": [
    {"$ref": "#/definitions/foo"},
    {"$ref": "#/definitions/bar"}],
  "definitions": {
    "foo": {
      "type": "integer",
      "maximum": 1000
    },
    "bar": {
      "type": "integer",
      "minimum": 2000,
      "maximum": 3000
    }
  }
}
=> nil
irb(main):007:0> JSON::Validator.fully_validate_schema(valid_schema, version: :draft4)
=> []

It also validates against the Java json-schema-validator.

Before these changes, trying to use the provided example schema would result in an exception:

/usr/local/opt/rbenv/versions/2.1.2/lib/ruby/2.1.0/uri/common.rb:176:in `split': bad URI(is not URI?): {"id"=>"#/example", "type"=>"integer", "oneOf"=>[{"$ref"=>"#/definitions/foo"}, {"$ref"=>"#/definitions/bar"}], "definitions"=>{"foo"=>{"type"=>"integer", "maximum"=>1000}, "bar"=>{"type"=>"integer", "minimum"=>2000, "maximum"=>3000}}} (URI::InvalidURIError)
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/2.1.0/uri/common.rb:211:in `parse'
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/2.1.0/uri/common.rb:747:in `parse'
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/json-schema-2.4.1/lib/json-schema/validator.rb:584:in `normalized_uri'
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/json-schema-2.4.1/lib/json-schema/validator.rb:528:in `rescue in initialize_schema'
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/json-schema-2.4.1/lib/json-schema/validator.rb:517:in `initialize_schema'
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/json-schema-2.4.1/lib/json-schema/validator.rb:47:in `block in initialize'
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/json-schema-2.4.1/lib/json-schema/validator.rb:47:in `synchronize'
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/json-schema-2.4.1/lib/json-schema/validator.rb:47:in `initialize'
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/json-schema-2.4.1/lib/json-schema/validator.rb:294:in `new'
    from /usr/local/opt/rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/json-schema-2.4.1/lib/json-schema/validator.rb:294:in `fully_validate'
    from ./bin/json-validator:19:in `<main>'

This PR refactors some things and fixes the issue.

@gabrielg
Refactor JSON::Schema and fix ListOptionTest.
482521d 
* Refactor JSON::Schema#initialize to move schema URI generation into
  its own method, #generate_schema_uri.
* Refactor JSON::Schema#base_uri to do less path munging itself.
* Fix ListOptionTest, which was relying on seemingly unintended
  behaviour where JSON::Schema's constructor was modifying a URI
  in-place, thus appending a '#' to the URI used in the test. As
  JSON::Schema's constructor doesn't modify its arguments anymore,
  this test started to fail.
@gabrielg
Generate absolute URIs as fake schema URIs.
0c63566 
This generates absolute URN URIs for schemas as necessary, rather than
relative ones. Using relative URIs causes errors given schemas that
should actually be valid.

BadSchemaRefTest has been altered to use a reference to an invalid
URI, rather than a valid relative URI.
@gabrielg
Add tests for other valid schemas with id properties.
9fcad2f 
Commits 0c63566 and 482521d fixed improper handling of the id
attribute, this adds the tests to prove it.

Also introduces a minor refactor to JSON::Schema#generate_schema_uri.
pd
pd reviewed Nov 7, 2014
View reviewed changes
lib/json-schema/schema.rb
@@ -50,14 +38,23 @@ def self.stringify(schema)
end

def base_uri
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you introduced a new usage I haven't noticed, I think this method has been entirely unused since the first commit of this project. I propose just deleting it unless anyone has a reason to keep it. =)

@pd pd mentioned this pull request Nov 7, 2014
Merged
@pd
Copy link
Contributor

pd commented Nov 7, 2014

Seems okay. I don't really grok why a schema would ever define a fragment as its top-level id, tho. I find it very difficult to reason about the Correct Behavior ™️ of the id property, especially since it seems to be largely redefined from draft3->draft4.

@malandrina malandrina mentioned this pull request Oct 30, 2015
Open
@pelted pelted mentioned this pull request Nov 2, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gabrielg @pd