Nicer syntax for jsonapi `include` #1131

beauby · 2015-09-10T00:12:52Z

This PR ~~removes~~ keeps the old string format for includes (include: 'posts, posts.author, posts.comments, posts.comments.author, post.comments.likes, friends) and introduces a new format ~~instead~~ as well (include: [ :friends, posts: [:author, comments: [:author, :likes]]]), as discussed in #1128.

The "user-friendly" syntax is transformed into a big hash internally (like { friends: {}, posts: { author: {}, comments: { author: {}, likes: {} } } }), which is much easier to work with. The helper method that does the translation could be extracted and put in some kind of Utils module (WIP by @NullVoxPopuli, #1127).

The tests, which use a mix of the old string syntax and an hybrid "array of strings" syntax ~~obviously fail for now~~ are now fixed.

Notice that one test was actually changed, since the previous code allowed for including a resource nested 2 levels deep without including the resource in between, which breaks full linkage (see note http://jsonapi.org/format/#fetching-includes).

willcosgrove · 2015-09-10T02:53:21Z

Excited for this! Will this continue to work with the JSONAPI spec for an include string? It would still probably be good to be able to accept a url parameter for the include, and pass that directly to the serializer.

beauby · 2015-09-10T02:58:10Z

@willcosgrove Yeah, I just brought back the string format as well, as I had not thought about that use case when I decided to get rid of it.

estum · 2015-09-10T04:03:27Z

lib/active_model/serializer/adapter.rb

+          included.inject({}) { |a, e| a.merge(include_option_to_hash(e)) }
+        when String
+          included.delete(' ').split(',').inject({}) do |hash, path|
+            hash.deep_merge(path.split('.').reverse_each.inject({}) { |a, e| { e.to_sym => a } })


It allocates many extra hashes. Why not to use a deep_merge!?

Fixed. Thanks for spotting that!

joaomdmoura · 2015-09-11T15:26:28Z

Would be nice to have the commits with failing test squashed 😁 I'm triggering travis again.

joaomdmoura · 2015-09-11T15:31:58Z

test/action_controller/json_api/linked_test.rb

@@ -43,29 +43,29 @@ def render_resource_without_include

          def render_resource_with_include
            setup_post
-            render json: @post, include: 'author', adapter: :json_api
+            render json: @post, include: [:author], adapter: :json_api


These are the only tests involving action_controller + include would be nice to have some of them or new ones covering the string behavior in to make sure we con't break it in the future.

I reverted some to the string syntax.

beauby · 2015-09-11T17:40:38Z

All comments taken into account + squashed.

bf4 · 2015-09-11T18:09:23Z

lib/active_model/serializer/adapter/json_api.rb

-            collection.respond_to?(:total_pages) &&
-            collection.respond_to?(:size)
-        end
+  extend ActiveSupport::Autoload


The indentation of the whole file was wrong.

it's supposed to be so that the diffs are easy to read.. e.g. so we can #1138

Fair enough, will revert it.

bf4 · 2015-09-11T18:13:29Z

Overall, looks good, should have docs added, if it's not too much

beauby · 2015-09-11T18:42:42Z

lib/active_model/serializer/utils.rb

+    when Symbol
+      { included => {} }
+    when Hash
+      included.each_with_object({}) { |(key, value), hash| hash[key] = include_args_to_hash(value) }


@bf4: How about this?

this aligns well with https://github.com/rails-api/active_model_serializers/pull/1127/files#diff-2e4bf15928c11c2f4eff9285340b602d

comments would be nice though.

Have you tried seeing what happens if you somehow pass an invalid string as an include option?

It would pretty much do what it's supposed to. What do you have in mind?

Possibly just adding tests for if someone specified 'authr' instead of 'author' stuff like that.

strings can be a little difficult to catch errors in sometimes, due to silent failing.

Oh I see. Currently, the way it works, it should silently ignore any included relationship that was not defined on the serializer. Moreover, I believe there already are some tests on the adapter for trying to include a missing relationship.

Do you think there should be a logger output for when that happens? maybe not in this PR, but in another?

Yeah it would make sense. Also, there should be a logger output when attempting to serialize a related relationship for which there exists no serializer.

bacarini · 2015-09-13T20:41:38Z

docs/general/adapters.md

@@ -30,6 +30,8 @@ resources in the `"included"` member when the resource names are included in the
  render @posts, include: 'authors,comments'
 ```

+The format of the `include` option can be either a String composed of a comma-separated list of [http://jsonapi.org/format/#fetching-includes](relationship paths), an Array of Symbols and Hashes, or a mix of both.


@beauby I think this link is wrong, isn't?

I believe the right way is something like that:

[relationship paths](http://jsonapi.org/format/#fetching-includes)

Good catch, thanks @bacarini!

Extended format for JSONAPI `include` option

beauby force-pushed the jsonapi-include-tree branch 2 times, most recently from 68ac59c to f4b9e6e Compare September 10, 2015 00:58

NullVoxPopuli mentioned this pull request Sep 10, 2015

Support nested associations for Json and Attributes adapters + Refactor Attributes adapter #1127

Merged

beauby force-pushed the jsonapi-include-tree branch from f4b9e6e to 82d7fc4 Compare September 10, 2015 01:40

beauby mentioned this pull request Sep 10, 2015

JSON API nicer include syntax #1128

Closed

beauby force-pushed the jsonapi-include-tree branch from ca0085f to 4d2978e Compare September 10, 2015 02:34

beauby changed the title ~~[WIP] Nicer syntax for jsonapi include~~ Nicer syntax for jsonapi include Sep 10, 2015

estum reviewed Sep 10, 2015
View reviewed changes

joaomdmoura reviewed Sep 11, 2015
View reviewed changes

beauby force-pushed the jsonapi-include-tree branch 2 times, most recently from f4f74ac to 366012c Compare September 11, 2015 17:39

bf4 reviewed Sep 11, 2015
View reviewed changes

beauby force-pushed the jsonapi-include-tree branch 3 times, most recently from a4a3ba2 to 0603208 Compare September 11, 2015 18:41

beauby reviewed Sep 11, 2015
View reviewed changes

beauby force-pushed the jsonapi-include-tree branch 3 times, most recently from 72460e7 to 802898c Compare September 13, 2015 20:33

bacarini reviewed Sep 13, 2015
View reviewed changes

beauby force-pushed the jsonapi-include-tree branch from 802898c to a07792b Compare September 13, 2015 20:42

Extended format for JSONAPI include option.

ce7a839

beauby force-pushed the jsonapi-include-tree branch from a07792b to ce7a839 Compare September 13, 2015 20:46

beauby added a commit that referenced this pull request Sep 13, 2015

Merge pull request #1131 from beauby/jsonapi-include-tree

2789a57

Extended format for JSONAPI `include` option

beauby merged commit 2789a57 into rails-api:master Sep 13, 2015

joaomdmoura mentioned this pull request Oct 6, 2015

Full JSON-API support #1235

Open

11 tasks

bf4 mentioned this pull request Jan 29, 2016

Document JSON API implementation defs and progress in class #1482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nicer syntax for jsonapi `include` #1131

Nicer syntax for jsonapi `include` #1131

beauby commented Sep 10, 2015

willcosgrove commented Sep 10, 2015

beauby commented Sep 10, 2015

estum Sep 10, 2015

beauby Sep 10, 2015

joaomdmoura commented Sep 11, 2015

joaomdmoura Sep 11, 2015

beauby Sep 11, 2015

beauby commented Sep 11, 2015

bf4 Sep 11, 2015

beauby Sep 11, 2015

bf4 Sep 11, 2015

beauby Sep 11, 2015

bf4 commented Sep 11, 2015

beauby Sep 11, 2015

NullVoxPopuli Sep 11, 2015

beauby Sep 12, 2015

NullVoxPopuli Sep 12, 2015

beauby Sep 12, 2015

NullVoxPopuli Sep 12, 2015

beauby Sep 12, 2015

bacarini Sep 13, 2015

beauby Sep 13, 2015

Nicer syntax for jsonapi include #1131

Nicer syntax for jsonapi include #1131

Conversation

beauby commented Sep 10, 2015

willcosgrove commented Sep 10, 2015

beauby commented Sep 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaomdmoura commented Sep 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Sep 11, 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

bf4 commented Sep 11, 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

Nicer syntax for jsonapi `include` #1131

Nicer syntax for jsonapi `include` #1131