From 3274aaf6ca73cb158e98f4a562477128d10733a4 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 11 Nov 2026 09:49:09 -0800 Subject: [PATCH] Treat an unspecified extension version as version 0 in cel::Config and the env YAML parser, consistent with cel-go and cel-java. PiperOrigin-RevId: 1019273261 --- env/config.cc | 19 +++++---- env/config.h | 9 ++-- env/config_test.cc | 29 +++++++++++-- env/env_std_extensions_test.cc | 6 ++- env/env_test.cc | 48 +++++++++++++++++++--- env/env_yaml.cc | 31 +++++++++----- env/env_yaml.h | 16 +++++++- env/env_yaml_test.cc | 75 ++++++++++++++++++++++++++++++++-- 8 files changed, 196 insertions(+), 37 deletions(-) diff --git a/env/config.cc b/env/config.cc index 202a607bf..89120b5cb 100644 --- a/env/config.cc +++ b/env/config.cc @@ -35,6 +35,11 @@ namespace cel { namespace { +std::string VersionToString(int version) { + return version == Config::ExtensionConfig::kLatest ? "'latest'" + : absl::StrCat(version); +} + const char* ConstantKindToTypeName(const ConstantKind& kind) { return std::visit(absl::Overload{ [](const std::monostate& arg) { return "dyn"; }, @@ -58,15 +63,11 @@ absl::Status Config::AddExtensionConfig(std::string name, int version) { if (extension_config.version == version) { return absl::OkStatus(); } - std::string version_str; - if (version == ExtensionConfig::kLatest) { - version_str = "'latest'"; - } else { - version_str = absl::StrCat(version); - } - return absl::AlreadyExistsError(absl::StrCat( - "Extension '", name, "' version ", extension_config.version, - " is already included. Cannot also include version ", version_str)); + return absl::AlreadyExistsError( + absl::StrCat("Extension '", name, "' version ", + VersionToString(extension_config.version), + " is already included. Cannot also include version ", + VersionToString(version))); } } extension_configs_.push_back( diff --git a/env/config.h b/env/config.h index 68e4a1dd9..f8b1dfb3b 100644 --- a/env/config.h +++ b/env/config.h @@ -60,14 +60,17 @@ class Config { } struct ExtensionConfig { + // Sentinel requesting the newest registered version of the extension. static constexpr int kLatest = std::numeric_limits::max(); std::string name; - int version = kLatest; + // Defaults to version 0, consistent with cel-go and cel-java. kLatest + // requests the newest registered version. + int version = 0; }; - absl::Status AddExtensionConfig(std::string name, - int version = ExtensionConfig::kLatest); + // Adds an extension config. If version is omitted, defaults to version 0. + absl::Status AddExtensionConfig(std::string name, int version = 0); const std::vector& GetExtensionConfigs() const { return extension_configs_; diff --git a/env/config_test.cc b/env/config_test.cc index 8cfc3cf7f..ade83fac2 100644 --- a/env/config_test.cc +++ b/env/config_test.cc @@ -48,16 +48,37 @@ TEST(EnvConfigTest, ExtensionConfigs) { AllOf(Field(&Config::ExtensionConfig::name, "optional"), Field(&Config::ExtensionConfig::version, 2)), AllOf(Field(&Config::ExtensionConfig::name, "strings"), - Field(&Config::ExtensionConfig::version, - Config::ExtensionConfig::kLatest)))); + Field(&Config::ExtensionConfig::version, 0)))); } TEST(EnvConfigTest, ExtensionConfigConflict) { Config config; ASSERT_THAT(config.AddExtensionConfig("math", 2), IsOk()); ASSERT_THAT(config.AddExtensionConfig("math", 2), IsOk()); - ASSERT_THAT(config.AddExtensionConfig("math", 3), - StatusIs(absl::StatusCode::kAlreadyExists)); + EXPECT_THAT(config.AddExtensionConfig("math", 3), + StatusIs(absl::StatusCode::kAlreadyExists, + HasSubstr("Extension 'math' version 2 is already " + "included. Cannot also include version 3"))); + + Config config2; + ASSERT_THAT( + config2.AddExtensionConfig("strings", Config::ExtensionConfig::kLatest), + IsOk()); + EXPECT_THAT( + config2.AddExtensionConfig("strings", 0), + StatusIs( + absl::StatusCode::kAlreadyExists, + HasSubstr("Extension 'strings' version 'latest' is already included. " + "Cannot also include version 0"))); + + Config config3; + ASSERT_THAT(config3.AddExtensionConfig("strings", 0), IsOk()); + EXPECT_THAT( + config3.AddExtensionConfig("strings", Config::ExtensionConfig::kLatest), + StatusIs( + absl::StatusCode::kAlreadyExists, + HasSubstr("Extension 'strings' version 0 is already included. Cannot " + "also include version 'latest'"))); } struct StandardLibraryConfigTestCase { diff --git a/env/env_std_extensions_test.cc b/env/env_std_extensions_test.cc index 7d9572cc0..6edd9b21d 100644 --- a/env/env_std_extensions_test.cc +++ b/env/env_std_extensions_test.cc @@ -45,7 +45,11 @@ TEST_P(EnvStdExtensions, RegistrationTest) { env.SetDescriptorPool(internal::GetSharedTestingDescriptorPool()); Config config; - ASSERT_THAT(config.AddExtensionConfig(param.extension), IsOk()); + // These test cases exercise features from the newest version of each + // extension. + ASSERT_THAT(config.AddExtensionConfig(param.extension, + Config::ExtensionConfig::kLatest), + IsOk()); env.SetConfig(config); ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, env.NewCompiler()); diff --git a/env/env_test.cc b/env/env_test.cc index 00143a857..822f1507f 100644 --- a/env/env_test.cc +++ b/env/env_test.cc @@ -150,6 +150,8 @@ absl::StatusOr CompileAndEvalBooleanExpr( class LibraryConfigTest : public testing::Test { protected: void SetUp() override { + env_.RegisterCompilerLibrary("testlib", "ml", 0, + []() { return TestLibrary(0); }); env_.RegisterCompilerLibrary("testlib", "ml", 1, []() { return TestLibrary(1); }); env_.RegisterCompilerLibrary("testlib", "ml", 2, @@ -160,17 +162,23 @@ class LibraryConfigTest : public testing::Test { Env env_; }; -TEST_F(LibraryConfigTest, DefaultVersion) { +TEST_F(LibraryConfigTest, LatestVersion) { Config config; - ASSERT_THAT(config.AddExtensionConfig("testlib"), IsOk()); + ASSERT_THAT( + config.AddExtensionConfig("testlib", Config::ExtensionConfig::kLatest), + IsOk()); env_.SetConfig(config); ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, env_.NewCompiler()); - ASSERT_OK_AND_ASSIGN(auto result1, compiler->Compile("testMacro1()")); - ASSERT_OK_AND_ASSIGN(auto result2, compiler->Compile("testFunc1()")); - ASSERT_OK_AND_ASSIGN(auto result3, compiler->Compile("testMacro2()")); - ASSERT_OK_AND_ASSIGN(auto result4, compiler->Compile("testFunc2()")); + ASSERT_OK_AND_ASSIGN(ValidationResult result1, + compiler->Compile("testMacro1()")); + ASSERT_OK_AND_ASSIGN(ValidationResult result2, + compiler->Compile("testFunc1()")); + ASSERT_OK_AND_ASSIGN(ValidationResult result3, + compiler->Compile("testMacro2()")); + ASSERT_OK_AND_ASSIGN(ValidationResult result4, + compiler->Compile("testFunc2()")); EXPECT_THAT(result1.GetIssues(), IsEmpty()); EXPECT_THAT(result2.GetIssues(), IsEmpty()); @@ -178,6 +186,34 @@ TEST_F(LibraryConfigTest, DefaultVersion) { EXPECT_THAT(result4.GetIssues(), IsEmpty()); } +TEST_F(LibraryConfigTest, DefaultVersionIsZero) { + Config config; + ASSERT_THAT(config.AddExtensionConfig("testlib"), IsOk()); + + env_.SetConfig(config); + + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, env_.NewCompiler()); + ASSERT_OK_AND_ASSIGN(ValidationResult result1, + compiler->Compile("testMacro1()")); + ASSERT_OK_AND_ASSIGN(ValidationResult result2, + compiler->Compile("testFunc1()")); + ASSERT_OK_AND_ASSIGN(ValidationResult result3, + compiler->Compile("testMacro2()")); + ASSERT_OK_AND_ASSIGN(ValidationResult result4, + compiler->Compile("testFunc2()")); + + EXPECT_THAT(result1.GetIssues(), IsEmpty()); + EXPECT_THAT(result2.GetIssues(), IsEmpty()); + EXPECT_THAT(result3.GetIssues(), + UnorderedElementsAre( + Property(&TypeCheckIssue::message, + HasSubstr("undeclared reference to 'testMacro2'")))); + EXPECT_THAT(result4.GetIssues(), + UnorderedElementsAre( + Property(&TypeCheckIssue::message, + HasSubstr("undeclared reference to 'testFunc2'")))); +} + TEST_F(LibraryConfigTest, SpecificVersion) { Config config; ASSERT_THAT(config.AddExtensionConfig("testlib", 1), IsOk()); diff --git a/env/env_yaml.cc b/env/env_yaml.cc index 281cf3ff1..a7276a918 100644 --- a/env/env_yaml.cc +++ b/env/env_yaml.cc @@ -236,7 +236,8 @@ absl::Status ParseContainerConfig(Config& config, absl::string_view yaml, } absl::Status ParseExtensionConfigs(Config& config, absl::string_view yaml, - const YAML::Node& root) { + const YAML::Node& root, + const EnvConfigFromYamlOptions& options) { const YAML::Node extensions = root["extensions"]; if (!extensions.IsDefined()) { return absl::OkStatus(); @@ -256,9 +257,11 @@ absl::Status ParseExtensionConfigs(Config& config, absl::string_view yaml, std::string name_str = GetString(yaml, name); const YAML::Node version = extension["version"]; - std::string version_str = GetString(yaml, version); - int extension_version; + int extension_version = options.legacy_default_extension_version_latest + ? Config::ExtensionConfig::kLatest + : 0; if (version.IsDefined()) { + std::string version_str = GetString(yaml, version); bool is_valid_version = false; if (version.IsScalar()) { if (version_str == "latest") { @@ -277,8 +280,6 @@ absl::Status ParseExtensionConfigs(Config& config, absl::string_view yaml, absl::StrCat("Extension '", name_str, "' version is not a valid number or 'latest'")); } - } else { - extension_version = Config::ExtensionConfig::kLatest; } absl::Status add_status = config.AddExtensionConfig(name_str, extension_version); @@ -876,7 +877,8 @@ void EmitContainerConfig(const Config& env_config, YAML::Emitter& out) { } } -void EmitExtensionConfigs(const Config& env_config, YAML::Emitter& out) { +void EmitExtensionConfigs(const Config& env_config, YAML::Emitter& out, + const EnvConfigToYamlOptions& options) { if (env_config.GetExtensionConfigs().empty()) { return; } @@ -894,7 +896,15 @@ void EmitExtensionConfigs(const Config& env_config, YAML::Emitter& out) { out << YAML::BeginMap; out << YAML::Key << "name"; out << YAML::Value << YAML::DoubleQuoted << extension_config.name; - if (extension_config.version != Config::ExtensionConfig::kLatest) { + if (options.legacy_omit_latest_extension_version) { + if (extension_config.version != Config::ExtensionConfig::kLatest) { + out << YAML::Key << "version"; + out << YAML::Value << extension_config.version; + } + } else if (extension_config.version == Config::ExtensionConfig::kLatest) { + out << YAML::Key << "version"; + out << YAML::Value << "latest"; + } else if (extension_config.version != 0) { out << YAML::Key << "version"; out << YAML::Value << extension_config.version; } @@ -1280,7 +1290,8 @@ absl::Status ParseContextVariableConfig(Config& config, absl::string_view yaml, } // namespace -absl::StatusOr EnvConfigFromYaml(const std::string& yaml) { +absl::StatusOr EnvConfigFromYaml( + const std::string& yaml, const EnvConfigFromYamlOptions& options) { Config config; CEL_ASSIGN_OR_RETURN(YAML::Node root, LoadYaml(yaml)); if (!root.IsDefined() || root.IsNull()) { @@ -1294,7 +1305,7 @@ absl::StatusOr EnvConfigFromYaml(const std::string& yaml) { CEL_RETURN_IF_ERROR(ParseName(config, yaml, root)); CEL_RETURN_IF_ERROR(ParseContainerConfig(config, yaml, root)); - CEL_RETURN_IF_ERROR(ParseExtensionConfigs(config, yaml, root)); + CEL_RETURN_IF_ERROR(ParseExtensionConfigs(config, yaml, root, options)); CEL_RETURN_IF_ERROR(ParseStandardLibraryConfig(config, yaml, root)); CEL_RETURN_IF_ERROR(ParseContextVariableConfig(config, yaml, root)); CEL_RETURN_IF_ERROR(ParseVariableConfigs(config, yaml, root)); @@ -1312,7 +1323,7 @@ void EnvConfigToYaml(const Config& env_config, std::ostream& os, out << YAML::Value << YAML::DoubleQuoted << env_config.GetName(); } EmitContainerConfig(env_config, out); - EmitExtensionConfigs(env_config, out); + EmitExtensionConfigs(env_config, out, options); EmitStandardLibraryConfig(env_config, out); EmitVariableConfigs(env_config, out, options); EmitFunctionConfigs(env_config, out, options); diff --git a/env/env_yaml.h b/env/env_yaml.h index 7bf7bf6b4..86ef5cd2e 100644 --- a/env/env_yaml.h +++ b/env/env_yaml.h @@ -23,13 +23,21 @@ namespace cel { +struct EnvConfigFromYamlOptions { + // Whether to treat an omitted extension `version` as + // `Config::ExtensionConfig::kLatest` instead of version 0. + // Temporary rollout option; will be removed once callers migrate. + bool legacy_default_extension_version_latest = false; +}; + // EnvConfigFromYaml creates an environment configuration from a YAML string. // // To ensure safety, only pass trusted YAML input. yaml-cpp has some fuzz // coverage, but its security model is unclear. Additionally, callers should be // aware that improper CEL configuration can lead to unsafe or unpredictably // expensive expressions. -absl::StatusOr EnvConfigFromYaml(const std::string& yaml); +absl::StatusOr EnvConfigFromYaml( + const std::string& yaml, const EnvConfigFromYamlOptions& options = {}); struct EnvConfigToYamlOptions { // Whether to use type and overload signatures instead of arg/return types in @@ -63,6 +71,12 @@ struct EnvConfigToYamlOptions { // type_name: "int" // TODO(uncreated-issue/91): default to true after all dependencies are updated bool use_type_signatures = false; + + // Whether to omit `version` when it is `Config::ExtensionConfig::kLatest` and + // emit `version: 0` explicitly instead of emitting `version: latest` and + // omitting version 0. + // Temporary rollout option; will be removed once callers migrate. + bool legacy_omit_latest_extension_version = false; }; // EnvConfigToYaml serializes an environment configuration as a YAML string. diff --git a/env/env_yaml_test.cc b/env/env_yaml_test.cc index c5bd1b787..7bf70a091 100644 --- a/env/env_yaml_test.cc +++ b/env/env_yaml_test.cc @@ -88,6 +88,8 @@ TEST(EnvYamlTest, ParseExtensionConfigs) { - name: "optional" version: 2 - name: "strings" + - name: "lists" + version: 0 )yaml")); EXPECT_THAT(config.GetExtensionConfigs(), @@ -98,8 +100,9 @@ TEST(EnvYamlTest, ParseExtensionConfigs) { AllOf(Field(&Config::ExtensionConfig::name, "optional"), Field(&Config::ExtensionConfig::version, 2)), AllOf(Field(&Config::ExtensionConfig::name, "strings"), - Field(&Config::ExtensionConfig::version, - Config::ExtensionConfig::kLatest)))); + Field(&Config::ExtensionConfig::version, 0)), + AllOf(Field(&Config::ExtensionConfig::name, "lists"), + Field(&Config::ExtensionConfig::version, 0)))); } TEST(EnvYamlTest, DefaultExtensionConfigs) { @@ -109,6 +112,69 @@ TEST(EnvYamlTest, DefaultExtensionConfigs) { EXPECT_THAT(config.GetExtensionConfigs(), IsEmpty()); } +TEST(EnvYamlTest, RoundTripExtensionVersions) { + const std::string yaml = R"yaml( + extensions: + - name: "math" + - name: "lists" + version: 0 + - name: "strings" + version: latest + - name: "optional" + version: 2 + )yaml"; + ASSERT_OK_AND_ASSIGN(Config config, EnvConfigFromYaml(yaml)); + std::stringstream ss; + EnvConfigToYaml(config, ss); + ASSERT_OK_AND_ASSIGN(Config round_tripped_config, + EnvConfigFromYaml(ss.str())); + EXPECT_THAT(round_tripped_config.GetExtensionConfigs(), + UnorderedElementsAre( + AllOf(Field(&Config::ExtensionConfig::name, "math"), + Field(&Config::ExtensionConfig::version, 0)), + AllOf(Field(&Config::ExtensionConfig::name, "lists"), + Field(&Config::ExtensionConfig::version, 0)), + AllOf(Field(&Config::ExtensionConfig::name, "strings"), + Field(&Config::ExtensionConfig::version, + Config::ExtensionConfig::kLatest)), + AllOf(Field(&Config::ExtensionConfig::name, "optional"), + Field(&Config::ExtensionConfig::version, 2)))); +} + +TEST(EnvYamlTest, LegacyExtensionVersionOptions) { + const std::string yaml = R"yaml( + extensions: + - name: "math" + - name: "lists" + version: 0 + - name: "optional" + version: 2 + )yaml"; + ASSERT_OK_AND_ASSIGN( + Config config, + EnvConfigFromYaml(yaml, + {.legacy_default_extension_version_latest = true})); + EXPECT_THAT(config.GetExtensionConfigs(), + UnorderedElementsAre( + AllOf(Field(&Config::ExtensionConfig::name, "math"), + Field(&Config::ExtensionConfig::version, + Config::ExtensionConfig::kLatest)), + AllOf(Field(&Config::ExtensionConfig::name, "lists"), + Field(&Config::ExtensionConfig::version, 0)), + AllOf(Field(&Config::ExtensionConfig::name, "optional"), + Field(&Config::ExtensionConfig::version, 2)))); + + std::stringstream ss; + EnvConfigToYaml(config, ss, {.legacy_omit_latest_extension_version = true}); + EXPECT_EQ(ss.str(), + "extensions:\n" + " - name: \"lists\"\n" + " version: 0\n" + " - name: \"math\"\n" + " - name: \"optional\"\n" + " version: 2"); +} + TEST(EnvYamlTest, ParseStdlibConfig_ExclusionStyle) { ASSERT_OK_AND_ASSIGN(Config config, EnvConfigFromYaml(R"yaml( stdlib: @@ -1303,6 +1369,8 @@ std::vector GetExportTestCases() { CEL_RETURN_IF_ERROR(config.AddExtensionConfig("math")); CEL_RETURN_IF_ERROR(config.AddExtensionConfig("optional", 2)); CEL_RETURN_IF_ERROR(config.AddExtensionConfig("bindings")); + CEL_RETURN_IF_ERROR(config.AddExtensionConfig( + "strings", Config::ExtensionConfig::kLatest)); return config; }(), .expected_yaml = R"yaml( @@ -1311,6 +1379,8 @@ std::vector GetExportTestCases() { - name: "math" - name: "optional" version: 2 + - name: "strings" + version: latest )yaml", }, ExportTestCase{ @@ -1813,7 +1883,6 @@ std::vector GetStructuredRoundTripTestCases() { container: "common.proto.prefix" extensions: - name: "math" - version: 0 - name: "optional" version: 2 stdlib: