Unmarshalling of nested structs #99

mrowdy · 2017-07-15T13:52:10Z

This mainly addresses #49 and has nothing todo with #21

The marshalling function is able to convert a nested struct into a valid json:api representation but you can't use the output for the unmarshalPayload function since it fails when a struct is used as an attribute.

This should be possible according to the json:api spec: http://jsonapi.org/format/#document-resource-object-attributes

The PR adds this functionality with the limitation of pointers. At the moment this only works with values (as shown in the added tests)

I also had to restructure the unmarshalNode function in order to convert the attributes recursively. This should simplify the implementation of #95

…er on

googlebot · 2017-07-15T13:52:14Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

mrowdy · 2017-07-15T13:56:02Z

I signed it!

googlebot · 2017-07-15T13:56:05Z

CLAs look good, thanks!

dev-mush · 2017-08-04T11:27:02Z

hello @Slemgrim , I was having the same issues and got into your solution, I'm encountering however a few problems and before diving deep into your (and google's) code, I might use some feedback to know how are you using the tool.

There are two main problems I'm encountering:

problem 1 (tl;dr: how do you use the tags?)

Do you tag an attribute with the json or jsonapi tag for a nested struct (or well, an attribute of the main jsonapi object which is a struct)?
I found that if I use the jsonapi tag like the following:

package funnymodel 

type Person struct{
        ID string `jsonapi:"primary,persons"`
	Name  string `jsonapi:"attr,name`
	Contacts []Contact `jsonapi:"attr,contacts"`
}

type Contact struct {
	Name  string `jsonapi:"attr,name,omitempty"`
	Value string `jsonapi:"attr,value,omitempty"`
}

when marshalling, the fields of the Contact struct, they get translated into json with the struct's attribute names rather than the jsonapi tag values (i.e. : [{"Name":"work","Value":"fakemail@fakeworld.com"}] instead of [{"name":"work","value":"fakemail@fakeworld.com"}], notice the capital letters? I didn't for a long time and was getting crazy -_-).

If I use the json tag instead, everything seems smooth marshalling, but the unmarshalling fails, it basically skips the attributes and initialises them to their default empty value.
A mix of the two, meaning to put both the json and jsonapi tag, seems to work ok: I get to serialize and deserialize correctly, but it doesn't feel right to me.
I think the ideal would be to use just the json tag alone in nested structs, since there is no need (and it would be wrong) to specify jsonapi relations and other things in an attribute of an object.

problem 2

If I set omitemptyon a struct array, like on the []Contactsof the previous example, I get the following panic, wether the array is empty or not: panic: runtime error: comparing uncomparable type []funnymodel.Contact.
I assume the problem must be in thevisitModelNode() function, but it's quite a hell of a long function and I didn't have time to debug it yet.
If I cut away the omitempty tag, the attribute gets parsed to json as "null" when it is empty, like: "contacts":null, despite it is not a pointer and should be translated as "contacts":[] or omitted entirely (with the omitempty).
Right now I don't have enough time to deep dive into the visitModelNode function and see why the panic occurs in first place (deadlines, you know...) but as soon as I'll be more free, I'll be glad to help in case you won't figure it out by yourself in the meanwhile.

Thanks for your solution anyway, it is saving me some headaches already, hope to hear from you soon ;).

mrowdy · 2017-08-05T06:55:36Z

@dev-mush you're right. i don't see a reason why we should use jsonapi annotation inside nested structs. I haven't thought about that before. I try to update the PR in the next days.

Can you create a separate Issue for the second problem?

…r made sense to use json:api annotation in nested structs

mrowdy · 2017-08-14T18:50:06Z

@dev-mush i changed the PR to work with json annotation instead of json:api. Does the solution work for you?

we're waiting for google/jsonapi to implement nested structs or to accept this PR google/jsonapi#99 We need nested structs for the upcoming mail endpoint

shwoodard

I like this PR. It offers some much needed in addition to its intent. Can you add more informative test failures? Like expected x got y instead of just, it failed.

shwoodard · 2017-11-19T22:29:38Z

@Slemgrim I'd really like to merge this, especially because it introduces a lot of much needed cleanup. A couple of questions,

Is there a fundamental limitation preventing supporting ptr declared nested structs? If not, can you add support?
Can you augment your test failures to include expected vs actual outcomes so that the tests are easier to debug?

I'd like to merge this asap so that we can avoid conflicts with other PRs.

Thanks in advance, Sam

tests should have an expected outcome vs. an actual outcome to make them easier to debug

mrowdy · 2017-11-21T11:15:36Z

@shwoodard I changed the tests to display expected vs. actual outcome.

About the support for nested ptr:
I think it should be possible but I had some difficulties with go's reflection system. I'd be happy if someone with more experience can add this feature.

shwoodard · 2017-11-21T19:00:25Z

@Slemgrim I'll look into it. The key is the use of the .Elem() method.

https://golang.org/pkg/reflect/#Value.Elem

msabramo · 2018-01-17T21:17:42Z

I opened a PR to @Slemgrim's branch that makes nested struct pointers work for me: mrowdy#3

to aid in troubleshooting. Before: ``` Pointer type in struct is not supported ``` After: ``` jsonapi: Can't unmarshal true (bool) to struct field `Name`, which is a pointer to `string` ```

shwoodard · 2018-03-13T01:05:42Z

We're working on reviewing this CL now. We're going to merge this before any others. Everyone else who has open PR's please rebase on top of this branch or wait for it to be merged within a week or two.

shwoodard

Please fix all lines to 80 chars. Thanks for the much needed cleanup help!

shwoodard · 2018-03-13T01:14:15Z

request.go

 	// ErrInvalidType is returned when the given type is incompatible with the expected type.
 	ErrInvalidType = errors.New("Invalid type provided") // I wish we used punctuation.
 )

+// ErrUnsupportedPtrType is returned when the Struct field was a pointer but
+// the JSON value was of a different type
+func ErrUnsupportedPtrType(rf reflect.Value, t reflect.Type, structField reflect.StructField) error {


Please refactor this so as to use the error interface, the standard Go pattern for errors that have state.

Create a public/exported struct called called ErrUnsupportedPtrType with private (lower-case) fields rf, t, and structField as members.

Implement func (eupt ErrUnsupportedPtrType) Error() string to complete the error interface, See, https://golang.org/src/builtin/builtin.go?h=error#L255.

Create a private/unexported function called newErrUnsupportedPtrType and take the formal params that are listed above, reflect.Value, reflect.Type, reflect.StructField and returns a ErrUnsupportedPtrType, for convenience.

Use return newErrUnsupportedPtrType(...) in place of your exported function here.

shwoodard · 2018-03-13T01:16:25Z

request.go

@@ -548,3 +374,239 @@ func assign(field, value reflect.Value) {
 		field.Set(reflect.Indirect(value))
 	}
 }
+
+func unmarshalAttribute(attribute interface{}, args []string, structField reflect.StructField, fieldValue reflect.Value) (value reflect.Value, err error) {
+


Remove whitespace.

shwoodard · 2018-03-13T01:17:08Z

request.go

+	if kind.String() != "" && kind.String() != typeName {
+		typeName = fmt.Sprintf("%s (%s)", typeName, kind.String())
+	}
+	return fmt.Errorf(


When you do the change described above, you'll need to change this to fmt.Sprintf to return string.

shwoodard · 2018-03-13T01:17:51Z

request.go

@@ -118,6 +129,7 @@ func UnmarshalManyPayload(in io.Reader, t reflect.Type) ([]interface{}, error) {
 }

 func unmarshalNode(data *Node, model reflect.Value, included *map[string]*Node) (err error) {
+


Remove whitespace

shwoodard · 2018-03-13T01:21:37Z

request.go

+}
+
+func handleTime(attribute interface{}, args []string, fieldType reflect.Type, fieldValue reflect.Value) (reflect.Value, error) {
+


Remove whitepace.

shwoodard · 2018-03-13T01:23:01Z

request.go

+	return numericValue, nil
+}
+
+func handlePointer(attribute interface{}, args []string, fieldType reflect.Type, fieldValue reflect.Value, structField reflect.StructField) (reflect.Value, error) {


80 chars, etc

request.go

shwoodard · 2018-03-13T01:33:38Z

request.go

+func handleStruct(attribute interface{}, args []string, fieldType reflect.Type, fieldValue reflect.Value) (reflect.Value, error) {
+	model := reflect.New(fieldValue.Type())
+
+	var er error


shwoodard · 2018-03-13T01:33:48Z

request.go

+
+	var er error
+
+	data, er := json.Marshal(attribute)


Rename err and update all below.

request_test.go

shwoodard · 2018-03-13T01:38:20Z

@Slemgrim, Thank you so much for your contributions! I look forward to getting this merged soon.

Refactor ptr error and cleanup

mrowdy

@shwoodard i changed all the requested things except for the switch. Maybe you can spot why it doesn't work if the two primitive types are added.

Thanks you for the feedback!

request.go

msabramo · 2018-03-17T21:03:01Z

Awesome! Looking forward to this!

driquet · 2018-03-20T21:38:28Z

I look forward to this PR being merged soon as well! Greatly needed here, good job!

korfuri · 2018-04-11T13:21:47Z

Awesome work, thank you! Is there an estimate on when this could be merged? I'd love to be able to use this.

valentindotxyz · 2018-04-11T13:25:40Z

Hoping it will be merged soon too! 🙏🏻

mrowdy · 2018-04-14T11:45:15Z

@shwoodard is there anything i can do to get this merged soon?

nathanKramer · 2018-05-24T22:35:40Z

Looking forward to this being merged :)

svanharmelen · 2018-05-29T11:12:57Z

@shwoodard and @aren55555 I would like to join the list of people that would be really happy if this PR can get merged! It seems the authors and yourselves have given it quite some attention, so would be really awesome if you could give it a last push in order to merge the PR 😃Thanks!

svanharmelen · 2018-06-14T18:33:54Z

@shwoodard and @aren55555 sorry to bother you, but is there anyone who could give this project some TLC? It seems the last activity from any of it's maintainers is about 3 months ago, while there are a few very valuable PRs pending that solve quite some issues and/or use cases. So would be really great if someone could review and/or merge a few of them so we don't end up will everyone having it's own custom fork and lose the ability to share back 😏Thanks!

msabramo · 2018-07-21T20:55:12Z

Maybe if the current maintainers are too busy to tend to this project, perhaps they might consider adding a few more committers?

marcgiovannoni · 2018-07-23T03:24:26Z

Any update on this?

svanharmelen · 2018-07-23T07:33:30Z

Anything that can be done to move this forward?

aren55555 · 2018-07-25T04:13:57Z

models_test.go

+}
+
+type Team struct {
+	Name    string     `json:"name"`


I've read the comment @dev-mush left.

I think all these tags (even for a nested resource) should all be jsonapi rather than json. Why? What if you want to support serializations for both JSON and JSONAPI; you'd want separate tags for each. I'd recommend just using jsonapi:"attr,names" even for these nested struct's fields.

The rest of this PR seems OK to me. @shwoodard perhaps you can also take a quick look?

@aren55555 according to the spec attributes need to be valid json but can't be a json:api object since they are not allowed to have links or relationships.

Maybe I get this totaly wrong but this would allow the following:

{ "data": { "id": "1324", "type": "article", "attributes": { "title": "The best article", "author": { "id": "18", "type": "author", "attributes": { "name": "Slemgrim" } } } } }

where you also could do this:

{ "data": { "id": "1324", "type": "article", "attributes": { "title": "The best article", } }, "relationships": { "author": { "data": {"id": "18", "type": "author"} } }, "included": [{ "id": "18", "type": "author", "attributes": { "name": "Slemgrim" } }] }

Could the unmarshal/marshal not just return an error if anything other than an attr is serialized/deserialized? Or it could ignore any tag that was not an attr?

@Slemgrim bump on question from @aren55555 . If we can resolve this, we'll merge this immediately.

@aren55555 @shwoodard I tried to look into this today. The changes required to allow "jsonapi" marshalling for nested attributes, seem non-trivial with the current architecture. At least non-trivial to me.

I'm happy to spend my time on this if someone could give me an example where this would be needed.

The jsonapi spec does not allow the fields "id, type, relationship, links" as attribute names. In other words: it doesn't allow to have a resource inside a resource. Allowing to tag the attributes of a non jsonapi resource with jsonapi tags would be missleading. It's like saying: "hey, you can name this json value jsonapi even though it will always be a normal json"

If you insist on this without further explanation then maybe one of the other contributers of this PR are willing to implement this. (@msabramo)

Could the solution be to validate that id, type, and links are missing from nested structs? Maybe we should allow relationships and process the data to be in the "included" part of the response?

I agree with @aren55555 that we should use jsonapi for all properties serialized or deserialized by this lib.

adamlc · 2018-08-29T14:09:36Z

Any more updates on this? We really need this :(

Use jsonapi struct tags for nested attrs

shwoodard · 2018-10-04T16:29:58Z

I'm ready to merge the PR. I need all contributors to confirm CLA(s).

@msabramo I think you're consent may be missing.

PothulaAdiseshu · 2020-03-03T13:51:57Z

@Slemgrim @dev-mush I could still face Problem 1 in the repo.. Is it because of overriding or am i missing something? can anyone suggest on this ?

* Change TLS certificate `name` attribute to be optional Custom TLS certificate without names default to the certificates common name or first SAN entry * Not use uint pointers in ListPrivateKeysInput struct This was causing problems when trying to use the page[number] parameter so I brought it more in line with the other methods. * Change TLS Activation `configuration` attribute to be optional (#28) Activations without the configuration specified default to the default configuration * Add TLS domains endpoint support * Drop use of reflection to build request parameters * Add documentation * TLS Subscriptions endpoints (#30) * Add ListTLSSubscriptions function * Add CreateTLSSubscriptions function * Add GetTLSSubscriptions function * Add DeleteTLSSubscriptions function * Add integration test with all endpoints * Add TLS Authorizations to TLS Subscriptions (#31) I had a lot of trouble deserialising the API response into appropriate structs. Initially I could only get it to work by deserialising into a map[string]interface{} but this didn't give the rich typing I wanted to return to the user. After a long time digging through the JSONAPI implementation and an associated PR to allow nested structs (google/jsonapi#99), I found that the go-fastly dependency was on an older version of the library before this PR came in. Updating the version, and playing around some more with the slice of structs instead of slice of pointers, finally got this to work. Furthermore I found that the Header in ListTLSSubscriptions needed to include the "Accept" header for "application/vnd.api+json" so I added it back in. I previously removed it as it didn't seem to do anything with the Filter and Pagination parameters, but it seems to be required for Include to work. * Avoid use pointers and uint in ListBulkCertificatesInput struct * Add allow_untrusted_root field to platform TLS * Get Certificate ID for TLS Subscriptions when available The certificate can't really be queried in the same way as the custom TLS certificate, but the ID will be present in the TLS activation automatically created for the TLS subscription, so can help filter for this activation. * Remove (TLS)\w* from field names of TLS structs * Add CommonName to Create, Get, and List TLSSubscription functions * Add "force" flag to TLS Subscription deletion Allows deleting a subscription with active domains; a potentially dangerous operation if the subscription handles production traffic. By using a flag, the user opts in to this risky behaviour and takes responsibility for knowing what they are doing. * go mod tidy * Add comments describing new structs and function * Document TLSAuthorizations struct Co-authored-by: Will May <will.j.may@gmail.com>

mrowdy added 6 commits July 10, 2017 22:09

extract type handling

e13a199

Merge branch 'master' of https://github.com/Slemgrim/jsonapi

ec077ed

Merge remote-tracking branch 'upstream/master'

f0b268e

unify type handler

ab915cc

extract attribute handling into custom method for recursive usage lat…

0c97f0c

…er on

handle nested struct slices

af21dba

mrowdy added 2 commits August 14, 2017 20:39

remove duplicated code

b391a84

change nested structs to json annotation instead of json:api. It neve…

fc6968d

…r made sense to use json:api annotation in nested structs

mrowdy added a commit to mrowdy/styx that referenced this pull request Aug 15, 2017

change to slemgrim/jsonapi

2ce2e7f

we're waiting for google/jsonapi to implement nested structs or to accept this PR google/jsonapi#99 We need nested structs for the upcoming mail endpoint

shwoodard reviewed Nov 10, 2017

View reviewed changes

mrowdy added 3 commits November 21, 2017 18:42

Merge remote-tracking branch 'upstream/master'

7e5c901

Add more speaking tests

a6577df

tests should have an expected outcome vs. an actual outcome to make them easier to debug

fix wrong type for formatting

0400041

msabramo mentioned this pull request Jan 17, 2018

Show type for ErrUnsupportedPtrType #88

Open

Test more things in TestUnmarshalNestedStruct

e3c0871

This was referenced Jan 17, 2018

Test more things in TestUnmarshalNestedStruct mrowdy/jsonapi#1

Merged

Make nested struct pointers work mrowdy/jsonapi#3

Merged

msabramo added 2 commits January 17, 2018 13:02

Add TestUnmarshalNestedStructPtr

b28beab

Make nested struct pointers work

16e19ab

Show type for ErrUnsupportedPtrType

a3b3bb2

to aid in troubleshooting. Before: ``` Pointer type in struct is not supported ``` After: ``` jsonapi: Can't unmarshal true (bool) to struct field `Name`, which is a pointer to `string` ```

shwoodard reviewed Mar 13, 2018

View reviewed changes

Markus Ritberger and others added 4 commits March 14, 2018 20:52

replace public function with custom error type

9bc94d8

check for ptr error type in tests

72f7bad

remove whitespaces and stick to 80chars

d490a0f

Merge pull request #5 from Slemgrim/refactor-ptr-error

8b7e0bc

Refactor ptr error and cleanup

mrowdy commented Mar 14, 2018

View reviewed changes

request.go Show resolved Hide resolved

aren55555 reviewed Jul 25, 2018

View reviewed changes

use jsonapi tags for nested attrs

3c8221b

shwoodard mentioned this pull request Oct 4, 2018

Use jsonapi struct tags for nested attrs mrowdy/jsonapi#6

Merged

Merge pull request #6 from shwoodard/shwoodard-slemgrim-pull-99

417d4eb

Use jsonapi struct tags for nested attrs

shwoodard mentioned this pull request Oct 4, 2018

Support unmarshalling of nested attributes #157

Closed

shwoodard merged commit bdc73a2 into google:master Oct 4, 2018

shwoodard mentioned this pull request Oct 5, 2018

Add support for attributes of custom defined types #159

Merged

		@@ -118,6 +129,7 @@ func UnmarshalManyPayload(in io.Reader, t reflect.Type) ([]interface{}, error) {
		}

		func unmarshalNode(data Node, model reflect.Value, included map[string]*Node) (err error) {

		}

		func handleTime(attribute interface{}, args []string, fieldType reflect.Type, fieldValue reflect.Value) (reflect.Value, error) {

Unmarshalling of nested structs #99

Unmarshalling of nested structs #99

Conversation

mrowdy commented Jul 15, 2017

googlebot commented Jul 15, 2017

mrowdy commented Jul 15, 2017

googlebot commented Jul 15, 2017

dev-mush commented Aug 4, 2017

problem 1 (tl;dr: how do you use the tags?)

problem 2

mrowdy commented Aug 5, 2017

mrowdy commented Aug 14, 2017

shwoodard left a comment

Choose a reason for hiding this comment

shwoodard commented Nov 19, 2017

mrowdy commented Nov 21, 2017

shwoodard commented Nov 21, 2017

msabramo commented Jan 17, 2018

shwoodard commented Mar 13, 2018

shwoodard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shwoodard commented Mar 13, 2018

mrowdy left a comment

Choose a reason for hiding this comment

msabramo commented Mar 17, 2018

driquet commented Mar 20, 2018

korfuri commented Apr 11, 2018

valentindotxyz commented Apr 11, 2018

mrowdy commented Apr 14, 2018

nathanKramer commented May 24, 2018

svanharmelen commented May 29, 2018

svanharmelen commented Jun 14, 2018

msabramo commented Jul 21, 2018

marcgiovannoni commented Jul 23, 2018

svanharmelen commented Jul 23, 2018

aren55555 Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aren55555 Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shwoodard Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamlc commented Aug 29, 2018

shwoodard commented Oct 4, 2018

PothulaAdiseshu commented Mar 3, 2020

aren55555 Jul 25, 2018 •

edited

Loading

aren55555 Jul 25, 2018 •

edited

Loading

shwoodard Sep 19, 2018 •

edited

Loading