From 4f1ba99881e55ddb02d235e9335094558bc7aee4 Mon Sep 17 00:00:00 2001 From: Steven McCanne Date: Mon, 10 May 2021 17:35:35 -0700 Subject: [PATCH] remove mapstructure I have tripped over mapstructure one too many times so this commit removes it. When/if the extra round-trip through JSON adds measurable overhead to Zed query times, we can revisit this and hopefully mapstructure will be more mature. For now, I wanted to add order.Which to proc.Sort and similar but mapstructure cannot handle custom JSON unmarshalers so this caused unpack to fail for apparently no reason. See https://github.com/mitchellh/mapstructure/issues/115 --- compiler/ast/dag/unpack.go | 15 ++++++++++++--- compiler/ast/unpack.go | 14 +++++++++++--- go.mod | 1 - go.sum | 2 -- pkg/unpack/reflector.go | 34 ++++++++-------------------------- pkg/unpack/unpack_test.go | 16 ++++++++-------- zio/zjsonio/writer.go | 11 ++++++++++- 7 files changed, 49 insertions(+), 44 deletions(-) diff --git a/compiler/ast/dag/unpack.go b/compiler/ast/dag/unpack.go index 63aad059bf..b291fefd64 100644 --- a/compiler/ast/dag/unpack.go +++ b/compiler/ast/dag/unpack.go @@ -1,6 +1,7 @@ package dag import ( + "encoding/json" "errors" "github.com/brimdata/zed/compiler/ast/zed" @@ -80,7 +81,7 @@ func UnpackJSON(buf []byte) (interface{}, error) { if len(buf) == 0 { return nil, nil } - return unpacker.UnpackBytes(buf) + return unpacker.Unmarshal(buf) } // UnpackJSONAsOp transforms a JSON representation of an operator into an dag.Op. @@ -97,7 +98,11 @@ func UnpackJSONAsOp(buf []byte) (Op, error) { } func UnpackMapAsOp(m interface{}) (Op, error) { - object, err := unpacker.UnpackMap(m) + b, err := json.Marshal(m) + if err != nil { + return nil, err + } + object, err := unpacker.Unmarshal(b) if object == nil || err != nil { return nil, err } @@ -109,7 +114,11 @@ func UnpackMapAsOp(m interface{}) (Op, error) { } func UnpackMapAsExpr(m interface{}) (Expr, error) { - object, err := unpacker.UnpackMap(m) + b, err := json.Marshal(m) + if err != nil { + return nil, err + } + object, err := unpacker.Unmarshal(b) if object == nil || err != nil { return nil, err } diff --git a/compiler/ast/unpack.go b/compiler/ast/unpack.go index 5b232d52cf..a9326259b7 100644 --- a/compiler/ast/unpack.go +++ b/compiler/ast/unpack.go @@ -86,7 +86,7 @@ func UnpackJSON(buf []byte) (interface{}, error) { if len(buf) == 0 { return nil, nil } - return unpacker.UnpackBytes(buf) + return unpacker.Unmarshal(buf) } // UnpackJSONAsProc transforms a JSON representation of a proc into an ast.Proc. @@ -103,7 +103,11 @@ func UnpackJSONAsProc(buf []byte) (Proc, error) { } func UnpackMapAsProc(m interface{}) (Proc, error) { - object, err := unpacker.UnpackMap(m) + b, err := json.Marshal(m) + if err != nil { + return nil, err + } + object, err := unpacker.Unmarshal(b) if object == nil || err != nil { return nil, err } @@ -115,7 +119,11 @@ func UnpackMapAsProc(m interface{}) (Proc, error) { } func UnpackMapAsExpr(m interface{}) (Expr, error) { - object, err := unpacker.UnpackMap(m) + b, err := json.Marshal(m) + if err != nil { + return nil, err + } + object, err := unpacker.Unmarshal(b) if object == nil || err != nil { return nil, err } diff --git a/go.mod b/go.mod index fbca40dee5..71c32b8748 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,6 @@ require ( github.com/hashicorp/golang-lru v0.5.4 github.com/kr/text v0.2.0 github.com/mattn/go-isatty v0.0.12 // indirect - github.com/mitchellh/mapstructure v1.3.3 github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect github.com/pbnjay/memory v0.0.0-20190104145345-974d429e7ae4 github.com/peterh/liner v1.1.0 diff --git a/go.sum b/go.sum index 7c5f32655a..1bc89f6041 100644 --- a/go.sum +++ b/go.sum @@ -112,8 +112,6 @@ github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0j github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0= github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= -github.com/mitchellh/mapstructure v1.3.3 h1:SzB1nHZ2Xi+17FP0zVQBHIZqvwRN9408fJO8h+eeNA8= -github.com/mitchellh/mapstructure v1.3.3/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3RllmbCylyMrvgv0= diff --git a/pkg/unpack/reflector.go b/pkg/unpack/reflector.go index c12f2cd56a..8e4c6669f5 100644 --- a/pkg/unpack/reflector.go +++ b/pkg/unpack/reflector.go @@ -4,8 +4,6 @@ import ( "encoding/json" "fmt" "reflect" - - "github.com/mitchellh/mapstructure" ) var zero reflect.Value @@ -69,19 +67,15 @@ func (r Reflector) get(unpackKey string, create bool) map[string]reflect.Type { return types } -func (r Reflector) Unpack(s string) (interface{}, error) { - return r.UnpackBytes([]byte(s)) +func (r Reflector) UnmarshalString(s string) (interface{}, error) { + return r.Unmarshal([]byte(s)) } -func (r Reflector) UnpackBytes(b []byte) (interface{}, error) { - var jsonMap interface{} - if err := json.Unmarshal(b, &jsonMap); err != nil { +func (r Reflector) Unmarshal(b []byte) (interface{}, error) { + var m interface{} + if err := json.Unmarshal(b, &m); err != nil { return nil, fmt.Errorf("unpacker error parsing JSON: %w", err) } - return r.UnpackMap(jsonMap) -} - -func (r Reflector) UnpackMap(m interface{}) (interface{}, error) { object, ok := m.(map[string]interface{}) if !ok { return nil, fmt.Errorf("cannot unpack non-object JSON value") @@ -93,22 +87,10 @@ func (r Reflector) UnpackMap(m interface{}) (interface{}, error) { if rv, ok := skeleton.(reflect.Value); ok { skeleton = rv.Interface() } - v := skeleton - if _, ok := v.(map[string]interface{}); ok { - // If the root record wasn't decoded to a struct ptr, - // we pass a pointer to mapstructure as it requires - // a pointer val. - v = &skeleton - } - c := &mapstructure.DecoderConfig{ - TagName: "json", - Result: v, - } - dec, err := mapstructure.NewDecoder(c) - if err != nil { - return nil, fmt.Errorf("unpack (mapstructure): %w", err) + if err := json.Unmarshal(b, &skeleton); err != nil { + return nil, err } - return skeleton, dec.Decode(m) + return skeleton, err } func (r Reflector) lookup(object map[string]interface{}) (reflect.Value, error) { diff --git a/pkg/unpack/unpack_test.go b/pkg/unpack/unpack_test.go index 455af41aa5..e988c73c99 100644 --- a/pkg/unpack/unpack_test.go +++ b/pkg/unpack/unpack_test.go @@ -51,7 +51,7 @@ func TestUnpackBinaryExpr(t *testing.T) { Terminal{}, List{}, ) - actual, err := reflector.Unpack(binaryExprJSON) + actual, err := reflector.UnmarshalString(binaryExprJSON) require.NoError(t, err) assert.Equal(t, binaryExprExpected, actual) } @@ -80,7 +80,7 @@ func TestUnpackTypeTag(t *testing.T) { BinaryExpr2{}, Terminal{}, ) - actual, err := reflector.Unpack(typeTagJSON) + actual, err := reflector.UnmarshalString(typeTagJSON) require.NoError(t, err) assert.Equal(t, typeTagExpected, actual) } @@ -119,7 +119,7 @@ func TestUnpackNested(t *testing.T) { Terminal{}, List{}, ) - actual, err := reflector.Unpack(nestedJSON) + actual, err := reflector.UnmarshalString(nestedJSON) require.NoError(t, err) assert.Equal(t, nestedExpected, actual) } @@ -172,7 +172,7 @@ func TestUnpackEmbedded(t *testing.T) { List{}, Embedded{}, ) - actual, err := reflector.Unpack(embeddedJSON) + actual, err := reflector.UnmarshalString(embeddedJSON) require.NoError(t, err) assert.Equal(t, embeddedExpected, actual) } @@ -197,7 +197,7 @@ func TestUnpackList(t *testing.T) { Terminal{}, List{}, ) - actual, err := reflector.Unpack(listJSON) + actual, err := reflector.UnmarshalString(listJSON) require.NoError(t, err) assert.Equal(t, listExpected, actual) } @@ -242,7 +242,7 @@ func TestUnpackPairList(t *testing.T) { Terminal{}, PairList{}, ) - actual, err := reflector.Unpack(pairListJSON) + actual, err := reflector.UnmarshalString(pairListJSON) require.NoError(t, err) assert.Equal(t, pairListExpected, actual) } @@ -321,7 +321,7 @@ func TestUnpackCut(t *testing.T) { Identifier{}, Assignment{}, ) - actual, err := reflector.Unpack(cutJSON) + actual, err := reflector.UnmarshalString(cutJSON) require.NoError(t, err) assert.Equal(t, cutExpected, actual) } @@ -352,7 +352,7 @@ func TestUnpackSkip(t *testing.T) { BinaryExpr3{}, Terminal{}, ) - actual, err := reflector.Unpack(skipJSON) + actual, err := reflector.UnmarshalString(skipJSON) require.NoError(t, err) assert.Equal(t, skipExpected, actual) } diff --git a/zio/zjsonio/writer.go b/zio/zjsonio/writer.go index 30c0b84023..1adf997302 100644 --- a/zio/zjsonio/writer.go +++ b/zio/zjsonio/writer.go @@ -26,7 +26,16 @@ func unmarshal(b []byte) (*Object, error) { } var types []zed.Type for _, t := range template.Types { - object, err := unpacker.UnpackMap(t) + // We should enhance the unpacker to take the template struct + // here so we don't have to do this song and dance. But not + // a big deal because we only do it for inbound zjson (which is + // not performance critical and only for typedefs which are + // typically infrequent.) See issue #2702. + b, err := json.Marshal(t) + if err != nil { + return nil, err + } + object, err := unpacker.Unmarshal(b) if object == nil || err != nil { return nil, err }