List which keys are erroring

[gjtorikian]

This PR updates the error messaging to note which property a schema fails for.

We had an interesting case where a user tried to submit some data, and received the stark error message of \nnil is not a string.\nnil is not a string.. The problem was that the schema was violated in two places, but for the user, nil is not a string didn't really help them identify either one. It looked like a duplicated error in the message output.

This PR simply appends the final part of the schema pointer alongside the rest of the message. In the event that the schema is validating with a pattern, we need to travel up a bit to get the actual key name.

Thoughts?

[brandur]

@gjtorikian Thanks! I left a couple minor comments, but more generally: IMO, more information is rarely a bad thing. +1.

[gjtorikian]

@brandur Ok, I lied about not needing split_pointer, so I've turned it into a variable and applied the rest of your feedback.

[brandur]

@gjtorikian Thanks!

What do you think about this amendment:

diff --git a/lib/json_schema/schema.rb b/lib/json_schema/schema.rb
index 084610f..f376f73 100644
--- a/lib/json_schema/schema.rb
+++ b/lib/json_schema/schema.rb
@@ -23,9 +23,6 @@ module JsonSchema
     # schema for debugging.
     attr_accessor :fragment

-    # An array that represents the nested path of the final JSON Pointer.
-    attr_accessor :split_pointer
-
     # Rather than a normal schema, the node may be a JSON Reference. In this
     # case, no other attributes will be filled in except for #parent.
     attr_accessor :reference
@@ -270,10 +267,6 @@ module JsonSchema
       end
     end

-    def split_pointer
-      @split_pointer ||= pointer.split("/")
-    end
-
     def validate(data)
       validator = Validator.new(self)
       valid = validator.validate(data)
diff --git a/lib/json_schema/validator.rb b/lib/json_schema/validator.rb
index 32c2483..c584d6f 100644
--- a/lib/json_schema/validator.rb
+++ b/lib/json_schema/validator.rb
@@ -508,13 +508,18 @@ module JsonSchema
       else
         fragment = schema.fragment
         key = if fragment =~ /patternProperties/
-                idx = schema.split_pointer.index('patternProperties')
-                # this join mimics the fragment format below in that it's
-                # parent + key
-                schema.split_pointer[idx - 2] + "/" + schema.split_pointer[idx - 1]
-              else
-                fragment
-              end
+          split_pointer = schema.pointer.split("/").reverse
+          idx = split_pointer.index('patternProperties')
+
+          # this join mimics the fragment format below in that it's
+          # parent + key
+          parts = split_pointer[(idx + 1)..(idx + 2)]
+
+          # protect against a `nil` that could occur if
+          # `patternProperties` has no parent
+          parts ? parts.compact.reverse.join("/") : nil
+        end
+        key = fragment if key.nil?
         message = %{For '#{key}', #{data.inspect} is not #{ErrorFormatter.to_list(schema.type)}.}
         errors << ValidationError.new(schema, path, message, :invalid_type)
         false

Most of the changes are there to protect against a degenerate case where patternProperties is located in a top-level schema and where we might otherwise try to reach up the tree to nodes that don't exist.

[gjtorikian]

Looks good to me! Three questions:

1. Any reason why split_pointer on schema.rb was dropped?
2. Can you think of a schema where the patternProperties' parent would be nil? I'd like to add a test. 😀
3. I made a slight change to the patch in 17975d3 because the idea of reversing twice seemed icky to me. I made it a separate commit in case you'd prefer that to be reverted. What do you think?

Thanks!

[brandur]

@gjtorikian

Any reason why split_pointer on schema.rb was dropped?

Mostly just because it's not really needed by any other features. It kind of make sense to me to try and minimize the public interface that you expose until you have to widen it? What do you think?

Can you think of a schema where the patternProperties' parent would be nil? I'd like to add a test. :grinning:

Good call! I would think that just a top-level patternProperties would allow you to reproduce. Take the following for example:

{
  "patternProperties": {
    "^\\w+$": {
      "type": [
        "string"
      ]
    }
  }
}

I made a slight change to the patch in 17975d3 because the idea of reversing twice seemed icky to me. I made it a separate commit in case you'd prefer that to be reverted. What do you think?

Fair enough! Looks fine as you have it (without the reversals).

[gjtorikian]

What do you think?

Makes sense to me!

[brandur]

Thanks again!

[brandur]

Released as 0.8.0.

[gjtorikian]

:heart: