test(suite) :: share helpers, close pools deterministically, slim OIDC harness

.github/workflows/ci.yml

@@ -99,6 +99,7 @@ jobs:
             container: mssql
             db_url: "mssql://root:Password123!@127.0.0.1/sqlpage"
           - database: oracle
+            test_args: --test-threads=2
             container: oracle
             db_url: "Driver=Oracle 23 ODBC driver;Dbq=//127.0.0.1:1521/FREEPDB1;Uid=root;Pwd=Password123!"
           - database: duckdb
@@ -142,7 +143,7 @@ jobs:
         run: docker compose logs ${{ matrix.container }}
       - name: Run tests against ${{ matrix.database }}
         timeout-minutes: 5
-        run: scripts/run-test-binaries.sh
+        run: scripts/run-test-binaries.sh ${{ matrix.test_args }}
         env:
           DATABASE_URL: ${{ matrix.db_url }}
           MALLOC_CHECK_: 3

CONTRIBUTING.md

@@ -96,6 +96,14 @@ export DATABASE_URL=mssql://root:Password123!@localhost/sqlpage
 cargo test
 ```
 
+Use `common::response_for(path)` for ordinary integration requests and
+`common::response_with(path, config)` for custom configuration. Custom requests use
+`common::response_from(request)`; tests sharing database or cache state use `common::send_request`.
+These helpers use the production `create_app` routes and middleware through an Actix test service,
+whose destructor drains its request pool.
+Keep `#[actix_web::test]`; standalone `TestRequest::to_srv_request()` retains application state.
+When testing Oracle locally, use `cargo test -- --test-threads=2` to avoid overwhelming the listener.
+
 ### End-to-End Tests
 
 We use Playwright for end-to-end testing of dynamic frontend features.

scripts/run-test-binaries.sh

@@ -13,6 +13,6 @@ fi
 
 for test_binary in "${test_binaries[@]}"; do
   echo "::group::$(basename "$test_binary")"
-  "$test_binary" --quiet
+  "$test_binary" --quiet "$@"
   echo "::endgroup::"
 done

src/webserver/database/sqlpage_expr.rs

@@ -12,7 +12,6 @@ use serde_json::Value;
 use super::execute_queries::DbConn;
 use super::sqlpage_functions::functions::SqlPageFunctionName;
 use crate::webserver::http_request_info::ExecutionContext;
-use crate::webserver::single_or_vec::SingleOrVec;
 
 /// An expression evaluated by `SQLPage`.
 ///
@@ -163,50 +162,21 @@ fn value_to_sqlpage_value(value: Value) -> SqlPageValue<'static> {
 
 impl VariableRef {
     fn evaluate<'a>(&self, request: &'a ExecutionContext) -> SqlPageValue<'a> {
-        let value = match self.source {
-            VariableSource::Url => request
-                .url_params
-                .get(&self.name)
-                .map(SingleOrVec::as_json_str),
-            VariableSource::SetOrForm => {
-                if let Some(value) = request.set_variables.borrow().get(&self.name) {
-                    return value.as_ref().map_or(SqlPageValue::Null, |value| {
-                        SqlPageValue::Text(Cow::Owned(value.as_json_str().into_owned()))
-                    });
-                }
-                request
-                    .post_variables
-                    .get(&self.name)
-                    .map(SingleOrVec::as_json_str)
-            }
-            VariableSource::SetOrUrl => {
-                if let Some(value) = request.set_variables.borrow().get(&self.name) {
-                    return value.as_ref().map_or(SqlPageValue::Null, |value| {
-                        SqlPageValue::Text(Cow::Owned(value.as_json_str().into_owned()))
-                    });
-                }
-                let url_value = request.url_params.get(&self.name);
-                if request.post_variables.contains_key(&self.name) {
-                    if url_value.is_some() {
-                        log::warn!(
-                            "Deprecation warning! There is both a URL parameter named '{}' and a form field named '{}'. SQLPage is using the URL parameter for ${}. Please use :{} to reference the form field explicitly.",
-                            self.name,
-                            self.name,
-                            self.name,
-                            self.name,
-                        );
-                    } else {
-                        log::warn!(
-                            "Deprecation warning! ${} was used to reference a form field value (a POST variable). This now uses only URL parameters. Please use :{} instead.",
-                            self.name,
-                            self.name,
-                        );
-                    }
-                }
-                url_value.map(SingleOrVec::as_json_str)
-            }
+        use crate::webserver::request_variables::{LookupPolicy, VariableAccess, VariableValue};
+        let policy = match self.source {
+            VariableSource::Url => LookupPolicy::GetOnly,
+            VariableSource::SetOrForm => LookupPolicy::SetThenPost,
+            VariableSource::SetOrUrl => LookupPolicy::SetThenGet,
         };
-        value.map_or(SqlPageValue::Null, SqlPageValue::Text)
+        let variables = VariableAccess::new(
+            &request.url_params,
+            &request.post_variables,
+            &request.set_variables,
+        );
+        match variables.lookup(&self.name, policy) {
+            VariableValue::Missing | VariableValue::Null => SqlPageValue::Null,
+            VariableValue::Text(value) => SqlPageValue::Text(value),
+        }
     }
 }
 

src/webserver/database/sqlpage_functions/functions/variables.rs

@@ -22,28 +22,11 @@ pub(super) async fn variables<'a>(
             ));
         }
     } else {
-        use serde::{Serializer, ser::SerializeMap};
-        let mut res = Vec::new();
-        let mut serializer = serde_json::Serializer::new(&mut res);
-        let set_vars = request.set_variables.borrow();
-        let len = request.url_params.len() + request.post_variables.len() + set_vars.len();
-        let mut ser = serializer.serialize_map(Some(len))?;
-        let mut seen_keys = std::collections::HashSet::new();
-        for (k, v) in &*set_vars {
-            seen_keys.insert(k);
-            ser.serialize_entry(k, v)?;
-        }
-        for (k, v) in &request.post_variables {
-            if seen_keys.insert(k) {
-                ser.serialize_entry(k, v)?;
-            }
-        }
-        for (k, v) in &request.url_params {
-            if seen_keys.insert(k) {
-                ser.serialize_entry(k, v)?;
-            }
-        }
-        ser.end()?;
-        String::from_utf8(res)?
+        let variables = crate::webserver::request_variables::VariableAccess::new(
+            &request.url_params,
+            &request.post_variables,
+            &request.set_variables,
+        );
+        serde_json::to_string(&variables)?
     })
 }

src/webserver/request_variables.rs

@@ -1,10 +1,95 @@
+use std::borrow::Cow;
+use std::cell::RefCell;
 use std::collections::{HashMap, hash_map::Entry};
 
+use serde::{Serialize, Serializer, ser::SerializeMap};
+
 use crate::webserver::single_or_vec::SingleOrVec;
 
 pub type ParamMap = HashMap<String, SingleOrVec>;
 pub type SetVariablesMap = HashMap<String, Option<SingleOrVec>>;
 
+/// Request values stay borrowed; mutable SET values are copied before releasing
+/// their `RefCell` borrow. A present SET NULL must suppress request fallbacks.
+pub(crate) struct VariableAccess<'a> {
+    get: &'a ParamMap,
+    post: &'a ParamMap,
+    set: &'a RefCell<SetVariablesMap>,
+}
+
+#[derive(Clone, Copy)]
+pub(crate) enum LookupPolicy {
+    GetOnly,
+    SetThenGet,
+    SetThenPost,
+}
+
+pub(crate) enum VariableValue<'a> {
+    Missing,
+    Null,
+    Text(Cow<'a, str>),
+}
+
+impl<'a> VariableAccess<'a> {
+    pub(crate) fn new(
+        get: &'a ParamMap,
+        post: &'a ParamMap,
+        set: &'a RefCell<SetVariablesMap>,
+    ) -> Self {
+        Self { get, post, set }
+    }
+
+    pub(crate) fn lookup(&self, name: &str, policy: LookupPolicy) -> VariableValue<'a> {
+        if !matches!(policy, LookupPolicy::GetOnly)
+            && let Some(value) = self.set.borrow().get(name)
+        {
+            return value.as_ref().map_or(VariableValue::Null, |value| {
+                VariableValue::Text(Cow::Owned(value.as_json_str().into_owned()))
+            });
+        }
+        let values = match policy {
+            LookupPolicy::SetThenPost => self.post,
+            LookupPolicy::GetOnly | LookupPolicy::SetThenGet => self.get,
+        };
+        if matches!(policy, LookupPolicy::SetThenGet) && self.post.contains_key(name) {
+            if values.contains_key(name) {
+                log::warn!(
+                    "Deprecation warning! There is both a URL parameter named '{name}' and a form field named '{name}'. SQLPage is using the URL parameter for ${name}. Please use :{name} to reference the form field explicitly."
+                );
+            } else {
+                log::warn!(
+                    "Deprecation warning! ${name} was used to reference a form field value (a POST variable). This now uses only URL parameters. Please use :{name} instead."
+                );
+            }
+        }
+        values.get(name).map_or(VariableValue::Missing, |value| {
+            VariableValue::Text(value.as_json_str())
+        })
+    }
+}
+
+/// Serialize the merged SET > POST > GET view without cloning its values.
+impl Serialize for VariableAccess<'_> {
+    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
+        let set = self.set.borrow();
+        let mut map = serializer.serialize_map(None)?;
+        for (key, value) in &*set {
+            map.serialize_entry(key, value)?;
+        }
+        for (key, value) in self.post {
+            if !set.contains_key(key) {
+                map.serialize_entry(key, value)?;
+            }
+        }
+        for (key, value) in self.get {
+            if !set.contains_key(key) && !self.post.contains_key(key) {
+                map.serialize_entry(key, value)?;
+            }
+        }
+        map.end()
+    }
+}
+
 pub fn param_map<PAIRS: IntoIterator<Item = (String, String)>>(values: PAIRS) -> ParamMap {
     values
         .into_iter()
@@ -26,3 +111,47 @@ pub fn param_map<PAIRS: IntoIterator<Item = (String, String)>>(values: PAIRS) ->
             map
         })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use LookupPolicy::{GetOnly, SetThenGet, SetThenPost};
+    use VariableValue::{Missing, Null, Text};
+
+    #[test]
+    fn lookup_distinguishes_null_missing_and_releases_set_borrows() {
+        let get = param_map([("value".into(), "get".into())]);
+        let post = param_map([("value".into(), "post".into())]);
+        for (policy, fallback) in [(GetOnly, "get"), (SetThenGet, "get"), (SetThenPost, "post")] {
+            let set = RefCell::new(SetVariablesMap::from([("value".into(), None)]));
+            let view = VariableAccess::new(&get, &post, &set);
+            assert!(matches!(view.lookup("absent", policy), Missing));
+            let get_only = matches!(policy, GetOnly);
+            assert_eq!(matches!(view.lookup("value", policy), Null), !get_only);
+            for (value, expected) in [
+                (SingleOrVec::Single("set".into()), "set"),
+                (
+                    SingleOrVec::Vec(vec!["a".into(), "b".into()]),
+                    r#"["a","b"]"#,
+                ),
+            ] {
+                set.borrow_mut().insert("value".into(), Some(value));
+                let Text(actual) = view.lookup("value", policy) else {
+                    panic!("missing SET")
+                };
+                if expected.starts_with('[') {
+                    assert_eq!(
+                        serde_json::to_value(&view).unwrap()["value"],
+                        serde_json::json!(["a", "b"])
+                    );
+                }
+                set.borrow_mut().clear();
+                assert_eq!(matches!(actual, Cow::Borrowed(_)), get_only);
+                assert_eq!(actual, if get_only { "get" } else { expected });
+                assert!(
+                    matches!(view.lookup("value", policy), Text(Cow::Borrowed(value)) if value == fallback)
+                );
+            }
+        }
+    }
+}

tests/basic/mod.rs

@@ -4,22 +4,21 @@ use actix_web::{
     test,
 };
 
-use crate::common::req_path;
+use crate::common::{response_for, response_with, test_config};
 
 #[actix_web::test]
 async fn test_index_ok() {
-    let resp = req_path("/").await.unwrap();
+    let resp = response_for("/").await;
     assert_eq!(resp.status(), http::StatusCode::OK);
-    let body = test::read_body(resp).await;
-    assert!(body.starts_with(b"<!DOCTYPE html>"));
-    let body = String::from_utf8(body.to_vec()).unwrap();
+    let body = crate::common::read_body_string(resp).await;
+    assert!(body.starts_with("<!DOCTYPE html>"));
     assert!(body.contains("It works !"));
     assert!(!body.contains("error"));
 }
 
 #[actix_web::test]
 async fn test_access_config_forbidden() {
-    let resp_result = req_path("/sqlpage/sqlpage.json").await;
+    let resp_result = response_with("/sqlpage/sqlpage.json", test_config()).await;
     assert!(
         resp_result.is_err(),
         "Accessing the config file should be forbidden, but we received a response: {resp_result:?}"
@@ -35,19 +34,16 @@ async fn test_access_config_forbidden() {
 
 #[actix_web::test]
 async fn test_static_files() {
-    let resp = req_path("/tests/it_works.txt").await.unwrap();
+    let resp = response_for("/tests/it_works.txt").await;
     assert_eq!(resp.status(), http::StatusCode::OK);
     let body = test::read_body(resp).await;
     assert_eq!(&body, &b"It works !"[..]);
 }
 
 #[actix_web::test]
 async fn test_spaces_in_file_names() {
-    let resp = req_path("/tests/core/spaces%20in%20file%20name.sql")
-        .await
-        .unwrap();
+    let resp = response_for("/tests/core/spaces%20in%20file%20name.sql").await;
     assert_eq!(resp.status(), http::StatusCode::OK);
-    let body = test::read_body(resp).await;
-    let body_str = String::from_utf8(body.to_vec()).unwrap();
+    let body_str = crate::common::read_body_string(resp).await;
     assert!(body_str.contains("It works !"), "{body_str}");
 }