From ca604eac1c8df573c35dc76f874339ab15bc88a1 Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Tue, 29 Sep 2026 13:36:39 -0400 Subject: [PATCH 1/2] fix: filter NULL column defaults case-insensitively to match builtin (#122) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit row_to_table_column's default_value filter used a case-sensitive `d == "NULL"`, so a column declared `DEFAULT null` (lowercase — legal PostgreSQL, stored verbatim in information_schema.columns.column_default) surfaced a spurious `default_value: "null"` in the wire JSON, where the built-in driver omits the field. The builtin filters case-insensitively (`eq_ignore_ascii_case("null")`) in both get_columns and get_all_columns_batch. Affects get_columns, get_all_columns_batch, and get_schema_snapshot, which all share the row_to_table_column mapper. Fixed by matching the builtin exactly: the bare-null check is now case-insensitive, while the `NULL::` cast-prefix check stays case-sensitive (the builtin keeps that one starts_with("NULL::"), not eq_ignore_ascii_case). Extracted the filter into a pure column_default_value(column_default, is_identity) helper split out of row_to_table_column for unit testing without a live tokio_postgres::Row, per the repo's extract-pure-logic pattern. TDD: added 5 unit tests for column_default_value covering the divergence (lowercase `null` — the bug), mixed-case `Null`, uppercase `NULL`, `NULL::` casts, real defaults, auto-increment (nextval + is_identity), and empty/None. Confirmed the lowercase-null test failed against the pre-fix code for the right reason (returned Some("null") instead of None) before applying the fix; all 5 pass after. Verified: cargo build --release; 361 unit + 30 live-DB against the local pg-tabularis-test podman container; the cross-repo 83-test byte-for-byte parity suite (POSTGRES_PLUGIN_BIN against tabularis' src-tauri/tests/postgres_integration parity*) passes 83/83; cargo clippy --all-targets -D warnings; cargo fmt --all --check; npx markdownlint CHANGELOG.md -- all clean. Note: the 83-test parity suite's seed fixtures declare no `DEFAULT null` columns, so this divergence was never caught by it — only a source-level audit of the builtin's filter found it (#122). --- CHANGELOG.md | 10 +++++ src/handlers/metadata.rs | 35 ++++++++++++---- src/handlers/metadata_tests.rs | 77 +++++++++++++++++++++++++++++++++- 3 files changed, 114 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7aa5180..3a43b5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,16 @@ ## [Unreleased] +### Fixed + +- `get_columns`/`get_all_columns_batch`/`get_schema_snapshot` filtered out + `NULL` column defaults case-sensitively (`d == "NULL"`), so a column + declared `DEFAULT null` (lowercase — legal PostgreSQL) surfaced a + spurious `default_value: "null"` where the built-in driver omits it. + Now filters case-insensitively (`eq_ignore_ascii_case("null")`), + matching the builtin exactly; the `NULL::` cast prefix check + stays case-sensitive as in the builtin (#122). + ## [1.0.0-rc.5] - 2026-09-29 ### Added diff --git a/src/handlers/metadata.rs b/src/handlers/metadata.rs index 0b3d344..fbd0728 100644 --- a/src/handlers/metadata.rs +++ b/src/handlers/metadata.rs @@ -284,13 +284,7 @@ fn row_to_table_column(r: &tokio_postgres::Row) -> Value { let is_nullable = is_nullable_str == "YES"; - let default_value = column_default.as_deref().and_then(|d| { - if is_auto_increment || d.is_empty() || d == "NULL" || d.starts_with("NULL::") { - None - } else { - Some(d.to_string()) - } - }); + let default_value = column_default_value(column_default.as_deref(), &is_identity); let mut col = json!({ "name": name, @@ -319,6 +313,33 @@ fn row_to_table_column(r: &tokio_postgres::Row) -> Value { col } +/// Compute a column's `default_value` wire field from its raw +/// `information_schema.columns.column_default` and `is_identity`, mirroring +/// the built-in driver's filter exactly. Returns `None` (omit the field) for +/// auto-increment columns, empty defaults, and `NULL` defaults of any case +/// (`NULL`, `null`, `Null`, ...), and for `NULL::` casts; everything +/// else is surfaced verbatim. Split out of `row_to_table_column` for unit +/// testing without a live `tokio_postgres::Row` (#122). +fn column_default_value(column_default: Option<&str>, is_identity: &str) -> Option { + let d = column_default?; + let is_auto_increment = is_identity == "YES" || d.contains("nextval"); + // Match the builtin's filter exactly: the bare-`null` check is + // case-insensitive (PostgreSQL accepts `DEFAULT null` in any case and + // stores it verbatim, so `null`/`Null`/`NULL` all mean "no default"), + // while the `NULL::` cast prefix stays case-sensitive — the + // builtin keeps that one `starts_with("NULL::")`, not + // `eq_ignore_ascii_case`. #122. + if is_auto_increment + || d.is_empty() + || d.eq_ignore_ascii_case("null") + || d.starts_with("NULL::") + { + None + } else { + Some(d.to_string()) + } +} + pub async fn get_foreign_keys(id: Value, params: &Value) -> Value { let conn_params = ConnectionParams::from_value(inner_params(params)); let table = params.get("table").and_then(Value::as_str).unwrap_or(""); diff --git a/src/handlers/metadata_tests.rs b/src/handlers/metadata_tests.rs index e7c6291..243810f 100644 --- a/src/handlers/metadata_tests.rs +++ b/src/handlers/metadata_tests.rs @@ -7,7 +7,7 @@ //! (`.rules/rust.md` #4/#5) — loaded via //! `#[cfg(test)] #[path = "metadata_tests.rs"] mod metadata_tests;`. -use super::{build_table_ddl, routine_query_for_version}; +use super::{build_table_ddl, column_default_value, routine_query_for_version}; use serde_json::{json, Map, Value}; #[test] @@ -248,3 +248,78 @@ fn build_schema_snapshot_drops_table_comments_to_match_builtin_table_schema() { "snapshot entry must be exactly the builtin's TableSchema shape (name/columns/foreign_keys), dropping comment" ); } + +#[test] +fn column_default_value_drops_a_lowercase_null_default_to_match_builtin() { + // #122: the builtin filters NULL defaults case-insensitively + // (eq_ignore_ascii_case("null")), so `DEFAULT null` (lowercase, which + // PostgreSQL accepts) must produce no default_value — same as + // `DEFAULT NULL`. The plugin's filter was case-sensitive (`== "NULL"`), + // so it emitted a spurious `default_value: "null"` here. This is the + // divergence the test proves before the fix. + assert_eq!( + column_default_value(Some("null"), "NO"), + None, + "lowercase `null` default must be dropped, matching the builtin's case-insensitive filter" + ); + assert_eq!( + column_default_value(Some("NULL"), "NO"), + None, + "uppercase `NULL` default must be dropped (this case already worked)" + ); + assert_eq!( + column_default_value(Some("Null"), "NO"), + None, + "mixed-case `Null` default must be dropped, matching the builtin's case-insensitive filter" + ); +} + +#[test] +fn column_default_value_drops_null_cast_prefixes() { + // `NULL::` casts are PostgreSQL's representation of a nullable + // column with no real default; the builtin drops these too. The + // `NULL::` prefix check stays case-sensitive in the builtin (only the + // bare-null check is eq_ignore_ascii_case), so match that exactly. + assert_eq!(column_default_value(Some("NULL::text"), "NO"), None); + assert_eq!(column_default_value(Some("NULL::integer"), "NO"), None); +} + +#[test] +fn column_default_value_surfaces_real_defaults_unchanged() { + // Non-NULL defaults pass through verbatim — the value the host shows + // in the column's default cell. + assert_eq!(column_default_value(Some("0"), "NO"), Some("0".to_string())); + assert_eq!( + column_default_value(Some("'neutral'"), "NO"), + Some("'neutral'".to_string()) + ); + assert_eq!( + column_default_value(Some("now()"), "NO"), + Some("now()".to_string()) + ); +} + +#[test] +fn column_default_value_drops_auto_increment_defaults() { + // SERIAL/IDENTITY columns carry a `nextval(...)` default or an + // is_identity of YES; the host surfaces those as is_auto_increment, + // not as default_value, so the raw default must not leak through. + assert_eq!( + column_default_value(Some("nextval('users_id_seq'::regclass)"), "NO"), + None, + "nextval default must be dropped (auto-increment)" + ); + assert_eq!( + column_default_value(Some("42"), "YES"), + None, + "is_identity=YES must drop the default regardless of its value" + ); +} + +#[test] +fn column_default_value_drops_empty_defaults() { + // An empty string is not a meaningful default — drop it rather than + // surfacing an empty default_value field. + assert_eq!(column_default_value(Some(""), "NO"), None); + assert_eq!(column_default_value(None, "NO"), None, "no default at all -> None"); +} From 7881a24d133b27bbb0215cb3e7aeda66abb6d99c Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Tue, 29 Sep 2026 13:41:31 -0400 Subject: [PATCH 2/2] refactor: compute is_auto_increment once in row_to_table_column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #122 fix extracted column_default_value with its own is_auto_increment computation, duplicating the one row_to_table_column already computes for the is_auto_increment field. The builtin computes is_auto once and reuses it for both the field and the filter; match that structure so the two can't drift. column_default_value now takes the pre-computed bool instead of is_identity, and the unit tests pass it through. Also adds a test guarding that a string *literal* 'null' (stored by PG as 'null'::text, quoted+cast) survives the filter — it doesn't match the bare-null check — which is the empirical boundary the live-probe confirmed. No behavior change: is_auto_increment was already computed identically in both places; this just dedupes it. 361 unit + 30 live-DB + 83/83 parity all pass. --- src/handlers/metadata.rs | 28 ++++++++++-------- src/handlers/metadata_tests.rs | 54 ++++++++++++++++++++++++---------- 2 files changed, 54 insertions(+), 28 deletions(-) diff --git a/src/handlers/metadata.rs b/src/handlers/metadata.rs index fbd0728..1b038b1 100644 --- a/src/handlers/metadata.rs +++ b/src/handlers/metadata.rs @@ -284,7 +284,7 @@ fn row_to_table_column(r: &tokio_postgres::Row) -> Value { let is_nullable = is_nullable_str == "YES"; - let default_value = column_default_value(column_default.as_deref(), &is_identity); + let default_value = column_default_value(column_default.as_deref(), is_auto_increment); let mut col = json!({ "name": name, @@ -314,21 +314,23 @@ fn row_to_table_column(r: &tokio_postgres::Row) -> Value { } /// Compute a column's `default_value` wire field from its raw -/// `information_schema.columns.column_default` and `is_identity`, mirroring -/// the built-in driver's filter exactly. Returns `None` (omit the field) for -/// auto-increment columns, empty defaults, and `NULL` defaults of any case -/// (`NULL`, `null`, `Null`, ...), and for `NULL::` casts; everything -/// else is surfaced verbatim. Split out of `row_to_table_column` for unit -/// testing without a live `tokio_postgres::Row` (#122). -fn column_default_value(column_default: Option<&str>, is_identity: &str) -> Option { +/// `information_schema.columns.column_default` and the already-computed +/// `is_auto_increment`, mirroring the built-in driver's filter exactly. +/// Returns `None` (omit the field) for auto-increment columns, empty +/// defaults, and `NULL` defaults of any case (`NULL`, `null`, `Null`, ...), +/// and for `NULL::` casts; everything else is surfaced verbatim. +/// Split out of `row_to_table_column` for unit testing without a live +/// `tokio_postgres::Row` (#122). `is_auto_increment` is computed once in +/// the caller (matching the builtin's single computation) and passed in +/// so the field and the filter can't drift apart. +fn column_default_value(column_default: Option<&str>, is_auto_increment: bool) -> Option { let d = column_default?; - let is_auto_increment = is_identity == "YES" || d.contains("nextval"); // Match the builtin's filter exactly: the bare-`null` check is // case-insensitive (PostgreSQL accepts `DEFAULT null` in any case and - // stores it verbatim, so `null`/`Null`/`NULL` all mean "no default"), - // while the `NULL::` cast prefix stays case-sensitive — the - // builtin keeps that one `starts_with("NULL::")`, not - // `eq_ignore_ascii_case`. #122. + // normalizes it to NULL in information_schema.columns.column_default, + // so `null`/`Null`/`NULL` all mean "no default"), while the + // `NULL::` cast prefix stays case-sensitive — the builtin keeps + // that one `starts_with("NULL::")`, not `eq_ignore_ascii_case`. #122. if is_auto_increment || d.is_empty() || d.eq_ignore_ascii_case("null") diff --git a/src/handlers/metadata_tests.rs b/src/handlers/metadata_tests.rs index 243810f..5473b77 100644 --- a/src/handlers/metadata_tests.rs +++ b/src/handlers/metadata_tests.rs @@ -257,18 +257,21 @@ fn column_default_value_drops_a_lowercase_null_default_to_match_builtin() { // `DEFAULT NULL`. The plugin's filter was case-sensitive (`== "NULL"`), // so it emitted a spurious `default_value: "null"` here. This is the // divergence the test proves before the fix. + // + // The second arg is the caller's already-computed `is_auto_increment` + // (false here — these aren't sequence/identity columns). assert_eq!( - column_default_value(Some("null"), "NO"), + column_default_value(Some("null"), false), None, "lowercase `null` default must be dropped, matching the builtin's case-insensitive filter" ); assert_eq!( - column_default_value(Some("NULL"), "NO"), + column_default_value(Some("NULL"), false), None, "uppercase `NULL` default must be dropped (this case already worked)" ); assert_eq!( - column_default_value(Some("Null"), "NO"), + column_default_value(Some("Null"), false), None, "mixed-case `Null` default must be dropped, matching the builtin's case-insensitive filter" ); @@ -280,23 +283,33 @@ fn column_default_value_drops_null_cast_prefixes() { // column with no real default; the builtin drops these too. The // `NULL::` prefix check stays case-sensitive in the builtin (only the // bare-null check is eq_ignore_ascii_case), so match that exactly. - assert_eq!(column_default_value(Some("NULL::text"), "NO"), None); - assert_eq!(column_default_value(Some("NULL::integer"), "NO"), None); + assert_eq!(column_default_value(Some("NULL::text"), false), None); + assert_eq!(column_default_value(Some("NULL::integer"), false), None); } #[test] fn column_default_value_surfaces_real_defaults_unchanged() { // Non-NULL defaults pass through verbatim — the value the host shows - // in the column's default cell. - assert_eq!(column_default_value(Some("0"), "NO"), Some("0".to_string())); + // in the column's default cell. (A string literal `'null'` is stored + // by PG as `'null'::text`, which doesn't match the bare-null check and + // so correctly survives as a real default.) assert_eq!( - column_default_value(Some("'neutral'"), "NO"), + column_default_value(Some("0"), false), + Some("0".to_string()) + ); + assert_eq!( + column_default_value(Some("'neutral'"), false), Some("'neutral'".to_string()) ); assert_eq!( - column_default_value(Some("now()"), "NO"), + column_default_value(Some("now()"), false), Some("now()".to_string()) ); + assert_eq!( + column_default_value(Some("'null'::text"), false), + Some("'null'::text".to_string()), + "a string *literal* 'null' is a real default (PG stores it quoted+cast) and must survive, not be dropped as the bare keyword" + ); } #[test] @@ -304,15 +317,22 @@ fn column_default_value_drops_auto_increment_defaults() { // SERIAL/IDENTITY columns carry a `nextval(...)` default or an // is_identity of YES; the host surfaces those as is_auto_increment, // not as default_value, so the raw default must not leak through. + // The caller computes is_auto_increment once (matching the builtin's + // single computation) and passes it in, so the helper just honors it. assert_eq!( - column_default_value(Some("nextval('users_id_seq'::regclass)"), "NO"), + column_default_value(Some("nextval('users_id_seq'::regclass)"), true), None, - "nextval default must be dropped (auto-increment)" + "a nextval default the caller flagged as auto-increment must be dropped" ); assert_eq!( - column_default_value(Some("42"), "YES"), + column_default_value(Some("42"), true), None, - "is_identity=YES must drop the default regardless of its value" + "an is_identity=YES column (caller sets is_auto_increment=true) drops the default regardless of its value" + ); + // Negative control: the same `42` default on a non-auto column survives. + assert_eq!( + column_default_value(Some("42"), false), + Some("42".to_string()) ); } @@ -320,6 +340,10 @@ fn column_default_value_drops_auto_increment_defaults() { fn column_default_value_drops_empty_defaults() { // An empty string is not a meaningful default — drop it rather than // surfacing an empty default_value field. - assert_eq!(column_default_value(Some(""), "NO"), None); - assert_eq!(column_default_value(None, "NO"), None, "no default at all -> None"); + assert_eq!(column_default_value(Some(""), false), None); + assert_eq!( + column_default_value(None, false), + None, + "no default at all -> None" + ); }