Show attribute name when JSON::ParseException occurs #4873

kenta-s · 2017-08-22T22:20:12Z

When JSON.mapping raises a JSON::ParseException, we can see messages like below. Currently it is a bit hard to find which attribute leads to the exception.

Expected bool but was null at 1:866 (JSON::ParseException)
0x4ccad7: *CallStack::unwind:Array(Pointer(Void)) at ??
0x5f456d: parse_exception at /opt/crystal/src/json/pull_parser.cr 495:5
0x5f470e: expect_kind at /opt/crystal/src/json/pull_parser.cr 488:5
0x5f67d3: read_bool at /opt/crystal/src/json/pull_parser.cr 101:5
0x542ab6: new at /opt/crystal/src/json/from_json.cr 66:3

...

This Pull Request is to improve the message like this:

Expected bool but was null for attribute: activated at 1:866 (JSON::ParseException)
0x4ca1e7: *CallStack::unwind:Array(Pointer(Void)) at ??
0x5f1b6d: parse_exception at /home/kenta-s/Repositories/crystal/src/json/pull_parser.cr 518:5
0x5f1d69: expect_kind at /home/kenta-s/Repositories/crystal/src/json/pull_parser.cr 511:5
0x5f3e23: read_bool at /home/kenta-s/Repositories/crystal/src/json/pull_parser.cr 102:5
0x5400c6: new at /home/kenta-s/Repositories/crystal/src/json/from_json.cr 66:3

...

makenowjust · 2017-08-23T03:50:58Z

src/json/pull_parser.cr

@@ -507,7 +507,7 @@ class JSON::PullParser
  end

  private def expect_kind(kind)
-    parse_exception "Expected #{kind} but was #{@kind}" unless @kind == kind
+    parse_exception "Expected #{kind} but was #{@kind} for attribute: #{@string_value}" unless @kind == kind


This implementation is too weak. For example this result is your expected?

require "json" class Foo JSON.mapping({ foo: Int32, }) def initialize(@foo) end end Foo.from_json(%{{"foo": "bar"}}) # raises: Expected int but was string for attribute: bar at 1:14 (JSON::ParseException)

I think you can add attribute_name argument to expect_kind and each read_xxx method, then use it to show attribute name inside JSON.mapping.

this result is your expected?

No, definitely not...
Thank you for pointing out the problem and showing me the better way. Let me try to fix it :)

asterite · 2017-08-26T03:18:39Z

The improvement should be on the mapping macro

kenta-s · 2017-08-26T12:10:55Z

@asterite Yeah, thank you :)

@makenowjust
To be honest I am being scared of adding a new argument to expect_kind because I realized the effect of change would be significant(I tried though...). So I just decided to add a new property attribute_name to JSON::PullParser.
https://github.com/crystal-lang/crystal/pull/4873/files#diff-eaf20134a0f1c47a6c6f2e7e0f072e94R11

Does this sound good to you?

makenowjust · 2017-08-26T14:27:21Z

@kenta-s

Current implementation cannot work in this case:

require "json"

class Foo
  JSON.mapping({
    foo: Int32 | String,
  })
end

begin
  foo = Foo.from_json(%{{"foo": true}})
rescue e
  pp e.to_s # => "Couldn't parse (Int32 | String) from true at 1:9"
end

And it is broken by Hash or NamedTuple:

require "json"

alias Foo = {
  foo: String,
}

begin
  foo = Foo.from_json(%{{"foo": 1}})
rescue e
  pp e.to_s # => "Expected string but was int for attribute:  at 1:10"
end

begin
  foo = Hash(String, String).from_json(%{{"foo": 1}})
rescue e
  pp e.to_s # => "Expected string but was int for attribute:  at 1:10"
end

Also, modifying PullParser seems ugly to me.

I have an idea to implement this without modifying PullParser.

add attribute_name property to JSON::ParseException (defined in src/json.cr)
catch JSON::ParseException in JSON.mapping
update attribute_name property of this error object.

Perhaps you can get full key trace like "foo.bar[42].baz" by this method, it is awesome. But I think this method is very costed, so Benchmark is important, please do it! (And please write spec for this.)

Good luck!

asterite · 2017-08-26T16:56:56Z

I don't see the difficulty of implementing this:

diff --git a/src/json/mapping.cr b/src/json/mapping.cr
index 68353789f..f1a3fb766 100644
--- a/src/json/mapping.cr
+++ b/src/json/mapping.cr
@@ -112,13 +112,17 @@ module JSON
                 %pull.on_key!({{value[:root]}}) do
               {% end %}
 
-              {% if value[:converter] %}
-                {{value[:converter]}}.from_json(%pull)
-              {% elsif value[:type].is_a?(Path) || value[:type].is_a?(Generic) %}
-                {{value[:type]}}.new(%pull)
-              {% else %}
-                ::Union({{value[:type]}}).new(%pull)
-              {% end %}
+              begin
+                {% if value[:converter] %}
+                  {{value[:converter]}}.from_json(%pull)
+                {% elsif value[:type].is_a?(Path) || value[:type].is_a?(Generic) %}
+                  {{value[:type]}}.new(%pull)
+                {% else %}
+                  ::Union({{value[:type]}}).new(%pull)
+                {% end %}
+              rescue ex : JSON::ParseException
+                raise JSON::ParseException.new("Error parsing attribute #{self.class}.{{key.id}}: #{ex.message}", *%key_location)
+              end
 
               {% if value[:root] %}
                 end

RX14 · 2017-08-26T20:41:04Z

Would be nice if it worked for named tuples too.

kenta-s · 2017-08-28T11:51:23Z

src/json.cr

+        super "#{message} at #{@line_number}:#{@column_number}"
+      else
+        super "#{message}"
+      end


Explanation why this condition is needed:

After JSON::ParseException is rescued, the exception class will be initialized again with original exception message: https://github.com/crystal-lang/crystal/pull/4873/files#diff-3f54210a12cee69b8bbb656af3626edfR124

This would create an exception message like:

Error parsing attribute JSONPerson.name: Expected string but was int at 2:13 at 2:13

After the exception is rescued, the location should not be set again(otherwise location appears twice like above). So I made @line_number and @column_number nilable.

I don't like this. line_number and column_number aren't just for creating that message, otherwise they wouldn't be ivars. They are for machines too and should be set properly. I would introduce an option to the constructor which skips appending these values.

See #4873 (comment). I believe next commit changes this.

It's not fixed currently?

Unfortunately... I got response, but I didn't see such a change. @kenta-s said "I will try to fix that.", so I'm waiting.

Sija · 2017-08-28T11:56:20Z

src/json/from_json.cr

-        pull.skip
+      begin
+        case key
+            {% for key, type in T %}


Wrong indentation here.

makenowjust · 2017-08-28T11:58:50Z

src/json/from_json.cr

+          pull.skip
+        end
+      rescue ex : JSON::ParseException
+        raise ::JSON::ParseException.new("Error parsing attribute #{key} of #{self}: #{ex.message}")


Is this really expected?

require "json" class Foo JSON.mapping({ foo: Bar }) end class Bar JSON.mapping({ bar: Int32 }) end begin foo = Foo.from_json(%q{{"foo": {"bar": "baz"}}}) rescue e pp e.to_s # => "Error parsing attribute Foo.foo: Error parsing attribute Bar.bar: Expected int but was string at 1:22" end

And so, I suggested to add attribute_name property to JSON::ParseException to detect such duplication, but I didn't explain this, sorry.

I didn't consider that case... I will try to fix that. Thank you again :)

And so, I suggested to add attribute_name property to JSON::ParseException to detect such duplication, but I didn't explain this, sorry.

Ah, it makes sense.

asterite · 2017-08-29T16:08:27Z

As a side note, this small improvement is probably not needed: the error includes the line and column number, and together with the source code you can understand what's wrong.

RX14 · 2017-08-29T16:35:59Z

@asterite I think I would find it quite useful when debugging and the diff really isn't that big. (at least with ?w=1)

kenta-s · 2017-08-30T00:06:15Z

sorry for the late reply. I updated a few things you guys pointed out, but one more thing needs to be done: #4873 (comment)
I looked into why this happens but I still have no idea. I need more time sorry :(

As a side note, this small improvement is probably not needed: the error includes the line and column number, and together with the source code you can understand what's wrong.

Yeah, this improvement is not something has to be done. Just making a bit more developer-friendly. Feel free to close this PR if you decide Crystal does not need this :)

straight-shoota · 2018-07-02T19:14:42Z

This PR was superseded by #5932 with a more detailed implementation. Close?

kenta-s changed the title ~~Improve JSON::ParseException message~~ Show attribute name when JSON::ParseException occurs Aug 23, 2017

makenowjust suggested changes Aug 23, 2017

View reviewed changes

kenta-s force-pushed the make-error-message-better branch 3 times, most recently from bb90632 to 303ae23 Compare August 26, 2017 12:02

kenta-s force-pushed the make-error-message-better branch from 303ae23 to a333ddb Compare August 28, 2017 11:13

kenta-s commented Aug 28, 2017

View reviewed changes

Sija reviewed Aug 28, 2017

View reviewed changes

src/json/from_json.cr Outdated

pull.skip

begin

case key

{% for key, type in T %}

Copy link

Contributor

Sija Aug 28, 2017

Choose a reason for hiding this comment

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

Wrong indentation here.

makenowjust reviewed Aug 28, 2017

View reviewed changes

kenta-s force-pushed the make-error-message-better branch from a333ddb to 55cf2e6 Compare August 28, 2017 23:16

Improve parse exception message

e80a0e4

kenta-s force-pushed the make-error-message-better branch from 55cf2e6 to e80a0e4 Compare August 29, 2017 23:55

RX14 closed this Jul 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show attribute name when JSON::ParseException occurs #4873

Show attribute name when JSON::ParseException occurs #4873

kenta-s commented Aug 22, 2017 •

edited

makenowjust Aug 23, 2017

kenta-s Aug 23, 2017

asterite commented Aug 26, 2017

kenta-s commented Aug 26, 2017 •

edited

makenowjust commented Aug 26, 2017

asterite commented Aug 26, 2017

RX14 commented Aug 26, 2017

kenta-s Aug 28, 2017

RX14 Aug 29, 2017 •

edited

makenowjust Aug 29, 2017

RX14 Aug 29, 2017

makenowjust Aug 29, 2017

Sija Aug 28, 2017

makenowjust Aug 28, 2017

kenta-s Aug 28, 2017

asterite commented Aug 29, 2017

RX14 commented Aug 29, 2017 •

edited

kenta-s commented Aug 30, 2017

straight-shoota commented Jul 2, 2018

Show attribute name when JSON::ParseException occurs #4873

Show attribute name when JSON::ParseException occurs #4873

Conversation

kenta-s commented Aug 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Aug 26, 2017

kenta-s commented Aug 26, 2017 • edited

makenowjust commented Aug 26, 2017

asterite commented Aug 26, 2017

RX14 commented Aug 26, 2017

Choose a reason for hiding this comment

RX14 Aug 29, 2017 • edited

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

asterite commented Aug 29, 2017

RX14 commented Aug 29, 2017 • edited

kenta-s commented Aug 30, 2017

straight-shoota commented Jul 2, 2018

kenta-s commented Aug 22, 2017 •

edited

kenta-s commented Aug 26, 2017 •

edited

RX14 Aug 29, 2017 •

edited

RX14 commented Aug 29, 2017 •

edited