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

Support etag headers #355

Merged
merged 9 commits into from Feb 14, 2019
Merged

Support etag headers #355

merged 9 commits into from Feb 14, 2019

Conversation

jsnshrmn
Copy link
Contributor

@jsnshrmn jsnshrmn commented Feb 8, 2019

I believe this closes #264. This is my first time dealing with Tornado, and I was quite pleased to see that it can do most of the heavy lifting here.

I wasn't exactly sure of the client behavior we wanted to support, so I implemented it for GET and HEAD requests.

@jsnshrmn jsnshrmn changed the title 264 support etag headers Support etag headers Feb 8, 2019
@jsnshrmn
Copy link
Contributor Author

jsnshrmn commented Feb 9, 2019

Note to self: check etag behavior for error responses.

Copy link
Member

@Mr0grog Mr0grog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not tested yet (on my phone now), but this seems pretty good. Mainly nits about style.

web_monitoring/diffing_server.py Outdated Show resolved Hide resolved
web_monitoring/diffing_server.py Outdated Show resolved Hide resolved
web_monitoring/diffing_server.py Outdated Show resolved Hide resolved
@Mr0grog
Copy link
Member

Mr0grog commented Feb 9, 2019

Oh. This should have a test!

@Mr0grog
Copy link
Member

Mr0grog commented Feb 9, 2019

Also just recalled that we have utils.hash_content(content_bytes).

@jsnshrmn
Copy link
Contributor Author

jsnshrmn commented Feb 14, 2019

I added a super basic http get test against local files that just checks for appropriate response codes based on whether the client sends the If-None-Match header and if the value matches our computed Etag.

Mr0grog and others added 2 commits February 14, 2019 13:42
Co-Authored-By: jsnshrmn <jsnshrmn@users.noreply.github.com>
@jsnshrmn
Copy link
Contributor Author

I did some manual testing of the head method, and it doesn't behave exactly has desired, since it doesn't have any of the validation that happens for get. I should either drop head, or make sure it performs enough checks to throw errors for bad requests. If we're not planning on doing head requests, I'd just drop it, but I thought I'd check the room. @Mr0grog or @danielballan?

from test_diffing_server_exc_handling.py
@Mr0grog
Copy link
Member

Mr0grog commented Feb 14, 2019

[A HEAD request] doesn't have any of the validation that happens for get

We didn’t support HEAD requests before (I had missed the fact that it was new when reviewing originally), so I’m not sure why we would want to add support for them in this PR. If we did (they might be nice to verify that and endpoint URL corresponds to an actual diff, but I’m not sure we have a real use-case for that, since you can also just check /), that should be a separate PR.

That work can be scoped out in its own issue.
@jsnshrmn jsnshrmn merged commit 78dc02d into master Feb 14, 2019
@jsnshrmn jsnshrmn deleted the 264-support-etag-headers branch February 14, 2019 22:49
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.

Support ETag headers
2 participants