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

fix: Fix multipleOf to work with decimals #436

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

torkeln
Copy link

@torkeln torkeln commented Sep 20, 2023

A more simplistic approach to validating floats, where we multiply both value and multipleOf with 10 until multipleOf does not have any fractions, and then compare them as integers. This should work since both numbers are fetched from a text string that can't be repeating. Could possibly help solve #397

A more simplistic approach to validating floats, where we multiply both
value and multipleOf with 10 until multipleOf does not have any
fractions, and then compare them as integers. This should work since
both numbers are fetched from a text string that can't be repeating.
@torkeln torkeln changed the title fix(rust): Fix multipleOf to work with decimals fix: Fix multipleOf to work with decimals Sep 20, 2023
@OrangeTux
Copy link
Contributor

Pinging @Stranger6667.

Is there any way we can bring this issue forward? I'm affected as well by the bug #397. Let me know if I can help.

@OrangeTux
Copy link
Contributor

I can confirm the fix works.

@OrangeTux
Copy link
Contributor

OrangeTux commented Jan 5, 2024

I've to correct myself. The solution doesn't always work.

For example, the fix fails to validate that 78.0 is a multiple of 0.1.

If found this issue by fuzzing with cargo fuzz.
Here is a diff to replicate the bug:

diff --git a/jsonschema/src/keywords/multiple_of.rs b/jsonschema/src/keywords/multiple_of.rs
index 64c1509..3cba1df 100644
--- a/jsonschema/src/keywords/multiple_of.rs
+++ b/jsonschema/src/keywords/multiple_of.rs
@@ -100,6 +100,8 @@ mod tests {
     #[test_case(&json!({"multipleOf": 0.1}), &json!(1.2))]
     #[test_case(&json!({"multipleOf": 0.1}), &json!(1.3))]
     #[test_case(&json!({"multipleOf": 0.02}), &json!(1.02))]
+    #[test_case(&json!({"multipleOf": 0.1}), &json!(78.0))]
     fn multiple_of_is_valid(schema: &Value, instance: &Value) {
         tests_util::is_valid(schema, instance)
     }

@OrangeTux
Copy link
Contributor

OrangeTux commented Jan 5, 2024

The core of this PR is this code:

while tmp_item.fract() != 0. {
    tmp_item *= 10.0;
    tmp_multiple_of *= 10.0;
}
tmp_item % tmp_multiple_of == 0.0

It returns false when tmp_item is 78.0 and tmp_multiple_of is 0.1.

Since tmp_item.fract() == 0, the execution jumps to the last line.
Interestingly, 78.0 % 0.1 returns 0.09999884. Whereas I would expect it returns 0.
See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=61f390594bccbfee2e853495a30fd52b

@OrangeTux
Copy link
Contributor

OrangeTux commented Jan 5, 2024

I found a fix. But I'm not sure yet if the fix works for all inputs.
The fix divides tmp_item by tmp_multiple_of and validates that the fractional part of the result equals to 0.0.

diff --git a/jsonschema/src/keywords/multiple_of.rs b/jsonschema/src/keywords/multiple_of.rs
index 64c1509..eb49b59 100644
--- a/jsonschema/src/keywords/multiple_of.rs
+++ b/jsonschema/src/keywords/multiple_of.rs
@@ -35,7 +35,9 @@ impl Validate for MultipleOfValidator {
                 tmp_multiple_of *= 10.0;
             }

-            tmp_item % tmp_multiple_of == 0.0
+           (tmp_item / tmp_multiple_of).fract() == 0.0
+
+
         } else {
             true
         }

@OrangeTux
Copy link
Contributor

My solution doesn't work for very large numbers and a value for "mutlipleOf" that's below 1.
Consider the values 1e308 and 0.1. 1e308 / 0.1 == f32::inf, and therefore returns false.

diff --git a/jsonschema/src/keywords/multiple_of.rs b/jsonschema/src/keywords/multiple_of.rs
index 64c1509..3d431a8 100644
--- a/jsonschema/src/keywords/multiple_of.rs
+++ b/jsonschema/src/keywords/multiple_of.rs
@@ -100,6 +100,8 @@ mod tests {
     #[test_case(&json!({"multipleOf": 0.1}), &json!(1.2))]
     #[test_case(&json!({"multipleOf": 0.1}), &json!(1.3))]
     #[test_case(&json!({"multipleOf": 0.02}), &json!(1.02))]
+    #[test_case(&json!({"multipleOf": 0.5}), &json!(1e308))]
     fn multiple_of_is_valid(schema: &Value, instance: &Value) {
         tests_util::is_valid(schema, instance)
     }

@OrangeTux
Copy link
Contributor

Here's a relevant discussion explaining why 78.0 % 0.1 is not 0.0: https://users.rust-lang.org/t/how-does-the-operator-work-with-floating-point-numbers/82030

@Stranger6667 Stranger6667 force-pushed the master branch 2 times, most recently from c8ee78a to 8eacf2d Compare March 3, 2024 19:32
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