-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add Rails url_helpers #1550
Add Rails url_helpers #1550
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,7 +103,10 @@ PR please :) | |
|
||
#### links | ||
|
||
##### How to add top-level links | ||
If you wish to use Rails url helpers for link generation, e.g., `link(:resources) { resources_url }`, ensure your application sets | ||
`Rails.application.routes.default_url_options`. | ||
|
||
##### Top-level | ||
|
||
JsonApi supports a [links object](http://jsonapi.org/format/#document-links) to be specified at top-level, that you can specify in the `render`: | ||
|
||
|
@@ -144,6 +147,33 @@ That's the result: | |
|
||
This feature is specific to JsonApi, so you have to use the use the [JsonApi Adapter](adapters.md#jsonapi) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. jsonapi only true for resource-level, also, right? Might it be helpful to just refer to ok for followup pr |
||
|
||
|
||
##### Resource-level | ||
|
||
In your serializer, define each link in one of the following methods: | ||
|
||
As a static string | ||
|
||
```ruby | ||
link :link_name, 'https://example.com/resource' | ||
``` | ||
|
||
As a block to be evaluated. When using Rails, URL helpers are available. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
which might go in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since having revised this PR to use
Anything else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests w grape adapter? B mobile phone
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figured when Grape is used with Rails, the Railtie would load and SerializationContext would be available and include the url helpers. When Grape is used without Rails, the railtie would not load, SerializationContext would not be available, and no url helpers would be available. Are you wanting more of an explicit test? It doesn't look like links are even tested under Grape. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a grape impl, but let's not delay this pr B mobile phone
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, sorry for a little yagni i introduced B mobile phone
|
||
Ensure your application sets `Rails.application.routes.default_url_options`. | ||
|
||
```ruby | ||
link :link_name_ do | ||
"https://example.com/resource/#{object.id}" | ||
end | ||
|
||
link(:link_name) { "https://example.com/resource/#{object.id}" } | ||
|
||
link(:link_name) { resource_url(object) } | ||
|
||
link(:link_name) { url_for(controller: 'controller_name', action: 'index', only_path: false) } | ||
|
||
``` | ||
|
||
### serializer_opts | ||
|
||
#### include | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,13 +135,15 @@ With the `:json_api` adapter, the previous serializers would be rendered as: | |
|
||
#### ::link | ||
|
||
e.g. | ||
|
||
```ruby | ||
link :other, 'https://example.com/resource' | ||
link :self do | ||
href "https://example.com/link_author/#{object.id}" | ||
href "https://example.com/link_author/#{object.id}" | ||
end | ||
link :author { link_author_url(object) } | ||
link :link_authors { link_authors_url } | ||
link :other, 'https://example.com/resource' | ||
link :posts { link_author_posts_url(object) } | ||
``` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. somewhere we need to mention that the generic interface only supports ok for followup pr |
||
``` | ||
|
||
#### #object | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,24 @@ | ||
module ActiveModelSerializers | ||
class SerializationContext | ||
class << self | ||
attr_writer :url_helpers, :default_url_options | ||
end | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Self.url_helpers = Module.new? |
||
attr_reader :request_url, :query_parameters | ||
|
||
def initialize(request) | ||
def initialize(request, options = {}) | ||
@request_url = request.original_url[/\A[^?]+/] | ||
@query_parameters = request.query_parameters | ||
@url_helpers = options.delete(:url_helpers) || self.class.url_helpers | ||
@default_url_options = options.delete(:default_url_options) || self.class.default_url_options | ||
end | ||
|
||
def self.url_helpers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably good to have a comment here about the default value ok for followup pr |
||
@url_helpers ||= Module.new | ||
end | ||
|
||
def self.default_url_options | ||
@default_url_options ||= {} | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,12 @@ class WithRails < RailtieTest | |
"ActionController::Serialization should be included in ActionController::Base, but isn't" | ||
end | ||
|
||
test 'prepares url_helpers for SerializationContext' do | ||
assert ActiveModelSerializers::SerializationContext.url_helpers.respond_to? :url_for | ||
assert_equal Rails.application.routes.default_url_options, | ||
ActiveModelSerializers::SerializationContext.default_url_options | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no non-rails test? ok for followup pr There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds reasonable. Just an obvious asymmetry. |
||
|
||
test 'sets the ActiveModelSerializers.logger to Rails.logger' do | ||
refute_nil Rails.logger | ||
refute_nil ActiveModelSerializers.logger | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
# Execute this test in isolation | ||
require 'support/isolated_unit' | ||
require 'minitest/mock' | ||
|
||
class SerializationContextTest < ActiveSupport::TestCase | ||
include ActiveSupport::Testing::Isolation | ||
|
||
def create_request | ||
request = Minitest::Mock.new | ||
request.expect(:original_url, 'original_url') | ||
request.expect(:query_parameters, 'query_parameters') | ||
end | ||
|
||
class WithRails < SerializationContextTest | ||
setup do | ||
require 'rails' | ||
require 'active_model_serializers' | ||
make_basic_app | ||
@context = ActiveModelSerializers::SerializationContext.new(create_request) | ||
end | ||
|
||
test 'create context with request url and query parameters' do | ||
assert_equal @context.request_url, 'original_url' | ||
assert_equal @context.query_parameters, 'query_parameters' | ||
end | ||
|
||
test 'url_helpers is set up for Rails url_helpers' do | ||
assert_equal Module, ActiveModelSerializers::SerializationContext.url_helpers.class | ||
assert ActiveModelSerializers::SerializationContext.url_helpers.respond_to? :url_for | ||
end | ||
|
||
test 'default_url_options returns Rails.application.routes.default_url_options' do | ||
assert_equal Rails.application.routes.default_url_options, | ||
ActiveModelSerializers::SerializationContext.default_url_options | ||
end | ||
end | ||
|
||
class WithoutRails < SerializationContextTest | ||
setup do | ||
require 'active_model_serializers/serialization_context' | ||
@context = ActiveModelSerializers::SerializationContext.new(create_request) | ||
end | ||
|
||
test 'create context with request url and query parameters' do | ||
assert_equal @context.request_url, 'original_url' | ||
assert_equal @context.query_parameters, 'query_parameters' | ||
end | ||
|
||
test 'url_helpers is a module when Rails is not present' do | ||
assert_equal Module, ActiveModelSerializers::SerializationContext.url_helpers.class | ||
refute ActiveModelSerializers::SerializationContext.url_helpers.respond_to? :url_for | ||
end | ||
|
||
test 'default_url_options return a Hash' do | ||
assert Hash, ActiveModelSerializers::SerializationContext.default_url_options.class | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,18 +7,24 @@ class LinksTest < ActiveSupport::TestCase | |
LinkAuthor = Class.new(::Model) | ||
class LinkAuthorSerializer < ActiveModel::Serializer | ||
link :self do | ||
href "//example.com/link_author/#{object.id}" | ||
href "http://example.com/link_author/#{object.id}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should recommend There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did https in the docs but tried to vary it more here to convey what you pass in is what you get back. It used to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed there are several other areas that use http URLs. I'd advocate for changing them all in a single, separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been a point of discussion. json-api/json-api#898 |
||
meta stuff: 'value' | ||
end | ||
|
||
link :other, '//example.com/resource' | ||
|
||
link(:author) { link_author_url(object.id) } | ||
link(:link_authors) { url_for(controller: 'link_authors', action: 'index', only_path: false) } | ||
link(:posts) { link_author_posts_url(object.id) } | ||
link :resource, 'http://example.com/resource' | ||
link :yet_another do | ||
"//example.com/resource/#{object.id}" | ||
"http://example.com/resource/#{object.id}" | ||
end | ||
end | ||
|
||
def setup | ||
Rails.application.routes.draw do | ||
resources :link_authors do | ||
resources :posts | ||
end | ||
end | ||
@post = Post.new(id: 1337, comments: [], author: nil) | ||
@author = LinkAuthor.new(id: 1337, posts: [@post]) | ||
end | ||
|
@@ -29,15 +35,15 @@ def test_toplevel_links | |
adapter: :json_api, | ||
links: { | ||
self: { | ||
href: '//example.com/posts', | ||
href: 'http://example.com/posts', | ||
meta: { | ||
stuff: 'value' | ||
} | ||
} | ||
}).serializable_hash | ||
expected = { | ||
self: { | ||
href: '//example.com/posts', | ||
href: 'http://example.com/posts', | ||
meta: { | ||
stuff: 'value' | ||
} | ||
|
@@ -68,13 +74,16 @@ def test_resource_links | |
hash = serializable(@author, adapter: :json_api).serializable_hash | ||
expected = { | ||
self: { | ||
href: '//example.com/link_author/1337', | ||
href: 'http://example.com/link_author/1337', | ||
meta: { | ||
stuff: 'value' | ||
} | ||
}, | ||
other: '//example.com/resource', | ||
yet_another: '//example.com/resource/1337' | ||
author: 'http://example.com/link_authors/1337', | ||
link_authors: 'http://example.com/link_authors', | ||
resource: 'http://example.com/resource', | ||
posts: 'http://example.com/link_authors/1337/posts', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you wanna get wild, let's use a json api relationships link :) GET /link_authors/1/relationships/posts HTTP/1.1
Accept: application/vnd.api+json There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I want to get that wild yet ;) |
||
yet_another: 'http://example.com/resource/1337' | ||
} | ||
assert_equal(expected, hash[:data][:links]) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dup text maybe instead link one to other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I was thinking it would be easy to miss this in several places if it just linked to once section.