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

Fixed fragment_cached? method to check if caching #1477

Merged
merged 1 commit into from
Feb 10, 2016

Conversation

bdmac
Copy link
Contributor

@bdmac bdmac commented Jan 28, 2016

I noticed that fragment caching does not actually check if caching is
enabled as it seemingly should.

The way CachedSerializer#fragment_cached? worked previously would return
true even in an environment where caching was disabled as defined by
ActiveModelSerializers.config.perform_caching.

Added check for _cache like in the cached? method before checking
whether _cache_only or _cache_except is set.

There were no existing tests for any of these methods but it's a pretty
trivial change.

I noticed that fragment caching does not actually check if caching is
enabled as it seemingly should.

The way CachedSerializer#fragment_cached? worked previously would return
true even in an environment where caching was disabled as defined by
`ActiveModelSerializers.config.perform_caching`.

Added check for `_cache` like in the `cached?` method before checking
whether `_cache_only` or `_cache_except` is set.

There were no existing tests for any of these methods but it's a pretty
trivial change.
@bdmac
Copy link
Contributor Author

bdmac commented Jan 28, 2016

BTW this happens because the implementation of the cache DSL sets the various options (in this case specifically _cache_only and _cache_except regardless of whether or not caching is actually enabled. Relevant code for reference:

        def cache(options = {})
          self._cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching
          self._cache_key = options.delete(:key)
          self._cache_only = options.delete(:only)
          self._cache_except = options.delete(:except)
          self._cache_options = (options.empty?) ? nil : options
        end

@@ -24,7 +24,7 @@ def cached?
end

def fragment_cached?
@klass._cache_only && !@klass._cache_except || !@klass._cache_only && @klass._cache_except
@klass._cache && (@klass._cache_only && !@klass._cache_except || !@klass._cache_only && @klass._cache_except)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the idea was to check if fragment_caching was available independent of whether or not caching is available

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really, the block in cache_check should check for @klass._cache first, I think. return yield unless @klass._cache guard clause or something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right maybe except I don't think you can fragment cache if caching isn't available so that seems moot. I think the answer to that question shouldn't be relevant if caching is disabled.

@bf4
Copy link
Member

bf4 commented Jan 29, 2016

@bdmac Would you mind adding a test for this and a changelog entry at the top of 'fixes'?

@bf4
Copy link
Member

bf4 commented Jan 31, 2016

thanks #1477 (comment)

@bf4 bf4 merged commit 3a092c9 into rails-api:master Feb 10, 2016
@bf4
Copy link
Member

bf4 commented Feb 10, 2016

Merged with changelog and tests added in 2789cc5

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

Successfully merging this pull request may close these issues.

None yet

2 participants