Skip to content
This repository was archived by the owner on Sep 4, 2025. It is now read-only.

Conversation

@keighl
Copy link
Contributor

@keighl keighl commented Dec 19, 2016

Here's a an implementation for supporting document links.

To include relationships links, implement the RelationshipLinkable interface on a parent struct. It'll be invoked by the marshaler for each defined relationship:

func (post Post) JSONRelationshipLinks(relation string) *map[string]jsonapi.Link {
	if relation == "comments" {
		return &map[string]jsonapi.Link{
			"related": {
				Href: fmt.Sprintf("https://example.com/posts/%d/comments", post.ID),
				// Optionally include a meta payload
				Meta: map[string]interface{}{
					"count": len(post.Comments),
				},
			},
		}
	}
	return nil
}

To include document links, implement the Linkable interface on a struct:

func (post Post) JSONLinks() *map[string]jsonapi.Link {
	return &map[string]jsonapi.Link{
		"self": {
			Href: fmt.Sprintf("https://example.com/posts/%d", post.ID),
		},
	}
}

@shwoodard shwoodard requested a review from aren55555 January 6, 2017 20:48
@shwoodard shwoodard self-assigned this Jan 6, 2017
@svperfecta
Copy link

svperfecta commented Jan 10, 2017

It's pretty cool that we have not only one, but two Link implementations. (IE: #58 ) Very cool! This is important for us (as is Meta). Specific use cases for links on our end at the moment are pagination related (we'd like to do previous and next links). For meta, we'd like to return some idea of total results found.

After looking at both implementations, I think I'd like to vote for this one because it feels flexible, and doesn't add any additional fields to our structs. In general I don't think LINK and META objects really have much to do with the resource objects themselves, they are really more related to the context in which the object is used. This approach keeps structs cleaner.

@shwoodard wondering if you guys wanted to make an "official" call on these, we'd like to stick with whatever is blessed by you guys.

@keighl
Copy link
Contributor Author

keighl commented Jan 10, 2017

One thing to note; this pull doesn't specifically address "top-level" links. The interface approach (e.g. Linkable) would work OK for marshal-one routines, but is kind of weird for marshal-many (models []interface{}).

In my opinion, MarshalOne/MarshalMany already facilitate top-level links pretty well. E.g:

payload, _ := jsonapi.MarshalMany(blogPosts)

payload.Links = map[string]jsonapi.Link{
	"self": {
		Href: "https://example.com/api/blogs/1/posts",
	},
}

// Then JSON encode the payload myself...
json.NewEncoder(w).Encode(payload)

@shwoodard
Copy link
Contributor

We're pretty sure this is going to be the design we go with on this one. Doing final review and local testing. Should have a conclusion within a day or two.

This was referenced Jan 11, 2017
@aren55555
Copy link
Contributor

I've reviewed this PR thoroughly; and as far as I can tell there is only one minor modification required.

The JSON API spec allows links objects to be of the form (map[string]string):

 "links": {
    "self": "http://example.com/articles?page[number]=3&page[size]=1",
    "first": "http://example.com/articles?page[number]=1&page[size]=1",
    "prev": "http://example.com/articles?page[number]=2&page[size]=1",
    "next": "http://example.com/articles?page[number]=4&page[size]=1",
    "last": "http://example.com/articles?page[number]=13&page[size]=1"
  }

or

"links": {
    "self": "/articles/1/relationships/author",
    "related": "/articles/1/author"
}

However this PR only enables the later (map[string]Link). To solve this I think both OnePayload and ManyPayload need to implement the Links field as Links *map[string]interface{}. Then when marshalling reflect would have to be used; if a string write it out as the value; if a Link build the nested map representation; if something else error.

Copy link
Contributor

@aren55555 aren55555 left a comment

Choose a reason for hiding this comment

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

Need to support string links:

 "links": {
    "self": "http://example.com/articles?page[number]=3&page[size]=1"
  }

See my comment for more info.

…ible for different allowed links spec implementations.
@keighl
Copy link
Contributor Author

keighl commented Jan 15, 2017

Hey @aren55555. Yeah that makes sense; both link styles should be possible. I think just a vanilla map[string]interface{} would be most appropriate. The user can build the links structure either way.

Question: is there a specific reason the Links fields are pointers (*map[string]interface{})? Seems like an uncommon pattern to me. Marshaling responses work perfectly on values.

My vote is change 'em to Links map[string]interface{}. But I understand if you guys want to avoid changing the public API.

@aren55555
Copy link
Contributor

aren55555 commented Jan 17, 2017

@keighl the reason for the ptr is that it enables the serialization of an empty links object; &map[string]interface{}{} would result in:

"links": {}

Without a ptr map[string]interface{}{} would be the default zero value. It then eliminates the developer's flexibility to specify whether the links should not be present or if it should just be empty like the JSON above.

@keighl
Copy link
Contributor Author

keighl commented Jan 18, 2017

@aren55555 Ah, I see. Thanks for the explanation

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@aren55555 aren55555 requested a review from shwoodard January 20, 2017 18:55
* master:
  Skimata's null relationships + fixes (#62)
  Simplify the MarshalMany implementation. For & range rather than for with int inc; etc. (#59)

# Conflicts:
#	node.go
#	response.go
…y user implements the interface it is clear the functions are used for JSON API as opposed to JSON
Copy link
Contributor

@shwoodard shwoodard left a comment

Choose a reason for hiding this comment

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

LGTM

@aren55555 aren55555 merged commit 46a5b96 into google:master Jan 21, 2017
aren55555 added a commit to lulezi/jsonapi that referenced this pull request Jan 24, 2017
* stash-master:
  Update the go versions
  Use constants for status codes, HTTP methods and JSON API media types in comments and readme
  Add support for 'links' via optional interfaces (google#57)
  Skimata's null relationships + fixes (google#62)
  Simplify the MarshalMany implementation. For & range rather than for with int inc; etc. (google#59)
@keighl
Copy link
Contributor Author

keighl commented Jan 26, 2017

Cool! Thanks for your feedback and tweaks, @shwoodard @aren55555

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants