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.

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

[fix] Optional belongs_to #210

Closed
wants to merge 1 commit into from
Closed

[fix] Optional belongs_to #210

wants to merge 1 commit into from

Conversation

malina
Copy link

@malina malina commented May 14, 2018

If belongs_to relation was empty there was the error undefined method `id' for nil:NilClass

@@ -149,6 +149,7 @@ def get_included_records(record, includes_list, known_included_objects, params =
included_records.concat(serializer_records) unless serializer_records.empty?
end

next unless inc_obj
Copy link

@llenodo llenodo May 14, 2018

Choose a reason for hiding this comment

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

I ran into the same error. Seems like they're trying to solve the issue above on line 144:

included_objects = [included_objects] unless relationship_type == :has_many
next if included_objects.blank?

The problem is that included_objects is an array containing an emtpy element [nil]

I think if we just switch the order of the above 2 methods it will solve the problem and then you dont need to add this nil check

next if included_objects.blank?
included_objects = [included_objects] unless relationship_type == :has_many

@shishirmk shishirmk self-requested a review May 15, 2018 02:33
@shishirmk
Copy link
Collaborator

@malina we should write a test to demonstrate this issue. I agree with @llenodo i think the issue would be fixed by just changing the order of those 2 lines.

Copy link
Collaborator

@shishirmk shishirmk left a comment

Choose a reason for hiding this comment

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

Please consider making changes suggested by @llenodo and also please write a test to prevent this from happening in the future

@llenodo
Copy link

llenodo commented May 17, 2018

I opened up #216 to fix this issue. Issue filed here #215

@malina malina closed this May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants