Add support for passing options to JSON.parse #156

stefansedich · 2017-06-09T21:38:05Z

This should fix #124 and allow all options to be passed to JSON.parse, it felt best to pass the options to the parse proc instead of overriding the parse method which was my second option and looks to be the way that the multijson middleware does it (which I am also open to doing).

Passing the options to the proc should be a non breaking change as any existing parsing middleware will continue to work as before and if required they will have access to the options argument.

stefansedich · 2017-06-09T22:01:47Z

I notice the build is failing for Ruby 1.8.x, I saw in another issue that it was mentioned in 2016 that Faraday had dropped support for < 1.9 and that the middleware would follow suit, is this still the case?

iMacTia

Hi @stefansedich and thanks for your PR.
I think we can achieve the same result in a cleaner and backward compatible way so please address the comments in my review before we proceed 👍

iMacTia · 2017-06-12T08:49:17Z

lib/faraday_middleware/response/parse_json.rb

+        :create_additions => options[:create_additions],
+        :object_class => options[:object_class],
+        :array_class => options[:array_class]
+      )


This doesn't look DRY, it would be better to simply pass options as it is to JSON.parse.

iMacTia · 2017-06-12T08:52:38Z

lib/faraday_middleware/response_middleware.rb

@@ -47,7 +47,7 @@ def process_response(env)
    def parse(body)
      if self.class.parser
        begin
-          self.class.parser.call(body)
+          self.class.parser.call(body, @options)


Rather than passing the entire @options hash, I would opt for deciding on a specific @options key (we can call it parser_options) to be passed to the parser.
This will make the call like:

self.class.parser.call(body, @options[:parser_options])

IMPORTANT: We should pass this parameter ONLY if it's provided. We might have people defining custom parser that doesn't accept the second parameter. In order to maintain backward compatibility we need to make the second parameter optional

iMacTia · 2017-06-12T08:54:27Z

spec/unit/parse_json_spec.rb

+      response = process(body)
+      expect(response.body).to be(result)
+    end
+  end


After applying the other 2 changes, this test can be refactored using the new parser_options option.

Um, is 1.8 complaining that the lines aren't:

allow(::JSON).to receive(:parse).with(body, options[:parser_options]) { result }

stefansedich · 2017-06-12T15:03:08Z

Sounds good! Will make the changes today.

stefansedich · 2017-06-12T16:14:46Z

@iMacTia how does that look now?

iMacTia · 2017-06-12T16:37:35Z

It looks much better now!
My only concern is about that next in the parser. I'd rather rollback to the unless from before as next is usually used in loops and enumerators.

Unless there's any specific reason for using it, of course!

Finally, don't worry about ruby 1.8.7, we're not supporting it anymore on Faraday so we can just do the same here. I need to update travis tests for that though.

stefansedich · 2017-06-12T16:46:22Z

No problems @iMacTia I have rolled that back to what it was too!

iMacTia · 2017-06-12T17:28:34Z

Great, thanks @stefansedich 👍
Now it's up to me fixing those ruby 1.8.7 test.
I'll try to find some time in the next days

stefansedich · 2017-06-12T20:05:32Z

No problems. if there is anything I can help out with let me know!

stefansedich · 2017-07-02T18:18:29Z

Yup, period at end of line works but looks ugly IMO, if 1.8 is not supported anyway I was happy to leave it behind in the dust.

…

On Sun, Jul 2, 2017, 10:38 AM Olle Jonsson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In spec/unit/parse_json_spec.rb <#156 (comment)> : > + allow(::JSON).to receive(:parse) + .with( + body, + :max_nesting => options[:max_nesting], + :allow_nan => options[:allow_nan], + :symbolize_names => options[:symbolize_names], + :create_additions => options[:create_additions], + :object_class => options[:object_class], + :array_class => options[:array_class] + ) + .and_return(result) + + response = process(body) + expect(response.body).to be(result) + end + end Um, is 1.8 complaining that the lines aren't: allow(::JSON).to receive(:parse).with(body, options[:parser_options]) { result } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#156 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIqVKZF-nkk3txeJbwXh2fW_wnTSs5zks5sJ9WXgaJpZM4N13EC> .

iMacTia · 2017-07-27T13:05:34Z

@stefansedich can you please pull/rebase from master?
I've dropped support for ruby < 1.9.3 and that should fix your tests :)
Preparing release 0.12

stefansedich · 2017-07-27T15:07:28Z

@iMacTia done, funny thing is just the other day I was sitting there thinking to myself "I wish the middleware would let me pass in symbolize_names", I completely forgot I even had this PR sitting there 😄

iMacTia · 2017-07-27T15:42:41Z

Totally my fault there, it took me too much getting on this!
LGTM

stefansedich · 2017-07-27T15:54:21Z

No problems at all!

…

On Thu, Jul 27, 2017, 8:42 AM Mattia ***@***.***> wrote: Totally my fault there, it took me too much getting on this! LGTM — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#156 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIqVF7M_uFQGcnegxsNLsvBiuDdIkpoks5sSK_0gaJpZM4N13EC> .

bartoszkopinski · 2017-07-31T11:32:26Z

@iMacTia @stefansedich This change is backwards incompatible.
When parser_options is nil JSON raises an exception - ArgumentError: options must be a hash.
I suggest updating to something like:

::JSON.parse(body, parser_options || {})

iMacTia · 2017-07-31T12:50:11Z

Mmmmh strange, I'm quite sure we have a good test coverage there.
First things first, I'd like to replicate the issue. @bartoszkopinski do you have an example that shows the backward incompatibility?

iMacTia · 2017-07-31T13:14:51Z

FYI, I've just tried the following:

conn = Faraday.new('https://jsonplaceholder.typicode.com') do |f|
  f.response :json
  f.adapter :net_http
end

res = conn.get('/posts/1')

and it works perfectly. res.body is correctly parsed as a ruby Hash

bartoszkopinski · 2017-07-31T13:28:33Z

@iMacTia Sorry, I thought this was something easier to recreate.

Looking at the code, it's possible to invoke the parser block without any options:

faraday_middleware/lib/faraday_middleware/response_middleware.rb

Line 54 in 693ee0d

self.class.parser.call(body)

Which will then raise the error I mentioned:

faraday_middleware/lib/faraday_middleware/response/parse_json.rb

Line 11 in 693ee0d

::JSON.parse(body, parser_options) unless body.strip.empty?

I noticed it using a class that inherits from Faraday::Middleware:
https://github.com/shutl/shutl_resource/blob/master/lib/shutl/resource/default_logger.rb#L21

Now I'm not exactly sure how to write a standalone failing example yet, but let me know if you'd instead want me to open a separate issue once I have one.

iMacTia · 2017-07-31T14:38:59Z

Ok, let me complicate this one:

JSON::VERSION
#=> "2.1.0"
raw_body = "{\n  \"userId\": 1,\n  \"id\": 1,\n  \"title\": \"blahblahblah\",\n  \"body\": \"blahblahblah\"\n}"
JSON.parse(raw_body, nil)
#=> {"userId"=>1, "id"=>1, "title"=>"blahblahblah", "body"=>"blahblahblah"}

So it appears that the restriction on the options parameter you're referring to has been removed at some point.
Which version of JSON are you using?

bartoszkopinski · 2017-07-31T15:27:42Z

@iMacTia Interesting, I'm using Oj:

2.4.0 :002 > require 'json'
 => true
2.4.0 :003 > JSON::VERSION
 => "2.1.0"
2.4.0 :004 > JSON.parse("{}", nil)
 => {}
2.4.0 :005 > require 'oj'
 => true
2.4.0 :006 > Oj::VERSION
 => "3.3.2"
2.4.0 :007 > Oj.mimic_JSON()
 => JSON
2.4.0 :008 > JSON.parse("{}", nil)
ArgumentError: options must be a hash.
  from (irb):15:in `parse'

Didn't expect it to be incompatible with JSON, thanks for pointing it out.

stefansedich · 2017-07-31T15:29:45Z

I will get a fix over for this sometime later today, just woke up and saw this thread, looks like we want to just always pass at least an empty options hash in. - Stefan

…

On Mon, Jul 31, 2017, 8:27 AM Bart Kopinski ***@***.***> wrote: @iMacTia <https://github.com/imactia> Interesting, I'm using Oj: 2.4.0 :002 > require 'json' => true2.4.0 :003 > JSON::VERSION => "2.1.0"2.4.0 :004 > JSON.parse("{}", nil) => {}2.4.0 :005 > require 'oj' => true2.4.0 :006 > Oj::VERSION => "3.2.0"2.4.0 :007 > Oj.mimic_JSON() => JSON2.4.0 :008 > JSON.parse("{}", nil)ArgumentError: options must be a hash. from (irb):15:in `parse' Didn't expect it to be incompatible with JSON, thanks for pointing it out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#156 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIqVIzBkIjMEyFY8W76XrYwZA7Xx7GNks5sTfJvgaJpZM4N13EC> .

iMacTia · 2017-07-31T16:36:11Z

Mistery solved!
However I do understand people prefer to use Oj as it offers better performances.
Feel free to open an issue and I'll try to get support back in the next hotfix release 😉

stefansedich · 2017-07-31T16:40:43Z

PR opened @iMacTia but GitHub seems to be having some fun at the moment!

booleanbetrayal · 2017-07-31T18:38:07Z

Also experiencing the issue @bartoszkopinski mentioned with slack-ruby-client upon upgrading faraday_middleware to latest. Also using OJ gem.

iMacTia · 2017-07-31T21:48:21Z

@booleanbetrayal I'll try to review @stefansedich PR tomorrow and plan an hotfix release for this week.
However, may I suggest someone opening an issue also on the Oj gem and point out that they're not 100% compatible with the latest JSON version?

stefansedich · 2017-07-31T21:50:59Z

I will open the issue up over on OJ right now @iMacTia

iMacTia · 2017-08-01T08:15:06Z

Thank you!
For tracking purposes (and future visitors): ohler55/oj#411

booleanbetrayal · 2017-08-03T19:47:22Z

LGTM!

iMacTia · 2017-08-04T08:26:54Z

For future visitors.
The issue with Oj was addressed in Oj v3.3.3.
Moreover, @stefansedich provided a fix in #163 to support Oj < 3.3.3 which was shipped with v 0.12.2

lostisland/faraday_middleware#156 since there `parser_options` is not defaulted to a Hash, `JSON.parse` bombs

mtyeh411 · 2017-08-09T18:31:19Z

lib/faraday_middleware/response/parse_json.rb

@@ -7,8 +7,8 @@ class ParseJson < ResponseMiddleware
      require 'json' unless defined?(::JSON)
    end

-    define_parser do |body|
-      ::JSON.parse body unless body.strip.empty?
+    define_parser do |body, parser_options|


since you're introducing an argument and there are other things that rely on this, i think it'd be prudent to default the block argument to an empty hash. otherwise JSON.parse will bomb with an error that options is not a hash.

stefansedich · 2017-08-09T18:34:42Z

@mtyeh411 this has already been dealt with in #163, JSON.parse can handle a nil options but Oj could not, both have been patched now and everything should be happy again!

stefansedich force-pushed the master branch from d1b2221 to 08ecfed Compare June 9, 2017 22:00

iMacTia requested changes Jun 12, 2017

View reviewed changes

iMacTia approved these changes Jun 12, 2017

View reviewed changes

iMacTia added this to the 0.12.0 milestone Jun 13, 2017

stefansedich added 4 commits July 27, 2017 08:05

Add support for passing options to JSON.parse

0ba9702

implement feedback

c73bb0d

implement feedback

dea7f6a

pass options

89d8879

stefansedich force-pushed the master branch from 9aaeefb to 89d8879 Compare July 27, 2017 15:05

iMacTia merged commit 038632a into lostisland:master Jul 27, 2017

stefansedich mentioned this pull request Jul 31, 2017

Handle empty parser_options more reliably #163

Merged

iMacTia mentioned this pull request Aug 1, 2017

mimic_JSON compatibility with Faraday ohler55/oj#411

Closed

mtyeh411 added a commit to mtyeh411/KahunaClient that referenced this pull request Aug 9, 2017

ARCT-2813 removed JSON parse middleware

918e1b1

lostisland/faraday_middleware#156 since there `parser_options` is not defaulted to a Hash, `JSON.parse` bombs

mtyeh411 added a commit to mtyeh411/KahunaClient that referenced this pull request Aug 9, 2017

ARCT-2813 removed JSON parse middleware

5005b62

lostisland/faraday_middleware#156 since there `parser_options` is not defaulted to a Hash, `JSON.parse` bombs

mtyeh411 reviewed Aug 9, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for passing options to JSON.parse #156

Add support for passing options to JSON.parse #156

stefansedich commented Jun 9, 2017

stefansedich commented Jun 9, 2017

iMacTia left a comment

iMacTia Jun 12, 2017

iMacTia Jun 12, 2017

iMacTia Jun 12, 2017

olleolleolle Jul 2, 2017

stefansedich commented Jun 12, 2017

stefansedich commented Jun 12, 2017

iMacTia commented Jun 12, 2017

stefansedich commented Jun 12, 2017

iMacTia commented Jun 12, 2017

stefansedich commented Jun 12, 2017

stefansedich commented Jul 2, 2017 via email

iMacTia commented Jul 27, 2017

stefansedich commented Jul 27, 2017

iMacTia commented Jul 27, 2017

stefansedich commented Jul 27, 2017 via email

bartoszkopinski commented Jul 31, 2017 •

edited

Loading

iMacTia commented Jul 31, 2017

iMacTia commented Jul 31, 2017

bartoszkopinski commented Jul 31, 2017 •

edited

Loading

iMacTia commented Jul 31, 2017 •

edited

Loading

bartoszkopinski commented Jul 31, 2017 •

edited

Loading

stefansedich commented Jul 31, 2017 via email

iMacTia commented Jul 31, 2017

stefansedich commented Jul 31, 2017

booleanbetrayal commented Jul 31, 2017 •

edited

Loading

iMacTia commented Jul 31, 2017

stefansedich commented Jul 31, 2017

iMacTia commented Aug 1, 2017 •

edited

Loading

booleanbetrayal commented Aug 3, 2017

iMacTia commented Aug 4, 2017

mtyeh411 Aug 9, 2017

stefansedich commented Aug 9, 2017

Add support for passing options to JSON.parse #156

Add support for passing options to JSON.parse #156

Conversation

stefansedich commented Jun 9, 2017

stefansedich commented Jun 9, 2017

iMacTia left a comment

Choose a reason for hiding this comment

iMacTia Jun 12, 2017

Choose a reason for hiding this comment

iMacTia Jun 12, 2017

Choose a reason for hiding this comment

iMacTia Jun 12, 2017

Choose a reason for hiding this comment

olleolleolle Jul 2, 2017

Choose a reason for hiding this comment

stefansedich commented Jun 12, 2017

stefansedich commented Jun 12, 2017

iMacTia commented Jun 12, 2017

stefansedich commented Jun 12, 2017

iMacTia commented Jun 12, 2017

stefansedich commented Jun 12, 2017

stefansedich commented Jul 2, 2017 via email

iMacTia commented Jul 27, 2017

stefansedich commented Jul 27, 2017

iMacTia commented Jul 27, 2017

stefansedich commented Jul 27, 2017 via email

bartoszkopinski commented Jul 31, 2017 • edited Loading

iMacTia commented Jul 31, 2017

iMacTia commented Jul 31, 2017

bartoszkopinski commented Jul 31, 2017 • edited Loading

iMacTia commented Jul 31, 2017 • edited Loading

bartoszkopinski commented Jul 31, 2017 • edited Loading

stefansedich commented Jul 31, 2017 via email

iMacTia commented Jul 31, 2017

stefansedich commented Jul 31, 2017

booleanbetrayal commented Jul 31, 2017 • edited Loading

iMacTia commented Jul 31, 2017

stefansedich commented Jul 31, 2017

iMacTia commented Aug 1, 2017 • edited Loading

booleanbetrayal commented Aug 3, 2017

iMacTia commented Aug 4, 2017

mtyeh411 Aug 9, 2017

Choose a reason for hiding this comment

stefansedich commented Aug 9, 2017

bartoszkopinski commented Jul 31, 2017 •

edited

Loading

bartoszkopinski commented Jul 31, 2017 •

edited

Loading

iMacTia commented Jul 31, 2017 •

edited

Loading

bartoszkopinski commented Jul 31, 2017 •

edited

Loading

booleanbetrayal commented Jul 31, 2017 •

edited

Loading

iMacTia commented Aug 1, 2017 •

edited

Loading