-
Notifications
You must be signed in to change notification settings - Fork 1.4k
sparse fieldsets #700
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
sparse fieldsets #700
Changes from 5 commits
39bee48
34f0847
be54e0b
c9c58e3
fc1562c
2ed52f9
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 |
---|---|---|
|
@@ -7,14 +7,20 @@ def initialize(serializer, options = {}) | |
serializer.root = true | ||
@hash = {} | ||
@top = @options.fetch(:top) { @hash } | ||
|
||
if fields = options.delete(:fields) | ||
@fieldset = ActiveModel::Serializer::Fieldset.new(fields, serializer.json_key) | ||
else | ||
@fieldset = options[:fieldset] | ||
end | ||
end | ||
|
||
def serializable_hash(options = {}) | ||
@root = (@options[:root] || serializer.json_key.to_s.pluralize).to_sym | ||
|
||
if serializer.respond_to?(:each) | ||
@hash[@root] = serializer.map do |s| | ||
self.class.new(s, @options.merge(top: @top)).serializable_hash[@root] | ||
self.class.new(s, @options.merge(top: @top, fieldset: @fieldset)).serializable_hash[@root] | ||
end | ||
else | ||
@hash[@root] = attributes_for_serializer(serializer, @options) | ||
|
@@ -93,6 +99,10 @@ def add_linked(resource, serializer, parent = nil) | |
private | ||
|
||
def attributes_for_serializer(serializer, options) | ||
if fields = @fieldset && @fieldset.fields_for(serializer) | ||
options[:fields] = fields | ||
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. I think just options[:fields] = @fields if @fieldset.fields_for(serializer) will do. No need to set this variable. |
||
attributes = serializer.attributes(options) | ||
attributes[:id] = attributes[:id].to_s if attributes[:id] | ||
attributes | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
module ActiveModel | ||
class Serializer | ||
class Fieldset | ||
|
||
attr_reader :fields, :root | ||
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. Is this being used by anyone outside this class? If not, could you move it to below the |
||
|
||
def initialize(fields, root = nil) | ||
@root = root | ||
@fields = parse(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. Do you mind moving this ...
@fields = fields
...
def fields
@parsed_fields ||= parse(fields)
end |
||
end | ||
|
||
def fields_for(serializer) | ||
key = serializer.json_key || serializer.class.root_name | ||
fields[key.to_sym] | ||
end | ||
|
||
private | ||
|
||
def parse(fields) | ||
if fields.is_a?(Hash) | ||
fields.inject({}) { |h,(k,v)| h[k.to_sym] = v.map(&:to_sym); h} | ||
elsif fields.is_a?(Array) | ||
if root.nil? | ||
raise ArgumentError, 'The root argument must be specified if the fileds argument is an array.' | ||
end | ||
hash = {} | ||
hash[root.to_sym] = fields.map(&:to_sym) | ||
hash | ||
else | ||
{} | ||
end | ||
end | ||
|
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
require 'test_helper' | ||
|
||
module ActiveModel | ||
class Serializer | ||
class FieldsetTest < Minitest::Test | ||
|
||
def test_fieldset_with_hash | ||
fieldset = ActiveModel::Serializer::Fieldset.new({'post' => ['id', 'title'], 'coment' => ['body']}) | ||
|
||
assert_equal( | ||
{:post=>[:id, :title], :coment=>[:body]}, | ||
fieldset.fields | ||
) | ||
end | ||
|
||
def test_fieldset_with_array_of_fields_and_root_name | ||
fieldset = ActiveModel::Serializer::Fieldset.new(['title'], 'post') | ||
|
||
assert_equal( | ||
{:post => [:title]}, | ||
fieldset.fields | ||
) | ||
end | ||
end | ||
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.
I don't think this change belongs into the serializer. The serializer should just return all attributes, where jsonapi adapter would then intersect the ones it needs. I think this should go inside
JsonApiAdapter
.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 would agree If it just returned the attribute names but it calls send for each attribute. It would be terribly inefficient to call the method just to filter it out later.
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.
@kurko any suggestions?