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

bugfix: preserve optional interfaces on http.ResponseWriter #91

Merged
merged 3 commits into from
Mar 14, 2019

Conversation

chrisradek
Copy link
Contributor

Issue #, if available:
#80

Description of changes:
This change to preserves optional interfaces on http.ResponseWriter, while also preserving the behavior wherein checking for the presence of an optional interface correctly returns true or false.

Since we currently wrap the http.ResponseWriter to capture the status code and content length, we do not expose the common optional interfaces included in this PR.

Ultimately, I went with a solution based on what httpsnoop did: https://github.com/felixge/httpsnoop/blob/master/wrap_generated_gteq_1.8.go#L66

I looked at a few other options.

  • Reflection to generate the struct at runtime based on available optional interfaces.
    Unfortunately, reflection currently doesn't allow you to add methods to structs for embedded types, so this doesn't work for our situation.
  • Implement methods (e.g. Flush, Hijack) from optional interfaces directly, and forward to the underlying ResponseWriter's implementation if they exist. I saw that a few other libraries went this route. It ends up being less code overall, however it breaks the ability for customer handlers to check for the presence of one of these methods before using it. Instead, this implementation panics or returns an error, which is different than what you'd expect if the ResponseWriter was not being wrapped.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@luluzhao luluzhao merged commit 6818260 into aws:master Mar 14, 2019
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