From 6a1ddd6a7eb727a8af798459b4609328f8bdb1d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Mon, 9 Apr 2018 13:13:34 +0000 Subject: [PATCH 1/2] Add context to errors in JSON.mapping --- spec/std/json/mapping_spec.cr | 28 ++++++++++-- src/json.cr | 4 +- src/json/mapping.cr | 81 ++++++++++++++++++++++++++--------- 3 files changed, 87 insertions(+), 26 deletions(-) diff --git a/spec/std/json/mapping_spec.cr b/spec/std/json/mapping_spec.cr index 672bd2234a00..15742cea550f 100644 --- a/spec/std/json/mapping_spec.cr +++ b/spec/std/json/mapping_spec.cr @@ -221,7 +221,7 @@ describe "JSON mapping" do end it "parses strict person with unknown attributes" do - ex = expect_raises JSON::ParseException, "Unknown json attribute: foo" do + ex = expect_raises JSON::MappingError, "Unknown JSON attribute: foo\n parsing StrictJSONPerson" do StrictJSONPerson.from_json <<-JSON { "name": "John", @@ -234,12 +234,34 @@ describe "JSON mapping" do end it "raises if non-nilable attribute is nil" do - ex = expect_raises JSON::ParseException, "Missing json attribute: name" do + ex = expect_raises JSON::MappingError, "Missing JSON attribute: name\n parsing JSONPerson at 1:1" do JSONPerson.from_json(%({"age": 30})) end ex.location.should eq({1, 1}) end + it "raises if not an object" do + ex = expect_raises JSON::MappingError, "Expected begin_object but was string at 1:1\n parsing StrictJSONPerson at 0:0" do + StrictJSONPerson.from_json <<-JSON + "foo" + JSON + end + ex.location.should eq({1, 1}) + end + + it "raises if data type does not match" do + ex = expect_raises JSON::MappingError, "Expected int but was string at 3:15\n parsing StrictJSONPerson#age at 3:3" do + StrictJSONPerson.from_json <<-JSON + { + "name": "John", + "age": "foo", + "foo": "bar" + } + JSON + end + ex.location.should eq({3, 15}) + end + it "doesn't emit null by default when doing to_json" do person = JSONPerson.from_json(%({"name": "John"})) (person.to_json =~ /age/).should be_falsey @@ -523,7 +545,7 @@ describe "JSON mapping" do end it "raises if non-nilable attribute is nil" do - ex = expect_raises JSON::ParseException, "Missing json attribute: foo" do + ex = expect_raises JSON::MappingError, "Missing JSON attribute: foo\n parsing JSONWithQueryAttributes at 1:1" do JSONWithQueryAttributes.from_json(%({"is_bar": true})) end ex.location.should eq({1, 1}) diff --git a/src/json.cr b/src/json.cr index dee3dbae8d61..79143b407575 100644 --- a/src/json.cr +++ b/src/json.cr @@ -66,8 +66,8 @@ module JSON getter line_number : Int32 getter column_number : Int32 - def initialize(message, @line_number, @column_number) - super "#{message} at #{@line_number}:#{@column_number}" + def initialize(message, @line_number, @column_number, cause = nil) + super "#{message} at #{@line_number}:#{@column_number}", cause end def location diff --git a/src/json/mapping.cr b/src/json/mapping.cr index 1e0d8c9b8351..e1b2383f9a01 100644 --- a/src/json/mapping.cr +++ b/src/json/mapping.cr @@ -101,7 +101,11 @@ module JSON {% end %} %location = %pull.location - %pull.read_begin_object + begin + %pull.read_begin_object + rescue exc : ::JSON::ParseException + raise ::JSON::MappingError.new(self.class, *%location, cause: exc) + end while %pull.kind != :end_object %key_location = %pull.location key = %pull.read_object_key @@ -109,32 +113,34 @@ module JSON {% for key, value in _properties_ %} when {{value[:key] || value[:key_id].stringify}} %found{key.id} = true + begin + %var{key.id} = + {% if value[:nilable] || value[:default] != nil %} %pull.read_null_or { {% end %} - %var{key.id} = - {% if value[:nilable] || value[:default] != nil %} %pull.read_null_or { {% end %} - - {% if value[:root] %} - %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 %} + {% if value[:root] %} + %pull.on_key!({{value[:root]}}) do + {% end %} - {% if value[:root] %} - end - {% 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 %} - {% if value[:nilable] || value[:default] != nil %} } {% end %} + {% if value[:root] %} + end + {% end %} + {% if value[:nilable] || value[:default] != nil %} } {% end %} + rescue exc : ::JSON::ParseException + raise ::JSON::MappingError.new(self.class, {{value[:key] || value[:key_id].stringify}}, *%key_location, cause: exc) + end {% end %} else {% if strict %} - raise ::JSON::ParseException.new("Unknown json attribute: #{key}", *%key_location) + raise ::JSON::MappingError.new("Unknown JSON attribute: #{key}", self.class, *%key_location) {% else %} %pull.skip {% end %} @@ -145,7 +151,7 @@ module JSON {% for key, value in _properties_ %} {% unless value[:nilable] || value[:default] != nil %} if %var{key.id}.nil? && !%found{key.id} && !::Union({{value[:type]}}).nilable? - raise ::JSON::ParseException.new("Missing json attribute: {{(value[:key] || value[:key_id]).id}}", *%location) + raise ::JSON::MappingError.new("Missing JSON attribute: {{(value[:key] || value[:key_id]).id}}", self.class, *%location) end {% end %} @@ -220,4 +226,37 @@ module JSON macro mapping(**_properties_) ::JSON.mapping({{_properties_}}) end + + class MappingError < ParseException + getter klass : String + getter attribute : String? + + def self.new(klass : Class, line_number, column_number, *, cause : JSON::ParseException) + new(cause.message, klass, nil, line_number, column_number, cause: cause) + end + + def self.new(klass : Class, attribute, line_number, column_number, *, cause : JSON::ParseException) + new(cause.message, klass, attribute, line_number, column_number, cause: cause) + end + + def self.new(message : String?, klass : String | Class, line_number : Int32, column_number : Int32, cause = nil) + new(message, klass, nil, line_number, column_number, cause) + end + + def initialize(message : String?, klass : String | Class, @attribute : String?, line_number : Int32, column_number : Int32, cause = nil) + message = String.build do |io| + io << message + io << "\n parsing " + io << klass + if attribute = @attribute + io << '#' << attribute + end + end + super(message, line_number, column_number, cause) + if cause + @line_number, @column_number = cause.location + end + @klass = klass.to_s + end + end end From 98fd9eb90fa85575e74157dd7a4af61aa2254170 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Sat, 21 Apr 2018 18:23:38 +0000 Subject: [PATCH 2/2] fixup! Add context to errors in JSON.mapping --- spec/std/json/mapping_spec.cr | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/spec/std/json/mapping_spec.cr b/spec/std/json/mapping_spec.cr index 15742cea550f..60bd6047bc74 100644 --- a/spec/std/json/mapping_spec.cr +++ b/spec/std/json/mapping_spec.cr @@ -221,7 +221,11 @@ describe "JSON mapping" do end it "parses strict person with unknown attributes" do - ex = expect_raises JSON::MappingError, "Unknown JSON attribute: foo\n parsing StrictJSONPerson" do + error_message = <<-'MSG' + Unknown JSON attribute: foo + parsing StrictJSONPerson + MSG + ex = expect_raises JSON::MappingError, error_message do StrictJSONPerson.from_json <<-JSON { "name": "John", @@ -234,14 +238,22 @@ describe "JSON mapping" do end it "raises if non-nilable attribute is nil" do - ex = expect_raises JSON::MappingError, "Missing JSON attribute: name\n parsing JSONPerson at 1:1" do + error_message = <<-'MSG' + Missing JSON attribute: name + parsing JSONPerson at 1:1 + MSG + ex = expect_raises JSON::MappingError, error_message do JSONPerson.from_json(%({"age": 30})) end ex.location.should eq({1, 1}) end it "raises if not an object" do - ex = expect_raises JSON::MappingError, "Expected begin_object but was string at 1:1\n parsing StrictJSONPerson at 0:0" do + error_message = <<-'MSG' + Expected begin_object but was string at 1:1 + parsing StrictJSONPerson at 0:0 + MSG + ex = expect_raises JSON::MappingError, error_message do StrictJSONPerson.from_json <<-JSON "foo" JSON @@ -250,7 +262,11 @@ describe "JSON mapping" do end it "raises if data type does not match" do - ex = expect_raises JSON::MappingError, "Expected int but was string at 3:15\n parsing StrictJSONPerson#age at 3:3" do + error_message = <<-'MSG' + Expected int but was string at 3:15 + parsing StrictJSONPerson#age at 3:3 + MSG + ex = expect_raises JSON::MappingError, error_message do StrictJSONPerson.from_json <<-JSON { "name": "John", @@ -545,7 +561,11 @@ describe "JSON mapping" do end it "raises if non-nilable attribute is nil" do - ex = expect_raises JSON::MappingError, "Missing JSON attribute: foo\n parsing JSONWithQueryAttributes at 1:1" do + error_message = <<-'MSG' + Missing JSON attribute: foo + parsing JSONWithQueryAttributes at 1:1 + MSG + ex = expect_raises JSON::MappingError, error_message do JSONWithQueryAttributes.from_json(%({"is_bar": true})) end ex.location.should eq({1, 1})