Embedded structs v2 #78
Conversation
|
I can confirm that this PR works as expected. Thanks @skimata! |
|
Following up: any updates on this? This would be nice to have. |
| if shouldIgnoreField(tag) { | ||
| continue | ||
| } | ||
| model := reflect.ValueOf(modelValue.Field(i).Addr().Interface()) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
My gometalinter actually caught this not me :)
|
|
||
| // 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 |
There was a problem hiding this comment.
What did you mean by "poly"?
There was a problem hiding this comment.
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 shouldIgnoreField(tag) { | ||
| continue | ||
| } | ||
| model := modelValue.Field(i).Addr().Interface() |
There was a problem hiding this comment.
see similar comment, I'll pass directly to unmarshalNode()
| } | ||
|
|
||
| func isEmbeddedStruct(sField reflect.StructField) bool { | ||
| if sField.Anonymous && sField.Type.Kind() == reflect.Struct { |
There was a problem hiding this comment.
Can be simplified by returning the comparison; don't need if
| } | ||
|
|
||
| func shouldIgnoreField(japiTag string) bool { | ||
| if strings.HasPrefix(japiTag, annotationIgnore) { |
There was a problem hiding this comment.
Can be simplified by returning the comparison; don't need if
| } | ||
|
|
||
| // assert encoding from model to jsonapi output | ||
| expected := `{"data":{"type":"things","id":"1","attributes":{"bar":"barry","bat":"batty","buzz":99,"fizz":"fizzy","foo":"fooey"}}}` |
There was a problem hiding this comment.
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
}| model.Bat = "batty" | ||
|
|
||
| buf := bytes.NewBuffer(nil) | ||
| if err := MarshalOnePayload(buf, model); err != nil { |
There was a problem hiding this comment.
Needs to be updated to MarshalPayload (if you want []byte) or Marshal if you want a Payloader
| } | ||
|
|
||
| type Model struct { | ||
| Thing |
There was a problem hiding this comment.
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.
|
Refer to: #100 |
4 participants
(this is a cleaner PR)