-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
to_number : built-in function now rejects "Inf", "Infinity" and "NaN" values #7203
to_number : built-in function now rejects "Inf", "Infinity" and "NaN" values #7203
Conversation
topdown/casts.go
Outdated
_, err := strconv.ParseFloat(string(a), 64) | ||
strValue := string(a) | ||
|
||
if strValue == "Inf" || strValue == "Infinity" || strValue == "NaN" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs linked in the issue:
ParseFloat recognizes the string "NaN", and the (possibly signed) strings "Inf" and "Infinity" as their respective special floating point values. It ignores case when matching.
So I think we'll need to match ignoring case here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad; didn't see the link there.I've made the change. Let me know if any other change needs to be made
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
If you could please squash your commits into one and sign that:
git commit --amend --signoff
And then push that, we'll have it merged. Thanks!
(technically, just a signoff is enough for the DCO check, and I can squash when merging if you want.. but if you do it, you can author the commit message yourself) |
5e518be
to
73e9509
Compare
… values open-policy-agent#7203 Signed-off-by: sikehish <[email protected]>
73e9509
to
5c266d9
Compare
Hi @anderseknert . I've squashed the commits. |
… values open-policy-agent#7203 Signed-off-by: sikehish <[email protected]>
5c266d9
to
cae5738
Compare
closes #7200
Why the changes in this PR are needed?
The
to_number
built-in function in OPA currently allows inputs such as"Inf"
,"Infinity"
, and"NaN"
because these are accepted by Go'sstrconv.ParseFloat
function. However, these values are not valid JSON numbers and can cause errors during JSON marshaling or undefined behavior. This change ensures thatto_number
explicitly rejects these inputs, aligning with OPA's expected behavior for numeric conversions.What are the changes in this PR?
to_number
function to explicitly reject string inputs"Inf"
,"Infinity"
, and"NaN"
.NewOperandTypeErr
when these invalid strings are encountered.