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

Handle "null" json responses #103

Merged
merged 3 commits into from Sep 28, 2018
Merged

Conversation

iaddict
Copy link
Contributor

@iaddict iaddict commented Sep 28, 2018

"null" gets parsed to nil and causes the following code to break.

With this patch nil gets returned.

When a json api responds with a "null" body and status 200, parsing did fail. With this patch it returns nil as response.
@andyjeffries
Copy link
Collaborator

I think I'd rather it returned an empty object of the current class rather than nil, that way you can still check things like the return status or have callbacks extract standard custom headers, etc.

@coveralls
Copy link

coveralls commented Sep 28, 2018

Coverage Status

Coverage increased (+0.06%) to 98.73% when pulling d439d73 on iaddict:patch-2 into 696d08e on flexirest:master.

@iaddict
Copy link
Contributor Author

iaddict commented Sep 28, 2018

Ok. This would be fine too.
Actually this also was my first thought :).

@andyjeffries
Copy link
Collaborator

Would you like to resubmit a PR with that response, or close this one and open a new one, or close this one and I'll do it when I get a chance ;-) ?

"null" json responses are now handled and return an empty object.
@andyjeffries
Copy link
Collaborator

Wow, that was quick!

@iaddict
Copy link
Contributor Author

iaddict commented Sep 28, 2018

Many thanks for this gem 👍. Hope that this PR is ok.

@andyjeffries
Copy link
Collaborator

Actually, is there a test for this? The existing @response.body.blank? should catch #nil? but you said it was when it returned the string null. Or maybe the code should be:

begin
  if @response.body.blank? || @response.body == "null"
    body = {}
  else 
    body = MultiJson.load(@response.body)
  end
rescue MultiJson::ParseError
  raise ResponseParseException.new(status:@response.status, body:@response.body, headers:@response.headers)
end

@andyjeffries
Copy link
Collaborator

And you're very welcome for the gem, glad you like it :-)

@iaddict
Copy link
Contributor Author

iaddict commented Sep 28, 2018

This also would be an option. I thought it would be more safe to let the json parser decide what a null value actually is. But the string comparison may also be an option. I'll change it.

@andyjeffries
Copy link
Collaborator

Ahhhh OK, ignore me - I thought the JSON parser was failing on the null. Don't worry, your last commit is fine then.

@andyjeffries andyjeffries merged commit 76c8374 into flexirest:master Sep 28, 2018
@iaddict
Copy link
Contributor Author

iaddict commented Sep 28, 2018

Great! Many thanks!

@andyjeffries
Copy link
Collaborator

Released as v1.7.2

@iaddict
Copy link
Contributor Author

iaddict commented Sep 28, 2018

Perfect. That was also fast :).

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.

None yet

3 participants