bidirectional params fix by JasonShin · Pull Request #268 · JasonShin/sqlx-ts

JasonShin · 2026-03-25T13:22:45Z

No description provided.

Copilot

Pull request overview

This PR addresses Issue #266 by improving SQL parameter detection when placeholders appear on the left side of comparisons and within BETWEEN clauses, and adds regression tests for both Postgres and MySQL.

Changes:

Extend query-param inference to handle reversed binary comparisons (placeholder on the left).
Add explicit handling for BETWEEN when the placeholder is the expr (e.g. ? BETWEEN col1 AND col2 / $1 BETWEEN ...).
Add Postgres/MySQL regression tests covering these cases.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
tests/postgres_query_parameters.rs	Adds Postgres regression tests for reversed comparisons and `BETWEEN` placeholder placement.
tests/mysql_query_parameters.rs	Adds MySQL regression tests for reversed comparisons and `BETWEEN` placeholder placement.
src/ts_generator/sql_parser/expressions/translate_expr.rs	Updates expression translation to detect params in reversed comparisons and `BETWEEN` with placeholder `expr`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-25T13:29:09Z

src/ts_generator/sql_parser/expressions/translate_expr.rs

+      let expr_is_placeholder = get_expr_placeholder(expr).is_some();
+
+      if expr_is_placeholder {
+        // Case 2: `? BETWEEN low_col AND high_col`
+        // The placeholder is the expr itself, infer its type from low (or high) column
+        let result = get_sql_query_param_directed(
+          low,
+          expr,
+          single_table_name,
+          table_with_joins,
+          db_conn,
+          &ts_query.table_valued_function_columns,
+        )
+        .await?;
+        if let Some((value, is_nullable, placeholder)) = result {
+          ts_query.insert_param(&value, &is_nullable, &placeholder)?;
+        } else {
+          // Fallback: try high column
+          let result = get_sql_query_param_directed(
+            high,
+            expr,
+            single_table_name,
+            table_with_joins,
+            db_conn,
+            &ts_query.table_valued_function_columns,
+          )
+          .await?;
+          if let Some((value, is_nullable, placeholder)) = result {
+            ts_query.insert_param(&value, &is_nullable, &placeholder)?;
+          }
+        }


In the Expr::Between branch when expr is a placeholder, if neither low nor high can be treated as a column (e.g. ? BETWEEN 1 AND 10, or columns are wrapped in casts/expressions), result stays None and the placeholder in expr is never inserted into ts_query.params. This will generate a params tuple that is missing an argument. Consider falling back to collecting the placeholder from expr (e.g. call translate_expr(expr, ...) or directly insert_param with a conservative type) when both directional lookups fail.

Copilot · 2026-03-25T13:29:09Z

src/ts_generator/sql_parser/expressions/translate_expr.rs


  if table_with_joins.is_some() {
-    table_name = translate_table_from_expr(table_with_joins, &left.clone()).ok();
+    table_name = translate_table_from_expr(table_with_joins, &column_expr.clone()).ok();


translate_table_from_expr(table_with_joins, &column_expr.clone()) clones the entire Expr just to pass a reference. This is unnecessary work on a potentially large AST node; passing column_expr directly should be sufficient.

Suggested change

table_name = translate_table_from_expr(table_with_joins, &column_expr.clone()).ok();

table_name = translate_table_from_expr(table_with_joins, column_expr).ok();

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-29T02:30:21Z

test-utils/src/sandbox.rs

+/// Checks if the MySQL server at the given host:port is above the specified major.minor version.
+/// Uses `docker exec` to query the version. Returns true if version cannot be determined.
+pub fn is_mysql_version_above(db_host: &str, db_port: i32, major: u32, minor: u32) -> bool {


is_mysql_version_above’s signature suggests the version check is performed against the provided db_host:db_port, but those parameters are unused. Either use them (e.g., run mysql against that host/port) or rename/underscore the parameters / adjust the doc comment to avoid misleading callers and future maintainers.

Suggested change

/// Checks if the MySQL server at the given host:port is above the specified major.minor version.

/// Uses `docker exec` to query the version. Returns true if version cannot be determined.

pub fn is_mysql_version_above(db_host: &str, db_port: i32, major: u32, minor: u32) -> bool {

/// Checks if the MySQL server used for tests is above the specified major.minor version.

/// Uses `docker exec` against the `sqlx-ts-mysql-1` container to query the version.

/// Returns true if the version cannot be determined.

pub fn is_mysql_version_above(_db_host: &str, _db_port: i32, major: u32, minor: u32) -> bool {

Copilot · 2026-03-29T02:30:22Z

test-utils/src/sandbox.rs

+/// Uses `docker exec` to query the version. Returns true if version cannot be determined.
+pub fn is_mysql_version_above(db_host: &str, db_port: i32, major: u32, minor: u32) -> bool {
+  let output = std::process::Command::new("docker")
+    .args(["exec", "sqlx-ts-mysql-1", "mysql", "-u", "root", "-N", "-e", "SELECT VERSION();"])
+    .output();
+  match output {
+    Ok(out) => {
+      let version = String::from_utf8_lossy(&out.stdout);
+      let version = version.trim();
+      let parts: Vec<&str> = version.split('.').collect();
+      if parts.len() >= 2 {
+        let srv_major: u32 = parts[0].parse().unwrap_or(0);
+        let srv_minor: u32 = parts[1].parse().unwrap_or(0);
+        srv_major > major || (srv_major == major && srv_minor > minor)
+      } else {
+        true
+      }
+    }
+    Err(_) => true,


is_mysql_version_above ignores db_host/db_port and hard-codes the container name (sqlx-ts-mysql-1). In addition, it treats any command failure / empty output as “version above”, which can make the min-version skip logic ineffective and lead to flaky failures on older MySQL images. Consider resolving the container dynamically (e.g., via docker compose ps -q mysql or using the provided host/port), and only returning true when the command succeeds and the parsed version is actually above the threshold (otherwise return false so tests are skipped safely).

Suggested change

/// Uses `docker exec` to query the version. Returns true if version cannot be determined.

pub fn is_mysql_version_above(db_host: &str, db_port: i32, major: u32, minor: u32) -> bool {

let output = std::process::Command::new("docker")

.args(["exec", "sqlx-ts-mysql-1", "mysql", "-u", "root", "-N", "-e", "SELECT VERSION();"])

.output();

match output {

Ok(out) => {

let version = String::from_utf8_lossy(&out.stdout);

let version = version.trim();

let parts: Vec<&str> = version.split('.').collect();

if parts.len() >= 2 {

let srv_major: u32 = parts[0].parse().unwrap_or(0);

let srv_minor: u32 = parts[1].parse().unwrap_or(0);

srv_major > major || (srv_major == major && srv_minor > minor)

} else {

true

}

}

Err(_) => true,

/// Uses the `mysql` client to query the version. Returns false if version cannot be determined.

pub fn is_mysql_version_above(db_host: &str, db_port: i32, major: u32, minor: u32) -> bool {

let port = db_port.to_string();

let output = std::process::Command::new("mysql")

.args(["-h", db_host, "-P", &port, "-u", "root", "-N", "-e", "SELECT VERSION();"])

.output();

match output {

Ok(out) => {

if !out.status.success() {

return false;

}

let version = String::from_utf8_lossy(&out.stdout);

let version = version.trim();

if version.is_empty() {

return false;

}

let parts: Vec<&str> = version.split('.').collect();

if parts.len() >= 2 {

if let (Ok(srv_major), Ok(srv_minor)) = (parts[0].parse::<u32>(), parts[1].parse::<u32>()) {

return srv_major > major || (srv_major == major && srv_minor > minor);

}

}

false

}

Err(_) => false,

JasonShin added 2 commits March 26, 2026 00:21

fix

5abc2af

fmt

fdc2f78

Copilot AI review requested due to automatic review settings March 25, 2026 13:22

Copilot started reviewing on behalf of JasonShin March 25, 2026 13:25 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

fix

0d467c6

JasonShin changed the title ~~between fix~~ bidirectional params fix Mar 29, 2026

add tests cases

2a983c3

Copilot AI review requested due to automatic review settings March 29, 2026 02:23

Copilot started reviewing on behalf of JasonShin March 29, 2026 02:25 View session

JasonShin merged commit 8d09eb8 into main Mar 29, 2026
36 checks passed

JasonShin deleted the between-clause branch March 29, 2026 02:29

JasonShin mentioned this pull request Mar 29, 2026

Query params only detected properly if parameter comes after condition #266

Closed

Copilot AI reviewed Mar 29, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bidirectional params fix#268

bidirectional params fix#268
JasonShin merged 4 commits intomainfrom
between-clause

JasonShin commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 25, 2026

Uh oh!

Copilot AI Mar 25, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	table_name = translate_table_from_expr(table_with_joins, &column_expr.clone()).ok();
	table_name = translate_table_from_expr(table_with_joins, column_expr).ok();

-/// Checks if the MySQL server at the given host:port is above the specified major.minor version.
-/// Uses `docker exec` to query the version. Returns true if version cannot be determined.
-pub fn is_mysql_version_above(db_host: &str, db_port: i32, major: u32, minor: u32) -> bool {
+/// Checks if the MySQL server used for tests is above the specified major.minor version.
+/// Uses `docker exec` against the `sqlx-ts-mysql-1` container to query the version.
+/// Returns true if the version cannot be determined.
+pub fn is_mysql_version_above(_db_host: &str, _db_port: i32, major: u32, minor: u32) -> bool {

Conversation

JasonShin commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants