fix: improve map rendering

Author: maltesander

rust/operator-binary/src/controller.rs

@@ -62,6 +62,7 @@ mod build;
 mod dereference;
 mod validate;
 
+use build::properties::render_java_properties;
 pub use validate::{TrinoRoleGroupConfig, ValidatedCluster};
 
 use crate::{
@@ -554,14 +555,9 @@ fn build_rolegroup_catalog_config_map(
             catalogs
                 .iter()
                 .map(|catalog| {
-                    let catalog_props: BTreeMap<String, String> = catalog
-                        .properties
-                        .iter()
-                        .map(|(k, v)| (k.to_string(), v.to_string()))
-                        .collect();
                     Ok((
                         format!("{}.properties", catalog.name),
-                        build::properties::to_java_properties_string(&catalog_props)
+                        render_java_properties(catalog.properties.clone())
                             .context(FailedToWriteJavaPropertiesSnafu)?,
                     ))
                 })

rust/operator-binary/src/controller/build/config_map.rs

@@ -19,7 +19,7 @@ use crate::{
         build::properties::{
             ConfigFileName, access_control_properties, config_properties,
             exchange_manager_properties, log_properties, logging::get_vector_toml, node_properties,
-            security_properties, spooling_manager_properties, to_java_properties_string,
+            render_java_properties, security_properties, spooling_manager_properties,
         },
     },
     crd::{TrinoRole, v1alpha1},
@@ -65,7 +65,6 @@ pub enum Error {
 
 type Result<T, E = Error> = std::result::Result<T, E>;
 
-#[allow(clippy::too_many_arguments)]
 pub fn build_rolegroup_config_map(
     cluster: &ValidatedCluster,
     role: &TrinoRole,
@@ -94,10 +93,9 @@ pub fn build_rolegroup_config_map(
     // Auth files (e.g. password-authenticator file contents) — inserted FIRST
     // to match the legacy precedence.
     for (file_name, props) in cluster.cluster_config.authentication.config_files(role) {
-        let rendered =
-            to_java_properties_string(&props).with_context(|_| WritePropertiesSnafu {
-                file: file_name.clone(),
-            })?;
+        let rendered = render_java_properties(props).with_context(|_| WritePropertiesSnafu {
+            file: file_name.clone(),
+        })?;
         data.insert(file_name, rendered);
     }
 
@@ -106,7 +104,7 @@ pub fn build_rolegroup_config_map(
         .context(BuildConfigPropertiesSnafu)?;
     data.insert(
         ConfigFileName::Config.to_string(),
-        to_java_properties_string(&cfg).with_context(|_| WritePropertiesSnafu {
+        render_java_properties(cfg).with_context(|_| WritePropertiesSnafu {
             file: ConfigFileName::Config.to_string(),
         })?,
     );
@@ -115,7 +113,7 @@ pub fn build_rolegroup_config_map(
     let node = node_properties::build(cluster, rg);
     data.insert(
         ConfigFileName::Node.to_string(),
-        to_java_properties_string(&node).with_context(|_| WritePropertiesSnafu {
+        render_java_properties(node).with_context(|_| WritePropertiesSnafu {
             file: ConfigFileName::Node.to_string(),
         })?,
     );
@@ -125,7 +123,7 @@ pub fn build_rolegroup_config_map(
     if !log.is_empty() {
         data.insert(
             ConfigFileName::Log.to_string(),
-            to_java_properties_string(&log).with_context(|_| WritePropertiesSnafu {
+            render_java_properties(log).with_context(|_| WritePropertiesSnafu {
                 file: ConfigFileName::Log.to_string(),
             })?,
         );
@@ -135,7 +133,7 @@ pub fn build_rolegroup_config_map(
     let sec = security_properties::build(rg);
     data.insert(
         ConfigFileName::Security.to_string(),
-        to_java_properties_string(&sec).with_context(|_| WritePropertiesSnafu {
+        render_java_properties(sec).with_context(|_| WritePropertiesSnafu {
             file: ConfigFileName::Security.to_string(),
         })?,
     );
@@ -145,7 +143,7 @@ pub fn build_rolegroup_config_map(
     if !ac.is_empty() {
         data.insert(
             ConfigFileName::AccessControl.to_string(),
-            to_java_properties_string(&ac).with_context(|_| WritePropertiesSnafu {
+            render_java_properties(ac).with_context(|_| WritePropertiesSnafu {
                 file: ConfigFileName::AccessControl.to_string(),
             })?,
         );
@@ -156,7 +154,7 @@ pub fn build_rolegroup_config_map(
     if !em.is_empty() {
         data.insert(
             ConfigFileName::ExchangeManager.to_string(),
-            to_java_properties_string(&em).with_context(|_| WritePropertiesSnafu {
+            render_java_properties(em).with_context(|_| WritePropertiesSnafu {
                 file: ConfigFileName::ExchangeManager.to_string(),
             })?,
         );
@@ -167,7 +165,7 @@ pub fn build_rolegroup_config_map(
     if !sm.is_empty() {
         data.insert(
             ConfigFileName::SpoolingManager.to_string(),
-            to_java_properties_string(&sm).with_context(|_| WritePropertiesSnafu {
+            render_java_properties(sm).with_context(|_| WritePropertiesSnafu {
                 file: ConfigFileName::SpoolingManager.to_string(),
             })?,
         );

rust/operator-binary/src/controller/build/properties/mod.rs

@@ -8,7 +8,7 @@
 use std::collections::BTreeMap;
 
 use stackable_operator::v2::{
-    config_file_writer::{self, PropertiesWriterError},
+    config_file_writer::{PropertiesWriterError, to_java_properties_string},
     config_overrides::KeyValueConfigOverrides,
 };
 
@@ -40,6 +40,19 @@ pub enum ConfigFileName {
     SpoolingManager,
 }
 
+/// Render a `key -> value` properties map to the Java `.properties` on-wire format.
+///
+/// The upstream [`to_java_properties_string`] consumes an iterator of
+/// `(&String, &Option<String>)`, so we lift the plain `String` values into
+/// `Some(..)` first.
+pub(crate) fn render_java_properties(
+    props: BTreeMap<String, String>,
+) -> Result<String, PropertiesWriterError> {
+    let props: BTreeMap<String, Option<String>> =
+        props.into_iter().map(|(k, v)| (k, Some(v))).collect();
+    to_java_properties_string(props.iter())
+}
+
 /// Keep only the set (`Some`) entries of a `key -> optional value` map, as `(key, value)` pairs.
 fn defined_entries(
     entries: BTreeMap<String, Option<String>>,
@@ -57,27 +70,6 @@ fn resolved_overrides(
     defined_entries(overrides.overrides)
 }
 
-/// Serialize `props` as a Java-properties string, sorted by key, via the shared
-/// [`config_file_writer`] — the same `product-config`-derived writer used by all
-/// other operators (and by this operator before the product-config refactoring;
-/// a hand-rolled escaper briefly lived here on this branch but accidentally
-/// diverged from it on non-ASCII and control characters, so it was dropped in
-/// favour of the shared implementation).
-///
-/// This adapter maps trino's `String`-valued property maps onto the writer's
-/// `Option<String>`-valued interface. Alternative considered: converting at
-/// each of the nine call sites — rejected as repetitive; the clone here is
-/// negligible for config-sized maps.
-pub fn to_java_properties_string(
-    props: &BTreeMap<String, String>,
-) -> Result<String, PropertiesWriterError> {
-    let props: BTreeMap<String, Option<String>> = props
-        .iter()
-        .map(|(k, v)| (k.clone(), Some(v.clone())))
-        .collect();
-    config_file_writer::to_java_properties_string(props.iter())
-}
-
 #[cfg(test)]
 pub(crate) mod test_support {
     use stackable_operator::cli::OperatorEnvironmentOptions;
@@ -138,7 +130,7 @@ mod tests {
             .iter()
             .map(|(k, v)| ((*k).to_string(), (*v).to_string()))
             .collect();
-        to_java_properties_string(&props).expect("rendering the test properties should succeed")
+        render_java_properties(props).expect("rendering the test properties should succeed")
     }
 
     /// The escape behaviours pinned by the kuttl smoke snapshot