Skip to content

Conversation

@beauby
Copy link
Member

@beauby beauby commented Sep 11, 2017

Closes #50.

}.freeze
}

DEFAULT_JSONAPI_FIELDS = ->() { params[:fields] }
Copy link
Contributor

@smaximov smaximov Sep 12, 2017

Choose a reason for hiding this comment

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

I'm afraid this won't work out of the box.

params[:fields] would return AC::Parameters, which is not compatible with Hash in Rails 5. So we can use something like params[:fields]&.to_unsafe_hash instead. But this won't work either because fields for every resource specified still need to be parsed (for instance, a request to /api/posts?fields[posts]=title,summary&fields[comments]=content,rating will result in a :fields hash { "posts" => "title,summary", "comments" => "content,rating" }, which will cause an error if passed to the renderer).

So I believe it will be safe to disable parsing/processing of JSONAPI fields by default (set it to nil or an empty hash) and provide a parser, just like you did with jsonapi_include.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the PR according to your feedback. At the moment I simply provide an example in the initializer but I agree it might make sense in the future to:

  1. provide Hash[fields_param.map { |k, v| [k.to_sym, v.split(',').map!(&:to_sym)] }] as a method somewhere,
  2. expose some mechanism to parse the fields param by default without necessarily using it (i.e. make it into a hash and turn it back to an AC::Parameters instance that the user can directly validate).

Copy link
Contributor

@smaximov smaximov Sep 12, 2017

Choose a reason for hiding this comment

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

Cool, I like it. I can test this PR (the fields part) in an hour on my website.

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested jsonapi_fields configuration, works without any issue.

@beauby beauby force-pushed the default-includes-fields branch 2 times, most recently from 5eb9ae0 to 6e66a0a Compare September 12, 2017 09:54
@beauby beauby force-pushed the default-includes-fields branch from 6e66a0a to 6b323ae Compare September 12, 2017 09:59
@beauby beauby changed the title Add configurations/hooks for fields and include. Add configurations/hooks for fields, include, and links. Sep 12, 2017
@beauby beauby force-pushed the default-includes-fields branch from 910d321 to 5ee0c4b Compare September 12, 2017 11:38
}.freeze
}

DEFAULT_JSONAPI_FIELDS = ->() { params[:fields] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested jsonapi_fields configuration, works without any issue.

@beauby beauby merged commit 0da3e8d into master Sep 12, 2017
@beauby beauby deleted the default-includes-fields branch September 12, 2017 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants