Add optional fields support for associations #1284

vasilakisfil · 2015-10-20T18:32:01Z

I am really fond of JSONAPI but there are people that they still need JSON (mostly) and Attributes adapters :) Plus, we should opt for best migration from 0.8.x and 0.9x adapters.

There is a minor bug in the fields option in master branch. If you have:

render json: @videos.limit(1), each_serializer: VideoSerializer,  fields: [:id],

This will apply the fields to all associations too. We don't want that. What we want is:

If there is a fields param, apply it to the main resource. If there are associations, serialize the associations without removing any attributes. For instance:

render json: @videos.limit(1), each_serializer: VideoSerializer,  fields: [:id]

Then this should apply the fields: [:id] only in the VideoSerializer and not in any association.

If there are associations in the fields array (just like in StrongParams) and these also exist in the main resource, apply the associations fields there. For instance:

render json: @videos.limit(1), each_serializer: VideoSerializer,  fields: [:id, user: [:name]]

Then this should apply the fields: [:id] only in the VideoSerializer and if there is a user association, apply fields: [:name] in the UserSerializer.

I am really looking forward to merge this PR so if you need anything from me just ping me. I will also create a regression test highlighting the bug.

NullVoxPopuli · 2015-10-20T18:39:44Z

@vasilakisfil can you fix the errors mentioned in travis? thanks!

Add optional fields support for associations wip fix tests fix styling

vasilakisfil · 2015-10-20T19:03:48Z

@NullVoxPopuli fixed in travis, waiting for appveyor which takes some time..
Possibly linked to #1285 and #1286

beauby · 2015-10-20T19:09:49Z

So if we're going down this way you should make use of the IncludeTree class that does exactly what you want, plus it can be built from a string and an extended array/hash syntax.

beauby · 2015-10-20T19:11:53Z

lib/active_model/serializer/adapter/attributes.rb

-            relationships[association.key] = relationship_value_for(association, options)
+            relationships[association.key] = relationship_value_for(
+              association,
+              options.merge(fields: association_fields_hash[association.name])


If you do that, not providing the fields option to the adapter will result in no attribute being serialized.

not the case since this runs only for associations. The case you said would only happen with the following (given that there is a user association):

render json: @videos.limit(1), each_serializer: VideoSerializer, fields: [:id, user: []]

Plus, test would cry out if that was true.

Sorry that was an oversight.

vasilakisfil · 2015-10-20T22:19:43Z

@beauby "So if we're going down this way you should make use of the IncludeTree class that does exactly what you want, plus it can be built from a string and an extended array/hash syntax."

I never considered AMS job to parse the query string (+ strings are not the way to go in Ruby but hashes/arrays). Especially when we are talking about JSON adapter. But sure I can fix it.

What do you mean extended array/hash syntax ?

beauby · 2015-10-22T01:55:07Z

@vasilakisfil The query string parsing is here just as a commodity for people dealing with JSON API stuff. The "extended array/hash syntax" is the one described here. Basically it's an array of hashes and symbols (S → Array<Symbol | Hash{Symbol:S}>).

vasilakisfil · 2015-10-22T07:09:35Z

@beauby the array/hash syntax is supported in this pull request: fields: [:id, { user: [:name] }]. Query string was not supported before this pull request. Do you want me to add support for it?

I am totally against it though because unless you are writting an internal API, you will want to filter the query params (query string) beforehand using StrongParams or something else inside the controller so you would need to convert it to an array of hashes/symbols anyway. Extract a helper method that parses this query string in the controller is the way to go in my opinion.

passalini · 2015-11-05T11:33:44Z

+1
ps: nice idea @vasilakisfil

bf4 · 2015-11-19T20:14:42Z

test/adapter/json/collection_test.rb

+          def test_fields_option
+            serializer = ArraySerializer.new([@first_post, @second_post])
+            adapter = ActiveModel::Serializer::Adapter::Json.new(serializer)
+            actual = adapter.serializable_hash(fields: [:id])


Looking good

this needs an integration (controller) test

Please use ActiveModel::SerializableResource.new([@first_post, @second_post], fields: [:id])

Please don't test passing options into serializable_hash since only the renderer would do that

Which is to say, this isn't testing how the library is used

(that would be a better test usage in the integration test, where the options to render json: foo, .... are passed to AMS then to the renderer (unless we mutate them))

@vasilakisfil Are you still interested in this?

vasilakisfil · 2015-12-23T16:49:37Z

@bf4 hmm I am not sure. I have already created my own adapter that is compatible with 0.9.x version and supports association fields among others.

I could work on that but I feel it's kind of deadend. Most attributes/json-adapter related code hasn't been updated for a long time now and most committers in this project doesn't seem to be interested to extend json/attributes adapter features but instead try to keep them as minimal as possible and try to spend more time to jsonapi adapter (which might be a good idea until Rails 5 comes out). That's why I opted for a custom adapter to move faster.

bf4 · 2015-12-23T17:46:43Z

custom adapter is good. I'm not sure what you mean by extending the json/attributes adapters. Do you mean improving or otherwise working on?

williamweckl · 2016-02-26T00:41:46Z

+1

bf4 · 2016-02-26T05:52:10Z

I have already created my own adapter that is compatible with 0.9.x version and supports association fields among others.

@vasilakisfil Is that something you could share? Would be very helpful.

Any interest in reviving this PR?

NullVoxPopuli · 2016-03-16T11:32:32Z

@vasilakisfil could you rebase this? :-)

NullVoxPopuli · 2016-03-16T11:34:35Z

I'd like to get this merged before taking a look at #1286

vasilakisfil · 2016-03-16T13:17:45Z

Sure I would love to rebase it and give some love to the JSON/Attributes adapters but not now during Easter (1.5 week from now) as I am super busy currently.

I have created 4-5 monkey patched eventually to make it work like GraphQL, didn't create my own adapter.

bf4 · 2016-03-16T13:46:39Z

@vasilakisfil

I have created 4-5 monkey patched eventually to make it work like GraphQL, didn't create my own adapter.

would love to see them!

vasilakisfil · 2016-03-16T13:54:38Z

Ok if it helps, the following have been tested up until b2cd7bb (the git commit locked in my Gemfile).

There is one more bug (I think it's a bug and not intended) down the line (in ActiveModel::Serializer::Associations#association method), so this pull request doesn't actually solves it. But as I said I think JSON/Attributes adapters have been slightly neglected by the team in favor of JSONAPI adapter and I would love to help out.

class ActiveModel::Serializer::Adapter::Attributes
  def relationship_value_for(association, options)
    return association.options[:virtual_value] if association.options[:virtual_value]
    return unless association.serializer && association.serializer.object

    opts = instance_options.merge(
      include: @include_tree[association.key],
      fields: nil
    )
    self.class.new(association.serializer, opts).serializable_hash(
      options.merge(fields: nil)
    )
  end

  def serializable_hash(options = nil)
    options ||= {}
    options[:fields] = instance_options[:fields] if options[:fields].nil?

    if serializer.respond_to?(:each)
      serializable_hash_for_collection(options)
    else
      serializable_hash_for_single_resource(options)
    end
  end

  def serializable_hash_for_collection(options)
    serializer.map { |s|
      self.class.new(s, instance_options).serializable_hash(options)
    }.select{|s| !s.blank?}
  end

  def serializable_hash_for_single_resource(options)
    return {} if serializer.invalid_for_serialization?

    resource = resource_object_for(options)
    relationships = resource_relationships(options)
    resource.merge!(relationships)
  end
end
module ActiveModel
  class Serializer
    module Associations
      def associations(include_tree = DEFAULT_INCLUDE_TREE)
        return unless object

        Enumerator.new do |y|
          self.class._reflections.each do |reflection|
            key = reflection.options.fetch(:key, reflection.name)
            next unless include_tree.key?(key)
            y.yield reflection.build_association(
              self, instance_options.merge(fields: nil)
            )
          end
        end
      end
    end
  end
end

class ActiveModel::SerializableResource
  def initialize(resource, options = {})
    @resource = resource
    @adapter_opts, @serializer_opts =
      options.partition { |k, _| ADAPTER_OPTION_KEYS.include? k }.map { |h| Hash[h] }
    @serializer_opts.merge!(fields: options[:fields])
  end
end

class ActiveModel::Serializer
  def attributes
    attributes = self.class._attributes.dup

    attributes.each_with_object({}) do |name, hash|
      next unless instance_options[:fields].nil? || field_allowed?(name)

      if self.class._fragmented
        hash[name] = self.class._fragmented.public_send(name)
      else
        hash[name] = send(name)
      end
    end
  end
  def field_allowed?(name)
   instance_options[:fields] && instance_options[:fields].include?(name)
  end
end

vasilakisfil · 2016-03-16T13:57:01Z

The last monkeypatch is irrelevant actually, it allows you to take out an item from a collection just before serializing.

vasilakisfil · 2016-03-16T14:12:54Z

No sorry, that's wrong, I don't even remember my own monkey patches haha. The last one is something else I think but the invalid_for_serialization? part does what I said :)

Anyway :) But I would be happy to commit to this project.

remear · 2016-09-16T00:41:20Z

Closing due to inactivity

NullVoxPopuli · 2016-09-16T03:02:49Z

I also believe this functionality already exists

Add optional fields support for associations

492b9c9

Add optional fields support for associations wip fix tests fix styling

beauby reviewed Oct 20, 2015
View reviewed changes

bf4 reviewed Nov 19, 2015
View reviewed changes

bf4 added C: Feature V: 0.10.x S: Needs Submitter Response labels Dec 9, 2015

remear added S: Needs Team Discussion and removed S: Needs Submitter Response labels Mar 28, 2016

vasilakisfil mentioned this pull request Mar 29, 2016

Optional fields in associations (priority on serializer) #1628

Closed

remear closed this Sep 16, 2016

Add optional fields support for associations #1284

Add optional fields support for associations #1284

Uh oh!

Conversation

vasilakisfil commented Oct 20, 2015

Uh oh!

NullVoxPopuli commented Oct 20, 2015

Uh oh!

vasilakisfil commented Oct 20, 2015

Uh oh!

beauby commented Oct 20, 2015

Uh oh!

beauby Oct 20, 2015

Choose a reason for hiding this comment

Uh oh!

vasilakisfil Oct 20, 2015

Choose a reason for hiding this comment

Uh oh!

beauby Oct 22, 2015

Choose a reason for hiding this comment

Uh oh!

vasilakisfil commented Oct 20, 2015

Uh oh!

beauby commented Oct 22, 2015

Uh oh!

vasilakisfil commented Oct 22, 2015

Uh oh!

passalini commented Nov 5, 2015

Uh oh!

bf4 Nov 19, 2015

Choose a reason for hiding this comment

Uh oh!

bf4 Dec 23, 2015

Choose a reason for hiding this comment

Uh oh!

vasilakisfil commented Dec 23, 2015

Uh oh!

bf4 commented Dec 23, 2015

Uh oh!

williamweckl commented Feb 26, 2016

Uh oh!

bf4 commented Feb 26, 2016

Uh oh!

NullVoxPopuli commented Mar 16, 2016

Uh oh!

NullVoxPopuli commented Mar 16, 2016

Uh oh!

vasilakisfil commented Mar 16, 2016

Uh oh!

bf4 commented Mar 16, 2016

Uh oh!

vasilakisfil commented Mar 16, 2016

Uh oh!

vasilakisfil commented Mar 16, 2016

Uh oh!

vasilakisfil commented Mar 16, 2016

Uh oh!

remear commented Sep 16, 2016

Uh oh!

NullVoxPopuli commented Sep 16, 2016

Uh oh!

Uh oh!