Fix fragment_cache_key cache invalidation #341

Merged
merged 3 commits into from Jun 14, 2016

Projects

None yet

6 participants

@javan
Member
javan commented Jun 13, 2016 edited

Jbuilder currently does not account for an app's controller-wide fragment_cache_keys when computing its cache keys.

This change adds support, and also aligns Jbuilder's caching setup with ActionView's cache helpers and Abstract Controller's fragment caching and instrumentation.

To test, I made a fresh app with a fragment_cache_key call that should invalidate the template's cache with every request.

# config/environments/development.rb
Rails.application.configure do
  config.x.counter = 0
  config.action_controller.perform_caching = true
  config.cache_store = :mem_cache_store
end

# app/controllers/widgets_controller.rb
class WidgetsController < ApplicationController
  before_action { Rails.application.config.x.counter += 1 }

  fragment_cache_key { Rails.application.config.x.counter }

  def index
  end
end

# app/views/widgets/index.json.jbuilder
json.cache! "unwavering-key" do
  json.config_x_counter Rails.application.config.x.counter
end

Requests using Jbuilder 2.5.0

$ curl http://0.0.0.0:3000/widgets.json
{"config_x_counter":1}

$ curl http://0.0.0.0:3000/widgets.json
{"config_x_counter":1} 
Started GET "/widgets.json" for 127.0.0.1 at 2016-06-13 18:42:18 -0400
Processing by WidgetsController#index as JSON
  Rendering widgets/index.json.jbuilder
  Rendered widgets/index.json.jbuilder (5.7ms)
Completed 200 OK in 18ms (Views: 10.4ms | ActiveRecord: 0.0ms)

Started GET "/widgets.json" for 127.0.0.1 at 2016-06-13 18:42:20 -0400
Processing by WidgetsController#index as JSON
  Rendering widgets/index.json.jbuilder
  Rendered widgets/index.json.jbuilder (5.7ms)
Completed 200 OK in 10ms (Views: 8.7ms | ActiveRecord: 0.0ms)

Requests using this branch

$ curl http://0.0.0.0:3000/widgets.json
{"config_x_counter":1}

$ curl http://0.0.0.0:3000/widgets.json
{"config_x_counter":2}
Processing by WidgetsController#index as JSON
  Rendering widgets/index.json.jbuilder
Read fragment jbuilder/views/1/unwavering-key/c9b8ffbaac0ab06acdab1ba36772a5dc (2.7ms)
Write fragment jbuilder/views/1/unwavering-key/c9b8ffbaac0ab06acdab1ba36772a5dc (0.5ms)
  Rendered widgets/index.json.jbuilder (6.5ms)
Completed 200 OK in 19ms (Views: 11.1ms | ActiveRecord: 0.0ms)

Started GET "/widgets.json" for 127.0.0.1 at 2016-06-13 18:39:53 -0400
Processing by WidgetsController#index as JSON
  Rendering widgets/index.json.jbuilder
Read fragment jbuilder/views/2/unwavering-key/c9b8ffbaac0ab06acdab1ba36772a5dc (0.5ms)
Write fragment jbuilder/views/2/unwavering-key/c9b8ffbaac0ab06acdab1ba36772a5dc (0.7ms)
  Rendered widgets/index.json.jbuilder (6.5ms)
Completed 200 OK in 11ms (Views: 9.5ms | ActiveRecord: 0.0ms)

Note that the cache was correctly invalidated and the presence logging like you'd see when using cache in an .erb template.

/cc @jeremy

@rails-bot

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rwz (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rwz rwz was assigned by rails-bot Jun 13, 2016
@dhh dhh merged commit 65dfb33 into rails:master Jun 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dhh
Member
dhh commented Jun 14, 2016

👍. It's a bit of a smell that each of the template engines have to implement this logic themselves. Wonder what we can do at a higher level to prevent that.

@javan
Member
javan commented Jun 14, 2016

Ideally, Jbuilder and others could use the Rails caching helpers more directly, but they don't work with non-string objects. See action_view/helpers/cache_helper.rb#L227-L238 and abstract_controller/caching/fragments.rb#L77.

@javan
Member
javan commented Jun 14, 2016

I think it's worth considering folding Jbuilder into Rails so it can move forward at the same pace. In its current form, it has to remain backwards compatible with a number of Rails versions, and has accumulated gunk like jbuilder/jbuilder_template.rb#L137-L146.

@dhh
Member
dhh commented Jun 14, 2016

Yeah, possibly. Currently the default serialization format for Rails API is
on hold until 5.1 to see if something palpable pops up from the
serialization attempts. If not, then default jbuilder and folding jbuilder
in could make sense.

On Tue, Jun 14, 2016 at 3:28 PM, Javan Makhmali notifications@github.com
wrote:

I think it's worth considering folding Jbuilder into Rails so it can move
forward at the same pace. In its current form, it has to remain backwards
compatible with a number of Rails versions, and has accumulated gunk like
jbuilder/jbuilder_template.rb#L137-L146
https://github.com/rails/jbuilder/blob/master/lib/jbuilder/jbuilder_template.rb#L137-L146
.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#341 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAAKtXYrV5NqFjV1eUqS-CPhiGhmcEBXks5qLqx7gaJpZM4I00Z-
.

@rafaelfranca
Member

I prefer to create better abstractions in rails han fold all gems to the
same repository. If jbuilder is having problems to evolve in the same pace
be sure that alternative will have more problems to evolve after we fold
jbuilder and stop thinking that alternatives should be possible.
On ter, 14 de jun de 2016 at 09:31 David Heinemeier Hansson <
notifications@github.com> wrote:

Yeah, possibly. Currently the default serialization format for Rails API is
on hold until 5.1 to see if something palpable pops up from the
serialization attempts. If not, then default jbuilder and folding jbuilder
in could make sense.

On Tue, Jun 14, 2016 at 3:28 PM, Javan Makhmali notifications@github.com
wrote:

I think it's worth considering folding Jbuilder into Rails so it can move
forward at the same pace. In its current form, it has to remain backwards
compatible with a number of Rails versions, and has accumulated gunk like
jbuilder/jbuilder_template.rb#L137-L146
<
https://github.com/rails/jbuilder/blob/master/lib/jbuilder/jbuilder_template.rb#L137-L146

.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#341 (comment), or
mute
the thread
<
https://github.com/notifications/unsubscribe/AAAKtXYrV5NqFjV1eUqS-CPhiGhmcEBXks5qLqx7gaJpZM4I00Z-

.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#341 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAC66F24tPUPHHjc5T03WfXuEnk54IxOks5qLq1EgaJpZM4I00Z-
.

@javan javan referenced this pull request in basecamp/bc3-api Jun 23, 2016
Closed

Scheduled Items link to basecampapi.com #25

@kaspth
Member
kaspth commented Jul 14, 2016

I'll second the notion of finding a better public opt-in API for templates. It's clear that what we do in rails/rails#25825 won't automatically arrive in Jbuilder either, and that's a shame.

From the caching side of things templates just need to be able to read and write fragments (or maybe a fetch fragment method could do). That module should then handle: does the controller enable caching, is there a cache store setup, making sure instrumentation is done correctly, etc. That's the high points as I see it at least.

@javan
Member
javan commented Jul 14, 2016

I'll second the notion of finding a better public opt-in API for templates.

For that to happen, the Rails caching helpers will need to work with non-string objects. Jbuilder's "fragments" are objects like Hashes and Arrays. See #341 (comment)

@kaspth
Member
kaspth commented Jul 14, 2016

Ah, right on. Yeah, it's pretty much a non-starter without that support.

Hm, write_fragment_for seems pretty attuned to there being a block that evaluates to a string.

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