diff --git a/src/webserver/database/sqlpage_expr.rs b/src/webserver/database/sqlpage_expr.rs index 126a133fc..7b7ecd769 100644 --- a/src/webserver/database/sqlpage_expr.rs +++ b/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), + } } } diff --git a/src/webserver/database/sqlpage_functions/functions/variables.rs b/src/webserver/database/sqlpage_functions/functions/variables.rs index d482b1534..8ca8aa46e 100644 --- a/src/webserver/database/sqlpage_functions/functions/variables.rs +++ b/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)? }) } diff --git a/src/webserver/request_variables.rs b/src/webserver/request_variables.rs index 1b73f4120..c2ca579c8 100644 --- a/src/webserver/request_variables.rs +++ b/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; pub type SetVariablesMap = HashMap>; +/// 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, +} + +#[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, + ) -> 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(&self, serializer: S) -> Result { + 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>(values: PAIRS) -> ParamMap { values .into_iter() @@ -26,3 +111,47 @@ pub fn param_map>(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) + ); + } + } + } +} diff --git a/tests/requests/mod.rs b/tests/requests/mod.rs index bd308f96a..12f3a2611 100644 --- a/tests/requests/mod.rs +++ b/tests/requests/mod.rs @@ -135,8 +135,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 = get_request_to(url) .await? .insert_header(("content-type", "application/x-www-form-urlencoded")) @@ -152,30 +152,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", @@ -184,9 +184,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(); diff --git a/tests/requests/variables.sql b/tests/requests/variables.sql index 4f9bf5ca6..ec9c9d84b 100644 --- a/tests/requests/variables.sql +++ b/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; diff --git a/tests/sql_test_files/data/immutable_variables.sql b/tests/sql_test_files/data/immutable_variables.sql index 2fe3285c1..31d82d021 100644 --- a/tests/sql_test_files/data/immutable_variables.sql +++ b/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; diff --git a/tests/sql_test_files/data/run_sql_variable_isolation.sql b/tests/sql_test_files/data/run_sql_variable_isolation.sql new file mode 100644 index 000000000..77ef2cf32 --- /dev/null +++ b/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; diff --git a/tests/sql_test_files/variable_scope_child.sql b/tests/sql_test_files/variable_scope_child.sql new file mode 100644 index 000000000..20689fe52 --- /dev/null +++ b/tests/sql_test_files/variable_scope_child.sql @@ -0,0 +1,3 @@ +select $scope as inherited; +set scope = 'child'; +select $scope as changed;