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

mimic_JSON compatibility with Faraday #411

Closed
werleo opened this issue Jul 31, 2017 · 7 comments
Closed

mimic_JSON compatibility with Faraday #411

werleo opened this issue Jul 31, 2017 · 7 comments

Comments

@werleo
Copy link

werleo commented Jul 31, 2017

Recent changes in FaradayMiddleware https://github.com/lostisland/faraday_middleware/pull/156/files

makes oj incompatible with it.

We use rails, faraday and oj.
Everything worked fine until change mentioned above.
Right now we receive ParsingError with a message: "options must be a hash".

This is the same message which you will receive calling JSON.parse with incorrect options.

Example:

JSON.parse( { ok: true }.to_json, '')
ArgumentError: options must be a hash.
        from (irb):17:in `parse'

I'm not sure if it is Oj error or faraday_middleware error.

For now, we need to remove oj completely or use faraday_middleware in the previous version.

@ohler55
Copy link
Owner

ohler55 commented Jul 31, 2017

Since the JSON gem also raises an exception it seems Oj is working as expected in mimicking the JSON gem. The same behavior is also present in a Rails console. The documentation for bot the JSON gem and Rails calls for the second argument, if present, to be an options hash. The faraday_middleware seems to have introduced a bug.

Looking at the patch code it is clear they have introduced a bug as the @parse_options will end of defaulting to nil instead of {}.

@stefansedich
Copy link

stefansedich commented Jul 31, 2017

@ohler55 it looks to me as it does not behave the same as the JSON gem, for example:

irb(main):002:0> JSON.parse('{}', nil)
=> {}

Then if I add Oj and attempt the same thing it blows up:

irb(main):005:0> Oj.mimic_JSON
irb(main):006:0> JSON.parse('{}', nil)
ArgumentError: options must be a hash.

I have patched the faraday_middleware to fix the issue for now but just wanted to chime in on this here as it does look like Oj handles nil options differently.

@ohler55
Copy link
Owner

ohler55 commented Jul 31, 2017

I stand corrected. For nil the behavior is different than an empty string, or any string for that matter.

irb(main):001:0> JSON.parse('{}', '')
ArgumentError: wrong number of arguments (given 2, expected 1)
	from /Users/ohler/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/json-2.1.0/lib/json/common.rb:156:in `initialize'
	from /Users/ohler/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/json-2.1.0/lib/json/common.rb:156:in `new'
	from /Users/ohler/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/json-2.1.0/lib/json/common.rb:156:in `parse'
	from (irb):1
	from /Users/ohler/.rbenv/versions/2.4.1/bin/irb:11:in `<main>'

I'll update Oj to special case nil. I thank you for making the change in faraday_middleware.

@werleo
Copy link
Author

werleo commented Aug 1, 2017

I'm sorry for the confusion about the empty string.
I forgot that in project console I have already mimic_JSON turned on.

Thank you @stefansedich @ohler55 for making changes in your gems to fix the problem.

@iMacTia
Copy link

iMacTia commented Aug 1, 2017

Thank you for your understanding, I'll create a link to our PR to keep this tracked:
lostisland/faraday_middleware#156

@ohler55
Copy link
Owner

ohler55 commented Aug 2, 2017

Just released Oj 3.3.3 that allows a nil second argument to parse but continues to raise an exception on anything else other than a Hash.

@werleo
Copy link
Author

werleo commented Aug 2, 2017

I just update gem version and all tests are passing.

Thank you very much.
We may close this issue.

@ohler55 ohler55 closed this as completed Aug 2, 2017
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

No branches or pull requests

4 participants