Closed goccy closed 1 month ago
@goccy could you give an example of the expressions and inputs where this is a problem? In general, the special-casing for null
or primitive wrapper type is handled at the field selection step. If you're looking for an optional value, then prefer using cel.OptionalTypes()
and marking the input as an optional type cel.OptionalType(int)
which is usually a much better user experience than doing null
testing and ternaries.
@TristonianJones
For example, in a case where the input value is nil
, we want the output result to also be nil
. We use Int64Value
to distinguish between null
and default value, so being unable to distinguish between them goes against the intended purpose of the Int64Value
type.
The example code is the following.
// cond is bool type
// a and b are google.protobuf.Int64Value type
ast, iss := env.Compile("cond ? a : b")
if iss.Err() != nil { ... }
prg, err := env.Program(ast)
if err != nil { ... }
out, _, err := prg.ContextEval(ctx, map[string]any{
"cond": true,
"a": nil,
"b": &wrapperspb.Int64Value{Value: 1},
})
if err != nil { ... }
expected, err := out.ConvertToNative(reflect.TypeOf((*wrapperspb.Int64Value)(nil))))
if err != nil { ... }
// expected value is `nil` for `wrapperspb.Int64Value` type, but got `0`
We cannot control env.Compile
's argument and cond
, a
, b
values, they are user's input variables.
/gcbrun
@goccy Thanks for your patience. I just need to create conformance tests around this to make sure I understand how all the CEL implementations are handling this case.
@goccy would you mind adding tests for this case? I think it's fine to support this way since MaybeUnwrap
and MaybeUnwrapDynamic
should have identical unwrap behaviors and there's a clear gap here. It hadn't surfaced in the field selection paths (where such protos normally appear) due to those calls being routed through MaybeUnwrapDynamic
@TristonianJones Thank you for your comment, I've added the test cases, please confirm it.
@goccy Thank you again for all your patience. As a final request could you update the following file as well to include cases that validate the end to end behavior during NativeToValue
calls:
I believe the following should work:
expectNativeToValue(t, (*wrapperspb.BoolValue)(nil), NullValue)
@TristonianJones Thank you for reviewing ! I've added the test cases !
/gcbrun
We sometimes use types like
Int64Value
defined inwrapperspb
to distinguish betweennull
and default values. However, in the current implementation, specifyingnil
for*wrapperspb.Int64Value
results in0
(int
type ), and there is no way to distinguish if it isnull
. Therefore, we have adjusted it to returnnil
as intended when the value isnull
.If this change is acceptable, I will also add test case later.
Pull Requests Guidelines
See CONTRIBUTING.md for more details about when to create a GitHub [Pull Request][1] and when other kinds of contributions or consultation might be more desirable.
When creating a new pull request, please fork the repo and work within a development branch.
Commit Messages
Background on why the change is being made with additional detail on consequences of the changes elsewhere in the code or to the general functionality of the library. Multiple paragraphs may be used, but please keep lines to 72 characters or less.