Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Semantic versioning and behavioral changes #830

Closed
fvanham-coveo opened this issue Jun 20, 2023 · 2 comments
Closed

Semantic versioning and behavioral changes #830

fvanham-coveo opened this issue Jun 20, 2023 · 2 comments

Comments

@fvanham-coveo
Copy link

Hi folks, first of all, great work on this project! I have a few comments on changes over the last few months which are causing undue pain on integrators. Most of us update to the latest patch version whenever it's available using e.g. renovate. However, recently patch versions have significantly changed the external behavior of the validator. Here is a small non-exhaustive sample:

If you change the internals of the validator, that directly influences the json which downstream processes will have to deal with. We have testing in place to prevent downstream systems suddenly being confronted with unexpected input, but had to adapt these tests multiple times over recent patch versions.

Ultimately, the output of this project is a set of messages that determine whether a json input is valid or not. I think any significant change in that output (insofar they're not obviously a bug fix) warrants a minor version increase rather than a patch. Happy to entertain a discussion!

@fdutton
Copy link
Contributor

fdutton commented Jun 20, 2023

@fvanham-coveo I agree with your basic position but wish to point out that the recent changes are an effort to bring the functionality into compliance with the JSON Schema Specification as demonstrated by successfully passing the JSON Schema Test Suite. This implies that all non-compliant behaviors are actually bugs whether obvious or not.

However, there are a few items that have taken a few tries to get right. Two examples are behaviors related to regular expressions (e.g., patternProperties) and unevaluated locations (e.g., unevaluatedProperties). It has also proven problematic to introduce better test-case coverage without significantly affecting performance. I apologize for any churn my incomplete implementations caused but I cannot say the same for most of the changes as they have improved the overall correctness significantly (i.e., 3x increase in passing test-cases).

The question now becomes one of allowing clients to continue accepting possibly malformed content in an effort to minimize churn or alert them to the fact that their content is actually flawed. I think there are clients that fall on each side of this question. It seems that you prefer the former over the latter.

I am really interested in understanding how recent changes have caused you issues.

  • According to your description, Supports uri-reference format. #764 relaxes a constraint so I am really confused by this one. In my experience, relaxing a constraint is backwards-compatible while tightening a constraint is always a breaking change.
  • I must have missed something with Makes JsonSchemaFactory solely responsible for creating JsonSchema instances. #781 since I attempted to keep the existing functionality intact and only mark it as deprecated. I would like to know what I missed.
  • Regular expressions have been difficult to get correct as I mentioned above. The specification requires that you use the syntax and semantics as defined in ECMA-262 with the additional understanding that regular expressions are not implicitly anchored (i.e., they require a find rather than a match). This library supports both ECMA-262 and Java as a regular-expression engine. This is a decision that predates my involvement with the project but has become a thorn in my side as it seems impossible to get both to behave the same. I wish to know if you tried using SchemaValidatorsConfig.setEcma262Validator(true) since the default is to use the Java engine.

@fvanham-coveo
Copy link
Author

For the first two changes, we had tests which validated that specific content did not pass validation. So widening validation affected the tests only. For the latter we had a small production issue, we use the java engine. I appreciate you moving to full compatibility with JSON Schema Spec, and we're on board with that 👍 It would be nice if big-step changes in that direction (even if they're arguably improvements) are tagged as minor version increases rather than patches though, it causes a little less churn for integrators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants