From c25f2f3863fe904898c2e734a5245b15fbc1f42a Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 8 Jan 2017 21:33:30 -0600 Subject: [PATCH] Fix model attributes accessors --- lib/active_model_serializers/model.rb | 45 +++++++++ .../adapter_selector_test.rb | 9 ++ .../namespace_lookup_test.rb | 2 +- test/adapter/json/has_many_test.rb | 12 ++- test/adapter/json_api/has_many_test.rb | 12 ++- .../include_data_if_sideloaded_test.rb | 10 ++ test/cache_test.rb | 92 +++++++++++++++---- test/fixtures/poro.rb | 10 -- test/serializers/associations_test.rb | 12 ++- 9 files changed, 169 insertions(+), 35 deletions(-) diff --git a/lib/active_model_serializers/model.rb b/lib/active_model_serializers/model.rb index e49bea33..b61661bc 100644 --- a/lib/active_model_serializers/model.rb +++ b/lib/active_model_serializers/model.rb @@ -6,6 +6,17 @@ module ActiveModelSerializers include ActiveModel::Serializers::JSON include ActiveModel::Model + # Declare names of attributes to be included in +sttributes+ hash. + # Is only available as a class-method since the ActiveModel::Serialization mixin in Rails + # uses an +attribute_names+ local variable, which may conflict if we were to add instance methods here. + # + # @overload attribute_names + # @return [Array] + class_attribute :attribute_names, instance_writer: false, instance_reader: false + # Initialize +attribute_names+ for all subclasses. The array is usually + # mutated in the +attributes+ method, but can be set directly, as well. + self.attribute_names = [] + # Easily declare instance attributes with setters and getters for each. # # All attributes to initialize an instance must have setters. @@ -25,12 +36,46 @@ module ActiveModelSerializers # @param names [Array] # @param name [String, Symbol] def self.attributes(*names) + self.attribute_names |= names.map(&:to_sym) # Silence redefinition of methods warnings ActiveModelSerializers.silence_warnings do attr_accessor(*names) end end + # Opt-in to breaking change + def self.derive_attributes_from_names_and_fix_accessors + unless included_modules.include?(DeriveAttributesFromNamesAndFixAccessors) + prepend(DeriveAttributesFromNamesAndFixAccessors) + end + end + + module DeriveAttributesFromNamesAndFixAccessors + def self.included(base) + # NOTE that +id+ will always be in +attributes+. + base.attributes :id + end + + # Override the initialize method so that attributes aren't processed. + # + # @param attributes [Hash] + def initialize(attributes = {}) + @errors = ActiveModel::Errors.new(self) + super + end + + # Override the +attributes+ method so that the hash is derived from +attribute_names+. + # + # The the fields in +attribute_names+ determines the returned hash. + # +attributes+ are returned frozen to prevent any expectations that mutation affects + # the actual values in the model. + def attributes + self.class.attribute_names.each_with_object({}) do |attribute_name, result| + result[attribute_name] = public_send(attribute_name).freeze + end.with_indifferent_access.freeze + end + end + # Support for validation and other ActiveModel::Errors # @return [ActiveModel::Errors] attr_reader :errors diff --git a/test/action_controller/adapter_selector_test.rb b/test/action_controller/adapter_selector_test.rb index 6f22aae2..db93573b 100644 --- a/test/action_controller/adapter_selector_test.rb +++ b/test/action_controller/adapter_selector_test.rb @@ -3,6 +3,15 @@ require 'test_helper' module ActionController module Serialization class AdapterSelectorTest < ActionController::TestCase + class Profile < Model + attributes :id, :name, :description + associations :comments + end + class ProfileSerializer < ActiveModel::Serializer + type 'profiles' + attributes :name, :description + end + class AdapterSelectorTestController < ActionController::Base def render_using_default_adapter @profile = Profile.new(name: 'Name 1', description: 'Description 1', comments: 'Comments 1') diff --git a/test/action_controller/namespace_lookup_test.rb b/test/action_controller/namespace_lookup_test.rb index f40cca11..7b6c20fb 100644 --- a/test/action_controller/namespace_lookup_test.rb +++ b/test/action_controller/namespace_lookup_test.rb @@ -4,7 +4,7 @@ module ActionController module Serialization class NamespaceLookupTest < ActionController::TestCase class Book < ::Model - attributes :title, :body + attributes :id, :title, :body associations :writer, :chapters end class Chapter < ::Model diff --git a/test/adapter/json/has_many_test.rb b/test/adapter/json/has_many_test.rb index 3f6fa546..feeec93c 100644 --- a/test/adapter/json/has_many_test.rb +++ b/test/adapter/json/has_many_test.rb @@ -4,6 +4,10 @@ module ActiveModelSerializers module Adapter class Json class HasManyTestTest < ActiveSupport::TestCase + class ModelWithoutSerializer < ::Model + attributes :id, :name + end + def setup ActionController::Base.cache_store.clear @author = Author.new(id: 1, name: 'Steve K.') @@ -16,7 +20,7 @@ module ActiveModelSerializers @second_comment.post = @post @blog = Blog.new(id: 1, name: 'My Blog!!') @post.blog = @blog - @tag = Tag.new(id: 1, name: '#hash_tag') + @tag = ModelWithoutSerializer.new(id: 1, name: '#hash_tag') @post.tags = [@tag] end @@ -30,7 +34,11 @@ module ActiveModelSerializers end def test_has_many_with_no_serializer - serializer = PostWithTagsSerializer.new(@post) + post_serializer_class = Class.new(ActiveModel::Serializer) do + attributes :id + has_many :tags + end + serializer = post_serializer_class.new(@post) adapter = ActiveModelSerializers::Adapter::Json.new(serializer) assert_equal({ id: 42, diff --git a/test/adapter/json_api/has_many_test.rb b/test/adapter/json_api/has_many_test.rb index 9da73af9..a9fa9ac9 100644 --- a/test/adapter/json_api/has_many_test.rb +++ b/test/adapter/json_api/has_many_test.rb @@ -4,6 +4,10 @@ module ActiveModelSerializers module Adapter class JsonApi class HasManyTest < ActiveSupport::TestCase + class ModelWithoutSerializer < ::Model + attributes :id, :name + end + def setup ActionController::Base.cache_store.clear @author = Author.new(id: 1, name: 'Steve K.') @@ -26,7 +30,7 @@ module ActiveModelSerializers @blog.articles = [@post] @post.blog = @blog @post_without_comments.blog = nil - @tag = Tag.new(id: 1, name: '#hash_tag') + @tag = ModelWithoutSerializer.new(id: 1, name: '#hash_tag') @post.tags = [@tag] @serializer = PostSerializer.new(@post) @adapter = ActiveModelSerializers::Adapter::JsonApi.new(@serializer) @@ -129,7 +133,11 @@ module ActiveModelSerializers end def test_has_many_with_no_serializer - serializer = PostWithTagsSerializer.new(@post) + post_serializer_class = Class.new(ActiveModel::Serializer) do + attributes :id + has_many :tags + end + serializer = post_serializer_class.new(@post) adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) assert_equal({ diff --git a/test/adapter/json_api/include_data_if_sideloaded_test.rb b/test/adapter/json_api/include_data_if_sideloaded_test.rb index b5c81869..c0da9488 100644 --- a/test/adapter/json_api/include_data_if_sideloaded_test.rb +++ b/test/adapter/json_api/include_data_if_sideloaded_test.rb @@ -14,11 +14,21 @@ module ActiveModel [{ foo: 'bar' }] end end + class Tag < ::Model + attributes :id, :name + end class TagSerializer < ActiveModel::Serializer + type 'tags' attributes :id, :name end + class PostWithTagsSerializer < ActiveModel::Serializer + type 'posts' + attributes :id + has_many :tags + end + class IncludeParamAuthorSerializer < ActiveModel::Serializer class_attribute :comment_loader diff --git a/test/cache_test.rb b/test/cache_test.rb index f9a10464..f0958931 100644 --- a/test/cache_test.rb +++ b/test/cache_test.rb @@ -4,6 +4,20 @@ require 'tempfile' module ActiveModelSerializers class CacheTest < ActiveSupport::TestCase + class Article < ::Model + attributes :title + # To confirm error is raised when cache_key is not set and cache_key option not passed to cache + undef_method :cache_key + end + class ArticleSerializer < ActiveModel::Serializer + cache only: [:place], skip_digest: true + attributes :title + end + + class Author < ::Model + attributes :id, :name + associations :posts, :bio, :roles + end # Instead of a primitive cache key (i.e. a string), this class # returns a list of objects that require to be expanded themselves. class AuthorWithExpandableCacheElements < Author @@ -27,30 +41,32 @@ module ActiveModelSerializers ] end end - class UncachedAuthor < Author # To confirm cache_key is set using updated_at and cache_key option passed to cache undef_method :cache_key end + class AuthorSerializer < ActiveModel::Serializer + cache key: 'writer', skip_digest: true + attributes :id, :name - class Article < ::Model - attributes :title - # To confirm error is raised when cache_key is not set and cache_key option not passed to cache - undef_method :cache_key + has_many :posts + has_many :roles + has_one :bio end - class ArticleSerializer < ActiveModel::Serializer - cache only: [:place], skip_digest: true - attributes :title + class Blog < ::Model + attributes :name + associations :writer end + class BlogSerializer < ActiveModel::Serializer + cache key: 'blog' + attributes :id, :name - class InheritedRoleSerializer < RoleSerializer - cache key: 'inherited_role', only: [:name, :special_attribute] - attribute :special_attribute + belongs_to :writer end class Comment < ::Model - attributes :body + attributes :id, :body associations :post, :author # Uses a custom non-time-based cache key @@ -58,14 +74,52 @@ module ActiveModelSerializers "comment/#{id}" end end + class CommentSerializer < ActiveModel::Serializer + cache expires_in: 1.day, skip_digest: true + attributes :id, :body + belongs_to :post + belongs_to :author + end + + class Post < ::Model + attributes :id, :title, :body + associations :author, :comments, :blog + end + class PostSerializer < ActiveModel::Serializer + cache key: 'post', expires_in: 0.1, skip_digest: true + attributes :id, :title, :body + + has_many :comments + belongs_to :blog + belongs_to :author + end + + class Role < ::Model + attributes :name, :description, :special_attribute + associations :author + end + class RoleSerializer < ActiveModel::Serializer + cache only: [:name, :slug], skip_digest: true + attributes :id, :name, :description + attribute :friendly_id, key: :slug + belongs_to :author + + def friendly_id + "#{object.name}-#{object.id}" + end + end + class InheritedRoleSerializer < RoleSerializer + cache key: 'inherited_role', only: [:name, :special_attribute] + attribute :special_attribute + end setup do cache_store.clear @comment = Comment.new(id: 1, body: 'ZOMG A COMMENT') - @post = Post.new(title: 'New Post', body: 'Body') + @post = Post.new(id: 'post', title: 'New Post', body: 'Body') @bio = Bio.new(id: 1, content: 'AMS Contributor') - @author = Author.new(name: 'Joao M. D. Moura') - @blog = Blog.new(id: 999, name: 'Custom blog', writer: @author, articles: []) + @author = Author.new(id: 'author', name: 'Joao M. D. Moura') + @blog = Blog.new(id: 999, name: 'Custom blog', writer: @author) @role = Role.new(name: 'Great Author') @location = Location.new(lat: '-23.550520', lng: '-46.633309') @place = Place.new(name: 'Amazing Place') @@ -325,12 +379,14 @@ module ActiveModelSerializers def test_uses_file_digest_in_cache_key render_object_with_cache(@blog) - key = "#{@blog.cache_key}/#{adapter.cache_key}/#{::Model::FILE_DIGEST}" + file_digest = Digest::MD5.hexdigest(File.open(__FILE__).read) + key = "#{@blog.cache_key}/#{adapter.cache_key}/#{file_digest}" assert_equal(@blog_serializer.attributes, cache_store.fetch(key)) end def test_cache_digest_definition - assert_equal(::Model::FILE_DIGEST, @post_serializer.class._cache_digest) + file_digest = Digest::MD5.hexdigest(File.open(__FILE__).read) + assert_equal(file_digest, @post_serializer.class._cache_digest) end def test_object_cache_keys @@ -532,7 +588,7 @@ module ActiveModelSerializers role_hash = role_serializer.fetch_attributes_fragment(adapter_instance) assert_equal(role_hash, expected_result) - role.attributes[:id] = 'this has been updated' + role.id = 'this has been updated' role.name = 'this was cached' role_hash = role_serializer.fetch_attributes_fragment(adapter_instance) diff --git a/test/fixtures/poro.rb b/test/fixtures/poro.rb index 384610cc..a362fb5a 100644 --- a/test/fixtures/poro.rb +++ b/test/fixtures/poro.rb @@ -1,6 +1,4 @@ class Model < ActiveModelSerializers::Model - FILE_DIGEST = Digest::MD5.hexdigest(File.open(__FILE__).read) - attr_writer :id # At this time, just for organization of intent @@ -108,10 +106,6 @@ class PostPreviewSerializer < ActiveModel::Serializer has_many :comments, serializer: ::CommentPreviewSerializer belongs_to :author, serializer: ::AuthorPreviewSerializer end -class PostWithTagsSerializer < ActiveModel::Serializer - attributes :id - has_many :tags -end class PostWithCustomKeysSerializer < ActiveModel::Serializer attributes :id has_many :comments, key: :reviews @@ -208,10 +202,6 @@ module Spam end end -class Tag < Model - attributes :name -end - class VirtualValue < Model; end class VirtualValueSerializer < ActiveModel::Serializer attributes :id diff --git a/test/serializers/associations_test.rb b/test/serializers/associations_test.rb index 6d6447c3..90d213dc 100644 --- a/test/serializers/associations_test.rb +++ b/test/serializers/associations_test.rb @@ -2,13 +2,17 @@ require 'test_helper' module ActiveModel class Serializer class AssociationsTest < ActiveSupport::TestCase + class ModelWithoutSerializer < ::Model + attributes :id, :name + end + def setup @author = Author.new(name: 'Steve K.') @author.bio = nil @author.roles = [] @blog = Blog.new(name: 'AMS Blog') @post = Post.new(title: 'New Post', body: 'Body') - @tag = Tag.new(id: 'tagid', name: '#hashtagged') + @tag = ModelWithoutSerializer.new(id: 'tagid', name: '#hashtagged') @comment = Comment.new(id: 1, body: 'ZOMG A COMMENT') @post.comments = [@comment] @post.tags = [@tag] @@ -46,7 +50,11 @@ module ActiveModel end def test_has_many_with_no_serializer - PostWithTagsSerializer.new(@post).associations.each do |association| + post_serializer_class = Class.new(ActiveModel::Serializer) do + attributes :id + has_many :tags + end + post_serializer_class.new(@post).associations.each do |association| key = association.key serializer = association.serializer options = association.options