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.

Sign up for GitHub

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

Simplify attributes handling. #1370

Merged
merged 6 commits into from
Jan 4, 2016
Merged

Simplify attributes handling. #1370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee0283c
Simplify attributes handling.
beauby Dec 10, 2015
1d4b27f
Improve attribute value computation.
beauby Dec 20, 2015
7d24cbf
Extract latent Attribute object.
beauby Dec 27, 2015
a586a45
Remove `key` from `Attribute` class.
beauby Dec 29, 2015
77095f2
Add ActiveSupport::Autoload extension to Attribute.
beauby Dec 30, 2015
ccb05f1
Add changelog.
beauby Dec 30, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ Fixes:
- [#1358](https://github.com/rails-api/active_model_serializers/pull/1358) Handle serializer file paths with spaces (@rwstauner, @bf4)

Misc:
- [#1370](https://github.com/rails-api/active_model_serializers/pull/1370) Simplify attributes handling via a mixin (@beauby)
- [#1233](https://github.com/rails-api/active_model_serializers/pull/1233) Top-level meta and meta_key options no longer handled at serializer level (@beauby)
- [#1232](https://github.com/rails-api/active_model_serializers/pull/1232) fields option no longer handled at serializer level (@beauby)
- [#1178](https://github.com/rails-api/active_model_serializers/pull/1178) env CAPTURE_STDERR=false lets devs see hard failures (@bf4)
Expand Down
13 changes: 13 additions & 0 deletions lib/active_model/serializer/attribute.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
module ActiveModel
class Serializer
Attribute = Struct.new(:name, :block) do
def value(serializer)
if block
serializer.instance_eval(&block)
else
serializer.read_attribute_for_serialization(name)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

I feel better about this l, since it appears to be a pattern we're using all over. However, I still don't have a good sense if we'll want to keep this style, and would like to mark the object as part of the internal api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Every reference to an Attribute object is currently marked as @api private.

end
end
end
61 changes: 15 additions & 46 deletions lib/active_model/serializer/attributes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,63 +1,32 @@
module ActiveModel
class Serializer
module Attributes
# @api private
class Attribute
delegate :call, to: :reader

attr_reader :name, :reader

def initialize(name)
@name = name
@reader = :no_reader
end

def self.build(name, block)
if block
AttributeBlock.new(name, block)
else
AttributeReader.new(name)
end
end
end
# @api private
class AttributeReader < Attribute
def initialize(name)
super(name)
@reader = ->(instance) { instance.read_attribute_for_serialization(name) }
end
end
# @api private
class AttributeBlock < Attribute
def initialize(name, block)
super(name)
@reader = ->(instance) { instance.instance_eval(&block) }
end
end

extend ActiveSupport::Concern

included do
with_options instance_writer: false, instance_reader: false do |serializer|
serializer.class_attribute :_attribute_mappings # @api private : maps attribute key names to names to names of implementing methods, @see #attribute
self._attribute_mappings ||= {}
serializer.class_attribute :_attributes_data # @api private
self._attributes_data ||= {}
Copy link
Member

Choose a reason for hiding this comment

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

@beauby For anyone looking back at this, would mind jotting down the advantage of the name change?

I know that part of it is that

  • in both cases the Attribute can get the serialization value given a block or record's attribute name, and then
  • whether or not the Attribute should also know the serialization key used for its value/
-          serializer.class_attribute :_attribute_mappings # @api private : maps attribute key names to names to names of implementing methods, @see #attribute
+          serializer.class_attribute :_attributes_data # @api private
       def attributes(requested_attrs = nil, reload = false)
            @attributes = nil if reload
-          @attributes ||= self.class._attribute_mappings.each_with_object({}) do |(key, attribute_mapping), hash|
+          @attributes ||= self.class._attributes_data.values.each_with_object({}) do |attr, hash|
         def attribute(attr, options = {}, &block)
-          _attribute_mappings[key] = Attribute.build(attr, block)
+          _attributes_data[attr] = Attribute.new(attr, key, block)
-        # names of attribute methods
+        # keys of attributes
          # @see Serializer::attribute
          def _attributes
-          _attribute_mappings.keys
+          _attributes_data.values.map(&:key)
      def _attributes_keys
 -          _attribute_mappings
+          _attributes_data.values
 -            .each_with_object({}) do |(key, attribute_mapping), hash|
+            .each_with_object({}) do |attr, hash|
 -              next if key == attribute_mapping.name
+              next if attr.key == attr.name
-              hash[attribute_mapping.name] = { key: key }
+              hash[attr.name] = { key: attr.key }
+    Attribute = Struct.new(:name, :key, :block) do
-                                        (name, block)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I'd rather have it called _attributes, but it was already used... So _attributes_data instead of _attribute_mappings is simply because the Attribute object stores all data about an attribute definition, rather than a mapping.

Copy link
Member

Choose a reason for hiding this comment

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

I wrote above:

  • in both cases the Attribute can get the serialization value given a block or record's attribute name, and then
  • whether or not the Attribute should also know the serialization key used for its value/

So, the difference in the data structure is that given a serializer containing

attribute :object_attribute
attribute :aliased_attribute, :attribute_alias
attribute :dynamic_attribute { 'much_dynamic' }

The difference in _mappings vs. _data key/value pairs would be

_attribute_mappings _attributes_data
object_attribute: { name: :object_attribute, value: serializer.read_attribute_for_serialization(:object_attribute) } object_attribute: { key: :object_attribute, name: :object_attribute, value: serializer.read_attribute_for_serialization(:object_attribute) }
attribute_alias: { name: :aliased_attribute, value: serializer.read_attribute_for_serialization(:aliased_attribute) } aliased_attribute: { key: :attribute_alias, name: :aliased_attribute, value: serializer.read_attribute_for_serialization(:aliased_attribute) }
dynamic_attribute: { name: :dynamic_attribute, value: serializer.instance_eval(&block) } dynamic_attribute: { key: :dynamic_attribute, name: :dynamic_attribute, value: serializer.instance_eval(&block) }

both of which resolve to:

{
  object_attribute: object.send(:object_attribute),
  attribute_alias: object.send(:aliased_attribute),
  dynamic_attribute: 'dynamic_attribute'
}

by applying _attributes to either the _mappings or _data where they are:

---- for _attribute_mappings for _attributes_data
_attributes _attribute_mappings.keys #=> [:object_attribute, :attribute_alias, :dynamic_attribute] _attributes_data.values.map(&:key) #=> [:object_attribute, :attribute_alias, :dynamic_attribute]

which means that the definition of _attributes has changed when an attribute specifies :key; it would no longer be the serialization key, but the object attribute name.

I think this is why in every case I see, the _attributes_data ignores the key in each pair and calls _attributes_data.values, which is also why I called it a mapping. (Which I think I got from a comment on the original PR, IIRC).

That makes sense? That's the heart of what I was really asking about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. Thing is, _attributes_data could really be just an array, but access indexed by attribute name makes it easier to avoid/catch duplicate attribute definitions (although it shouldn't happen).

Copy link
Member

Choose a reason for hiding this comment

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

So you're using a hash to ensure unique attributes via its lookup keys, but otherwise only using the values?

Interesting. I believe that's what Set does internally, or functionally similar.

What do you see as the downside of just declaring the key in the serializer's data structure and let the attribute just be the value (ie previous vs current)?

It sounds like we were both considered having the Attribute contain the key, name, and block, when writing the _serialized_attributes (87d18e9, @bf4)/``_attribute_mapping(https://github.com/rails-api/active_model_serializers/commit/7bde7bf752091db741bcdc4cc83154615171d5a8, @noahsilas)/_attributes_data` (#1370, @beauby), but didn't like the idea of having an Array to iterate through to look up an attribute. So, we both preferred to put the `Attribute` in a hash, so that `_attribute_mappings`/`_attributes_data` can be used to look up an `Attribute` from a `key`. However, in this implemenation, I only see the `_attributes_data` hash being used for its values; its keys are only to ensure unique attribute keys.

So, I think whether an Attribute should know the key and value it represents, is a bit philosophical, but you obviously preferred the change, so my questions are really asking you to sell me on it! :)

Copy link
Member

Choose a reason for hiding this comment

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

I still think it maps the serialization's attribute key to its value (or something that can get its value). But, I won't hold the PR for that. Too bad no one else is commenting 😞

-          _attribute_mappings[key] = Attribute.new(attr, block) # @api private : maps attribute key names to names to names of implementing methods, @see #attribute
+          _attributes_data[key] = Attribute.new(attr, block) # @api private

Code looks good. Just:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed on both, will update later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those two remarks were taken care of today. I believe the only thing left is naming? I'd like for other people to chip in as well, but it does not seem like it's going to happen...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm willing to revert the naming back to attribute_mappings as attributes_data does not fully satisfy me either. Apart from that, are we good to merge?

Copy link
Member

Choose a reason for hiding this comment

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

  • I don't want to nitpick. I would support it, but don't want it to delay us.

end

extend ActiveSupport::Autoload
autoload :Attribute
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer the autoload below the extend ActiveSupport::Concern. I believe that's how we're doing it elsewhere. Any particular reason it's here?

Copy link
Member

Choose a reason for hiding this comment

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

or should it be auto-loaded at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel strongly about this but I followed what was in Associations. However, I didn't put the extend ActiveSupport::Autoload, which usually goes above the autoload :things. What's the use of extend ActiveSupport::Autoload?

Copy link
Member

Choose a reason for hiding this comment

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

It's a different autoload from the one in ruby. I forget what it adds. Autoload is now thread safe in our supported rubies

B mobile phone

On Dec 27, 2015, at 7:19 PM, Lucas Hosseini [email protected] wrote:

In lib/active_model/serializer/attributes.rb:

     end

  •    autoload :Attribute
    I don't feel strongly about this but I followed what was in Associations. However, I didn't put the extend ActiveSupport::Autoload, which usually goes above the autoload :things. What's the use of extend ActiveSupport::Autoload?

—
Reply to this email directly or view it on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

  • I don't know that referring to this as a mixin is a useful changelog entry, but I'm going to merge it.. just to end the back and forth on it ;)

Simplify attributes handling via a mixin


# Return the +attributes+ of +object+ as presented
# by the serializer.
def attributes(requested_attrs = nil, reload = false)
@attributes = nil if reload
@attributes ||= self.class._attribute_mappings.each_with_object({}) do |(key, attribute_mapping), hash|
@attributes ||= self.class._attributes_data.each_with_object({}) do |(key, attr), hash|
next unless requested_attrs.nil? || requested_attrs.include?(key)
hash[key] = attribute_mapping.call(self)
hash[key] = attr.value(self)
end
end
end

module ClassMethods
def inherited(base)
super
base._attribute_mappings = _attribute_mappings.dup
base._attributes_data = _attributes_data.dup
end

# @example
Expand Down Expand Up @@ -85,25 +54,25 @@ def attributes(*attrs)
# end
def attribute(attr, options = {}, &block)
key = options.fetch(:key, attr)
_attribute_mappings[key] = Attribute.build(attr, block)
_attributes_data[key] = Attribute.new(attr, block)
end

# @api private
# names of attribute methods
# keys of attributes
# @see Serializer::attribute
def _attributes
_attribute_mappings.keys
_attributes_data.keys
end

# @api private
# maps attribute value to explict key name
# @see Serializer::attribute
# @see Adapter::FragmentCache#fragment_serializer
def _attributes_keys
_attribute_mappings
.each_with_object({}) do |(key, attribute_mapping), hash|
next if key == attribute_mapping.name
hash[attribute_mapping.name] = { key: key }
_attributes_data
.each_with_object({}) do |(key, attr), hash|
next if key == attr.name
hash[attr.name] = { key: key }
end
end
end
Expand Down