Move database manager SQL queries to backend

[diegoimbert]

No description provided.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	a59a49a
Status:	✅  Deploy successful!
Preview URL:	https://36bb7d96.windmill.pages.dev
Branch Preview URL:	https://query-builders-backend.windmill.pages.dev

View logs

[diegoimbert]

/ai review this PR

[diegoimbert]

/ai review this PR

[claude]

Claude finished @diegoimbert's task in 2m 33s —— View job

---

PR Review: Move database manager SQL queries to backend

• Read and understand all changed files
• Review backend changes (Rust)
• Review frontend changes (Svelte/TypeScript)
• Post detailed review feedback

---

Overview

This PR introduces a "marker-based" architecture for the Database Manager's SQL query building. Instead of generating full SQL on the frontend (which creates policy/digest issues), the frontend now sends lightweight -- WM_INTERNAL_DB_<OP> {...} comment markers containing a JSON payload. These markers are expanded into real SQL either:

1. At execution time in the worker (worker.rs) — for actual query execution
2. Via a new API endpoint (/internal_db/expand_marker) — for SQL preview in the table editor

This is a well-motivated architectural change that improves security (SQL generation moves server-side) and solves the policy digest mismatch problem that arises when SQL is generated client-side.

---

Strengths

1. Clean separation of concerns: The marker pattern is elegant — the frontend declares intent (what operation + parameters), and the backend handles SQL generation.
2. Comprehensive test coverage: ~1700 lines of tests covering all 6 database types across SELECT, COUNT, DELETE, INSERT, UPDATE, DDL, and metadata operations.
3. Backwards compatibility: Legacy frontend query builders are preserved with clear documentation comments explaining they exist only for Database Studio app policy compatibility.
4. Proper SQL escaping: escape_sql_literal() and render_db_quoted_identifier() are used consistently for interpolated values.

---

Issues & Suggestions

🔴 SQL Injection Risk — Snowflake SHOW PRIMARY KEYS

File: backend/windmill-common/src/query_builders.rs:2475

Some(table) => Ok(format!("SHOW PRIMARY KEYS IN TABLE {}", table)),

The table value from the JSON payload is interpolated directly into the SQL without any escaping or quoting. While Snowflake's SHOW PRIMARY KEYS has limited syntax, a malicious table name could potentially inject commands. Consider using quote_table_name():

Some(table) => Ok(format!("SHOW PRIMARY KEYS IN TABLE {}", quote_table_name(&table, DbType::Snowflake))),

Fix this →

🟡 BigQuery Metadata — Dataset Name Not Escaped

File: backend/windmill-common/src/query_builders.rs:2399-2408

let dataset = parts[0];
let tname = parts[1];
Ok(format!(
    "...FROM\n    {}.INFORMATION_SCHEMA.COLUMNS c\n    LEFT JOIN\n    {}.INFORMATION_SCHEMA.KEY_COLUMN_USAGE p...",
    dataset, dataset, escape_sql_literal(tname)
))

The dataset name is interpolated raw while tname is properly escaped. The dataset should be quoted with backticks via qi(dataset, DbType::Bigquery).

Fix this →

🟡 count_query — Convoluted WHERE/AND Logic

File: backend/windmill-common/src/query_builders.rs:1022-1047

The logic that builds the WHERE clause for count queries uses string replacement (replacen) to swap " AND " with " WHERE " when there's no where_clause. This is fragile and hard to follow. Consider building the WHERE clause more explicitly:

let where_keyword = if where_clause.is_some() || db_type == DbType::MsSqlServer {
    " WHERE "
} else {
    " WHERE " // always use WHERE, build conditions list
};

This mirrors how SELECT queries handle it more cleanly.

🟡 Missing language_override in expand_marker API Response

File: backend/windmill-api/src/internal_db.rs:31

Some(Ok(expanded)) => Ok(Json(ExpandMarkerResponse { code: expanded.code })),

The ExpandedQuery struct has a language_override field (used e.g. for BigQuery all-tables which returns Bun code), but the API response only returns code. If this endpoint is ever used for metadata expansion, the caller won't know the language changed. Consider returning the override:

#[derive(Serialize)]
struct ExpandMarkerResponse {
    code: String,
    language_override: Option<ScriptLang>,
}

🟢 Minor: #[cfg(test)] on table_ref Helper

File: backend/windmill-common/src/query_builders.rs:1669-1677

The table_ref function is marked #[cfg(test)] but could be useful as the canonical way to build schema-qualified table references in non-test code too. The production code in make_alter_table_queries and make_create_table_query duplicates this logic inline. Consider making it a shared helper.

🟢 Minor: Frontend — computePreview Now Async

File: frontend/src/lib/components/DBTableEditor.svelte:81

The computePreview prop changed from sync to async (Promise<...>), and the component correctly added previewLoading state and error handling. This is a clean change. Just noting that the computeBtnProps function in DBManager.svelte still computes the diff synchronously to check diff.operations.length, which is correct since it doesn't need the SQL preview.

🟢 Minor: Copy Button Changed from <Button> to <button>

File: frontend/src/lib/components/DBTableEditor.svelte:493-498

The clipboard copy button was changed from the design-system <Button> component to a raw <button>. Per project guidelines, Windmill's design-system components should be preferred. This is minor since it's a small icon-only button in a code preview area.

---

Architecture Questions

1. Dual expansion paths: The marker is expanded both in the worker (worker.rs:3935-3955) and via the API endpoint (internal_db.rs). The worker path handles the language_override but the API path doesn't. Is there a scenario where the API path needs it?

2. 4367-line file: query_builders.rs is very large. Consider splitting into submodules (e.g., query_builders/select.rs, query_builders/ddl.rs, query_builders/metadata.rs) for maintainability.

---

Summary

Overall this is a solid, well-tested PR that moves SQL generation to a more secure and maintainable location. The main actionable items are:

Priority	Issue
🔴	Snowflake SHOW PRIMARY KEYS — table name not escaped
🟡	BigQuery metadata — dataset name not quoted
🟡	Count query WHERE/AND logic is fragile
🟡	expand_marker API doesn't return language_override