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

Add Rails url_helpers #1550

Merged
merged 1 commit into from
Mar 7, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
Breaking changes:

Features:
- [#1550](https://github.com/rails-api/active_model_serializers/pull/1550) Add
Rails url_helpers to `SerializationContext` for use in links. (@remear, @bf4)
- [#1004](https://github.com/rails-api/active_model_serializers/pull/1004) JSON API errors object implementation.
- Only implements `detail` and `source` as derived from `ActiveModel::Error`
- Provides checklist of remaining questions and remaining parts of the spec.
Expand Down
30 changes: 14 additions & 16 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -45,25 +45,23 @@ Rake::TestTask.new do |t|
end

desc 'Run isolated tests'
task isolated: ['test:isolated:railtie']
task isolated: ['test:isolated']
namespace :test do
namespace :isolated do
task :isolated do
desc 'Run isolated tests for Railtie'
task :railtie do
require 'shellwords'
dir = File.dirname(__FILE__)
file = Shellwords.shellescape("#{dir}/test/active_model_serializers/railtie_test_isolated.rb")
dir = Shellwords.shellescape(dir)

# https://github.com/rails/rails/blob/3d590add45/railties/lib/rails/generators/app_base.rb#L345-L363
_bundle_command = Gem.bin_path('bundler', 'bundle')
require 'bundler'
Bundler.with_clean_env do
command = "-w -I#{dir}/lib -I#{dir}/test #{file}"
require 'shellwords'
dir = File.dirname(__FILE__)
dir = Shellwords.shellescape(dir)
isolated_test_files = FileList['test/**/*_test_isolated.rb']
# https://github.com/rails/rails/blob/3d590add45/railties/lib/rails/generators/app_base.rb#L345-L363
_bundle_command = Gem.bin_path('bundler', 'bundle')
require 'bundler'
Bundler.with_clean_env do
isolated_test_files.all? do |test_file|
command = "-w -I#{dir}/lib -I#{dir}/test #{Shellwords.shellescape(test_file)}"
full_command = %("#{Gem.ruby}" #{command})
system(full_command) or # rubocop:disable Style/AndOr
fail 'Failures'
end
system(full_command)
end or fail 'Failures' # rubocop:disable Style/AndOr
end
end
end
Expand Down
9 changes: 9 additions & 0 deletions docs/general/getting_started.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,12 @@ class PostsController < ApplicationController

end
```

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`.

Copy link
Member

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?

Copy link
Member Author

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.

```ruby
Rails.application.routes.default_url_options = {
host: 'example.com'
}
```
32 changes: 31 additions & 1 deletion docs/general/rendering.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`:

Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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 json_api/links.md or how_to/links.md for most of the text here to keep the rendering.md slimmer and more general?

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.
Copy link
Member

Choose a reason for hiding this comment

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

  • Should also add a /howto/ for configuring the Support::UrlHelpers (after the meat of the PR is reviewed)

Copy link
Member

Choose a reason for hiding this comment

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

  • would be good to have url_for example and maybe default_url_options?

which might go in the configuration docs

Copy link
Member Author

Choose a reason for hiding this comment

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

Since having revised this PR to use SerializationContext I think we need:

  • Docs for configuring Rails.application.routes.default_url_options
  • Docs that give an example of url_for. (as previously mentioned)

Anything else?

Copy link
Member

Choose a reason for hiding this comment

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

Tests w grape adapter?

B mobile phone

On Mar 3, 2016, at 12:14 PM, Ben Mills notifications@github.com wrote:

Since having revised this PR to use SerializationContext I think we need:

Docs for configuring Rails.application.routes.default_url_options
Docs that give an example of url_for. (as previously mentioned)
Anything else?


Reply to this email directly or view it on GitHub.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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

On Mar 3, 2016, at 2:41 PM, Ben Mills notifications@github.com wrote:

In docs/general/rendering.md:

@@ -144,6 +144,32 @@ That's the result:

This feature is specific to JsonApi, so you have to use the use the JsonApi Adapter

+##### 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.
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.


Reply to this email directly or view it on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sorry for a little yagni i introduced

B mobile phone

On Mar 3, 2016, at 2:41 PM, Ben Mills notifications@github.com wrote:

In docs/general/rendering.md:

@@ -144,6 +144,32 @@ That's the result:

This feature is specific to JsonApi, so you have to use the use the JsonApi Adapter

+##### 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.
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.


Reply to this email directly or view it on GitHub.

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
Expand Down
10 changes: 6 additions & 4 deletions docs/general/serializers.md
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
```
Copy link
Member

Choose a reason for hiding this comment

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

somewhere we need to mention that the generic interface only supports url_for, the rails helpers also introduce e.g. link_author_posts_url

ok for followup pr

```
#### #object
Expand Down
6 changes: 4 additions & 2 deletions lib/active_model/serializer/links.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ def inherited(base)

# Define a link on a serializer.
# @example
# link :self { "//example.com/posts/#{object.id}" }
# link(:self) { resource_url(object) }
# @example
# link :self, "//example.com/user"
# link(:self) { "http://example.com/resource/#{object.id}" }
# @example
# link :resource, "http://example.com/resource"
#
def link(name, value = nil, &block)
_links[name] = block || value
Expand Down
5 changes: 5 additions & 0 deletions lib/active_model_serializers/adapter/json_api/link.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
require 'active_support/core_ext/module/delegation'

module ActiveModelSerializers
module Adapter
class JsonApi
class Link
include SerializationContext.url_helpers
delegate :default_url_options, to: SerializationContext

def initialize(serializer, value)
@object = serializer.object
@scope = serializer.scope
Expand Down
5 changes: 5 additions & 0 deletions lib/active_model_serializers/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ class Railtie < Rails::Railtie
end
end

initializer 'active_model_serializers.prepare_serialization_context' do
SerializationContext.url_helpers = Rails.application.routes.url_helpers
SerializationContext.default_url_options = Rails.application.routes.default_url_options
end

# This hook is run after the action_controller railtie has set the configuration
# based on the *environment* configuration and before any config/initializers are run
# and also before eager_loading (if enabled).
Expand Down
16 changes: 15 additions & 1 deletion lib/active_model_serializers/serialization_context.rb
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

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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
6 changes: 6 additions & 0 deletions test/active_model_serializers/railtie_test_isolated.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

  • would be good to test behavior of url_for and default_url_options here and below

Copy link
Member

Choose a reason for hiding this comment

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

no non-rails test?

ok for followup pr

Copy link
Member Author

Choose a reason for hiding this comment

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

ActiveModelSerializers::SerializationContext is an uninitialized constant in the non-rails test area. Adding that test seemed to repeat https://github.com/remear/active_model_serializers/blob/url-helpers/test/active_model_serializers/serialization_context_test_isolated.rb#L49-L56. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
18 changes: 0 additions & 18 deletions test/active_model_serializers/serialization_context_test.rb

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
29 changes: 19 additions & 10 deletions test/adapter/json_api/links_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Copy link
Member

Choose a reason for hiding this comment

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

we should recommend https :trollface:

Copy link
Member

Choose a reason for hiding this comment

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

+1 😁

Copy link
Member Author

Choose a reason for hiding this comment

The 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 //example.com... which, admittedly, confused me when I recently discovered that JSON API requires links to be full URLs. They cannot be relative. I have no reservations about making them all https.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand All @@ -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'
}
Expand Down Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down
1 change: 1 addition & 0 deletions test/support/isolated_unit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ def make_basic_app
# Set a fake logger to avoid creating the log directory automatically
fake_logger = Logger.new(nil)
config.logger = fake_logger
Rails.application.routes.default_url_options = { host: 'example.com' }
end
@app.respond_to?(:secrets) && @app.secrets.secret_key_base = '3b7cd727ee24e8444053437c36cc66c4'

Expand Down
2 changes: 2 additions & 0 deletions test/support/rails_app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ class ActiveModelSerializers::RailsApplication < Rails::Application

config.action_controller.perform_caching = true
ActionController::Base.cache_store = :memory_store

Rails.application.routes.default_url_options = { host: 'example.com' }
end
end
ActiveModelSerializers::RailsApplication.initialize!
Expand Down