-
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
RC3 Updates for JSON API #853
Changes from 11 commits
3ba4386
da86747
83c2854
0f55f21
d82c599
33f3a88
294d066
946d1db
4fcacb0
ef3bfdd
b695180
90c7005
9480b56
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 |
---|---|---|
|
@@ -164,6 +164,14 @@ def json_key | |
end | ||
end | ||
|
||
def id | ||
object.id if object | ||
end | ||
|
||
def type | ||
object.class.to_s.demodulize.underscore.pluralize | ||
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. Oh, I see what you're doing. You are:
Is that it? I'm onboard with that. 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. Yes, easily overriding the type is one of the main reasons I put this here. One can create an I put the id here as well because it made thing more symmetrical, but I can remove it. 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. Wouldn't be a good idea to add comments or 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. Good idea, I was already planning on adding it to the documentation. However, we need to figure out what to do about 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. I'd suggest raising an error when the |
||
end | ||
|
||
def attributes(options = {}) | ||
attributes = | ||
if options[:fields] | ||
|
@@ -172,6 +180,8 @@ def attributes(options = {}) | |
self.class._attributes.dup | ||
end | ||
|
||
attributes += options[:required_fields] if options[:required_fields] | ||
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. Just noticed that this could lead to duplicates, I'll fix it. 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. nvm it's fine |
||
|
||
attributes.each_with_object({}) do |name, hash| | ||
hash[name] = send(name) | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ def initialize(serializer, options = {}) | |
end | ||
|
||
def serializable_hash(options = {}) | ||
@root = (@options[:root] || serializer.json_key.to_s.pluralize).to_sym | ||
@root = :data | ||
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. As this fixed by standard whould be better to move that into 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. Actually, there's no reason to store the root, as it's not used elsewhere |
||
|
||
if serializer.respond_to?(:each) | ||
@hash[@root] = serializer.map do |s| | ||
|
@@ -35,58 +35,40 @@ def serializable_hash(options = {}) | |
private | ||
|
||
def add_links(resource, name, serializers) | ||
type = serialized_object_type(serializers) | ||
resource[:links] ||= {} | ||
|
||
if name.to_s == type || !type | ||
resource[:links][name] ||= [] | ||
resource[:links][name] += serializers.map{|serializer| serializer.id.to_s } | ||
else | ||
resource[:links][name] ||= {} | ||
resource[:links][name][:type] = type | ||
resource[:links][name][:ids] ||= [] | ||
resource[:links][name][:ids] += serializers.map{|serializer| serializer.id.to_s } | ||
end | ||
resource[:links][name] ||= { linkage: [] } | ||
resource[:links][name][:linkage] += serializers.map { |serializer| { type: serializer.type, id: serializer.id.to_s } } | ||
end | ||
|
||
def add_link(resource, name, serializer) | ||
resource[:links] ||= {} | ||
resource[:links][name] = nil | ||
resource[:links][name] = { linkage: nil } | ||
|
||
if serializer && serializer.object | ||
type = serialized_object_type(serializer) | ||
if name.to_s == type || !type | ||
resource[:links][name] = serializer.id.to_s | ||
else | ||
resource[:links][name] ||= {} | ||
resource[:links][name][:type] = type | ||
resource[:links][name][:id] = serializer.id.to_s | ||
end | ||
resource[:links][name][:linkage] = { type: serializer.type, id: serializer.id.to_s } | ||
end | ||
end | ||
|
||
def add_linked(resource_name, serializers, parent = nil) | ||
def add_included(resource_name, serializers, parent = nil) | ||
serializers = Array(serializers) unless serializers.respond_to?(:each) | ||
|
||
resource_path = [parent, resource_name].compact.join('.') | ||
|
||
if include_assoc?(resource_path) && resource_type = serialized_object_type(serializers) | ||
plural_name = resource_type.pluralize.to_sym | ||
@top[:linked] ||= {} | ||
@top[:linked][plural_name] ||= [] | ||
if include_assoc?(resource_path) | ||
@top[:included] ||= [] | ||
|
||
serializers.each do |serializer| | ||
attrs = attributes_for_serializer(serializer, @options) | ||
|
||
add_resource_links(attrs, serializer, add_linked: false) | ||
add_resource_links(attrs, serializer, add_included: false) | ||
|
||
@top[:linked][plural_name].push(attrs) unless @top[:linked][plural_name].include?(attrs) | ||
@top[:included].push(attrs) unless @top[:included].include?(attrs) | ||
end | ||
end | ||
|
||
serializers.each do |serializer| | ||
serializer.each_association do |name, association, opts| | ||
add_linked(name, association, resource_path) if association | ||
add_included(name, association, resource_path) if association | ||
end if include_nested_assoc? resource_path | ||
end | ||
end | ||
|
@@ -97,14 +79,16 @@ def attributes_for_serializer(serializer, options) | |
result = [] | ||
serializer.each do |object| | ||
options[:fields] = @fieldset && @fieldset.fields_for(serializer) | ||
options[:required_fields] = [:id, :type] | ||
attributes = object.attributes(options) | ||
attributes[:id] = attributes[:id].to_s if attributes[:id] | ||
attributes[:id] = attributes[:id].to_s | ||
result << attributes | ||
end | ||
else | ||
options[:fields] = @fieldset && @fieldset.fields_for(serializer) | ||
options[:required_fields] = [:id, :type] | ||
result = serializer.attributes(options) | ||
result[:id] = result[:id].to_s if result[:id] | ||
result[:id] = result[:id].to_s | ||
end | ||
|
||
result | ||
|
@@ -128,18 +112,8 @@ def check_assoc(assoc) | |
end | ||
end | ||
|
||
def serialized_object_type(serializer) | ||
return false unless Array(serializer).first | ||
type_name = Array(serializer).first.object.class.to_s.demodulize.underscore | ||
if serializer.respond_to?(:first) | ||
type_name.pluralize | ||
else | ||
type_name | ||
end | ||
end | ||
|
||
def add_resource_links(attrs, serializer, options = {}) | ||
options[:add_linked] = options.fetch(:add_linked, true) | ||
options[:add_included] = options.fetch(:add_included, true) | ||
|
||
serializer.each_association do |name, association, opts| | ||
attrs[:links] ||= {} | ||
|
@@ -150,9 +124,9 @@ def add_resource_links(attrs, serializer, options = {}) | |
add_link(attrs, name, association) | ||
end | ||
|
||
if @options[:embed] != :ids && options[:add_linked] | ||
if options[:add_included] | ||
Array(association).each do |association| | ||
add_linked(name, association) | ||
add_included(name, association) | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
require 'pathname' | ||
|
||
class DefaultScopeNameTest < ActionController::TestCase | ||
TestUser = Struct.new(:name, :admin) | ||
TestUser = Struct.new(:id, :name, :admin) | ||
|
||
class UserSerializer < ActiveModel::Serializer | ||
attributes :admin? | ||
|
@@ -17,24 +17,24 @@ class UserTestController < ActionController::Base | |
before_filter { request.format = :json } | ||
|
||
def current_user | ||
TestUser.new('Pete', false) | ||
TestUser.new(1, 'Pete', false) | ||
end | ||
|
||
def render_new_user | ||
render json: TestUser.new('pete', false), serializer: UserSerializer, adapter: :json_api | ||
render json: TestUser.new(1, 'pete', false), serializer: UserSerializer, adapter: :json_api | ||
end | ||
end | ||
|
||
tests UserTestController | ||
|
||
def test_default_scope_name | ||
get :render_new_user | ||
assert_equal '{"users":{"admin?":false}}', @response.body | ||
assert_equal '{"data":{"admin?":false,"id":"1","type":"test_users"}}', @response.body | ||
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. I think that the resource type should stay pluralized as the urls in a rails app are normally pluralized and the jsonapi api spec recommends that the URL for a collection of resources be formed from the resource type. 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. @kurko I've refactored the test code a little bit, the output of type is now |
||
end | ||
end | ||
|
||
class SerializationScopeNameTest < ActionController::TestCase | ||
TestUser = Struct.new(:name, :admin) | ||
TestUser = Struct.new(:id, :name, :admin) | ||
|
||
class AdminUserSerializer < ActiveModel::Serializer | ||
attributes :admin? | ||
|
@@ -50,18 +50,18 @@ class AdminUserTestController < ActionController::Base | |
before_filter { request.format = :json } | ||
|
||
def current_admin | ||
TestUser.new('Bob', true) | ||
TestUser.new(1, 'Bob', true) | ||
end | ||
|
||
def render_new_user | ||
render json: TestUser.new('pete', false), serializer: AdminUserSerializer, adapter: :json_api | ||
render json: TestUser.new(1, 'pete', false), serializer: AdminUserSerializer, adapter: :json_api | ||
end | ||
end | ||
|
||
tests AdminUserTestController | ||
|
||
def test_override_scope_name_with_controller | ||
get :render_new_user | ||
assert_equal '{"admin_users":{"admin?":true}}', @response.body | ||
assert_equal '{"data":{"admin?":true,"id":"1","type":"test_users"}}', @response.body | ||
end | ||
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.
The JSON API spec requires resource
id
to be a string.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.
It's converted to a string in the adapter (https://github.com/rails-api/active_model_serializers/pull/853/files#diff-ae1e38e96a8bf28ae37918a65c448c73R89)
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.
Okay, great 👍