Skip to content

Conversation

@hkupty
Copy link
Contributor

@hkupty hkupty commented Feb 24, 2021

Should (partially) address #344

The idea is that this allows allows for simpler Narrowing of types when it's safe (from round doubles to integers) since this conversion doesn't lose information.

@stevehu stevehu merged commit 46cc360 into networknt:master Mar 14, 2021
@stevehu
Copy link
Contributor

stevehu commented Mar 14, 2021

@hkupty Thanks a lot for your contribution.

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
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

Successfully merging this pull request may close these issues.

2 participants