From fb16f7cfe94456a5f8455425118d8bac4588152a Mon Sep 17 00:00:00 2001 From: Stephan Kim Date: Thu, 3 Aug 2017 01:12:25 -0700 Subject: [PATCH 1/4] peer attributes: track and remove dominantField errors --- node.go | 59 +++++++++++++++++++++++++++++++++++++++++++++++++---- response.go | 22 ++++++++++++-------- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/node.go b/node.go index 73b7d59..b3de3cb 100644 --- a/node.go +++ b/node.go @@ -2,6 +2,10 @@ package jsonapi import "fmt" +type nodeError bool + +const dominantFieldConflict nodeError = false + // Payloader is used to encapsulate the One and Many payload types type Payloader interface { clearIncluded() @@ -33,18 +37,67 @@ func (p *ManyPayload) clearIncluded() { p.Included = []*Node{} } +type attributes map[string]interface{} + // Node is used to represent a generic JSON API Resource type Node struct { Type string `json:"type"` ID string `json:"id,omitempty"` ClientID string `json:"client-id,omitempty"` - Attributes map[string]interface{} `json:"attributes,omitempty"` + Attributes attributes `json:"attributes,omitempty"` Relationships map[string]interface{} `json:"relationships,omitempty"` Links *Links `json:"links,omitempty"` Meta *Meta `json:"meta,omitempty"` } +func isNodeError(i interface{}) bool { + _, ok := i.(nodeError) + return ok +} + +func (n *Node) cleanupDominantFieldIssues() { + for k, v := range n.Attributes { + if isNodeError(v) { + delete(n.Attributes, k) + } + } +} + +func (a attributes) set(k string, v interface{}) { + if _, ok := a[k]; ok { + a[k] = dominantFieldConflict + } else { + a[k] = v + } +} + +func (n *Node) mergeAttributes(attrs attributes) { + for k, v := range attrs { + n.Attributes[k] = v + } +} + +func combineNodes(nodes []*Node) *Node { + n := &Node{} + for _, node := range nodes { + n.peerMerge(node) + } + return n +} + +func (n *Node) peerMerge(node *Node) { + n.mergeFunc(node, func(attrs attributes) { + for k, v := range node.Attributes { + n.Attributes.set(k, v) + } + }) +} + func (n *Node) merge(node *Node) { + n.mergeFunc(node, n.mergeAttributes) +} + +func (n *Node) mergeFunc(node *Node, attrSetter func(attrs attributes)) { if node.Type != "" { n.Type = node.Type } @@ -60,9 +113,7 @@ func (n *Node) merge(node *Node) { if n.Attributes == nil && node.Attributes != nil { n.Attributes = make(map[string]interface{}) } - for k, v := range node.Attributes { - n.Attributes[k] = v - } + attrSetter(node.Attributes) if n.Relationships == nil && node.Relationships != nil { n.Relationships = make(map[string]interface{}) diff --git a/response.go b/response.go index a19848a..1ff9b08 100644 --- a/response.go +++ b/response.go @@ -213,6 +213,7 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool modelValue := reflect.ValueOf(model).Elem() modelType := reflect.ValueOf(model).Type().Elem() + nodes := make([]*Node, 0) // handle just the embedded models first for i := 0; i < modelValue.NumField(); i++ { fieldValue := modelValue.Field(i) @@ -241,11 +242,14 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool er = err break } - node.merge(embNode) + nodes = append(nodes, embNode) + } + node.merge(combineNodes(nodes)) } // handle everthing else + attrs := attributes{} for i := 0; i < modelValue.NumField(); i++ { fieldValue := modelValue.Field(i) fieldType := modelType.Field(i) @@ -346,7 +350,7 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool if node.Attributes == nil { node.Attributes = make(map[string]interface{}) } - + attributeKey := args[1] if fieldValue.Type() == reflect.TypeOf(time.Time{}) { t := fieldValue.Interface().(time.Time) @@ -355,9 +359,9 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool } if iso8601 { - node.Attributes[args[1]] = t.UTC().Format(iso8601TimeFormat) + attrs.set(attributeKey, t.UTC().Format(iso8601TimeFormat)) } else { - node.Attributes[args[1]] = t.Unix() + attrs.set(attributeKey, t.Unix()) } } else if fieldValue.Type() == reflect.TypeOf(new(time.Time)) { // A time pointer may be nil @@ -375,9 +379,9 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool } if iso8601 { - node.Attributes[args[1]] = tm.UTC().Format(iso8601TimeFormat) + attrs.set(attributeKey, tm.UTC().Format(iso8601TimeFormat)) } else { - node.Attributes[args[1]] = tm.Unix() + attrs.set(attributeKey, tm.Unix()) } } } else { @@ -391,9 +395,9 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool strAttr, ok := fieldValue.Interface().(string) if ok { - node.Attributes[args[1]] = strAttr + attrs.set(attributeKey, strAttr) } else { - node.Attributes[args[1]] = fieldValue.Interface() + attrs.set(attributeKey, fieldValue.Interface()) } } } else if annotation == annotationRelation { @@ -511,6 +515,8 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool node.Meta = metableModel.JSONAPIMeta() } + node.mergeAttributes(attrs) + node.cleanupDominantFieldIssues() return node, nil } From 92996bfd315f7b55d5bfb8c9e9d98a34aaa94b8b Mon Sep 17 00:00:00 2001 From: Stephan Kim Date: Thu, 3 Aug 2017 11:48:36 -0700 Subject: [PATCH 2/4] refactor marker for dominantFieldConflicts; add comments and tests --- embeded_structs_test.go | 28 +---- node.go | 78 ++++++++++---- node_test.go | 226 ++++++++++++++++++++++++++++++++++++++++ response.go | 18 +++- 4 files changed, 298 insertions(+), 52 deletions(-) create mode 100644 node_test.go diff --git a/embeded_structs_test.go b/embeded_structs_test.go index 17381ca..415a0a0 100644 --- a/embeded_structs_test.go +++ b/embeded_structs_test.go @@ -7,33 +7,6 @@ import ( "testing" ) -func TestMergeNode(t *testing.T) { - parent := &Node{ - Type: "Good", - ID: "99", - Attributes: map[string]interface{}{"fizz": "buzz"}, - } - - child := &Node{ - Type: "Better", - ClientID: "1111", - Attributes: map[string]interface{}{"timbuk": 2}, - } - - expected := &Node{ - Type: "Better", - ID: "99", - ClientID: "1111", - Attributes: map[string]interface{}{"fizz": "buzz", "timbuk": 2}, - } - - parent.merge(child) - - if !reflect.DeepEqual(expected, parent) { - t.Errorf("Got %+v Expected %+v", parent, expected) - } -} - func TestIsEmbeddedStruct(t *testing.T) { type foo struct{} @@ -888,6 +861,7 @@ func TestMarshal_duplicateFieldFromEmbeddedStructs_serializationNameDiffers(t *t if err := MarshalPayload(jsonAPIData, &o); err != nil { t.Fatal(err) } + if err := json.Unmarshal(jsonAPIData.Bytes(), &payloadData); err != nil { t.Fatal(err) } diff --git a/node.go b/node.go index b3de3cb..2e38703 100644 --- a/node.go +++ b/node.go @@ -2,10 +2,6 @@ package jsonapi import "fmt" -type nodeError bool - -const dominantFieldConflict nodeError = false - // Payloader is used to encapsulate the One and Many payload types type Payloader interface { clearIncluded() @@ -37,8 +33,6 @@ func (p *ManyPayload) clearIncluded() { p.Included = []*Node{} } -type attributes map[string]interface{} - // Node is used to represent a generic JSON API Resource type Node struct { Type string `json:"type"` @@ -50,34 +44,43 @@ type Node struct { Meta *Meta `json:"meta,omitempty"` } -func isNodeError(i interface{}) bool { - _, ok := i.(nodeError) - return ok -} - -func (n *Node) cleanupDominantFieldIssues() { +func (n *Node) handleNodeErrors() { for k, v := range n.Attributes { - if isNodeError(v) { + if _, ok := v.(nodeError); ok { delete(n.Attributes, k) } } } +type attributes map[string]interface{} + +// this attributes setter +// * adds new entries as-is +// * tracks dominant field conflicts func (a attributes) set(k string, v interface{}) { - if _, ok := a[k]; ok { - a[k] = dominantFieldConflict + if val, ok := a[k]; ok { + if ne, ok := val.(nodeError); ok { + ne.Add(k, v) + return + } + a[k] = newDominantFieldConflict(k, val, v) } else { a[k] = v } } +// mergeAttributes consolidates 2 attribute maps +// if there are conflicting keys, the values in the argument take priority func (n *Node) mergeAttributes(attrs attributes) { for k, v := range attrs { n.Attributes[k] = v } } -func combineNodes(nodes []*Node) *Node { +// mergeAttributes consolidates multiple nodes into a single consolidated node +// the last node value has a higher priority over others in setting single value types (i.e. node.ID, node.Type) +// if there are conflicting attributes, those will get flagged as errors +func combinePeerNodes(nodes []*Node) *Node { n := &Node{} for _, node := range nodes { n.peerMerge(node) @@ -85,6 +88,12 @@ func combineNodes(nodes []*Node) *Node { return n } +// merge - a simple merge where the values in the argument have a higher priority +func (n *Node) merge(node *Node) { + n.mergeFunc(node, n.mergeAttributes) +} + +// peerMerge - when merging peers, we need to track nodeErrors func (n *Node) peerMerge(node *Node) { n.mergeFunc(node, func(attrs attributes) { for k, v := range node.Attributes { @@ -93,10 +102,6 @@ func (n *Node) peerMerge(node *Node) { }) } -func (n *Node) merge(node *Node) { - n.mergeFunc(node, n.mergeAttributes) -} - func (n *Node) mergeFunc(node *Node, attrSetter func(attrs attributes)) { if node.Type != "" { n.Type = node.Type @@ -234,3 +239,36 @@ func deepCopyNode(n *Node) *Node { } return © } + +// nodeError is used to track errors in processing Node related values +type nodeError interface { + Error() string + Add(key string, val interface{}) +} + +// dominantFieldConflict is a specific type of marshaling scenario that the standard json lib treats as an error +// comment from json.dominantField(): "If there are multiple top-level fields...This condition is an error in Go and we skip" +// tracking the key and conflicting values for future use to possibly surface an error +type dominantFieldConflict struct { + key string + vals []interface{} +} + +func newDominantFieldConflict(key string, vals ...interface{}) interface{} { + return &dominantFieldConflict{ + key: key, + vals: vals, + } +} + +func (dfc *dominantFieldConflict) Error() string { + return fmt.Sprintf("there is a conflict with this attribute: %s", dfc.key) +} + +func (dfc *dominantFieldConflict) Add(key string, val interface{}) { + dfc.key = key + if dfc.vals == nil { + dfc.vals = make([]interface{}, 0) + } + dfc.vals = append(dfc.vals, val) +} diff --git a/node_test.go b/node_test.go new file mode 100644 index 0000000..30187ca --- /dev/null +++ b/node_test.go @@ -0,0 +1,226 @@ +package jsonapi + +import ( + "reflect" + "testing" +) + +func TestHandleNodeErrors(t *testing.T) { + tests := []struct { + name string + hasNodeError bool + input *Node + expected *Node + }{ + { + name: "has no errors", + hasNodeError: false, + input: &Node{Attributes: attributes{"foo": true, "bar": false}}, + expected: &Node{Attributes: attributes{"foo": true, "bar": false}}, + }, + { + name: "has an error", + hasNodeError: true, + input: &Node{Attributes: attributes{"foo": true, "bar": &dominantFieldConflict{key: "bar", vals: []interface{}{true, false}}}}, + expected: &Node{Attributes: attributes{"foo": true}}, + }, + { + name: "has a couple errors", + hasNodeError: true, + input: &Node{ + Attributes: attributes{ + "foo": true, + "bar": &dominantFieldConflict{key: "bar", vals: []interface{}{true, false}}, + "bat": &dominantFieldConflict{key: "bat", vals: []interface{}{true, false}}, + }, + }, + expected: &Node{Attributes: attributes{"foo": true}}, + }, + } + + for _, scenario := range tests { + t.Logf("scenario: %s\n", scenario.name) + if scenario.hasNodeError && reflect.DeepEqual(scenario.input, scenario.expected) { + t.Error("expected input and expected to be different") + } + scenario.input.handleNodeErrors() + if !reflect.DeepEqual(scenario.input, scenario.expected) { + t.Errorf("Got\n%#v\nExpected\n%#v\n", scenario.input, scenario.expected) + } + } +} + +func TestSetAttributes(t *testing.T) { + key := "foo" + attr := attributes{} + + // set first val + attr.set(key, false) + + // check presence of first val + val, ok := attr[key] + if !ok { + t.Errorf("expected attributes to have key: %s\n", key) + } + + // assert first val is not an error + _, ok = val.(nodeError) + if ok { + t.Errorf("val stored for key (%s) should NOT be a nodeError\n", key) + } + + // add second val; same key + attr.set(key, true) + + // assert val converted to an error + _, ok = attr[key].(nodeError) + if !ok { + t.Errorf("val stored for key (%s) should be a nodeError\n", key) + } + + // add third val; same key + attr.set(key, nil) + + // assert val remains an error + _, ok = attr[key].(nodeError) + if !ok { + t.Errorf("val stored for key (%s) should be a nodeError\n", key) + } +} + +func TestMergeNode(t *testing.T) { + parent := &Node{ + Type: "Good", + ID: "99", + Attributes: map[string]interface{}{"fizz": "buzz"}, + } + + child := &Node{ + Type: "Better", + ClientID: "1111", + Attributes: map[string]interface{}{"timbuk": 2}, + } + + expected := &Node{ + Type: "Better", + ID: "99", + ClientID: "1111", + Attributes: map[string]interface{}{"fizz": "buzz", "timbuk": 2}, + } + + parent.merge(child) + + if !reflect.DeepEqual(expected, parent) { + t.Errorf("Got %+v Expected %+v", parent, expected) + } +} + +func TestCombinePeerNodesNoConflict(t *testing.T) { + brother := &Node{ + Type: "brother", + ID: "99", + ClientID: "9999", + Attributes: map[string]interface{}{"fizz": "buzz"}, + Relationships: map[string]interface{}{"father": "Joe"}, + } + + sister := &Node{ + Type: "sister", + ID: "11", + ClientID: "1111", + Attributes: map[string]interface{}{"timbuk": 2}, + Relationships: map[string]interface{}{"mother": "Mary"}, + } + + expected := &Node{ + Type: "sister", + ID: "11", + ClientID: "1111", + Attributes: map[string]interface{}{"fizz": "buzz", "timbuk": 2}, + Relationships: map[string]interface{}{"father": "Joe", "mother": "Mary"}, + } + + actual := combinePeerNodes([]*Node{brother, sister}) + + if !reflect.DeepEqual(actual, expected) { + t.Errorf("Got %+v Expected %+v", actual, expected) + } +} +func TestCombinePeerNodesWithConflict(t *testing.T) { + brother := &Node{ + Type: "brother", + ID: "99", + ClientID: "9999", + Attributes: map[string]interface{}{"timbuk": 2}, + Relationships: map[string]interface{}{"father": "Joe"}, + } + + sister := &Node{ + Type: "sister", + ID: "11", + ClientID: "1111", + Attributes: map[string]interface{}{"timbuk": 2}, + Relationships: map[string]interface{}{"mother": "Mary"}, + } + + expected := &Node{ + Type: "sister", + ID: "11", + ClientID: "1111", + Attributes: map[string]interface{}{"timbuk": &dominantFieldConflict{key: "timbuk", vals: []interface{}{2, 2}}}, + Relationships: map[string]interface{}{"father": "Joe", "mother": "Mary"}, + } + + actual := combinePeerNodes([]*Node{brother, sister}) + + if !reflect.DeepEqual(actual, expected) { + t.Errorf("Got %+v Expected %+v", actual, expected) + } +} + +func TestDeepCopyNode(t *testing.T) { + source := &Node{ + Type: "thing", + ID: "1", + ClientID: "2", + Attributes: attributes{"key": "val"}, + Relationships: map[string]interface{}{"key": "val"}, + Links: &Links{"key": "val"}, + Meta: &Meta{"key": "val"}, + } + + badCopy := *source + if !reflect.DeepEqual(badCopy, *source) { + t.Errorf("Got %+v Expected %+v", badCopy, *source) + } + if reflect.ValueOf(badCopy.Attributes).Pointer() != reflect.ValueOf(source.Attributes).Pointer() { + t.Error("Expected map address to be the same") + } + if reflect.ValueOf(badCopy.Relationships).Pointer() != reflect.ValueOf(source.Relationships).Pointer() { + t.Error("Expected map address to be the same") + } + if reflect.ValueOf(badCopy.Links).Pointer() != reflect.ValueOf(source.Links).Pointer() { + t.Error("Expected map address to be the same") + } + if reflect.ValueOf(badCopy.Meta).Pointer() != reflect.ValueOf(source.Meta).Pointer() { + t.Error("Expected map address to be the same") + } + + deepCopy := deepCopyNode(source) + if !reflect.DeepEqual(*deepCopy, *source) { + t.Errorf("Got %+v Expected %+v", *deepCopy, *source) + } + if reflect.ValueOf(deepCopy.Attributes).Pointer() == reflect.ValueOf(source.Attributes).Pointer() { + t.Error("Expected map address to be different") + } + if reflect.ValueOf(deepCopy.Relationships).Pointer() == reflect.ValueOf(source.Relationships).Pointer() { + t.Error("Expected map address to be different") + } + if reflect.ValueOf(deepCopy.Links).Pointer() == reflect.ValueOf(source.Links).Pointer() { + t.Error("Expected map address to be different") + } + if reflect.ValueOf(deepCopy.Meta).Pointer() == reflect.ValueOf(source.Meta).Pointer() { + t.Error("Expected map address to be different") + } + +} diff --git a/response.go b/response.go index 1ff9b08..39479e7 100644 --- a/response.go +++ b/response.go @@ -206,13 +206,12 @@ func MarshalOnePayloadEmbedded(w io.Writer, model interface{}) error { // it handles the deepest models first. (i.e.) embedded models // this is so that upper-level attributes can overwrite lower-level attributes func visitModelNode(model interface{}, included *map[string]*Node, sideload bool) (*Node, error) { - node := new(Node) - var er error modelValue := reflect.ValueOf(model).Elem() modelType := reflect.ValueOf(model).Type().Elem() + node := new(Node) nodes := make([]*Node, 0) // handle just the embedded models first for i := 0; i < modelValue.NumField(); i++ { @@ -242,14 +241,19 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool er = err break } + // append to array to process together in order to find dominant field conflicts nodes = append(nodes, embNode) } - node.merge(combineNodes(nodes)) + // treat all embedded structs on this level as peers + // combine w/ func that track for dominant field conflicts + // set combined as the initial node value + node = combinePeerNodes(nodes) } - // handle everthing else + // track all attributes through attr vs node.Attributes, so that we can track dominant field conflicts on this level attrs := attributes{} + // handle everthing else for i := 0; i < modelValue.NumField(); i++ { fieldValue := modelValue.Field(i) fieldType := modelType.Field(i) @@ -515,8 +519,12 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool node.Meta = metableModel.JSONAPIMeta() } + // assign; attrs values will overwrite conflicting values in node.Attributes node.mergeAttributes(attrs) - node.cleanupDominantFieldIssues() + // this currently handles just the dominant field conflicts + // the standard json library just drops these attributes that have conflicts; and this does the same + // however, it might be useful to surface these as an explicit error + node.handleNodeErrors() return node, nil } From ca31c07c76fee58a35a07f83dd3bdd55947a24aa Mon Sep 17 00:00:00 2001 From: Stephan Kim Date: Thu, 3 Aug 2017 11:49:00 -0700 Subject: [PATCH 3/4] rename embeded_structs_test.go to embedded_structs_test.go --- embeded_structs_test.go => embedded_structs_test.go | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename embeded_structs_test.go => embedded_structs_test.go (100%) diff --git a/embeded_structs_test.go b/embedded_structs_test.go similarity index 100% rename from embeded_structs_test.go rename to embedded_structs_test.go From d00c64cefbea1a2a5174270d35c406016e2fd400 Mon Sep 17 00:00:00 2001 From: Stephan Kim Date: Wed, 16 Aug 2017 13:26:57 -0700 Subject: [PATCH 4/4] combine nodes should be outside of loop --- response.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/response.go b/response.go index 39479e7..1403c0e 100644 --- a/response.go +++ b/response.go @@ -245,11 +245,11 @@ func visitModelNode(model interface{}, included *map[string]*Node, sideload bool nodes = append(nodes, embNode) } - // treat all embedded structs on this level as peers - // combine w/ func that track for dominant field conflicts - // set combined as the initial node value - node = combinePeerNodes(nodes) } + // treat all embedded structs on this level as peers + // combine w/ func that track for dominant field conflicts + // set combined as the initial node value + node = combinePeerNodes(nodes) // track all attributes through attr vs node.Attributes, so that we can track dominant field conflicts on this level attrs := attributes{}