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

Jump to bottom

Fix for Deserialization erroring when a relationship is null in the json api document. #1651

Merged
merged 1 commit into from
Apr 3, 2016
Merged

Fix for Deserialization erroring when a relationship is null in the json api document. #1651

merged 1 commit into from
Apr 3, 2016

Conversation

NullVoxPopuli
Copy link
Contributor

Purpose

I noticed when I was trying to parse the request for building a join model, I had this error:

NoMethodError - undefined method `[]' for nil:NilClass:
   () home/me/.rvm/gems/ruby-2.3.0/bundler/gems/active_model_serializers-96c5516d217a/lib/active_model_serializers/adapter/json_api/deserialization.rb:191:in `parse_relationship'
   () home/me/.rvm/gems/ruby-2.3.0/bundler/gems/active_model_serializers-96c5516d217a/lib/active_model_serializers/adapter/json_api/deserialization.rb:199:in `block in parse_relationships'
   () home/me/.rvm/gems/ruby-2.3.0/bundler/gems/active_model_serializers-96c5516d217a/lib/active_model_serializers/adapter/json_api/deserialization.rb:199:in `parse_relationships'
   () home/me/.rvm/gems/ruby-2.3.0/bundler/gems/active_model_serializers-96c5516d217a/lib/active_model_serializers/adapter/json_api/deserialization.rb:101:in `parse'
   () home/me/.rvm/gems/ruby-2.3.0/bundler/gems/active_model_serializers-96c5516d217a/lib/active_model_serializers/deserialization.rb:6:in `jsonapi_parse'
  app/controllers/api/restraints_controller.rb:35:in `create_restraint_params'

Changes

added test

Caveats

Related GitHub issues

Additional helpful information

How I'm parsing the params

    ActiveModelSerializers::Deserialization.jsonapi_parse(
      params,
      polymorphic: [:restriction_for, :restricted_to]
    )

This happened with the latest (as of this time) version of AMS.

@bf4
Copy link
Member

bf4 commented Apr 1, 2016

There's related issues and PRs for this too, gotta 🏃

@NullVoxPopuli
Copy link
Contributor Author

well, I need this fixed, so I'm working on fixing it. :-)

@NullVoxPopuli NullVoxPopuli changed the title failing test for Deserialization Fix for Deserialization erroring when a relationship is null in the json api document. Apr 1, 2016
@NullVoxPopuli NullVoxPopuli self-assigned this Apr 1, 2016
@NullVoxPopuli
Copy link
Contributor Author

lots of rubocop errors :-\

NullVoxPopuli
NullVoxPopuli reviewed Apr 1, 2016
View reviewed changes
lib/active_model_serializers/adapter/json_api/deserialization.rb
@@ -188,7 +188,7 @@ def parse_relationship(assoc_name, assoc_data, options)
end

polymorphic = (options[:polymorphic] || []).include?(assoc_name.to_sym)
hash["#{prefix_key}_type".to_sym] = assoc_data[:type] if polymorphic
hash["#{prefix_key}_type".to_sym] = assoc_data.try(:[], :type) if polymorphic
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in Ember, if an association isn't set: upon saving, the data for that association is just straight up nil

Copy link
Member

Choose a reason for hiding this comment

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

no more try, try! if you must

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 per JSON API spec

Resource linkage MUST be represented as one of the following:

  • null for empty to-one relationships.
  • an empty array ([]) for empty to-many relationships.
  • a single resource identifier object for non-empty to-one relationships.
  • an array of resource identifier objects for non-empty to-many relationships.

so is assoc_data possibly nil?

-          hash["#{prefix_key}_type".to_sym] = assoc_data[:type] if polymorphic
+          hash["#{prefix_key}_type".to_sym] = assoc_data[:type] if assoc_data && polymorphic

@NullVoxPopuli
Copy link
Contributor Author

looks like on rails master, assoc_data is "", whereas on non-master assoc_data is nil

@NullVoxPopuli NullVoxPopuli force-pushed the deserialization-error-with-no-attributes branch from 4cf774e to e7c1f2e Compare April 1, 2016 18:17
@NullVoxPopuli
Copy link
Contributor Author

squashed -- all tests should pass now.

I believe this could go out with rc5, @bf4

bf4
bf4 reviewed Apr 1, 2016
View reviewed changes
lib/active_model_serializers/adapter/json_api/deserialization.rb
@@ -188,7 +188,9 @@ def parse_relationship(assoc_name, assoc_data, options)
end

polymorphic = (options[:polymorphic] || []).include?(assoc_name.to_sym)
hash["#{prefix_key}_type".to_sym] = assoc_data[:type] if polymorphic
if polymorphic
hash["#{prefix_key}_type".to_sym] = assoc_data.present? ? assoc_data[:type] : nil
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you're not checking polymorphic && assoc_data. Is the idea that we want to set it to nil? so, is assoc data an empty string then? assoc_data.empty? would work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assoc_data is nil on non-master rails, and empty string on master rails. but yeah, the idea is that if the assoc_data isn't there, it would still be nice to get the _id and _type attributes in the resulting hash, so at least you know your front end is aware of the relationship (but hasn't set it)

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 did try polymorphic && assoc_data earlier, it ended up giving me just the id field and not the type field.

@bf4
Copy link
Member

bf4 commented Apr 1, 2016

changelog and ok to merge

@NullVoxPopuli
Copy link
Contributor Author

cool

@NullVoxPopuli
Fix deserialization of nil relationships
5be33af 
failing test

use try for when the assoc_data is possibly nil

rubocop test/action_controller/json_api/deserialization_test.rb -a

attempt to work on rails-master

account for rails/master having  instead of nil for assoc_data

added changelog
@NullVoxPopuli NullVoxPopuli force-pushed the deserialization-error-with-no-attributes branch from 58f54dc to 5be33af Compare April 1, 2016 20:07
@NullVoxPopuli
Copy link
Contributor Author

changelog added

@bf4 bf4 merged commit 22f88ef into rails-api:master Apr 3, 2016
@bf4 bf4 deleted the deserialization-error-with-no-attributes branch April 3, 2016 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@NullVoxPopuli NullVoxPopuli

Labels
S: Bug S: LGTM V: 0.10.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NullVoxPopuli @bf4