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.

Sign up for GitHub

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

Prevent creation of unnecessary fieldset(mirror and revised) #2450

Open
wants to merge 1 commit into
base: 0-10-stable
Choose a base branch
from
Open

Prevent creation of unnecessary fieldset(mirror and revised) #2450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion lib/active_model_serializers/adapter/attributes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ module Adapter
class Attributes < Base
def initialize(*)
super
instance_options[:fieldset] ||= ActiveModel::Serializer::Fieldset.new(fields_to_fieldset(instance_options.delete(:fields)))
liijunwei marked this conversation as resolved.
Show resolved Hide resolved

instance_options[:fieldset] ||= begin
Copy link
Author

@liijunwei liijunwei Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

change#1: only initialize ActiveModel::Serializer::Fieldset when fieldset is not nil

instance_options[:fieldset] could be:

  1. the fieldset passed in through option
  2. ActiveModel::Serializer::Fieldset instance
  3. nil

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 fine. Or add a class method on fieldset which does what's in this begin block

Copy link
Author

Choose a reason for hiding this comment

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

echoing what you previously mentioned, I also want keep the changes minimal 🙏
if it's necessary, is it better to do it in another refactoring PR?

fieldset = fields_to_fieldset(instance_options.delete(:fields))
fieldset && ActiveModel::Serializer::Fieldset.new(fieldset)
end
end

def serializable_hash(options = nil)
Expand Down
5 changes: 3 additions & 2 deletions lib/active_model_serializers/adapter/json_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def self.fragment_cache(cached_hash, non_cached_hash, root = true)
def initialize(serializer, options = {})
super
@include_directive = JSONAPI::IncludeDirective.new(options[:include], allow_wildcard: true)
@fieldset = options[:fieldset] || ActiveModel::Serializer::Fieldset.new(options.delete(:fields))
@fieldset = options[:fieldset] || ((option_fields = options.delete(:fields)) && ActiveModel::Serializer::Fieldset.new(option_fields))
Copy link
Author

Choose a reason for hiding this comment

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

@fieldset could be:

  1. the fieldset passed in through option
  2. ActiveModel::Serializer::Fieldset instance
  3. nil

end

# {http://jsonapi.org/format/#crud Requests are transactional, i.e. success or failure}
Expand Down Expand Up @@ -348,7 +348,8 @@ def data_for(serializer, include_slice)
data.tap do |resource_object|
next if resource_object.nil?
# NOTE(BF): the attributes are cached above, separately from the relationships, below.
requested_associations = fieldset.fields_for(resource_object[:type]) || '*'
requested_fields = fieldset && fieldset.fields_for(resource_object[:type])
Copy link
Author

@liijunwei liijunwei Aug 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

change#2: only ask for fields_for when fieldset is not nil

this is not a change, it just saves the fields_for cost

also align with above

requested_associations = requested_fields || '*'
relationships = relationships_for(serializer, requested_associations, include_slice)
resource_object[:relationships] = relationships if relationships.any?
end
Expand Down
14 changes: 14 additions & 0 deletions test/serializers/attribute_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,20 @@ def test_attribute_inheritance_with_key
assert_equal({ id: 1, title: 'AMS Hints' }, adapter.serializable_hash)
end

def test_attribute_initialization_with_empty_fields
inherited_klass = Class.new(AlternateBlogSerializer)
blog_serializer = inherited_klass.new(@blog)
adapter = ActiveModelSerializers::Adapter::Attributes.new(blog_serializer, {fieldset: nil, fields: nil})
assert_nil(adapter.instance_options[:fieldset])
end

def test_attribute_initialization_with_present_fields
inherited_klass = Class.new(AlternateBlogSerializer)
blog_serializer = inherited_klass.new(@blog)
adapter = ActiveModelSerializers::Adapter::Attributes.new(blog_serializer, {fieldset: nil, fields: [:id]})
assert_instance_of(ActiveModel::Serializer::Fieldset, adapter.instance_options[:fieldset])
end

def test_multiple_calls_with_the_same_attribute
serializer_class = Class.new(ActiveModel::Serializer) do
attribute :title
Expand Down