improve handling of fragments #382

notEthan · 2017-05-17T18:15:35Z

I've encountered a few problems dealing with fragments that this PR addresses:

traversing the pointer through the document tries to instantiate the value as a json schema at every level, which is not valid when the value is an array, and not needed in any case. only the final value pointed to needs to be instantiated as a schema.
when validate_schema is on, it validates the given json document, not the schema at the pointer. it should not assume that the given value is a json schema, only that the value at the pointer is.
strings containing escaped values, either URI escaped in the fragment, or escaped with ~0 / ~1 as defined in https://tools.ietf.org/html/rfc6901 are not handled correctly.

I made a class JSON::Schema::Pointer to abstract the parsing and evaluation of pointers, and updated other code around this.

notEthan · 2017-05-17T18:17:02Z

lib/json-schema/schema.rb

@@ -6,6 +6,8 @@ class Schema
    attr_accessor :schema, :uri, :validator

    def initialize(schema,uri,parent_validator=nil)
+      raise ArgumentError, "schema must be a hash; got: #{schema.inspect}" unless schema.is_a?(Hash)


this change is kind of unrelated, but it results in a better error message when an array is passed as the schema argument (as the previous fragment-handling code was prone to do) - before the error message would end up being TypeError: no implicit conversion of String into Integer when trying to call @schema['id'] below, which is rather obscure.

I'm trying (slowly) to move the codebase away from explicit type checking towards duck typing. Could you please alter this check from schema.is_a?(Hash) to schema.respond_to?(:to_hash), and the line below from @schema = schema to @schema = schema.to_hash?

notEthan · 2017-05-17T18:17:41Z

lib/json-schema/validator.rb

+      # If the :fragment option is set, try and validate against the fragment
+      if opts[:fragment]
+        @base_schema = schema_from_fragment(@base_schema, opts[:fragment])
+      end


this is moved before the validate_schema block so that the correct schema, at the fragment pointer, is validated

notEthan · 2017-05-17T18:20:52Z

lib/json-schema/pointer.rb

+        if match
+          parse_pointer(match.post_match)
+        else
+          raise(PointerSyntaxError, "Invalid fragment syntax in #{fragment.inspect}: fragment must begin with #")


I've also tried to improve the information given in these error messages, as the previous "Invalid fragment resolution for :fragment option" was not very useful to debug

notEthan · 2017-05-17T18:24:38Z

lib/json-schema/validator.rb

    end

    def schema_from_fragment(base_schema, fragment)
      schema_uri = base_schema.uri
-      fragments = fragment.split("/")


another fix I just noticed: this should be split("/", -1); as it is empty string keys would be mishandled. #/foo// should parse to fragments ['foo', '', ''] and in json {"foo": {"": {"": 7}}} should evaluate to 7.

notEthan · 2017-05-17T23:14:13Z

added #383 and rebased this against it

notEthan · 2017-06-16T07:32:08Z

this thing alive?

iainbeeston · 2017-06-16T08:14:43Z

@notEthan Sorry, I've been immensely busy at work and at home for several weeks. I'll take a proper look at this once things calm down here

notEthan · 2017-06-16T19:16:15Z

sounds good, thanks

iainbeeston

I really like this. It seems to be mostly refactoring with a few targetted changes?

I've requested a few small changes. Could you also please add those bug-fixes to the CHANGELOG.md? (Ideally in the same commits as the code changes they refer to)

I believe there are other places in the code where we use pointers but I'm happy to update those later myself (unless you have time to do it yourself, in which case I'd be very grateful!)

iainbeeston · 2017-07-03T06:44:26Z

lib/json-schema/schema.rb

@@ -6,6 +6,8 @@ class Schema
    attr_accessor :schema, :uri, :validator

    def initialize(schema,uri,parent_validator=nil)
+      raise ArgumentError, "schema must be a hash; got: #{schema.inspect}" unless schema.is_a?(Hash)


I'm trying (slowly) to move the codebase away from explicit type checking towards duck typing. Could you please alter this check from schema.is_a?(Hash) to schema.respond_to?(:to_hash), and the line below from @schema = schema to @schema = schema.to_hash?

iainbeeston · 2017-07-03T06:47:22Z

lib/json-schema/pointer.rb

+      # - :reference_tokens - the representation is an array of tokens referencing a path in a document
+      def initialize(type, representation)
+        @type = type
+        if type == :reference_tokens


I can't see anywhere where this is used. Is it needed? (And if not we can remove the last clause in representation_s too)

I used it in code that I have been working on that relies on this Pointer class - I use the array of reference tokens there. dealing with the string (pointer or fragment) in code is less useful than the tokens. I'd like this to be part of the API of this class.

iainbeeston · 2017-07-03T06:54:04Z

lib/json-schema/pointer.rb

+        "#<#{self.class.name} #{@type} = #{representation_s}>"
+      end
+
+      def representation_s


Could we make this and the methods below private, to remove them from the public api of the class?

I've made this method private but #pointer and #fragment should be public. I've added method documents for those.

iainbeeston · 2017-07-03T07:37:25Z

lib/json-schema/pointer.rb

+      # - :fragment - the representation is a fragment containing a pointer (starting with #)
+      # - :pointer - the representation is a pointer (starting with /)
+      # - :reference_tokens - the representation is an array of tokens referencing a path in a document
+      def initialize(type, representation)


Actually, the more I think about this the more I feel that this aught to be refactored from a single Pointer class, into a Pointer parent class and a subclass for each type (eg. FragmentPointer and AbsolutePointer maybe?). Right now most instance methods work differently based on the type, and it feels like an instance of the Switch Statements Smell, where polymorphism would work better.

I started down this path but on further reflection I think I disagree. the class represents the same sort of pointer no matter what the external representation it came from. the only method that is any different, once the external representation has been parsed, is #to_s and that is only to keep track of where it came from - it's not really a part of what the Pointer is, just something I found useful for debugging.

iainbeeston · 2017-07-05T17:49:31Z

You might need to rebase this, as I've merged a fragment-related bugfix (#217)

notEthan · 2018-03-12T23:57:18Z

oh my, what a long time I have neglected this. back to it. rebased and addressed some comments.

notEthan · 2018-03-13T00:07:21Z

if you agree that a Pointer is a Pointer, I hope this is ready to be merged. if you feel strongly that it should be polymorphic, I can do that.

notEthan · 2018-03-13T01:46:34Z

hmm. test failures on jruby that I'm not getting locally. one looks related to this change, others don't. will address what I can.

…e schema of the root is incorrect; it is correct to validate the schema located at the fragment.

… can't be instantiated as a JSON::Schema

…rs that must be escaped in json pointer syntax

… dealing with pointers in fragments or in plain pointer syntax. use this in #schema_from_fragment.

…chema#initialize, use #to_hash

notEthan · 2018-03-13T02:29:56Z

I put a commit fixing a require that was missing, causing failures on jruby, ahead of the content of this PR, it can be merged together or separately

notEthan · 2018-03-25T09:10:04Z

ping on this, hopefully you have time to look at it and we can bring it home

skryukov · 2019-03-17T21:39:43Z

Hi @iainbeeston thanks for the great gem! Could you please look at this PR again? Keys with slashes are all over the place in the swagger specs =)

scalp42 · 2019-06-03T05:04:44Z

@iainbeeston is it still maintened? Would it be possible to add more contributors as a lot of PRs are sitting? Thanks!

iainbeeston · 2019-06-04T10:31:31Z

@scalp42 I'm afraid that the gem does not have any active maintainers at the moment. If you're interested in becoming one please comment on #423

notEthan commented May 17, 2017

View reviewed changes

notEthan force-pushed the fragments branch from cd18648 to 80e36d3 Compare May 17, 2017 23:13

notEthan force-pushed the fragments branch from 80e36d3 to 8a71cc8 Compare May 30, 2017 08:12

iainbeeston suggested changes Jul 3, 2017

View reviewed changes

iainbeeston mentioned this pull request Jul 3, 2017

Fix validator when the specified fragment includes array #217

Closed

iainbeeston reviewed Jul 3, 2017

View reviewed changes

notEthan force-pushed the fragments branch from 8a71cc8 to c81c026 Compare March 12, 2018 23:53

notEthan force-pushed the fragments branch from c81c026 to 85d75de Compare March 13, 2018 00:03

notEthan force-pushed the fragments branch 3 times, most recently from 3a6d994 to 127e482 Compare March 13, 2018 02:24

notEthan added 8 commits March 12, 2018 19:27

fix missing require for caching test

e27c87c

test fragment resolution with a deep array

d77ac9c

dereference the fragment before validating the schema - validating th…

4e2f7f0

…e schema of the root is incorrect; it is correct to validate the schema located at the fragment.

fix resolution of fragments where part of the path is an array, which…

fb847d0

… can't be instantiated as a JSON::Schema

add test for fragment resolution where the fragment contains characte…

a0779be

…rs that must be escaped in json pointer syntax

add JSON::Schema::Pointer class to abstract handling of JSON pointer,…

c6d82d2

… dealing with pointers in fragments or in plain pointer syntax. use this in #schema_from_fragment.

improve error message when a schema that is not a hash is passed to S…

8cf57b5

…chema#initialize, use #to_hash

test JSON::Schema::Pointer

d020ff2

notEthan added 3 commits March 12, 2018 19:27

add a test of json schema pointer including a number of empty strings

5eba4fc

test precise error classes raised in fragment_resolution_test

0928ee6

add fragment fixes to changelog

08e266d

notEthan force-pushed the fragments branch from 127e482 to 08e266d Compare March 13, 2018 02:29

notEthan added a commit to notEthan/scorpio that referenced this pull request Apr 7, 2018

monkey-patch application of voxpupuli/json-schema#382 until its merge

cfa0a95

notEthan added a commit to notEthan/scorpio that referenced this pull request Apr 8, 2018

monkey-patch application of voxpupuli/json-schema#382 until its merge

2f6c5a6

notEthan added a commit to notEthan/scorpio that referenced this pull request Apr 11, 2018

monkey-patch application of voxpupuli/json-schema#382 until its merge

e634d4e

notEthan added a commit to notEthan/scorpio that referenced this pull request Apr 12, 2018

monkey-patch application of voxpupuli/json-schema#382 until its merge

4e08505

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve handling of fragments #382

improve handling of fragments #382

notEthan commented May 17, 2017

notEthan May 17, 2017

iainbeeston Jul 3, 2017

notEthan May 17, 2017

notEthan May 17, 2017

notEthan May 17, 2017 •

edited

Loading

notEthan commented May 17, 2017

notEthan commented Jun 16, 2017

iainbeeston commented Jun 16, 2017

notEthan commented Jun 16, 2017

iainbeeston left a comment

iainbeeston Jul 3, 2017

iainbeeston Jul 3, 2017

notEthan Mar 12, 2018

iainbeeston Jul 3, 2017

notEthan Mar 13, 2018

iainbeeston Jul 3, 2017

notEthan Mar 13, 2018

iainbeeston commented Jul 5, 2017

notEthan commented Mar 12, 2018

notEthan commented Mar 13, 2018

notEthan commented Mar 13, 2018

notEthan commented Mar 13, 2018

notEthan commented Mar 25, 2018

skryukov commented Mar 17, 2019

scalp42 commented Jun 3, 2019

iainbeeston commented Jun 4, 2019

improve handling of fragments #382

Are you sure you want to change the base?

improve handling of fragments #382

Conversation

notEthan commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notEthan May 17, 2017 • edited Loading

Choose a reason for hiding this comment

notEthan commented May 17, 2017

notEthan commented Jun 16, 2017

iainbeeston commented Jun 16, 2017

notEthan commented Jun 16, 2017

iainbeeston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iainbeeston commented Jul 5, 2017

notEthan commented Mar 12, 2018

notEthan commented Mar 13, 2018

notEthan commented Mar 13, 2018

notEthan commented Mar 13, 2018

notEthan commented Mar 25, 2018

skryukov commented Mar 17, 2019

scalp42 commented Jun 3, 2019

iainbeeston commented Jun 4, 2019

notEthan May 17, 2017 •

edited

Loading