`to_number` built-in function should reject "Inf", "Infinity" and "NaN" values

[anderseknert]

While providing these will omit errors, these errors are from later JSON marshalling and not from input validation. Compare:

opa eval -f pretty --strict-builtin-errors 'to_number("Infinite")'
1 error occurred: to_number("Infinite"): eval_builtin_error: to_number: strconv.ParseFloat: parsing "Infinite": invalid syntax

opa eval -f pretty --strict-builtin-errors 'to_number("Infinity")'
json: invalid number literal "Infinity"

This since the strcov.ParseFloat function accepts these values, but OPA obviously should not.

[sikehish]

I'm looking into this issue :) Could you assign this to me?

[sikehish]

While providing these will omit errors, these errors are from later JSON marshalling and not from input validation. Compare:

opa eval -f pretty --strict-builtin-errors 'to_number("Infinite")'
1 error occurred: to_number("Infinite"): eval_builtin_error: to_number: strconv.ParseFloat: parsing "Infinite": invalid syntax

opa eval -f pretty --strict-builtin-errors 'to_number("Infinity")'
json: invalid number literal "Infinity"

This since the strcov.ParseFloat function accepts these values, but OPA obviously should not.

Hi @anderseknert . I've made the required changes. I just wanted to know where the test cases have to be added in this case(if that's required, as I've manually tested for all three inputs: "Inf", "Infinity" and "NaN").

[anderseknert]

Hi there!

There was a PR open on that already, but unfortunately it doesn't seem like it was linked https://github.com/open-policy-agent/opa/pull/7201 ... and also that there's quite some work left to do there.

@raj921 , do you want to keep going there?

As for test cases (no matter who adds them, the preferred way is to add "YAML tests" under test/cases/testdata/v1/casts ... you can look at some existing tests to see what they look like (you can also add attributes to test for errors returned).

The comment in this file contains some information for how to run those tests isolated (if not as part of the whole make test suite).

Sorry for having snoozed for a while, it's evening here and 2 kids are demanding my attention 😅

[raj921]

no sir I will try another issue

[anderseknert]

Alright, I'll keep that in mind, and can assign you to one when I find one good to start with 👍 Thanks a lot!

[anderseknert]

@sikehish you can proceed with adding some tests then as previously mentioned. Let me know if you have any questions!

[sikehish]

@sikehish you can proceed with adding some tests then as previously mentioned. Let me know if you have any questions!

Do I need to create a new file for it or can I make changes in any of the existing test case yml file?

[anderseknert]

I'd probably create a new file and name it something appropriate, but either way is fine. Don't mind the naming or the contents of the old files too much. They were generated from code long ago, and new tests don't need to follow them.

[sikehish]

I'd probably create a new file and name it something appropriate, but either way is fine. Don't mind the naming or the contents of the old files too much. They were generated from code long ago, and new tests don't need to follow them.

Cool. Also is there a way to generate the tests or do I have to write it manually?

[anderseknert]

The YAML file? You'd write that yourself. But you can obviously copy the structure from other files.

[sikehish]

The YAML file? You'd write that yourself. But you can obviously copy the structure from other files.

Yup, I was referring to the YAML file. The yml file names looked like they were generated so was a bit curious xD.

[sikehish]

Thanks for the opportunity @anderseknert !

[anderseknert]

Thanks for the contribution!