Add caching? helper method

[joelhawksley]

Problem

Caching something that shouldn't be cached is a potential source of bugs and security vulnerabilities. For example, one could write a form helper that outputs a request-specific auth token, only for the helper to be used inside of a cache block.

Proposed solution

In the GitHub application, we implemented a caching? method and used it to raise an error if a specific code path is being cached that we don't want to be cached.

I've credited its original author, @btoews.

Co-authored-by: Ben Toews mastahyeti@gmail.com
Co-authored-by: John Hawthorn jhawthorn@github.com

[eileencodes]

I've been thinking about this PR and I'm not entirely convinced it's necessary or useful to other apps. If the problem is that we don't want to cache sensitive data then we shouldn't add caching blocks for that. Am I missing a piece of how this is used that results in sensitive data getting cached accidentally? The code author would still need to know what data is considered sensitive and need to know to raise if there was sensitive data they didn't want cached.

[rafaelfranca]

I think one of the problem is that something people don't know they are inside a cached template/partial but this would still be true even if we add this method. People would still accidentally add sensitive data inside cached blocks. This helper only makes easy to opt-out of the cache fragment and instead of having two different templates for a cached partial and an uncached partial, you only have one. I'm not sure it is worth a fiber-local.

[joelhawksley]

@eileencodes @rafaelfranca thanks for taking the time to review. I'll provide a more detailed example to explain why I think this feature is generally useful.

We have a custom form helper (let's call it button_form) for building buttons that are forms. Part of this helper includes building CSRF tokens. In order to prevent developers from accidentally using button_form inside a cache block, we raise an error if we're currently caching:

# Build a button that is a form
#
# Returns an HTML String.
def button_form(name, options = {}, html_options = {})
  # Ensure that we aren't caching CSRF tokens.
  raise NoCachingError if caching?

  # build the form, including CSRF tokens
end

At least in our application, it can be difficult to reason about the render stack to be certain that a given line of code won't execute inside a cache block. I think this kind of helper is a good tool to have for building safety mechanisms like the given example.

[rafaelfranca]

We don't use much fragment cache for me to care about this solution, but if others think it is fine it is ok to me. About the implementation, I'd prefer if we add a Runtime Registry in Action View using ActiveSupport::PerThreadRegistry instead of using the fiber local directly.

[kaspth]

Could also consider adding an uncachable! method to auto-raise that NoCachingError so users wouldn't have to define their own exception. Think we could use __callee__ within the exception to extract e.g. button_form for the message.

[joelhawksley]

@rafaelfranca @kaspth thanks for the reviews. I rewrote this PR to use ActiveSupport::PerThreadRegistry.

@kaspth what do you think about having both caching? and uncachable!?

[joelhawksley]

@kaspth friendly bump 😄

[kaspth]

@kaspth what do you think about having both caching? and uncachable!?

Sorry I wasn't clear, I meant that we'd have both yeah.

[kaspth]

Sweet, getting there!

Let's just define the CachingRegistry inline in CacheHelper right below the private marker. It doesn't warrant being a top-level file at the action_view/lib/* root.

[kaspth]

Do we need this comment when the registry is nodoc'ed?

[kaspth]

Looks like the changelog entry just needs to be moved and then we could ship this now — we can also tackle the uncachable! method here if you'd want.

[kaspth]

Rad!

[kaspth]

uhoh indeed, love it 😂

[kaspth]

I know it was marked as a draft PR, but I couldn't see anything else that was missing, so I went ahead. If that was erroneous, I'll be around so we can tackle it in a follow up PR. Thanks for working on this, I'm really happy with how it turned out! ❤️

[joelhawksley]

@kaspth Ha! I was just waiting for CI to pass before moving it out of draft.

Thanks for your help landing this PR, I really appreciate your thoughtful feedback ❤️