Allow not keyword in more locations

[anderseknert]

Expected Behavior

The not keyword should be allowed in most places where assignment of any type is allowed:

a := not 1 in [2, 3, 4]
b := {input.x, not input.y, not deny} 

etc..

Actual Behavior

not is only allowed as a "standalone" construct, i.e:

allow {
    not deny
}

[stale[bot]]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[ericjkao]

Want to note that there are some potentially very confusing behaviors coming with allowing not in more places as proposed. not is very unique in its definition/behavior. not of true, for example, is undefined. There is going to be endless confusions over say:

is_admin {
    ...
}

allow {
     check_something(input.user, not is_admin)  # undefined if is_admin is true
}

If we are going to allow a general negation operator, a lot of care needs to be taken to define its behavior.

Perhaps it should mimic Golang !. not true is false not false is true not of any non-boolean type is undefined (Golang exception) not undefined is undefined (Golang exception) not null is undefined (Golang exception) But that deviates from the present behavior of not.

What about keeping not as is today (only valid as the outermost modifier on a condition), but introduce ! from Golang for the general logical not operator?

Another reason we may prefer a new operator is to preserve the current parsing of conditions such as not x == y. If we allow not more generally, but use a low operator precedence to preserve the current parsing of not x == y, it would be endlessly confusing because most programmers are so trained to expect high operator precedence from not.

To summarize, I think the current Rego not is just too different from most common programming languages to be a good general not operator.

• parsing is different
• not undefined is true differs from Go and Java (making undefined analogous to nil and null)
• not 0 is undefined differs from C/Python/Javascript.
• not true is undefined differs from just about everything (but at least here we may be able to change it to false without major problems)

On the other hand, if the goal is not to have a classical logical negation, but rather something to deal with undefined, then it may make more sense to introduce a specific operator for it.

On another note, if the purpose is to have a general operator that can turn undefined into something defined, then it may make more sense to introduce an operator specifically for that purpose (turns undefined to true, everything else to false). Some candidate names:

• undef
• nonexistent
• !exists (if we introduce !)
• !defined (if we introduce !)

[srenatus]

Just a quick note, isn't this,

not true is false not false is true not of any non-boolean type is undefined (Golang exception) not undefined is undefined (Golang exception) not null is undefined (Golang exception)

like a rego function,

n(false) = true
n(true) = false
# all else is undefined

But we'd lack the extra syntactical features, having to use n(1 == 2) is not as nice as ! (1 == 2).

[srenatus]

On another note, if the purpose is to have a general operator that can turn undefined into something defined

There's also a bit of discussion around that in https://github.com/open-policy-agent/opa/issues/2345.

[oren-zohar]

Want to note that there are some potentially very confusing behaviors coming with allowing not in more places as proposed. not is very unique in its definition/behavior.

I find the entire argument here fundamentally puzzling. There is no doubt that the not keyword represents things differently from other languages. In spite of this, I strongly disagree with the conclusion that allowing its use elsewhere in the code will cause more confusion. In fact, there are two unexpected things here. One, the keyword's functionality, and two, its inability to be used where it is permitted in other languages. Allowing its use in other places will only make things less confusing IMO.

[anderseknert]

Yeah, the current confusion around not was pretty much the reason this was filed in the first place 😄 @ekcs definitely has a point though regarding how much could be changed without breaking existing policy. Could be that a new keyword/function is the only viable way forward for a great solution here. However, since the not keyword isn't going anywhere.. it would still be interesting to see where we could extend its use into more places without compromising what it does, or breaking with the existing contract.

[stale[bot]]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[ericjkao]

Want to note that there are some potentially very confusing behaviors coming with allowing not in more places as proposed. not is very unique in its definition/behavior.

I find the entire argument here fundamentally puzzling. There is no doubt that the not keyword represents things differently from other languages. In spite of this, I strongly disagree with the conclusion that allowing its use elsewhere in the code will cause more confusion. In fact, there are two unexpected things here. One, the keyword's functionality, and two, its inability to be used where it is permitted in other languages. Allowing its use in other places will only make things less confusing IMO.

Apologies @oren-zohar that I lost track of this discussion (need to change my notification settings). Let me try to amend and improve my previous comment in light of your response.

The non-standard behavior of not turning true into undefined instead of false does not currently cause major confusion precisely because not is allowed only in places where the distinction between undefined and false ultimately makes no difference (both stop the rule from succeeding). (Ok there is one rare exception: using not in a query today.)

If we allow not in more places, specifically allowing not to be used in a sub-expression, the distinction between undefined and false will have big, confusing consequences. I will show two examples that are more complete than what I shared in my original comment and end with a new idea for mitigating the situation.

Example 1

In this example, our policy authorizes an incoming request by checking with an external authz service then doing additional checks.

Example input

{
  "user": "foo",
  "action": "create",
  "prevent_overwrite": true
}

Policy

prelim_authz_allow if {
  request_body := {
    "username": input.user,
    "allow_overwrite": not input.prevent_overwrite
  }
  resp := http.send(
    {"url": "http://example.com/users/authz",
     "method": "POST",
     "body": request_body
    })
  resp.status_code == "200"
}

final_authz_allow if {
  prelim_authz_allow
  # other checks
}

Because not true evaluates to undefined, the http request body is undefined and prelim_authz_allow fails. One could argue that this is just what OPA policy authors need to get used to. This is the behavior of not and you just need to get used to it. The problem is that not flipping true and false is just so ingrained in most developer's minds that even if we drill it over and over it will still be a common, frustrating error. And yes, the desired behavior can be accomplished by a user-defined logical-not function, but that will not stop every new author from making this mistake many many times.

Example 2

# custom str match utility function to match two strings, configurable by parameter
custom_str_match(str1, str2, ignore_case, ignore_whitespace) := str1 == str2 if {
  ignore_whitespace == false
  ignore_case == false
}
custom_str_match(str1, str2, ignore_case, ignore_whitespace) := lower(str1) == lower(str2) if {
  ignore_whitespace == false
  ignore_case == true
}  # there are two more cases not defined to save space

# list of known usernames, along with a flag indicating case sensitivity
known_users := [
  {"username": "foo", "case_sensitive": true}, 
  {"username": "bar", "case_sensitive": false}
]

# use the utility function to check whether an input user matches a known user
is_known_user if {
  some user in known_users
  ignore_case := not user.case_sensitive  # intended to true->false, but actually true->undef
  ignore_whitespace := false
  custom_str_match(user.username, input.username, ignore_case, ignore_whitespace)
}

test_bar_pass if {  # test fails because author forgot weird not behavior
  test_input := {"username": "BaR"}
  is_known_user with input as test_input
}

test_foo_pass if {  # test passes
  test_input := {"username": "foo"}
  is_known_user with input as test_input
}

Candidate solution?

Here is another idea for working around the confusing situation illustrated by the two examples above. We can consider changing the behavior of not slightly, but in a way that does not actually affect today's rules. not true is false (new behavior, today this is undefined) not false is true (existing behavior) not undefined is true (existing behavior) not of any valid non-boolean value is undefined (existing behavior)

With this we would remove the biggest weird behavior (not true is undefined), and still preserve all of today's rule evaluation behavior. Very few people would even notice the change.

The one place it does change existing behavior is in a query. A query of not EXPR where EXPR evaluates to true today returns undefined. With the change suggested above this would return false. Hopefully very few people depend on this behavior, but we can't really be sure how people query OPA and then how they use the result of the query.

[stale[bot]]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.