Add regression test for fields in JSON/Attributes adapter #1285

vasilakisfil · 2015-10-20T18:36:30Z

Regression test showing how fields fails when there are associations

beauby · 2015-10-20T18:37:40Z

test/adapter/json/collection_test.rb

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


The option should not be passed to serializable_hash but to the adapter's constructor.

I can fix it. But I should inform you that fields are working out of the box because there is a bug here: https://github.com/rails-api/active_model_serializers/blob/master/lib/action_controller/serialization.rb#L50-L51

and not because the adapter's constructor passes the fields option to the serializer. (the adapter's options are in instance_options whereas the serializer options are in options all Attributes methods)

Actually, you should replace those 3 lines with:

actual = ActiveModel::SerializableResource.new([@first_post, @second_post], adapter: :json, fields: [:id])

Yes I agree, but it's not working even in master, I will send a regression test now.

actual = ActiveModel::SerializableResource.new([@first_post, @second_post], adapter: :json, fields: [:id])

@vasilakisfil plz amend per @beauby

# maybe some setup to explain actual = ActiveModel::SerializableResource.new([@first_post, @second_post], adapter: :json, fields: [:id]).as_json expected = { posts: [{id: 1}, {id: 2}] } assert_equal(actual, expected)

Also, I thought fields was JSON API specific. I'm not against adding some of the JSON API semantics to the JSON adapter, though

@bf4 Well currently the Json adapter does support a lesser version of the fields option (namely it allows to specify the attributes but just for the primary data). Support is broken though.

beauby · 2015-10-20T18:45:10Z

So, for the record:

  --- expected
  +++ actual
  @@ -1 +1 @@
  -{:posts=>[{:id=>1}, {:id=>2}]}
  +{:posts=>[{:id=>1, :comments=>[], :blog=>{:id=>999}, :author=>{:id=>1}}, {:id=>2, :comments=>[], :blog=>{:id=>999}, :author=>{:id=>1}}]}

beauby · 2015-10-20T18:46:22Z

It is expected that the (first-level) associations be included, as the fields option deals only with attributes. What's possibly not expected is that those associations only get their id serialized, and no other attribute.

beauby · 2015-10-20T18:50:57Z

So there are two issues here:

currently the Attributes/Json adapter only support specifying fields for the primary resource (on paper at least). We could discuss about making it work with an IncludeTree as well.
the fields option should not be passed to recursive calls to build the related resources (and this should happen here)

vasilakisfil · 2015-10-20T18:59:23Z

@beauby see #1286
Regarding fields for associations I am a big advocate of this since it's very little amount of work compared to what we gain.

beauby · 2015-10-20T19:14:41Z

@vasilakisfil Currently, associations are handled through the include option and attributes through the fields option. Mixing both will prevent people from just constraining the associations.

vasilakisfil · 2015-10-20T19:17:50Z

@beauby you can still filter associations based on includes. Fields on association is optional meaning that if you don't provide any fields for an association it won't apply and thus it will render all attributes of the associations (like it is now).

I want and I believe that you should be able to select in the JSON/Attributes adapters specific fields of an association. Do you have something else to suggest ?

beauby · 2015-10-20T19:19:38Z

I'm not convinced. Could you show me some use cases?

vasilakisfil · 2015-10-20T19:21:34Z

@beauby the same use cases that apply on filtering the attributes of the main association, the same apply on filtering the attributes of an association. Am I missing something ?

beauby · 2015-10-20T19:22:52Z

Sure sure, but In what situation would you really need to control that at the adapter level? You can already design your serializers in such a way that it covers most use cases I think. Do you have an (real-world) example where this feature would be needed?

vasilakisfil · 2015-10-20T19:27:49Z

I thought we had agreed that we need that in the constructor level in another thread #1141 (comment)

Permissions is one reason: I don't want to have multiple serializers for every role, neither do I want to have the authorization logic in the serializers.
Imitating Facebook's GraphQL is another reason. Let the client decide.

vasilakisfil · 2015-10-20T19:29:00Z

Plus, isn't that possible currently in the JSONAPI adapter?

beauby · 2015-10-20T19:30:04Z

I was talking about JsonApi in that thread. The reason I'm not convinced is because the Json/Attributes adapters are clearly not good choices when you start having complex needs, and I think it's better if users realize this sooner than later.

vasilakisfil · 2015-10-20T19:33:23Z

I disagree with that but OK. So should I continue working on that PR or should I start creating my own adapter ?

beauby · 2015-10-20T19:34:20Z

Currently in the JsonApi adapter we support only the "array syntax", that is {type1: [:attr1, :attr2, ...], type2: [...], type3: [...]}. There is no notion of nesting here, just for each type of resource which attributes we want to include.

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

@vasilakisfil This decision is not final, let's hear other people's thoughts (cc @joaomdmoura @bf4 @NullVoxPopuli)

NullVoxPopuli · 2015-10-20T20:12:03Z

I think fields shouldn't touch associations -- that (I believe) would cause a lot of confusion around the choice of the word fields and with existing functionality.

maybe the same thing could be accomplished by also specifying the include option?

render json: @objects, fields: [:id], include: ''

vasilakisfil · 2015-10-20T20:14:58Z

fields don't touch associations unless you have one under includes. I don't
think it's possible what you said.

I think the main issue here is: do we want filtering the attributes on
association level or allow only on the main resource?
On Oct 20, 2015 10:12 PM, "L. Preston Sego III" [email protected]
wrote:

I think fields shouldn't touch associations -- that (I believe) would
cause a lot of confusion around the choice of the word fields and with
existing functionality.

maybe the same thing could be accomplished by also specifying the include
option?

render json: @objects, fields: [:id], include: ''

—
Reply to this email directly or view it on GitHub
#1285 (comment)
.

bf4 · 2015-10-20T20:22:04Z

test/adapter/json/collection_test.rb

@@ -85,6 +85,20 @@ def test_include_option

            assert_equal(expected, actual)
          end
+
+          def test_fields_with_no_associations_include_option


(test name out of date)

NullVoxPopuli · 2015-10-20T20:27:45Z

ah, I see what you're saying.

I would be in favor of mimicing what json api is currently doing.

Add regression test for fields in JSON/Attributes adapter fix regression tests

bf4 · 2016-03-13T05:20:39Z

@NullVoxPopuli @vasilakisfil @beauby If I understand the JSON API spec correctly, fields act on resources in the response. includes just adds resources to the response. As it reads:

Given an 'article' of type 'articles" that has an 'author' of type 'people':

Request with fields parameter:

GET /articles?include=author&fields[articles]=title,body,author&fields[people]=name
Here we want articles objects to have fields title, body and author only and people objects to have name field only [emphasis mine -BF].
{
  "data": [{
    "type": "articles",
    "id": "1",
    "attributes": {
      "title": "JSON API paints my bikeshed!",
      "body": "The shortest article. Ever."
    },
    "relationships": {
      "author": {
        "data": {"id": "42", "type": "people"}
      }
    }
  }],
  "included": [
    {
      "type": "people",
      "id": "42",
      "attributes": {
        "name": "John"
      }
    }
  ]
}
Pay attention to the fact that you have to add a relationship name both in include and fields (since relationships are fields too) [Emphasis mine -BF]

remear · 2016-03-28T14:47:31Z

@rails-api/ams Are we still interested in this?

bf4 · 2016-03-29T02:28:36Z

@remear Yes

remear · 2016-09-16T00:42:04Z

Closing due to inactivity

beauby reviewed Oct 20, 2015
View reviewed changes

vasilakisfil mentioned this pull request Oct 20, 2015

Add optional fields support for associations #1284

Closed

bf4 reviewed Oct 20, 2015
View reviewed changes

Add regression test for fields in JSON/Attributes adapter

4c327ec

Add regression test for fields in JSON/Attributes adapter fix regression tests

bf4 added C: Regression V: 0.10.x labels Dec 9, 2015

remear modified the milestone: 0.10 Mar 17, 2016

remear added S: Needs Submitter Response 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regression test for fields in JSON/Attributes adapter #1285

Add regression test for fields in JSON/Attributes adapter #1285

vasilakisfil commented Oct 20, 2015

beauby Oct 20, 2015

vasilakisfil Oct 20, 2015

beauby Oct 20, 2015

vasilakisfil Oct 20, 2015

bf4 Oct 20, 2015

bf4 Oct 20, 2015

beauby Oct 20, 2015

beauby commented Oct 20, 2015

beauby commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

beauby commented Oct 20, 2015

NullVoxPopuli commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

bf4 Oct 20, 2015

NullVoxPopuli commented Oct 20, 2015

bf4 commented Mar 13, 2016

remear commented Mar 28, 2016

bf4 commented Mar 29, 2016

remear commented Sep 16, 2016

Add regression test for fields in JSON/Attributes adapter #1285

Add regression test for fields in JSON/Attributes adapter #1285

Conversation

vasilakisfil commented Oct 20, 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

beauby commented Oct 20, 2015

beauby commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

beauby commented Oct 20, 2015

beauby commented Oct 20, 2015

NullVoxPopuli commented Oct 20, 2015

vasilakisfil commented Oct 20, 2015

Choose a reason for hiding this comment

NullVoxPopuli commented Oct 20, 2015

bf4 commented Mar 13, 2016

remear commented Mar 28, 2016

bf4 commented Mar 29, 2016

remear commented Sep 16, 2016