Add inline syntax for attributes and associations #1356

bf4 · 2015-11-30T23:52:53Z

Goals:

Allow defining attributes so that they don't conflict with existing methods
Allow customizing associations e.g. has_many :comments do last(5) end
- compare to using virtual_value such as has_many :reviews, virtual_value: [{ id: 1 }, { id: 2 }]
  and how association virtual values differ from attribute values.
Remove dynamically defined methods on the serializer

Based on work in #1262, #1354

This PR is intended as a starting point for further work improving how
serializers map their resources attributes and associations

Introduced, in support of this:

Serializer#_attribute_mappings
Extracted Serializer attribute methods to an Attributes module. I'd consider a class but this is a simpler first step and doesn't block changing it to an Attributes class than has many Attribute objects in the future

Possible regression:

removed referring to object attributes in the serializer without calling them directly on object. (I didn't even know this was possible. We could add a respond_to_missing / method_missing for this)

   def slug
-    "#{name}-#{id}"
+    "#{object.name}-#{object.id}"

Discuss

Associations as 'includes'

It seems like we might benefit from thinking of associations, at least in the JSON API adapter, as includes, and only including them when requested (which is also similar to earlier naming and usage in the library, which I like). This PR made me think of it, in part, because the difference between a reflection, an association, and associations, I think should be more clear (i.e. just an include).

Discuss

Blocks in serializer attribute definitions

I like this. They remove the need for defining methods on the serializer, and make these attributes seem more like transformations. I don't mind that they require a call to object, but would be okay eval'ing them in the context of the object, instead of the serializer.

attribute :name do 'AMS' end

Blocks in serializer association definitions

Since there may be more options to pass in here, and to distinguish the context of an association block, which is on the association, from the attribute block, which is on the serializer, I'm tossing around the idea of having a scope option the receives a proc or lambda. e.g.

has_many :comments, ->{ last(5) } (NOT ->{ comments.last(5) }
- consider has_many :comments, scope: ->{ last(5) }

beauby · 2015-11-30T23:55:43Z

lib/active_model/serializer/associations.rb

+            serialized_associations[reflection_name] = ->(instance) { instance.object.send(reflection_name) }
+          end
+
+          define_method reflection_name do


Is there still a need/reason to define such methods on the serializer?

No, except that it breaks some tests, and I didn't want to go and fix 'em :)

So, we can do it in two steps

Yeah that's what was holding me back, because even after fixing the tests, there are caching issues (I believe caching is broken tbh).

oh, and re having a serializer/attributes.rb file, like in your prs, .. I'm pro moving more stuff in there (though I think it should be a class), but even the current Struct isn't really necessary now. we can put that off to future work

The idea behind serializer/attributes.rb was mainly to separate various serializer concerns into their own modules/files. The goal was that serializer.rb would contain as little stuff as possible and that the logic for handling attributes-related stuff would be in one file (class and instance methods).

Yeah, I started on it, but it wasn't worth the extra effort in this PR. That file is sitting on my file-system

beauby · 2015-11-30T23:59:49Z

lib/active_model/serializer.rb

+      if block_given?
+        serialized_attributes[key] = Attribute.new(->(instance) { instance.instance_eval(&block) })
+      else
+        serialized_attributes[key] = Attribute.new(->(instance) { instance.object.read_attribute_for_serialization(attr) })


Why encapsulate the lambda in an Attribute object? (Especially considering we're not doing the same for associations)

no reason right now. I was thinking I'd do more with it, but I didn't. So, can be removed.

Yeah, I think we talked about it and I agree that at some point it would be nice, but at the moment I think it just adds complexity.

bf4 · 2015-12-01T00:36:06Z

rebased and force pushed. I don't think I broke anything...

Adapted from rails-api#1262

noahsilas · 2015-12-01T18:58:41Z

It's not clear to me that this PR satisfies goal 1 (Allow defining attributes so that they don't conflict with existing methods). It looks like trying to add a key whose name is already a method name on the serializer will add an entry to serialized_attributes, but that it still delegates to the existing method. I tried to get around this in #1354 by avoiding the usage of the key name as the method name, but it seems like there is room for improvement over that here in evaluating the proc directly instead of (or in addition to?) adding methods to the serializer.

bf4 · 2015-12-01T19:24:00Z

test/serializers/attribute_test.rb

+        attribute :name do
+          "#{object.first_name} #{object.last_name}"
+        end
+      end


@noahsilas I do want to remove the dynamic methods on the serializer, just not in this PR.

However, I do think it's a good idea for there to be a test against attribute names/keys that conflict with methods on the serializer. I think the read_attribute_for_serialization should get around that

I mean, right now it is

self.class._fragmented ? self.class._fragmented.public_send(key) : send(key)

but it could be

self.class._fragmented ? self.class._fragmented.public_send(key) : object.read_attribute_for_serialization(key)

I think that the problem here is that the key may not be the name of the attribute that we want to read, as in:

class MyThingSerializer < ActiveModel::Serializer attribute :name, key: :object end

This should render something like { "object": "my name" }, but to get that, we actually need to ~~invoke the name method on the object~~ read the name attribute for serialization.

I should add a case here using the virtual_value option. e.g. the poro in our fixtures file

class VirtualValueSerializer < ActiveModel::Serializer attributes :id has_many :reviews, virtual_value: [{ id: 1 }, { id: 2 }] end

noahsilas · 2015-12-01T20:19:42Z

If you want, I can rebuild #1354 on top of this change and submit a new PR, separating the goal of block definition from the goal of avoiding namespace conflicts.

bf4 · 2015-12-01T20:40:46Z

You can re-use this pr

Take it away (please)!

B mobile phone

On Dec 1, 2015, at 2:19 PM, Noah Silas [email protected] wrote:

If you want, I can rebuild #1354 on top of this change and submit a new PR, separating the goal of block definition from the goal of avoiding namespace conflicts.

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

noahsilas · 2015-12-01T23:47:37Z

I'm not sure that I can push onto this PR, but rebasing #1354 onto this branch ends up looking like this: https://github.com/brigade/active_model_serializers/commits/attribute_objects (specifically, see brigade/active_model_serializers@ca76be1a46 ).

There is still some weird interplay with defining methods on the serializer so that the old style method overrides still work, which is a little disappointing, but a major break in the API, so I didn't feel comfortable removing it.

bf4 · 2015-12-02T05:08:01Z

@noahsilas re: 'you can re-use this pr', my mistake. I was replying from email and thought it was the one you authored.

I was thinking that a way to get rid of the dynamic methods would be to pretend they're there. Maybe with a respond_to_missing, or in the 'reader'

bf4 · 2015-12-02T06:28:33Z

@noahsilas I added your commit from that branch and made one major and a few minor changes

removed dynamically define methods (yay!)
- added the _fragment attributes to the mapping. This handles the _fragmented.respond_to?(attr) the prevented the dynamic method from being defined
- added Serializer#read_attribute_for_serialization (part of the ActiveModel::Serialization API) to handle first getting the attribute from a serializer method, then from the object.
  - Checking if the attribute is defined on the serializer handles the dynamic method check for method_defined?(attr), except now it just checks if there's a public instance method by that name not defined in Object.
  - possible regression: removed referring to object attributes in the serializer without calling them directly on object. (I didn't even know this was possible. We could add a respond_to_missing / method_missing for this)
changed _attributes_map to _attribute_mappings so that the collection element could be referred to as each of an _attribute_mapping.
- Changed the mapping from a hash to an object
```
-      _attributes_map[key] = { attr: attr, reader: reader }
+      _attribute_mappings[key] = Attribute.new(attr, reader)
```
- Defined the object as a callable that knows the attribute name it represents
```
+    Attribute = Struct.new(:name, :reader) do
+      delegate :call, to: :reader
+    end
```

The PR description is now out of date

bf4 · 2015-12-02T06:31:02Z

lib/active_model/serializer.rb

@@ -14,6 +14,9 @@ class Serializer
    include Configuration
    include Associations
    require 'active_model/serializer/adapter'
+    Attribute = Struct.new(:name, :reader) do
+      delegate :call, to: :reader
+    end


See #1356 (comment)

I could go either way on this.

bf4 · 2015-12-03T16:57:20Z

lib/active_model/serializer/associations.rb

-        class << base
-          attr_accessor :_reflections
+      included do
+        with_options instance_writer: false, instance_reader: true do |serializer|


I don't know if there's a difference between the block variables base and serializer in what we want here.

Unclear to me as well.

bf4 · 2015-12-03T17:02:03Z

@groyoh @beauby Updated removing all dynamic methods on attributes and associations with full backwards compatibility

Anything you think is needed in this PR to support future work?

noahsilas · 2015-12-03T23:34:52Z

@groyoh @beauby The relationship links work seems like it is primarily applicable to the JsonApi adapter; is there a way to satisfy this desire that doesn't introduce overhead for all other adapters? (I may be misunderstanding, but having to specify the relationship via a data method seems confusing for the AttributesAdapter, for instance).

bf4 · 2015-12-04T16:57:26Z

@noahsilas

The relationship links work seems like it is primarily applicable to the JsonApi adapter;

True, but it's not in this PR :)

groyoh · 2015-12-04T18:22:17Z

lib/active_model/serializer.rb

+
+    private
+
+    def _serializer_instance_methods


Wouldn't that be better as a class method performance wise?

I think you're right. I was working on the instance-level and got tunnel-vision

Per groyoh rails-api#1356 (comment)

bf4 · 2015-12-04T19:46:12Z

lib/active_model/serializer/reflection.rb

+
+      def self.build_reader(name, block)
+        if block
+          ->(instance) { instance.instance_eval(&block) }


I'm looking at 0.8 and how JSON API considers associations 'includes' and am thinking that association block should be applied to the association, rather than the serializer. Otherwise, there's really nothing that ties the block to an association.

So, usage from current impl would be

has_many :comments do - comments.limit(5) + limit(5) end

which is also more inline with how the scope method works in Rails. I think this makes the api a lot more clear. If you want a custom attribute, use the attribute block. If you want a scoped association, I think it the instance_eval should be on the association.

See

0.8

def associate(klass, attrs) #:nodoc: options = attrs.extract_options! self._associations = _associations.dup attrs.each do |attr| unless method_defined?(attr) define_method attr do object.send attr end end define_include_method attr self._associations[attr] = klass.refine(attr, options) end end def define_include_method(name) method = "include_#{name}?".to_sym INCLUDE_METHODS[name] = method unless method_defined?(method) define_method method do true end end end def include_associations! _associations.each_key do |name| include!(name) if include?(name) end end def include?(name) return false if @options.key?(:only) && !Array(@options[:only]).include?(name) return false if @options.key?(:except) && Array(@options[:except]).include?(name) send INCLUDE_METHODS[name] end def include!(name, options={})

0.9

EMBED_IN_ROOT_OPTIONS = [ :include, ] def associations(options={}) associations = self.class._associations included_associations = filter(associations.keys) associations.each_with_object({}) do |(name, association), hash| if included_associations.include? name # ... end def filter(keys) if @only keys & @only elsif @except keys - @except else keys end end

bf4 · 2015-12-04T20:04:01Z

lib/active_model/serializer/reflection.rb

+    #  Notice that the association block is evaluated in the context of the association.
+    #  Specifically, the association 'comments' is evaluated two different ways:
+    #  1) as 'comments' and named 'comments'.
+    #  2) as 'comments.last(1)' and named 'last_comments'.


See comment https://github.com/rails-api/active_model_serializers/pull/1356/files#r46723777 about how we should think of association blocks (and how we might want to also consider them as only used when included under JSON API) and see #1325 (comment)

groyoh · 2015-12-04T20:35:05Z

@noahsilas I'm pretty sure we can work around this issue. For example, making sure that meta, and link methods return a certain object (e.g. a symbol as in @beauby's example) and serialize the relationship depending on the block return value:

Use object.comments

has_many :comments

Use the block return value

has_many :comments do
  object.special_comments
end

With link but use object.comments

has_many :comments do
  link :self do
    #something
  end # => :SpecialSymbolToPreventInlineAssociation
end

With link and use block return value

has_many :comments do
  link :related do
    #something
  end
  object.association # or data object.association
end

The first two would work pretty well with any adapter while the last one would only fit JSONAPI and everybody is happy 😁 .

Of course, this probably require more thoughts and maybe have some flaws too.

beauby · 2015-12-07T02:31:28Z

lib/active_model/serializer/reflection.rb

@@ -17,7 +25,28 @@ class Serializer
    #
    # So you can inspect reflections in your Adapters.
    #
-    Reflection = Struct.new(:name, :options) do
+    Reflection = Struct.new(:name, :options, :block) do
+      delegate :call, to: :reader


Why delegating it rather than calling @reader.call in value?

I just think it reads better. It might be a performance issue.

I think there's definitely room for refactoring and revising this, but I don't think it should be a blocker. I don't really like how the reflection differs from the attribute, but figure we can kick that can down the road a bit more into its own PR.

bf4 · 2015-12-09T04:19:29Z

@rails-api/ams Anything blocking merging this? I'll merge in 24 hours unless I hear otherwise, since it seems pretty well-discussed, and we can always make changes later

NullVoxPopuli · 2015-12-09T12:29:07Z

@bf4 I think it'll be a good addition :-)

Add inline syntax for attributes and associations

Per groyoh rails-api#1356 (comment)

bf4 added C: Feature V: 0.10.x S: Needs Team Discussion S: Needs Code Review S: Ready for PR labels Nov 30, 2015

bf4 mentioned this pull request Nov 30, 2015

Add inline syntax to override attributes/associations. #1262

Closed

beauby reviewed Nov 30, 2015
View reviewed changes

bf4 added the S: Changelog Missing label Nov 30, 2015

beauby reviewed Nov 30, 2015
View reviewed changes

bf4 force-pushed the attribute_objects branch from 1de7489 to 011b589 Compare December 1, 2015 00:34

bf4 force-pushed the attribute_objects branch from 011b589 to 4bdf44a Compare December 1, 2015 00:36

bf4 referenced this pull request in bf4/active_model_serializers Dec 1, 2015

Add inline syntax for defining associations

ab6b343

Adapted from rails-api#1262

bf4 reviewed Dec 1, 2015
View reviewed changes

bf4 force-pushed the attribute_objects branch from 33513fc to eb6850e Compare December 2, 2015 06:18

bf4 reviewed Dec 2, 2015
View reviewed changes

bf4 reviewed Dec 3, 2015
View reviewed changes

groyoh reviewed Dec 4, 2015
View reviewed changes

Serializer instance methods don't change; track at class level

3e8290a

Per groyoh rails-api#1356 (comment)

bf4 reviewed Dec 4, 2015
View reviewed changes

Evaluate association blocks as scopes on the association

386a567

bf4 reviewed Dec 4, 2015
View reviewed changes

beauby reviewed Dec 7, 2015
View reviewed changes

Document Serializer settings and private api [ci skip]

bf8270b

bf4 force-pushed the attribute_objects branch from d3837f9 to bf8270b Compare December 10, 2015 21:08

bf4 added a commit that referenced this pull request Dec 10, 2015

Merge pull request #1356 from bf4/attribute_objects

f562449

Add inline syntax for attributes and associations

bf4 merged commit f562449 into rails-api:master Dec 10, 2015

bf4 deleted the attribute_objects branch December 10, 2015 21:10

bf4 removed S: Needs Code Review S: Ready for PR labels Dec 10, 2015

beauby mentioned this pull request Dec 10, 2015

Simplify attributes handling. #1370

Merged

noahsilas mentioned this pull request Dec 10, 2015

Enable use of an object key in serilaized forms #1354

Closed

This was referenced Dec 14, 2015

Block syntax for associations. #1311

Closed

Consider evaluating association in serializer context #1378

Merged

joaomdmoura pushed a commit to joaomdmoura/active_model_serializers that referenced this pull request Jan 19, 2016

Serializer instance methods don't change; track at class level

bd3df5b

Per groyoh rails-api#1356 (comment)

Add inline syntax for attributes and associations #1356

Add inline syntax for attributes and associations #1356

Conversation

bf4 commented Nov 30, 2015

Goals:

Introduced, in support of this:

Possible regression:

Associations as 'includes'

Blocks in serializer attribute definitions

Blocks in serializer association definitions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Dec 1, 2015

noahsilas commented Dec 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahsilas commented Dec 1, 2015

bf4 commented Dec 1, 2015

noahsilas commented Dec 1, 2015

bf4 commented Dec 2, 2015

bf4 commented Dec 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Dec 3, 2015

noahsilas commented Dec 3, 2015

bf4 commented Dec 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groyoh commented Dec 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Dec 9, 2015

NullVoxPopuli commented Dec 9, 2015