diff --git a/base/BUILD b/base/BUILD index 5820966d1..a239d4751 100644 --- a/base/BUILD +++ b/base/BUILD @@ -35,6 +35,7 @@ cc_library( ":kind", "//internal:status_macros", "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/container:btree", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", diff --git a/base/attribute.cc b/base/attribute.cc index bf0f0c10d..f750a1850 100644 --- a/base/attribute.cc +++ b/base/attribute.cc @@ -19,6 +19,7 @@ #include #include "absl/base/macros.h" +#include "absl/base/nullability.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" @@ -70,6 +71,46 @@ class AttributeStringPrinter { Kind type_; }; +// Visitor for appending string representation for different qualifier kinds. +class AttributeQualifierStringPrinter { + public: + // String representation for the given qualifier is appended to output. + explicit AttributeQualifierStringPrinter(std::string* absl_nonnull output, + Kind type) + : output_(*output), type_(type) {} + + absl::Status operator()(const Kind& ignored) const { + // Attributes are represented as a variant, with illegal attribute + // qualifiers represented with their type as the first alternative. + return absl::InvalidArgumentError( + absl::StrCat("Unsupported attribute qualifier ", KindToString(type_))); + } + + absl::Status operator()(int64_t index) { + absl::StrAppend(&output_, index); + return absl::OkStatus(); + } + + absl::Status operator()(uint64_t index) { + absl::StrAppend(&output_, index); + return absl::OkStatus(); + } + + absl::Status operator()(bool bool_key) { + absl::StrAppend(&output_, (bool_key) ? "true" : "false"); + return absl::OkStatus(); + } + + absl::Status operator()(const std::string& field) { + absl::StrAppend(&output_, field); + return absl::OkStatus(); + } + + private: + std::string& output_; + Kind type_; +}; + struct AttributeQualifierTypeVisitor final { Kind operator()(const Kind& type) const { return type; } @@ -279,4 +320,11 @@ bool AttributeQualifier::IsMatch(const AttributeQualifier& other) const { return value_ == other.value_; } +absl::StatusOr AttributeQualifier::AsString() const { + std::string result; + CEL_RETURN_IF_ERROR( + absl::visit(AttributeQualifierStringPrinter(&result, kind()), value_)); + return result; +} + } // namespace cel diff --git a/base/attribute.h b/base/attribute.h index 91dc98700..69dcaf161 100644 --- a/base/attribute.h +++ b/base/attribute.h @@ -105,6 +105,8 @@ class AttributeQualifier final { return (key.has_value() && key.value() == other_key); } + absl::StatusOr AsString() const; + private: friend class Attribute; friend struct ComparatorVisitor; diff --git a/eval/public/BUILD b/eval/public/BUILD index f916f16d6..4e25c0481 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -681,10 +681,12 @@ cc_test( ":cel_attribute", ":cel_value", "//eval/public/structs:cel_proto_wrapper", - "//internal:status_macros", "//internal:testing", "@com_google_absl//absl/status", + "@com_google_absl//absl/status:status_matchers", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", + "@com_google_absl//absl/types:optional", "@com_google_protobuf//:protobuf", ], ) diff --git a/eval/public/cel_attribute_test.cc b/eval/public/cel_attribute_test.cc index f595ae97d..b72189332 100644 --- a/eval/public/cel_attribute_test.cc +++ b/eval/public/cel_attribute_test.cc @@ -3,10 +3,12 @@ #include #include "absl/status/status.h" +#include "absl/status/status_matchers.h" +#include "absl/status/statusor.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" #include "eval/public/cel_value.h" #include "eval/public/structs/cel_proto_wrapper.h" -#include "internal/status_macros.h" #include "internal/testing.h" #include "google/protobuf/arena.h" @@ -15,9 +17,8 @@ namespace { using cel::expr::Expr; +using ::absl_testing::IsOkAndHolds; using ::absl_testing::StatusIs; -using ::google::protobuf::Duration; -using ::google::protobuf::Timestamp; using ::testing::Eq; using ::testing::IsEmpty; using ::testing::SizeIs; @@ -51,17 +52,19 @@ TEST(CelAttributeQualifierTest, TestBoolAccess) { EXPECT_FALSE(qualifier.GetUint64Key().has_value()); EXPECT_TRUE(qualifier.GetBoolKey().has_value()); EXPECT_THAT(qualifier.GetBoolKey().value(), Eq(true)); + EXPECT_THAT(qualifier.AsString(), IsOkAndHolds("true")); } TEST(CelAttributeQualifierTest, TestInt64Access) { - auto qualifier = CreateCelAttributeQualifier(CelValue::CreateInt64(1)); + auto qualifier = CreateCelAttributeQualifier(CelValue::CreateInt64(-1)); EXPECT_FALSE(qualifier.GetBoolKey().has_value()); EXPECT_FALSE(qualifier.GetStringKey().has_value()); EXPECT_FALSE(qualifier.GetUint64Key().has_value()); EXPECT_TRUE(qualifier.GetInt64Key().has_value()); - EXPECT_THAT(qualifier.GetInt64Key().value(), Eq(1)); + EXPECT_THAT(qualifier.GetInt64Key().value(), Eq(-1)); + EXPECT_THAT(qualifier.AsString(), IsOkAndHolds("-1")); } TEST(CelAttributeQualifierTest, TestUint64Access) { @@ -73,6 +76,7 @@ TEST(CelAttributeQualifierTest, TestUint64Access) { EXPECT_TRUE(qualifier.GetUint64Key().has_value()); EXPECT_THAT(qualifier.GetUint64Key().value(), Eq(1UL)); + EXPECT_THAT(qualifier.AsString(), IsOkAndHolds("1")); } TEST(CelAttributeQualifierTest, TestStringAccess) { @@ -85,6 +89,7 @@ TEST(CelAttributeQualifierTest, TestStringAccess) { EXPECT_TRUE(qualifier.GetStringKey().has_value()); EXPECT_THAT(qualifier.GetStringKey().value(), Eq("test")); + EXPECT_THAT(qualifier.AsString(), IsOkAndHolds("test")); } void TestAllInequalities(const CelAttributeQualifier& qualifier) { diff --git a/extensions/protobuf/internal/qualify.cc b/extensions/protobuf/internal/qualify.cc index 411244744..6eac7e1d8 100644 --- a/extensions/protobuf/internal/qualify.cc +++ b/extensions/protobuf/internal/qualify.cc @@ -337,7 +337,12 @@ absl::StatusOr ProtoQualifyState::CheckMapIn qualifier)); if (!value_ref.has_value()) { - return runtime_internal::CreateNoSuchKeyError(""); + std::string key_string; + absl::StatusOr key_string_or = qualifier.AsString(); + if (key_string_or.ok()) { + key_string = *key_string_or; + } + return runtime_internal::CreateNoSuchKeyError(key_string); } return std::move(value_ref).value(); } diff --git a/extensions/select_optimization_test.cc b/extensions/select_optimization_test.cc index 7a861bd13..24c86c560 100644 --- a/extensions/select_optimization_test.cc +++ b/extensions/select_optimization_test.cc @@ -101,6 +101,7 @@ using ::google::api::expr::runtime::LegacyTypeInfoApis; using ::google::api::expr::runtime::LegacyTypeMutationApis; using ::google::protobuf::Empty; using ::testing::_; +using ::testing::AllOf; using ::testing::AnyOf; using ::testing::ElementsAre; using ::testing::Eq; @@ -1222,7 +1223,8 @@ INSTANTIATE_TEST_SUITE_P( ASSERT_TRUE(result->Is()) << result->DebugString(); EXPECT_THAT(result.GetError().NativeValue(), StatusIs(absl::StatusCode::kNotFound, - HasSubstr("Key not found"))); + AllOf(HasSubstr("Key not found"), + HasSubstr("$not_a_field")))); }, }, {