fix #701

lovasoa · lovasoa · commit 4f8d21c3a70d · 2024-11-18T21:42:22.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,6 +40,7 @@
 - Fixed layout issues in the card component when embedding content with `embed`: remove double border and padding.
   - ![embedded card screenshot](https://github.com/user-attachments/assets/ea85438d-5fcb-4eed-b90b-a4385675355d)
 - Added support for `empty_option` in the form component to add an empty option before the options defined in `options`. Useful when generating other options from a database table.
+- Allow nested json objects and arrays as sqlpage function parameters (useful in `sqlpage.fetch`).
 
 ## 0.30.1 (2024-10-31)
 - fix a bug where table sorting would break if table search was not also enabled.
diff --git a/src/webserver/database/execute_queries.rs b/src/webserver/database/execute_queries.rs
@@ -96,7 +96,10 @@ pub fn stop_at_first_error(
     results_stream.take_while(move |item| {
         // We stop the stream AFTER the first error, so that the error is still returned to the client, but the rest of the queries are not executed.
         let should_continue = !has_error;
-        has_error |= matches!(item, DbItem::Error(_));
+        if let DbItem::Error(err) = item {
+            log::error!("{err:?}");
+            has_error = true;
+        }
         futures_util::future::ready(should_continue)
     })
 }
@@ -167,7 +170,6 @@ async fn execute_set_variable_query<'a>(
         Ok(None) => None,
         Err(e) => {
             let err = display_db_error(source_file, &statement.query, e);
-            log::error!("{err}");
             return Err(err);
         }
     };
@@ -244,7 +246,6 @@ fn parse_single_sql_result(
         }
         Err(err) => {
             let nice_err = display_db_error(source_file, sql, err);
-            log::error!("{:?}", nice_err);
             DbItem::Error(nice_err)
         }
     }
diff --git a/src/webserver/database/sql.rs b/src/webserver/database/sql.rs
@@ -144,16 +144,8 @@ fn parse_single_statement(
         semicolon = true;
     }
     let mut params = ParameterExtractor::extract_parameters(&mut stmt, db_kind);
-    if let Some((variable, query)) = extract_set_variable(&mut stmt) {
-        return Some(ParsedStatement::SetVariable {
-            variable,
-            value: StmtWithParams {
-                query,
-                params,
-                delayed_functions: Vec::new(),
-                json_columns: Vec::new(),
-            },
-        });
+    if let Some((variable, value)) = extract_set_variable(&mut stmt, &mut params, db_kind) {
+        return Some(ParsedStatement::SetVariable { variable, value });
     }
     if let Some(csv_import) = extract_csv_copy_statement(&mut stmt) {
         return Some(ParsedStatement::CsvImport(csv_import));
@@ -406,7 +398,11 @@ fn is_simple_select_placeholder(e: &Expr) -> bool {
     }
 }
 
-fn extract_set_variable(stmt: &mut Statement) -> Option<(StmtParam, String)> {
+fn extract_set_variable(
+    stmt: &mut Statement,
+    params: &mut Vec<StmtParam>,
+    db_kind: AnyKind,
+) -> Option<(StmtParam, StmtWithParams)> {
     if let Statement::SetVariable {
         variables: OneOrManyWithParens::One(ObjectName(name)),
         value,
@@ -420,7 +416,20 @@ fn extract_set_variable(stmt: &mut Statement) -> Option<(StmtParam, String)> {
             } else {
                 StmtParam::PostOrGet(std::mem::take(&mut ident.value))
             };
-            return Some((variable, format!("SELECT {value}")));
+            let owned_expr = std::mem::replace(value, Expr::Value(Value::Null));
+            let mut select_stmt: Statement = expr_to_statement(owned_expr);
+            let delayed_functions = extract_toplevel_functions(&mut select_stmt);
+            remove_invalid_function_calls(&mut select_stmt, params);
+            let json_columns = extract_json_columns(&select_stmt, db_kind);
+            return Some((
+                variable,
+                StmtWithParams {
+                    query: select_stmt.to_string(),
+                    params: std::mem::take(params),
+                    delayed_functions,
+                    json_columns,
+                },
+            ));
         }
     }
     None
@@ -862,6 +871,47 @@ fn is_json_function(expr: &Expr) -> bool {
     }
 }
 
+fn expr_to_statement(expr: Expr) -> Statement {
+    Statement::Query(Box::new(sqlparser::ast::Query {
+        with: None,
+        body: Box::new(sqlparser::ast::SetExpr::Select(Box::new(
+            sqlparser::ast::Select {
+                distinct: None,
+                top: None,
+                projection: vec![SelectItem::ExprWithAlias {
+                    expr,
+                    alias: Ident::new("sqlpage_set_expr"),
+                }],
+                into: None,
+                from: vec![],
+                lateral_views: vec![],
+                selection: None,
+                group_by: sqlparser::ast::GroupByExpr::Expressions(vec![], vec![]),
+                cluster_by: vec![],
+                distribute_by: vec![],
+                sort_by: vec![],
+                having: None,
+                named_window: vec![],
+                qualify: None,
+                top_before_distinct: false,
+                prewhere: None,
+                window_before_qualify: false,
+                value_table_mode: None,
+                connect_by: None,
+            },
+        ))),
+        order_by: None,
+        limit: None,
+        offset: None,
+        fetch: None,
+        locks: vec![],
+        limit_by: vec![],
+        for_clause: None,
+        settings: None,
+        format_clause: None,
+    }))
+}
+
 #[cfg(test)]
 mod test {
     use super::super::sqlpage_functions::functions::SqlPageFunctionName;
@@ -1169,7 +1219,7 @@ mod test {
                     StmtParam::PostOrGet("x".to_string()),
                     "{dialect:?}"
                 );
-                assert_eq!(query, "SELECT 42");
+                assert_eq!(query, "SELECT 42 AS sqlpage_set_expr");
                 assert!(params.is_empty());
             } else {
                 panic!("Failed for dialect {dialect:?}: {stmt:#?}",);
@@ -1261,4 +1311,43 @@ mod test {
             ]
         );
     }
+
+    #[test]
+    fn test_set_variable_with_sqlpage_function() {
+        let sql = "set x = sqlpage.url_encode(some_db_function())";
+        for &(dialect, db_kind) in ALL_DIALECTS {
+            let mut parser = Parser::new(dialect).try_with_sql(sql).unwrap();
+            let stmt = parse_single_statement(&mut parser, db_kind, sql);
+            let Some(ParsedStatement::SetVariable {
+                variable,
+                value:
+                    StmtWithParams {
+                        query,
+                        params,
+                        delayed_functions,
+                        json_columns,
+                        ..
+                    },
+            }) = stmt
+            else {
+                panic!("for dialect {dialect:?}: {stmt:#?} instead of SetVariable");
+            };
+            assert_eq!(
+                variable,
+                StmtParam::PostOrGet("x".to_string()),
+                "{dialect:?}"
+            );
+            assert_eq!(
+                delayed_functions,
+                [DelayedFunctionCall {
+                    function: SqlPageFunctionName::url_encode,
+                    argument_col_names: vec!["_sqlpage_f0_a0".to_string()],
+                    target_col_name: "sqlpage_set_expr".to_string()
+                }]
+            );
+            assert_eq!(query, "SELECT some_db_function() AS _sqlpage_f0_a0");
+            assert_eq!(params, []);
+            assert_eq!(json_columns, Vec::<String>::new());
+        }
+    }
 }
diff --git a/src/webserver/database/sqlpage_functions/http_fetch_request.rs b/src/webserver/database/sqlpage_functions/http_fetch_request.rs
@@ -1,3 +1,5 @@
+use anyhow::Context;
+
 use super::function_traits::BorrowFromStr;
 use std::borrow::Cow;
 
@@ -55,10 +57,11 @@ impl<'a> BorrowFromStr<'a> for HttpFetchRequest<'a> {
             }
         } else {
             match s {
-                Cow::Borrowed(s) => serde_json::from_str(s)?,
-                Cow::Owned(s) => serde_json::from_str::<HttpFetchRequest<'_>>(&s)
-                    .map(HttpFetchRequest::into_owned)?,
+                Cow::Borrowed(s) => serde_json::from_str(s),
+                Cow::Owned(ref s) => serde_json::from_str::<HttpFetchRequest<'_>>(s)
+                    .map(HttpFetchRequest::into_owned),
             }
+            .with_context(|| format!("Invalid http fetch request definition: {s}"))?
         })
     }
 }
diff --git a/src/webserver/database/syntax_tree.rs b/src/webserver/database/syntax_tree.rs
@@ -203,8 +203,23 @@ async fn json_object_params<'a, 'b>(
         let val = it
             .next()
             .ok_or_else(|| anyhow::anyhow!("Odd number of arguments in JSON_OBJECT"))?;
-        let val = Box::pin(extract_req_param(val, request, db_connection)).await?;
-        map_ser.serialize_value(&val)?;
+
+        match val {
+            StmtParam::JsonObject(args) => {
+                let raw_json = Box::pin(json_object_params(args, request, db_connection)).await?;
+                let obj = cow_to_raw_json(&raw_json);
+                map_ser.serialize_value(&obj)?;
+            }
+            StmtParam::JsonArray(args) => {
+                let raw_json = Box::pin(json_array_params(args, request, db_connection)).await?;
+                let obj = cow_to_raw_json(&raw_json);
+                map_ser.serialize_value(&obj)?;
+            }
+            val => {
+                let evaluated = Box::pin(extract_req_param(val, request, db_connection)).await?;
+                map_ser.serialize_value(&evaluated)?;
+            }
+        };
     }
     map_ser.end()?;
     Ok(Some(Cow::Owned(String::from_utf8(result)?)))
@@ -220,9 +235,33 @@ async fn json_array_params<'a, 'b>(
     let mut ser = serde_json::Serializer::new(&mut result);
     let mut seq_ser = ser.serialize_seq(Some(args.len()))?;
     for element in args {
-        let element = Box::pin(extract_req_param(element, request, db_connection)).await?;
-        seq_ser.serialize_element(&element)?;
+        match element {
+            StmtParam::JsonObject(args) => {
+                let raw_json = json_object_params(args, request, db_connection).await?;
+                let obj = cow_to_raw_json(&raw_json);
+                seq_ser.serialize_element(&obj)?;
+            }
+            StmtParam::JsonArray(args) => {
+                let raw_json = Box::pin(json_array_params(args, request, db_connection)).await?;
+                let obj = cow_to_raw_json(&raw_json);
+                seq_ser.serialize_element(&obj)?;
+            }
+            element => {
+                let evaluated =
+                    Box::pin(extract_req_param(element, request, db_connection)).await?;
+                seq_ser.serialize_element(&evaluated)?;
+            }
+        };
     }
     seq_ser.end()?;
     Ok(Some(Cow::Owned(String::from_utf8(result)?)))
 }
+
+fn cow_to_raw_json<'a>(
+    raw_json: &'a Option<Cow<'a, str>>,
+) -> Option<&'a serde_json::value::RawValue> {
+    raw_json
+        .as_deref()
+        .map(serde_json::from_str::<&'a serde_json::value::RawValue>)
+        .map(Result::unwrap)
+}
diff --git a/tests/sql_test_files/it_works_native_json_array_impl.sql b/tests/sql_test_files/it_works_native_json_array_impl.sql
@@ -4,7 +4,7 @@ set res = sqlpage.fetch(json_object(
     'headers', json_object('x-custom', '1'),
     'body', json_array('hello', 'world')
 ));
-set expected = 'POST /post|accept-encoding: br, gzip, deflate, zstd|content-length: 18|content-type: application/json|host: localhost:62802|user-agent: sqlpage|x-custom: 1|["hello", "world"]';
+set expected = 'POST /post|accept-encoding: br, gzip, deflate, zstd|content-length: 17|content-type: application/json|host: localhost:62802|user-agent: sqlpage|x-custom: 1|["hello","world"]';
 select 'text' as component,
     case $res
         when $expected then 'It works !'
diff --git a/tests/sql_test_files/it_works_set_variable_to_sqlpage_function.sql b/tests/sql_test_files/it_works_set_variable_to_sqlpage_function.sql
@@ -0,0 +1,7 @@
+set my_var = sqlpage.url_encode(' ');
+select 'text' as component, 
+    CASE $my_var
+        WHEN '%20' THEN 'It works !'
+        ELSE 'It failed !'
+    END
+    AS contents;
diff --git a/tests/sql_test_files/it_works_set_variable_to_sqlpage_post_function.sql b/tests/sql_test_files/it_works_set_variable_to_sqlpage_post_function.sql
@@ -0,0 +1,7 @@
+set my_var = sqlpage.url_encode(UPPER('a'));
+select 'text' as component, 
+    CASE $my_var
+        WHEN 'A' THEN 'It works !'
+        ELSE 'It failed !'
+    END
+    AS contents;

Original file line number	Diff line number	Diff line change
`@@ -96,7 +96,10 @@ pub fn stop_at_first_error(`
`96`	`96`	`results_stream.take_while(move \|item\| {`
`97`	`97`	`// We stop the stream AFTER the first error, so that the error is still returned to the client, but the rest of the queries are not executed.`
`98`	`98`	`let should_continue = !has_error;`
`99`		`- has_error \|= matches!(item, DbItem::Error(_));`
	`99`	`+ if let DbItem::Error(err) = item {`
	`100`	`+ log::error!("{err:?}");`
	`101`	`+ has_error = true;`
	`102`	`+ }`
`100`	`103`	`futures_util::future::ready(should_continue)`
`101`	`104`	`})`
`102`	`105`	`}`
`@@ -167,7 +170,6 @@ async fn execute_set_variable_query<'a>(`
`167`	`170`	`Ok(None) => None,`
`168`	`171`	`Err(e) => {`
`169`	`172`	`let err = display_db_error(source_file, &statement.query, e);`
`170`		`- log::error!("{err}");`
`171`	`173`	`return Err(err);`
`172`	`174`	`}`
`173`	`175`	`};`
`@@ -244,7 +246,6 @@ fn parse_single_sql_result(`
`244`	`246`	`}`
`245`	`247`	`Err(err) => {`
`246`	`248`	`let nice_err = display_db_error(source_file, sql, err);`
`247`		`- log::error!("{:?}", nice_err);`
`248`	`249`	`DbItem::Error(nice_err)`
`249`	`250`	`}`
`250`	`251`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,5 @@`
	`1`	`+use anyhow::Context;`
	`2`	`+`
`1`	`3`	`use super::function_traits::BorrowFromStr;`
`2`	`4`	`use std::borrow::Cow;`
`3`	`5`
`@@ -55,10 +57,11 @@ impl<'a> BorrowFromStr<'a> for HttpFetchRequest<'a> {`
`55`	`57`	`}`
`56`	`58`	`} else {`
`57`	`59`	`match s {`
`58`		`- Cow::Borrowed(s) => serde_json::from_str(s)?,`
`59`		`- Cow::Owned(s) => serde_json::from_str::<HttpFetchRequest<'_>>(&s)`
`60`		`- .map(HttpFetchRequest::into_owned)?,`
	`60`	`+ Cow::Borrowed(s) => serde_json::from_str(s),`
	`61`	`+ Cow::Owned(ref s) => serde_json::from_str::<HttpFetchRequest<'_>>(s)`
	`62`	`+ .map(HttpFetchRequest::into_owned),`
`61`	`63`	`}`
	`64`	`+ .with_context(\|\| format!("Invalid http fetch request definition: {s}"))?`
`62`	`65`	`})`
`63`	`66`	`}`
`64`	`67`	`}`