From e54eb00b95698ec91f5d76552f15da5bdff4f140 Mon Sep 17 00:00:00 2001 From: ganthern Date: Wed, 1 Jan 2025 22:10:51 +0100 Subject: [PATCH 1/8] feat: add variable interpolation in compose files Introduce the `Options::interpolate_vars()` function to add name-value mappings that will be used to replace variable placeholders in YAML strings when `Compose` is constructed. The behavior is unchanged if no variables are given: strings containing placeholders are used as-is and may lead to errors if they don't pass validation (which is likely, since they'll contain constructs like `${FOO:-${BAR?missing foo & bar}}` ). If any variables are given to `Options`, the parser will be invoked and attempt to replace all of the variables in the YAML source and throw an error if it is unable to. --- src/lib.rs | 1 + src/options.rs | 66 +++- src/variable_interpolation.rs | 32 ++ src/variable_interpolation/error.rs | 66 ++++ src/variable_interpolation/parser.rs | 500 ++++++++++++++++++++++++ src/variable_interpolation/yaml_walk.rs | 42 ++ 6 files changed, 703 insertions(+), 4 deletions(-) create mode 100644 src/variable_interpolation.rs create mode 100644 src/variable_interpolation/error.rs create mode 100644 src/variable_interpolation/parser.rs create mode 100644 src/variable_interpolation/yaml_walk.rs diff --git a/src/lib.rs b/src/lib.rs index 02addf8..788f64b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -105,6 +105,7 @@ mod options; pub mod secret; mod serde; pub mod service; +mod variable_interpolation; mod volume; use std::{ diff --git a/src/options.rs b/src/options.rs index 98bd7a6..a98bee6 100644 --- a/src/options.rs +++ b/src/options.rs @@ -1,8 +1,11 @@ //! [`Options`] builder for deserialization options for a [`Compose`] file. -use std::io::Read; +use std::{collections::HashMap, io::Read}; -use crate::{Compose, YamlValue}; +use crate::{ + variable_interpolation::{yaml_walk::interpolate_value, VariableResolver}, + Compose, YamlValue, +}; /// Deserialization options builder for a [`Compose`] file. #[allow(missing_copy_implementations)] // Will include interpolation vars as a HashMap. @@ -10,6 +13,8 @@ use crate::{Compose, YamlValue}; pub struct Options { /// Whether to perform merging of `<<` keys. apply_merge: bool, + /// the variables to perform environment variable interpolation on, if any + interpolate_vars: Option, } impl Options { @@ -49,10 +54,58 @@ impl Options { self } + /// Add a map of values to interpolate variable placeholders in YAML before constructing the `Compose` + /// instance. + /// The given `HashMap` could be sourced from dotenv files or the processes' environment, or both. + /// Can be called multiple times to merge different variable sources. + /// The last mapping for any given variable name wins. + /// + /// ``` + /// use compose_spec::Compose; + /// use std::collections::HashMap; + /// + /// let yaml = "\ + /// services: + /// one: + /// environment: + /// FOO: $FOO # simple variable without default + /// BAR: ${BAR-Bar-Default} # braced variable with default value + /// BAZ: $$ESCAPED # double-$ to write a literal $ + /// "; + /// + /// let vars = HashMap::from([ + /// ("FOO".into(), "Foo-Value".into()) + /// ]); + /// + /// let compose = Compose::options() + /// .interpolate_vars(vars) + /// .from_yaml_str(yaml) + /// .unwrap(); + /// + /// let one_env = compose.services["one"] + /// .environment + /// .clone() + /// .into_map() + /// .unwrap(); + /// + /// assert_eq!(one_env["FOO"].as_ref().unwrap().as_string().unwrap(), "Foo-Value"); + /// assert_eq!(one_env["BAR"].as_ref().unwrap().as_string().unwrap(), "Bar-Default"); + /// assert_eq!(one_env["BAZ"].as_ref().unwrap().as_string().unwrap(), "$ESCAPED"); + /// ``` + pub fn interpolate_vars(&mut self, vars: HashMap) -> &mut Self { + let mut resolver = self.interpolate_vars.take().unwrap_or_default(); + resolver.add_vars(vars); + let _ = self.interpolate_vars.replace(resolver); + self + } + /// Return `true` if any options are set. const fn any(&self) -> bool { - let Self { apply_merge } = *self; - apply_merge + let Self { + apply_merge, + interpolate_vars, + } = self; + *apply_merge || interpolate_vars.is_some() } /// Use the set options to deserialize a [`Compose`] file from a string slice of YAML. @@ -103,6 +156,11 @@ impl Options { if self.apply_merge { value.apply_merge()?; } + + if let Some(vars) = &self.interpolate_vars { + interpolate_value(vars, &mut value)?; + } + serde_yaml::from_value(value) } } diff --git a/src/variable_interpolation.rs b/src/variable_interpolation.rs new file mode 100644 index 0000000..3f882cd --- /dev/null +++ b/src/variable_interpolation.rs @@ -0,0 +1,32 @@ +//! Implementation of the environment variable interpolation as defined in the +//! [compose spec](https://github.com/compose-spec/compose-spec/blob/main/12-interpolation.md) +//! +//! `YamlValues` are interpolated in-place. + +use std::collections::HashMap; + +mod error; +mod parser; +pub(crate) mod yaml_walk; + +#[derive(Debug, Default, Clone, PartialEq, Eq)] +/// Resolves values for a given key from the relevant sources (.env files, process environment, ...) +pub(crate) struct VariableResolver { + /// map of all resolvable variable names + vars: HashMap, +} + +impl VariableResolver { + /// add some name <-> value mappings to the interpolation. last definition of a name wins. + pub(crate) fn add_vars(&mut self, new_vars: HashMap) -> &mut Self { + for (key, value) in new_vars { + let _ = self.vars.insert(key, value); + } + self + } + + /// get the value associated with the given key. + pub(crate) fn get(&self, key: impl AsRef) -> Option<&str> { + self.vars.get(key.as_ref()).map(String::as_str) + } +} diff --git a/src/variable_interpolation/error.rs b/src/variable_interpolation/error.rs new file mode 100644 index 0000000..98e1a69 --- /dev/null +++ b/src/variable_interpolation/error.rs @@ -0,0 +1,66 @@ +//! contains the error type used by the variable interpolation parser +//! +//! since the calling code expects the error type to be serde_yaml::Error, +//! `ParseError` can be converted to that type, using the error information +//! to construct a custom error message + +use serde::de::Error; + +/// the error type used for variable interpolation +#[derive(Debug)] +pub(super) struct ParseError { + /// error message displayed to the user + message: String, + /// the current output string at the time of failure + output: String, + /// the rest of the input at the time of failure + rest: String, +} + +impl ParseError { + /// construct a new error. + /// `message` should be a human-readable error message. + /// `output` is the current state of the parser's output buffer. + /// `rest` contains the rest of the parser's unparsed input. + pub(super) const fn new(message: String, output: String, rest: String) -> Self { + Self { + message, + output, + rest, + } + } +} + +impl From for serde_yaml::Error { + fn from(value: ParseError) -> Self { + let ParseError { + message, + output, + rest, + } = value; + let msg = format!("failed ({message}) around {output} ... {rest}"); + Self::custom(msg) + } +} + +/// convenience macro to quickly construct and return a parsing error +/// from a message and the current parser state. +/// the first argument must be the parser, from the second argument onwards +/// it works like format!() +macro_rules! terminate { + ($slf:ident, $msg:literal $(,)? $($fragments:expr),*) => {{ + let slf: &crate::variable_interpolation::parser::Parser = $slf; + let message: ::std::string::String = format!($msg, $($fragments),* ); + let output: ::std::string::String = slf.output.to_owned(); + let rest: ::std::string::String = slf.rest.clone().collect(); + return ::std::result::Result::Err( + crate::variable_interpolation::error::ParseError::new( + message, + output, + rest, + ), + ); + }}; +} + +pub(super) use terminate; diff --git a/src/variable_interpolation/parser.rs b/src/variable_interpolation/parser.rs new file mode 100644 index 0000000..81aded8 --- /dev/null +++ b/src/variable_interpolation/parser.rs @@ -0,0 +1,500 @@ +//! the parsing and interpolation logic being applied to the yaml strings +use super::error::terminate; +use super::error::ParseError; +use super::VariableResolver; +use std::{iter::Peekable, str::Chars}; + +/// the result type for the parser +type Result = std::result::Result<(), ParseError>; + +#[derive(Debug, Clone)] +/// the struct containing the state of the parser during the parsing process +pub(super) struct Parser<'s> { + /// the current rest of the input source + rest: Peekable>, + /// the current state of the output + output: String, + /// the source of the variable values + env: &'s VariableResolver, +} + +#[derive(Debug, Clone)] +/// variables can be modified to replace an empty or unset variable with a default value +/// or to replace a generic failure with a better error message. +/// encodes all the ways a braced variable can be modified: +/// - "NAME-DEFAULT": use DEFAULT if variable NAME is not set +/// - "NAME:-DEFAULT": use DEFAULT if variable NAME is not set or empty +/// - "NAME?MESSAGE": fail with MESSAGE if variable NAME is not set +/// - "NAME:?MESSAGE": fail with MESSAGE if variabele NAME is not set or empty +enum Modifier { + /// if the condition matches, replace the failure with the error message + ErrorMessage(ModifierCondition, String), + /// if the condition matches, replace the variable name with the default + DefaultValue(ModifierCondition, String), +} + +#[derive(Debug, Clone)] +/// encodes when the modifier value should be applied +enum ModifierCondition { + /// apply when the variable is not set or when it is empty + WhenUnsetOrEmpty, + /// apply only when the variable is not set + WhenUnset, +} + +impl<'s> Parser<'s> { + /// parse the given `String` and return a new `String`, replacing any encountered + /// variables with the value returned by `vars` + pub(super) fn start(vars: &'s VariableResolver, source: &'s str) -> serde_yaml::Result { + let mut parser = Self { + rest: source.chars().peekable(), + output: String::with_capacity(source.len()), + env: vars, + }; + parser + .parse_string() + .map_err(Into::::into)?; + Ok(parser.output) + } + + /// entry point into the parsing logic. after this returns Ok, self.output + /// contains the interpolated input string. + fn parse_string(&mut self) -> Result { + loop { + if self.rest.peek().is_none() { + // nothing to parse anymore, we're done + break; + } + + self.parse_variable_start()?; + self.parse_literal(); + } + + Ok(()) + } + + /// parse either the start of a variable name ($) or an escaped dollar sign ($$) + fn parse_variable_start(&mut self) -> Result { + // check that the iterator points to a $ and consume it + match self.rest.peek() { + Some('$') => { + let _ = self.rest.next(); + } + _ => return Ok(()), + } + + // check if it's escaped. if so, consume & push $ to output and return. + // if not, continue with a braced or non-braced variable. + match self.rest.peek() { + Some('$') => { + let _ = self.rest.next(); + self.output.push('$'); + Ok(()) + } + Some('{') => self.parse_braced_variable_start(), + _ => self.parse_variable_name(), + } + } + + /// parse a character that's not part of a variable name and not a `$` + fn parse_literal(&mut self) { + match self.rest.peek() { + Some('$') | None => (), + Some(character) => { + self.output.push(*character); + let _ = self.rest.next(); + } + } + } + + /// parse a braced variable: `{NAME}` + fn parse_braced_variable_start(&mut self) -> Result { + // check that the iterator points to a `{` + match self.rest.next() { + Some('{') => {} + Some(character) => terminate!(self, "expected {{, got {}", character), + None => terminate!(self, "input ended unexpectedly"), + } + self.parse_braced_variable_name() + } + + /// parse the name of an unbraced variable and replace it with its value + fn parse_variable_name(&mut self) -> Result { + let mut name = String::new(); + match self.rest.next() { + Some(character @ ('_' | 'a'..='z' | 'A'..='Z')) => name.push(character), + Some(character) => { + terminate!( + self, + "expected one of $, {{, _, a-z, A-Z, got {}", + character + ) + } + None => terminate!(self, "input ended unexpectedly"), + } + + // consume & push until we know the variable name ended. + while let Some(character @ ('a'..='z' | 'A'..='Z' | '0'..='9' | '_')) = self.rest.peek() { + name.push(*character); + let _ = self.rest.next(); + } + + if let Some(value) = self.env.get(&name) { + self.output.push_str(value); + } else { + terminate!(self, "unbound variable: {}", name) + } + + Ok(()) + } + + /// parse a braced variable name and push the associated value into `output` + fn parse_braced_variable_name(&mut self) -> Result { + let mut name = String::new(); + self.consume_whitespace(); + // the next character should be the start of a variable name. + match self.rest.next() { + Some(character @ ('_' | 'a'..='z' | 'A'..='Z')) => name.push(character), + Some(character) => terminate!( + self, + "expected _, a-z, A-Z or whitespace, got {}", + character + ), + None => terminate!(self, "input ended unexpectedly"), + } + + // consume & push until we know the variable name ended. + let mut modifier: Option = None; + loop { + match self.rest.peek() { + Some('}') => { + let _ = self.rest.next(); + break; + } + Some(':' | '?' | '-') => { + self.consume_whitespace(); + modifier.replace(self.parse_modifier()?); + break; + } + Some(character) if character.is_whitespace() => { + let _ = self.rest.next(); + break; + } + Some(character) => { + name.push(*character); + let _ = self.rest.next(); + } + None => terminate!(self, "input ended unexpectedly"), + } + } + + let value: &str = if let Some(value) = self.env.get(&name) { + if value.is_empty() { + use Modifier::{DefaultValue, ErrorMessage}; + use ModifierCondition::{WhenUnset, WhenUnsetOrEmpty}; + + match modifier { + Some(DefaultValue(WhenUnsetOrEmpty, ref default)) => default, + Some(ErrorMessage(WhenUnsetOrEmpty, err_msg)) => { + terminate!(self, "empty variable: {} ({})", name, err_msg) + } + Some(DefaultValue(WhenUnset, _) | ErrorMessage(WhenUnset, _)) | None => "", + } + } else { + value + } + } else { + use Modifier::{DefaultValue, ErrorMessage}; + use ModifierCondition::{WhenUnset, WhenUnsetOrEmpty}; + match modifier { + Some(DefaultValue(WhenUnset | WhenUnsetOrEmpty, ref default)) => default, + Some(ErrorMessage(WhenUnset | WhenUnsetOrEmpty, err_msg)) => { + terminate!(self, "unbound variable: {} ({})", name, err_msg) + } + None => { + terminate!(self, "unbound variable: {}", name) + } + } + }; + + self.output.push_str(value); + Ok(()) + } + + /// consume characters from source until the first non-whitespace character is encountered + fn consume_whitespace(&mut self) { + while let Some(character) = self.rest.peek() { + if character.is_whitespace() { + let _ = self.rest.next(); + } else { + break; + } + } + } + + /// check for a modifier or default value. these can contain nested variables. + fn parse_modifier(&mut self) -> std::result::Result { + use Modifier::{DefaultValue, ErrorMessage}; + + let cond = match self.rest.peek() { + Some(':') => { + let _ = self.rest.next(); + ModifierCondition::WhenUnsetOrEmpty + } + // error cases (EOF, missing ? or -) are handled below. + _ => ModifierCondition::WhenUnset, + }; + + let mut mod_val = String::new(); + let modifier: Modifier = match self.rest.next() { + Some('-') => { + self.parse_modifier_value(&mut mod_val)?; + DefaultValue(cond, mod_val) + } + Some('?') => { + self.parse_modifier_value(&mut mod_val)?; + ErrorMessage(cond, mod_val) + } + Some(character) => terminate!(self, "expected one of ? or -, got {}", character), + None => terminate!(self, "input ended unexpectedly"), + }; + + Ok(modifier) + } + + /// parse the contents of the error message or default value of a braced variable + fn parse_modifier_value(&mut self, output: &mut String) -> Result { + loop { + match self.rest.peek() { + Some('}') => { + let _ = self.rest.next(); + break; + } + Some('$') => self.parse_variable_start()?, + Some(character) => { + output.push(*character); + let _ = self.rest.next(); + } + None => terminate!(self, "input ended unexpectedly"), + } + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use crate::variable_interpolation::{parser::Parser, VariableResolver}; + + #[test] + fn blank_strings() { + let res = VariableResolver::default(); + assert_eq!(Parser::start(&res, "").expect("failed"), ""); + assert_eq!(Parser::start(&res, " ").expect("failed"), " "); + } + + #[test] + fn without_variables() { + let res = VariableResolver::default(); + assert_eq!(Parser::start(&res, "abc").expect("failed"), "abc"); + assert_eq!(Parser::start(&res, "a{c").expect("failed"), "a{c"); + assert_eq!( + Parser::start(&res, "/this/is/a/path").expect("failed"), + "/this/is/a/path" + ); + } + + #[test] + fn with_escaped_dollar() { + let res = VariableResolver::default(); + assert_eq!(Parser::start(&res, "$$").expect("failed"), "$"); + assert_eq!(Parser::start(&res, "$$$$").expect("failed"), "$$"); + assert_eq!(Parser::start(&res, "$$ $$").expect("failed"), "$ $"); + assert_eq!( + Parser::start(&res, "$$ab$$cd$$ef").expect("failed"), + "$ab$cd$ef" + ); + } + + #[test] + fn with_unterminated_brace() { + let res = VariableResolver::default(); + let result = Parser::start(&res, "${").expect_err("did not fail"); + assert!(result.to_string().contains("input ended unexpectedly")); + } + + #[test] + fn with_unbound_variable_unbraced() { + let res = VariableResolver::default(); + let result = Parser::start(&res, "$MOO").expect_err("did not fail"); + assert!(result.to_string().contains("unbound variable")); + assert!(result.to_string().contains("MOO")); + } + + #[test] + fn unbraced_variable() { + let mut res = VariableResolver::default(); + res.add_vars(HashMap::from([ + ("_".into(), "UNDERSCORE".into()), + ("__".into(), "DOUBLESCORE".into()), + ("a_".into(), "ALPHASCORE".into()), + ])); + assert_eq!( + Parser::start(&res, "_ is $_").expect("failed"), + "_ is UNDERSCORE" + ); + assert_eq!( + Parser::start(&res, "__ is $__").expect("failed"), + "__ is DOUBLESCORE" + ); + assert_eq!( + Parser::start(&res, "__ is $__ and $a_").expect("failed"), + "__ is DOUBLESCORE and ALPHASCORE" + ); + } + + #[test] + fn braced_variable() { + let mut res = VariableResolver::default(); + res.add_vars(HashMap::from([ + ("_".into(), "UNDERSCORE".into()), + ("__".into(), "DOUBLESCORE".into()), + ("a_".into(), "ALPHASCORE".into()), + ])); + assert_eq!( + Parser::start(&res, "_ is ${_}").expect("failed"), + "_ is UNDERSCORE" + ); + assert_eq!( + Parser::start(&res, "__ is ${__}").expect("failed"), + "__ is DOUBLESCORE" + ); + assert_eq!( + Parser::start(&res, "__ is ${__} and $a_").expect("failed"), + "__ is DOUBLESCORE and ALPHASCORE" + ); + assert_eq!( + Parser::start(&res, "__ is ${ __} and $a_").expect("failed"), + "__ is DOUBLESCORE and ALPHASCORE" + ); + } + + #[test] + fn braced_with_default_unset_and_empty() { + let mut res = VariableResolver::default(); + res.add_vars(HashMap::from([("a_".into(), String::new())])); + + assert_eq!( + Parser::start(&res, "_ is ${_:-UNDERSCORE} and more").expect("failed"), + "_ is UNDERSCORE and more" + ); + assert_eq!( + Parser::start(&res, "__ is ${__:-DOUBLESCORE}").expect("failed"), + "__ is DOUBLESCORE" + ); + assert_eq!( + Parser::start(&res, "__ is ${__:-DOUBLESCORE} and ${a_:-ALPHASCORE}").expect("failed"), + "__ is DOUBLESCORE and ALPHASCORE" + ); + } + + #[test] + fn braced_with_default_unset_only() { + let mut res = VariableResolver::default(); + res.add_vars(HashMap::from([("_".into(), String::new())])); + + assert_eq!( + Parser::start(&res, "_ is ${_-UNDERSCORE} and more").expect("failed"), + "_ is and more" + ); + + assert_eq!( + Parser::start(&res, "a_ is ${a_-UNDERSCORE} and more").expect("failed"), + "a_ is UNDERSCORE and more" + ); + } + + #[test] + fn with_syntax_errors_incomplete_modifier() { + let mut res = VariableResolver::default(); + res.add_vars(HashMap::from([("NAME".into(), "BRAVO".into())])); + let result = Parser::start(&res, "${NAME:}").expect_err("did not fail"); + assert!(result.to_string().contains("expected one of ? or -")); + } + + #[test] + fn with_error_message_unset() { + let res = VariableResolver::default(); + let result = Parser::start(&res, "${ANOTHERNAME??}").expect_err("did not fail"); + assert!(result.to_string().contains("(?)")); + } + + #[test] + fn with_error_message_empty() { + let mut res = VariableResolver::default(); + res.add_vars(HashMap::from([("ANOTHERNAME".into(), String::new())])); + let result = Parser::start( + &res, + "sometext and ${ANOTHERNAME:?ANOTHERNAME_EMPTY} or more text", + ) + .expect_err("did not fail"); + assert!(result.to_string().contains("(ANOTHERNAME_EMPTY)")); + } + + #[test] + fn with_nested_variable_both_unset() { + let res = VariableResolver::default(); + + assert_eq!( + Parser::start(&res, "${VARIABLE:-${FOO:-default}}").expect("failed"), + "default" + ); + + assert_eq!( + Parser::start(&res, "${VARIABLE-${FOO-default}}").expect("failed"), + "default" + ); + } + + #[test] + fn with_nested_variable_second_set() { + let mut res = VariableResolver::default(); + res.add_vars(HashMap::from([("FOO".into(), "foo".into())])); + assert_eq!( + Parser::start(&res, "${VARIABLE-${FOO}}").expect("failed"), + "foo" + ); + assert_eq!( + Parser::start(&res, "${VARIABLE-${FOO:-default}}").expect("failed"), + "foo" + ); + assert_eq!( + Parser::start(&res, "${VARIABLE-$FOO}").expect("failed"), + "foo" + ); + } + + #[test] + fn with_nested_variable_second_with_error() { + let res = VariableResolver::default(); + + let result = Parser::start( + &res, + "sometext and ${ANOTHERNAME:-${FALLBACK:?something failed}} or more text", + ) + .expect_err("did not fail"); + assert!(result.to_string().contains("(something failed)")); + } + + #[test] + fn with_nested_variables_three_levels() { + let res = VariableResolver::default(); + + assert_eq!( + Parser::start(&res, "${VARIABLE:-${FOO:-${BAR:-third} second} first}").expect("failed"), + "third second first" + ); + } +} diff --git a/src/variable_interpolation/yaml_walk.rs b/src/variable_interpolation/yaml_walk.rs new file mode 100644 index 0000000..e51af58 --- /dev/null +++ b/src/variable_interpolation/yaml_walk.rs @@ -0,0 +1,42 @@ +//! recursively walk a `YamlValue` and find the strings to apply variable interpolation to +use serde_yaml::{Mapping, Sequence}; + +use super::{parser::Parser, VariableResolver}; +use crate::common::YamlValue; + +/// recursively interpolate all nested YAML strings containing variables. +/// will return the first failure to replace a variable. +pub(crate) fn interpolate_value( + vars: &VariableResolver, + value: &mut YamlValue, +) -> serde_yaml::Result<()> { + match value { + YamlValue::String(s) => { + let new_s = Parser::start(vars, s)?; + let _ = std::mem::replace(value, YamlValue::String(new_s)); + } + YamlValue::Sequence(sequence) => interpolate_sequence(vars, sequence)?, + YamlValue::Mapping(mapping) => interpolate_mapping(vars, mapping)?, + _ => {} + }; + Ok(()) +} + +/// try to interpolate variables in each of the elements of the given sequence. +fn interpolate_sequence( + vars: &VariableResolver, + sequence: &mut Sequence, +) -> serde_yaml::Result<()> { + for element in sequence.iter_mut() { + interpolate_value(vars, element)?; + } + Ok(()) +} + +/// try to interpolate variables in each of the values of the mapping. keys are never interpolated. +fn interpolate_mapping(vars: &VariableResolver, mapping: &mut Mapping) -> serde_yaml::Result<()> { + for (_key, value) in mapping.iter_mut() { + interpolate_value(vars, value)?; + } + Ok(()) +} From 8d759b7594bc80331a17d5864608bc8652f0c822 Mon Sep 17 00:00:00 2001 From: ganthern Date: Fri, 25 Apr 2025 23:03:38 +0200 Subject: [PATCH 2/8] chore(lints) fix clippy warnings in env var interpolation code - allow literal_string_with_formatting_args in interpolation test code - fix warning due to missing backticks --- src/variable_interpolation/error.rs | 2 +- src/variable_interpolation/parser.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/variable_interpolation/error.rs b/src/variable_interpolation/error.rs index 98e1a69..d0a7317 100644 --- a/src/variable_interpolation/error.rs +++ b/src/variable_interpolation/error.rs @@ -1,6 +1,6 @@ //! contains the error type used by the variable interpolation parser //! -//! since the calling code expects the error type to be serde_yaml::Error, +//! since the calling code expects the error type to be `serde_yaml::Error`, //! `ParseError` can be converted to that type, using the error information //! to construct a custom error message diff --git a/src/variable_interpolation/parser.rs b/src/variable_interpolation/parser.rs index 81aded8..4a33025 100644 --- a/src/variable_interpolation/parser.rs +++ b/src/variable_interpolation/parser.rs @@ -284,9 +284,10 @@ impl<'s> Parser<'s> { #[cfg(test)] mod tests { - use std::collections::HashMap; - + // false positive: variable placeholders look a lot like rust formatting args + #![allow(clippy::literal_string_with_formatting_args)] use crate::variable_interpolation::{parser::Parser, VariableResolver}; + use std::collections::HashMap; #[test] fn blank_strings() { From b90ea31fe28895e13646caaf03143612975930a8 Mon Sep 17 00:00:00 2001 From: Tokarak <63452145+Tokarak@users.noreply.github.com> Date: Wed, 23 Apr 2025 00:07:55 +0100 Subject: [PATCH 3/8] Add and fix a whitespace test --- src/variable_interpolation/parser.rs | 47 ++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/src/variable_interpolation/parser.rs b/src/variable_interpolation/parser.rs index 4a33025..b4afa84 100644 --- a/src/variable_interpolation/parser.rs +++ b/src/variable_interpolation/parser.rs @@ -172,16 +172,31 @@ impl<'s> Parser<'s> { break; } Some(':' | '?' | '-') => { - self.consume_whitespace(); modifier.replace(self.parse_modifier()?); break; } - Some(character) if character.is_whitespace() => { - let _ = self.rest.next(); - break; + Some(whitespace_char) if whitespace_char.is_whitespace() => { + self.rest.next(); + // Whitespace marks end of variable name + // Proceed forward until some variable-name terminator is found + self.consume_whitespace(); + match self.rest.peek() { + Some('}') => { + self.rest.next(); + break; + } + Some(':' | '?' | '-') => { + modifier.replace(self.parse_modifier()?); + break; + } + Some(&c) => { + terminate!(self, "expected one of }}:?- , got {}", c); + } + None => terminate!(self, "input ended unexpectedly"), + } } - Some(character) => { - name.push(*character); + Some(c) => { + name.push(*c); let _ = self.rest.next(); } None => terminate!(self, "input ended unexpectedly"), @@ -223,13 +238,11 @@ impl<'s> Parser<'s> { /// consume characters from source until the first non-whitespace character is encountered fn consume_whitespace(&mut self) { - while let Some(character) = self.rest.peek() { - if character.is_whitespace() { - let _ = self.rest.next(); - } else { - break; - } - } + while self + .rest + .next_if(|c: &char| char::is_whitespace(*c)) + .is_some() + {} } /// check for a modifier or default value. these can contain nested variables. @@ -319,6 +332,14 @@ mod tests { ); } + #[test] + fn test_end_braces_with_whitespace() { + let mut res = VariableResolver::default(); + res.add_vars(HashMap::from([("KEY".into(), "VALUE".into())])); + Parser::start(&res, "${KEY ") + .expect_err("Closed '{' with a whitespace. Input: \"${KEY \"; got"); + } + #[test] fn with_unterminated_brace() { let res = VariableResolver::default(); From e1501ebcbc2fb80b318685b603ee3a36c6d9ccf4 Mon Sep 17 00:00:00 2001 From: Tokarak <63452145+Tokarak@users.noreply.github.com> Date: Wed, 23 Apr 2025 00:10:44 +0100 Subject: [PATCH 4/8] Style changes --- src/variable_interpolation/parser.rs | 73 +++++++++++++++------------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/src/variable_interpolation/parser.rs b/src/variable_interpolation/parser.rs index b4afa84..1dedef1 100644 --- a/src/variable_interpolation/parser.rs +++ b/src/variable_interpolation/parser.rs @@ -43,7 +43,7 @@ enum ModifierCondition { } impl<'s> Parser<'s> { - /// parse the given `String` and return a new `String`, replacing any encountered + /// parse the given `String` and return a new `String`, replacing any encountered /// variables with the value returned by `vars` pub(super) fn start(vars: &'s VariableResolver, source: &'s str) -> serde_yaml::Result { let mut parser = Self { @@ -78,7 +78,7 @@ impl<'s> Parser<'s> { // check that the iterator points to a $ and consume it match self.rest.peek() { Some('$') => { - let _ = self.rest.next(); + self.rest.next(); } _ => return Ok(()), } @@ -87,7 +87,7 @@ impl<'s> Parser<'s> { // if not, continue with a braced or non-braced variable. match self.rest.peek() { Some('$') => { - let _ = self.rest.next(); + self.rest.next(); self.output.push('$'); Ok(()) } @@ -102,7 +102,7 @@ impl<'s> Parser<'s> { Some('$') | None => (), Some(character) => { self.output.push(*character); - let _ = self.rest.next(); + self.rest.next(); } } } @@ -133,10 +133,10 @@ impl<'s> Parser<'s> { None => terminate!(self, "input ended unexpectedly"), } - // consume & push until we know the variable name ended. + // consume and push until we know the variable name ended. while let Some(character @ ('a'..='z' | 'A'..='Z' | '0'..='9' | '_')) = self.rest.peek() { name.push(*character); - let _ = self.rest.next(); + self.rest.next(); } if let Some(value) = self.env.get(&name) { @@ -163,12 +163,12 @@ impl<'s> Parser<'s> { None => terminate!(self, "input ended unexpectedly"), } - // consume & push until we know the variable name ended. + // consume and push until we know the variable name ended. let mut modifier: Option = None; loop { match self.rest.peek() { Some('}') => { - let _ = self.rest.next(); + self.rest.next(); break; } Some(':' | '?' | '-') => { @@ -177,7 +177,7 @@ impl<'s> Parser<'s> { } Some(whitespace_char) if whitespace_char.is_whitespace() => { self.rest.next(); - // Whitespace marks end of variable name + // Whitespace marks the end of variable name // Proceed forward until some variable-name terminator is found self.consume_whitespace(); match self.rest.peek() { @@ -197,37 +197,40 @@ impl<'s> Parser<'s> { } Some(c) => { name.push(*c); - let _ = self.rest.next(); + self.rest.next(); } None => terminate!(self, "input ended unexpectedly"), } } - let value: &str = if let Some(value) = self.env.get(&name) { - if value.is_empty() { - use Modifier::{DefaultValue, ErrorMessage}; - use ModifierCondition::{WhenUnset, WhenUnsetOrEmpty}; + let value: &str = match self.env.get(&name) { + Some(value) => { + if value.is_empty() { + use Modifier::{DefaultValue, ErrorMessage}; + use ModifierCondition::{WhenUnset, WhenUnsetOrEmpty}; - match modifier { - Some(DefaultValue(WhenUnsetOrEmpty, ref default)) => default, - Some(ErrorMessage(WhenUnsetOrEmpty, err_msg)) => { - terminate!(self, "empty variable: {} ({})", name, err_msg) + match modifier { + Some(DefaultValue(WhenUnsetOrEmpty, ref default)) => default, + Some(ErrorMessage(WhenUnsetOrEmpty, err_msg)) => { + terminate!(self, "empty variable: {} ({})", name, err_msg) + } + Some(DefaultValue(WhenUnset, _) | ErrorMessage(WhenUnset, _)) | None => "", } - Some(DefaultValue(WhenUnset, _) | ErrorMessage(WhenUnset, _)) | None => "", + } else { + value } - } else { - value } - } else { - use Modifier::{DefaultValue, ErrorMessage}; - use ModifierCondition::{WhenUnset, WhenUnsetOrEmpty}; - match modifier { - Some(DefaultValue(WhenUnset | WhenUnsetOrEmpty, ref default)) => default, - Some(ErrorMessage(WhenUnset | WhenUnsetOrEmpty, err_msg)) => { - terminate!(self, "unbound variable: {} ({})", name, err_msg) - } - None => { - terminate!(self, "unbound variable: {}", name) + None => { + use Modifier::{DefaultValue, ErrorMessage}; + use ModifierCondition::{WhenUnset, WhenUnsetOrEmpty}; + match modifier { + Some(DefaultValue(WhenUnset | WhenUnsetOrEmpty, ref default)) => default, + Some(ErrorMessage(WhenUnset | WhenUnsetOrEmpty, err_msg)) => { + terminate!(self, "unbound variable: {} ({})", name, err_msg) + } + None => { + terminate!(self, "unbound variable: {}", name) + } } } }; @@ -251,7 +254,7 @@ impl<'s> Parser<'s> { let cond = match self.rest.peek() { Some(':') => { - let _ = self.rest.next(); + self.rest.next(); ModifierCondition::WhenUnsetOrEmpty } // error cases (EOF, missing ? or -) are handled below. @@ -280,13 +283,13 @@ impl<'s> Parser<'s> { loop { match self.rest.peek() { Some('}') => { - let _ = self.rest.next(); + self.rest.next(); break; } Some('$') => self.parse_variable_start()?, Some(character) => { output.push(*character); - let _ = self.rest.next(); + self.rest.next(); } None => terminate!(self, "input ended unexpectedly"), } @@ -339,7 +342,7 @@ mod tests { Parser::start(&res, "${KEY ") .expect_err("Closed '{' with a whitespace. Input: \"${KEY \"; got"); } - + #[test] fn with_unterminated_brace() { let res = VariableResolver::default(); From f6f65591269ac058acc0f1ccd9f86c210def7779 Mon Sep 17 00:00:00 2001 From: Tokarak <63452145+Tokarak@users.noreply.github.com> Date: Wed, 23 Apr 2025 00:48:18 +0100 Subject: [PATCH 5/8] Change signature of parse_modifier_value --- src/variable_interpolation/parser.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/variable_interpolation/parser.rs b/src/variable_interpolation/parser.rs index 1dedef1..c9cc68b 100644 --- a/src/variable_interpolation/parser.rs +++ b/src/variable_interpolation/parser.rs @@ -261,15 +261,12 @@ impl<'s> Parser<'s> { _ => ModifierCondition::WhenUnset, }; - let mut mod_val = String::new(); let modifier: Modifier = match self.rest.next() { Some('-') => { - self.parse_modifier_value(&mut mod_val)?; - DefaultValue(cond, mod_val) + DefaultValue(cond, self.parse_modifier_value()?) } Some('?') => { - self.parse_modifier_value(&mut mod_val)?; - ErrorMessage(cond, mod_val) + ErrorMessage(cond, self.parse_modifier_value()?) } Some(character) => terminate!(self, "expected one of ? or -, got {}", character), None => terminate!(self, "input ended unexpectedly"), @@ -279,7 +276,8 @@ impl<'s> Parser<'s> { } /// parse the contents of the error message or default value of a braced variable - fn parse_modifier_value(&mut self, output: &mut String) -> Result { + fn parse_modifier_value(&mut self) -> std::result::Result { + let mut output = String::new(); loop { match self.rest.peek() { Some('}') => { @@ -294,7 +292,7 @@ impl<'s> Parser<'s> { None => terminate!(self, "input ended unexpectedly"), } } - Ok(()) + Ok(output) } } From 4f69b97c0c58520cbea6b6678440948f9a499471 Mon Sep 17 00:00:00 2001 From: Tokarak <63452145+Tokarak@users.noreply.github.com> Date: Wed, 23 Apr 2025 01:02:05 +0100 Subject: [PATCH 6/8] Add type argument to parser Result type --- src/variable_interpolation/parser.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/variable_interpolation/parser.rs b/src/variable_interpolation/parser.rs index c9cc68b..020ee4c 100644 --- a/src/variable_interpolation/parser.rs +++ b/src/variable_interpolation/parser.rs @@ -5,7 +5,7 @@ use super::VariableResolver; use std::{iter::Peekable, str::Chars}; /// the result type for the parser -type Result = std::result::Result<(), ParseError>; +type Result = std::result::Result; #[derive(Debug, Clone)] /// the struct containing the state of the parser during the parsing process @@ -59,7 +59,7 @@ impl<'s> Parser<'s> { /// entry point into the parsing logic. after this returns Ok, self.output /// contains the interpolated input string. - fn parse_string(&mut self) -> Result { + fn parse_string(&mut self) -> Result<()> { loop { if self.rest.peek().is_none() { // nothing to parse anymore, we're done @@ -74,7 +74,7 @@ impl<'s> Parser<'s> { } /// parse either the start of a variable name ($) or an escaped dollar sign ($$) - fn parse_variable_start(&mut self) -> Result { + fn parse_variable_start(&mut self) -> Result<()> { // check that the iterator points to a $ and consume it match self.rest.peek() { Some('$') => { @@ -108,7 +108,7 @@ impl<'s> Parser<'s> { } /// parse a braced variable: `{NAME}` - fn parse_braced_variable_start(&mut self) -> Result { + fn parse_braced_variable_start(&mut self) -> Result<()> { // check that the iterator points to a `{` match self.rest.next() { Some('{') => {} @@ -119,7 +119,7 @@ impl<'s> Parser<'s> { } /// parse the name of an unbraced variable and replace it with its value - fn parse_variable_name(&mut self) -> Result { + fn parse_variable_name(&mut self) -> Result<()> { let mut name = String::new(); match self.rest.next() { Some(character @ ('_' | 'a'..='z' | 'A'..='Z')) => name.push(character), @@ -149,7 +149,7 @@ impl<'s> Parser<'s> { } /// parse a braced variable name and push the associated value into `output` - fn parse_braced_variable_name(&mut self) -> Result { + fn parse_braced_variable_name(&mut self) -> Result<()> { let mut name = String::new(); self.consume_whitespace(); // the next character should be the start of a variable name. @@ -249,7 +249,7 @@ impl<'s> Parser<'s> { } /// check for a modifier or default value. these can contain nested variables. - fn parse_modifier(&mut self) -> std::result::Result { + fn parse_modifier(&mut self) -> Result { use Modifier::{DefaultValue, ErrorMessage}; let cond = match self.rest.peek() { @@ -276,7 +276,7 @@ impl<'s> Parser<'s> { } /// parse the contents of the error message or default value of a braced variable - fn parse_modifier_value(&mut self) -> std::result::Result { + fn parse_modifier_value(&mut self) -> Result { let mut output = String::new(); loop { match self.rest.peek() { From 2654e64ae834770d4c4c7a03e387e6f88534d6e6 Mon Sep 17 00:00:00 2001 From: Tokarak <63452145+Tokarak@users.noreply.github.com> Date: Wed, 23 Apr 2025 01:18:20 +0100 Subject: [PATCH 7/8] Add test --- src/variable_interpolation/parser.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/variable_interpolation/parser.rs b/src/variable_interpolation/parser.rs index 020ee4c..33d583f 100644 --- a/src/variable_interpolation/parser.rs +++ b/src/variable_interpolation/parser.rs @@ -25,7 +25,7 @@ pub(super) struct Parser<'s> { /// - "NAME-DEFAULT": use DEFAULT if variable NAME is not set /// - "NAME:-DEFAULT": use DEFAULT if variable NAME is not set or empty /// - "NAME?MESSAGE": fail with MESSAGE if variable NAME is not set -/// - "NAME:?MESSAGE": fail with MESSAGE if variabele NAME is not set or empty +/// - "NAME:?MESSAGE": fail with MESSAGE if variable NAME is not set or empty enum Modifier { /// if the condition matches, replace the failure with the error message ErrorMessage(ModifierCondition, String), @@ -262,12 +262,8 @@ impl<'s> Parser<'s> { }; let modifier: Modifier = match self.rest.next() { - Some('-') => { - DefaultValue(cond, self.parse_modifier_value()?) - } - Some('?') => { - ErrorMessage(cond, self.parse_modifier_value()?) - } + Some('-') => DefaultValue(cond, self.parse_modifier_value()?), + Some('?') => ErrorMessage(cond, self.parse_modifier_value()?), Some(character) => terminate!(self, "expected one of ? or -, got {}", character), None => terminate!(self, "input ended unexpectedly"), }; @@ -348,6 +344,15 @@ mod tests { assert!(result.to_string().contains("input ended unexpectedly")); } + #[test] + fn with_illegal_name_unbraced_variable() { + let res = VariableResolver::default(); + let result = Parser::start(&res, "$1abc").expect_err("did not fail"); + assert!(result + .to_string() + .contains("expected one of $, {, _, a-z, A-Z, got 1")); + } + #[test] fn with_unbound_variable_unbraced() { let res = VariableResolver::default(); From a78fe262bbcd941f6d25c46787df1f8252f94c81 Mon Sep 17 00:00:00 2001 From: Tokarak <63452145+Tokarak@users.noreply.github.com> Date: Mon, 28 Apr 2025 15:23:00 +0100 Subject: [PATCH 8/8] Fix Clippy lints --- src/variable_interpolation/parser.rs | 11 ++++++----- src/variable_interpolation/yaml_walk.rs | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/variable_interpolation/parser.rs b/src/variable_interpolation/parser.rs index 33d583f..b1f73f3 100644 --- a/src/variable_interpolation/parser.rs +++ b/src/variable_interpolation/parser.rs @@ -189,20 +189,21 @@ impl<'s> Parser<'s> { modifier.replace(self.parse_modifier()?); break; } - Some(&c) => { - terminate!(self, "expected one of }}:?- , got {}", c); + Some(&char) => { + terminate!(self, "expected one of }}:?- , got {}", char); } None => terminate!(self, "input ended unexpectedly"), } } - Some(c) => { - name.push(*c); + Some(char) => { + name.push(*char); self.rest.next(); } None => terminate!(self, "input ended unexpectedly"), } } + #[allow(clippy::single_match_else)] let value: &str = match self.env.get(&name) { Some(value) => { if value.is_empty() { @@ -243,7 +244,7 @@ impl<'s> Parser<'s> { fn consume_whitespace(&mut self) { while self .rest - .next_if(|c: &char| char::is_whitespace(*c)) + .next_if(|char: &char| char::is_whitespace(*char)) .is_some() {} } diff --git a/src/variable_interpolation/yaml_walk.rs b/src/variable_interpolation/yaml_walk.rs index e51af58..7a1db0a 100644 --- a/src/variable_interpolation/yaml_walk.rs +++ b/src/variable_interpolation/yaml_walk.rs @@ -18,7 +18,7 @@ pub(crate) fn interpolate_value( YamlValue::Sequence(sequence) => interpolate_sequence(vars, sequence)?, YamlValue::Mapping(mapping) => interpolate_mapping(vars, mapping)?, _ => {} - }; + } Ok(()) }