Skip to content

add optional attributes #66

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

Closed
wants to merge 2 commits into from
Closed

add optional attributes #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc9cc41
91 allow includes strings (#93)
sjmog Feb 28, 2018
1b3de7e
add optional attributes
andyjeffries Feb 8, 2018
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
21 changes: 19 additions & 2 deletions lib/fast_jsonapi/object_serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,17 @@ def process_options(options)

if options[:include].present?
@includes = options[:include].delete_if(&:blank?).map(&:to_sym)
self.class.validate_includes!(@includes)
validate_includes!(@includes)
end
end

def validate_includes!(includes)
return if includes.blank?

existing_relationships = self.class.relationships_to_serialize.keys.to_set

unless existing_relationships.superset?(includes.to_set)
raise ArgumentError, "One of keys from #{includes} is not specified as a relationship on the serializer"
end
end

Expand Down Expand Up @@ -149,11 +159,18 @@ def cache_options(cache_options)

def attributes(*attributes_list, &block)
attributes_list = attributes_list.first if attributes_list.first.class.is_a?(Array)
options = attributes_list.last.is_a?(Hash) ? attributes_list.pop : {}
self.attributes_to_serialize = {} if self.attributes_to_serialize.nil?
self.optional_attributes_to_serialize = {} if self.optional_attributes_to_serialize.nil?

attributes_list.each do |attr_name|
method_name = attr_name
key = run_key_transform(method_name)
attributes_to_serialize[key] = block || method_name
if options[:if].present?
optional_attributes_to_serialize[key] = [method_name, options[:if]]
else
attributes_to_serialize[key] = block || method_name
end
Copy link
Contributor

@christophersansone christophersansone Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think it will be more maintainable if we put all attributes into a single attributes_to_serialize list, rather than splitting them up. It will help to have one single code path to enumerate the attributes and to evaluate them, as we continue to evolve this and add more option parameters besides if. The if check should be part of the evaluation process.

Maybe the value for each key of attributes_to_serialize should be a hash with { method_name: ..., block: ..., params: ... }. Or an actual AttributeDefinition class. That way we can have a single list of attributes with a common structure that defines how to evaluate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good points. I like the idea of the AttributeDefinition class if it's to have a defined structure. Using a hash and expecting it to be of a particular form can get fragile as multiple people continue iterating on it (in my opinion).

Copy link
Contributor

@christophersansone christophersansone Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@TrevorHinesley Yeah, in fact, the AttributeDefinition class can be the one with the serialization method:

class AttributeDefinition
  ...
  def serialize(record, serialization_params, output_hash)
    if include_attribute?(record, serialization_params)
      output_hash[key] = ...
    end
  end

  def include_attribute?(record, serialization_params)
    if conditional_proc.present?
      conditional_proc.call(record, serialization_params)
    else
      true
    end
  end
end

(EDIT: extracting the attribute evaluation away from the serializer itself may make the ability to define custom attribute methods on the serializer (#49) harder, so let's take that into account too.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@christophersansone re: EDIT -- totally understand, but since we have the ability to use blocks for custom methods as it stands, it seems reasonable that this could be merged in, then #49 could be updated to work with this new functionality as necessary (whether that means rolling this back or revising its implementation a bit).

I don't mean to imply that we should just ignore other PRs as these kinds of abstractions are discussed, but I don't think it should be a primary concern for this PR if it makes the most sense for this feature set (considering the other PR isn't merged in, and the focus of this PR is implementing this feature into the existing codebase). Hope that makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TrevorHinesley 👍 Totally agree, let's focus on the right implementation here and worry about #49 later. (It could be as simple as passing the serializer instance to AttributeDefinition#serialize and having it be called... lots of options here, not worth fretting about now though.)

end
end

Expand Down
11 changes: 10 additions & 1 deletion lib/fast_jsonapi/serialization_core.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ module SerializationCore
included do
class << self
attr_accessor :attributes_to_serialize,
:optional_attributes_to_serialize,
:relationships_to_serialize,
:cachable_relationships_to_serialize,
:uncachable_relationships_to_serialize,
Expand Down Expand Up @@ -73,13 +74,21 @@ def links_hash(record, params = {})
end

def attributes_hash(record, params = {})
attributes_to_serialize.each_with_object({}) do |(key, method), attr_hash|
attributes = attributes_to_serialize.each_with_object({}) do |(key, method), attr_hash|
attr_hash[key] = if method.is_a?(Proc)
method.arity == 1 ? method.call(record) : method.call(record, params)
else
record.public_send(method)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

If we had a single "attribute definition", then we can extract the evaluation into a separate method, e.g.:

attributes_to_serialize.each_with_object({}) do |(key, definition), attr_hash|
  inject_attribute(record, params, attr_hash, key, definition)
end

...

def inject_attribute(record, serialization_params, output_hash, key, definition)
  if_proc = definition[:if]
  include_key = if_proc ? if_proc.call(record, serialization_params) : true
  if include_key
    output_hash[key] = ...
  end
end

Something like that. In general it just seems like it would be nice to have a single function that says "given a record, an output hash, and an attribute definition, apply the attribute definition to the record and adjust the output hash accordingly".


self.optional_attributes_to_serialize = {} if self.optional_attributes_to_serialize.nil?
optional_attributes_to_serialize.each do |key, details|
method_name, check_proc = details
attributes[key] = record.send(method_name) if check_proc.call(record)
Copy link
Contributor

@TrevorHinesley TrevorHinesley Jun 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@andyjeffries couldn't this be check_proc.call(record, params) to get access to the custom parameters? That'd be necessary for our implementation (@kyreeves caught this), and if you're checking admin key I'd imagine you'd need that as well. I feel like most context-driven responses won't rely on the record's data to determine conditional rendering, but rather outside data given in params.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, that's a nice touch too.

end

attributes
end

def relationships_hash(record, relationships = nil, params = {})
Expand Down
3 changes: 3 additions & 0 deletions spec/lib/object_serializer_performance_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
}
}

before(:all) { GC.disable }
after(:all) { GC.enable }

context 'when testing performance of serialization' do
it 'should create a hash of 1000 records in less than 50 ms' do
movies = 1000.times.map { |_i| movie }
Expand Down
16 changes: 16 additions & 0 deletions spec/lib/object_serializer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,4 +302,20 @@ class BlahSerializer
expect(serializable_hash[:included][0][:links][:self]).to eq url
end
end

context 'optional attributes' do
it 'returns optional attribute when attribute is included' do
movie.release_year = 2001
json = MovieOptionalSerializer.new(movie).serialized_json
serializable_hash = JSON.parse(json)
expect(serializable_hash['data']['attributes']['release_year']).to eq movie.release_year
end

it "doesn't returns optional attribute when attribute is not included" do
movie.release_year = 1970
json = MovieOptionalSerializer.new(movie).serialized_json
serializable_hash = JSON.parse(json)
expect(serializable_hash['data']['attributes'].has_key?('release_year')).to be_falsey
end
end
end
7 changes: 7 additions & 0 deletions spec/shared/contexts/movie_context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,13 @@ class AccountSerializer
set_type :account
belongs_to :supplier
end

class MovieOptionalSerializer
include FastJsonapi::ObjectSerializer
set_type :movie
attributes :name
attribute :release_year, if: Proc.new { |record| record.release_year >= 2000 }
end
end


Expand Down