Skip to content

No public description #2395

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.

Sign up for GitHub

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

Open
copybara-service wants to merge 1 commit into
base: master
Choose a base branch
from
Open

No public description #2395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions runtime/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,11 @@ cc_library(
"//base:function_adapter",
"//common:type",
"//common:value",
"//eval/compiler:cel_expression_builder_flat_impl",
"//eval/compiler:flat_expr_builder",
"//eval/public:cel_expression",
"//eval/public:cel_function_registry",
"//eval/public:cel_type_registry",
"//internal:casts",
"//internal:number",
"//internal:status_macros",
Expand Down Expand Up @@ -612,6 +617,12 @@ cc_test(
"//common:kind",
"//common:value",
"//common:value_testing",
"//eval/public:activation",
"//eval/public:builtin_func_registrar",
"//eval/public:cel_expr_builder_factory",
"//eval/public:cel_expression",
"//eval/public:cel_options",
"//eval/public:cel_value",
"//extensions/protobuf:runtime_adapter",
"//internal:testing",
"//internal:testing_descriptor_pool",
Expand All @@ -620,6 +631,7 @@ cc_test(
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:status_matchers",
"@com_google_absl//absl/status:statusor",
"@com_google_absl//absl/types:optional",
"@com_google_absl//absl/types:span",
"@com_google_cel_spec//proto/cel/expr:syntax_cc_proto",
"@com_google_protobuf//:protobuf",
Expand Down
96 changes: 71 additions & 25 deletions runtime/optional_types.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@
#include "base/function_adapter.h"
#include "common/type.h"
#include "common/value.h"
#include "eval/compiler/cel_expression_builder_flat_impl.h"
#include "eval/compiler/flat_expr_builder.h"
#include "eval/public/cel_expression.h"
#include "eval/public/cel_function_registry.h"
#include "eval/public/cel_type_registry.h"
#include "internal/casts.h"
#include "internal/number.h"
#include "internal/status_macros.h"
Expand Down Expand Up @@ -104,20 +109,40 @@ absl::StatusOr<Value> SelectOptionalFieldStruct(
return OptionalValue::Of(std::move(field), arena);
}

// Finds `key` in `map`. With kCheckHas, the key must also pass MapValue::Has,
// since legacy maps may return values for absent keys.
template <bool kCheckHas>
absl::StatusOr<absl::optional<Value>> FindMapValue(
const MapValue& map, const Value& key,
const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
google::protobuf::MessageFactory* absl_nonnull message_factory,
google::protobuf::Arena* absl_nonnull arena) {
if constexpr (kCheckHas) {
CEL_ASSIGN_OR_RETURN(Value has,
map.Has(key, descriptor_pool, message_factory, arena));
if (auto has_bool = has.AsBool(); has_bool && !has_bool->NativeValue()) {
return absl::nullopt;
}
}
return map.Find(key, descriptor_pool, message_factory, arena);
}

template <bool kCheckHas>
absl::StatusOr<Value> SelectOptionalFieldMap(
const MapValue& map, const StringValue& key,
const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
google::protobuf::MessageFactory* absl_nonnull message_factory,
google::protobuf::Arena* absl_nonnull arena) {
absl::optional<Value> value;
CEL_ASSIGN_OR_RETURN(value,
map.Find(key, descriptor_pool, message_factory, arena));
CEL_ASSIGN_OR_RETURN(value, FindMapValue<kCheckHas>(map, key, descriptor_pool,
message_factory, arena));
if (value) {
return OptionalValue::Of(std::move(*value), arena);
}
return OptionalValue::None();
}

template <bool kCheckHas>
absl::StatusOr<Value> SelectOptionalField(
const OpaqueValue& opaque_value, const StringValue& key,
const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
Expand All @@ -129,8 +154,8 @@ absl::StatusOr<Value> SelectOptionalField(
}
auto container = optional_value->Value();
if (auto map_value = container.AsMap(); map_value) {
return SelectOptionalFieldMap(*map_value, key, descriptor_pool,
message_factory, arena);
return SelectOptionalFieldMap<kCheckHas>(*map_value, key, descriptor_pool,
message_factory, arena);
}
if (auto struct_value = container.AsStruct(); struct_value) {
return SelectOptionalFieldStruct(*struct_value, key, descriptor_pool,
Expand All @@ -141,6 +166,7 @@ absl::StatusOr<Value> SelectOptionalField(
runtime_internal::CreateNoMatchingOverloadError("_[?_]"), arena);
}

template <bool kCheckHas>
absl::StatusOr<Value> MapOptIndexOptionalValue(
const MapValue& map, const Value& key,
const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
Expand All @@ -151,32 +177,34 @@ absl::StatusOr<Value> MapOptIndexOptionalValue(
// Try int/uint.
auto number = internal::Number::FromDouble(double_key->NativeValue());
if (number.LosslessConvertibleToInt()) {
CEL_ASSIGN_OR_RETURN(value,
map.Find(IntValue{number.AsInt()}, descriptor_pool,
message_factory, arena));
CEL_ASSIGN_OR_RETURN(value, FindMapValue<kCheckHas>(
map, IntValue{number.AsInt()},
descriptor_pool, message_factory, arena));
if (value) {
return OptionalValue::Of(std::move(*value), arena);
}
}
if (number.LosslessConvertibleToUint()) {
CEL_ASSIGN_OR_RETURN(value,
map.Find(UintValue{number.AsUint()}, descriptor_pool,
message_factory, arena));
CEL_ASSIGN_OR_RETURN(value, FindMapValue<kCheckHas>(
map, UintValue{number.AsUint()},
descriptor_pool, message_factory, arena));
if (value) {
return OptionalValue::Of(std::move(*value), arena);
}
}
} else {
CEL_ASSIGN_OR_RETURN(
value, map.Find(key, descriptor_pool, message_factory, arena));
CEL_ASSIGN_OR_RETURN(value,
FindMapValue<kCheckHas>(map, key, descriptor_pool,
message_factory, arena));
if (value) {
return OptionalValue::Of(std::move(*value), arena);
}
if (auto int_key = key.AsInt(); int_key && int_key->NativeValue() >= 0) {
CEL_ASSIGN_OR_RETURN(
value,
map.Find(UintValue{static_cast<uint64_t>(int_key->NativeValue())},
descriptor_pool, message_factory, arena));
FindMapValue<kCheckHas>(
map, UintValue{static_cast<uint64_t>(int_key->NativeValue())},
descriptor_pool, message_factory, arena));
if (value) {
return OptionalValue::Of(std::move(*value), arena);
}
Expand All @@ -186,8 +214,9 @@ absl::StatusOr<Value> MapOptIndexOptionalValue(
static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {
CEL_ASSIGN_OR_RETURN(
value,
map.Find(IntValue{static_cast<int64_t>(uint_key->NativeValue())},
descriptor_pool, message_factory, arena));
FindMapValue<kCheckHas>(
map, IntValue{static_cast<int64_t>(uint_key->NativeValue())},
descriptor_pool, message_factory, arena));
if (value) {
return OptionalValue::Of(std::move(*value), arena);
}
Expand All @@ -211,6 +240,7 @@ absl::StatusOr<Value> ListOptIndexOptionalInt(
return OptionalValue::Of(std::move(element), arena);
}

template <bool kCheckHas>
absl::StatusOr<Value> OptionalOptIndexOptionalValue(
const OpaqueValue& opaque_value, const Value& key,
const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
Expand All @@ -222,8 +252,8 @@ absl::StatusOr<Value> OptionalOptIndexOptionalValue(
}
auto container = optional_value->Value();
if (auto map_value = container.AsMap(); map_value) {
return MapOptIndexOptionalValue(*map_value, key, descriptor_pool,
message_factory, arena);
return MapOptIndexOptionalValue<kCheckHas>(
*map_value, key, descriptor_pool, message_factory, arena);
}
if (auto list_value = container.AsList(); list_value) {
if (auto int_value = key.AsInt(); int_value) {
Expand Down Expand Up @@ -293,6 +323,7 @@ absl::StatusOr<Value> ListUnwrapOpt(
return std::move(*builder).Build();
}

template <bool kCheckHas>
absl::Status RegisterOptionalTypeFunctions(FunctionRegistry& registry,
const RuntimeOptions& options) {
if (!options.enable_qualified_type_identifiers) {
Expand Down Expand Up @@ -335,17 +366,17 @@ absl::Status RegisterOptionalTypeFunctions(FunctionRegistry& registry,
BinaryFunctionAdapter<absl::StatusOr<Value>, MapValue,
StringValue>::CreateDescriptor("_?._", false),
BinaryFunctionAdapter<absl::StatusOr<Value>, MapValue, StringValue>::
WrapFunction(&SelectOptionalFieldMap)));
WrapFunction(&SelectOptionalFieldMap<kCheckHas>)));
CEL_RETURN_IF_ERROR(registry.Register(
BinaryFunctionAdapter<absl::StatusOr<Value>, OpaqueValue,
StringValue>::CreateDescriptor("_?._", false),
BinaryFunctionAdapter<absl::StatusOr<Value>, OpaqueValue,
StringValue>::WrapFunction(&SelectOptionalField)));
BinaryFunctionAdapter<absl::StatusOr<Value>, OpaqueValue, StringValue>::
WrapFunction(&SelectOptionalField<kCheckHas>)));
CEL_RETURN_IF_ERROR(registry.Register(
BinaryFunctionAdapter<absl::StatusOr<Value>, MapValue,
Value>::CreateDescriptor("_[?_]", false),
BinaryFunctionAdapter<absl::StatusOr<Value>, MapValue,
Value>::WrapFunction(&MapOptIndexOptionalValue)));
BinaryFunctionAdapter<absl::StatusOr<Value>, MapValue, Value>::
WrapFunction(&MapOptIndexOptionalValue<kCheckHas>)));
CEL_RETURN_IF_ERROR(registry.Register(
BinaryFunctionAdapter<absl::StatusOr<Value>, ListValue,
int64_t>::CreateDescriptor("_[?_]", false),
Expand All @@ -355,7 +386,7 @@ absl::Status RegisterOptionalTypeFunctions(FunctionRegistry& registry,
BinaryFunctionAdapter<absl::StatusOr<Value>, OpaqueValue,
Value>::CreateDescriptor("_[?_]", false),
BinaryFunctionAdapter<absl::StatusOr<Value>, OpaqueValue, Value>::
WrapFunction(&OptionalOptIndexOptionalValue)));
WrapFunction(&OptionalOptIndexOptionalValue<kCheckHas>)));
CEL_RETURN_IF_ERROR(registry.Register(
UnaryFunctionAdapter<absl::StatusOr<Value>, ListValue>::CreateDescriptor(
"optional.unwrap", false),
Expand Down Expand Up @@ -384,11 +415,26 @@ absl::Status RegisterOptionalTypeFunctions(FunctionRegistry& registry,
absl::Status EnableOptionalTypes(RuntimeBuilder& builder) {
auto& runtime = cel::internal::down_cast<runtime_internal::RuntimeImpl&>(
runtime_internal::RuntimeFriendAccess::GetMutableRuntime(builder));
CEL_RETURN_IF_ERROR(RegisterOptionalTypeFunctions(
CEL_RETURN_IF_ERROR(RegisterOptionalTypeFunctions</*kCheckHas=*/false>(
builder.function_registry(), runtime.expr_builder().options()));
CEL_RETURN_IF_ERROR(builder.type_registry().RegisterType(OptionalType()));
runtime.expr_builder().enable_optional_types();
return absl::OkStatus();
}

absl::Status EnableOptionalTypes(
google::api::expr::runtime::CelExpressionBuilder& builder) {
google::api::expr::runtime::FlatExprBuilder& expr_builder =
cel::internal::down_cast<
google::api::expr::runtime::CelExpressionBuilderFlatImpl&>(builder)
.flat_expr_builder();
CEL_RETURN_IF_ERROR(RegisterOptionalTypeFunctions</*kCheckHas=*/true>(
builder.GetRegistry()->InternalGetRegistry(), expr_builder.options()));
CEL_RETURN_IF_ERROR(
builder.GetTypeRegistry()->InternalGetModernRegistry().RegisterType(
OptionalType()));
expr_builder.enable_optional_types();
return absl::OkStatus();
}

} // namespace cel::extensions
9 changes: 9 additions & 0 deletions runtime/optional_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#define THIRD_PARTY_CEL_CPP_RUNTIME_OPTIONAL_TYPES_H_

#include "absl/status/status.h"
#include "eval/public/cel_expression.h"
#include "runtime/runtime_builder.h"

namespace cel::extensions {
Expand Down Expand Up @@ -147,6 +148,14 @@ namespace cel::extensions {
// msg.?elements.optFlatMap(e, e[?0]) // return the first element if present.
absl::Status EnableOptionalTypes(RuntimeBuilder& builder);

// Legacy CelExpressionBuilder version of EnableOptionalTypes. The builder must
// be created with InterpreterOptions.enable_qualified_type_identifiers.
//
// On maps, `m.?k` and `m[?k]` also check `has(m.k)`, since legacy CelMap
// implementations may return values for absent keys.
absl::Status EnableOptionalTypes(
google::api::expr::runtime::CelExpressionBuilder& builder);

} // namespace cel::extensions

#endif // THIRD_PARTY_CEL_CPP_RUNTIME_OPTIONAL_TYPES_H_
69 changes: 69 additions & 0 deletions runtime/optional_types_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,18 @@
#include "absl/status/status.h"
#include "absl/status/status_matchers.h"
#include "absl/status/statusor.h"
#include "absl/types/optional.h"
#include "absl/types/span.h"
#include "common/function_descriptor.h"
#include "common/kind.h"
#include "common/value.h"
#include "common/value_testing.h"
#include "eval/public/activation.h"
#include "eval/public/builtin_func_registrar.h"
#include "eval/public/cel_expr_builder_factory.h"
#include "eval/public/cel_expression.h"
#include "eval/public/cel_options.h"
#include "eval/public/cel_value.h"
#include "extensions/protobuf/runtime_adapter.h"
#include "internal/testing.h"
#include "internal/testing_descriptor_pool.h"
Expand Down Expand Up @@ -57,6 +64,7 @@ using ::cel::test::OptionalValueIsEmpty;
using ::cel::expr::ParsedExpr;
using ::google::api::expr::parser::Parse;
using ::google::api::expr::parser::ParserOptions;
using ::google::api::expr::runtime::CelValue;
using ::testing::ElementsAre;
using ::testing::HasSubstr;
using ::testing::TestWithParam;
Expand Down Expand Up @@ -419,5 +427,66 @@ TEST(OptionalTypesTest, CreateStruct_KeyTypeConversionError) {
HasSubstr("type conversion error")));
}

// A legacy map that only has key "a", but returns a value for any key.
class DefaultingCelMap : public google::api::expr::runtime::CelMap {
public:
absl::optional<CelValue> operator[](CelValue key) const override {
return CelValue::CreateInt64(IsA(key) ? 1 : 0);
}
absl::StatusOr<bool> Has(const CelValue& key) const override {
return IsA(key);
}
int size() const override { return 1; }
absl::StatusOr<const google::api::expr::runtime::CelList*> ListKeys()
const override {
return absl::UnimplementedError("ListKeys");
}

private:
static bool IsA(const CelValue& key) {
return key.IsString() && key.StringOrDie().value() == "a";
}
};

using LegacyOptionalTypesTest = TestWithParam<std::string>;

TEST_P(LegacyOptionalTypesTest, EvaluatesToTrue) {
google::api::expr::runtime::InterpreterOptions options;
options.enable_qualified_type_identifiers = true;
std::unique_ptr<google::api::expr::runtime::CelExpressionBuilder> builder =
google::api::expr::runtime::CreateCelExpressionBuilder(options);
ASSERT_THAT(google::api::expr::runtime::RegisterBuiltinFunctions(
builder->GetRegistry(), options),
IsOk());
ASSERT_THAT(EnableOptionalTypes(*builder), IsOk());
ASSERT_OK_AND_ASSIGN(ParsedExpr expr,
Parse(GetParam(), "<input>",
ParserOptions{.enable_optional_syntax = true}));
ASSERT_OK_AND_ASSIGN(auto expression, builder->CreateExpression(
&expr.expr(), &expr.source_info()));
DefaultingCelMap map;
google::api::expr::runtime::Activation activation;
activation.InsertValue("m", CelValue::CreateMap(&map));
google::protobuf::Arena arena;
ASSERT_OK_AND_ASSIGN(CelValue result,
expression->Evaluate(activation, &arena));
EXPECT_TRUE(result.IsBool() && result.BoolOrDie()) << result.DebugString();
}

INSTANTIATE_TEST_SUITE_P(LegacyOptionalTypesTest, LegacyOptionalTypesTest,
testing::Values("m.?a.orValue(-1) == 1",
"m.?b.orValue(-1) == -1",
"m[?'b'].orValue(-1) == -1",
"m.?b.or(m.?a).orValue(-1) == 1",
"type(m.?a) == optional_type"));

TEST(LegacyOptionalTypes, RequiresQualifiedTypeIdentifiers) {
std::unique_ptr<google::api::expr::runtime::CelExpressionBuilder> builder =
google::api::expr::runtime::CreateCelExpressionBuilder(
google::api::expr::runtime::InterpreterOptions());
EXPECT_THAT(EnableOptionalTypes(*builder),
StatusIs(absl::StatusCode::kFailedPrecondition));
}

} // namespace
} // namespace cel::extensions
Loading