Add tests for fields option demonstrating usage on both attributes and relationships

[NullVoxPopuli]

Purpose

Adds tests demonstrating proper syntax / format for using fields on both attributes and relationships.

Wierdness

The root key needs to be specified in order to whitelist relationships, which feels weird.

[mention-bot]

@NullVoxPopuli, thanks for your PR! By analyzing the annotation information on this pull request, we identified @arenoir, @beauby and @dubadub to be potential reviewers

[NullVoxPopuli]

@NullVoxPopuli, thanks for your PR! By analyzing the annotation information on this pull request, we identified @arenoir, @beauby and @dubadub to be potential reviewers

waaaaaaaaaaaaaa O.O

neat

[NullVoxPopuli]

Not sure if this would open us up to too many issues, but it works with all the tests.

[bf4]

I was thinking the change should be in the adapter that passes in fields: {} instead of fields: nil when options[:fields].nil?

see

active_model_serializers/lib/active_model_serializers/adapter/json_api.rb

Lines 300 to 306 in 91b37ce

	requested_fields = fieldset && fieldset.fields_for(resource_object[:type])
	attributes = attributes_for(serializer, requested_fields)
	resource_object[:attributes] = attributes if attributes.any?
	resource_object
	end
	requested_associations = fieldset.fields_for(resource_object[:type]) || '*'

[bf4]

or just that fields_for should return {}, n/m

active_model_serializers/lib/active_model/serializer/fieldset.rb

Lines 12 to 14 in 91b37ce

	def fields_for(type)
	fields[type.singularize.to_sym] || fields[type.pluralize.to_sym]
	end

      def fields_for(type)
-        fields[type.singularize.to_sym] || fields[type.pluralize.to_sym]
+        fields[type.singularize.to_sym] || fields[type.pluralize.to_sym] || {}
      end

[NullVoxPopuli]

well, the issue IS with parsed_fields because it removes the way you have to specify relationships.

root_type: [:relationship_type]

[NullVoxPopuli]

alternatively, I could maybe change how the relationship's call to fields_for is handled. maybe it would be a bit more intuitive that way.

[NullVoxPopuli]

example:

# the root type *should* be assumed (it's currently not with JSON API
fields: :author
fields :author, :comments
fields :access_token # from the docs

docs: https://github.com/rails-api/active_model_serializers/blob/master/docs/general/rendering.md#fields
and: https://github.com/rails-api/active_model_serializers/blob/master/docs/general/fields.md

There is an inconsistency between APIs in the different adapters.

JSON API requires the root key in the fields specification, whereas the other adapters do not require the root.

So, maybe this PR should focus on unifying this between all adapters.

• @bf4, @beauby

[bf4]

btw, I've been thinking we should just name everything key. attribute key, relationship key, object key, collection key, and forget about json_key and have key make use of type and root options when available.

[NullVoxPopuli]

I think that would be a great topic for a PR this week. Hmm.

[bf4]

Related #1857

While working on that I've discovered an architectural problem that will require some careful unraveling

In brief

serializer.attributes(fields) builds a hash from calling value (aka read_attribute_for_serialization ) on the Attribute objects with matching names

serializer.associations(include_directive) iterates over all the Reflection objects and rejects excluded and yields an Association object built jit by the Reflection

The Association is used by calling association.serializer.serializable_hash

As currently implemented, the reflection is evaluated (method is called) when the Association is built. This prevents lazy evaluation

I have a hack in that pr to delay evaluation until the serializer asks for it, which works when the serializer calls serializable_hash

However, the JSONAPI adapter iterates the attributes and associations directly and passes the association into a new Relationship which promptly evaluates the association before determining whether to include it. Complicating this is that there is an assumption in the code that we mist evaluate the association so that we can infer the serializer class so that we can instantiate it, but it is already too late.

The fundamental way that a serializer is told which fields and associations to evaluate is inconsistent and far to eager. The already-evaluated-association object permeates the code.... Should be much simpler

Also would be good to make Fields and similar apis consistent

[NullVoxPopuli]

Also would be good to make Fields and similar apis consistent

I have a hack that brings fields functionality to the Attributes adapter (I use this for CSV generation)

module ActiveModelSerializers
  module Adapter
    class Attributes < Base
      def serializable_hash(options = nil)
        options = serialization_options(options)
        options[:fields] ||= instance_options[:fields]
        hash = serializer.serializable_hash(instance_options, options, self)
        fields = JSONAPI::IncludeDirective.new(options[:fields]).to_hash
        apply_fields_whitelist(hash, fields)
      end

      # this is a little backwards, but it's needed until AMS has a more unified interface
      def apply_fields_whitelist(hash, fields)
        return hash.map{ |e| apply_fields_whitelist(e, fields) } if hash.is_a?(Array)

        result = {}
        fields_keys = fields.keys
        hash.each do |k, v|
          next unless fields_keys.include?(k)
          if v.is_a?(Hash)
            result[k] = apply_fields_whitelist(v, fields[k])
          else
            result[k] = v
          end
        end

        result
      end
    end
  end
end

this sort of thing def needs to be unified.
As for unification timeline, I think I'll start this weekend, if not Monday, but I'd like to see #1857 merged, as well as this one, and the test refactor merged. :-)

[bf4]

Well, 0.10 is. 0.8 and 0.9 are still pretty clean. So sad.

[cgmckeever]

Is it at least glutten free? 🍝 🍷

[NullVoxPopuli]

0.10 will become clean soon enough. :-)

The important thing is we have enough tests to accurately keep functionality as we refactor

[bf4]

If we can return to a clean adapterless cache unaware serializer that can take in options to serialize its attributes and association... and I'd love to use just functions here, like in knuckles, we could rock this

[NullVoxPopuli]

and I'd love to use just functions here, like in knuckles

what do you mean by these two statements?
for first, do you mean more FP pure functions?

[NullVoxPopuli]

In other news, I learned that multi-word types MUST use the same key-casing as how the key transform is configured, which I didn't think was very apparent, as there is no logging or anything. I'll add that to the docs.

[NullVoxPopuli]

any objections to this?

[bf4]

@NullVoxPopuli The PR description make it sound like it's adding a feature, but the actual is that it is adding tests. I've added a commit undoing the change in the FieldSet since is not a functional change, as far as I can tell.

While making that change, some of the discussion has gotten lost in the diff.

• where should the fields default value be set
• discussion of differences in fields usage by attributes/associations and general ams architecture

[bf4]

not a fix, I don't think just tests? #1839 (comment)

[NullVoxPopuli]

yeah, just tests

[NullVoxPopuli]

PR description updated

[NullVoxPopuli]

@bf4 changelog entry moved, and re-worded.