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

Never store ETags computed on partial data #28

Merged
merged 1 commit into from Jul 6, 2015

Conversation

@kanongil
Copy link
Member

kanongil commented Jul 5, 2015

This fixes #27 by checking that all bytes have been processed before caching the ETag.

I have included two tests that would fail on the old code.

/cc @hueniverse for review.

Fixes #27.
@kanongil kanongil added the bug label Jul 5, 2015
@kanongil kanongil added this to the 2.1.6 milestone Jul 5, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 5, 2015

Looks good. Is there a way to not even listen to responses when the status code indicates not to produce an etag?

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Jul 6, 2015

I initially moved the actual generation logic to the marshal handler, which is never called on 304s.
However, I could not come up with a case that would not trigger the processed === size conditional. I could had kept this solution by removing this conditional. However, to keep it working, I would have to rely on undocumented internal hapi processing logic to not change, and I believe the coupling is to loose for that. It has already broken test logic in the 8.6.0 to 8.6.1 update.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 6, 2015

Makes sense.

kanongil added a commit that referenced this pull request Jul 6, 2015
Never store ETags computed on partial data
@kanongil kanongil merged commit d86ddc9 into hapijs:master Jul 6, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.