Don't modify JSON input. Fixes #94 #95

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@tysonmote

MultiJson shouldn't modify the users input before it is passed to the JSON
parsing adapter.

This fixes #94 (which was introduced in 707aae7).

@sferik sferik and 1 other commented on an outdated diff Feb 26, 2013
spec/adapter_shared_example.rb
it 'allows to load JSON with UTF-8 characters' do
expect(MultiJson.load('{"color":"żółć"}')).to eq({'color' => 'żółć'})
end
+
+ describe "options" do
+ it 'allows JSON fragment parsing with quirks_mode' do
+ expect(MultiJson.load('42', quirks_mode: true)).to eq(42)
+ end
+ end
@sferik
sferik Feb 26, 2013 Member

Please use Ruby 1.8-compatible hash syntax.

tysonmote added some commits Feb 26, 2013
@tysonmote tysonmote Don't modify JSON input passed to `load`.
MultiJson shouldn't modify the users input before it is passed to the JSON
parsing adapter.

This fixes #94 (which was introduced in 707aae7).
964f9c6
@tysonmote tysonmote Add spec for not modifying user input.
We shouldn't need this, but it's been broken before, so...
483e9fa
@sferik
Member
sferik commented Feb 26, 2013

Did you see my comment at the bottom of #94? What do you think about leaving the default the same in order to preserve backward compatibility and introduce a new :strict => true option?

@tysonmote

I feel like MultiJson shouldn't add extra options that behave differently and potentially conflict with JSON library options. If MultiJson is a thin wrapper around other JSON libraries, it shouldn't require extra options just to emulate the existing behavior of JSON, etc.

@sferik
Member
sferik commented Feb 27, 2013

Philosophically, I agree with you.

Practically speaking, we've already shipped quirks mode as the default and I don’t want to break backward compatibility. Arguably, this was a bad decision, but there’s no turning back (at least, not until the next major version, but I prefer to never break backward compatibility if I don’t have to). One of the best features of MultiJSON is a consistent, stable interface and I suspect many users depend on the existing behavior.

I definitely empathize with your perspective. There’s a tradeoff between thinness and compatibility and I’m of the opinion that we should make MultiJSON a little bit thicker in exchange for compatibility. I don’t think it’s such a big deal for people to type :strict => true if they want the behavior you’re proposing.

I’m happy to reconsider the default if and when we decide to ship 2.0.

If I were to merge and release this patch as-is, it would break many people’s applications and I’m the one who would have to deal with the fallout. If you were willing to sign-on to be a permanent maintainer of this project, I’d be somewhat more open to this request (but I’d still probably opt for compatibility because I believe that’s what’s best for MultiJSON users).

@sferik
Member
sferik commented Feb 27, 2013

You may be interested in reading Linux Torvalds’ thoughts on breaking compatibility: https://lkml.org/lkml/2012/3/8/495. 😉

@tysonmote

I 100% agree with you, which is why I opened the issue and this pull request in the first place. MultiJson's backwards-compatibilty was broken. This pull request seeks to restore its previous behavior by requiring a new option if you want the new behavior.

It just depends on what you want to retain backwards compatibility with: the original MultiJSON API or the API after it was changed in a backwards-incompatible manner. I'm fine with either, really. It just means that if this pull request is rejected and I want to upgrade my MultiJSON gem, I need to make changes to my code because MultiJSON's responses have changed.

@sferik
Member
sferik commented Feb 27, 2013

You’re correct. There’s an argument to be made that the release of 1.4 result broke compatilibity with earlier 1.x versions. However, you’re the first person to report this breakage in three months. In the mean time, we’ve shipped 1.5 and 1.6 and people have come to depend on this new behavior.

If parsing an empty string was specified in the spec suite, it would have obvious that 707aae7 was a breaking change and I wouldn’t have merged #63. But now that it’s been merged and released for a few months, it’s hard to say which compatibility is more important to preserve (pre-1.4 or post-1.4). There’s no “right” answer. I’m forced to choose between the lesser of two evils. I hope you appeciate the difficulty of this decision.

If you read through #17, you’ll see that I was against this change from the beginning. However, many people expected this behavior to work (even though it’s not the default for Ruby’s standard JSON library). I think very few people relied on this behavior not working, since you’re the first person to bring it up in months out of millions of users. Currently, the most popular version of the gem is the latest version (1.6.1) so that’s the compatibility I believe it’s more important to preserve.

I’m sympathetic to the fact that you’ll have to change your code if you want to upgrade to 1.4 (or newer). I wish this were not true. I would not have knowingly made this change. But it seems a little late to revert it now. 😧

@tysonmote

Sounds like the decision is to just keep the current behavior. If that's true, you can go ahead and close this pull request. Thanks for considering it, anyways!

@sferik
Member
sferik commented Mar 1, 2013

Thanks for submitting this pull request and making your case.

I'm going to leave this open for a little while and see if any additional feedback comes in. If there's overwhelming popular support for this patch, I'd consider applying it.

@rwz
Member
rwz commented Mar 9, 2013

Fixed in master.

@rwz rwz closed this Mar 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment