tdegrunt / jsonschema

JSON Schema validation
Other
1.83k stars 262 forks source link

Version 1.2.3 should have been semver major #256

Closed romgrk closed 6 years ago

romgrk commented 6 years ago

Commit https://github.com/tdegrunt/jsonschema/commit/0aee236f2e6ea9e08ac6cb9729451a5af65cfc81 throws an error when two schemas have the same ID, which wasn't the case previously. When 1.2.3 is installed instead of previous 1.x versions, the following error is thrown: Schema <[object Object]> already exists with different definition.

tdegrunt commented 6 years ago

Sorry for the inconvenience, but not something we can change anymore.

romgrk commented 6 years ago

Fair enough

awwright commented 6 years ago

@romgrk Can you open an issue that demonstrates the problem? The version number isn't something we can really change, but if you can provide a sample of code with expected and actual output, I can take a look at that.

romgrk commented 6 years ago

This isn't an issue, the new behavior is more correct than the older one. The problem was that doing a fresh install would fetch the newest 1.x version, which would not work with the schemas that I was using, because of the semver incompatibility.

jasonpincin commented 6 years ago

1.2.3 should have been semver major for this change as well: https://github.com/tdegrunt/jsonschema/commit/107ac1afe7cd18fa55e25f1a70d28f1adaab8dc7#diff-aa61954d23c3708bb1b5cafc3163069dR198

Previously, JSONSchema would pass if a property was inherited from a prototype. After 1.2.3, prototype chain is not considered and errors are thrown.

awwright commented 6 years ago

@jasonpincin If something broke in a release then please file a bug report, that would be considered a bug.

As far as I can tell, though, that was part of a bugfix for the problem that certain properties like "hasOwnProperty" would always be considered present. Because of a side-effect of the behavior you describe:

> var o = {};
undefined
> o['hasOwnProperty']
[Function: hasOwnProperty]

There's just no good way to fix this, except breaking a completely undocumented side-effect that's not currently tested in the test suite, and not described in the README.

jasonpincin commented 6 years ago

I'll create a bug shortly, but to address your latter point, in that case, o['hasOwnProperty'] being considered to be present seems to be the right course of action. If you want the other behavour you can be explicit:

> var o = Object.create(null);
undefined
> o['hasOwnProperty']
undefined
awwright commented 6 years ago

@jasonpincin Cool, we'll see if we can work something out there.

Note though, JSON.parse creates objects with a prototype, which is the module is typically already used.

jasonpincin commented 6 years ago

Bug created here: https://github.com/tdegrunt/jsonschema/issues/261

tdegrunt commented 6 years ago

My 2 cents: you are more or less depending on internal behaviour.

To drag semver (which is just a tool and problematic in itself) in here and discuss semantics: => Software using Semantic Versioning MUST declare a public API We do this, this library is documented in test and in readme. We did not break API, I doubt we broke our tests.

So, in my opinion: it's not a bug.

Having said this, if @awwright sees an opening ...