-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Show attribute name when JSON::ParseException occurs #4873
Conversation
src/json/pull_parser.cr
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
The improvement should be on the mapping macro |
bb90632
to
303ae23
Compare
@asterite Yeah, thank you :) @makenowjust Does this sound good to you? |
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 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 I have an idea to implement this without modifying
Perhaps you can get full key trace like Good luck! |
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 |
Would be nice if it worked for named tuples too. |
303ae23
to
a333ddb
Compare
src/json.cr
Outdated
super "#{message} at #{@line_number}:#{@column_number}" | ||
else | ||
super "#{message}" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #4873 (comment). I believe next commit changes this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not fixed currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/json/from_json.cr
Outdated
pull.skip | ||
begin | ||
case key | ||
{% for key, type in T %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong indentation here.
src/json/from_json.cr
Outdated
pull.skip | ||
end | ||
rescue ex : JSON::ParseException | ||
raise ::JSON::ParseException.new("Error parsing attribute #{key} of #{self}: #{ex.message}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
a333ddb
to
55cf2e6
Compare
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. |
@asterite I think I would find it quite useful when debugging and the diff really isn't that big. (at least with |
55cf2e6
to
e80a0e4
Compare
sorry for the late reply. I updated a few things you guys pointed out, but one more thing needs to be done: #4873 (comment)
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 :) |
This PR was superseded by #5932 with a more detailed implementation. Close? |
When
JSON.mapping
raises aJSON::ParseException
, we can see messages like below. Currently it is a bit hard to find which attribute leads to the exception.This Pull Request is to improve the message like this: