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

Prefer object.cache_key when available. #1642

Merged
merged 2 commits into from
Apr 1, 2016
Merged

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Mar 31, 2016

Continues work in #1346

This will prevent objects PORO objects that don't have updated_at defined, from throwing an error.

Not as big a deal now that PORO objects can inherit ActiveModelSerializers::Model, but still necessary if it's not inherited for whatever reason.

Fixes #1344

Cherry-pick of
bf4@040a97b
which was a squash of
https://github.com/rails-api/active_model_serializers/commits/f89ed71058322fe7dd35d5c8b209856f8e03ad14

@bf4
Copy link
Member Author

bf4 commented Mar 31, 2016

I know this is broken, but now it's easier to see what it should be.

@bf4 bf4 changed the title When caching, return the object's cache_key up front if it's defined. Prefer object.cache_key when available. Cache serializers by adapter. Mar 31, 2016
@bf4 bf4 force-pushed the kevintyll-master branch from f12c3fc to f497feb Compare March 31, 2016 18:57
@bf4 bf4 added C: Feature and removed S: Bug labels Mar 31, 2016
@bf4 bf4 force-pushed the kevintyll-master branch from f497feb to 0f3a729 Compare March 31, 2016 19:49
@bf4
Copy link
Member Author

bf4 commented Mar 31, 2016

This code was hard to extract and could use a good review.

@kevintyll Does this work for you? Also see #1644

@bf4 bf4 force-pushed the kevintyll-master branch from 0f3a729 to 6775938 Compare March 31, 2016 19:54
@@ -161,13 +193,20 @@ def test_fragment_cache_with_inheritance
refute_includes(base.keys, :special_attribute)
end

def test_uses_adapter_in_cache_key
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is not proving that the adapter is in the cache key.

Copy link
Member Author

Choose a reason for hiding this comment

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

lulz. Thanks! 👍

@kevintyll
Copy link
Contributor

👍

@bf4 bf4 changed the title Prefer object.cache_key when available. Cache serializers by adapter. Prefer object.cache_key when available. Apr 1, 2016
@bf4 bf4 force-pushed the kevintyll-master branch 3 times, most recently from 9cb0841 to e7c3ebc Compare April 1, 2016 03:26
kevintyll and others added 2 commits March 31, 2016 22:29
This will prevent objects PORO objects that don't have updated_at defined, from throwing an error.

Not as big a deal now that PORO objects can inherit ActiveModelSerializers::Model, but still necessary if it's not inherited for whatever reason.

Add the Adapter type to the cache key.

This prevents incorrect results when the same object is serialized with different adapters.

BF:

Cherry-pick of
040a97b
which was a squash of
https://github.com/rails-api/active_model_serializers/commits/f89ed71058322fe7dd35d5c8b209856f8e03ad14

from pr 1346
@bf4 bf4 force-pushed the kevintyll-master branch from e7c3ebc to 4ba4c29 Compare April 1, 2016 03:31
@bf4 bf4 merged commit e118599 into rails-api:master Apr 1, 2016
@bf4 bf4 deleted the kevintyll-master branch April 1, 2016 04:39
@lambda2
Copy link
Contributor

lambda2 commented Jun 13, 2016

Does it generates the same cache key for different serializers on the same model ?

@bf4
Copy link
Member Author

bf4 commented Jun 13, 2016

@lambda2 if using object.cache_key, then yes, it will. Please open a new issue with some examples of this

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