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

JSONAPI resource relation urls #834

Closed
lsylvester opened this issue Mar 11, 2015 · 20 comments
Closed

JSONAPI resource relation urls #834

lsylvester opened this issue Mar 11, 2015 · 20 comments

Comments

@lsylvester
Copy link
Contributor

Currently, the jsonapi adapter adds resource relationships as link objects with ids and types. The jsonapi spec also allows the the resource relationship to be defined as a URL (see http://jsonapi.org/format/#document-structure-resource-relationships). For example:

{
  "type": "articles",
  "id": "1",
  "links": {
    "comments": "http://example.com/articles/1/comments"
  }
}

There is currently no way to do this using ActiveModel::Serializers as far as I can tell.

I propose that there should be a :url option for the assoications methods has_many and belongs_to that can be used to generate an resource url instead of linked objects, so that the above resource could be generated by the following serializer:

class ArticleSerializer
  has_many :comments, url: [:article, :comments]
end

I am not sure if this option is useful for the other serializers, so feedback on this would be appreciated.

If you are happy with this suggestion I can work on the implementation for this.

@joaomdmoura
Copy link
Member

It seems a good proposal but I'm not so sure how many of AMS users are using this pattern.
About the implementation itself it think there is no need for [:article, :comment].URL might be just a flag.

Ex.

class ArticleSerializer
  has_many :comments, url: true
end

But would be cool to hear more thoughts about it. btw keep up the good work! 😄

@lsylvester
Copy link
Contributor Author

So the url would be generated by something along the lines of url_for(object, association_name).

That works for me.

@lsylvester
Copy link
Contributor Author

The spec also allows this url to be embedded in the link object instead of replacing the link object with a url, so the original example would become:

{
  "type": "articles",
  "id": "1",
  "links": {
    "comments": {
       "related": "http://example.com/articles/1/comments"
     }
  }
}

In this format, it can optionally be combined with the currently used linkage.

I think that it would be better to implement support for this format than the original suggestion.

From a api point of view, I think that it would make sense to use the :embed option to describe what members to put into the link object, so the :embed options for the jsonapi adapter could be :linkage, :related_uri and :self_uri, or an array of any combination of these, with the default being :linkage

@krzkrzkrz
Copy link

Any updates on this? In the meantime, is there a workaround we can use to get the links feature going?

Btw, @joaomdmoura links is part of the jsonapi spec: http://jsonapi.org/format/#document-links

@joaomdmoura
Copy link
Member

@krzkrzkrz not yet, there is a pagination link PR going on but not this one yet.
But I still think that this is necessary and a valid proposal.
It's in out radar, I would be willing to review a PR, otherwise I'll work on it once I finish some new features I've being working on focused on deserialization (that will be also awesome to JSON API 😄)

@lsylvester
Copy link
Contributor Author

The jsonapi spec has been updated since the initial proposal.

The relationships can now look like:

{
  "links": {
    "self": "...",
    "related": "..."
  },
  "data": [],
  "meta": {}
}

I think that to represent this in a serializer, it could be like

has_many :comments, data: true, links: {self: true, related: true}

In order to include a query param in the link (for example for pagination), you could set the value of the link object to a hash, like:

has_many :comments, data: false, links: {first: {page: '1'}}

Or a custom url could be used by passing in an array or proc as the value in order to generate a different url to the default one for if the url structure doesn't match the default behaviour.

has_many :comments, data: false, links: {self: [:article, :relationships, :comments], related: -> object { custom_path(object) } }

I am happy to work on a PR once I know the desired format to define the links for relationships.

@joaomdmoura
Copy link
Member

Hey @lsylvester, tks for point out the change on relationship proposal.
But I'm not sure about the format proposals.
I wouldn't worry about pagination yet, and maybe we could make link a boolean

has_many :comments, links: true
  • data default value would be true
  • links default value would be false

How do you feel about it?

@SeyZ
Copy link
Contributor

SeyZ commented Jul 16, 2015

👍

@SeyZ
Copy link
Contributor

SeyZ commented Jul 22, 2015

Hey guys, I repeat the @krzkrzkrz question:

In the meantime, is there a workaround we can use to get the links feature going?

@joaomdmoura
Copy link
Member

so @SeyZ and @krzkrzkrz you could try to workaround this by creating a new adapter that inherits from json-api's one but it is almost implement the feature itself.
I can't thing on a easy workaround on this that would still follow the json-api conventions. 😢

@bterkuile
Copy link

I had the same problem. I created a virtual example for getting a list of posts and then clicking on it what should trigger fetching the comments. For ORMs where a hasMany relation call results in a query there will be N+1 queries performed just for returning the posts including the comments ids, where 1+1 = 2 queries should be enough. Based on the advice of @joaomdmoura I modified the adapter to solve this using a serializer given the following code:

class PostSerializer < ActiveModel::Serializer
  attributes :id, :title
  has_many :comments, url: ->(post){ "/api/1.0/posts/#{post.id}/comments" }
end

I had to combine the each_association method on the serializer with the add_resource_relationships method on the adapter, since the modification needs to be done in two places given the current codebase. The custom adapter implementation could look like (config/initializers/active_model_serializers.rb):

class CustomJsonApiAdapter < ActiveModel::Serializer::Adapter::JsonApi
  def add_resource_relationships(attrs, serializer, options = {})
    options[:add_included] = options.fetch(:add_included, true)
    serializer.class._associations.dup.each do |name, association_options| #do |name, association, opts|
      next unless object = serializer.object
      if association_options[:type] == :has_many
        association_value = association_options[:association_options][:url] ? [] : serializer.send(name)
        association_serializer_class = ActiveModel::Serializer.serializer_for(association_value, association_options)
      else
        association_value = serializer.send(name)
        association_serializer_class = ActiveModel::Serializer.serializer_for(association_value, association_options)
      end
      if association_serializer_class
        association_serializer = association_serializer_class.new(
          association_value,
          options.except(:serializer).merge(serializer.serializer_from_options(association_options))
        )
      elsif !association_value.nil? && !association_value.instance_of?(Object)
        association_options[:association_options][:virtual_value] = association_value
      end
      opts = association_options[:association_options]
      attrs[:relationships] ||= {}

      if association_serializer.respond_to?(:each)
        if opts[:url]
          add_related(attrs, name, serializer.object, opts[:url])
        else
          add_relationships(attrs, name, association_serializer)
        end
      else
        if opts[:virtual_value]
          add_relationship(attrs, name, nil, opts[:virtual_value])
        else
          add_relationship(attrs, name, association_serializer)
        end
      end
      if options[:add_included]
        Array(association_serializer).each do |association|
          add_included(name, association)
        end
      end
    end
  end

  def add_related(resource, name, record, related)
    related_url =  related.is_a?(Proc) ? related.call(record) : related
    resource[:relationships] ||= {}
    resource[:relationships][name] ||= { links: {} }
    resource[:relationships][name][:links][:related] = related_url
  end
end

ActiveModel::Serializer.config.adapter = CustomJsonApiAdapter

Note that the only change is the check on the url: ... option given as association_options[:association_options][:url] and opts[:url] and the extra add_related method, only supporting procs and strings. Since I do not recommend this as a good solution this comment is just supposed to be a hint to a possible outcome and to aid in the discussion.

@bterkuile
Copy link

Note that the above code applies to 0.10.0.rc2 and fails against the current master.

@joaomdmoura
Copy link
Member

Nice @bterkuile thank you for sharing indeed this is something that we want to have no 0.10.x

@joaomdmoura joaomdmoura self-assigned this Sep 11, 2015
@bterkuile
Copy link

Note that this solution of merging parts of the serializer and the adapter indicates that a proper implementation using the current adapter structure will be a challenge 😄 Good luck!

@djsegal
Copy link

djsegal commented Dec 24, 2015

Any updates on this?

@beauby
Copy link
Contributor

beauby commented Jan 4, 2016

@djsegal I'm getting some other stuff into the pipe currently but this is on my priority list. What I have in mind is a block-based syntax such as:

class UserSerializer < ActiveModel::Serializer
  has_many :posts do
    link :self, "//api.example.com/users/#{object.id}/relationships/posts"
  end
end

@bf4
Copy link
Member

bf4 commented Jan 4, 2016

Needs the url_helpers PR mentioned in #1282 (comment)

@beauby
Copy link
Contributor

beauby commented Jan 21, 2016

ref #1454

@bf4
Copy link
Member

bf4 commented Feb 23, 2016

ref: #1269

@beauby
Copy link
Contributor

beauby commented Apr 20, 2016

Closing, as I believe this feature has been merged. Feel free to reopen if needed.

@beauby beauby closed this as completed Apr 20, 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 a pull request may close this issue.

9 participants