Add supports for :links and :data options to has_many associations for jsonapi adapter #1282

tchak · 2015-10-20T17:36:04Z

This is a rebase from #1028

I hope @lsylvester don't mind :)

I also added more test including the one requested by @joaomdmoura with links + data. I also added support for passing in a hash of urls or proc in order to be able to provide custom links.

I hope we can move forward from here.

bf4 · 2015-10-20T17:39:37Z

lib/action_controller/serialization.rb

@@ -27,6 +27,7 @@ def get_serializer(resource, options = {})
      end
      serializable_resource = ActiveModel::SerializableResource.new(resource, options)
      if serializable_resource.serializer?
+        serializable_resource.url_helper = self


Thanks for this

👎 We will not include the controller. That would be a huge bug and complexity attractor

I think there's an issue for this, but I think the :context option should be renamed :request_context and be an object named RequestContext that has the methods original_url, query_params, and url_helper, though them's just my thoughts.. but the main this is that we will not pass around a controller instance

Sounds good. Would you be ok with this pr only including hash with proc links option? In the mean time it will allow to manually expose links and we can se for automatic link generator in the feature.

yeah, plus there's already a serialization_scope options, so maybe serialization_context would be good

bf4 · 2015-10-20T17:41:14Z

test/fixtures/poro.rb

@@ -1,6 +1,7 @@
 verbose = $VERBOSE
 $VERBOSE = nil
 class Model
+  extend ActiveModel::Naming


please remove

bf4 · 2015-10-20T17:48:05Z

Thinking aloud: This is a big (important) change. Maybe let's make this smaller and start with the simplest case?

tchak · 2015-10-20T17:51:01Z

@bf4 completely agree about starting smaller.

Supporting only

has_many :posts, links: { related: url }, data: false

and

has_many :posts, links: { related: -> (object, name) { url } }, data: false

sound's like small enough for you?

bf4 · 2015-10-20T17:55:24Z

or even just has_many :posts, links: { related: url }, data: false, has_many :posts, links: true etc.

tchak · 2015-10-20T20:27:23Z

@bf4 the problem with has_many :posts, links: { related: url }, data: false is that it is rather useless, as usually an association url will contain parent id...

beauby · 2015-10-20T21:02:29Z

Yup, @tchak is right. We have basically 3 options here:

support uri templates
support lambdas/procs
define links inside blocks (which is basically the same as the previous option)

tchak · 2015-10-20T21:16:01Z

@beauby my current plan is to simplify this PR to support only proc/lambda. Everything else can be implemented in terms of proc on top of it once we figured out context thing in another PR.

beauby · 2015-10-20T21:17:06Z

@tchak uri templates is a very interesting option as well, I'm not sure which is best

tchak · 2015-10-20T21:52:10Z

@beauby I completely agree. I am just thinking making it one step at a time.

bf4 · 2015-10-20T22:48:46Z

Link template + link_attributes (like mustache) is easy to understand and switch out impl

Using the template rfc makes building an info response object easier but isn't super high priority

B mobile phone

On Oct 20, 2015, at 4:52 PM, Paul Chavard notifications@github.com wrote:

@beauby I completely agree. I am just thinking making it one step at a time.

—
Reply to this email directly or view it on GitHub.

beauby · 2015-10-22T02:21:24Z

We have to be careful when passing data: false not to allow the corresponding associated resource's association to be included via the include option in order not to break the full-linkage property.

tchak · 2015-10-22T10:06:58Z

@beauby Right! I will look in to it.

tchak · 2015-10-22T20:23:31Z

@beauby I added a bunch of tests to test different combinations of data and include options

tchak · 2015-10-22T20:25:02Z

@bf4 @beauby I want to add uri template but I feel like there is already a lot of stuff in this PR. Would prefer to see it merged and then I will work on a next PR with uri template stuff.

bf4 · 2015-10-22T21:02:30Z

lib/active_model/serializer/adapter/json_api.rb

+        def relationship_links_for(name, owner, options = {})
+          options[:links].each_with_object({}) do |(key, link), hash|
+            if link.is_a? Proc
+              hash[key] = link.call(owner, name)


should be respond_to?(:call)

here and elsewhere

bf4 · 2015-10-22T21:03:26Z

lib/active_model/serializer/adapter/json_api.rb

+            hash[association.key] = {}
+
+            if association.options.fetch(:data, true)
+              hash[association.key][:data] = relationship_data_for(association.serializer, association.options)


oh, interesting, you're re-using the :data key

bf4 · 2015-10-22T21:14:39Z

@tchak Looks really good, 💯!

Two more things:

needs some examples of how to use either in code comments or in the /docs folder. Explain what data and linksare, how they're used, and that in the absence of the jsonapi adapter, they do nothing.
the proc usage makes me really nervous. It seems really inflexible and not very intent-revealing. Would you mind trying with a (roll your own ok) uri template to see how that feels? I think it'll be a lot more user and dev friendly.

tchak · 2015-10-22T21:37:30Z

@bf4 I was planing on implementing uri template in termes of default proc once this was merged. I think having proc as a backup "power-user" option is good. If you prefer I can add uri template directly to this PR. I am just not sure about all the "variables" I should interpolate with uri template. Obvious one is parent_id. I was actually thinking of making it "named", I mean tag_id in case of Tag serializer with has_many :posts, links: { related: "http://host/tags/{tag_id}/posts" }. Not sure if anything else actually needs to be dynamic.

richmolj · 2015-11-10T15:15:48Z

I actually really like the procs since I can imagine scenarios the url should be dynamic. Something like:

has_many :posts, proc { |object, name|
  if v2_feature_toggled_on_for_current_user?
    "http://test.host/apt/v2/posts"
  else
    "http://test.host/apt/v1/posts"
  end
}

bf4 · 2015-12-23T06:02:18Z

@tchak are we still working on this?

tchak · 2015-12-23T10:49:37Z

@bf4 I will have some time to rebase and cleanup this weekend

bf4 · 2015-12-23T14:02:55Z

Awesome!

B mobile phone

On Dec 23, 2015, at 4:49 AM, Paul Chavard notifications@github.com wrote:

@bf4 I will have some time to rebase and cleanup this weekend

—
Reply to this email directly or view it on GitHub.

djsegal · 2015-12-24T02:44:14Z

Is this supposed to resolve: #834?

djsegal · 2015-12-25T00:28:12Z

While using this, I noticed that I didn't have access to _url helpers, so I added the following line to the serializer, which allows you to do routes.blank_url:

def self.routes
  Rails.application.routes.url_helpers
end

to reduce the repetitiveness of this task,

i added a base class ApplicationSerializer < ActiveModel::Serializer serializer file
and overwrote the serializer template with my own at lib/templates/rails/serializer/serializer.rb.erb:

<% module_namespacing do -%>
class <%= class_name %>Serializer < ApplicationSerializer
  attributes <%= attributes_names.map(&:inspect).join(", ") %>
<% association_names.each do |attribute| -%>
  has_one :<%= attribute %>
<% end -%>
end
<% end -%>

bf4 · 2015-12-25T03:14:41Z

@djsegal The url handling is going to be in the serialization_context #1313, but just hasn't been finished. Would you like to give it a shot?

beauby · 2016-01-21T03:00:09Z

ref #1454

remear · 2016-03-17T16:24:30Z

@rails-api/ams Is this still relevant?

groyoh · 2016-03-30T22:02:24Z

@remear no, it's closed by #1454.

tchak mentioned this pull request Oct 20, 2015

Add supports for :links and :data options to has_many associations for jsonapi adapter #1028

Closed

bf4 reviewed Oct 20, 2015
View reviewed changes

bf4 added the S: Hold label Oct 20, 2015

bf4 reviewed Oct 20, 2015
View reviewed changes

test/fixtures/poro.rb

@@ -1,6 +1,7 @@

verbose = $VERBOSE

$VERBOSE = nil

class Model

extend ActiveModel::Naming

Copy link

Member

bf4 Oct 20, 2015

Choose a reason for hiding this comment

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

please remove

tchak force-pushed the jsonapi-link-v-data branch from b7adbb8 to 92dd0ed Compare October 20, 2015 21:50

tchak force-pushed the jsonapi-link-v-data branch 2 times, most recently from 31cfd12 to 5f81788 Compare October 20, 2015 22:09

tchak mentioned this pull request Oct 21, 2015

SerializationContext: request url info, content type, query params, api version, namespace, etc #1269

Closed

7 tasks

tchak force-pushed the jsonapi-link-v-data branch 3 times, most recently from 4ffde66 to f26bde2 Compare October 22, 2015 20:20

bf4 reviewed Oct 22, 2015
View reviewed changes

bf4 added C: Feature V: 0.10.x C: JSON API S: Changelog Missing labels Oct 22, 2015

bf4 removed the S: Hold label Oct 22, 2015

add relationship level links and meta options

2d0e417

tchak force-pushed the jsonapi-link-v-data branch from f26bde2 to 2d0e417 Compare October 22, 2015 21:52

bf4 mentioned this pull request Oct 26, 2015

Mapping JSON API spec / schema to AMS [ci skip] #1301

Merged

2 tasks

richmolj mentioned this pull request Nov 13, 2015

Avoid DB hits for JSONAPI relationships #1325

Closed

groyoh mentioned this pull request Dec 3, 2015

Add inline syntax for attributes and associations #1356

Merged

11 tasks

bf4 added the S: Needs Submitter Response label Dec 23, 2015

bf4 mentioned this pull request Jan 4, 2016

JSONAPI resource relation urls #834

Closed

bf4 mentioned this pull request Jan 29, 2016

Document JSON API implementation defs and progress in class #1482

Merged

groyoh closed this Mar 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add supports for :links and :data options to has_many associations for jsonapi adapter #1282

Add supports for :links and :data options to has_many associations for jsonapi adapter #1282

tchak commented Oct 20, 2015

bf4 Oct 20, 2015

tchak Oct 20, 2015

bf4 Oct 20, 2015

bf4 Oct 20, 2015

bf4 Oct 20, 2015

bf4 commented Oct 20, 2015

tchak commented Oct 20, 2015

bf4 commented Oct 20, 2015

tchak commented Oct 20, 2015

beauby commented Oct 20, 2015

tchak commented Oct 20, 2015

beauby commented Oct 20, 2015

tchak commented Oct 20, 2015

bf4 commented Oct 20, 2015

beauby commented Oct 22, 2015

tchak commented Oct 22, 2015

tchak commented Oct 22, 2015

tchak commented Oct 22, 2015

bf4 Oct 22, 2015

bf4 Oct 22, 2015

bf4 Oct 22, 2015

bf4 commented Oct 22, 2015

tchak commented Oct 22, 2015

richmolj commented Nov 10, 2015

bf4 commented Dec 23, 2015

tchak commented Dec 23, 2015

bf4 commented Dec 23, 2015

djsegal commented Dec 24, 2015

djsegal commented Dec 25, 2015

bf4 commented Dec 25, 2015

beauby commented Jan 21, 2016

remear commented Mar 17, 2016

groyoh commented Mar 30, 2016

Add supports for :links and :data options to has_many associations for jsonapi adapter #1282

Add supports for :links and :data options to has_many associations for jsonapi adapter #1282

Conversation

tchak 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

bf4 commented Oct 20, 2015

tchak commented Oct 20, 2015

bf4 commented Oct 20, 2015

tchak commented Oct 20, 2015

beauby commented Oct 20, 2015

tchak commented Oct 20, 2015

beauby commented Oct 20, 2015

tchak commented Oct 20, 2015

bf4 commented Oct 20, 2015

beauby commented Oct 22, 2015

tchak commented Oct 22, 2015

tchak commented Oct 22, 2015

tchak commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Oct 22, 2015

tchak commented Oct 22, 2015

richmolj commented Nov 10, 2015

bf4 commented Dec 23, 2015

tchak commented Dec 23, 2015

bf4 commented Dec 23, 2015

djsegal commented Dec 24, 2015

djsegal commented Dec 25, 2015

bf4 commented Dec 25, 2015

beauby commented Jan 21, 2016

remear commented Mar 17, 2016

groyoh commented Mar 30, 2016