Skip to content

Embedded structs v2 #78

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Embedded structs v2 #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0bfd2ea
working version
skimata Feb 4, 2017
8e20fb2
fix text
skimata Feb 6, 2017
e6596dc
combine test files
skimata Feb 6, 2017
5dbf82f
move private funcs to bottom
skimata Feb 6, 2017
8658417
ErrInvalidType should ignore interfaces
skimata Feb 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const (
annotationOmitEmpty = "omitempty"
annotationISO8601 = "iso8601"
annotationSeperator = ","
annotationIgnore = "-"

iso8601TimeFormat = "2006-01-02T15:04:05Z"

Expand Down
32 changes: 32 additions & 0 deletions node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,38 @@ type Node struct {
Links *Links `json:"links,omitempty"`
}

func (n *Node) merge(node *Node) {
if node.Type != "" {
n.Type = node.Type
}

if node.ID != "" {
n.ID = node.ID
}

if node.ClientID != "" {
n.ClientID = node.ClientID
}

if n.Attributes == nil && node.Attributes != nil {
n.Attributes = make(map[string]interface{})
}
for k, v := range node.Attributes {
n.Attributes[k] = v
}

if n.Relationships == nil && n.Relationships != nil {
n.Relationships = make(map[string]interface{})
}
for k, v := range node.Relationships {
n.Relationships[k] = v
}

if node.Links != nil {
n.Links = node.Links
}
}

// RelationshipOneNode is used to represent a generic has one JSON API relation
type RelationshipOneNode struct {
Data *Node `json:"data"`
Expand Down
21 changes: 18 additions & 3 deletions request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,28 @@ func unmarshalNode(data *Node, model reflect.Value, included *map[string]*Node)

for i := 0; i < modelValue.NumField(); i++ {
fieldType := modelType.Field(i)
tag := fieldType.Tag.Get("jsonapi")
tag := fieldType.Tag.Get(annotationJSONAPI)

// handles embedded structs
if isEmbeddedStruct(fieldType) {
if shouldIgnoreField(tag) {
continue
}
model := reflect.ValueOf(modelValue.Field(i).Addr().Interface())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be model =

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think whether I assign to the original argument, or shadow it here, it shouldn't make a difference since the main loop of this function works off of modelValue.

Between assign/overwriting to the original argument vs initializing a new var, I feel like the latter is preferred. But to avoid confusion, I'll pass it directly to unmarshalNode() vs using a shadowed var.

Also, note, w/ handling annotationRelation, we don't overwrite the original model argument. https://github.com/google/jsonapi/blob/master/request.go#L474

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gometalinter actually caught this not me :)

err := unmarshalNode(data, model, included)
if err != nil {
er = err
break
}
}

if tag == "" {
continue
}

fieldValue := modelValue.Field(i)

args := strings.Split(tag, ",")
args := strings.Split(tag, annotationSeperator)

if len(args) < 1 {
er = ErrBadJSONAPIStructTag
Expand Down Expand Up @@ -446,7 +460,8 @@ func unmarshalNode(data *Node, model reflect.Value, included *map[string]*Node)
}

// As a final catch-all, ensure types line up to avoid a runtime panic.
if fieldValue.Kind() != v.Kind() {
// Ignore interfaces since interfaces are poly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did you mean by "poly"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

polymorphic. But if you feel that isn't the correct term, a more accurate comment is welcome.

Why does this matter?
Basically, this "final catch-all" was introduced when I rebased, and it adversely affected our use of use interface{} struct fields as a generic JSON object.

We were doing something like this:

type Model struct {
   Raw  interface{}  `jsonapi:"attr,raw"`
}

...	

json.Unmarshal(jsonByteData, model.Raw)

if fieldValue.Kind() != reflect.Interface && fieldValue.Kind() != v.Kind() {
return ErrInvalidType
}
fieldValue.Set(reflect.ValueOf(val))
Expand Down
29 changes: 29 additions & 0 deletions response.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,21 @@ func visitModelNode(model interface{}, included *map[string]*Node,
for i := 0; i < modelValue.NumField(); i++ {
structField := modelValue.Type().Field(i)
tag := structField.Tag.Get(annotationJSONAPI)

// handles embedded structs
if isEmbeddedStruct(structField) {
if shouldIgnoreField(tag) {
continue
}
model := modelValue.Field(i).Addr().Interface()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be model =...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see similar comment, I'll pass directly to unmarshalNode()

embNode, err := visitModelNode(model, included, sideload)
if err != nil {
er = err
break
}
node.merge(embNode)
}

if tag == "" {
continue
}
Expand Down Expand Up @@ -517,3 +532,17 @@ func convertToSliceInterface(i *interface{}) ([]interface{}, error) {
}
return response, nil
}

func isEmbeddedStruct(sField reflect.StructField) bool {
if sField.Anonymous && sField.Type.Kind() == reflect.Struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified by returning the comparison; don't need if

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return true
}
return false
}

func shouldIgnoreField(japiTag string) bool {
if strings.HasPrefix(japiTag, annotationIgnore) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified by returning the comparison; don't need if

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return true
}
return false
}
202 changes: 201 additions & 1 deletion response_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"reflect"
"sort"
"strings"
"testing"
"time"
)
Expand Down Expand Up @@ -59,7 +60,7 @@ func (b *Blog) JSONAPIRelationshipLinks(relation string) *Links {
}

type Post struct {
Blog
Blog `jsonapi:"-"`
ID uint64 `jsonapi:"primary,posts"`
BlogID int `jsonapi:"attr,blog_id"`
ClientID string `jsonapi:"client-id"`
Expand Down Expand Up @@ -829,6 +830,205 @@ func TestMarshalMany_InvalidIntefaceArgument(t *testing.T) {
}
}

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{}

structType := reflect.TypeOf(foo{})
stringType := reflect.TypeOf("")
if structType.Kind() != reflect.Struct {
t.Fatal("structType.Kind() is not a struct.")
}
if stringType.Kind() != reflect.String {
t.Fatal("stringType.Kind() is not a string.")
}

type test struct {
scenario string
input reflect.StructField
expectedRes bool
}

tests := []test{
test{
scenario: "success",
input: reflect.StructField{Anonymous: true, Type: structType},
expectedRes: true,
},
test{
scenario: "wrong type",
input: reflect.StructField{Anonymous: true, Type: stringType},
expectedRes: false,
},
test{
scenario: "not embedded",
input: reflect.StructField{Type: structType},
expectedRes: false,
},
}

for _, test := range tests {
res := isEmbeddedStruct(test.input)
if res != test.expectedRes {
t.Errorf("Scenario -> %s\nGot -> %v\nExpected -> %v\n", test.scenario, res, test.expectedRes)
}
}
}

func TestShouldIgnoreField(t *testing.T) {
type test struct {
scenario string
input string
expectedRes bool
}

tests := []test{
test{
scenario: "opt-out",
input: annotationIgnore,
expectedRes: true,
},
test{
scenario: "no tag",
input: "",
expectedRes: false,
},
test{
scenario: "wrong tag",
input: "wrong,tag",
expectedRes: false,
},
}

for _, test := range tests {
res := shouldIgnoreField(test.input)
if res != test.expectedRes {
t.Errorf("Scenario -> %s\nGot -> %v\nExpected -> %v\n", test.scenario, res, test.expectedRes)
}
}
}

func TestIsValidEmbeddedStruct(t *testing.T) {
type foo struct{}

structType := reflect.TypeOf(foo{})
stringType := reflect.TypeOf("")
if structType.Kind() != reflect.Struct {
t.Fatal("structType.Kind() is not a struct.")
}
if stringType.Kind() != reflect.String {
t.Fatal("stringType.Kind() is not a string.")
}

type test struct {
scenario string
input reflect.StructField
expectedRes bool
}

tests := []test{
test{
scenario: "success",
input: reflect.StructField{Anonymous: true, Type: structType},
expectedRes: true,
},
test{
scenario: "opt-out",
input: reflect.StructField{Anonymous: true, Tag: "jsonapi:\"-\"", Type: structType},
expectedRes: false,
},
test{
scenario: "wrong type",
input: reflect.StructField{Anonymous: true, Type: stringType},
expectedRes: false,
},
test{
scenario: "not embedded",
input: reflect.StructField{Type: structType},
expectedRes: false,
},
}

for _, test := range tests {
res := (isEmbeddedStruct(test.input) && !shouldIgnoreField(test.input.Tag.Get(annotationJSONAPI)))
if res != test.expectedRes {
t.Errorf("Scenario -> %s\nGot -> %v\nExpected -> %v\n", test.scenario, res, test.expectedRes)
}
}
}

func TestMarshalUnmarshalCompositeStruct(t *testing.T) {
type Thing struct {
ID int `jsonapi:"primary,things"`
Fizz string `jsonapi:"attr,fizz"`
Buzz int `jsonapi:"attr,buzz"`
}

type Model struct {
Thing
Copy link
Contributor

@aren55555 aren55555 Jul 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some additional tests should be added to demonstrate what should happen when fields are overlapping. Ie both have a field w/ primary annotation or an attr with the same name has been used.

That behaviour will become expected by users of this lib; and such tests would ensure we don't break that expectation.

Foo string `jsonapi:"attr,foo"`
Bar string `jsonapi:"attr,bar"`
Bat string `jsonapi:"attr,bat"`
}

model := &Model{}
model.ID = 1
model.Fizz = "fizzy"
model.Buzz = 99
model.Foo = "fooey"
model.Bar = "barry"
model.Bat = "batty"

buf := bytes.NewBuffer(nil)
if err := MarshalOnePayload(buf, model); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be updated to MarshalPayload (if you want []byte) or Marshal if you want a Payloader

t.Fatal(err)
}

// assert encoding from model to jsonapi output
expected := `{"data":{"type":"things","id":"1","attributes":{"bar":"barry","bat":"batty","buzz":99,"fizz":"fizzy","foo":"fooey"}}}`
Copy link
Contributor

@aren55555 aren55555 Jul 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid adding string comparisons. Semantic JSON comparison is preferred... could add a test helper method like:

func isJSONEqual(b1, b2 []byte) (result bool, err error) {
	var i1, i2 interface{}
	if err = json.Unmarshal(b1, &i1); err != nil {
		return
	}
	if err = json.Unmarshal(b2, &i2); err != nil {
		return
	}
	result = true
	return
}

actual := strings.TrimSpace(string(buf.Bytes()))

if expected != actual {
t.Errorf("Got %+v Expected %+v", actual, expected)
}

dst := &Model{}
if err := UnmarshalPayload(buf, dst); err != nil {
t.Fatal(err)
}

// assert decoding from jsonapi output to model
if !reflect.DeepEqual(model, dst) {
t.Errorf("Got %#v Expected %#v", dst, model)
}
}

func testBlog() *Blog {
return &Blog{
ID: 5,
Expand Down