Improve type validation of numeric values#343
Merged
stevehu merged 1 commit intonetworknt:masterfrom Nov 15, 2020
Merged
Conversation
Contributor
|
@oguzhanunlu Thanks a lot for your help. |
Contributor
|
@oguzhanunlu This is an important new feature and it differentiates us from other validators. Could you please add a document in the doc folder? Thanks. |
Contributor
Author
|
thanks for the kind words @stevehu , sure, I'll add a doc and ping you on the new PR |
Contributor
Author
Contributor
|
@oguzhanunlu Thanks a lot for the help. It looks good to me and I have merged it. Planning for another release soon. |
Contributor
Author
|
Thanks @stevehu ! |
stevehu
pushed a commit
that referenced
this pull request
Jan 14, 2022
* Improve type validation of integrals Instead of doing string comparison, use new jackson method to determine if a number is an integral. The `javaSemantics` config option was added in PR #343 which partially addressed issue #334. In the notes for this PR: > Once jackson-databind 2.12.0 is out, I'll replace my solution with a call to canConvertToExactIntegral jackson-databind has been updated to 2.12.1 so this is available but the change has not yet been made. PR #450 which addressed #446 missed this location which is used when calling `JsonSchemaFactory.getSchema`. Issue #344 requested coercion of various types but the only type implemented in PR #379 was lossless narrowing, set with configuration option `losslessNarrowing`. I believe that setting is unnecessary now as this implementation of `javaSemantics` addresses the original issue, but left it for backwards compatibility. It also allows you to set `javaSemantics=false` and `losslessNarrowing=true` to achieve only this specific case rather than anything that `javaSemantics` is used for in the future. At this time, these properties do exactly the same thing. - Change from string comparison to `canConvertToExactIntegral` for `javaSemantics` and `losslessNarrowing` - Add missing documentation around `losslessNarrowing` - Add more test cases around integrals * Update changelog
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR tries to address #334 . The solution is not perfect, however, it improves the current situation.
In my opinion, we should not merge this PR as is. We should wait jackson-databind's upcoming
2.12.0release which will introduceJsonNode.canConvertToExactIntegral(), a new method that will meet our purpose.I wanted to open the PR so that maintainers can see my approach, the way I provide config to all validators etc.
Once jackson-databind 2.12.0 is out, I'll replace my solution with a call to
canConvertToExactIntegral