Added support for conditional fragment caching using :if and :unless options. #5396

Closed
wants to merge 6 commits into
from

Projects

None yet

10 participants

@sausman
sausman commented Mar 12, 2012

This allows fragment caching to be used conditionally by passing if/unless to the cache helper method. This comes in especially handy when caching forms that may contain changed values/errors.

Usage:

<% cache [ @user, 'form' ], :unless => @user.changed? do %>
  <%= render partial: 'shared/error_messages', locals: {instance: @user} %>
  <%= form_for @user do |f| %>
    <div class="field">
      <div><%= f.label :name %></div>
      <div><%= f.text_field :name %></div>
    </div>
    <div class="actions">
      <%= f.submit %>
    </div>
  <% end %>
<% end %>
@drogus
Member
drogus commented Mar 13, 2012

Thanks for the patch, but is it really needed? You can just use regular ruby if or unless and I'm pretty sure that most people will just use it (as nobody RTFM ;) ).

@sausman
sausman commented Mar 13, 2012

I tried using regular ruby if/unless, but the only way I could get it to work wasn't very DRY:

<% unless @user.changed? %>
  <% cache [ @user, 'form' ] do %>
    ...
  <% end %>
<% else %>
  ...
<% end %>

I also tried:

<% unless @user.changed? %>
  <% cache [ @user, 'form' ] do %>
<% end %>
    ...
<% unless @user.changed? %>
  <% end %>
<% end %>

but it didn't show anything (probably due to the <% end %> right after <% cache ... %>?).

I agree it's not needed from a technical perspective, but it would be very nice to have. What do you think?

@carlosantoniodasilva

I have to agree with @drogus that normal if / unless seem to be enough in this case. And you can always use something like this instead of wrapping in if/unless:

  <% cache [ @user, 'form' ] do %>
    ...
  <% end unless @user.changed? %>

I believe that the :if / :unless options wouldn't help that much in that example of unless/else you've used, so normal ruby should win in this case, right? Thanks :)

@sausman
sausman commented Mar 13, 2012

Aha, that looks a lot better than what I had tried.! Thanks for pointing that out, in that case I would absolutely agree :).

@sausman sausman closed this Mar 13, 2012
@sausman
sausman commented Mar 13, 2012

Damn, I just tried that in an app of mine and it's not yielding the cache block when @user.changed?. Any other ideas for how to do this using normal ruby?

@sausman sausman reopened this Mar 13, 2012
@carlosantoniodasilva

@sausman yeah it won't work, sorry. I think I've read too fast and didn't get the details at first, that will not work for your use case. You have to stick with ruby if/unless methods to render without cache in this scenario. I'm still unsure about the options on the cache method.

At.
Carlos Antonio

On Tuesday, March 13, 2012 at 8:56 PM, Stephen Ausman wrote:

Damn, I just tried that in an app of mine and it's not yielding the cache block when @user.changed?. Any other ideas for how to do this using normal ruby?


Reply to this email directly or view it on GitHub:
#5396 (comment)

@drogus
Member
drogus commented Mar 14, 2012

Ok, I have also not quite understood the real problem with this. In such situation you are right, there is no easy way to do what you want.

@jeremy
Member
jeremy commented Mar 14, 2012
This comes in especially handy when caching forms that may contain changed values/errors

Generally speaking, a form like this has too many variations to cache effectively.

Using a conditional cache is a red flag that either 1. the condition should be in your cache key or 2. that perhaps the content is too dynamic to get a decent cache hit rate anyway.

@swrobel
swrobel commented Apr 3, 2012

+1 for this ... definitely needs to be added for DRY purposes. There are a ton of workarounds to this exact problem out on the internet dating back 5+ years.

@ahawkins

@sausman caching forms is tricky because of authenticity_tokens.

I do generally support this pull request though.

@sausman
sausman commented Apr 17, 2012

@twinturbo Good point, I did run into issues with that. I ended up using:

application_helper.rb

def form_tag(location, options = {}, &proc)
  options[:authenticity_token] = (options.has_key?(:remote) && options[:remote]) ? false : ''
  super(location, options, &proc)
end

foo.coffee.js

@set_authenticity_tokens = ->
  authenticity_token = $('meta[name=csrf-token]').attr 'content'
  $('input[name=authenticity_token]').val authenticity_token

$ ->
  set_authenticity_tokens()
  $(document).bind 'pjax:end', ->
    set_authenticity_tokens()
@drogus
Member
drogus commented Apr 17, 2012

@sausman since 3.2.3 you can just configure rails to not embed auth token in remote forms 128cfbd

@drogus
Member
drogus commented Apr 17, 2012

@sausman I meant this: 805b15f

@sausman
sausman commented Apr 17, 2012

@drogus Nice, thanks.

@swrobel
swrobel commented May 31, 2012

Just ran into another situation where this is problematic:

<% cache @model do %>
  blah blah
<% end %>

If @model.persisted? everything works as expected, and I get Cache read: views/model/91-20120531002327 however I have a preview method that uses the same view with a non-persisted object, and this just does Cache write: views/model/new, which means if I try to preview another non-persisted object, it shows the previously cached version. I really just want to disable the fragment cache for previews ... is there not some way I can do this in the view or the controller?

Update: I realized I can just do expire_fragment(@sale) in my controller but I still think this is a good use case for conditional caching. I'd love to just be able to do

<% cache @model do %>
  blah blah
<% end if @model.persisted? %>
@josevalim
Member

Even though the points raised by @jeremy are true, it seems we have enough use cases of people doing the right thing™. That said, please add tests to the pull request and we will merge it.

@sausman
sausman commented Jun 1, 2012

@josevalim I added a test for options_pass_conditions?. Let me know if there's anything else that needs to be done.

@josevalim
Member

We should not test private methods. There is nothing ensuring cache is actually calling that method, so there is nothing stopping cache from regressing in the future. We need to write a test that asserts the behavior of calling cache with :if and :unless as options.

@sausman
sausman commented Jun 2, 2012

Thanks for the clarification.

The new test passes when conditional fragment caching works, but when it is not I get an error instead of 2 failed assertions:

1) Error:
test_conditional_fragment_caching(FragmentCachingTest):
NoMethodError: undefined method `<<' for nil:NilClass
    /Users/sausman/github/rails/actionpack/lib/action_view/helpers/text_helper.rb:52:in `concat'
    /Users/sausman/github/rails/actionpack/lib/action_view/helpers/text_helper.rb:56:in `safe_concat'
    /Users/sausman/github/rails/actionpack/lib/action_view/helpers/cache_helper.rb:35:in `cache'
    /Users/sausman/github/rails/actionpack/test/controller/caching_test.rb:857:in `test_conditional_fragment_caching'
    /Users/sausman/.rvm/gems/ruby-1.9.3-p0/gems/mocha-0.11.4/lib/mocha/integration/mini_test/version_230_to_262.rb:28:in `run'
    /Users/sausman/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:37:in `block in run'
    /Users/sausman/github/rails/activesupport/lib/active_support/callbacks.rb:377:in `_run_callbacks_70326160161660'
    /Users/sausman/github/rails/activesupport/lib/active_support/callbacks.rb:75:in `run_callbacks'
    /Users/sausman/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:36:in `run'

I think fragment_for is returning nil when called within cache, but I wasn't able to find out why.

@steveklabnik
Member

This will need a rebase if it's ever to get accepted.

@rafaelfranca
Member

@sausman what is the status of this?

@sausman
sausman commented Oct 15, 2012

Just rebased it. The test still does this though.

@freegenie

I took a stab at this here #8371

@rafaelfranca
Member

@sausman feature merged in. @freegenie did a awesome job and fixed the tests.

Thank you guys.

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