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

Hash value of null breaks parsing #264

Merged
merged 3 commits into from
Dec 21, 2022
Merged

Conversation

toadle
Copy link

@toadle toadle commented Dec 12, 2022

Hey everybody, I don't now if this would belong into #203

but I had a GraphQL-JSON-collection that I tried to parse with representable.
Because of an error the collection came out as:

{
...,
collection: [
 {
...
},
null,
{
...
}
]
...}

This completely broke the parsing flow and error-out with NoMethodError: undefined method has_key? for nil:NilClass on

Expected behaviour would have been to just return nothing. I could even imagine a scenario, where null-entries would result in nil.

I resorted to filter the collection with a skip_parse-first, but an exception in this case is kinda annoying.

@yogeshjain999
Copy link
Member

@toadle thanks for creating the PR!

Changes looks good to me! Could you also add test for this ?

@toadle
Copy link
Author

toadle commented Dec 14, 2022

@yogeshjain999 here you go!

result = nil
begin
result = @property.read(nil, "song")
rescue NoMethodError
Copy link
Member

Choose a reason for hiding this comment

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

@toadle we don't have to consider rescuing NoMethodError now as we know it shouldn't occur. If it does, then it must be due to some other bug and should be handled differently.

So in this test, we can just assert on return value of read, like in above test.

it "..." do
  assert_equal Representable::Binding::FragmentNotFound, @property.read(nil, "song")
end

Copy link
Author

Choose a reason for hiding this comment

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

@yogeshjain999 here you go

Copy link
Author

Choose a reason for hiding this comment

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

@yogeshjain999 Wanna merge?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, almost forgot :)

@yogeshjain999 yogeshjain999 merged commit a94e412 into trailblazer:master Dec 21, 2022
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.

2 participants