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.

Sign up for GitHub

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

Jump to bottom

Grape v1.4.0 regression: Cache-Control is always set to no-cache #2087

Closed
stanhu opened this issue Jul 10, 2020 · 2 comments · Fixed by #2088
Closed

Grape v1.4.0 regression: Cache-Control is always set to no-cache #2087

stanhu opened this issue Jul 10, 2020 · 2 comments · Fixed by #2088

Comments

@stanhu
Copy link
Contributor

stanhu commented Jul 10, 2020 •
edited
Loading

In Grape v1.4.0 https://github.com/ruby-grape/grape/pull/1520/files#diff-8e15cddb80f42ca7d43872846d80ecbeR275 broke our test suite because Cache-Control is always set to no-cache in

grape/lib/grape/dsl/inside_route.rb

Line 333 in 84d68bd

header 'Cache-Control', 'no-cache' # Skips ETag generation (reading the response up front)
.

We set Cache-Control to max-age=0, private, must-revalidate, no-store, no-cache.

Maybe we should appendno-cache only if isn't present? Or maybe the headers shouldn't be touched if nil is returned?
@stanhu
Copy link
Contributor Author

stanhu commented Jul 10, 2020

I'm also not sure if overriding the Content-Length and Transfer-Encoding by default creates other issues.

stanhu added a commit to stanhu/grape that referenced this issue Jul 10, 2020
@stanhu
Don't set Cache-Control for responses that don't use streams
7253a7b 
Cache-Control HTTP header with `no-cache`, even if the response wasn't a
stream. To fix this, we only set HTTP headers if there is an actual
stream.

Closes ruby-grape#2087
stanhu added a commit to stanhu/grape that referenced this issue Jul 10, 2020
@stanhu
Set Cache-Control only for streamed responses
b56f707 
The pull request ruby-grape#1520 introduced a regression that always caused the
`Cache-Control` HTTP header to be set to `no-cache`, even if the
response wasn't a stream. To fix this, we only set HTTP headers if there
is an actual stream.

Closes ruby-grape#2087
@stanhu stanhu mentioned this issue Jul 10, 2020
Merged
stanhu added a commit to stanhu/grape that referenced this issue Jul 11, 2020
@stanhu
Set Cache-Control only for streamed responses
019595e 
The pull request ruby-grape#1520 introduced a regression that always caused the
`Cache-Control` HTTP header to be set to `no-cache`, even if the
response wasn't a stream. To fix this, we only set HTTP headers if there
is an actual stream.

Closes ruby-grape#2087
stanhu added a commit to stanhu/grape that referenced this issue Jul 13, 2020
@stanhu
Set Cache-Control only for streamed responses
907fbee 
The pull request ruby-grape#1520 introduced a regression that always caused the
`Cache-Control` HTTP header to be set to `no-cache`, even if the
response wasn't a stream. To fix this, we only set HTTP headers if there
is an actual stream.

Closes ruby-grape#2087
stanhu added a commit to stanhu/grape that referenced this issue Jul 13, 2020
@stanhu
Set Cache-Control only for streamed responses
946e641 
The pull request ruby-grape#1520 introduced a regression that always caused the
`Cache-Control` HTTP header to be set to `no-cache`, even if the
response wasn't a stream. To fix this, we only set HTTP headers if there
is an actual stream.

Closes ruby-grape#2087
stanhu added a commit to stanhu/grape that referenced this issue Jul 13, 2020
@stanhu
Set Cache-Control only for streamed responses
28dde32 
The pull request ruby-grape#1520 introduced a regression that always caused the
`Cache-Control` HTTP header to be set to `no-cache`, even if the
response wasn't a stream. To fix this, we only set HTTP headers if there
is an actual stream.

Closes ruby-grape#2087
@jarthod
Copy link

jarthod commented Jul 17, 2020

Got the exact same problem. Good to see it's already fixed 👏 waiting for the next release then ;)

@magni- magni- mentioned this issue Aug 28, 2020
Closed
@bobbytables bobbytables mentioned this issue Oct 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Set Cache-Control only for streamed responses stanhu/grape
2 participants
@jarthod @stanhu