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

Refactor JSONAPI adapter #1103

Merged
merged 15 commits into from
Sep 6, 2015
Merged

Refactor JSONAPI adapter #1103

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8482abf
Move `id` and `json_api_type` methods from `Serializer` to `JsonApi`.
beauby Aug 31, 2015
f95f736
Refactor `add_resource_relationship`.
beauby Aug 31, 2015
343f8b9
Fix bug preventing id overriding.
beauby Aug 31, 2015
d9c6805
Refactor.
beauby Aug 31, 2015
c4faafd
Refactor `resource_identifier`.
beauby Sep 1, 2015
0401205
Fix `'id'` -> `:id`.
beauby Sep 1, 2015
f7612f2
Further refactor/streamline method names.
beauby Sep 1, 2015
91c5cbe
Cleanup `add_included`.
beauby Sep 1, 2015
bae4951
Further cleanup `included_for`.
beauby Sep 1, 2015
c593adb
Further cleanup `add_included`.
beauby Sep 1, 2015
a8a0566
Refactor `relationships_for`.
beauby Sep 1, 2015
f8c553a
Cleanup.
beauby Sep 1, 2015
3793f3f
Rename `resource_objects_for` to `primary_data_for`.
beauby Sep 2, 2015
070a2e6
Merge remote-tracking branch 'upstream/master' into fix-jsonapi-ri
beauby Sep 6, 2015
f27f13c
Fix style.
beauby Sep 6, 2015
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
14 changes: 0 additions & 14 deletions lib/active_model/serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,18 +146,6 @@ def json_key
@root || object.class.model_name.to_s.underscore
end

def id
object.id if object
end

def json_api_type
if config.jsonapi_resource_type == :plural
object.class.model_name.plural
else
object.class.model_name.singular
end
end

def attributes(options = {})
attributes =
if options[:fields]
Expand All @@ -166,8 +154,6 @@ def attributes(options = {})
self.class._attributes.dup
end

attributes += options[:required_fields] if options[:required_fields]

attributes.each_with_object({}) do |name, hash|
unless self.class._fragmented
hash[name] = send(name)
Expand Down
55 changes: 25 additions & 30 deletions lib/active_model/serializer/adapter/json_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,15 @@ def fragment_cache(cached_hash, non_cached_hash)

private

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

def add_relationship(resource, name, serializer, val=nil)
resource[:relationships] ||= {}
resource[:relationships][name] = { data: val }

if serializer && serializer.object
resource[:relationships][name][:data] = { type: serializer.json_api_type, id: serializer.id.to_s }
end
def resource_identifier(serializer)
type = if ActiveModel::Serializer.config.jsonapi_resource_type == :plural
serializer.object.class.model_name.plural
else
serializer.object.class.model_name.singular
end
id = serializer.object.id.to_s
Copy link
Member

Choose a reason for hiding this comment

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

This might be easier to read as a case statement

type = 
  case
  when  ActiveModel::Serializer.config.jsonapi_resource_type == :plural
    serializer.object.class.model_name.plural
  else
    serializer.object.class.model_name.singular
  end

maybe resource_type and resource_id as the local variable names?

This method seems like there's a latent object in here that transforms a single resource into a serializable hash

Nice on the naming http://jsonapi.org/format/#document-resource-identifier-objects :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed for resource_type and resource_id.
Could you elaborate about this "latent object"?


{ id: id, type: type }
end

def add_included(resource_name, serializers, parent = nil)
Expand Down Expand Up @@ -100,15 +96,12 @@ def attributes_for_serializer(serializer, options)

def resource_object_for(serializer, options)
options[:fields] = @fieldset && @fieldset.fields_for(serializer)
options[:required_fields] = [:id, :json_api_type]

cache_check(serializer) do
attributes = serializer.attributes(options)
attributes.delete(:id)

Copy link
Member

Choose a reason for hiding this comment

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

So, we don't need required fields because they're the { id: '', type: ''} in the resource identifier object method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.

result = {
id: attributes.delete(:id).to_s,
type: attributes.delete(:json_api_type)
}
result = resource_identifier(serializer)
Copy link
Member

Choose a reason for hiding this comment

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

I missed why you're throwing out seriarlizer.attributes(options)[:id]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we do not want it to be part of the attributes property of the resource, but on the top-level alongside type.

Copy link
Member

Choose a reason for hiding this comment

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

No, I get that.. but you're not using it, but it's there... that's what I mean by throwing it out... I'm sure there's a detail I'm missing in the refactor, but figured it'd be good to ask from ignorance :)

Copy link
Member

Choose a reason for hiding this comment

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

I mean, is the serializer.attributes(options)[:id] the same as serializer.object.id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, good catch. This change actually breaks id overriding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is fixed now.


result[:attributes] = attributes if attributes.any?
result
Expand Down Expand Up @@ -136,22 +129,24 @@ def check_assoc(assoc)
def add_resource_relationships(attrs, serializer, options = {})
options[:add_included] = options.fetch(:add_included, true)

attrs[:relationships] ||= {} if serializer.associations.any?
serializer.associations.each do |association|
key = association.key
serializer = association.serializer
opts = association.options

attrs[:relationships] ||= {}

if serializer.respond_to?(:each)
add_relationships(attrs, key, serializer)
else
if opts[:virtual_value]
add_relationship(attrs, key, nil, opts[:virtual_value])
else
add_relationship(attrs, key, serializer)
end
end
value = if serializer.respond_to?(:each)
serializer.map { |s| resource_identifier(s) }
else
if opts[:virtual_value]
opts[:virtual_value]
elsif serializer && serializer.object
resource_identifier(serializer)
else
nil
end
end
Copy link
Member

Choose a reason for hiding this comment

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

yeah, definitely a latent object in here for serializer and serializer collections... it would also make naming blocks easier e.g. serializers.each { |serializer| }

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 OT, sorry... just thinking out loud)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree for naming blocks but here there are already two local variables named serializer (and in an other PR I'd actually like to use the outside one, so any suggestion on how to handle the naming situation?).


attrs[:relationships][association.key] = { data: value }

if options[:add_included]
Array(serializer).each do |s|
Expand Down
6 changes: 0 additions & 6 deletions test/serializers/attributes_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,6 @@ def test_attributes_with_fields_option
@profile_serializer.attributes(fields: [:name]))
end

def test_required_fields
assert_equal({name: 'Name 1', description: 'Description 1'},
@profile_serializer.attributes(fields: [:name, :description], required_fields: [:name]))

end

def test_attributes_inheritance_definition
assert_equal([:id, :body], @serializer_klass._attributes)
end
Expand Down