From 487b89a4b6c67aacd7c15b0814a450801333153c Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Mon, 31 Jul 2023 14:52:55 -0700 Subject: [PATCH] fix: ignore extra fields in Postgres describe parsing (#2670) * fix(postgres): sqlx prepare fails if shared_preload_libraries=pg_stat_statements closes #2622 refs: * https://serde.rs/enum-representations.html#untagged * https://serde.rs/field-attrs.html#skip * https://www.postgresql.org/docs/current/pgstatstatements.html * https://www.postgresql.org/docs/current/runtime-config-statistics.html#GUC-COMPUTE-QUERY-ID * fix(postgres): regression of #1449 ``` error: error occurred while decoding column 0: data did not match any variant of untagged enum Explain at line 3 column 1 Error: --> tests/postgres/macros.rs:103:15 | 103 | let row = sqlx::query!(r#"CALL forty_two(null)"#) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `$crate::sqlx_macros::expand_query` which comes from the expansion of the macro `sqlx::query` (in Nightly builds, run with -Z macro-backtrace for more info) error: could not compile `sqlx` (test "postgres-macros") due to previous error ``` * refactor(postgres): don't define unused fields in QueryPlan * refactor(postgres): simplify query plan handling, add unit test * chore: document why we load `pg_stat_statements` in tests --------- Co-authored-by: mrl5 <31549762+mrl5@users.noreply.github.com> --- sqlx-postgres/src/connection/describe.rs | 101 +++++++++++++++++++---- tests/docker-compose.yml | 4 +- 2 files changed, 89 insertions(+), 16 deletions(-) diff --git a/sqlx-postgres/src/connection/describe.rs b/sqlx-postgres/src/connection/describe.rs index bcfbf3a9cf..1cd28a8b54 100644 --- a/sqlx-postgres/src/connection/describe.rs +++ b/sqlx-postgres/src/connection/describe.rs @@ -451,15 +451,16 @@ WHERE rngtypid = $1 let mut nullables = Vec::new(); - if let Explain::Plan( - plan @ Plan { - output: Some(outputs), - .. - }, - ) = &explain + if let Explain::Plan { + plan: + plan @ Plan { + output: Some(ref outputs), + .. + }, + } = &explain { nullables.resize(outputs.len(), None); - visit_plan(&plan, outputs, &mut nullables); + visit_plan(plan, outputs, &mut nullables); } Ok(nullables) @@ -491,17 +492,30 @@ fn visit_plan(plan: &Plan, outputs: &[String], nullables: &mut Vec> } } -#[derive(serde::Deserialize)] +#[derive(serde::Deserialize, Debug)] +#[serde(untagged)] enum Explain { - /// {"Plan": ...} -- returned for most statements - Plan(Plan), - /// The string "Utility Statement" -- returned for - /// a CALL statement - #[serde(rename = "Utility Statement")] - UtilityStatement, + // NOTE: the returned JSON may not contain a `plan` field, for example, with `CALL` statements: + // https://github.com/launchbadge/sqlx/issues/1449 + // + // In this case, we should just fall back to assuming all is nullable. + // + // It may also contain additional fields we don't care about, which should not break parsing: + // https://github.com/launchbadge/sqlx/issues/2587 + // https://github.com/launchbadge/sqlx/issues/2622 + Plan { + #[serde(rename = "Plan")] + plan: Plan, + }, + + // This ensures that parsing never technically fails. + // + // We don't want to specifically expect `"Utility Statement"` because there might be other cases + // and we don't care unless it contains a query plan anyway. + Other(serde::de::IgnoredAny), } -#[derive(serde::Deserialize)] +#[derive(serde::Deserialize, Debug)] struct Plan { #[serde(rename = "Join Type")] join_type: Option, @@ -512,3 +526,60 @@ struct Plan { #[serde(rename = "Plans")] plans: Option>, } + +#[test] +fn explain_parsing() { + let normal_plan = r#"[ + { + "Plan": { + "Node Type": "Result", + "Parallel Aware": false, + "Async Capable": false, + "Startup Cost": 0.00, + "Total Cost": 0.01, + "Plan Rows": 1, + "Plan Width": 4, + "Output": ["1"] + } + } +]"#; + + // https://github.com/launchbadge/sqlx/issues/2622 + let extra_field = r#"[ + { + "Plan": { + "Node Type": "Result", + "Parallel Aware": false, + "Async Capable": false, + "Startup Cost": 0.00, + "Total Cost": 0.01, + "Plan Rows": 1, + "Plan Width": 4, + "Output": ["1"] + }, + "Query Identifier": 1147616880456321454 + } +]"#; + + // https://github.com/launchbadge/sqlx/issues/1449 + let utility_statement = r#"["Utility Statement"]"#; + + let normal_plan_parsed = serde_json::from_str::<[Explain; 1]>(normal_plan).unwrap(); + let extra_field_parsed = serde_json::from_str::<[Explain; 1]>(extra_field).unwrap(); + let utility_statement_parsed = serde_json::from_str::<[Explain; 1]>(utility_statement).unwrap(); + + assert!( + matches!(normal_plan_parsed, [Explain::Plan { plan: Plan { .. } }]), + "unexpected parse from {normal_plan:?}: {normal_plan_parsed:?}" + ); + + assert!( + matches!(extra_field_parsed, [Explain::Plan { plan: Plan { .. } }]), + "unexpected parse from {extra_field:?}: {extra_field_parsed:?}" + ); + + assert!( + matches!(utility_statement_parsed, [Explain::Other(_)]), + "unexpected parse from {utility_statement:?}: {utility_statement_parsed:?}" + ) +} diff --git a/tests/docker-compose.yml b/tests/docker-compose.yml index f521efb3a4..4c59dc5034 100644 --- a/tests/docker-compose.yml +++ b/tests/docker-compose.yml @@ -182,8 +182,10 @@ services: POSTGRES_INITDB_ARGS: --auth-host=scram-sha-256 volumes: - "./postgres/setup.sql:/docker-entrypoint-initdb.d/setup.sql" + # Loading `pg_stat_statements` should serve as a regression test for: + # https://github.com/launchbadge/sqlx/issues/2622 command: > - -c ssl=on -c ssl_cert_file=/var/lib/postgresql/server.crt -c ssl_key_file=/var/lib/postgresql/server.key + -c ssl=on -c ssl_cert_file=/var/lib/postgresql/server.crt -c ssl_key_file=/var/lib/postgresql/server.key -c shared_preload_libraries=pg_stat_statements postgres_15_client_ssl: build: