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

Disable cache when downloading file #3484

Merged
merged 4 commits into from Apr 11, 2018

Conversation

Projects
None yet
3 participants
@ashleytqy
Copy link
Contributor

ashleytqy commented Mar 30, 2018

Fixes #3251

The route handler (NbconvertFileHandler) that processes a download request did not have cache-control header so this adds the header so ensure that the downloaded pdf / markdown etc. file will always be the most recent (not cached).

ashleytqy added some commits Mar 30, 2018

Merge pull request #1 from ashleytqy/ash.etag
Disable cache and etags
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Mar 31, 2018

Was it actually computing the etags wrong, or was it the Cache-Control change that fixed it? Looking through tornado's code, it looks like it computes the etag by hashing the content when we call self.finish(), which I think should work.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Mar 31, 2018

(I'd assumed before that tornado wasn't setting etags by default except for static files, and that we could try setting it from our code)

@ashleytqy

This comment has been minimized.

Copy link
Contributor Author

ashleytqy commented Apr 1, 2018

I think the Cache-Control fixed it – I'll remove the compute_etag method!

@ashleytqy ashleytqy changed the title Disable etags and cache when downloading file Disable cache when downloading file Apr 1, 2018

@ashleytqy

This comment has been minimized.

Copy link
Contributor Author

ashleytqy commented Apr 10, 2018

hi @takluyver ! does this look good to you?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 10, 2018

Oh, sorry, it dropped off my radar. It does look good, thanks. I assume you've tested that it fixes the issue?

I think we might also need the same in the respond_zip() function; that's used when nbconvert produces multiple files, so we assemble them into a zip file to be downloaded.

@ashleytqy

This comment has been minimized.

Copy link
Contributor Author

ashleytqy commented Apr 10, 2018

@takluyver yup! i’ll also add the change to respond_zip() !

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 11, 2018

Thanks :-)

@takluyver takluyver added this to the 5.5 milestone Apr 11, 2018

@takluyver takluyver merged commit f69ce9f into jupyter:master Apr 11, 2018

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project Absolute coverage decreased by -1.06% but relative coverage increased by +23.92% compared to faa0cab
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
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
You can’t perform that action at this time.