Fix `fields` option to restrict relationships as well. #1297

beauby · 2015-10-24T18:27:52Z

The current implementation breaks the JSON API spec in the way it handles sparse fieldsets, since the definition of fields is:

A resource object's attributes and its relationships are collectively called its "fields".

NullVoxPopuli · 2015-10-25T01:40:34Z

lib/active_model/serializer/adapter/json_api.rb

@@ -174,7 +174,10 @@ def relationship_value_for(serializer, options = {})
        end

        def relationships_for(serializer)
-          serializer.associations.each_with_object({}) do |association, hash|
+          resource_type = resource_identifier_type_for(serializer)
+          requested_associations = fieldset.try(:fields_for, resource_type) || '*'


in what situation does a fieldset not have fields_for?

I guess, I'd like some docs here explaining why this is happening :-)
I'll be ok with merging once that's done. :-)

When fieldset is not initialized (which currently happens when the fields option is not provided).

Docs explaining why what is happening?

the situation you just described with fieldset.
It's important to have docs like this when someone new starts working on the json api adapter.

also, that's not an excuse to get out of writing docs.

Code comments are not docs in my opinion. Maybe we're not talking about the same thing?

perhaps.
I'm not meaning to write something that just anyone would look at. but a reminder of what is happening in the code, possibly the scenarios in which certain control flows are followed - so that even the author of the code can have an easier time re-familiarizing with the code 1+ years from when the code was written.
node of that is intended to be in the public documentation that would end up on ruby-doc.org or anything.

Then I still disagree. Code that needs explanations is just bad code in my opinion. If you think this bit is obscure, then I'd rather we work to make it less obscure than write a comment to explain it.

Um.. I don't understand the change from the code :)

beauby · 2015-10-25T19:04:25Z

lib/active_model/serializer/fieldset.rb

@@ -21,7 +21,7 @@ def fields_for(type)

      def parsed_fields
        if raw_fields.is_a?(Hash)
-          raw_fields.inject({}) { |h, (k, v)| h[k.to_sym] = v.map(&:to_sym); h }
+          raw_fields.each_with_object({}) { |(k, v), h| h[k.to_sym] = v.map(&:to_sym) }


This (isomorphic) change is not necessary for this PR, but it makes the code slightly cleaner.

Fix `fields` option to restrict relationships as well.

bf4 · 2015-10-26T04:47:56Z

@beauby @NullVoxPopuli can you help me understand why the order of the fieldset was changed? Also, for a bit I thought the :fields option was deleted due to the inlining of the conditional, fwiw.

-          fields = options.delete(:fields)
-          if fields
-            @fieldset = ActiveModel::Serializer::Fieldset.new(fields)
-          else
-            @fieldset = options[:fieldset]
-          end
+          @fieldset = options[:fieldset] || ActiveModel::Serializer::Fieldset.new(options.delete(:fields))

Is really the same as

-          fields = options.delete(:fields)
-          if fields
-            @fieldset = ActiveModel::Serializer::Fieldset.new(fields)
-          else
-            @fieldset = options[:fieldset]
-          end
+         @fieldset = options[:fieldset]
+         if @fieldset.nil?
+           fields = options.delete(:fields))
+           @fieldset = ActiveModel::Serializer::Fieldset.new(fields || {}) # where the fieldset initializer was modified as paraphrased here
+          end

bf4 · 2015-10-26T04:51:59Z

lib/active_model/serializer/adapter/json_api.rb

+          resource_type = resource_identifier_type_for(serializer)
+          requested_associations = fieldset.fields_for(resource_type) || '*'
+          include_tree = IncludeTree.from_include_args(requested_associations)
+          serializer.associations(include_tree).each_with_object({}) do |association, hash|


In words, what does this diff do?

- serializer.associations.each_with_object({}) do |association, hash| + resource_type = resource_identifier_type_for(serializer) + requested_associations = fieldset.fields_for(resource_type) || '*' + include_tree = IncludeTree.from_include_args(requested_associations) + serializer.associations(include_tree).each_with_object({}) do |association, hash|

It's supposed to somehow remove associations somehow?

It filters out associations not present in the fieldset of the resource (unless no fieldset has been provided for that resource).

maybe it's just the method and variable names. I can be pretty literal-minded and it looks like whereas before we would iterate over the associations, now we ask the serializer what it's type ask the fieldset for its fields, or use *, which guess means ALL, the include_tree is something that is passed to associations?

Exactly. The IncludeTree is probably not a perfect name. It is basically a structure that describes a labelled tree in such a way that you can query it using wildcards.

serializer.associations(include_tree) this filters associations

@beauby I guess this means that an explanation needs to be in code comments. :-)

bf4 · 2015-10-26T04:55:01Z

Would like to promote discussion of optimizing code to not make me think: #1297 (comment) 😄

beauby · 2015-10-26T04:55:43Z

@bf4 Any suggestion welcome.

beauby added S: Bug V: 0.10.x S: Needs Code Review C: JSON API labels Oct 24, 2015

beauby force-pushed the fix-jsonapi-fields branch from 6d5a9a0 to f6b2cdc Compare October 24, 2015 18:41

Fix fields option to restrict relationships as well.

13ef8fe

beauby force-pushed the fix-jsonapi-fields branch from f6b2cdc to 13ef8fe Compare October 24, 2015 19:28

NullVoxPopuli reviewed Oct 25, 2015
View reviewed changes

Initialize fieldset.

b5aecfd

beauby reviewed Oct 25, 2015
View reviewed changes

NullVoxPopuli added a commit that referenced this pull request Oct 26, 2015

Merge pull request #1297 from beauby/fix-jsonapi-fields

55ff9ac

Fix `fields` option to restrict relationships as well.

NullVoxPopuli merged commit 55ff9ac into rails-api:master Oct 26, 2015

beauby added S: Changelog Missing and removed S: Needs Code Review labels Oct 26, 2015

bf4 reviewed Oct 26, 2015
View reviewed changes

bf4 added the S: Needs Team Discussion label Oct 26, 2015

bf4 removed the S: Changelog Missing label Jan 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `fields` option to restrict relationships as well. #1297

Fix `fields` option to restrict relationships as well. #1297

beauby commented Oct 24, 2015

NullVoxPopuli Oct 25, 2015

NullVoxPopuli Oct 25, 2015

beauby Oct 25, 2015

beauby Oct 25, 2015

NullVoxPopuli Oct 25, 2015

NullVoxPopuli Oct 25, 2015

beauby Oct 25, 2015

NullVoxPopuli Oct 25, 2015

beauby Oct 26, 2015

bf4 Oct 26, 2015

beauby Oct 25, 2015

bf4 commented Oct 26, 2015

bf4 Oct 26, 2015

beauby Oct 26, 2015

bf4 Oct 26, 2015

beauby Oct 26, 2015

beauby Oct 26, 2015

NullVoxPopuli Oct 26, 2015

bf4 commented Oct 26, 2015

beauby commented Oct 26, 2015

Fix fields option to restrict relationships as well. #1297

Fix fields option to restrict relationships as well. #1297

Conversation

beauby commented Oct 24, 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

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 Oct 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Oct 26, 2015

beauby commented Oct 26, 2015

Fix `fields` option to restrict relationships as well. #1297

Fix `fields` option to restrict relationships as well. #1297