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

[Cleanup] Serializer caching is its own concern #1471

Merged
merged 1 commit into from
Mar 14, 2016

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Jan 28, 2016

Update 2016-03-13

Rebased off of master.

Original Description

I was looking at the caching code and realized that although it is called
from the adapter, it is it's own thing. In fact, if it weren't for
fragment caching, which uses the adapter instance, the caching code could
be used entirely by the serializer!

I wrote this PR in two commits so that the moving down of the CachedSerializer
and FragmentCache classes from the Adapter namespace and the moving to the
ActiveModelSerializers namespace can be more easily seen. I intend to squash
them before merge, but would be happy to do it now.

non_cached_hash = non_cached_adapter.serializable_hash

# Merge both results
adapter.fragment_cache(cached_hash, non_cached_hash)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an adapter method since the adapter might want to customize how the two hashes are merged, e.g. see JSON API adapter.

-          non_cached_hash.merge cached_hash
+          root = false if instance_options.include?(:include)
+          core_cached       = cached_hash.first
+          core_non_cached   = non_cached_hash.first
+          no_root_cache     = cached_hash.delete_if { |key, _value| key == core_cached[0] }
+          no_root_non_cache = non_cached_hash.delete_if { |key, _value| key == core_non_cached[0] }
+          cached_resource   = (core_cached[1]) ? core_cached[1].deep_merge(core_non_cached[1]) : core_non_cached[1]
+          hash = (root) ? { root => cached_resource } : cached_resource
+
+          hash.deep_merge no_root_non_cache.deep_merge no_root_cache

class CachedSerializer
def initialize(serializer)
@cached_serializer = serializer
@klass = @cached_serializer.class
Copy link
Member

Choose a reason for hiding this comment

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

I think using serializer.class here is less typo-prone. A typo on serializer would result in NameError whereas a typo in @cached_serializer would result in nil.

@NullVoxPopuli
Copy link
Contributor

needs rebase :-)

@bf4 bf4 force-pushed the move_serializer_caching_from_adapter branch 2 times, most recently from 639a10e to 3a91802 Compare March 14, 2016 00:33
@bf4 bf4 force-pushed the move_serializer_caching_from_adapter branch from 3a91802 to eda8ff1 Compare March 14, 2016 00:58
bf4 added a commit that referenced this pull request Mar 14, 2016
[Cleanup] Serializer caching is its own concern
@bf4 bf4 merged commit 6014b52 into rails-api:master Mar 14, 2016
@bf4 bf4 deleted the move_serializer_caching_from_adapter branch March 14, 2016 01:28
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.

3 participants