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: Implemented Conditional GET using etags #1333

Merged
merged 5 commits into from Dec 13, 2018

Conversation

Projects
None yet
4 participants
@CalebKussmaul
Copy link
Contributor

CalebKussmaul commented Sep 11, 2018

On non-range requests, kitura StaticFileServer will never send HTTP 304 NOT MODIFIED response, even if client already has the latest version of a resource. It will send the entire file instead, which is a waste of resources. Additionally, a documentation error regarding the max-age header parameter was fixed.

Description

A check was added to see if the file's Etag matched the request's corresponding "If-None-Match" parameter. If they are equal, the server will respond with HTTP 304 NOT MODIFIED. Otherwise it will still send the file.

Motivation and Context

See issue #1332. In some use cases, a browser may request the same resource on many different pages, or multiple times for a number of other reasons. Using the max-age parameter is inadequate because it does not allow the developer to update a resource at any time without clients continuing to use an outdated version of a resource. I created this because I need my server to be as efficient as possible, and under my use case this change results in a significant resource savings.

How Has This Been Tested?

I have tested the code on a personal project (on a mac using a browser on the same computer). It should not affect other areas of the code.

Checklist:

  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Sep 11, 2018

CLA assistant check
All committers have signed the CLA.

@CalebKussmaul CalebKussmaul changed the title Conditional get Fix: Implemented Conditional GET using etags Sep 11, 2018

@ianpartridge ianpartridge requested a review from djones6 Sep 11, 2018

@djones6
Copy link
Member

djones6 left a comment

Thank you for the PR! I agree this is a worthwhile change. I have a few comments on implementation below.

// Send the entire file
try response.send(fileName: filePath)
response.statusCode = .OK
if let etag = request.serverRequest.headers["If-None-Match"]?.first,

This comment has been minimized.

@djones6

djones6 Sep 13, 2018

Member

This could be simplified as

if let etag = request.headers["If-None-Match"],
response.statusCode = .OK
if let etag = request.serverRequest.headers["If-None-Match"]?.first,
etag == CacheRelatedHeadersSetter.calculateETag(from: fileAttributes) {
_ = response.send(status: .notModified)

This comment has been minimized.

@djones6

djones6 Sep 13, 2018

Member

I don't think we want to send the literal string "not modified" here - just the status code and respond without a body:

  response.statusCode = .notModified
@ianpartridge

This comment has been minimized.

Copy link
Member

ianpartridge commented Nov 9, 2018

Hi @CalebKussmaul! Are you able to update this PR? We'd like to merge it :)

Implemented HTTP Not Modified response for conditional-GET using etag…
…s and fixed documentation issue where max-age is listed as milliseconds instead of seconds

@CalebKussmaul CalebKussmaul force-pushed the CalebKussmaul:Conditional-GET branch from 2368ec1 to e2697e9 Nov 10, 2018

@CalebKussmaul

This comment has been minimized.

Copy link
Contributor

CalebKussmaul commented Nov 10, 2018

@ianpartridge It should be all set now, thank you!

@ianpartridge

This comment has been minimized.

Copy link
Member

ianpartridge commented Nov 12, 2018

@djones6 can you take another look please :)

@djones6
Copy link
Member

djones6 left a comment

Thanks for updating this PR - one comment:

if let etag = request.headers["If-None-Match"],
etag == CacheRelatedHeadersSetter.calculateETag(from: fileAttributes) {
response.statusCode = .notModified
response.send("")

This comment has been minimized.

@djones6

djones6 Nov 21, 2018

Member

I don't think this empty send("") is necessary - it'll do some work under the covers, but result in no data being appended to the response buffer. Setting the statusCode is sufficient.

One of the outcomes of invoking send() is that it sets response.state.invokedSend to indicate that a response has/is being sent. However, we only send a default response if invokedSend is false and statusCode is .unknown.

@ianpartridge

This comment has been minimized.

Copy link
Member

ianpartridge commented Dec 1, 2018

Thanks @CalebKussmaul! @djones6 - please rereview.

@djones6 djones6 merged commit 5bc4fd3 into IBM-Swift:master Dec 13, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment