Fieldset support in Attributes/JSON adapters #2223

bf4 · 2017-11-20T06:02:02Z

Supersedes stale PR #2100
Closes: #2100, #2278
Addresses partially: #2273

bf4 · 2017-11-20T06:04:51Z

lib/active_model/serializer.rb

+        begin
+          object.class.model_name.to_s.underscore
+        rescue ArgumentError
+          'anonymous_object'


some of the tests use anonymous classes.. not sure how real world this is, but I figure giving it a descriptive fallback name is a good compromise

For serialized POROs (i.e. whose class does not understand model_name message), does this imply json_key resolves to 'anonymous_object'? Or do I miss something?

Rationale:

with :attributes adapter there is no root;

_type is not required for AMS v0.9.x serializers;

PORO classes do not respond to model_name:

NoMethodError: undefined method `model_name' for OpenStruct:Class # /Users/user/.rvm/gems/ruby-2.3.1@gemset1/gems/active_model_serializers-0.10.7/ lib/active_model/serializer.rb:374:in `json_key'

rescue ArgumentError won't catch NoMethodError.

Well, since this version of AMS describe what's a valid PORO, and even provides a linter for it, then it's reasonable to assume that's what you're using...

looks like AMS should rescue the NoMethodError and raise saying "no explicit root given, could not infer root from object" with maybe a link for more info.

The argument error is fine.

begin object.class.model_name.to_s.underscore rescue ArgumentError 'anonymous_object' rescue NoMethodError raise 'no explicit root given; could not infer root from object.'

something like this? also can we provide the PORO link in the msg?

@bf4 would you like to have another look at this, please?

@wasifhossain that seems fine, though maybe raise a different error

can you please point me out which error class can we use for the case :) do you mean creating a custom class?

@wasifhossain I was thinking we could just define one. Maybe

UnknownSerializerRootError = Class.new(NoMethodError)

?

looks fine to me. will do this. thank you!

bf4 · 2017-11-20T06:08:03Z

lib/active_model_serializers/adapter/attributes.rb

      def serializable_hash(options = nil)
        options = serialization_options(options)
        options[:fields] ||= instance_options[:fields]
        serialized_hash = serializer.serializable_hash(instance_options, options, self)

        self.class.transform_key_casing!(serialized_hash, instance_options)
      end
+
+      def fields_to_fieldset(fields)


Because, as per docs/general/fields.md, we have a different syntax for fieldset in JSON adapter vs. JSON:API

The test usage in #2100 is [:title, {comments: [:body]}. I'm not sure if we ever considered the fields option for relationships

However, I'd accept just turning an array into an implicit fieldset, but otherwise require it to be jsonapi-like.

In other words, I have no idea at this time if the below code is introducing new or desired behavior. Needs thought

and in a specific controller, you want to return access_token only, fields will help you:

class AnonymousController < ApplicationController def create render json: User.create(activation_state: 'anonymous'), fields: [:access_token], status: 201 end end

Note that this is only valid for the json and attributes adapter. For the json_api adapter, you would use

render json: @user, fields: { users: [:access_token] }

Where users is the JSONAPI type.

bf4 · 2017-11-20T06:10:06Z

lib/active_model/serializer.rb

@@ -357,6 +357,9 @@ def associations(include_directive = ActiveModelSerializers.default_include_dire
    def serializable_hash(adapter_options = nil, options = {}, adapter_instance = self.class.serialization_adapter_instance)
      adapter_options ||= {}
      options[:include_directive] ||= ActiveModel::Serializer.include_directive_from_options(adapter_options)
+      if (fieldset = adapter_options[:fieldset])
+        options[:fields] = fieldset.fields_for(json_key)
+      end


For some reason, this seemed to be the right place for this, code-wise. I think it's because the serializer has some responsibility for serializing associations

bf4 · 2017-11-20T06:11:11Z

lib/active_model_serializers/adapter/attributes.rb

@@ -1,13 +1,32 @@
 module ActiveModelSerializers
  module Adapter
    class Attributes < Base
+      def initialize(*)
+        super
+        instance_options[:fieldset] ||= ActiveModel::Serializer::Fieldset.new(fields_to_fieldset(instance_options.delete(:fields)))


the fieldset for the adapter is set at init. The fields the adapter is told to use is passed to serializable_hash. Not sure how good a distinction this is, but it's also how the json:api adapter works

Since the JSON Adapter delegates to Attributes here, this is the only necessary adapter change

honestly, this particular comment answered my long-standing question for this PR 😄 clever work 👏

cintamani

This LGTM! Shall we rebase it against the latest 0-10-stable and go for a merge?

wasifhossain · 2019-02-11T15:34:10Z

@bf4 very good explanations for each set of changes 👍

krzysiek1507 · 2019-02-19T12:49:12Z

@bf any update on this?

krzysiek1507 · 2019-02-19T12:51:20Z

lib/active_model_serializers/adapter/attributes.rb

+          else fail ArgumentError, "Unknown conversion of fields to fieldset: '#{field.inspect}' in '#{fields.inspect}'"
+          end
+        end
+        relationship_fields.merge(serializer.json_key.to_sym => resource_fields)


Could please change it either to merge! or

relationship_fields[serializer.json_key.to_sym] = resource_fields relationship_fields

good point. I wonder why I didn't mutate a local var we own and mutate elsewhere

@wasifhossain you're welcome to take ownership of this PR if you'd like

@bf4 thanks for the offer. I will rebase it as @cintamani suggested earlier as well as make this small fix (prefer using merge!) before merging this PR. do you have any suggestion, pls?

Only real suggestion is to bear in mind once merged we need to support it.

I didn't merge it in part due to lack of confidence it was the right change.

I don't like to add features without someone actually trying it out.

johan-smits · 2019-03-25T12:38:53Z

@wasifhossain It works as expected indeed 😄. A 👍 from my side.

wasifhossain · 2019-03-25T13:31:49Z

that's great news @johan-smits! Thanks very much for your valuable feedback 👍

will look forward to some more feedback from production usage of this PR in coming weeks and then merge it.

johan-smits · 2019-03-25T13:56:12Z

@wasifhossain we are using the branch in acceptance tomorrow. Without negative feedback the coming week it is ok.

johan-smits · 2019-04-04T11:44:18Z

No negative feedback from my side, looking forward to the new release!

wasifhossain · 2019-04-04T22:25:15Z

great feedback! @johan-smits I will work in this weekend to merge it. thanks

johan-smits · 2019-04-16T12:27:05Z

@wasifhossain no pressure but no merge done yet?

wasifhossain · 2019-04-16T12:30:13Z

@johan-smits honestly I was running under a bit of pressure in past weeks. but looking forward to taking care of this in coming weekend. appreciate your patience 👍 thanks!

wasifhossain · 2019-04-23T09:55:27Z

so we are all green on travis. let's merge it 🚢

wasifhossain · 2019-04-23T09:58:18Z

@johan-smits now you can use this feature by pointing your Gemfile to 0-10-stable branch until the next gem version (guessing 0.10.10) is published

johan-smits · 2019-04-23T13:43:40Z

@wasifhossain testing...

flvrone · 2019-07-29T14:53:03Z

Guys, I think we are rescuing wrong error here:

active_model_serializers/lib/active_model/serializer.rb

Line 388 in 06fcd34

rescue ArgumentError

In my case when I work with PORO's, it actually raises

NoMethodError:
       undefined method `model_name' for Money:Class

And it totally makes sense, isn't it? :)

P.S. I didn't have any issues with it on version 0.10.9. But it looks like that model_name call was there all the time.

wasifhossain · 2019-07-29T17:58:50Z

@FunkyloverOne here is a good thread for you to follow: #2223 (comment)

AMS expects serialized models to behave like an ActiveModel::Model now: rails-api/active_model_serializers#2223

AMS requires that `root` be set to a string: rails-api/active_model_serializers#2223

bf4 commented Nov 20, 2017

View reviewed changes

bf4 mentioned this pull request Aug 10, 2018

Serializer upgrade question - options (:meta & :fields) in v0.9.x and v0.10.x #2273

Open

bf4 mentioned this pull request Aug 28, 2018

Enable filtering of fields in nested assocation #2278

Closed

bf4 mentioned this pull request Oct 11, 2018

Eagerload on application boot #2289

Merged

bf4 mentioned this pull request Feb 8, 2019

Support association nested fields without json api #2146

Closed

cintamani approved these changes Feb 11, 2019

View reviewed changes

krzysiek1507 suggested changes Feb 19, 2019

View reviewed changes

wasifhossain force-pushed the fields_support_in_all_adapters branch from e527977 to 6a8994f Compare February 20, 2019 21:04

wasifhossain force-pushed the fields_support_in_all_adapters branch from e300ce7 to d796a0e Compare March 21, 2019 19:54

wasifhossain mentioned this pull request Mar 22, 2019

+ Added association field support. #2324

Closed

bf4 and others added 5 commits April 23, 2019 04:33

Follows #2100

06fcd34

Refactoring

a9cb097

Add missing frozen string literal comment

807e160

Add changelog entries for this PR and #2327

96303af

Mention current rails version for #2327 ruby2.6/rails4.2 monkeypatch

c77fdda

wasifhossain force-pushed the fields_support_in_all_adapters branch from d796a0e to c77fdda Compare April 23, 2019 09:18

wasifhossain merged commit 3419a01 into 0-10-stable Apr 23, 2019

wasifhossain mentioned this pull request Apr 23, 2019

Fieldset support in Attributes/JSON adapters #2100

Closed

wasifhossain deleted the fields_support_in_all_adapters branch April 23, 2019 10:01

jpawlyn mentioned this pull request May 1, 2019

Prevent undefined method for Array:Class error #2329

Closed

wasifhossain mentioned this pull request May 2, 2019

Stick to bundler version < 2 on AppVeyor #2321

Merged

sikachu mentioned this pull request Jun 3, 2019

Fix regression introduced in #2223 when a serializer defines an attribute method name root #2336

Closed

This was referenced Jul 17, 2019

Lookup fieldset using either string or symbol #2344

Merged

Versioning support #2343

Closed

bmelton-mdsol mentioned this pull request Aug 14, 2019

root key for :attributes adapter is pointless #2350

Open

dmann added a commit to dmann/duke-data-service that referenced this pull request Sep 19, 2019

Fix "...undefined method `model_name' for..." error.

c61e522

AMS expects serialized models to behave like an ActiveModel::Model now: rails-api/active_model_serializers#2223

dmann added a commit to dmann/duke-data-service that referenced this pull request Sep 19, 2019

Fix "undefined method `singularize' for :results:Symbol" error

aa5c383

AMS requires that `root` be set to a string: rails-api/active_model_serializers#2223

dependabot bot mentioned this pull request Mar 18, 2021

Bump active_model_serializers from 0.10.10 to 0.10.12 YoheiZuho/mastodon#378

Closed

ryanwi mentioned this pull request Oct 7, 2021

undefined method `model_name' for Array:Class #2352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fieldset support in Attributes/JSON adapters #2223

Fieldset support in Attributes/JSON adapters #2223

bf4 commented Nov 20, 2017 •

edited by wasifhossain

Loading

bf4 Nov 20, 2017

agalloch Aug 16, 2018

bf4 Aug 28, 2018

bf4 Jul 29, 2019

wasifhossain Jul 29, 2019

wasifhossain Jul 29, 2019

bf4 Jul 30, 2019

wasifhossain Jul 30, 2019

bf4 Jul 30, 2019

wasifhossain Jul 30, 2019

bf4 Nov 20, 2017 •

edited

Loading

bf4 Nov 20, 2017 •

edited

Loading

bf4 Nov 20, 2017

bf4 Nov 20, 2017

wasifhossain Feb 11, 2019

cintamani left a comment

wasifhossain commented Feb 11, 2019

krzysiek1507 commented Feb 19, 2019

krzysiek1507 Feb 19, 2019

bf4 Feb 20, 2019

wasifhossain Feb 20, 2019

bf4 Feb 21, 2019

johan-smits commented Mar 25, 2019

wasifhossain commented Mar 25, 2019

johan-smits commented Mar 25, 2019

johan-smits commented Apr 4, 2019

wasifhossain commented Apr 4, 2019

johan-smits commented Apr 16, 2019

wasifhossain commented Apr 16, 2019

wasifhossain commented Apr 23, 2019

wasifhossain commented Apr 23, 2019

johan-smits commented Apr 23, 2019

flvrone commented Jul 29, 2019 •

edited

Loading

wasifhossain commented Jul 29, 2019

Fieldset support in Attributes/JSON adapters #2223

Fieldset support in Attributes/JSON adapters #2223

Conversation

bf4 commented Nov 20, 2017 • edited by wasifhossain Loading

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 Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

bf4 Nov 20, 2017 • edited Loading

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

cintamani left a comment

Choose a reason for hiding this comment

wasifhossain commented Feb 11, 2019

krzysiek1507 commented Feb 19, 2019

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

johan-smits commented Mar 25, 2019

wasifhossain commented Mar 25, 2019

johan-smits commented Mar 25, 2019

johan-smits commented Apr 4, 2019

wasifhossain commented Apr 4, 2019

johan-smits commented Apr 16, 2019

wasifhossain commented Apr 16, 2019

wasifhossain commented Apr 23, 2019

wasifhossain commented Apr 23, 2019

johan-smits commented Apr 23, 2019

flvrone commented Jul 29, 2019 • edited Loading

wasifhossain commented Jul 29, 2019

bf4 commented Nov 20, 2017 •

edited by wasifhossain

Loading

bf4 Nov 20, 2017 •

edited

Loading

bf4 Nov 20, 2017 •

edited

Loading

flvrone commented Jul 29, 2019 •

edited

Loading