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

Hypermedia responses #13

Merged
merged 7 commits into from
Dec 27, 2013
Merged

Hypermedia responses #13

merged 7 commits into from
Dec 27, 2013

Conversation

technoweenie
Copy link
Member

This moves the mediaheader package into hypermedia. Hypermedia is now broken up to separate the different ways of pulling hypermedia for an HTTP response. The resource itself is only checked automatically if it's decoded through the response's media type.

This is written for #12 and should be applied first.

@technoweenie
Copy link
Member Author

@jingweno: Would love your thoughts. This lets us get away with skipping the inject library. It also gives me a handy property to cache all of the hypermedia for a response in #12. It shouldn't matter where the links come from.

@owenthereal
Copy link
Member

@technoweenie The refactoring looks good to me. But we prob. need to pay attention to race condition on the lazily loaded fields, if we're not setting the fields ahead of time at the JSON serialization step.

@technoweenie
Copy link
Member Author

Ah good point. I might even take that out since that Rels() shouldn't ever really be used.

I also don't intend to support these resource objects used in a shared context. They should be initialized on every request, or passed around in channels.

@owenthereal
Copy link
Member

This got me thinking though why we're using two approaches to set the relations, one with lazy load, the other with reflection. I would still call the latter approach a DI (dependency injection) one, even though we don't use an library.

@owenthereal
Copy link
Member

In case my last comment doesn't much sense, we're supposed to use the HyperFieldRelations function to fill the relations as part of the JSON serialization step, right?

@owenthereal
Copy link
Member

Sorry, serialization => deserialization

@technoweenie
Copy link
Member Author

The "HyperField" relations are to support the GitHub API (due to our initial support of *_url properties). We could probably grab HAL links through reflection by looking for the Links field (or the _links json tag).

@owenthereal
Copy link
Member

Ah, ok, I think I'm more clear now. Would you think it makes sense to set both HyperFieldRelations and the HALResource relation at the deserialization step?

// in Response.Decode

func (r *Response) Decode(resource interface{}) error {
  // decode with media type decoder

  // fill relations by using HyperFieldRelations and some code from HALResource
}

It'll remove the lazy fields loading and make setting the relations consistent at one place: after the JSON deserialization steps.

@technoweenie
Copy link
Member Author

That's how it works: https://github.com/lostisland/go-sawyer/pull/13/files#diff-b01558cabf60ed66d8a2c08b2bfaba31R56

Response.fillRels() is called after decoding if there aren't any errors.

The Rels() function is no longer needed and can be removed. The goal of this PR is to store the relations in the sawyer.Response, regardless if they come from the header, HAL, or reflected from the resource's fields.

@owenthereal
Copy link
Member

👍

@technoweenie
Copy link
Member Author

Okay, I removed Rels(). This test highlights how this should all work

technoweenie added a commit that referenced this pull request Dec 27, 2013
@technoweenie technoweenie merged commit 66a1719 into master Dec 27, 2013
@technoweenie technoweenie deleted the hypermedia-responses branch December 27, 2013 16:40
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.

None yet

2 participants