Preserve the serializer type when fragment caching #1458
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were not previously cloning the type setting into the dynamically generated cached/non-cached serializers for a given fragment-cached serializer. This led to the type generated for JsonApi having the wrong value when fragment caching is enabled by adding either :except or :only options to cache. This pulls the type setting from the fragment-cached serializer forward onto the dynamic caching classes so it is preserved in the output.
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.
| @@ -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) | |||
There was a problem hiding this comment.
this can be it's own PR. would you mind? TYMessaging@e9e2b57
|
Awesome, thanks! 👍 some comments above |
| def test_fragment_fetch_with_type_override | ||
| type_serializer = TypedRoleSerializer.new(Role.new(name: 'Another Author')) | ||
| type_hash = ActiveModel::Serializer::Adapter::JsonApi.new(type_serializer).serializable_hash | ||
| assert_equal(type_hash[:data][:type], TypedRoleSerializer._type) |
There was a problem hiding this comment.
better to use the serializable test helper (below may contain typos)
serialization = serializable(Role.new(name: 'Another Author'), serializer: TypedRoleSerializer, adapter: :json_api).serializable_hash
assert_equal(TypedRoleSerializer._type, serialization.fetch(:data).fetch(:type))
|
@bf4 comments addressed. Will open a secondary PR with the fix for |
bf4
added a commit
that referenced
this pull request
Feb 10, 2016
|
Closed by 527c2ae |
|
Merged in a409982 |
|
This was merged, but GitHub didn't pick it up. I'm not sure why |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were not previously cloning the type setting into the dynamically
generated cached/non-cached serializers for a given fragment-cached
serializer. This led to the type generated for JsonApi having the wrong
value when fragment caching is enabled by adding either :except or :only
options to cache.
This pulls the type setting from the fragment-cached serializer forward
onto the dynamic caching classes so it is preserved in the output.