From 40062192a027a37d3c129fa036c6a4a15f3a0e64 Mon Sep 17 00:00:00 2001 From: Dmitri Plotnikov Date: Fri, 27 Feb 2026 11:40:44 -0800 Subject: [PATCH] Introduce versioning for the CEL "strings" extension library. Tests are added to ensure that each function/macro is only available in the versions it's expected to be in. PiperOrigin-RevId: 876351348 --- extensions/BUILD | 3 ++ extensions/strings.cc | 31 +++++++---- extensions/strings.h | 10 ++-- extensions/strings_test.cc | 105 +++++++++++++++++++++++++++++++++++++ 4 files changed, 137 insertions(+), 12 deletions(-) diff --git a/extensions/BUILD b/extensions/BUILD index 35eea53b9..15b5305e9 100644 --- a/extensions/BUILD +++ b/extensions/BUILD @@ -602,11 +602,13 @@ cc_test( deps = [ ":strings", "//checker:standard_library", + "//checker:type_check_issue", "//checker:type_checker_builder", "//checker:validation_result", "//common:decl", "//common:type", "//common:value", + "//compiler", "//compiler:compiler_factory", "//compiler:standard_library", "//extensions/protobuf:runtime_adapter", @@ -620,6 +622,7 @@ cc_test( "//runtime:runtime_options", "//runtime:standard_runtime_builder_factory", "//testutil:baseline_tests", + "@com_google_absl//absl/algorithm:container", "@com_google_absl//absl/status", "@com_google_absl//absl/status:status_matchers", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", diff --git a/extensions/strings.cc b/extensions/strings.cc index 454644013..652c72572 100644 --- a/extensions/strings.cc +++ b/extensions/strings.cc @@ -181,7 +181,7 @@ const Type& ListStringType() { return *kInstance; } -absl::Status RegisterStringsDecls(TypeCheckerBuilder& builder) { +absl::Status RegisterStringsDecls(TypeCheckerBuilder& builder, int version) { // Runtime Supported functions. CEL_ASSIGN_OR_RETURN( auto join_decl, @@ -214,11 +214,6 @@ absl::Status RegisterStringsDecls(TypeCheckerBuilder& builder) { StringType(), StringType(), StringType(), StringType(), IntType()))); - CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(join_decl))); - CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(split_decl))); - CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(lower_decl))); - CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(replace_decl))); - // Additional functions described in the spec. CEL_ASSIGN_OR_RETURN( auto char_at_decl, @@ -277,17 +272,33 @@ absl::Status RegisterStringsDecls(TypeCheckerBuilder& builder) { MakeFunctionDecl("trim", MakeMemberOverloadDecl( "string_trim", StringType(), StringType()))); + CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(split_decl))); + CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(lower_decl))); + CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(replace_decl))); CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(char_at_decl))); CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(index_of_decl))); CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(last_index_of_decl))); CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(substring_decl))); CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(upper_ascii_decl))); + CEL_RETURN_IF_ERROR(builder.MergeFunction(std::move(trim_decl))); + if (version == 0) { + return absl::OkStatus(); + } + CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(format_decl))); CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(quote_decl))); + if (version == 1) { + return absl::OkStatus(); + } + + CEL_RETURN_IF_ERROR(builder.AddFunction(std::move(join_decl))); + if (version == 2) { + return absl::OkStatus(); + } + // MergeFunction is used to combine with the reverse function // defined in cel.lib.ext.lists extension. CEL_RETURN_IF_ERROR(builder.MergeFunction(std::move(reverse_decl))); - CEL_RETURN_IF_ERROR(builder.MergeFunction(std::move(trim_decl))); return absl::OkStatus(); } @@ -394,8 +405,10 @@ absl::Status RegisterStringsFunctions( google::api::expr::runtime::ConvertToRuntimeOptions(options)); } -CheckerLibrary StringsCheckerLibrary() { - return {"strings", &RegisterStringsDecls}; +CheckerLibrary StringsCheckerLibrary(int version) { + return {"strings", [version](TypeCheckerBuilder& builder) { + return RegisterStringsDecls(builder, version); + }}; } } // namespace cel::extensions diff --git a/extensions/strings.h b/extensions/strings.h index c5b7d1d63..5dab33c5d 100644 --- a/extensions/strings.h +++ b/extensions/strings.h @@ -25,6 +25,8 @@ namespace cel::extensions { +constexpr int kStringsExtensionLatestVersion = 4; + // Register extension functions for strings. absl::Status RegisterStringsFunctions(FunctionRegistry& registry, const RuntimeOptions& options); @@ -33,10 +35,12 @@ absl::Status RegisterStringsFunctions( google::api::expr::runtime::CelFunctionRegistry* registry, const google::api::expr::runtime::InterpreterOptions& options); -CheckerLibrary StringsCheckerLibrary(); +CheckerLibrary StringsCheckerLibrary( + int version = kStringsExtensionLatestVersion); -inline CompilerLibrary StringsCompilerLibrary() { - return CompilerLibrary::FromCheckerLibrary(StringsCheckerLibrary()); +inline CompilerLibrary StringsCompilerLibrary( + int version = kStringsExtensionLatestVersion) { + return CompilerLibrary::FromCheckerLibrary(StringsCheckerLibrary(version)); } } // namespace cel::extensions diff --git a/extensions/strings_test.cc b/extensions/strings_test.cc index af8ae4794..a5d56eaed 100644 --- a/extensions/strings_test.cc +++ b/extensions/strings_test.cc @@ -17,16 +17,20 @@ #include #include #include +#include #include "cel/expr/syntax.pb.h" +#include "absl/algorithm/container.h" #include "absl/status/status.h" #include "absl/status/status_matchers.h" #include "checker/standard_library.h" +#include "checker/type_check_issue.h" #include "checker/type_checker_builder.h" #include "checker/validation_result.h" #include "common/decl.h" #include "common/type.h" #include "common/value.h" +#include "compiler/compiler.h" #include "compiler/compiler_factory.h" #include "compiler/standard_library.h" #include "extensions/protobuf/runtime_adapter.h" @@ -49,7 +53,10 @@ using ::absl_testing::IsOk; using ::cel::expr::ParsedExpr; using ::google::api::expr::parser::Parse; using ::google::api::expr::parser::ParserOptions; +using ::testing::HasSubstr; +using ::testing::IsEmpty; using ::testing::Values; +using ::testing::ValuesIn; TEST(StringsCheckerLibrary, SmokeTest) { ASSERT_OK_AND_ASSIGN( @@ -320,5 +327,103 @@ INSTANTIATE_TEST_SUITE_P(EvaluationErrors, StringsRuntimeErrorTest, "'a'.substring(0, -1)", "'a'.substring(0, 2)", "'a'.substring(1, 0)")); +struct StringsExtensionVersionTestCase { + std::string expr; + std::vector expected_supported_versions; +}; + +class StringsExtensionVersionTest + : public ::testing::TestWithParam {}; + +TEST_P(StringsExtensionVersionTest, StringsExtensionVersions) { + const StringsExtensionVersionTestCase& test_case = GetParam(); + for (int version = 0; + version <= cel::extensions::kStringsExtensionLatestVersion; ++version) { + CompilerLibrary compiler_library = StringsCompilerLibrary(version); + + ASSERT_OK_AND_ASSIGN( + std::unique_ptr builder, + cel::NewCompilerBuilder(internal::GetTestingDescriptorPool(), + CompilerOptions())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->AddLibrary(std::move(compiler_library)), IsOk()); + + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, builder->Build()); + ASSERT_OK_AND_ASSIGN(ValidationResult result, + compiler->Compile(test_case.expr)); + if (absl::c_contains(test_case.expected_supported_versions, version)) { + EXPECT_THAT(result.GetIssues(), IsEmpty()) + << "Expected no issues for expr: " << test_case.expr + << " at version: " << version << " but got: " << result.FormatError(); + } else { + EXPECT_THAT(result.GetIssues(), + Contains(Property(&TypeCheckIssue::message, + HasSubstr("undeclared reference")))); + } + } +}; + +std::vector +CreateStringsExtensionVersionParams() { + return { + StringsExtensionVersionTestCase{ + .expr = "'foo'.charAt(0)", + .expected_supported_versions = {0, 1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'foo'.indexOf('f')", + .expected_supported_versions = {0, 1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'foo'.lastIndexOf('f')", + .expected_supported_versions = {0, 1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'foo'.lowerAscii()", + .expected_supported_versions = {0, 1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'foo'.replace('f', 'b')", + .expected_supported_versions = {0, 1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'foo'.split('o')", + .expected_supported_versions = {0, 1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'foo'.substring(0, 1)", + .expected_supported_versions = {0, 1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'foo'.trim()", + .expected_supported_versions = {0, 1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'foo'.upperAscii()", + .expected_supported_versions = {0, 1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'%d'.format([1])", + .expected_supported_versions = {1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "strings.quote('foo')", + .expected_supported_versions = {1, 2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "['a', 'b', 'c'].join(',')", + .expected_supported_versions = {2, 3, 4}, + }, + StringsExtensionVersionTestCase{ + .expr = "'foo'.reverse()", + .expected_supported_versions = {3, 4}, + }, + }; +} + +INSTANTIATE_TEST_SUITE_P(StringsExtensionVersionTest, + StringsExtensionVersionTest, + ValuesIn(CreateStringsExtensionVersionParams())); + } // namespace } // namespace cel::extensions