Skip to content

Commit d7b6491

Browse files
sdairsclaude
andcommitted
Require both pgConfig and pgBouncerConfig in PATCH/POST bodies
The behaviour matrix captured in #163 shows the live API rejects any postgresInstanceConfig PATCH or POST body that omits either nested object: omitting pgBouncerConfig yields `BAD_REQUEST: request body.pgBouncerConfig: 'undefined'`, omitting pgConfig yields the symmetric error. Sending `{}` for either is accepted (matrix body 2 → 200). The previous commit had flipped both to Option<T> with skip_serializing_if so partial PATCHes could send only the changed half, but that shape doesn't match the API. Flip pg_config and pg_bouncer_config on PostgresInstanceConfig back to required (bare T with #[serde(default)] so the default envelope still serialises to `{ "pgConfig": {}, "pgBouncerConfig": {} }`), drop the two corresponding OPTIONALITY_EXEMPTIONS entries, and update the fixture/integration tests to construct the new shape. Inner pgConfig fields stay Option<T> with skip_serializing_if — those remain opt-in per the spec's all-optional partial-update semantics. Response-side deserialisation is still broken (the API returns strictly-numeric pgConfig fields wrapped in JSON strings), which is tracked separately and blocks the integration round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 61bc8d9 commit d7b6491

5 files changed

Lines changed: 46 additions & 48 deletions

File tree

crates/clickhouse-cloud-api/src/models.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11968,10 +11968,10 @@ pub struct PgConfig {
1196811968
/// `postgresInstanceConfig` from the ClickHouse Cloud API.
1196911969
#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)]
1197011970
pub struct PostgresInstanceConfig {
11971-
#[serde(rename = "pgBouncerConfig", skip_serializing_if = "Option::is_none", default)]
11972-
pub pg_bouncer_config: Option<PgBouncerConfig>,
11973-
#[serde(rename = "pgConfig", skip_serializing_if = "Option::is_none", default)]
11974-
pub pg_config: Option<PgConfig>,
11971+
#[serde(rename = "pgBouncerConfig", default)]
11972+
pub pg_bouncer_config: PgBouncerConfig,
11973+
#[serde(rename = "pgConfig", default)]
11974+
pub pg_config: PgConfig,
1197511975
}
1197611976

1197711977
/// `postgresInstanceUpdateConfigResponse` from the ClickHouse Cloud API.

crates/clickhouse-cloud-api/tests/client_test.rs

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2168,10 +2168,7 @@ async fn get_postgres_config() {
21682168
.await
21692169
.unwrap();
21702170
let config = resp.result.unwrap();
2171-
assert_eq!(
2172-
config.pg_config.and_then(|c| c.max_connections),
2173-
Some(100)
2174-
);
2171+
assert_eq!(config.pg_config.max_connections, Some(100));
21752172
}
21762173

21772174
#[tokio::test]
@@ -2192,11 +2189,11 @@ async fn replace_postgres_config() {
21922189
.await;
21932190

21942191
let body = PostgresInstanceConfig {
2195-
pg_config: Some(PgConfig {
2192+
pg_config: PgConfig {
21962193
max_connections: Some(200),
21972194
..Default::default()
2198-
}),
2199-
pg_bouncer_config: None,
2195+
},
2196+
pg_bouncer_config: PgBouncerConfig::default(),
22002197
};
22012198
let resp = c
22022199
.postgres_instance_config_post("org-1", "pg-1", &body)
@@ -2225,11 +2222,11 @@ async fn patch_postgres_config() {
22252222
.await;
22262223

22272224
let body = PostgresInstanceConfig {
2228-
pg_config: Some(PgConfig {
2225+
pg_config: PgConfig {
22292226
max_connections: Some(150),
22302227
..Default::default()
2231-
}),
2232-
pg_bouncer_config: None,
2228+
},
2229+
pg_bouncer_config: PgBouncerConfig::default(),
22332230
};
22342231
let resp = c
22352232
.postgres_instance_config_patch("org-1", "pg-1", &body)

crates/clickhouse-cloud-api/tests/integration_postgres_test.rs

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -236,11 +236,7 @@ async fn cloud_postgres_crud_lifecycle() -> TestResult<()> {
236236
// Round-trip a single pgConfig field: PATCH max_connections to a
237237
// new value, poll-until GET reflects it, then PATCH back.
238238
if let Some(baseline) = baseline {
239-
let baseline_max = baseline
240-
.pg_config
241-
.as_ref()
242-
.and_then(|c| c.max_connections)
243-
.unwrap_or(100);
239+
let baseline_max = baseline.pg_config.max_connections.unwrap_or(100);
244240
let target = baseline_max + 7;
245241

246242
failures
@@ -254,11 +250,11 @@ async fn cloud_postgres_crud_lifecycle() -> TestResult<()> {
254250
let postgres_id = postgres_id.clone();
255251
async move {
256252
let body = PostgresInstanceConfig {
257-
pg_config: Some(PgConfig {
253+
pg_config: PgConfig {
258254
max_connections: Some(target),
259255
..Default::default()
260-
}),
261-
pg_bouncer_config: None,
256+
},
257+
pg_bouncer_config: PgBouncerConfig::default(),
262258
};
263259
client
264260
.postgres_instance_config_patch(&org_id, &postgres_id, &body)
@@ -295,8 +291,7 @@ async fn cloud_postgres_crud_lifecycle() -> TestResult<()> {
295291
.await?;
296292
let observed = resp
297293
.result
298-
.and_then(|r| r.pg_config)
299-
.and_then(|c| c.max_connections);
294+
.and_then(|r| r.pg_config.max_connections);
300295
if observed == Some(target) {
301296
Ok(Some(()))
302297
} else {
@@ -322,11 +317,11 @@ async fn cloud_postgres_crud_lifecycle() -> TestResult<()> {
322317
let postgres_id = postgres_id.clone();
323318
async move {
324319
let body = PostgresInstanceConfig {
325-
pg_config: Some(PgConfig {
320+
pg_config: PgConfig {
326321
max_connections: Some(baseline_max),
327322
..Default::default()
328-
}),
329-
pg_bouncer_config: None,
323+
},
324+
pg_bouncer_config: PgBouncerConfig::default(),
330325
};
331326
client
332327
.postgres_instance_config_patch(&org_id, &postgres_id, &body)

crates/clickhouse-cloud-api/tests/models_test.rs

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -763,33 +763,36 @@ fn serialize_servic_private_endpointe_post_request() {
763763
#[test]
764764
fn serialize_postgres_instance_config() {
765765
let config = PostgresInstanceConfig {
766-
pg_config: Some(PgConfig {
766+
pg_config: PgConfig {
767767
max_connections: Some(200),
768768
..Default::default()
769-
}),
770-
pg_bouncer_config: Some(PgBouncerConfig {}),
769+
},
770+
pg_bouncer_config: PgBouncerConfig::default(),
771771
};
772772
let json = serde_json::to_value(&config).unwrap();
773773
assert_eq!(json["pgConfig"]["max_connections"], 200);
774774
assert!(json.get("pgBouncerConfig").is_some());
775775
}
776776

777777
#[test]
778-
fn serialize_postgres_instance_config_omits_none_nested() {
779-
// The fix for #163: partial PATCH must omit pgBouncerConfig when None,
780-
// and pgConfig must omit fields the caller didn't set.
778+
fn serialize_postgres_instance_config_always_includes_both_nested() {
779+
// The live API rejects PATCH/POST bodies that omit either `pgConfig` or
780+
// `pgBouncerConfig` with `BAD_REQUEST: ... 'undefined'`, so the envelope
781+
// always serializes both — defaulting to `{}` — while inner pgConfig
782+
// fields stay opt-in. See #163 for the matrix evidence.
781783
let config = PostgresInstanceConfig {
782-
pg_config: Some(PgConfig {
784+
pg_config: PgConfig {
783785
max_connections: Some(200),
784786
..Default::default()
785-
}),
786-
pg_bouncer_config: None,
787+
},
788+
pg_bouncer_config: PgBouncerConfig::default(),
787789
};
788790
let json = serde_json::to_value(&config).unwrap();
789791
assert!(
790-
json.get("pgBouncerConfig").is_none(),
791-
"pgBouncerConfig must be omitted when None"
792+
json.get("pgBouncerConfig").is_some(),
793+
"pgBouncerConfig must always be present"
792794
);
795+
assert_eq!(json["pgBouncerConfig"], serde_json::json!({}));
793796
assert_eq!(json["pgConfig"]["max_connections"], 200);
794797
let pg = json["pgConfig"].as_object().unwrap();
795798
assert_eq!(
@@ -799,6 +802,13 @@ fn serialize_postgres_instance_config_omits_none_nested() {
799802
);
800803
}
801804

805+
#[test]
806+
fn serialize_postgres_instance_config_default_envelope() {
807+
// Default envelope serializes to the minimal accepted body shape.
808+
let json = serde_json::to_value(PostgresInstanceConfig::default()).unwrap();
809+
assert_eq!(json, serde_json::json!({ "pgConfig": {}, "pgBouncerConfig": {} }));
810+
}
811+
802812
// ===========================================================================
803813
// Forward compatibility: extra unknown fields ignored
804814
// ===========================================================================
@@ -1071,10 +1081,7 @@ fn deserialize_postgres_instance_config() {
10711081
"pgBouncerConfig": {}
10721082
}"#;
10731083
let config: PostgresInstanceConfig = serde_json::from_str(json).unwrap();
1074-
assert_eq!(
1075-
config.pg_config.and_then(|c| c.max_connections),
1076-
Some(200)
1077-
);
1084+
assert_eq!(config.pg_config.max_connections, Some(200));
10781085
}
10791086

10801087
#[test]

crates/clickhouse-cloud-api/tests/spec_coverage_test.rs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -390,10 +390,11 @@ const OPTIONALITY_EXEMPTIONS: &[(&str, &str)] = &[
390390
// the endpoint is partial-update: sending the default value for any
391391
// field yields `Validation failed for following fields: pg_config.*`.
392392
// Every property is effectively optional. The wrapping
393-
// `postgresInstanceConfig` also lists both nested objects as required,
394-
// but PATCH must accept either alone. Exempt all until the upstream
395-
// spec adds proper required semantics. See #163 for the behaviour
396-
// matrix evidence and the upstream report draft.
393+
// `postgresInstanceConfig` is *not* exempted — the live API requires
394+
// both `pgConfig` and `pgBouncerConfig` to be present in PATCH and
395+
// POST bodies (omitting either yields `BAD_REQUEST: ... 'undefined'`),
396+
// matching the spec's `required` listing. See #163 for the behaviour
397+
// matrix evidence.
397398
("PgConfig", "default_transaction_isolation"),
398399
("PgConfig", "effective_cache_size"),
399400
("PgConfig", "effective_io_concurrency"),
@@ -417,8 +418,6 @@ const OPTIONALITY_EXEMPTIONS: &[(&str, &str)] = &[
417418
("PgConfig", "wal_keep_size"),
418419
("PgConfig", "wal_sender_timeout"),
419420
("PgConfig", "work_mem"),
420-
("PostgresInstanceConfig", "pgConfig"),
421-
("PostgresInstanceConfig", "pgBouncerConfig"),
422421
];
423422

424423
fn assert_field_optionality(spec: &Value) {

0 commit comments

Comments
 (0)