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

add ETag to header output #52

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

dohomi
Copy link

@dohomi dohomi commented Mar 5, 2018

Add ETag to header output based on s3 ETag
Attempt to close #13
@schickling can you have a look? Thanks!

Copy link
Contributor

@kbrandwijk kbrandwijk left a comment

Choose a reason for hiding this comment

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

If using ETag, the hard-coded Cache-Control value might be confusing.

@dohomi
Copy link
Author

dohomi commented Mar 6, 2018

@kbrandwijk what do you suggest? I think because the images basically will never change - because if you change the image on graphcool it will be a new file anyways - the hardcoded cache value basically let the file stay in cache more-less forever? I think thats alright in this context.

@kbrandwijk
Copy link
Contributor

You are actually right! But if resources never change, what's the use for using ETag? Also, when using ETag, if you would query the server with If-None-Match, it should return 304 without a body. Right now, it doesn't. So combined with the fact that files never change, and the hard cache-control value, I don't think this is the right way to deal with ETag.

@dohomi
Copy link
Author

dohomi commented Mar 7, 2018

@kbrandwijk I want to get rid of downgrade in PageSpeed and YSlow due to missing Expired or ETag in the imageproxy. So what do you think is the right approach? Only show Last-Modified?

I was reading through several posts and it seems that IF-None-Match would still work if the date is set correctly?

As far as I understand if 304 is the response and the ETag hasnt changed the browser will fetch the correct image and extends the lifetime of the cached image based on the max-age.
http://www.benhallbenhall.com/2012/03/http-codes-200-from-cache-304/

If the client has performed a conditional GET request and access is allowed, but the document has not been modified, the server SHOULD respond with this status code. The 304 response MUST NOT contain a message-body, and thus is always terminated by the first empty line after the header fields.

For me this is all new topic so I was reading through many blog posts to make sure doing the right thing.

https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/http-caching
https://stackoverflow.com/questions/6350384/http-combining-expiration-and-validation-caching

@kbrandwijk
Copy link
Contributor

@dohomi You are mostly right. I'm just saying that in order to provide a meaningful implementation of the ETag header, the part that says: 'return a 304' should also be implemented, which isn't part of the implementation right now.

Also, returning the ETag from S3 might not be sufficient, because all the processing (cropping, resizing etc.) happens after the image is retrieved from S3. This would mean that different representations (based on the cropping/resizing arguments) would return the same ETag, which is also not right.

I like the idea of supporting ETag, but I think it should be supported properly, instead of just passing along a header from S3. In my opinion, these things are missing right now:

  • Generate a different ETag for different representations
  • Respond correctly to If-None-Match and other ETag-related requests (304 and more)

@dohomi
Copy link
Author

dohomi commented Mar 7, 2018

@kbrandwijk you are right, I added the npm etag module to generate an accurate etag for the body.

To return a proper 304 we would need to extract the right header from the event. Then doing basically the same modification to the original image and check if the HTTP request ETag equals the generated etag(body). Do you know how to read out the headers correctly?

@dohomi
Copy link
Author

dohomi commented Mar 8, 2018

I think I found a way. Could you have a look if that looks alright to you?

@dohomi
Copy link
Author

dohomi commented Mar 9, 2018

@kbrandwijk @schickling I cloned this project and tested the behaviour on my aws account and it worked as expected. Anything I can do to get this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

add ETag and Last-Modified to the handler.ts
2 participants