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

Fix regression in API controllers with view_cache_dependencies helper #575

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

excid3
Copy link
Contributor

@excid3 excid3 commented Sep 23, 2024

Rails 8's API controllers now includes the Caching module by default (to support rate_limit in API controllers), but does not include the Helpers module.

This causes the jbuilder cache method to raise an exception because the Rails caching module does not add the helper_method :view_cache_dependencies since that's only added if helper_method exists.

helper_method :view_cache_dependencies if respond_to?(:helper_method)

Example error:

ActionView::Template::Error: undefined local variable or method `view_cache_dependencies' for an instance of #<Class:0x000000012a6da468>
    /Users/chris/.local/share/mise/installs/ruby/3.3.5/lib/ruby/gems/3.3.0/bundler/gems/rails-a5d1e10449c6/actionview/lib/action_view/helpers/cache_helper.rb:257:in `digest_path_from_template'
    /Users/chris/.local/share/mise/installs/ruby/3.3.5/lib/ruby/gems/3.3.0/bundler/gems/rails-a5d1e10449c6/actionview/lib/action_view/helpers/cache_helper.rb:271:in `fragment_name_with_digest'
    /Users/chris/.local/share/mise/installs/ruby/3.3.5/lib/ruby/gems/3.3.0/bundler/gems/rails-a5d1e10449c6/actionview/lib/action_view/helpers/cache_helper.rb:252:in `cache_fragment_name'
    /Users/chris/.local/share/mise/installs/ruby/3.3.5/lib/ruby/gems/3.3.0/gems/jbuilder-2.13.0/lib/jbuilder/jbuilder_template.rb:227:in `_fragment_name_with_digest'
    /Users/chris/.local/share/mise/installs/ruby/3.3.5/lib/ruby/gems/3.3.0/gems/jbuilder-2.13.0/lib/jbuilder/jbuilder_template.rb:214:in `_cache_key'
    /Users/chris/.local/share/mise/installs/ruby/3.3.5/lib/ruby/gems/3.3.0/gems/jbuilder-2.13.0/lib/jbuilder/jbuilder_template.rb:194:in `_cache_fragment_for'
    /Users/chris/.local/share/mise/installs/ruby/3.3.5/lib/ruby/gems/3.3.0/gems/jbuilder-2.13.0/lib/jbuilder/jbuilder_template.rb:69:in `cache!'
    app/views/users/_user.json.jbuilder:1:in `_app_views_users__user_json_jbuilder___1927266165303472685_95340'

This adds the helper method to jbuilder to ensure that caching still works without having to include Helpers in API controllers.

Previously, this worked because you could include both the Helpers and Caching modules in API controllers in the correct order.

jbuilder doesn't have a test/dummy app to write tests for this.

@excid3 excid3 force-pushed the fix-caching-with-api-controllers branch from fdf4510 to fa54436 Compare September 23, 2024 18:42
@mguidetti
Copy link

I ran into this issue as well, but on rails 7.2.2. (I believe the API controller gained the Caching module there first?)

I found I needed to add the :combined_fragment_cache_key helper as well to get this to work in my jbuilder views

@dwightwatson
Copy link

Yeah, perhaps this PR should include combined_fragment_cache_key as well, as it's affected by the same issue in AbstractController::Caching::Fragments which is loaded by AbstractController::Caching.

@excid3 excid3 force-pushed the fix-caching-with-api-controllers branch from 66add2d to b928550 Compare November 9, 2024 16:24
@excid3
Copy link
Contributor Author

excid3 commented Nov 9, 2024

Added that helper. 👍

Now that Rails 8 has shipped, I imagine more people will run into this. cc @rafaelfranca

Failing test is because Ruby 3.1 isn't support on Rails head.

@eddygarcas
Copy link

eddygarcas commented Nov 14, 2024

Yes, same issue here running Rails 8.0 too, so far I'm using @excid3 branch and works, waiting for the merge.

@dwightwatson
Copy link

@rafaelfranca apologies to re-ping but any chance this could be reviewed? It's blocking upgrades to ~> 7.2.2/~> 8.0.0 which have both had security patches applied

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 this pull request may close these issues.

4 participants