Skip to content
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

Add unmarshalling of Links #4

Merged
merged 7 commits into from
Apr 1, 2021
Merged

Add unmarshalling of Links #4

merged 7 commits into from
Apr 1, 2021

Conversation

omarismail
Copy link

@omarismail omarismail commented Mar 31, 2021

Description

According to the JSON:API Spec, an object may contain a self link:

{
  "type": "articles",
  "id": "1",
  "attributes": {
    "title": "Rails is Omakase"
  },
  "links": {
    "self": "http://example.com/articles/1"
  }
}

We did not have the ability to unmarshal an object that had the links annotation to it. This PR adds the ability to unmarshal it.

Tests

$ go test -v ./...
...
PASS
ok      _/Users/omarismail/work/hashi-jsonapi/examples  0.198s

@omarismail omarismail requested a review from a team March 31, 2021 17:56
models_test.go Outdated
PostID int `jsonapi:"attr,post_id"`
Body string `jsonapi:"attr,body"`
Links *Links `jsonapi:"links,omitempty"`
Relation *SingleRelationLink `jsonapi:"relation,single-relation-link"`
Copy link
Author

@omarismail omarismail Apr 1, 2021

Choose a reason for hiding this comment

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

This is used for tests. This is added because relations, as currently implemented, only work for the data field. But according the spec:

A “relationship object” MUST contain at least one of the following: links, data, meta

Right now, it only works for data, so this adds it for links. Note: I have not tested or checked to see if this works for meta, but my says it does not.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds silly, but it took me a bit to get my bearings: I'd consider renaming this to be consistent with the toy things you're testing here (Car, Post, Comment, etc) and not Relation and SingleRelationLink. Maybe something like adding LikedBy to Post with *UserRelationLink (a link to a collection of users who liked the post). But anything else, really, if you can think of what makes sense with Comment!

linkModelValue := linkModel.Elem()
linkModelType := linkModelValue.Type()

for i := 0; i < linkModelValue.NumField(); i++ {
Copy link
Author

Choose a reason for hiding this comment

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

We are iterating through the model because this is the only way to find the attribute on the struct that has the jsonapi:"links" tag. So once we find it, we unmarshal it onto the model (the Resource struct).

}
}
if assignedValue {
models = reflect.Append(models, linkModel)
Copy link
Author

Choose a reason for hiding this comment

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

We are appending here because we are operating when the current relationship is a list of relations.

@@ -324,9 +366,23 @@ func unmarshalNode(data *Node, model reflect.Value, included *map[string]*Node)
}

fieldValue.Set(m)
}

} else if annotation == annotationLinks {
Copy link
Author

Choose a reason for hiding this comment

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

This is to handle when the a Resource struct has a jsonapi:"links" tag directly on it. In which case, we just unmarshal it directly.

"body": "First",
"title": "Post",
},
"links": map[string]string{
Copy link
Author

Choose a reason for hiding this comment

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

Tests when links are at the same level as the other jsonapi tags. Relates to this

request_test.go Outdated
"title": "Post",
},
"relationships": map[string]interface{}{
"single-relation-link": map[string]interface{}{
Copy link
Author

Choose a reason for hiding this comment

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

Tests when a relation only has links and does not have data or meta. Relates to this

// of `jsonapi:"links"` is done below via the
// Linkable interface.
// And the logic for Marshalling links should be done
// via that interface.
Copy link
Author

Choose a reason for hiding this comment

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

This function is called during MarshalPayload. The links functionality here is handled through the Linkable interface. We don't want to do anything here because if the resource struct implements the interface, then links will be handled there. Otherwise, we just want to handle the annotaiton or else it will blow up below.

Copy link
Member

@chrisarcand chrisarcand left a comment

Choose a reason for hiding this comment

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

Solid, looks good to me. One suggestion:

models_test.go Outdated
PostID int `jsonapi:"attr,post_id"`
Body string `jsonapi:"attr,body"`
Links *Links `jsonapi:"links,omitempty"`
Relation *SingleRelationLink `jsonapi:"relation,single-relation-link"`
Copy link
Member

Choose a reason for hiding this comment

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

Sounds silly, but it took me a bit to get my bearings: I'd consider renaming this to be consistent with the toy things you're testing here (Car, Post, Comment, etc) and not Relation and SingleRelationLink. Maybe something like adding LikedBy to Post with *UserRelationLink (a link to a collection of users who liked the post). But anything else, really, if you can think of what makes sense with Comment!

@omarismail omarismail merged commit 95d5725 into master Apr 1, 2021
@omarismail omarismail deleted the omarismail/links branch April 1, 2021 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants