voxpupuli / json-schema

Ruby JSON Schema Validator
MIT License
1.54k stars 243 forks source link

Replace yajl and json gem with multi JSON? #195

Open iainbeeston opened 9 years ago

iainbeeston commented 9 years ago

Right now there is a bit of custom code for both yajl and json-gem, which both behave slightly differently. I'd like to start a discussion about what the future should be for that.

I'd suggest that either:

  1. We switch to using multi_json (which supports all manner of json parsers with a unified api), or
  2. We switch to only supporting json (like rails did) and simplify the code a bit

There is an argument that multi_json is deprecated and nowadays the right move is for everyone to use json-gem, but I can't help thinking people should be given the choice (and therefore multi json is better)

RST-J commented 9 years ago

I think it would only be worth to keep multi_json if there is another argument than simply having the choice between different parsing libraries. Is there anything another parser does really different? If they are all more or less the same and we gain performance by sticking to one (json) then I think performance is more important.

hoxworth commented 9 years ago

The reasoning for allowing additional JSON libraries is that other JSON libraries are more performant in many cases. Oj, for example, usually shows parsing speed improvement of 50% or more.

Rails can do what they like as a framework, but as a library we shouldn't lock users into what we think is right or wrong.

RST-J commented 9 years ago

In this case I also favor keeping multi_json.

iainbeeston commented 9 years ago

But should we also aim to use multi_json by default? Instead of having code that explicitly targets yajl and json-gem?

iainbeeston commented 9 years ago

(My hope is we can reduce the amount of code by relying on just one gem for parsing json)

iainbeeston commented 9 years ago

Actually, this raises the whole broader question of whether we should be parsing json at all. What if in version 3 we didn't parse input and expect the user to parse for themself and give json schema a hash? Right now the input can be a url, a json text or a hash and we have to use heuristics to see which one we have, but generally the caller will know. Even worse, a string, a string url and a string containing json can all be valid json texts, that could be parsed or treated as (already parsed json) and we have no way of knowing which we have

RST-J commented 9 years ago

But what about $refs? If json-schema receives a schema for which the references schemas are not yet registered, we end up with the need to parse JSON. And I don't think that a user should have to register all referenced schemas by hand, that would feel tedious. If we let the user pass in some proc to parse JSON, this also complicates the interface and I would feel inconvenient with having to do this, I guess.

iainbeeston commented 9 years ago

Sorry, you're right. I'm getting confused between this issue (using one json api everywhere) and another issue (making JSON::Schema.validate not have to guess the meaning of any strings that it receives for schemas or data, by parsing them as json or loading them as urls)=

pd commented 9 years ago

multi_json is mostly dying, as can be seen by nearly all large projects slowing dropping their dependency on it. I rather liked the idea of multi_json, but so it goes. =)

I use oj on pretty much any ruby application I work on that touches JSON, but for oj, you don't need multi_json, cuz you can just call Oj.mimic_JSON and it will patch itself into place.

So, :thumbsup: for a single require 'json' (preferably in tandem with dropping 1.8.x support, so there's no dependency necessary).

RST-J commented 9 years ago

Can we find a consensus? @hoxworth is Oj.mimic_JSON alternative enough? I have no particular opinion on this except for performance. So if there is a way we could keep multi_json in and at the same time avoid the performance penalties referred to when using standard json then this would be fine for me.

hoxworth commented 9 years ago

I am in favor of reducing the amount of code, so moving to just multi-json is fine with me (if that's what we're planning on doing). Most of that code existed before multi-json was in widespread use, and wasn't ever properly refactored away.

iainbeeston commented 9 years ago

I have to admit, I don’t really understand the general argument against multi_json. Apparently it’s slower than just using the underlying gems directly?=

RST-J commented 9 years ago

I thought that I've read something about multi_json being slow(er). Ah, it is actually the question in the title from the link in your first post. Ok, then it is just a guess. Again a point where a benchmark would come in handy to probably outweigh the (to me) only argument against multi_json.

iainbeeston commented 9 years ago

Ok, I'm starting to believe we should only support the json gem. Multi json is "deprecated", plus OJ and Yajl both have a json-gem compatible mode (Oj.mimic_json and require 'yajl/json_gem'). Seems like json-gem is the new "standard"

RST-J commented 9 years ago

Is there any other major/serious JSON implementation we would rule out by this decision? If all serious alternatives have a compatibility mode then it seems fine to me.

brancz commented 9 years ago

In that case, only the code that specialises on yajl has to be removed if I am correct? I would do the changes if you can confirm that it is the consensus.

iainbeeston commented 9 years ago

Yes, I think so On Wed, 10 Jun 2015 at 7:46 pm, Frederic Branczyk notifications@github.com wrote:

In that case, only the code that specialises on yajl has to be removed if I am correct? I would do the changes if you can confirm that it is the consensus.

— Reply to this email directly or view it on GitHub https://github.com/ruby-json-schema/json-schema/issues/195#issuecomment-110874360 .

brancz commented 9 years ago

Nevermind, it seems like #206 already did the work, I looked at the wrong branch. I guess this issue can be closed?

iainbeeston commented 9 years ago

Sorry, yes. It's schedule for v3. On Sun, 21 Jun 2015 at 9:12 pm, Frederic Branczyk notifications@github.com wrote:

Nevermind, it seems like #206 https://github.com/ruby-json-schema/json-schema/pull/206 already did the work, I looked at the wrong branch. I guess this issue can be closed?

— Reply to this email directly or view it on GitHub https://github.com/ruby-json-schema/json-schema/issues/195#issuecomment-113951183 .