Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional fields in associations (priority on serializer) #1628

Closed
wants to merge 1 commit into from
Closed

Optional fields in associations (priority on serializer) #1628

wants to merge 1 commit into from

Conversation

vasilakisfil
Copy link
Contributor

Purpose

The purpose of this PR is to have a graphql-like support in Attributes/JSON adapters. With this PR, you can filter fields on nested resources on those adapters.

Currently, 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.

Also, a config option was also added (attributes_adapter_include_default) to denote the number of nested level is supported by the adapter. Default value is '*.*' which means 2 levels but this can change by the user.

Options on the serializer level have higher priority. So whatever you passed in the serializer constructor regarding associations, serializer's associations will render only id:

  class VideoSerializer
    attributes :id, :video_id

    belongs_to :user, serializer: UserSerializer, fields: [:id], includes: []
  end

Changes

A config option was also added (attributes_adapter_include_default) to denote the number of nested level is supported by the adapter. Default value is ('.') which means 2 levels but this can change by the user.

Caveats

Please note that you can pass an array to fields. If you want to filter a nested resource, then an element of the array must be a hash with key the name of the association and value an array with the fields that you would like to pass.

So for instance, to have only id in the primary resource and in the likes association you should do:

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

We need a helper function to convert url params to this structure

Unfortunately, url params do not support this kind of structure due to limitation in the rack parse_nested_query method (rack/rack#524).
Does it mean that this PR is useless ? No at all. Because you can manually exclude fields in the association level from the controller as well as in the serializer level.

And when rack issue is fixed (or someone monkey patches it) we have support for a graphql-like API.

Related GitHub issues

#1287
#1285
#1284
#1243
#1141

Additional helpful information

Also add default nested level in config for attributes/json adapters

wip

wip

wip

wip

Add default nested level in config for attributes/json adapters

Optional fields in associations

Also add default nested level in config for attributes/json adapters

wip

wip

wip

wip

Add default nested level in config for attributes/json adapters

Give priority on options on serializer
@bf4
Copy link
Member

bf4 commented Mar 29, 2016

For reference jsonapi fields specify the object #1285 (comment)

'GET /articles?include=author&fields[articles]=title,body,author&fields[people]=name'

def test_fields_option
serializer = ActiveModel::Serializer::CollectionSerializer.new([@first_post, @second_post])
adapter = ActiveModelSerializers::Adapter::Json.new(serializer)
actual = adapter.serializable_hash(fields: [:id])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these options should be passed into the adapter. most conventional way to do it is

actual = serializable([@first_post, @second_post], adapter: :json, fields: [:id]).serializable_hash

see #1643

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since fields is part of JSON API, we need to make sure fields behave properly there as well. (and the adapter already handles fields)

@bf4
Copy link
Member

bf4 commented Apr 1, 2016

@vasilakisfil The PR has good work in it. Do you plan on coming back to discuss? In particular, there are adapter concerns:

(I'm a little tired, so pardon if this looks jumbled in the morning 💤 )

I believe you've described the PR as

options result
fields: [:id] primary resource is restricted to id field. no associations affected
fields: [:id, user: [:name]] primary resource is restricted to id field, user association is restricted to name field. no other associations affected

I also thing this behavior is already true for the JSON API adapter, except that the JSON API only includes resources when specified

and

Default value is '.' which means 2 levels

This could only apply to the JSON and Attributes adapters. JSON API is by definitions no associations unless included, as if it were includes: []

Regarding fields and include

Since we're putting our effort behind supporting JSON API it's convenient to adopt its nomenclature where appropriate. In this case, JSON API uses include to specify which associations are returned, if and fields to restrict the primary resource and any included associations. e.g. 'GET /articles?include=author&fields[articles]=title,body,author&fields[people]=name' would get the articles with fields title,body,author and associated authors with fields name. No other associations would be returned.

One thing to bring up is to point out that fields is ultimately an adapter concern even if the adapter uses it to get attributes and associations from a serializer.

@bf4
Copy link
Member

bf4 commented Apr 1, 2016

Also, how would you say this pr relates to

@vasilakisfil
Copy link
Contributor Author

Hello @bf4,

I would like to give some love in those adapters. So I will try to help as much as I can.

The code is in sync with JSONAPI: it already checks includes first and then fields, meaning that if a resource is not included, fields definition on that resource are kind of pointless. The reason fields work without includes now is due to default include level (*.*). But if you remove that fields will not work. I will add tests in the weekend.

#1285 points that there is a bug in the fields (this PR fixes it)
#1287 (and #1286 points (in discussion) that fields are looked from the serializer options rather than the adapter options

About #1426 and default include level, we could split it in another feature I agree and I would prefer that to implemented/merged first before merging current PR.

I would work again this weekend to fix some things you mentioned but pretty much I don't have much work to do unless you would like to tell me more what you would like to see from that PR (the info you gave me were interesting but a bit vague as to which direction I should continue working :) )

One question: could option[:fields] be a string ?

@bf4
Copy link
Member

bf4 commented Apr 1, 2016

the info you gave me were interesting but a bit vague as to which direction

yeah, I know. Basically, I want to me confident that this PR isn't

  • duplicating existing behavior in ams
  • breaking existing behavior in ams
  • changing nesting behavior in unexpected ways or otherwise breaking changes

And sometimes I just don't have the brainpower to figure it out with out your help :)

@bf4
Copy link
Member

bf4 commented Apr 1, 2016

If we could split this PR into two, the nesting and everything else, then close any issues this one supersedes, that would be grewat

@remear
Copy link
Member

remear commented May 26, 2016

Any progress on this? Was this ever split into multiple PRs?

@remear
Copy link
Member

remear commented Sep 16, 2016

Closing due to inactivity

@remear remear closed this Sep 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants