From 9515496407590c588eb4496c0b16819ff9a83bd1 Mon Sep 17 00:00:00 2001 From: xxchan Date: Fri, 28 Jun 2024 16:36:39 +0800 Subject: [PATCH 1/3] 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), } } From 4aa6a9a727a53efbca6148d15041236a8dd6d83b Mon Sep 17 00:00:00 2001 From: xxchan Date: Fri, 28 Jun 2024 17:29:44 +0800 Subject: [PATCH 2/3] fix Signed-off-by: xxchan --- sqllogictest/src/substitution.rs | 2 +- tests/system_command/system_command.slt | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/sqllogictest/src/substitution.rs b/sqllogictest/src/substitution.rs index 57b05e1..7353620 100644 --- a/sqllogictest/src/substitution.rs +++ b/sqllogictest/src/substitution.rs @@ -22,7 +22,7 @@ impl Substitution { .replace("$__TEST_DIR__", &self.test_dir()) .replace("$__NOW__", &self.now())) } else { - subst::substitute(&input, self).map_err(SubstError) + subst::substitute(input, self).map_err(SubstError) } } diff --git a/tests/system_command/system_command.slt b/tests/system_command/system_command.slt index 161c5ef..fae1a11 100644 --- a/tests/system_command/system_command.slt +++ b/tests/system_command/system_command.slt @@ -1,7 +1,7 @@ control substitution on system ok -echo 114514 > ${__TEST_DIR__}/test.txt +echo 114514 > $__TEST_DIR__/test.txt query T select read("${__TEST_DIR__}/test.txt") @@ -23,10 +23,9 @@ echo "114514" 114514 -# Note: when substitution is on, we need to escape the backslash # Note: 1 blank line in the middle is ok, but not 2 system ok -echo "114\\n\\n514" +echo "114\n\n514" ---- 114 From 9b31bb366add4b96397aabb794a667ee8bd4d068 Mon Sep 17 00:00:00 2001 From: xxchan Date: Fri, 28 Jun 2024 17:31:39 +0800 Subject: [PATCH 3/3] fix Signed-off-by: xxchan --- tests/system_command/system_command.slt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/system_command/system_command.slt b/tests/system_command/system_command.slt index fae1a11..7945aab 100644 --- a/tests/system_command/system_command.slt +++ b/tests/system_command/system_command.slt @@ -9,7 +9,7 @@ select read("${__TEST_DIR__}/test.txt") 114514 system ok -echo 1919810 > ${__TEST_DIR__}/test.txt +echo 1919810 > $__TEST_DIR__/test.txt query T select read("${__TEST_DIR__}/test.txt")