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

Cache_store not set when using ActionCtrl::API and manually including Caching #35602

Closed
dennym opened this issue Mar 13, 2019 · 3 comments · Fixed by #36038
Closed

Cache_store not set when using ActionCtrl::API and manually including Caching #35602

dennym opened this issue Mar 13, 2019 · 3 comments · Fixed by #36038

Comments

@dennym
Copy link

dennym commented Mar 13, 2019

Steps to reproduce

Simple setup some cache_store for the Application

# application.rb
config.api_only = true
config.cache_store = :mem_cached_store`
# application_controller.rb
class ApplicationController < ActionController::API
  include ::ActionController::Caching
end

Actual behavior

Since I have config.action_controller.perform_caching = true I was checking if my dalli store was available since I use ActionController::API. After digging through the repository I found out I had to include manually ActionController::Caching. Ezpz but after checking for cache_store it turns out that the cache store is nil and therefor not set.

# application_controller.rb
class ApplicationController < ActionController::API
  include ::ActionController::Caching
end

ApplicationController.cache_store #=> nil

To eliminate any other configuration errors I just checked against having the ApplicationController inherit from ActionController::Base and there it returns the expected cache store.

# application_controller.rb
class ApplicationController < ActionController::Base
end

ApplicationController.cache_store #=> :mem_cached_store ...

Expected behavior

Well both setups should give the same output. Its also mentioned here that the modules can be included.

As a side thought:
When you rails new foo --api then it sets ApplicationController to ActionController::API instead of Base and also sets production to config.action_controller.perform_caching = true Which wouldn't work then at all.

System configuration

Rails version: 5.2.2

Ruby version: 2.6.1

st0012 added a commit to st0012/rails that referenced this issue Apr 19, 2019
Currently ActionController::API doesn't include Caching module, so it
can't perform caching. And even if users include it later manually, it
won't inherit application's default cache store for action_controllers.
So the only way to solve this issue is to include Caching module in
ActionController::API, too.

This closes rails#35602
@st0012
Copy link
Contributor

st0012 commented Apr 19, 2019

@dennym FYI, I already opened a PR for that. I hope them would accept it and backport it to 5.2 😄

@rafaelfranca rafaelfranca reopened this Apr 22, 2019
st0012 added a commit to st0012/rails that referenced this issue Apr 23, 2019
Rails doesn't support view caching in api controllers by default but the
document didn't clearerly declare this nor the manual config needed
after including the module manually. So we'll see people get confused
like rails#35602.
@dennym
Copy link
Author

dennym commented Apr 23, 2019

Mhm interesting... thanks for the input and work :) So the cache_store in the controller is just responsible for view caching? I was under the impression that it does couple of magical caching in the background.

If its its just for view caching I personally actually don't need it. But when setting a new rails project up with rails new foo --api it shouldn't set config.action_controller.perform_caching = true since this is probably won't have any use, right?

@rafaelfranca
Copy link
Member

But when setting a new rails project up with rails new foo --api it shouldn't set config.action_controller.perform_caching = true since this is probably won't have any use, right?

That is a good point. We need to double check if that config affects other things but I believe we can remove from the api apps.

st0012 added a commit to st0012/rails that referenced this issue Apr 23, 2019
As suggested in rails#35602 (comment), because we don't provide view caching and doesn't include `ActionController::Caching` for api apps, we should also avoid generating

```ruby
config.action_controller.perform_caching = true
```

for those api apps. So it won't confuse people.

**But because `perform_caching` will be `true` if not set, the behavior of the app would still be the same without these configs.**
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 a pull request may close this issue.

3 participants