From 37e19c73a8ca726656da6ebb0282f08e50b4217b Mon Sep 17 00:00:00 2001 From: fabiante Date: Fri, 2 Feb 2024 09:03:01 +0100 Subject: [PATCH 1/4] Add github.com/go-json-experiment/json dep --- go.mod | 1 + go.sum | 2 ++ 2 files changed, 3 insertions(+) diff --git a/go.mod b/go.mod index 462c2e2..4f67130 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/DEXPRO-Solutions-GmbH/easclient go 1.21.4 require ( + github.com/go-json-experiment/json v0.0.0-20231102232822-2e55bd4e08b0 github.com/google/uuid v1.5.0 github.com/joho/godotenv v1.5.1 github.com/stretchr/testify v1.8.4 diff --git a/go.sum b/go.sum index dc1f2a0..aad6bfa 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/go-json-experiment/json v0.0.0-20231102232822-2e55bd4e08b0 h1:ymLjT4f35nQbASLnvxEde4XOBL+Sn7rFuV+FOJqkljg= +github.com/go-json-experiment/json v0.0.0-20231102232822-2e55bd4e08b0/go.mod h1:6daplAwHHGbUGib4990V3Il26O0OC4aRyvewaaAihaA= github.com/google/uuid v1.5.0 h1:1p67kYwdtXjb0gL0BPiP1Av9wiZPo5A8z2cWkTZ+eyU= github.com/google/uuid v1.5.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/joho/godotenv v1.5.1 h1:7eLL/+HRGLY0ldzfGMeQkb7vMd0as4CfYvUVzLqw0N0= From 1d3a87c792b9a815911852908fc247999ee61b1d Mon Sep 17 00:00:00 2001 From: fabiante Date: Fri, 2 Feb 2024 09:03:22 +0100 Subject: [PATCH 2/4] Add dedicated json unmarshal function and tests to show bug behaviour we want to fix --- json.go | 11 ++++++ json_test.go | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 json.go create mode 100644 json_test.go diff --git a/json.go b/json.go new file mode 100644 index 0000000..0059016 --- /dev/null +++ b/json.go @@ -0,0 +1,11 @@ +package easclient + +import "github.com/go-json-experiment/json" + +// unmarshalJSON is a wrapper around the json library we want to use for unmarshaling. +// +// Since the std library does not handle our edge cases, a specialized library is used. +// Have a look at the tests for details. +func unmarshalJSON(data []byte, v any, opts ...json.Options) error { + return json.Unmarshal(data, v, opts...) +} diff --git a/json_test.go b/json_test.go new file mode 100644 index 0000000..6fc84b9 --- /dev/null +++ b/json_test.go @@ -0,0 +1,96 @@ +package easclient + +import ( + stdjson "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestUnmarshalOfSimilarKeys contains tests which show a challenge when dealing with the +// JSON representation of EAS responses: +// +// Standard fields are sometimes mixed with custom fields in the same +// object. The std lib's json package does not strictly enforce that only the field +// with a matching case is being used. Have a look at the tests and these links for details: +// +// - https://github.com/golang/go/issues/14750 +// - https://github.com/go-json-experiment/json +func TestUnmarshalOfSimilarKeys(t *testing.T) { + t.Run("std lib does unmarshal from exact key", func(t *testing.T) { + t.Run("uses last key 1", func(t *testing.T) { + attachmentStr := `{ + "id": "0dd018f8-bf23-455d-8214-44e76b24e5db", + "Id": "00000000-0000-0000-0000-000000000000" + }` + + attachment := RecordAttachment{} + + err := stdjson.Unmarshal([]byte(attachmentStr), &attachment) + require.NoError(t, err) + + // This test asserts that the std lib behaves "weired" - given that the Id struct field + // should be unmarshalled from the JSON field "id" and not "Id". + // + // In practice however, the std lib will use the last key it finds, accepting all case-variations. + assert.Equal(t, "00000000-0000-0000-0000-000000000000", attachment.Id.String()) + }) + + t.Run("uses last key 2", func(t *testing.T) { + attachmentStr := `{ + "Id": "00000000-0000-0000-0000-000000000000", + "id": "0dd018f8-bf23-455d-8214-44e76b24e5db" + }` + + attachment := RecordAttachment{} + + err := stdjson.Unmarshal([]byte(attachmentStr), &attachment) + require.NoError(t, err) + + // This test asserts that the std lib behaves "weired" - given that the Id struct field + // should be unmarshalled from the JSON field "id" and not "Id". + // + // In practice however, the std lib will use the last key it finds, accepting all case-variations. + assert.Equal(t, "0dd018f8-bf23-455d-8214-44e76b24e5db", attachment.Id.String()) + }) + }) + + t.Run("v2 json lib does unmarshal from exact key", func(t *testing.T) { + t.Run("uses correct key 1", func(t *testing.T) { + attachmentStr := `{ + "id": "0dd018f8-bf23-455d-8214-44e76b24e5db", + "Id": "00000000-0000-0000-0000-000000000000" + }` + + attachment := RecordAttachment{} + + err := unmarshalJSON([]byte(attachmentStr), &attachment) + require.NoError(t, err) + + // This test asserts that the std lib behaves "weired" - given that the Id struct field + // should be unmarshalled from the JSON field "id" and not "Id". + // + // In practice however, the std lib will use the last key it finds, accepting all case-variations. + assert.Equal(t, "0dd018f8-bf23-455d-8214-44e76b24e5db", attachment.Id.String()) + }) + + t.Run("uses correct key 2", func(t *testing.T) { + attachmentStr := `{ + "Id": "00000000-0000-0000-0000-000000000000", + "id": "0dd018f8-bf23-455d-8214-44e76b24e5db" + }` + + attachment := RecordAttachment{} + + err := unmarshalJSON([]byte(attachmentStr), &attachment) + require.NoError(t, err) + + // This test asserts that the std lib behaves "weired" - given that the Id struct field + // should be unmarshalled from the JSON field "id" and not "Id". + // + // In practice however, the std lib will use the last key it finds, accepting all case-variations. + assert.Equal(t, "0dd018f8-bf23-455d-8214-44e76b24e5db", attachment.Id.String()) + }) + }) +} From 9e013039c3009d36a27b6c22e2c04236a3c5dc1a Mon Sep 17 00:00:00 2001 From: fabiante Date: Fri, 2 Feb 2024 09:29:48 +0100 Subject: [PATCH 3/4] Add helper to copy resty.Client --- resty.go | 21 +++++++++++++++++++++ resty_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 resty.go create mode 100644 resty_test.go diff --git a/resty.go b/resty.go new file mode 100644 index 0000000..92aa9c1 --- /dev/null +++ b/resty.go @@ -0,0 +1,21 @@ +package easclient + +import ( + "github.com/go-json-experiment/json" + "gopkg.in/resty.v1" +) + +func copyRestyClient(c *resty.Client) *resty.Client { + // dereference the pointer and copy the value + cc := *c + return &cc +} + +func adaptRestyClient(c *resty.Client) { + c.JSONUnmarshal = func(data []byte, v interface{}) error { + opts := []json.Options{ + json.MatchCaseInsensitiveNames(false), + } + return unmarshalJSON(data, v, opts...) + } +} diff --git a/resty_test.go b/resty_test.go new file mode 100644 index 0000000..a98c4da --- /dev/null +++ b/resty_test.go @@ -0,0 +1,26 @@ +package easclient + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + "gopkg.in/resty.v1" +) + +func Test_copyRestyClient(t *testing.T) { + t.Run("returns valid copy", func(t *testing.T) { + original := resty.New() + copied := copyRestyClient(original) + + require.NotSame(t, original, copied) + + t.Run("copy modifications do not affect original", func(t *testing.T) { + copied.JSONUnmarshal = func(data []byte, v interface{}) error { + return errors.New("this is some error") + } + + require.NotSame(t, original.JSONUnmarshal, copied.JSONUnmarshal) + }) + }) +} From 2acb6c320a53efcd9b2a6bb9cf3458398e5b6477 Mon Sep 17 00:00:00 2001 From: fabiante Date: Fri, 2 Feb 2024 09:30:37 +0100 Subject: [PATCH 4/4] Add copying and adapting of resty.Clients to Store and Server clients Due to the usage of the experimental json package, this will fix the bug shown in the json test commit. --- server_client.go | 3 +++ store_client.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/server_client.go b/server_client.go index 465ec82..5f10755 100644 --- a/server_client.go +++ b/server_client.go @@ -6,6 +6,9 @@ type ServerClient struct { c *resty.Client } +// NewServerClient creates a new client for server interaction. func NewServerClient(c *resty.Client) *ServerClient { + c = copyRestyClient(c) + adaptRestyClient(c) return &ServerClient{c: c} } diff --git a/store_client.go b/store_client.go index 8149908..21e771d 100644 --- a/store_client.go +++ b/store_client.go @@ -11,7 +11,10 @@ type StoreClient struct { c *resty.Client } +// NewStoreClient creates a new client for store interaction. func NewStoreClient(c *resty.Client) *StoreClient { + c = copyRestyClient(c) + adaptRestyClient(c) return &StoreClient{c: c} }