From 9515496407590c588eb4496c0b16819ff9a83bd1 Mon Sep 17 00:00:00 2001 From: xxchan Date: Fri, 28 Jun 2024 16:36:39 +0800 Subject: [PATCH] feat: don't subst env vars for `system` Signed-off-by: xxchan --- CHANGELOG.md | 3 +- sqllogictest/src/runner.rs | 21 ++++++++------ sqllogictest/src/substitution.rs | 47 ++++++++++++++++++++++---------- 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 189f6e1..ef54df7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 **Breaking changes**: * runner: `RecordOutput` is now returned by `Runner::run` (or `Runner::run_async`). This allows users to access the output of each record, or check whether the record is skipped. -* substitution: add a special variable `__NOW__` which will be replaced with the current Unix timestamp in nanoseconds. +* runner(substitution): add a special variable `__NOW__` which will be replaced with the current Unix timestamp in nanoseconds. +* runner(substitution): for `system` commands, we do not substitute environment variables any more, because the shell can do that. It's necessary to escape like `\\` any more. `$__TEST_DIR__`, and are still supported. ## [0.20.6] - 2024-06-21 diff --git a/sqllogictest/src/runner.rs b/sqllogictest/src/runner.rs index d455f4c..72094e6 100644 --- a/sqllogictest/src/runner.rs +++ b/sqllogictest/src/runner.rs @@ -576,7 +576,7 @@ impl> Runner { expected: _, loc: _, } => { - let sql = match self.may_substitute(sql) { + let sql = match self.may_substitute(sql, true) { Ok(sql) => sql, Err(error) => { return RecordOutput::Statement { @@ -627,7 +627,7 @@ impl> Runner { return RecordOutput::Nothing; } - let mut command = match self.may_substitute(command) { + let mut command = match self.may_substitute(command, false) { Ok(command) => command, Err(error) => { return RecordOutput::System { @@ -723,7 +723,7 @@ impl> Runner { expected, loc: _, } => { - let sql = match self.may_substitute(sql) { + let sql = match self.may_substitute(sql, true) { Ok(sql) => sql, Err(error) => { return RecordOutput::Query { @@ -1196,12 +1196,17 @@ impl> Runner { /// Substitute the input SQL or command with [`Substitution`], if enabled by `control /// substitution`. - fn may_substitute(&self, input: String) -> Result { - #[derive(thiserror::Error, Debug)] - #[error("substitution failed: {0}")] - struct SubstError(subst::Error); + /// + /// If `subst_env_vars`, we will use the `subst` crate to support extensive substitutions, incl. `$NAME`, `${NAME}`, `${NAME:default}`. + /// The cost is that we will have to use escape characters, e.g., `\$` & `\\`. + /// + /// Otherwise, we just do simple string substitution for `__TEST_DIR__` and `__NOW__`. + /// This is useful for `system` commands: The shell can do the environment variables, and we can write strings like `\n` without escaping. + fn may_substitute(&self, input: String, subst_env_vars: bool) -> Result { if let Some(substitution) = &self.substitution { - subst::substitute(&input, substitution).map_err(|e| Arc::new(SubstError(e)) as AnyError) + substitution + .substitute(&input, subst_env_vars) + .map_err(|e| Arc::new(e) as AnyError) } else { Ok(input) } diff --git a/sqllogictest/src/substitution.rs b/sqllogictest/src/substitution.rs index d66041a..57b05e1 100644 --- a/sqllogictest/src/substitution.rs +++ b/sqllogictest/src/substitution.rs @@ -11,25 +11,44 @@ pub(crate) struct Substitution { test_dir: Arc>, } +#[derive(thiserror::Error, Debug)] +#[error("substitution failed: {0}")] +pub(crate) struct SubstError(subst::Error); + +impl Substitution { + pub fn substitute(&self, input: &str, subst_env_vars: bool) -> Result { + if !subst_env_vars { + Ok(input + .replace("$__TEST_DIR__", &self.test_dir()) + .replace("$__NOW__", &self.now())) + } else { + subst::substitute(&input, self).map_err(SubstError) + } + } + + fn test_dir(&self) -> String { + let test_dir = self + .test_dir + .get_or_init(|| tempdir().expect("failed to create testdir")); + test_dir.path().to_string_lossy().into_owned() + } + + fn now(&self) -> String { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("failed to get current time") + .as_nanos() + .to_string() + } +} + impl<'a> subst::VariableMap<'a> for Substitution { type Value = String; fn get(&'a self, key: &str) -> Option { match key { - "__TEST_DIR__" => { - let test_dir = self - .test_dir - .get_or_init(|| tempdir().expect("failed to create testdir")); - test_dir.path().to_string_lossy().into_owned().into() - } - - "__NOW__" => std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("failed to get current time") - .as_nanos() - .to_string() - .into(), - + "__TEST_DIR__" => self.test_dir().into(), + "__NOW__" => self.now().into(), key => Env.get(key), } }