Add nil checking for wrapperspb's types

[goccy]

We sometimes use types like Int64Value defined in wrapperspb to distinguish between null and default values. However, in the current implementation, specifying nil for *wrapperspb.Int64Value results in 0 ( int type ), and there is no way to distinguish if it is null. Therefore, we have adjusted it to return nil as intended when the value is null.

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 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

• Most changes should be accompanied by tests.
• Commit messages should explain why the changes were made.

Summary of change in 50 characters or less

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.

Reviews

• Perform a self-review.
• Make sure the Travis CI build passes.
• Assign a reviewer once both the above have been completed.

Merging

• If a CEL maintaner approves the change, it may be merged by the author if
  they have write access. Otherwise, the change will be merged by a maintainer.
• Multiple commits should be squashed before merging.
• Please append the line closes #<issue-num>: description in the merge message,
  if applicable.

[TristonianJones]

@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.

[goccy]

@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.

[TristonianJones]

/gcbrun

[TristonianJones]

@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.

[TristonianJones]

@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

[goccy]

@TristonianJones Thank you for your comment, I've added the test cases, please confirm it.

[TristonianJones]

@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:

cel-go/common/types/provider_test.go

Lines 694 to 697 in 9e64eb7

	func TestNativeToValue_Wrappers(t *testing.T) {
	// Wrapper conversion test.
	expectNativeToValue(t, wrapperspb.Bool(true), True)
	expectNativeToValue(t, &wrapperspb.BoolValue{}, False)

I believe the following should work:

expectNativeToValue(t, (*wrapperspb.BoolValue)(nil), NullValue)

[goccy]

@TristonianJones Thank you for reviewing ! I've added the test cases !

[TristonianJones]

/gcbrun