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

Include 'linked' member for json-api collections #692

Merged
merged 1 commit into from
Nov 4, 2014

Conversation

ggordon
Copy link
Contributor

@ggordon ggordon commented Oct 23, 2014

Include top-level 'linked' member when using JSON-API adapter. Needed to pass the hash to the elements in the collection, using the options hash worked, but I'm open to suggestions.

@kurko
Copy link
Member

kurko commented Oct 29, 2014

I agree with this but only if we pass include option to the serializer. It wouldn't make sense to include linked by default to every resource. Should be other way around imo, eg. include: [:comments] (with support for singular associations too, obviously)

@kurko
Copy link
Member

kurko commented Oct 29, 2014

cc @guilleiguaran

@ggordon
Copy link
Contributor Author

ggordon commented Oct 30, 2014

@kurko I'll add the include handling and update the PR

@kurko
Copy link
Member

kurko commented Oct 31, 2014

@ggordon I need this next week. Just out of curiosity, do you think you'll be able to add it anytime soon?

@ggordon
Copy link
Contributor Author

ggordon commented Oct 31, 2014

@kurko I plan on working on it tomorrow.

@ggordon ggordon force-pushed the linked_for_jsonapi_collection branch from 46b7580 to f014b3e Compare November 1, 2014 21:05
def render_resource_with_include
with_json_api_adapter do
setup_post
render json: @post, include: 'author,comments'
Copy link
Member

Choose a reason for hiding this comment

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

Initially, I believed it should be include: [:author, :comments] instead of strings because we need to whitelist these types, then we'd have to call .join(",") again.

However, we also need to support include=comments,comments.author (sublevels), which would invalidate the idea with symbols above. I don't know how we should express that, though. Perhaps include: [:author, [:comments, :author]]? Do you have any idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the whitelist the list of associations that are always included? Where would that be specified, in the controller or the serializer?

I made it a String since that's what would be coming in from params[:include], but if whitelist is specified in controller/action, then I'll need to do something else.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

After some thought, I'm ok with include: 'author,comments'.

@kurko
Copy link
Member

kurko commented Nov 2, 2014

This is awesome! Really good job here 👍

To proceed, I believe we should include support for sublevel associations first (e.g include=comments,comments.author). I wouldn't feel comfortable merging only half of the feature.

unless options[:embed] == :ids
@hash[:linked][name] ||= []
@hash[:linked][name] += serializers.map { |item| attributes_for_serializer(item, options) }
unless serializers.none? || @options[:embed] == :ids
Copy link
Member

Choose a reason for hiding this comment

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

I think that include should override embed: :ids and show the association in the linked hash. cc @guilleiguaran

Copy link
Member

Choose a reason for hiding this comment

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

Yes, when include is used any other rule for inclusion/exclusion of resources in linked section should be ignored

An endpoint MAY also support custom inclusion of linked resources based upon an include request parameter. This parameter should specify the path to one or more resources relative to the primary resource. If this parameter is used, ONLY the requested linked resources should be returned alongside the primary resource(s).

@ggordon
Copy link
Contributor Author

ggordon commented Nov 3, 2014

@kurko Does this seem right?
GET /posts/1?include=comments.author
According to the spec this should not automatically include comments. But the result seems to have broken links.

{
  "posts": {
    "title": "New Post",
    "body": "Body",
    "id": "1",
    "links": {
      "comments": [
        "1",
        "2"
      ],
      "author": "1"
    }
  },
  "linked": {
    "authors": [
      {
        "id": "2",
        "name": "Anonymous"
      }
    ]
  }
}

@kurko
Copy link
Member

kurko commented Nov 3, 2014

@ggordon yeah, this seems weird, but I'd stick with the spec. Personally, I don't think I'd ever call GET /posts/1?include=comments.author, but instead GET /posts/1?include=comments,comments.author. The spec should probably be reviewed.

The options passed to the render are partitioned into adapter options
and serializer options. 'include' and 'root' are sent to the adapter,
not sure what options would go directly to serializer, but leaving this
in until I understand that better.
@ggordon ggordon force-pushed the linked_for_jsonapi_collection branch from f014b3e to d5bae0c Compare November 3, 2014 22:16
kurko added a commit that referenced this pull request Nov 4, 2014
Include 'linked' member for json-api collections
@kurko kurko merged commit 95d1220 into rails-api:master Nov 4, 2014
@ggordon ggordon deleted the linked_for_jsonapi_collection branch November 5, 2014 08:55
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.

None yet

3 participants