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

Fix if more than one library replaces request.Body #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mackross
Copy link

We replace the ReadCloser body with a special one that dumps the request into a buffer for streaming to S3 logging. This library is then unable to find the original struct and assert the interface. With this commit context will try find the original replaced crc in nested ReadClosers.

Use reflect to try and find nested ReaderClosers.
@ydnar
Copy link
Contributor

ydnar commented Jul 24, 2015

I like what you're trying to do. We've seen similar issues with middleware replacing / wrapping the ResponseWriter.

My concern is the use of Reflect, and the need for the ReadCloser implementations to export their fields.

Honestly wish Go provided a method or at least a pattern to recover a wrapped interface.

Is it not possible to re-order your middleware to install the httpcontext after the buffering middleware?

@mackross
Copy link
Author

Thanks -- unfortunately I can't reorder easily. Context is used immediately to store the variable path params to the request by the http middleware/router library I'm using. However, the buffer middleware is added to the middleware/router library and thus after.

I'm not super happy with it either. Needing reflect is a bit sucky. On the up side if you're only using this library it'll never fall down into the reflect code paths -- its more like a safety net. If you don't want to merge these changes in I'm not at all offended. It is kind of dirty and I've vendored my copy for my use case 👍

Best, A

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