From 2845cdfb814ea6fb2308a84c006d148ebf9fb5e9 Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Thu, 23 Nov 2017 10:05:30 -0800 Subject: [PATCH] Improve package ergonomics and fix a bug. * Break up Attributer interface into AttrGetter and AttrSetter. * Get rid of the Attributes type in favour of simple []byte. * Make AttrGetter method set match what protobuf might generate. * When returning struct fields from GetField, use reflect.Indirect so that pointers are never returned. * Make sure that extended attributes are never returned by GetField and never marshaled by Marshal. Refs #586 --- types/dynamic/dynamic.go | 64 ++++++++++++++++++++++------------- types/dynamic/dynamic_test.go | 41 ++++++++++++++-------- 2 files changed, 66 insertions(+), 39 deletions(-) diff --git a/types/dynamic/dynamic.go b/types/dynamic/dynamic.go index 03b994a384..a9bc25d8df 100644 --- a/types/dynamic/dynamic.go +++ b/types/dynamic/dynamic.go @@ -10,16 +10,16 @@ import ( jsoniter "github.com/json-iterator/go" ) -// Attributes hold arbitrary JSON-encoded data. -type Attributes struct { - data []byte +// AttrGetter is required to be implemented by types that need to work with +// GetField and Marshal. +type AttrGetter interface { + GetExtendedAttributes() []byte } -// Attributer can be implemented to enable a type to work with the Marshal -// and Unmarshal functions in this package. -type Attributer interface { - Attributes() Attributes - SetAttributes(Attributes) +// AttrSetter is required to be implemented by types that need to work with +// Unmarshal. +type AttrSetter interface { + SetExtendedAttributes([]byte) } // GetField gets a field from v according to its name. @@ -27,7 +27,9 @@ type Attributer interface { // it will try to dynamically find the corresponding item in the 'Extended' // field. GetField is case-sensitive, but extended attribute names will be // converted to CamelCaps. -func GetField(v Attributer, name string) (interface{}, error) { +func GetField(v AttrGetter, name string) (interface{}, error) { + extendedAttributes := v.GetExtendedAttributes() + extAttrPtr := &extendedAttributes[0] if s := string([]rune(name)[0]); strings.Title(s) == s { // Exported fields are always upper-cased for the first rune strukt := reflect.Indirect(reflect.ValueOf(v)) @@ -36,11 +38,19 @@ func GetField(v Attributer, name string) (interface{}, error) { } field := strukt.FieldByName(name) if field.IsValid() { - return field.Interface(), nil + rval := reflect.Indirect(field).Interface() + if b, ok := rval.([]byte); ok { + // Make sure this field isn't the extended attributes + if extAttrPtr == &b[0] { + goto EXTENDED + } + } + return rval, nil } } +EXTENDED: // If we get here, we are dealing with extended attributes. - any := AnyParameters{any: jsoniter.Get(v.Attributes().data)} + any := AnyParameters{any: jsoniter.Get(extendedAttributes)} return any.Get(name) } @@ -112,7 +122,7 @@ func (s structField) jsonFieldName() (string, bool) { return fieldName, omitEmpty } -func getJSONFields(v reflect.Value) map[string]structField { +func getJSONFields(v reflect.Value, addressOfAttrs *byte) map[string]structField { typ := v.Type() numField := v.NumField() result := make(map[string]structField, numField) @@ -131,6 +141,11 @@ func getJSONFields(v reflect.Value) map[string]structField { if sf.OmitEmpty && sf.IsEmpty() { continue } + if b, ok := reflect.Indirect(value).Interface().([]byte); ok { + if len(b) > 0 && &b[0] == addressOfAttrs { + continue + } + } // sf is a valid JSON field. result[sf.JSONName] = sf } @@ -140,16 +155,16 @@ func getJSONFields(v reflect.Value) map[string]structField { // extractExtendedAttributes selects only extended attributes from msg. It will // ignore any fields in msg that correspond to fields in v. v must be of kind // reflect.Struct. -func extractExtendedAttributes(v interface{}, msg []byte) (Attributes, error) { +func extractExtendedAttributes(v interface{}, msg []byte) ([]byte, error) { strukt := reflect.Indirect(reflect.ValueOf(v)) if kind := strukt.Kind(); kind != reflect.Struct { - return Attributes{}, fmt.Errorf("invalid type (want struct): %v", kind) + return nil, fmt.Errorf("invalid type (want struct): %v", kind) } - fields := getJSONFields(strukt) + fields := getJSONFields(strukt, nil) stream := jsoniter.NewStream(jsoniter.ConfigDefault, nil, 4096) var anys map[string]jsoniter.Any if err := jsoniter.Unmarshal(msg, &anys); err != nil { - return Attributes{}, err + return nil, err } stream.WriteObjectStart() j := 0 @@ -167,12 +182,12 @@ func extractExtendedAttributes(v interface{}, msg []byte) (Attributes, error) { any.WriteTo(stream) } stream.WriteObjectEnd() - return Attributes{data: stream.Buffer()}, nil + return stream.Buffer(), nil } // Unmarshal decodes msg into v, storing what fields it can into the basic -// fields of the struct, and storing the rest into Attributes. -func Unmarshal(msg []byte, v Attributer) error { +// fields of the struct, and storing the rest into its extended attributes. +func Unmarshal(msg []byte, v AttrSetter) error { if _, ok := v.(json.Unmarshaler); ok { // Can't safely call UnmarshalJSON here without potentially causing an // infinite recursion. Copy the struct into a new type that doesn't @@ -206,7 +221,7 @@ func Unmarshal(msg []byte, v Attributer) error { if err != nil { return err } - v.SetAttributes(attrs) + v.SetExtendedAttributes(attrs) return nil } @@ -214,7 +229,7 @@ func Unmarshal(msg []byte, v Attributer) error { // It also encodes any extended attributes that are defined. Marshal // respects the encoding/json rules regarding exported fields, and tag // semantics. If v's kind is not reflect.Struct, an error will be returned. -func Marshal(v Attributer) ([]byte, error) { +func Marshal(v AttrGetter) ([]byte, error) { s := jsoniter.NewStream(jsoniter.ConfigDefault, nil, 4096) s.WriteObjectStart() @@ -222,7 +237,7 @@ func Marshal(v Attributer) ([]byte, error) { return nil, err } - extended := v.Attributes().data + extended := v.GetExtendedAttributes() if len(extended) > 0 { if err := encodeExtendedFields(extended, s); err != nil { return nil, err @@ -234,12 +249,13 @@ func Marshal(v Attributer) ([]byte, error) { return s.Buffer(), nil } -func encodeStructFields(v interface{}, s *jsoniter.Stream) error { +func encodeStructFields(v AttrGetter, s *jsoniter.Stream) error { strukt := reflect.Indirect(reflect.ValueOf(v)) if kind := strukt.Kind(); kind != reflect.Struct { return fmt.Errorf("invalid type (want struct): %v", kind) } - m := getJSONFields(strukt) + addressOfAttrs := &v.GetExtendedAttributes()[0] + m := getJSONFields(strukt, addressOfAttrs) fields := make([]structField, 0, len(m)) for _, s := range m { fields = append(fields, s) diff --git a/types/dynamic/dynamic_test.go b/types/dynamic/dynamic_test.go index e4d5d6cff5..17b558db1b 100644 --- a/types/dynamic/dynamic_test.go +++ b/types/dynamic/dynamic_test.go @@ -58,15 +58,17 @@ func TestGetJSONStructField(t *testing.T) { ValidEmpty int `json:"validEmpty"` InvalidEmpty int `json:"invalidEmpty,omitempty"` Invalid int `json:"-"` + Attributes []byte }{ Valid: 5, invalidUnexported: 1, ValidEmpty: 0, InvalidEmpty: 0, Invalid: 10, + Attributes: []byte(`"hello!"`), } - fields := getJSONFields(reflect.ValueOf(test)) + fields := getJSONFields(reflect.ValueOf(test), &test.Attributes[0]) require.Equal(2, len(fields)) field, ok := fields["valid"] @@ -85,15 +87,15 @@ type MyType struct { Foo string `json:"foo"` Bar []MyType `json:"bar"` - attrs Attributes + Attrs []byte // note that this will not be marshalled directly, despite missing the `json"-"`! } -func (m *MyType) Attributes() Attributes { - return m.attrs +func (m *MyType) GetExtendedAttributes() []byte { + return m.Attrs } -func (m *MyType) SetAttributes(a Attributes) { - m.attrs = a +func (m *MyType) SetExtendedAttributes(a []byte) { + m.Attrs = a } func (m *MyType) Get(name string) (interface{}, error) { @@ -117,7 +119,7 @@ func TestExtractEmptyExtendedAttributes(t *testing.T) { attrs, err := extractExtendedAttributes(m, msg) require.NoError(err) - assert.Equal([]byte("{}"), attrs.data) + assert.Equal([]byte("{}"), attrs) } func TestExtractExtendedAttributes(t *testing.T) { @@ -129,7 +131,7 @@ func TestExtractExtendedAttributes(t *testing.T) { attrs, err := extractExtendedAttributes(m, msg) require.NoError(err) - assert.Equal([]byte(`{"extendedattr":"such extended"}`), attrs.data) + assert.Equal([]byte(`{"extendedattr":"such extended"}`), attrs) } func TestMarshal(t *testing.T) { @@ -141,7 +143,7 @@ func TestMarshal(t *testing.T) { m := &MyType{ Foo: "hello world!", Bar: nil, - attrs: Attributes{data: extendedBytes}, + Attrs: extendedBytes, } b, err := Marshal(m) @@ -153,7 +155,7 @@ func TestGetField(t *testing.T) { m := &MyType{ Foo: "hello", Bar: []MyType{{Foo: "there"}}, - attrs: Attributes{data: []byte(`{"a":"a","b":1,"c":2.0,"d":true,"e":null,"foo":{"hello":5},"bar":[true,10.5]}`)}, + Attrs: []byte(`{"a":"a","b":1,"c":2.0,"d":true,"e":null,"foo":{"hello":5},"bar":[true,10.5]}`), } fooAny := jsoniter.Get([]byte(`{"hello":5}`)) @@ -213,7 +215,7 @@ func TestGetField(t *testing.T) { func TestQueryGovaluateSimple(t *testing.T) { m := &MyType{ - attrs: Attributes{data: []byte(`{"hello":5}`)}, + Attrs: []byte(`{"hello":5}`), } expr, err := govaluate.NewEvaluableExpression("hello == 5") @@ -227,7 +229,7 @@ func TestQueryGovaluateSimple(t *testing.T) { func BenchmarkQueryGovaluateSimple(b *testing.B) { m := &MyType{ - attrs: Attributes{data: []byte(`{"hello":5}`)}, + Attrs: []byte(`{"hello":5}`), } expr, err := govaluate.NewEvaluableExpression("hello == 5") @@ -242,7 +244,7 @@ func BenchmarkQueryGovaluateSimple(b *testing.B) { func TestQueryGovaluateComplex(t *testing.T) { m := &MyType{ - attrs: Attributes{data: []byte(`{"hello":{"foo":5,"bar":6.0}}`)}, + Attrs: []byte(`{"hello":{"foo":5,"bar":6.0}}`), } expr, err := govaluate.NewEvaluableExpression("hello.foo == 5") @@ -264,7 +266,7 @@ func TestQueryGovaluateComplex(t *testing.T) { func BenchmarkQueryGovaluateComplex(b *testing.B) { m := &MyType{ - attrs: Attributes{data: []byte(`{"hello":{"foo":5,"bar":6.0}}`)}, + Attrs: []byte(`{"hello":{"foo":5,"bar":6.0}}`), } expr, err := govaluate.NewEvaluableExpression("hello.foo < hello.bar") @@ -282,7 +284,7 @@ func TestMarshalUnmarshal(t *testing.T) { var m MyType err := json.Unmarshal(data, &m) require.NoError(t, err) - assert.Equal(t, MyType{Foo: "hello", attrs: Attributes{data: []byte(`{"a":10,"b":"c"}`)}}, m) + assert.Equal(t, MyType{Foo: "hello", Attrs: []byte(`{"a":10,"b":"c"}`)}, m) b, err := json.Marshal(&m) require.NoError(t, err) assert.Equal(t, data, b) @@ -305,3 +307,12 @@ func BenchmarkMarshal(b *testing.B) { json.Marshal(&m) } } + +func TestNoLookupAttrsDirectly(t *testing.T) { + m := MyType{ + Attrs: []byte(`{}`), + } + _, err := m.Get("Attrs") + require.NotNil(t, err) + assert.Equal(t, err.Error(), "[Attrs] not found") +}