From 6899688fa9ac07ef60252b477759bba16b27708f Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Wed, 16 Sep 2026 16:21:52 -0400 Subject: [PATCH] fix: wrap explain_query in the Raw { engine, format, payload, ... } shape (#89) explain_query returned the raw EXPLAIN JSON value directly, with a comment acknowledging the host would fall through to ExplainQueryOutput::Plan { plan: res } instead of the Raw variant the builtin driver emits. The host's plugin adapter only classifies a response as Raw when it finds engine/format/payload as strings (via .as_str()) plus an optional original_query -- any other shape, including a bare JSON array, falls through to Plan. Runtime-registered EXPLAIN parsers select on engine+format, so this plugin's output was never selectable by them, and the parsed-plan renderer ran instead of the raw-payload path the builtin uses. Wrapped the plan JSON in the exact shape the adapter checks: { engine: "postgres", format: "postgres-json", payload: , original_query: }. payload must be the JSON *string* form, not the live JSON value -- the adapter reads it with object.get("payload")?.as_str(), which silently returns None (not an error) for a nested object/array, dropping the response into the Plan fallback exactly like the pre-fix bare-array response did. TDD: added raw_explain_output unit tests to query_tests.rs, including one that specifically checks payload is Value::String (not a nested value) to catch that exact silent-fallback failure mode. Confirmed the new tests fail to even compile against the pre-fix code (the function didn't exist yet) before the fix. Verified live against a real PostgreSQL instance by simulating the host adapter's exact matching logic (object.get(field)?.as_str() for each of engine/format/payload) against real explain_query responses: confirmed the pre-fix binary's bare-array response is NOT classified as Raw (falls through to Plan, reproducing the bug), and the post-fix binary's response IS classified as Raw, with engine/format/ original_query correct and payload containing a real EXPLAIN plan (both without and with analyze=true, confirming actual execution stats like "Actual Rows" come through correctly in the ANALYZE case). --- src/handlers/query.rs | 35 +++++++++++++++++------- src/handlers/query_tests.rs | 54 ++++++++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 10 deletions(-) diff --git a/src/handlers/query.rs b/src/handlers/query.rs index 2634b05..f459e3f 100644 --- a/src/handlers/query.rs +++ b/src/handlers/query.rs @@ -111,21 +111,38 @@ pub async fn explain_query(id: Value, params: &Value) -> Value { match exec_query(&conn_params, &explain_sql, None, 1, schema).await { Ok(result) => { - // The host wraps this in ExplainQueryOutput::Plan { plan: res } - // We just return the raw explain JSON from the first row/col - if let Some(rows) = result.get("rows").and_then(Value::as_array) { - if let Some(first_row) = rows.first().and_then(Value::as_array) { - if let Some(plan_json) = first_row.first() { - return ok_response(id, plan_json.clone()); - } - } + let plan_json = result + .get("rows") + .and_then(Value::as_array) + .and_then(|rows| rows.first()) + .and_then(Value::as_array) + .and_then(|first_row| first_row.first()); + match plan_json { + Some(plan_json) => ok_response(id, raw_explain_output(plan_json, query)), + None => ok_response(id, result), } - ok_response(id, result) } Err(e) => error_response(id, -32603, &e), } } +/// Wrap an EXPLAIN plan value in the `Raw { engine, format, payload, +/// original_query }` shape the host's plugin adapter recognizes +/// (`tabularis` `plugins/driver.rs::explain_query`, which reads +/// `engine`/`format`/`payload` as strings via `.as_str()` before +/// classifying the result as `ExplainQueryOutput::Raw`; any other shape +/// falls through to the parsed-plan path instead). `payload` must be the +/// JSON **stringified**, not the live JSON value itself, to match the +/// builtin driver's `RawExplainOutput` contract exactly. +fn raw_explain_output(plan_json: &Value, original_query: &str) -> Value { + json!({ + "engine": "postgres", + "format": "postgres-json", + "payload": plan_json.to_string(), + "original_query": original_query, + }) +} + /// Execute a SQL query and return a QueryResult-shaped JSON value. async fn exec_query( conn_params: &ConnectionParams, diff --git a/src/handlers/query_tests.rs b/src/handlers/query_tests.rs index ac11b69..79cb3d5 100644 --- a/src/handlers/query_tests.rs +++ b/src/handlers/query_tests.rs @@ -10,7 +10,10 @@ //! These tests exercise the pure classification logic that decides whether //! pagination is applied to a statement. -use super::{returns_result_set, strip_leading_sql_comments, supports_trailing_limit_clause}; +use super::{ + raw_explain_output, returns_result_set, strip_leading_sql_comments, + supports_trailing_limit_clause, +}; #[test] fn strip_leading_sql_comments_skips_line_comments() { @@ -138,3 +141,52 @@ fn supports_trailing_limit_clause_does_not_silently_disable_cte_pagination() { "WITH t AS (SELECT 1) SELECT * FROM t" )); } + +#[test] +fn raw_explain_output_matches_the_host_adapters_raw_shape() { + // #89: the host's plugin adapter (tabularis plugins/driver.rs) only + // classifies a response as ExplainQueryOutput::Raw when it finds + // engine/format/payload as strings via .as_str() — anything else + // (including the bare EXPLAIN JSON this plugin used to return) falls + // through to the parsed-plan path instead. + let plan = serde_json::json!([{"Plan": {"Node Type": "Seq Scan"}}]); + let wire = raw_explain_output(&plan, "SELECT 1"); + + let obj = wire.as_object().expect("must be a JSON object"); + assert_eq!( + obj.get("engine").and_then(serde_json::Value::as_str), + Some("postgres") + ); + assert_eq!( + obj.get("format").and_then(serde_json::Value::as_str), + Some("postgres-json") + ); + assert_eq!( + obj.get("original_query") + .and_then(serde_json::Value::as_str), + Some("SELECT 1") + ); + + // payload must be the JSON *stringified*, not the live JSON value — the + // host adapter reads it with object.get("payload")?.as_str(), which + // returns None (not an error) for a JSON object/array, silently + // dropping this plugin's output into the Plan fallback path instead. + let payload = obj + .get("payload") + .and_then(serde_json::Value::as_str) + .expect("payload must be a JSON string, not a nested object/array"); + let reparsed: serde_json::Value = + serde_json::from_str(payload).expect("payload must be valid JSON once parsed"); + assert_eq!(reparsed, plan); +} + +#[test] +fn raw_explain_output_payload_is_not_the_live_json_value() { + let plan = serde_json::json!({"Node Type": "Index Scan"}); + let wire = raw_explain_output(&plan, "SELECT * FROM t WHERE id = 1"); + let payload_value = wire.get("payload").unwrap(); + assert!( + payload_value.is_string(), + "payload must be Value::String, got {payload_value:?}" + ); +}