-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Support polymorphic relationships through a join struct #13
Conversation
I think this looks good and makes sense!! Well, as far as I could tell by looking through it and talking it through with a mentor who knows Go. Thanks so much for working on this!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some initial comments/questions based on reading through the code. Probably a bit too nitpicky for a POC, but hopefully useful when we choose to complete the marshaling portion and merge.
I am going to try to run the code in a debugger a bit to get a better understanding of some of the pieces
Creates a new annotation that invokes special handling for the associated field, assigning exactly one of a selection of models in an intermediate struct. (See README)
70469c8
to
625119e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this strategy a lot, and I'm excited to play around with it a bit more once it is available!
I haven't found any issues running tests through the debugger and watching what's going on, and the code makes sense to me. I'll admit I dont have much experience with reflection in Go, and it is a bit tricky, so I could be missing something...but most of the new code is only accessed when you actually use the new annotation, so it feels pretty safe to merge without breaking backward compatibility, so it is a 👍 for me. Thanks Brandon!
@JarrettSpiker Nice, thanks for the review! I removed the "POC" label and I'll smoke test this against hashicorp/go-tfe next week before releasing it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems great!
A lot of the reflect stuff is very new to me, so I don't necessarily trust my reading of the code, but it seems like @JarrettSpiker had that part under control. For my part, I went back to my gist where I was trying and failing to do custom unmarshal behavior, and got the new polymorphic relations behavior working great. 👍🏼
polyrelation fields are marshaled as JSON to the first non-nil field within a choice type
Adds test for nil hasOne polyrelation
625119e
to
bb4d09f
Compare
One way of solving polymorphic relationships in jsonapi is to use a special choice type (aka tagged union, discriminated union, sum type, etc) of potential values that get populating according to the actual underlying type. The potential types are probed for their "primary" type and exactly one appropriate field is populated.
Additions to the README:
Polymorphic relations can be represented exactly as relations, except that
an intermediate type is needed within your model struct that will be populated
with exactly one value among all the fields in that struct.
Example:
During decoding, the
polyrelation
annotation instructs jsonapi to assign each relationshipto either
Video
orImage
within the value of the associated field. This value must bea pointer to a special choice type struct (also known as a tagged union, or sum type) containing
other pointer fields to jsonapi models. The actual field assignment depends on that type having
a jsonapi "primary" annotation with a type matching the relationship type found in the response.
All other fields will be remain nil.
During encoding, the very first non-nil field will be used to populate the payload. Others
will be ignored. Therefore, it's critical to set the value of only one field within the choice
struct. When accepting input values on this type of choice type, it would a good idea to enforce
and check that the value is set on only one field.