voxpupuli / json-schema

Ruby JSON Schema Validator
MIT License
1.53k stars 242 forks source link

Disable caching by default #310

Open Odaeus opened 8 years ago

Odaeus commented 8 years ago

I was testing a schema in a Rails Pry REPL, with commands like so:

data = {book: {title: "Schemas", author: "Joe Bloggs"}}
JSON::Validator.fully_validate("schema.json", data.to_json, strict: true)

I would edit the schema file and re-run those commands. My schema contained a "$ref" but I would keep getting the same errors even after I made changes. It took me a long time to find out that schemas are cached as I can't find mention of this in the readme and that the caching appears to be weakly-keyed (https://github.com/ruby-json-schema/json-schema/issues/214).

I can easily replicate the problem by making any minor change to a "$ref" section and noting that the new schema is not applied in the validation command until I run clear_cache.

My issue though is that caching should always be opt-in. I would never expect a gem to cache things by itself and it's problematic that it does so via class variables, (which is already noted).

Would a PR be accepted that disabled caching by default?

iainbeeston commented 8 years ago

I'd be in favour of disabling caching. In fact I'd be in favour of removing it completely and encouraging users to reuse json schema Validator objects instead, if they don't want to reinitialise everything each time they perform validation.

RST-J commented 8 years ago

I'm in. A PR should add a note about this to the change log.

ioquatix commented 8 years ago

+1 the current design is bad. For example, use in a multi-threaded web server could result in race conditions.

Odaeus commented 8 years ago

I started on unpicking the class-method based design but the code seems to rely on class state quite a lot and coupled with my unfamiliarity of the code unfortunately I don't have time to explore further. (tbh I ended up switching to a different gem) 😐

iainbeeston commented 8 years ago

Yeah I've also tried this before. It's not trivial

On Wed, Mar 2, 2016 at 9:16 am, Andrew France notifications@github.com wrote: I started on unpicking the class-method based design but the code seems to rely on class state quite a lot and coupled with my unfamiliarity of the code unfortunately I don't have time to explore further. (tbh I ended up switching to a different gem) 😐

— Reply to this email directly or view it on GitHub [https://github.com/ruby-json-schema/json-schema/issues/310#issuecomment-191145607] .[https://github.com/notifications/beacon/AAG1W7OEHddwZhAyOrOnhTo99tA5zXliks5ppVVagaJpZM4Hgdmi.gif]

ioquatix commented 8 years ago

What gem did you switch to?

Odaeus commented 8 years ago

@ioquatix I'll respond privately as I don't think it's polite to promote a "rival" gem. :smile:

iainbeeston commented 8 years ago

It's ok, we won't be offended! And there are only two json schema Ruby gems (that I'm aware of!)

On Wed, Mar 2, 2016 at 9:57 am, Andrew France notifications@github.com wrote: @ioquatix [https://github.com/ioquatix] I'll respond privately as I don't think it's polite to promote a "rival" gem.:smile: [https://assets-cdn.github.com/images/icons/emoji/unicode/1f604.png]

— Reply to this email directly or view it on GitHub [https://github.com/ruby-json-schema/json-schema/issues/310#issuecomment-191164180] .[https://github.com/notifications/beacon/AAG1W12SrQuPGSXWcv8kUGu73bKyUQR4ks5ppV75gaJpZM4Hgdmi.gif]

Odaeus commented 8 years ago

Heh ok, I'm using JSchema. I haven't done any in-depth comparison but it works for me and already has an instance-based design.

iainbeeston commented 8 years ago

I've just made a PR that fixes up some aspects of caching ( #334 ). On the latest release, setting JSON::Validator.cache_schemas = false or giving the clear_cache: true option to JSON:Schema.validate does not clear the cache.

At present caching is on by default. I have no issue with disabling it, but for now it might be safer to add some explanation of caching to the readme. If you do want to disable caching by default you can set JSON::Validator.cache_schemas from true to false (might not work until after #334 is merged)

chrisdhanson commented 6 years ago

@iainbeeston Has there been any update to this? The documentation surrounding caching in this package is pretty limited.