Adds support for top-level links objects #1018

leandrocp · 2015-07-25T02:15:05Z

It's a generic object which can be used to declare a top-level links object as specified on jsonapi: http://jsonapi.org/format/#document-top-level

render json: @post, links: { self: "/posts/1" }

beauby · 2015-07-27T08:33:56Z

Great, would love to see it merged!

joaomdmoura · 2015-07-31T07:28:07Z

@leandrocp indeed, I like the idea of having links support, but the proposal does not guarantee it will be a links object as specified on the docs.
As the docs guarantee a structure:

A link MUST be represented as either:

a string containing the link's URL.

an object ("link object") which can contain the following members:

href: a string containing the link's URL.

meta: a meta object containing non-standard meta-information about the link

I think the implementation could also enforce the format. There is a lot of different approaches to achieve it, we could do something serializer-wise, so you would setup the serializer instead of the render call. I'll ty to give it some thought, what about you, how do you feel about it?

leandrocp · 2015-07-31T12:39:51Z

@joaomdmoura I'm ok with enforcing the link object. Remember that this PR only adds a link to the top level, although the spec includes a link object at four levels: root (with or without pagination links), resource (data), relationships and included.

{
  "links": {
  },
  "data": [{
    "relationships": {
      "author": {
        "links": {
        }
      },
    },
    "links": {
    }
  }],
  "included": [{
    "links": {
    }
  }
  }]
}

Do you think we should achieve all of them or just at root level for now ?

About the implementation, do you think it's good idea exposing a LinksObject class to the user ? Something like:

render json: @posts, links: LinksObject.new("http://example.com/posts")

{
  "links": {
    "self": "http://example.com/posts"
  },
  "data": []
}

Pagination data can also be included at this link object, which could be obtained from @posts if using kaminari (http://www.rubydoc.info/github/amatsuda/kaminari/Kaminari/PageScopeMethods), or could be informed by the user.

joaomdmoura · 2015-08-06T01:10:40Z

lib/active_model/serializer/adapter.rb

+        unless self.class == FlattenJson
+          include_meta(hash)
+          include_links(hash)
+        end


This means we are also adding links to the usual JsonAdapter. Any particular reasons for that?

You're right, we should support only JsonApi. I'll update it.

joaomdmoura · 2015-08-06T01:41:22Z

Hey @leandrocp,

So, I know there is another PR to implement pagination going on, I haven't checked it yet (might do it tonight). I would ask you to check it out, maybe see if we can make both work together. that would be awesome I know @bacarini (the creator of that PR) is looking forward to make it work.

I would do just the top level link for now, wen can implement the other ones in specific PRs.

Just read the docs for linked objects, and actually I don't think we should enforce the format anymore 😁 you was right.

We could re-build the links object to check on the include_links1 if it's aHash` that match the rules:

href: a string containing the link's URL.

meta: a meta object containing non-standard meta-information about the link

But I don't think it worths.
Check my comment on the code, and let's check the pagination PR that I mentioned above.
Great work! Keep it up! 😄

joaomdmoura · 2015-08-06T01:41:41Z

Almost forgot... You might need to rebase it 😁

leandrocp · 2015-08-06T13:08:07Z

@joaomdmoura rebase and squash done!

You linked PR #104, I think the correct one is #1040 😄
There's also PR #1041 and #1028 which add support for Link. I'm looking these PR.

joaomdmoura · 2015-08-07T17:43:20Z

.gitignore

@@ -19,3 +19,4 @@ test/version_tmp
 tmp
 *.swp
 .ruby-version
+tags


what is this tags file? 😁

@joaomdmoura you don't use exuberant ctags? such win
@leandrocp You can put tags in your global gitignore if this change doesn't make it in :)

@bf4 good tip, I had forgotten about global gitignore.
@joaomdmoura do you prefer to remove this line from .gitignore ? (that's ok for me)

No worries, I just checked it, seems that I should be using it as well. No blocking.

joaomdmoura · 2015-08-07T18:20:52Z

This looks awesome, I'm willing to merge it, just made a few more comments, can you check it please?
And yeap, I meant #1041 😁

leandrocp · 2015-08-11T10:50:38Z

Sorry for the late response. I'll update this PR today.

bf4 · 2015-08-11T14:20:36Z

test/action_controller/serialization_test.rb

+              Profile.new({ name: 'Name 1', description: 'Description 1', comments: 'Comments 1' })
+            ]
+
+            render json: @profiles, links: { self: "/profiles/1" }


Do we have any thoughts about relative vs. absolute links?

As discussed on jsonapi PR it's best to use absolute links. I already updated this line.

leandrocp · 2015-08-11T19:13:05Z

@joaomdmoura updated docs as suggested. Feel free to correct if needed.

bf4 · 2015-08-13T03:37:21Z

lib/active_model/serializer.rb

@@ -105,7 +105,7 @@ def self.root_name
      name.demodulize.underscore.sub(/_serializer$/, '') if name
    end

-    attr_accessor :object, :root, :meta, :meta_key, :scope
+    attr_accessor :object, :root, :meta, :meta_key, :links, :scope


@joaomdmoura @kurko serializers are for the resource -> attributes mapping and adapters are for building the response object, right?

to ensure the links is an adapter-specific rendering option, it should really be passed to the adapter by adding :links to the list of ADAPTER_OPTION_KEYS at the top of SerializableResource. That way, I think, we can keep the responsibilities of the the response in the adapter, and continue to limit the serialization of a resource to the serializer.

I wrote similarly in #1041 (comment) that I think meta should also be moved to the adapter, and is here mostly due to how the library has changed from 0.8 to 0.9 to 0.10 and the evolution of the json-api spec.

Your thoughts? I think you can make this change mostly by deleting the code in the serializers that the adapter uses and just have the links passed directly to the adapter.

@bf4 thanks for clarification 👍

The way I did was to conform with meta already present on the code. But as you said, and I agree, it should be moved to the adapter. I'll update the code as you suggested.

Another issue is that both PR (mine and #1041) are defining the attribute links of the @hash

See here and here

What's the best way to avoid conflict ? Just check if links is already present and append to it ?

render json: @posts, pagination: true

{ "data": [ { } ], "links": { "first": "http://example.com/articles?page[number]=1&page[size]=1", "prev": "http://example.com/articles?page[number]=2&page[size]=1", "next": "http://example.com/articles?page[number]=4&page[size]=1", "last": "http://example.com/articles?page[number]=13&page[size]=1" } }

render json: @posts, pagination: true, links: { "self": "http://example.com/articles", "last": "http://example.com/articles/last" }

{ "data": [ { } ], "links": { "self": "http://example.com/articles", "first": "http://example.com/articles?page[number]=1&page[size]=1", "prev": "http://example.com/articles?page[number]=2&page[size]=1", "next": "http://example.com/articles?page[number]=4&page[size]=1", "last": "http://example.com/articles/last" } }

See that I replaced last from links. Is that the expected output, or should I conserve attributes already defined ?

Hi @leandrocp, from my point of view the keys first, prev, next and lastshould be responsibility of pagination and no one should override it. Thus, the others keys won't be replaced because I am doing update. So, besides the keys above every key pass through controllers will continue.

Well, links is a top-level member of the json-api response object. Pagination is a usage of the links object. So, my thinking is that this PR adds 'links' and the pagination one uses it to add pagination data. Make sense?

💯

There is a method to add_links in #1041 , so a think we can use it, right?

Maybe it should behave like include_meta such that there is an include_links (which calls include_pagination)? I've written some more (please don't shoot me :) thinking about how to 'pass pagination' in through the 'links' adapter option in https://github.com/rails-api/active_model_serializers/pull/1041/files#r36998222

joaomdmoura · 2015-08-17T06:18:28Z

Hey @leandrocp please check out my last comment on 1041

bf4 · 2015-08-23T01:06:41Z

Now that the [pagination links pr #1041][https://github.com//pull/1041) has been merged, we may want to rebase and repurposed this one to handle the non-pagination link scenarios and how they may interact with pagination links.

leandrocp · 2015-08-25T12:33:10Z

Let´s get back to work! I'll rebase and move the code to json_api adapter

leandrocp · 2015-08-25T14:51:13Z

@joaomdmoura @bf4 please, could you take a look at the last commit ? It´s a first version of what I think is the right way to handle top level links. And please tell me if I'm on the right direction 😄

bf4 · 2015-08-25T23:21:11Z

@leandrocp First review looks nice. Might help to rebase off of master and then squash all this to one commit so we can just look at the diff

bf4 · 2015-08-25T23:22:10Z

docs/README.md

@@ -13,6 +13,7 @@ This is the documentation of AMS, it's focused on the **0.10.x version.**

 - [How to add root key](howto/add_root_key.md)
 - [How to add pagination links](howto/add_pagination_links.md)
+- [How to add top-level links](howto/add_top_level_links.md) (```JSON-API``` only)


IMHO, How to add pagination links should be like

- [How to add top-level links](howto/add_top_level_links.md) (```JSON-API``` only) - [How to add pagination links](howto/add_pagination_links.md)

or better yet, just one 'add_links.md' page since they're the same thing more or less

correction, add_top_level_links.md because individual objects may have links

leandrocp · 2015-09-16T20:59:48Z

@bf4 @joaomdmoura the whole idea of Links is to be reusable on all possible contexts of jsonapi. This is a draft to be discussed.

leandrocp · 2015-09-16T21:00:55Z

Yeah I was on a deadline here. Sorry for taking so long.

leandrocp · 2015-09-16T22:33:52Z

The internal design of the serializer has improved, so I need to update the code to make test pass.

beauby · 2015-09-16T22:54:11Z

@leandrocp I did the JsonApi adapter refactor so don't hesitate if you have any question!

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

@leandrocp are we still working on this? I can help you update it to master

bf4 · 2015-12-23T06:08:07Z

ref: #1235

leandrocp · 2015-12-23T12:11:29Z

@bf4 updated to master and integrated with #1247. Please check if it's good to merge.

kurko · 2016-01-25T21:54:40Z

@leandrocp needs rebase

kurko · 2016-01-25T21:55:48Z

LGTM

beauby · 2016-01-25T22:33:32Z

LGTM as well, @leandrocp if you wouldn't mind rebasing one last time (I know – sorry 👯)!

http://jsonapi.org/format/#document-top-level fix failing tests support for top-level links limited to jsonapi adapter Move docs from README to docs/ dir move links to json-api adapter & create Links class to hold links data

update according rubocop rules

leandrocp · 2016-02-03T12:42:32Z

Done!

ping @kurko @beauby

bf4 · 2016-02-03T15:18:46Z

CHANGELOG.md

@@ -65,6 +65,8 @@ Features:
    CollectionSerializer for clarity, add ActiveModelSerializers.config.collection_serializer (@bf4)
 - [#1295](https://github.com/rails-api/active_model_serializers/pull/1295) Add config `serializer_lookup_enabled` that,
  when disabled, requires serializers to explicitly specified. (@trek)
+- [#1247](https://github.com/rails-api/active_model_serializers/pull/1247) Add top-level links (@beauby)


Unfortunately, Looks like you'll have to amend the changelog to stick this at the top of features in master since rc4 is out

kurko · 2016-02-04T13:22:30Z

@bf4 has good points. Once those are addressed, this is a +2

Needs followup - #1018 (comment) - #1018 (comment) - #1018 (comment) - #1018 (comment)

bf4 · 2016-02-08T23:57:08Z

Merged in b55fc32 addressed follow-up issues in 5b953ff

joaomdmoura reviewed Aug 6, 2015
View reviewed changes

leandrocp force-pushed the add-top-level-links branch 2 times, most recently from f610f72 to 516d89b Compare August 6, 2015 12:58

leandrocp force-pushed the add-top-level-links branch from b6c382f to 1dcb3b3 Compare August 6, 2015 14:18

joaomdmoura reviewed Aug 7, 2015
View reviewed changes

bacarini mentioned this pull request Aug 10, 2015

Pagination Links #1041

Merged

bf4 reviewed Aug 11, 2015
View reviewed changes

bf4 reviewed Aug 13, 2015
View reviewed changes

leandrocp force-pushed the add-top-level-links branch from e3d1668 to bd9696f Compare August 25, 2015 12:51

bf4 reviewed Aug 25, 2015
View reviewed changes

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

leandrocp force-pushed the add-top-level-links branch 2 times, most recently from 437d75d to 6775bdd Compare December 23, 2015 12:09

beauby added the S: LGTM label Jan 25, 2016

bf4 mentioned this pull request Jan 29, 2016

Document JSON API implementation defs and progress in class #1482

Merged

leandrocp added 2 commits February 3, 2016 10:16

Adds support for top-level links to JsonApi adapter

1844c16

http://jsonapi.org/format/#document-top-level fix failing tests support for top-level links limited to jsonapi adapter Move docs from README to docs/ dir move links to json-api adapter & create Links class to hold links data

Update top-level link with PR rails-api#1247

37e4f1c

update according rubocop rules

leandrocp force-pushed the add-top-level-links branch from 6775bdd to 37e4f1c Compare February 3, 2016 12:16

improve doc as suggested by @bf4

b55fc32

bf4 reviewed Feb 3, 2016
View reviewed changes

bf4 merged commit b55fc32 into rails-api:master Feb 8, 2016

bf4 added a commit that referenced this pull request Feb 8, 2016

Merge branch 'leandrocp-add-top-level-links'

c59668e

Needs followup - #1018 (comment) - #1018 (comment) - #1018 (comment) - #1018 (comment)

bf4 added a commit that referenced this pull request Feb 8, 2016

Address concerns from #1018 commit c59668e

5b953ff

bf4 removed the S: Needs Submitter Response label Feb 8, 2016

Adds support for top-level links objects #1018

Adds support for top-level links objects #1018

Conversation

leandrocp commented Jul 25, 2015

beauby commented Jul 27, 2015

joaomdmoura commented Jul 31, 2015

leandrocp commented Jul 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaomdmoura commented Aug 6, 2015

joaomdmoura commented Aug 6, 2015

leandrocp commented Aug 6, 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

joaomdmoura commented Aug 7, 2015

leandrocp commented Aug 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandrocp commented Aug 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

joaomdmoura commented Aug 17, 2015

bf4 commented Aug 23, 2015

leandrocp commented Aug 25, 2015

leandrocp commented Aug 25, 2015

bf4 commented Aug 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandrocp commented Sep 16, 2015

leandrocp commented Sep 16, 2015

leandrocp commented Sep 16, 2015

beauby commented Sep 16, 2015

bf4 commented Dec 23, 2015

bf4 commented Dec 23, 2015

leandrocp commented Dec 23, 2015

kurko commented Jan 25, 2016

kurko commented Jan 25, 2016

beauby commented Jan 25, 2016

leandrocp commented Feb 3, 2016

Choose a reason for hiding this comment

kurko commented Feb 4, 2016

bf4 commented Feb 8, 2016