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

is_number does not correctly return false (also the other is_ type checking builtins) #4943

Closed
ericjkao opened this issue Jul 27, 2022 · 4 comments · Fixed by #5064
Closed
Assignees
Labels

Comments

@ericjkao
Copy link
Contributor

Short description

% opa eval 'is_number("foo")'
{}

Steps To Reproduce

Expected behavior

Should return false according to docs and according to design.
https://www.openpolicyagent.org/docs/latest/policy-reference/#builtin-types-is_number

Additional context

% opa version  
Version: 0.42.2
Build Commit: efcf506
Build Timestamp: 2022-07-13T08:28:33Z
Build Hostname: Mac-1657700812705.local
Go Version: go1.18.3
Platform: darwin/amd64
WebAssembly: available

Seems to affect the other type checking builtins too. Because they are all affected, there may be a thought to simply adopt the current behavior as THE CORRECT behavior. But the current behavior has some serious downsides such as:

  • these type checking builtins are inconsistent with all the other is_ builtins in Rego which return false (at least according to docs) and lead to confusion/frustration.
  • confusing and very inconvenient behavior when we want to pass the result of is_number(x) as an argument into another function call.
@ericjkao ericjkao added the bug label Jul 27, 2022
@srenatus
Copy link
Contributor

Some of this could be worked around with helper methods...

import future.keywords

number_(x) if is_number(x)
number_(x) := false if not is_number(x)

x := number_("foo")
y := number_(2)

@anderseknert
Copy link
Member

We just recently made this exact change for the various x.is_valid functions, so having the same behavior here would be consistent with that. Unless there's a strong reason to treat is_number differently than those, I think we should make the same change here.

@anderseknert
Copy link
Member

Looking at the policy reference for is_number I'm reminded that there's a whole bunch of other is_x functions too. If we do change something here it should probably not be isolated to is_number then.

@philipaconrad
Copy link
Contributor

From the Policy Reference page this is the precise list of is_type functions that we need to worry about:

@philipaconrad philipaconrad self-assigned this Aug 29, 2022
philipaconrad added a commit to philipaconrad/opa that referenced this issue Aug 29, 2022
Fixes open-policy-agent#4943

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
srenatus pushed a commit that referenced this issue Aug 30, 2022
Fixes #4943

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants