From 234e6d6e7eb0d22e7c7d34c2eeb74eebd009d7f2 Mon Sep 17 00:00:00 2001
From: Brent Schroeter <contact@brentsch.com>
Date: Tue, 13 Jan 2026 18:10:44 +0000
Subject: [PATCH] fix filter expression sql syntax bug

---
 phono-models/src/expression.rs                | 14 ++++-
 .../relations_single/get_data_handler.rs      | 59 +++++++++----------
 2 files changed, 42 insertions(+), 31 deletions(-)

diff --git a/phono-models/src/expression.rs b/phono-models/src/expression.rs
index d110f71..b3cfde9 100644
--- a/phono-models/src/expression.rs
+++ b/phono-models/src/expression.rs
@@ -5,6 +5,9 @@ use serde::{Deserialize, Serialize};
 
 use crate::datum::Datum;
 
+/// Representation of a partial, parameterized SQL query. Allows callers to
+/// build queries iteratively and dynamically, handling parameter numbering
+/// (`$1`, `$2`, `$3`, ...) automatically.
 #[derive(Clone, Debug, PartialEq)]
 pub struct QueryFragment {
     /// SQL string, split wherever there is a query parameter. For example,
@@ -34,10 +37,12 @@ impl QueryFragment {
             .join("")
     }
 
+    /// Returns only the parameterized values, in order.
     pub fn to_params(&self) -> Vec<Datum> {
         self.params.clone()
     }
 
+    /// Parse from a SQL string with no parameters.
     pub fn from_sql(sql: &str) -> Self {
         Self {
             plain_sql: vec![sql.to_owned()],
@@ -45,6 +50,8 @@ impl QueryFragment {
         }
     }
 
+    /// Parse from a parameter value with no additional SQL. (Renders as `$n`,
+    /// where`n` is the appropriate parameter index.)
     pub fn from_param(param: Datum) -> Self {
         Self {
             plain_sql: vec!["".to_owned(), "".to_owned()],
@@ -52,6 +59,7 @@ impl QueryFragment {
         }
     }
 
+    /// Append another query fragment to this one.
     pub fn push(&mut self, mut other: QueryFragment) {
         assert!(self.plain_sql.len() == self.params.len() + 1);
         assert!(other.plain_sql.len() == other.params.len() + 1);
@@ -70,7 +78,8 @@ impl QueryFragment {
         self.params.append(&mut other.params);
     }
 
-    /// Combine multiple QueryFragments with a separator, similar to Vec::join().
+    /// Combine multiple QueryFragments with a separator, similar to
+    /// [`Vec::join`].
     pub fn join<I: IntoIterator<Item = Self>>(fragments: I, sep: Self) -> Self {
         let mut acc = QueryFragment::from_sql("");
         let mut iter = fragments.into_iter();
@@ -94,6 +103,9 @@ impl QueryFragment {
     }
 }
 
+/// Building block of a syntax tree for a constrained subset of SQL that can be
+/// statically analyzed, to validate that user-provided expressions perform only
+/// operations that are read-only and otherwise safe to execute.
 #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
 #[serde(tag = "t", content = "c")]
 pub enum PgExpressionAny {
diff --git a/phono-server/src/routes/relations_single/get_data_handler.rs b/phono-server/src/routes/relations_single/get_data_handler.rs
index 4614d5c..207d478 100644
--- a/phono-server/src/routes/relations_single/get_data_handler.rs
+++ b/phono-server/src/routes/relations_single/get_data_handler.rs
@@ -11,6 +11,7 @@ use phono_backends::{
 use phono_models::{
     accessors::{Accessor, Actor, portal::PortalAccessor},
     datum::Datum,
+    expression::QueryFragment,
     field::Field,
 };
 use serde::{Deserialize, Serialize};
@@ -96,39 +97,37 @@ pub(super) async fn get(
         field_info
     };
 
-    let mut sql_raw = format!(
-        "select {0} from {1}.{2} order by _id",
-        pkey_attrs
-            .iter()
-            .chain(attrs.iter())
-            .map(|attr| escape_identifier(&attr.attname))
-            .collect::<Vec<_>>()
-            .join(", "),
-        escape_identifier(&rel.regnamespace),
-        escape_identifier(&rel.relname),
-    );
-    let rows: Vec<PgRow> = if let Some(filter_expr) = portal.table_filter.0 {
-        let filter_fragment = filter_expr.into_query_fragment();
-        let filter_params = filter_fragment.to_params();
-        sql_raw = format!(
-            "{sql_raw} where {0} limit ${1}",
-            filter_fragment.to_sql(1),
-            filter_params.len() + 1
-        );
-        let mut q = query(&sql_raw);
-        for param in filter_params {
-            q = param.bind_onto(q);
+    let sql_fragment = {
+        // Defensive programming: Make `sql_fragment` immutable once built.
+        let mut sql_fragment = QueryFragment::from_sql(&format!(
+            "select {0} from {1}",
+            pkey_attrs
+                .iter()
+                .chain(attrs.iter())
+                .map(|attr| escape_identifier(&attr.attname))
+                .collect::<Vec<_>>()
+                .join(", "),
+            rel.get_identifier(),
+        ));
+        if let Some(filter_expr) = portal.table_filter.0 {
+            sql_fragment.push(QueryFragment::from_sql(" where "));
+            sql_fragment.push(filter_expr.into_query_fragment());
         }
-        q = q.bind(FRONTEND_ROW_LIMIT);
-        q.fetch_all(workspace_client.get_conn()).await?
-    } else {
-        sql_raw = format!("{sql_raw} limit $1");
-        query(&sql_raw)
-            .bind(FRONTEND_ROW_LIMIT)
-            .fetch_all(workspace_client.get_conn())
-            .await?
+        sql_fragment.push(QueryFragment::from_sql(" order by _id limit "));
+        sql_fragment.push(QueryFragment::from_param(Datum::Numeric(Some(
+            FRONTEND_ROW_LIMIT.into(),
+        ))));
+        sql_fragment
     };
 
+    let sql_raw = sql_fragment.to_sql(1);
+    let mut q = query(&sql_raw);
+    for param in sql_fragment.to_params() {
+        q = param.bind_onto(q);
+    }
+    q = q.bind(FRONTEND_ROW_LIMIT);
+    let rows: Vec<PgRow> = q.fetch_all(workspace_client.get_conn()).await?;
+
     #[derive(Serialize)]
     struct DataRow {
         pkey: String,