From ea27928591bf753de4a14e4ee0e86e293fcbfd1c Mon Sep 17 00:00:00 2001 From: avivpxi <42111576+avivpxi@users.noreply.github.com> Date: Mon, 3 Jul 2023 10:04:11 +0300 Subject: [PATCH] add error handling in JSONDataHandler (#28) --- CHANGELOG.md | 6 ++ README.md | 4 +- example_test.go | 3 +- options.go | 21 +++++- unmarshal.go | 13 ++-- unmarshal_from_json_map.go | 13 ++-- unmarshal_from_json_map_test.go | 81 ++++++++++++++++++++++- unmarshal_test.go | 110 ++++++++++++++++++++++++++++++-- 8 files changed, 230 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a331a4d..92937d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [[1.1.5](https://github.com/PerimeterX/marshmallow/compare/v1.1.4...v1.1.5)] - 2023-07-03 + +### Added + +- Support for reporting errors from `HandleJSONData` - [info](https://github.com/PerimeterX/marshmallow/issues/27). + ## [[1.1.4](https://github.com/PerimeterX/marshmallow/compare/v1.1.3...v1.1.4)] - 2022-11-10 ### Fixed diff --git a/README.md b/README.md index 1c866fd..bfa9036 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ [![Run Tests](https://img.shields.io/github/actions/workflow/status/perimeterx/marshmallow/go.yml?branch=main&logo=github&label=Run%20Tests)](https://github.com/PerimeterX/marshmallow/actions/workflows/go.yml?query=branch%3Amain) [![Dependency Review](https://img.shields.io/github/actions/workflow/status/perimeterx/marshmallow/dependency-review.yml?logo=github&label=Dependency%20Review)](https://github.com/PerimeterX/marshmallow/actions/workflows/dependency-review.yml?query=branch%3Amain) [![Go Report Card](https://goreportcard.com/badge/github.com/perimeterx/marshmallow)](https://goreportcard.com/report/github.com/perimeterx/marshmallow) -![Manual Code Coverage](https://img.shields.io/badge/coverage-92.4%25-green) +![Manual Code Coverage](https://img.shields.io/badge/coverage-92.6%25-green) [![Go Reference](https://pkg.go.dev/badge/github.com/perimeterx/marshmallow.svg)](https://pkg.go.dev/github.com/perimeterx/marshmallow) [![Licence](https://img.shields.io/github/license/perimeterx/marshmallow)](LICENSE) [![Latest Release](https://img.shields.io/github/v/release/perimeterx/marshmallow)](https://github.com/PerimeterX/marshmallow/releases) @@ -177,7 +177,7 @@ While unmarshalling, marshmallow supports the following optional options: * Excluding known fields from the result map using the [WithExcludeKnownFieldsFromMap](https://github.com/PerimeterX/marshmallow/blob/457669ae9973895584f2636eabfc104140d3b700/options.go#L50) function. * Skipping struct population to boost performance using the [WithSkipPopulateStruct](https://github.com/PerimeterX/marshmallow/blob/0e0218ab860be8a4b5f57f5ff239f281c250c5da/options.go#L41) function. -In order to capture unknown nested fields, structs must implement [JSONDataHandler](https://github.com/PerimeterX/marshmallow/blob/2d254bf2ed5f9b02cafb8ba6eaa726cba38bc92b/options.go#L65). +In order to capture unknown nested fields, structs must implement [JSONDataErrorHandler](https://github.com/PerimeterX/marshmallow/blob/195c994aa6e3e0852601ad9cf65bcddef0dd7479/options.go#L76). More info [here](https://github.com/PerimeterX/marshmallow/issues/15). Marshmallow also supports caching of refection information using diff --git a/example_test.go b/example_test.go index a35cbfa..8741988 100644 --- a/example_test.go +++ b/example_test.go @@ -248,6 +248,7 @@ type childStruct struct { Data map[string]interface{} `json:"-"` } -func (c *childStruct) HandleJSONData(data map[string]interface{}) { +func (c *childStruct) HandleJSONData(data map[string]interface{}) error { c.Data = data + return nil } diff --git a/options.go b/options.go index 423aa9a..ff97d33 100644 --- a/options.go +++ b/options.go @@ -69,9 +69,28 @@ func buildUnmarshalOptions(options []UnmarshalOption) *unmarshalOptions { return result } -// JSONDataHandler allow types to handle JSON data as maps. +// JSONDataErrorHandler allow types to handle JSON data as maps. // Types should implement this interface if they wish to act on the map representation of parsed JSON input. // This is mainly used to allow nested objects to capture unknown fields and leverage marshmallow's abilities. +// If HandleJSONData returns an error, it will be propagated as an unmarshal error +type JSONDataErrorHandler interface { + HandleJSONData(data map[string]interface{}) error +} + +// Deprecated: use JSONDataErrorHandler instead type JSONDataHandler interface { HandleJSONData(data map[string]interface{}) } + +func asJSONDataHandler(value interface{}) (func(map[string]interface{}) error, bool) { + if handler, ok := value.(JSONDataErrorHandler); ok { + return handler.HandleJSONData, true + } + if handler, ok := value.(JSONDataHandler); ok { + return func(m map[string]interface{}) error { + handler.HandleJSONData(m) + return nil + }, true + } + return nil, false +} diff --git a/unmarshal.go b/unmarshal.go index 4eb13d1..160ea30 100644 --- a/unmarshal.go +++ b/unmarshal.go @@ -310,16 +310,21 @@ func (d *decoder) buildStruct(structType reflect.Type) (interface{}, bool) { return d.lexer.Interface(), false } value := reflect.New(structType).Interface() - handler, ok := value.(JSONDataHandler) + handler, ok := asJSONDataHandler(value) if !ok { return d.populateStruct(true, value, nil) } data := make(map[string]interface{}) result, valid := d.populateStruct(true, value, data) - if valid { - handler.HandleJSONData(data) + if !valid { + return result, false } - return result, valid + err := handler(data) + if err != nil { + d.lexer.AddNonFatalError(err) + return result, false + } + return result, true } func (d *decoder) valueFromCustomUnmarshaler(unmarshaler json.Unmarshaler) { diff --git a/unmarshal_from_json_map.go b/unmarshal_from_json_map.go index fcf1696..0907f8f 100644 --- a/unmarshal_from_json_map.go +++ b/unmarshal_from_json_map.go @@ -262,16 +262,21 @@ func (m *mapDecoder) buildStruct(path []string, v interface{}, structType reflec return v, false } value := reflect.New(structType).Interface() - handler, ok := value.(JSONDataHandler) + handler, ok := asJSONDataHandler(value) if !ok { return m.populateStruct(true, path, mp, value, nil) } data := make(map[string]interface{}) result, valid := m.populateStruct(true, path, mp, value, data) - if valid { - handler.HandleJSONData(data) + if !valid { + return result, false } - return result, valid + err := handler(data) + if err != nil { + m.addError(err) + return result, false + } + return result, true } func (m *mapDecoder) valueFromCustomUnmarshaler(data interface{}, unmarshaler UnmarshalerFromJSONMap) { diff --git a/unmarshal_from_json_map_test.go b/unmarshal_from_json_map_test.go index 357b937..366ff31 100644 --- a/unmarshal_from_json_map_test.go +++ b/unmarshal_from_json_map_test.go @@ -2279,7 +2279,11 @@ func TestUnmarshalFromJSONMapJSONDataHandler(t *testing.T) { data := map[string]interface{}{ "known": "foo", "unknown": "boo", - "nested": map[string]interface{}{ + "nested1": map[string]interface{}{ + "known": "goo", + "unknown": "doo", + }, + "nested2": map[string]interface{}{ "known": "goo", "unknown": "doo", }, @@ -2289,7 +2293,80 @@ func TestUnmarshalFromJSONMapJSONDataHandler(t *testing.T) { if err != nil { t.Errorf("unexpected error %v", err) } - _, ok := result["nested"].(handleJSONDataChild) + _, ok := result["nested1"].(handleJSONDataChild) + if !ok { + t.Error("invalid map value") + } + if p.Nested1.Data == nil { + t.Error("HandleJSONData not called") + } + if len(p.Nested1.Data) != 2 || p.Nested1.Data["known"] != "goo" || p.Nested1.Data["unknown"] != "doo" { + t.Error("invalid JSON data") + } + _, ok = result["nested2"].(handleJSONDataChild) + if !ok { + t.Error("invalid map value") + } + if p.Nested2.Data == nil { + t.Error("HandleJSONData not called") + } + if len(p.Nested2.Data) != 2 || p.Nested2.Data["known"] != "goo" || p.Nested2.Data["unknown"] != "doo" { + t.Error("invalid JSON data") + } + }) + t.Run("test_JSONDataHandler_single_error", func(t *testing.T) { + data := map[string]interface{}{ + "known": "foo", + "unknown": "boo", + "nested1": map[string]interface{}{"known": "goo", "unknown": "doo", "fail": true}, + "nested2": map[string]interface{}{"known": "goo", "unknown": "doo", "fail": true}, + } + p := &handleJSONDataParent{} + _, err := UnmarshalFromJSONMap(data, p) + if err == nil { + t.Errorf("expected JSONDataHandler error %v", err) + } + if err.Error() != "HandleJSONData failure" { + t.Errorf("unexpected JSONDataHandler error type %v", err) + } + }) + t.Run("test_JSONDataHandler_multiple_error", func(t *testing.T) { + data := map[string]interface{}{ + "known": "foo", + "unknown": "boo", + "nested1": map[string]interface{}{"known": "goo", "unknown": "doo", "fail": true}, + "nested2": map[string]interface{}{"known": "goo", "unknown": "doo", "fail": true}, + } + p := &handleJSONDataParent{} + _, err := UnmarshalFromJSONMap(data, p, WithMode(ModeAllowMultipleErrors)) + if err == nil { + t.Errorf("expected JSONDataHandler error %v", err) + } + e, ok := err.(*MultipleError) + if !ok { + t.Errorf("unexpected JSONDataHandler error type %v", err) + } + for _, currentError := range e.Errors { + if currentError.Error() != "HandleJSONData failure" { + t.Errorf("unexpected JSONDataHandler error type %v", err) + } + } + }) + t.Run("test_JSONDataHandler_deprecated", func(t *testing.T) { + data := map[string]interface{}{ + "known": "foo", + "unknown": "boo", + "nested": map[string]interface{}{ + "known": "goo", + "unknown": "doo", + }, + } + p := &handleJSONDataDeprecatedParent{} + result, err := UnmarshalFromJSONMap(data, p) + if err != nil { + t.Errorf("unexpected error %v", err) + } + _, ok := result["nested"].(handleJSONDataDeprecatedChild) if !ok { t.Error("invalid map value") } diff --git a/unmarshal_test.go b/unmarshal_test.go index 938476e..c579e34 100644 --- a/unmarshal_test.go +++ b/unmarshal_test.go @@ -2306,8 +2306,9 @@ func TestEmbedding(t *testing.T) { } type handleJSONDataParent struct { - Known string `json:"known"` - Nested handleJSONDataChild `json:"nested"` + Known string `json:"known"` + Nested1 handleJSONDataChild `json:"nested1"` + Nested2 handleJSONDataChild `json:"nested2"` } type handleJSONDataChild struct { @@ -2316,19 +2317,110 @@ type handleJSONDataChild struct { Data map[string]interface{} `json:"-"` } -func (c *handleJSONDataChild) HandleJSONData(data map[string]interface{}) { +func (c *handleJSONDataChild) HandleJSONData(data map[string]interface{}) error { + if _, exists := data["fail"]; exists { + return errors.New("HandleJSONData failure") + } + c.Data = data + return nil +} + +type handleJSONDataDeprecatedParent struct { + Known string `json:"known"` + Nested handleJSONDataDeprecatedChild `json:"nested"` +} + +type handleJSONDataDeprecatedChild struct { + Known string `json:"known"` + + Data map[string]interface{} `json:"-"` +} + +func (c *handleJSONDataDeprecatedChild) HandleJSONData(data map[string]interface{}) { c.Data = data } func TestJSONDataHandler(t *testing.T) { t.Run("test_JSONDataHandler", func(t *testing.T) { - data := []byte(`{"known": "foo","unknown": "boo","nested": {"known": "goo","unknown": "doo"}}`) + data := []byte(`{ + "known": "foo", + "unknown":"boo", + "nested1": {"known": "goo","unknown": "doo"}, + "nested2": {"known": "goo","unknown": "doo"} + }`) + p := &handleJSONDataParent{} + result, err := Unmarshal(data, p) + if err != nil { + t.Errorf("unexpected error %v", err) + } + _, ok := result["nested1"].(handleJSONDataChild) + if !ok { + t.Error("invalid map value") + } + if p.Nested1.Data == nil { + t.Error("Nested1 HandleJSONData not called") + } + if len(p.Nested1.Data) != 2 || p.Nested1.Data["known"] != "goo" || p.Nested1.Data["unknown"] != "doo" { + t.Error("Nested1 invalid JSON data") + } + _, ok = result["nested2"].(handleJSONDataChild) + if !ok { + t.Error("invalid map value") + } + if p.Nested2.Data == nil { + t.Error("Nested2 HandleJSONData not called") + } + if len(p.Nested2.Data) != 2 || p.Nested2.Data["known"] != "goo" || p.Nested2.Data["unknown"] != "doo" { + t.Error("Nested2 invalid JSON data") + } + }) + t.Run("test_JSONDataHandler_single_error", func(t *testing.T) { + data := []byte(`{ + "known": "foo", + "unknown":"boo", + "nested1": {"known": "goo","unknown": "doo", "fail": true}, + "nested2": {"known": "goo","unknown": "doo", "fail": true} + }`) p := &handleJSONDataParent{} + _, err := Unmarshal(data, p) + if err == nil { + t.Errorf("expected JSONDataHandler error %v", err) + } + e, ok := err.(*jlexer.LexerError) + if !ok || e.Reason != "HandleJSONData failure" { + t.Errorf("unexpected JSONDataHandler error type %v", err) + } + }) + t.Run("test_JSONDataHandler_multiple_error", func(t *testing.T) { + data := []byte(`{ + "known": "foo", + "unknown":"boo", + "nested1": {"known": "goo","unknown": "doo", "fail": true}, + "nested2": {"known": "goo","unknown": "doo", "fail": true} + }`) + p := &handleJSONDataParent{} + _, err := Unmarshal(data, p, WithMode(ModeAllowMultipleErrors)) + if err == nil { + t.Errorf("expected JSONDataHandler error %v", err) + } + e, ok := err.(*MultipleLexerError) + if !ok { + t.Errorf("unexpected JSONDataHandler error type %v", err) + } + for _, lexerError := range e.Errors { + if lexerError.Reason != "HandleJSONData failure" { + t.Errorf("unexpected JSONDataHandler error type %v", err) + } + } + }) + t.Run("test_JSONDataHandler_deprecated", func(t *testing.T) { + data := []byte(`{"known": "foo","unknown": "boo","nested": {"known": "goo","unknown": "doo"}}`) + p := &handleJSONDataDeprecatedParent{} result, err := Unmarshal(data, p) if err != nil { t.Errorf("unexpected error %v", err) } - _, ok := result["nested"].(handleJSONDataChild) + _, ok := result["nested"].(handleJSONDataDeprecatedChild) if !ok { t.Error("invalid map value") } @@ -2477,7 +2569,9 @@ type nestedSkipPopulateChild struct { Foo string `json:"foo"` } -func (c *nestedSkipPopulateChild) HandleJSONData(map[string]interface{}) {} +func (c *nestedSkipPopulateChild) HandleJSONData(map[string]interface{}) error { + return nil +} var extraData = map[string]interface{}{ "extra1": "foo", @@ -2496,7 +2590,9 @@ type failOverStruct struct { C string `json:"c"` } -func (f *failOverStruct) HandleJSONData(map[string]interface{}) {} +func (f *failOverStruct) HandleJSONData(map[string]interface{}) error { + return nil +} func buildParentStruct() *parentStruct { return &parentStruct{