diff --git a/conformance/BUILD b/conformance/BUILD index b538e25d4..b357cef48 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -90,8 +90,6 @@ cc_binary( # uncommented when the spec changes to truncation rather than rounding. "--skip_test=conversions/int/double_nearest,double_nearest_neg,double_half_away_neg,double_half_away_pos", "--skip_test=conversions/uint/double_nearest,double_nearest_int,double_half_away", - # TODO(issues/82): Unexpected behavior when converting invalid bytes to string. - "--skip_test=conversions/string/bytes_invalid", # TODO(issues/96): Well-known type conversion support. "--skip_test=proto2/literal_wellknown", "--skip_test=proto3/literal_wellknown", diff --git a/eval/public/BUILD b/eval/public/BUILD index 3722053af..4bd638717 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -180,6 +180,7 @@ cc_library( ":cel_function_adapter", ":cel_function_registry", ":cel_options", + "//base:unilib", "//eval/public/containers:container_backed_list_impl", "@com_google_absl//absl/numeric:int128", "@com_google_absl//absl/status", diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index 34554d5d4..86fa2adbe 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -16,6 +16,7 @@ #include "eval/public/cel_options.h" #include "eval/public/containers/container_backed_list_impl.h" #include "re2/re2.h" +#include "base/unilib.h" namespace google { namespace api { @@ -1145,16 +1146,15 @@ absl::Status RegisterStringConversionFunctions( return absl::OkStatus(); } - // TODO(issues/82): ensure the bytes conversion to string handles UTF-8 - // properly, and avoids unncessary allocations. - // bytes -> string - auto status = FunctionAdapter:: - CreateAndRegister( + auto status = + FunctionAdapter::CreateAndRegister( builtin::kString, false, - [](Arena* arena, - CelValue::BytesHolder value) -> CelValue::StringHolder { - return CelValue::StringHolder( - Arena::Create(arena, std::string(value.value()))); + [](Arena* arena, CelValue::BytesHolder value) -> CelValue { + if (UniLib::IsStructurallyValid(value.value())) { + return CelValue::CreateStringView(value.value()); + } + return CreateErrorValue(arena, "invalid UTF-8 bytes value", + absl::StatusCode::kInvalidArgument); }, registry); if (!status.ok()) return status; diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index 5b3f7b267..008f3bf08 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -1686,6 +1686,15 @@ TEST_F(BuiltinsTest, BytesToString) { EXPECT_EQ(result_value.StringOrDie().value(), "abcd"); } +TEST_F(BuiltinsTest, BytesToStringInvalid) { + std::string input = "\xFF"; + std::vector args = {CelValue::CreateBytes(&input)}; + CelValue result_value; + ASSERT_NO_FATAL_FAILURE( + PerformRun(builtin::kString, {}, args, &result_value)); + ASSERT_TRUE(result_value.IsError()); +} + TEST_F(BuiltinsTest, StringToString) { std::string input = "abcd"; std::vector args = {CelValue::CreateString(&input)}; diff --git a/eval/public/cel_value.h b/eval/public/cel_value.h index 73bbf2623..a7c34bdce 100644 --- a/eval/public/cel_value.h +++ b/eval/public/cel_value.h @@ -162,6 +162,9 @@ class CelValue { static CelValue CreateString(StringHolder holder) { return CelValue(holder); } + // Returns a string value from a string_view. Warning: the caller is + // responsible for the lifecycle of the backing string. Prefer CreateString + // instead. static CelValue CreateStringView(absl::string_view value) { return CelValue(StringHolder(value)); } diff --git a/testutil/BUILD b/testutil/BUILD index 450474c48..eb71cff9b 100644 --- a/testutil/BUILD +++ b/testutil/BUILD @@ -49,6 +49,7 @@ cc_library( cc_library( name = "test_data_io", + testonly = True, srcs = [ "test_data_io.cc", ], @@ -76,6 +77,7 @@ cc_library( # third_party/cel/spec/testdata/unique_values.textpb cc_binary( name = "test_data_gen", + testonly = True, srcs = [ "test_data_gen.cc", ], @@ -110,6 +112,7 @@ cc_test( cc_library( name = "util", + testonly = True, hdrs = [ "util.h", ],