Resolve merge-queue conflict with variable lookup refactor

Merge the queued squash commit for PR #1535. Preserve its array and null-value assertions alongside PR #1529 production-app request helpers.

Author: lovasoa

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/requests/mod.rs

@@ -127,8 +127,8 @@ async fn test_large_form_field_roundtrip() -> actix_web::Result<()> {
 
 #[actix_web::test]
 async fn test_variables_function() -> actix_web::Result<()> {
-    let url = "/tests/requests/variables.sql?common=get_value&get_only=get_val";
-    let req_body = "common=post_value&post_only=post_val";
+    let url = "/tests/requests/variables.sql?common=get_value&get_only=get_val&array[]=get_a&array[]=get_b";
+    let req_body = "common=post_value&post_only=post_val&array[]=post_a&array[]=post_b";
     let req = request_for(url)
         .insert_header(("content-type", "application/x-www-form-urlencoded"))
         .insert_header(("accept", "application/json"))
@@ -142,30 +142,30 @@ async fn test_variables_function() -> actix_web::Result<()> {
         [
             (
                 "all_vars",
-                json!({"get_only": "get_val", "common": "get_value", "post_only": "post_val", "common": "post_value"}),
+                json!({"get_only": "get_val", "common": "post_value", "post_only": "post_val", "array": ["post_a", "post_b"]}),
             ),
             (
                 "get_vars",
-                json!({"get_only": "get_val", "common": "get_value"}),
+                json!({"get_only": "get_val", "common": "get_value", "array": ["get_a", "get_b"]}),
             ),
             (
                 "post_vars",
-                json!({"post_only": "post_val", "common": "post_value"}),
+                json!({"post_only": "post_val", "common": "post_value", "array": ["post_a", "post_b"]}),
             ),
             ("set_vars", json!({})),
         ],
         [
             (
                 "all_vars",
-                json!({"get_only": "get_val", "common": "set_common_value", "post_only": "post_val", "my_set_var": "set_value"}),
+                json!({"get_only": "get_val", "common": "set_common_value", "post_only": "post_val", "my_set_var": "set_value", "array": ["post_a", "post_b"]}),
             ),
             (
                 "get_vars",
-                json!({"get_only": "get_val", "common": "get_value"}),
+                json!({"get_only": "get_val", "common": "get_value", "array": ["get_a", "get_b"]}),
             ),
             (
                 "post_vars",
-                json!({"post_only": "post_val", "common": "post_value"}),
+                json!({"post_only": "post_val", "common": "post_value", "array": ["post_a", "post_b"]}),
             ),
             (
                 "set_vars",
@@ -174,9 +174,17 @@ async fn test_variables_function() -> actix_web::Result<()> {
         ],
     ];
 
+    let mut null_step = expected[1].clone();
+    null_step[0].1["common"] = json!(null);
+    null_step[3].1["common"] = json!(null);
     let actual_array = body_json.as_array().expect("response is nota json array");
-    for (i, expected_step) in expected.into_iter().enumerate() {
+    assert_eq!(actual_array.len(), expected.len() + 1);
+    for (i, expected_step) in expected.into_iter().chain([null_step]).enumerate() {
         let actual = &actual_array[i];
+        assert!(actual["missing"].is_null());
+        assert!(actual["post_only_compat"].is_null());
+        assert_eq!(actual["get_array_lookup"], r#"["get_a","get_b"]"#);
+        assert_eq!(actual["post_array_lookup"], r#"["post_a","post_b"]"#);
         for (key, expected_value) in expected_step {
             let actual_decoded: serde_json::Value =
                 serde_json::from_str(actual[key].as_str().unwrap()).unwrap();

tests/requests/variables.sql

@@ -2,7 +2,9 @@ select
        sqlpage.variables() as all_vars,
        sqlpage.variables('get') as get_vars,
        sqlpage.variables('post') as post_vars,
-       sqlpage.variables('set') as set_vars;
+       sqlpage.variables('set') as set_vars,
+       $absent as missing, $post_only as post_only_compat,
+       $array as get_array_lookup, :array as post_array_lookup;
 
 set my_set_var = 'set_value';
 set common = 'set_common_value';
@@ -11,4 +13,12 @@ select
        sqlpage.variables() as all_vars,
        sqlpage.variables('get') as get_vars,
        sqlpage.variables('post') as post_vars,
-       sqlpage.variables('set') as set_vars;
+       sqlpage.variables('set') as set_vars,
+       $absent as missing, $post_only as post_only_compat,
+       $array as get_array_lookup, :array as post_array_lookup;
+
+set common = NULL;
+select sqlpage.variables() as all_vars, sqlpage.variables('get') as get_vars,
+       sqlpage.variables('post') as post_vars, sqlpage.variables('set') as set_vars,
+       $absent as missing, $post_only as post_only_compat,
+       $array as get_array_lookup, :array as post_array_lookup;

tests/sql_test_files/data/immutable_variables.sql

@@ -8,3 +8,9 @@ select '"x":"set_value"' as expected_contains, sqlpage.variables('set') as actua
 select '"set_only":"only_in_set"' as expected_contains, sqlpage.variables('set') as actual;
 select '"x":"set_value"' as expected_contains, sqlpage.variables() as actual;
 select '"set_only":"only_in_set"' as expected_contains, sqlpage.variables() as actual;
+
+-- The fixture runner provides ?x=1. SET NULL suppresses $x but leaves GET enumeration immutable.
+set x = NULL;
+select NULL as expected, $x as actual;
+select '"x":null' as expected_contains, sqlpage.variables() as actual;
+select '"x":"1"' as expected_contains, sqlpage.variables('get') as actual;

tests/sql_test_files/data/run_sql_variable_isolation.sql

@@ -0,0 +1,8 @@
+set scope = 'parent';
+set child_rows = sqlpage.run_sql('tests/sql_test_files/variable_scope_child.sql');
+select '"inherited":"parent"' as expected_contains, $child_rows as actual;
+select '"changed":"child"' as expected_contains, $child_rows as actual;
+select 'parent' as expected, $scope as actual;
+set child_rows = sqlpage.run_sql('tests/sql_test_files/variable_scope_child.sql', '{"scope":null}');
+select '"inherited":null' as expected_contains, $child_rows as actual;
+select 'parent' as expected, $scope as actual;

tests/sql_test_files/variable_scope_child.sql

@@ -0,0 +1,3 @@
+select $scope as inherited;
+set scope = 'child';
+select $scope as changed;