Skip to content
This repository has been archived by the owner on Jun 20, 2022. It is now read-only.

Apply global headers #60

Merged
merged 4 commits into from
Apr 6, 2014
Merged

Apply global headers #60

merged 4 commits into from
Apr 6, 2014

Conversation

owenthereal
Copy link
Contributor

The changes allow us to have callbacks before sending requests and after receiving response (before the JSON deserialization). Previously the request/response cycle is all hidden and this'll open up an extension point. A use case in the new Hub:

type HubTestMiddleware struct {}
func (m *HubTestMiddleware) PrepareRequest(req *octokit.Request) error {
  req.Header.Set("X-HUB-HOST", "api.github.com")
}
func (m *HubTestMiddleware) PrepareResponse(resp *octokit.Response) error {
  return nil
}

...

client.Use(&HubTestMiddleware{})

And the cuke would be able to receive some custom headers.

@technoweenie
Copy link
Contributor

I prefer something like the RoundTripper interface to a middleware system.

What kind of stuff do you imagine setting in this in Octokit? I figure Octokit should support for various custom headers that the GitHub API needs.

@owenthereal
Copy link
Contributor Author

The intention is to being able to set extra headers so that the cukes of the new Hub are happy. For example, I need to set a HOST_NAME header to make the test pass.

RoundTripper looks pretty cool. But the doc says:

RoundTrip should not modify the request, except for
consuming and closing the Body. The request's URL and
Header fields are guaranteed to be initialized.

I wonder if there's any problem if I mutate the headers. I'll do more research on that though.

Good call on adding more support on the custom headers that GitHub API requires in go-octokit. Currently it only sets the mandatory headers. For those are optional, e.g. If-Modified-Since, we don't provide options for that. So the outcome of this PR will be able to fix it.

@owenthereal
Copy link
Contributor Author

@technoweenie I just read the source of how RoundTripper is used. I didn't see any side effect of mutating the headers. But I'll ask in the Google group to confirm.

To use RoundTripper as the middleware system, are you looking to use it like this? http://play.golang.org/p/pem5CZKInL

@technoweenie
Copy link
Contributor

I don't think RoundTripper is really intended to modify a header like that. You typically have the actual request object while using the HTTP client, so you can just add your headers there.

RoundTripper might be better if you want a specific header on ALL requests. But go-sawyer should give you this capability already.

client := sawyer.NewFromString("https://api.github.com")

// header set on EVERY request
client.Headers.Set("Accept", "application/vnd.github+json")

apierr := &ApiError{} // decoded from response body on non-20x responses
user := &User{}
req := client.NewRequest("user/21", apierr)

// header set on just this request
req.Headers.Set("X-Foo", "Bar")

@owenthereal
Copy link
Contributor Author

@technoweenie What do you think of 25a2655? I removed the middleware system and prefer Client.Header instead. Unfortunately we don't expose the Request object since go-octokit was designed to be a higher level abstraction. There's one caveat to pay attention to though as we discussed in mislav/hub#535 (comment). Details see https://code.google.com/p/go/issues/detail?id=7682.

* Remove introduced middleware system
* Expose `Client.Header` instead for globally applied headers

A caveat is that Go doesn't apply `Host` on the header but consulting
`Request.Header`. Details see https://code.google.com/p/go/issues/detail?id=7682.
@owenthereal owenthereal changed the title Introduce Middleware system to octokit Apply global headers Apr 5, 2014
@technoweenie
Copy link
Contributor

👍

@owenthereal
Copy link
Contributor Author

Wohoo, merging

@owenthereal owenthereal closed this Apr 6, 2014
@owenthereal owenthereal reopened this Apr 6, 2014
owenthereal added a commit that referenced this pull request Apr 6, 2014
@owenthereal owenthereal merged commit 4fee5e3 into master Apr 6, 2014
@owenthereal owenthereal deleted the middleware branch April 6, 2014 14:52
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.

2 participants