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

Respect the http.RoundTripper contract #61

Merged
merged 1 commit into from Mar 28, 2022

Conversation

CAFxX
Copy link
Contributor

@CAFxX CAFxX commented Feb 2, 2022

Fixes #20

Without this, it's impossible to use httpcache together with this package, as httpcache expects the parent RoundTripper to respect the RoundTripper contract:

	// RoundTrip should not modify the request, except for
	// consuming and closing the Request's Body. RoundTrip may
	// read fields of the request in a separate goroutine. 

ghinstallation currently does not comply with the contract, as it modifies the request headers (in addition to not closing the body in case of errors).

As a result of this, no request will ever result in a hit in the cache because ghinstallation modifies some of the request headers that the Github API includes in the Vary response header (Accept, Authorization).

As a hacky workaround until this bug is fixed in ghinstallation, you can add an intermediate roundtripper between httpcache and ghinstallation to clone the request, so that ghinstallation modifies the clone instead of the request passed by httpcache:

	// itr is the ghinstallation RoundTripper

	dtr := RoundTripperFunc(func(req *http.Request) (*http.Response, error) {
		return itr.RoundTrip(req.Clone(req.Context()))
	})

	// use dtr as the Transport for httpcache

@CAFxX CAFxX marked this pull request as ready for review February 2, 2022 07:26
@CAFxX
Copy link
Contributor Author

CAFxX commented Mar 28, 2022

@bradleyfalzon any chance to get this looked at? 🙏🏻

@bradleyfalzon
Copy link
Owner

Apologise, this looks good to me and thanks for correcting these issues!

@bradleyfalzon bradleyfalzon merged commit 6f00d4f into bradleyfalzon:master Mar 28, 2022
@CAFxX CAFxX deleted the patch-1 branch March 28, 2022 09:08
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.

Transport.RoundTrip does not respect the contract of the RoundTripper interface
2 participants