Skip to content
Closed
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
1 change: 1 addition & 0 deletions lib/action_controller/serialization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ def get_serializer(resource, options = {})
end
ActiveModel::SerializableResource.serialize(resource, options) do |serializable_resource|
if serializable_resource.serializer?
serializable_resource.url_helper = self
Copy link
Member

Choose a reason for hiding this comment

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

makes me sad we need to do this :( so much coupling, but I guess that's where we're going. I coulda sworn you could

url = Rails.application.routes.url_for(:controller => "users",

                                           :action => "verify_email",

                                           :id_token => "tok",

                                           :only_path => true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can do that, but what we are trying to is more like Rails.application.routes.url_for([@post, :comments])

Copy link
Member

Choose a reason for hiding this comment

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

That's right. Any way we can at least narrow the interface to not include the entire controller? This is just be being paranoid of scope creep enabled by it being easy to do. I like it when tools make it hard to do things the authors don't want you to :)

serializable_resource.serialization_scope ||= serialization_scope
serializable_resource.serialization_scope_name = _serialization_scope
begin
Expand Down
4 changes: 4 additions & 0 deletions lib/active_model/serializable_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ def serialization_scope=(scope)
serializer_opts[:scope] = scope
end

def url_helper=(url_helper)
adapter_opts[:url_helper] = url_helper
end

def serialization_scope
serializer_opts[:scope]
end
Expand Down
19 changes: 14 additions & 5 deletions lib/active_model/serializer/adapter/json_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,16 @@ class JsonApi < Adapter
def initialize(serializer, options = {})
super
@hash = { data: [] }

@url_helper = options[:url_helper]
if fields = options.delete(:fields)
@fieldset = ActiveModel::Serializer::Fieldset.new(fields, serializer.json_key)
else
@fieldset = options[:fieldset]
end
end

attr_reader :url_helper

def serializable_hash(options = nil)
options ||= {}
if serializer.respond_to?(:each)
Expand Down Expand Up @@ -44,10 +46,17 @@ def fragment_cache(cached_hash, non_cached_hash)

private

def add_relationships(resource, name, serializers)
def add_relationships(resource, name, serializers, opts)
resource[:relationships] ||= {}
resource[:relationships][name] ||= { data: [] }
resource[:relationships][name][:data] += serializers.map { |serializer| { type: serializer.json_api_type, id: serializer.id.to_s } }

if opts.fetch(:data, true)
resource[:relationships][name] ||= { data: [] }
resource[:relationships][name][:data] += serializers.map { |serializer| { type: serializer.json_api_type, id: serializer.id.to_s } }
end
if opts.fetch(:links, false)
resource[:relationships][name] ||= {}
resource[:relationships][name][:links] = { related: url_helper.url_for([serializer.object, name]) }
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider using Rails.application.routes.url_helpers (sp?) here instead of a reference to the controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rails.application.routes.url_helpers doesn't provide a polymorphic_url method, and at the time I wasn't able to work out how to access polymorphic_url without a controller. If you think that it is worth removing the need to have a controller I can have another go at it.

Copy link
Member

Choose a reason for hiding this comment

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

Might this PR want to talk to #1018 (though these are different links)

Copy link
Member

Choose a reason for hiding this comment

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

#1018 is kind stopped for sometime, I wouldn't worry abou that right now, but indeed:

So the link building code should ideally be reusable in different contexts

This would be nice.

end
end

def add_relationship(resource, name, serializer, val=nil)
Expand Down Expand Up @@ -144,7 +153,7 @@ def add_resource_relationships(attrs, serializer, options = {})
attrs[:relationships] ||= {}

if serializer.respond_to?(:each)
add_relationships(attrs, key, serializer)
add_relationships(attrs, key, serializer, opts)
else
if opts[:virtual_value]
add_relationship(attrs, key, nil, opts[:virtual_value])
Expand Down
36 changes: 36 additions & 0 deletions test/adapter/json_api/has_many_url_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
require 'test_helper'

module ActionController
module Serialization
class JsonApiHasManyUrlTest < ActionController::TestCase
class MyController < ActionController::Base

def render_resource_with_url_association
@tag = Tag.new(id: 1)
@tag.posts = []
render json: @tag, adapter: :json_api, serializer: LinkTagSerializer
end
end

tests MyController

def test_render_resource_with_url_association
get :render_resource_with_url_association
expected = {
data: {
id: "1",
type: "tags",
relationships: {
posts: {
links: {
related: "http://test.host/tags/1/posts"
}
}
}
}
}
assert_equal expected.to_json, response.body
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 nice to have test covering with both data and links as true

end
end
end
end
15 changes: 15 additions & 0 deletions test/fixtures/poro.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
class Model
extend ActiveModel::Naming
FILE_DIGEST = Digest::MD5.hexdigest(File.open(__FILE__).read)

def self.model_name
@_model_name ||= ActiveModel::Name.new(self)
end

def to_model
self
end

def initialize(hash={})
@attributes = hash
end
Expand All @@ -29,9 +34,14 @@ def id
@attributes[:id] || @attributes['id'] || object_id
end

def to_param
id.to_s
end
Copy link
Member

Choose a reason for hiding this comment

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

Hiya, Thanks for this. Would you mind helping me understand these changes?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, could you help us on understanding these? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to_param and to_model are used in the for url generation.

to_model is called by polymorphic_url and it is expected to return an ActionModel compliant model. Without it, the polymororphic_urlv method will error.
to_param is used to generate the value for the object in the url. Without it, I get a url like http://test.host/tags/%23%3CTag:0xXXXXXX%3E/posts\

Copy link
Member

Choose a reason for hiding this comment

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

👍 Nice, could you add some comments on top of those method explaining this to help new contributors when jumping into the code 😄


### Helper methods, not required to be serializable
#
# Convenience for adding @attributes readers and writers

def method_missing(meth, *args)
if meth.to_s =~ /^(.*)=$/
@attributes[$1.to_sym] = args[0]
Expand Down Expand Up @@ -87,6 +97,7 @@ def cache_key
"#{self.class.name.downcase}/#{self.id}"
end
end
Tag = Class.new(Model)

module Spam; end
Spam::UnrelatedLink = Class.new(Model)
Expand Down Expand Up @@ -243,6 +254,10 @@ def self.root_name
has_one :blog, key: :site
end

LinkTagSerializer = Class.new(ActiveModel::Serializer) do
has_many :posts, links: true, data: false
end

VirtualValueSerializer = Class.new(ActiveModel::Serializer) do
attributes :id

Expand Down
1 change: 1 addition & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ class Foo < Rails::Application
module TestHelper
Routes = ActionDispatch::Routing::RouteSet.new
Routes.draw do
get 'tags/:tag_id/posts', to: 'application#index', as: 'tag_posts'
get ':controller(/:action(/:id))'
get ':controller(/:action)'
end
Expand Down