Fix fragment_cache_key cache invalidation

[javan]

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.

[dhh]

👍. 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]

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]

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]

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 [email protected]
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]

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 <
[email protected]> 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 [email protected]
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-
.

[kaspth]

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]

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]

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.

[bobdutch]

This change removed the ability to use the :force option with cache! now that it uses Rails.cache.read and Rails.cache.write instead of Rails.cache.fetch.