colinhacks / zod

TypeScript-first schema validation with static type inference
https://zod.dev
MIT License
33.69k stars 1.17k forks source link

multipleOf acts wrong starting with 7 decimal places #3486

Open OlliL opened 5 months ago

OlliL commented 5 months ago

If you want to enforce a max amount of decimal places you can use multipleOf(). If you want to enforce a max amount of 3 decimal places you can use multipleOf(0.001) which works perfectly fine.

So while this works up to 6 decimals (multipleOf(0.000001)) fine, it stops working with 7 decimals and above. To make it parse with 7 decimal places and above, the number to parse must contain exactly the amount of decimal places as specified in multpleOf.A small showcase:

  const number3 = 5.123;
  const number6 = 5.123456;
  const number7 = 5.1234567;
  const number8 = 5.12345678;
  let schema6 = z.number().multipleOf(0.000001);
  let schema7 = z.number().multipleOf(0.0000001);
  console.log(schema6.parse(number3));  // OK
  console.log(schema6.parse(number6));  // OK
  console.log(schema6.parse(number7));  // Error --> OK
  console.log(schema6.parse(number8));  // Error --> OK
  console.log(schema7.parse(number3));  // Error --> Bug
  console.log(schema7.parse(number6));  // Error --> Bug
  console.log(schema7.parse(number7));  // OK
  console.log(schema7.parse(number8));  // Error --> OK

The bug happens because of how JavaScript handles complex numbers. Zods internal function floatSafeRemainder tries to parse its parameter. While it works fine with 0.000001, it stops working fine with 7 and more decimals because then step becomes 1e-7 instead of 0.0000001 and the parsing logic stops working.

grafik

A possible fix which accounts exponential representation of complex numbers could be the following drop-in replacement logic for stepDecCount:

    let stepDecCount = (step.toString().split(".")[1] || "").length;
    if(stepDecCount === 0 && /\d?e-\d?/.test(step)) {
      stepDecCount = step.toString().match(/\d?e-(\d?)/)[1];
    }

Seems to work fine for me.....

dellamora commented 5 months ago

I would like to work on this. Can you assign me this issue ^^

OlliL commented 3 months ago

Any news about getting this into main?

dellamora commented 2 months ago

Any news about getting this into main?

nothing yet

OlliL commented 1 week ago

Quite a lot of time has passed by for this rather trivial issue. I was just wondering - what holding the pull request back from getting merged?

dellamora commented 1 week ago

Quite a lot of time has passed by for this rather trivial issue. I was just wondering - what holding the pull request back from getting merged?

@colinhacks