diff --git a/README.md b/README.md index 88ad71e..3ce1416 100644 --- a/README.md +++ b/README.md @@ -472,10 +472,10 @@ clickhousectl cloud service reset-password \ --new-password-hash \ --new-double-sha1-hash -# Query endpoint management (manual — for custom roles or sharing keys with other tools) +# Query endpoint management (manual, for sharing keys with other tools) clickhousectl cloud service query-endpoint get clickhousectl cloud service query-endpoint create \ - --role admin \ + --role sql_console_admin \ --open-api-key key-1 \ --allowed-origins https://app.example.com clickhousectl cloud service query-endpoint delete diff --git a/crates/clickhouse-cloud-api/src/client/postgres.rs b/crates/clickhouse-cloud-api/src/client/postgres.rs index 3326e97..0c27a4a 100644 --- a/crates/clickhouse-cloud-api/src/client/postgres.rs +++ b/crates/clickhouse-cloud-api/src/client/postgres.rs @@ -453,8 +453,8 @@ impl Client { db_user: Option<&str>, db_operation: Option<&str>, app: Option<&str>, - sort_by: Option<&str>, - sort_order: Option<&str>, + sort_by: Option<&SlowQueryPatternsGetListSortby>, + sort_order: Option<&SlowQueryPatternsGetListSortorder>, limit: Option, offset: Option, ) -> Result>, Error> { diff --git a/crates/clickhouse-cloud-api/src/models.rs b/crates/clickhouse-cloud-api/src/models.rs index 5458fb3..9169ca3 100644 --- a/crates/clickhouse-cloud-api/src/models.rs +++ b/crates/clickhouse-cloud-api/src/models.rs @@ -164,8 +164,8 @@ pub use backups::{ GcpBackupBucketPropertiesBucketprovider, }; pub use byoc::{ - ByocConfig, ByocConfigCloudprovider, ByocConfigRegionid, ByocConfigState, - ByocInfrastructurePatchRequest, ByocInfrastructurePostRequest, + ByocAvailabilityZoneSuffix, ByocConfig, ByocConfigCloudprovider, ByocConfigRegionid, + ByocConfigState, ByocInfrastructurePatchRequest, ByocInfrastructurePostRequest, ByocInfrastructurePostRequestRegionid, }; pub use clickpipes::*; @@ -197,7 +197,8 @@ pub use postgres::{ PostgresServicePasswordResource, PostgresServicePatchRequest, PostgresServicePostRequest, PostgresServiceReadReplicaRequest, PostgresServiceRestoreRequest, PostgresServiceSetPassword, PostgresServiceSetState, PostgresServiceSetStateCommand, PostgresSlowQueryPattern, - PostgresSlowQueryPatternDetail, + PostgresSlowQueryPatternDetail, SlowQueryPatternsGetListSortby, + SlowQueryPatternsGetListSortorder, }; pub use quotas::{OrganizationQuota, OrganizationQuotaQuotacode, OrganizationQuotaScope}; pub use rbac::{ @@ -221,25 +222,26 @@ pub use services::{ AutoscalingMode, CurrentScaling, CurrentScalingEffectiveautoscalingmode, InstancePrivateEndpoint, InstancePrivateEndpointCloudprovider, InstancePrivateEndpointRegion, InstancePrivateEndpointsPatch, InstanceServiceQueryApiEndpointsPostRequest, InstanceTagsPatch, - PrivateEndpointConfig, ScalingSchedule, ScalingScheduleBaseConfig, ScalingScheduleEntry, - ScalingScheduleEntryRequest, ScalingSchedulePostRequest, ServicPrivateEndpointePostRequest, - Service, ServiceClickhouseSetting, ServiceClickhouseSettingSchemaEntry, - ServiceClickhouseSettingWarning, ServiceClickhouseSettingsList, - ServiceClickhouseSettingsPatchRequest, ServiceClickhouseSettingsPatchResponse, - ServiceClickhouseSettingsSchema, ServiceCompliancetype, ServiceEndpoint, ServiceEndpointChange, - ServiceEndpointChangeProtocol, ServiceEndpointProtocol, ServicePasswordPatchRequest, - ServicePasswordPatchResponse, ServicePatchRequest, ServicePatchRequestReleasechannel, - ServicePostRequest, ServicePostRequestCompliancetype, ServicePostRequestProfile, - ServicePostRequestProvider, ServicePostRequestRegion, ServicePostRequestReleasechannel, - ServicePostRequestTier, ServicePostResponse, ServiceProfile, ServiceProvider, - ServiceQueryAPIEndpoint, ServiceRegion, ServiceReleasechannel, + PrivateEndpointConfig, QueryEndpointRole, ScalingSchedule, ScalingScheduleBaseConfig, + ScalingScheduleEntry, ScalingScheduleEntryRequest, ScalingSchedulePostRequest, + ServicPrivateEndpointePostRequest, Service, ServiceClickhouseSetting, + ServiceClickhouseSettingSchemaEntry, ServiceClickhouseSettingWarning, + ServiceClickhouseSettingsList, ServiceClickhouseSettingsPatchRequest, + ServiceClickhouseSettingsPatchResponse, ServiceClickhouseSettingsSchema, ServiceCompliancetype, + ServiceEndpoint, ServiceEndpointChange, ServiceEndpointChangeProtocol, ServiceEndpointProtocol, + ServicePasswordPatchRequest, ServicePasswordPatchResponse, ServicePatchRequest, + ServicePatchRequestReleasechannel, ServicePostRequest, ServicePostRequestCompliancetype, + ServicePostRequestProfile, ServicePostRequestProvider, ServicePostRequestRegion, + ServicePostRequestReleasechannel, ServicePostRequestTier, ServicePostResponse, ServiceProfile, + ServiceProvider, ServiceQueryAPIEndpoint, ServiceRegion, ServiceReleasechannel, ServiceReplicaScalingPatchRequest, ServiceScalingPatchRequest, ServiceScalingPatchResponse, ServiceScalingPatchResponseCompliancetype, ServiceScalingPatchResponseProfile, ServiceScalingPatchResponseProvider, ServiceScalingPatchResponseRegion, ServiceScalingPatchResponseReleasechannel, ServiceScalingPatchResponseState, ServiceScalingPatchResponseTier, ServiceState, ServiceStatePatchRequest, - ServiceStatePatchRequestCommand, ServiceTier, UpgradeWindow, UpgradeWindowPutRequest, - UsageCost, UsageCostMetrics, UsageCostRecord, UsageCostRecordEntitytype, + ServiceStatePatchRequestCommand, ServiceTier, UpgradeWindow, UpgradeWindowDuration, + UpgradeWindowPutRequest, UpgradeWindowStartHourUtc, UsageCost, UsageCostMetrics, + UsageCostRecord, UsageCostRecordEntitytype, }; pub use shared::{ ApiResponse, AssignedRole, AssignedRoleRoletype, IpAccessListEntry, IpAccessListEntryResponse, diff --git a/crates/clickhouse-cloud-api/src/models/byoc.rs b/crates/clickhouse-cloud-api/src/models/byoc.rs index 7f3f479..36c7699 100644 --- a/crates/clickhouse-cloud-api/src/models/byoc.rs +++ b/crates/clickhouse-cloud-api/src/models/byoc.rs @@ -229,6 +229,41 @@ impl std::fmt::Display for ByocInfrastructurePostRequestRegionid { } } +/// Allowed availability-zone suffixes for BYOC infrastructure. +#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] +pub enum ByocAvailabilityZoneSuffix { + #[serde(rename = "a")] + #[default] + A, + #[serde(rename = "b")] + B, + #[serde(rename = "c")] + C, + #[serde(rename = "d")] + D, + #[serde(rename = "e")] + E, + #[serde(rename = "f")] + F, + /// Catch-all for unknown or newly-added values. + #[serde(untagged)] + Unknown(String), +} + +impl std::fmt::Display for ByocAvailabilityZoneSuffix { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::A => write!(f, "a"), + Self::B => write!(f, "b"), + Self::C => write!(f, "c"), + Self::D => write!(f, "d"), + Self::E => write!(f, "e"), + Self::F => write!(f, "f"), + Self::Unknown(s) => write!(f, "{s}"), + } + } +} + /// `ByocConfig` from the ClickHouse Cloud API. #[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] pub struct ByocConfig { @@ -259,7 +294,7 @@ pub struct ByocInfrastructurePostRequest { #[serde(rename = "accountId")] pub account_id: String, #[serde(rename = "availabilityZoneSuffixes")] - pub availability_zone_suffixes: Vec, + pub availability_zone_suffixes: Vec, #[serde(rename = "displayName")] pub display_name: String, #[serde(rename = "regionId")] diff --git a/crates/clickhouse-cloud-api/src/models/postgres.rs b/crates/clickhouse-cloud-api/src/models/postgres.rs index 2443655..93c9c4b 100644 --- a/crates/clickhouse-cloud-api/src/models/postgres.rs +++ b/crates/clickhouse-cloud-api/src/models/postgres.rs @@ -80,6 +80,85 @@ impl std::fmt::Display for PostgresLogsGetListSortorder { } } +/// Inline enum for `slowQueryPatternsGetList.sort_by`. +#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] +pub enum SlowQueryPatternsGetListSortby { + #[serde(rename = "total_duration")] + #[default] + Total_duration, + #[serde(rename = "avg_duration")] + Avg_duration, + #[serde(rename = "call_count")] + Call_count, + #[serde(rename = "total_blks_read")] + Total_blks_read, + #[serde(rename = "total_cpu_time")] + Total_cpu_time, + #[serde(rename = "error_count")] + Error_count, + #[serde(rename = "max_duration")] + Max_duration, + #[serde(rename = "p50_duration")] + P50_duration, + #[serde(rename = "p95_duration")] + P95_duration, + #[serde(rename = "p99_duration")] + P99_duration, + #[serde(rename = "total_rows")] + Total_rows, + #[serde(rename = "total_shared_blks_hit")] + Total_shared_blks_hit, + #[serde(rename = "total_wal_bytes")] + Total_wal_bytes, + /// Catch-all for unknown or newly-added values. + #[serde(untagged)] + Unknown(String), +} + +impl std::fmt::Display for SlowQueryPatternsGetListSortby { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Total_duration => write!(f, "total_duration"), + Self::Avg_duration => write!(f, "avg_duration"), + Self::Call_count => write!(f, "call_count"), + Self::Total_blks_read => write!(f, "total_blks_read"), + Self::Total_cpu_time => write!(f, "total_cpu_time"), + Self::Error_count => write!(f, "error_count"), + Self::Max_duration => write!(f, "max_duration"), + Self::P50_duration => write!(f, "p50_duration"), + Self::P95_duration => write!(f, "p95_duration"), + Self::P99_duration => write!(f, "p99_duration"), + Self::Total_rows => write!(f, "total_rows"), + Self::Total_shared_blks_hit => write!(f, "total_shared_blks_hit"), + Self::Total_wal_bytes => write!(f, "total_wal_bytes"), + Self::Unknown(s) => write!(f, "{s}"), + } + } +} + +/// Inline enum for `slowQueryPatternsGetList.sort_order`. +#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] +pub enum SlowQueryPatternsGetListSortorder { + #[serde(rename = "asc")] + Asc, + #[serde(rename = "desc")] + #[default] + Desc, + /// Catch-all for unknown or newly-added values. + #[serde(untagged)] + Unknown(String), +} + +impl std::fmt::Display for SlowQueryPatternsGetListSortorder { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Asc => write!(f, "asc"), + Self::Desc => write!(f, "desc"), + Self::Unknown(s) => write!(f, "{s}"), + } + } +} + /// `pgSize` enum from the ClickHouse Cloud API. #[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] pub enum PgSize { diff --git a/crates/clickhouse-cloud-api/src/models/services.rs b/crates/clickhouse-cloud-api/src/models/services.rs index e433736..1f749a9 100644 --- a/crates/clickhouse-cloud-api/src/models/services.rs +++ b/crates/clickhouse-cloud-api/src/models/services.rs @@ -36,6 +36,115 @@ impl AutoscalingMode { pub const VALUES: &'static [&'static str] = &["vertical", "horizontal"]; } +/// Role granted to a query API endpoint. +#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] +pub enum QueryEndpointRole { + #[serde(rename = "sql_console_read_only")] + #[default] + SqlConsoleReadOnly, + #[serde(rename = "sql_console_admin")] + SqlConsoleAdmin, + /// Catch-all for unknown or newly-added values. + #[serde(untagged)] + Unknown(String), +} + +impl std::fmt::Display for QueryEndpointRole { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::SqlConsoleReadOnly => write!(f, "sql_console_read_only"), + Self::SqlConsoleAdmin => write!(f, "sql_console_admin"), + Self::Unknown(s) => write!(f, "{s}"), + } + } +} + +impl QueryEndpointRole { + /// Wire values accepted by the API, excluding the catch-all. + pub const VALUES: &'static [&'static str] = &["sql_console_read_only", "sql_console_admin"]; +} + +/// Allowed UTC start hours for a ClickHouse upgrade window. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] +#[serde(from = "i64", into = "i64")] +#[repr(i64)] +pub enum UpgradeWindowStartHourUtc { + #[default] + Hour0 = 0, + Hour6 = 6, + Hour12 = 12, + Hour18 = 18, + /// Catch-all for unknown or newly-added values. + #[serde(untagged)] + Unknown(i64), +} + +impl From for UpgradeWindowStartHourUtc { + fn from(value: i64) -> Self { + match value { + 0 => Self::Hour0, + 6 => Self::Hour6, + 12 => Self::Hour12, + 18 => Self::Hour18, + value => Self::Unknown(value), + } + } +} + +impl From for i64 { + fn from(value: UpgradeWindowStartHourUtc) -> Self { + match value { + UpgradeWindowStartHourUtc::Hour0 => 0, + UpgradeWindowStartHourUtc::Hour6 => 6, + UpgradeWindowStartHourUtc::Hour12 => 12, + UpgradeWindowStartHourUtc::Hour18 => 18, + UpgradeWindowStartHourUtc::Unknown(value) => value, + } + } +} + +impl std::fmt::Display for UpgradeWindowStartHourUtc { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", i64::from(*self)) + } +} + +/// Duration of a ClickHouse upgrade window, in hours. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] +#[serde(from = "i64", into = "i64")] +#[repr(i64)] +pub enum UpgradeWindowDuration { + #[default] + SixHours = 6, + /// Catch-all for unknown or newly-added values. + #[serde(untagged)] + Unknown(i64), +} + +impl From for UpgradeWindowDuration { + fn from(value: i64) -> Self { + match value { + 6 => Self::SixHours, + value => Self::Unknown(value), + } + } +} + +impl From for i64 { + fn from(value: UpgradeWindowDuration) -> Self { + match value { + UpgradeWindowDuration::SixHours => 6, + UpgradeWindowDuration::Unknown(value) => value, + } + } +} + +impl std::fmt::Display for UpgradeWindowDuration { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", i64::from(*self)) + } +} + /// Inline enum for `CurrentScaling.effectiveAutoscalingMode`. #[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] pub enum CurrentScalingEffectiveautoscalingmode { @@ -1330,7 +1439,7 @@ pub struct InstanceServiceQueryApiEndpointsPostRequest { pub allowed_origins: String, #[serde(rename = "openApiKeys")] pub open_api_keys: Vec, - pub roles: Vec, + pub roles: Vec, } /// `InstanceTagsPatch` from the ClickHouse Cloud API. @@ -1775,7 +1884,7 @@ pub struct ServiceQueryAPIEndpoint { #[serde(rename = "openApiKeys", skip_serializing_if = "Option::is_none")] pub open_api_keys: Option>, #[serde(skip_serializing_if = "Option::is_none")] - pub roles: Option>, + pub roles: Option>, } /// `ServiceReplicaScalingPatchRequest` from the ClickHouse Cloud API. @@ -1927,9 +2036,9 @@ pub struct ServiceStatePatchRequest { #[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] pub struct UpgradeWindow { #[serde(skip_serializing_if = "Option::is_none")] - pub duration: Option, + pub duration: Option, #[serde(rename = "startHourUtc", skip_serializing_if = "Option::is_none")] - pub start_hour_utc: Option, + pub start_hour_utc: Option, #[serde(skip_serializing_if = "Option::is_none")] pub weekday: Option, } @@ -1938,7 +2047,7 @@ pub struct UpgradeWindow { #[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] pub struct UpgradeWindowPutRequest { #[serde(rename = "startHourUtc")] - pub start_hour_utc: i64, + pub start_hour_utc: UpgradeWindowStartHourUtc, pub weekday: i64, } diff --git a/crates/clickhouse-cloud-api/tests/client_test.rs b/crates/clickhouse-cloud-api/tests/client_test.rs index 7ac81dd..9422bc5 100644 --- a/crates/clickhouse-cloud-api/tests/client_test.rs +++ b/crates/clickhouse-cloud-api/tests/client_test.rs @@ -335,9 +335,11 @@ async fn create_byoc_infrastructure() { Mock::given(method("POST")) .and(path("/v1/organizations/org-1/byocInfrastructure")) - .and(body_partial_json( - serde_json::json!({"accountId": "123456789012", "displayName": "My BYOC"}), - )) + .and(body_partial_json(serde_json::json!({ + "accountId": "123456789012", + "availabilityZoneSuffixes": ["a", "b"], + "displayName": "My BYOC" + }))) .respond_with(ok_json(serde_json::json!({ "id": "byoc-1", "cloudProvider": "aws", @@ -348,6 +350,10 @@ async fn create_byoc_infrastructure() { let body = ByocInfrastructurePostRequest { account_id: "123456789012".to_string(), + availability_zone_suffixes: vec![ + ByocAvailabilityZoneSuffix::A, + ByocAvailabilityZoneSuffix::B, + ], display_name: "My BYOC".to_string(), ..Default::default() }; @@ -1106,7 +1112,7 @@ async fn get_query_endpoint() { .respond_with(ok_json(serde_json::json!({ "id": "qe-1", "allowedOrigins": "*", - "roles": ["admin"] + "roles": ["sql_console_admin"] }))) .mount(&s) .await; @@ -1118,6 +1124,7 @@ async fn get_query_endpoint() { let qe = resp.result.unwrap(); assert_eq!(qe.id.as_deref(), Some("qe-1")); assert_eq!(qe.allowed_origins.as_deref(), Some("*")); + assert_eq!(qe.roles, Some(vec![QueryEndpointRole::SqlConsoleAdmin])); } #[tokio::test] @@ -1128,20 +1135,21 @@ async fn upsert_query_endpoint() { .and(path( "/v1/organizations/org-1/services/svc-1/serviceQueryEndpoint", )) - .and(body_partial_json( - serde_json::json!({"allowedOrigins": "https://example.com", "roles": ["reader"]}), - )) + .and(body_partial_json(serde_json::json!({ + "allowedOrigins": "https://example.com", + "roles": ["sql_console_read_only"] + }))) .respond_with(ok_json(serde_json::json!({ "id": "qe-1", "allowedOrigins": "https://example.com", - "roles": ["reader"] + "roles": ["sql_console_read_only"] }))) .mount(&s) .await; let body = InstanceServiceQueryApiEndpointsPostRequest { allowed_origins: "https://example.com".to_string(), - roles: vec!["reader".to_string()], + roles: vec![QueryEndpointRole::SqlConsoleReadOnly], ..Default::default() }; let resp = c @@ -3216,6 +3224,50 @@ async fn list_postgres_logs_with_filters() { assert_eq!(logs[0].body.as_deref(), Some("checkpoint complete")); } +#[tokio::test] +async fn list_slow_query_patterns_with_typed_sorting() { + let (s, c) = setup().await; + + Mock::given(method("GET")) + .and(path( + "/v1/organizations/org-1/postgres/pg-1/slowQueryPatterns", + )) + .and(query_param("from_date", "2026-08-01T00:00:00Z")) + .and(query_param("to_date", "2026-08-02T00:00:00Z")) + .and(query_param("db_name", "analytics")) + .and(query_param("db_user", "reporter")) + .and(query_param("db_operation", "SELECT")) + .and(query_param("app", "dashboard")) + .and(query_param("sort_by", "p95_duration")) + .and(query_param("sort_order", "asc")) + .and(query_param("limit", "100")) + .and(query_param("offset", "20")) + .respond_with(ok_json(serde_json::json!([]))) + .mount(&s) + .await; + + let patterns = c + .slow_query_patterns_get_list( + "org-1", + "pg-1", + "2026-08-01T00:00:00Z", + "2026-08-02T00:00:00Z", + Some("analytics"), + Some("reporter"), + Some("SELECT"), + Some("dashboard"), + Some(&SlowQueryPatternsGetListSortby::P95_duration), + Some(&SlowQueryPatternsGetListSortorder::Asc), + Some(100), + Some(20), + ) + .await + .unwrap() + .result + .unwrap(); + assert!(patterns.is_empty()); +} + #[tokio::test] async fn create_postgres_service() { let (s, c) = setup().await; @@ -4148,7 +4200,7 @@ async fn upgrade_window_get_returns_window() { .respond_with(ok_json(serde_json::json!({ "weekday": 2, "startHourUtc": 6, - "duration": 21600 + "duration": 6 }))) .mount(&s) .await; @@ -4156,8 +4208,11 @@ async fn upgrade_window_get_returns_window() { let resp = c.upgrade_window_get("org-1", "svc-1").await.unwrap(); let window = resp.result.unwrap(); assert_eq!(window.weekday, Some(2)); - assert_eq!(window.start_hour_utc, Some(6)); - assert_eq!(window.duration, Some(21600)); + assert_eq!( + window.start_hour_utc, + Some(UpgradeWindowStartHourUtc::Hour6) + ); + assert_eq!(window.duration, Some(UpgradeWindowDuration::SixHours)); } #[tokio::test] @@ -4173,14 +4228,14 @@ async fn upgrade_window_update_sends_body() { .respond_with(ok_json(serde_json::json!({ "weekday": 2, "startHourUtc": 6, - "duration": 21600 + "duration": 6 }))) .mount(&s) .await; let body = UpgradeWindowPutRequest { weekday: 2, - start_hour_utc: 6, + start_hour_utc: UpgradeWindowStartHourUtc::Hour6, }; let resp = c .upgrade_window_update("org-1", "svc-1", &body) @@ -4188,7 +4243,10 @@ async fn upgrade_window_update_sends_body() { .unwrap(); let window = resp.result.unwrap(); assert_eq!(window.weekday, Some(2)); - assert_eq!(window.start_hour_utc, Some(6)); + assert_eq!( + window.start_hour_utc, + Some(UpgradeWindowStartHourUtc::Hour6) + ); } #[tokio::test] diff --git a/crates/clickhouse-cloud-api/tests/integration_test.rs b/crates/clickhouse-cloud-api/tests/integration_test.rs index c02b686..80e4659 100644 --- a/crates/clickhouse-cloud-api/tests/integration_test.rs +++ b/crates/clickhouse-cloud-api/tests/integration_test.rs @@ -359,7 +359,7 @@ async fn cloud_service_crud_lifecycle() -> TestResult<()> { let api_key_uuid = api_key_uuid.clone(); async move { let body = InstanceServiceQueryApiEndpointsPostRequest { - roles: vec!["sql_console_admin".to_string()], + roles: vec![QueryEndpointRole::SqlConsoleAdmin], open_api_keys: vec![api_key_uuid], allowed_origins: "*".to_string(), }; @@ -410,7 +410,7 @@ async fn cloud_service_crud_lifecycle() -> TestResult<()> { .roles .iter() .flatten() - .any(|r| r == "sql_console_admin") + .any(|r| r == &QueryEndpointRole::SqlConsoleAdmin) { return Err(format!( "get missing sql_console_admin role: {:?}", @@ -619,7 +619,7 @@ async fn cloud_service_crud_lifecycle() -> TestResult<()> { let initial_endpoint = initial_endpoint.clone(); async move { let body = InstanceServiceQueryApiEndpointsPostRequest { - roles: vec!["sql_console_admin".to_string()], + roles: vec![QueryEndpointRole::SqlConsoleAdmin], open_api_keys: vec![api_key_uuid.clone()], allowed_origins: "*".to_string(), }; @@ -652,7 +652,7 @@ async fn cloud_service_crud_lifecycle() -> TestResult<()> { .roles .iter() .flatten() - .any(|r| r == "sql_console_admin") + .any(|r| r == &QueryEndpointRole::SqlConsoleAdmin) { return Err(format!( "re-upsert dropped sql_console_admin role: {:?}", @@ -2266,7 +2266,7 @@ async fn cloud_service_crud_lifecycle() -> TestResult<()> { // 10a. PUT a known-valid window. let put_body = UpgradeWindowPutRequest { weekday: 0, - start_hour_utc: 0, + start_hour_utc: UpgradeWindowStartHourUtc::Hour0, }; let put_window = failures .run( diff --git a/crates/clickhouse-cloud-api/tests/model_facade_test.rs b/crates/clickhouse-cloud-api/tests/model_facade_test.rs index 220d75c..5887d8d 100644 --- a/crates/clickhouse-cloud-api/tests/model_facade_test.rs +++ b/crates/clickhouse-cloud-api/tests/model_facade_test.rs @@ -18,6 +18,10 @@ fn extracted_models_keep_root_and_models_paths() { api::ByocConfig::default(), api::models::ByocConfig::default(), ); + assert_same_type( + api::ByocAvailabilityZoneSuffix::default(), + api::models::ByocAvailabilityZoneSuffix::default(), + ); assert_same_type( api::ClickStackChartColor::default(), api::models::ClickStackChartColor::default(), @@ -61,6 +65,14 @@ fn extracted_models_keep_root_and_models_paths() { api::PostgresLogsGetListSortorder::default(), api::models::PostgresLogsGetListSortorder::default(), ); + assert_same_type( + api::SlowQueryPatternsGetListSortby::default(), + api::models::SlowQueryPatternsGetListSortby::default(), + ); + assert_same_type( + api::SlowQueryPatternsGetListSortorder::default(), + api::models::SlowQueryPatternsGetListSortorder::default(), + ); assert_same_type( api::PrometheusDiscoveryTargetGroup::default(), api::models::PrometheusDiscoveryTargetGroup::default(), @@ -72,5 +84,17 @@ fn extracted_models_keep_root_and_models_paths() { api::models::ApiResponse::<()>::default(), ); assert_same_type(api::Service::default(), api::models::Service::default()); + assert_same_type( + api::QueryEndpointRole::default(), + api::models::QueryEndpointRole::default(), + ); + assert_same_type( + api::UpgradeWindowDuration::default(), + api::models::UpgradeWindowDuration::default(), + ); + assert_same_type( + api::UpgradeWindowStartHourUtc::default(), + api::models::UpgradeWindowStartHourUtc::default(), + ); assert_same_type(api::Udf::default(), api::models::Udf::default()); } diff --git a/crates/clickhouse-cloud-api/tests/models_test.rs b/crates/clickhouse-cloud-api/tests/models_test.rs index 5294a9e..f18e508 100644 --- a/crates/clickhouse-cloud-api/tests/models_test.rs +++ b/crates/clickhouse-cloud-api/tests/models_test.rs @@ -905,14 +905,36 @@ fn serialize_postgres_read_replica_request() { fn serialize_byoc_infrastructure_post_request() { let req = ByocInfrastructurePostRequest { account_id: "123456789012".to_string(), + availability_zone_suffixes: vec![ + ByocAvailabilityZoneSuffix::A, + ByocAvailabilityZoneSuffix::B, + ByocAvailabilityZoneSuffix::C, + ByocAvailabilityZoneSuffix::D, + ByocAvailabilityZoneSuffix::E, + ByocAvailabilityZoneSuffix::F, + ], display_name: "My BYOC".to_string(), ..Default::default() }; let json = serde_json::to_value(&req).unwrap(); assert_eq!(json["accountId"], "123456789012"); + assert_eq!( + json["availabilityZoneSuffixes"], + serde_json::json!(["a", "b", "c", "d", "e", "f"]) + ); assert_eq!(json["displayName"], "My BYOC"); } +#[test] +fn byoc_availability_zone_suffix_preserves_unknown_values() { + let suffix: ByocAvailabilityZoneSuffix = serde_json::from_str(r#""future""#).unwrap(); + assert_eq!( + suffix, + ByocAvailabilityZoneSuffix::Unknown("future".to_string()) + ); + assert_eq!(serde_json::to_string(&suffix).unwrap(), r#""future""#); +} + #[test] fn serialize_byoc_infrastructure_patch_request() { let req = ByocInfrastructurePatchRequest { @@ -1006,12 +1028,39 @@ fn serialize_create_reverse_private_endpoint() { fn serialize_instance_query_endpoint_post_request() { let req = InstanceServiceQueryApiEndpointsPostRequest { allowed_origins: "https://example.com".to_string(), - roles: vec!["reader".to_string()], + roles: vec![QueryEndpointRole::SqlConsoleReadOnly], ..Default::default() }; let json = serde_json::to_value(&req).unwrap(); assert_eq!(json["allowedOrigins"], "https://example.com"); - assert_eq!(json["roles"], serde_json::json!(["reader"])); + assert_eq!(json["roles"], serde_json::json!(["sql_console_read_only"])); +} + +#[test] +fn deserialize_query_endpoint_roles_tolerates_unknown_values() { + let endpoint: ServiceQueryAPIEndpoint = serde_json::from_value(serde_json::json!({ + "roles": ["sql_console_admin", "future_role"] + })) + .unwrap(); + + assert_eq!( + endpoint.roles, + Some(vec![ + QueryEndpointRole::SqlConsoleAdmin, + QueryEndpointRole::Unknown("future_role".to_string()), + ]) + ); +} + +#[test] +fn slow_query_sort_enums_preserve_unknown_values() { + let sort_by: SlowQueryPatternsGetListSortby = + serde_json::from_str(r#""future_metric""#).unwrap(); + let sort_order: SlowQueryPatternsGetListSortorder = + serde_json::from_str(r#""future_order""#).unwrap(); + + assert_eq!(sort_by.to_string(), "future_metric"); + assert_eq!(sort_order.to_string(), "future_order"); } #[test] @@ -1752,12 +1801,12 @@ fn scaling_schedule_entry_response_converts_back_into_a_request_entry() { fn upgrade_window_response_converts_back_into_a_put_body() { let request = UpgradeWindowPutRequest::try_from(UpgradeWindow { // `duration` is response-only and does not cross over. - duration: Some(21600), - start_hour_utc: Some(6), + duration: Some(UpgradeWindowDuration::SixHours), + start_hour_utc: Some(UpgradeWindowStartHourUtc::Hour6), weekday: Some(2), }) .unwrap(); - assert_eq!(request.start_hour_utc, 6); + assert_eq!(request.start_hour_utc, UpgradeWindowStartHourUtc::Hour6); assert_eq!(request.weekday, 2); let missing = UpgradeWindowPutRequest::try_from(UpgradeWindow::default()).unwrap_err(); @@ -2050,24 +2099,68 @@ fn deserialize_upgrade_window() { let json = r#"{ "weekday": 2, "startHourUtc": 6, - "duration": 21600 + "duration": 6 }"#; let w: UpgradeWindow = serde_json::from_str(json).unwrap(); assert_eq!(w.weekday, Some(2)); - assert_eq!(w.start_hour_utc, Some(6)); - assert_eq!(w.duration, Some(21600)); + assert_eq!(w.start_hour_utc, Some(UpgradeWindowStartHourUtc::Hour6)); + assert_eq!(w.duration, Some(UpgradeWindowDuration::SixHours)); let round_tripped = serde_json::to_value(&w).unwrap(); assert_eq!(round_tripped["startHourUtc"], 6); assert_eq!(round_tripped["weekday"], 2); - assert_eq!(round_tripped["duration"], 21600); + assert_eq!(round_tripped["duration"], 6); +} + +#[test] +fn upgrade_window_numeric_enums_round_trip_every_known_value() { + for (wire, value) in [ + (0, UpgradeWindowStartHourUtc::Hour0), + (6, UpgradeWindowStartHourUtc::Hour6), + (12, UpgradeWindowStartHourUtc::Hour12), + (18, UpgradeWindowStartHourUtc::Hour18), + ] { + assert_eq!(serde_json::to_value(value).unwrap(), wire); + assert_eq!( + serde_json::from_value::(wire.into()).unwrap(), + value + ); + } + + assert_eq!( + serde_json::to_value(UpgradeWindowDuration::SixHours).unwrap(), + 6 + ); + assert_eq!( + serde_json::from_value::(serde_json::json!(6)).unwrap(), + UpgradeWindowDuration::SixHours + ); +} + +#[test] +fn deserialize_upgrade_window_tolerates_unknown_numeric_values() { + let window: UpgradeWindow = serde_json::from_value(serde_json::json!({ + "startHourUtc": 3, + "duration": 21600 + })) + .unwrap(); + + assert_eq!( + window.start_hour_utc, + Some(UpgradeWindowStartHourUtc::Unknown(3)) + ); + assert_eq!(window.duration, Some(UpgradeWindowDuration::Unknown(21600))); + assert_eq!( + serde_json::to_value(window).unwrap(), + serde_json::json!({"startHourUtc": 3, "duration": 21600}) + ); } #[test] fn serialize_upgrade_window_put_request() { let req = UpgradeWindowPutRequest { weekday: 5, - start_hour_utc: 18, + start_hour_utc: UpgradeWindowStartHourUtc::Hour18, }; let v = serde_json::to_value(&req).unwrap(); assert_eq!(v["weekday"], 5); diff --git a/crates/clickhouse-openapi-analyzer/src/compare.rs b/crates/clickhouse-openapi-analyzer/src/compare.rs index ac426ce..76c27bc 100644 --- a/crates/clickhouse-openapi-analyzer/src/compare.rs +++ b/crates/clickhouse-openapi-analyzer/src/compare.rs @@ -407,30 +407,21 @@ fn compare_enums( for constraint in &spec.enum_constraints { let mapping = map_enum(rust, constraint); - let EnumValues::Strings(spec_values) = &constraint.values else { - let reason = match constraint.values { - EnumValues::Numeric => { - "numeric enum constraints cannot be represented by Rust unit variants" - } - EnumValues::Mixed => { - "mixed enum constraints cannot be represented by one Rust value enum" + match (&constraint.values, mapping) { + (EnumValues::Strings(spec_values), EnumMapping::ValueEnum { name, rust_item }) => { + let enum_info = &rust.enums[&name]; + if !enum_info.integer_values.is_empty() { + record_unsupported( + constraint, + Some(rust_item), + &format!("Rust enum {name} uses integer discriminants for a string enum"), + config, + &mut unsupported_hits, + report, + ); + continue; } - EnumValues::Strings(_) => unreachable!(), - }; - record_unsupported( - constraint, - mapping_rust_item(&mapping), - reason, - config, - &mut unsupported_hits, - report, - ); - continue; - }; - - match mapping { - EnumMapping::ValueEnum { name, rust_item } => { - let rust_values = &rust.enums[&name].values; + let rust_values = &enum_info.values; for value in spec_values.difference(rust_values) { report.findings.push( Finding::new( @@ -463,7 +454,83 @@ fn compare_enums( } } } - EnumMapping::Unsupported { rust_item, reason } => record_unsupported( + (EnumValues::Integers(spec_values), EnumMapping::ValueEnum { name, rust_item }) => { + let enum_info = &rust.enums[&name]; + if !enum_info.uses_i64_serde_conversion { + record_unsupported( + constraint, + Some(rust_item), + &format!( + "Rust enum {name} does not declare Serde conversion to and from i64" + ), + config, + &mut unsupported_hits, + report, + ); + continue; + } + if !enum_info.values.is_empty() { + record_unsupported( + constraint, + Some(rust_item), + &format!("Rust enum {name} uses string values for an integer enum"), + config, + &mut unsupported_hits, + report, + ); + continue; + } + let rust_values = &enum_info.integer_values; + for value in spec_values.difference(rust_values) { + report.findings.push( + Finding::new( + FindingKind::MissingEnumValue, + format!("{name} has no variant for wire value {value}"), + ) + .at_spec(&constraint.pointer) + .at_rust(&rust_item) + .detail("enum", &name) + .detail("value", value.to_string()), + ); + } + for value in rust_values.difference(spec_values) { + let wire_value = value.to_string(); + let key = (name.clone(), wire_value.clone()); + if config.extra_enum_value_exemptions.contains(&key) { + extra_enum_hits.insert(key); + } else { + report.findings.push( + Finding::new( + FindingKind::ExtraEnumValue, + format!( + "{name} serializes wire value {value}, absent from the spec" + ), + ) + .at_spec(&constraint.pointer) + .at_rust(&rust_item) + .detail("enum", &name) + .detail("value", wire_value), + ); + } + } + } + (EnumValues::Numeric, mapping) => record_unsupported( + constraint, + mapping_rust_item(&mapping), + "numeric enum constraints that are non-integer or outside i64 cannot be represented by Rust unit variants", + config, + &mut unsupported_hits, + report, + ), + (EnumValues::Mixed, mapping) => record_unsupported( + constraint, + mapping_rust_item(&mapping), + "mixed enum constraints cannot be represented by one Rust value enum", + config, + &mut unsupported_hits, + report, + ), + (_, EnumMapping::Unsupported { rust_item, reason }) => record_unsupported( constraint, rust_item, &reason, @@ -471,7 +538,7 @@ fn compare_enums( &mut unsupported_hits, report, ), - EnumMapping::Unmapped => {} + (_, EnumMapping::Unmapped) => {} } } @@ -1208,7 +1275,7 @@ mod tests { pub struct Widget { pub states: Vec, pub mode: Mode, - pub count: i64, + pub count: Count, } pub enum State { #[serde(rename = "ready")] Ready } pub enum Mode { #[serde(rename = "fast")] Fast } @@ -1216,11 +1283,15 @@ mod tests { #[serde(rename = "asc")] Asc, #[serde(rename = "desc")] Desc, } + #[repr(i64)] + #[serde(from = "i64", into = "i64")] + pub enum Count { + One = 1, + Two = 2, + #[serde(untagged)] Unknown(i64), + } "#; - let mut config = AnalyzerConfig::default(); - config - .acknowledged_unsupported_enum_pointers - .insert("/components/schemas/Widget/properties/count".to_string()); + let config = AnalyzerConfig::default(); let rust = RustInventory::parse( client, models, @@ -1230,8 +1301,111 @@ mod tests { let openapi = OpenApiInventory::build(&spec, &config).unwrap(); let report = compare(&rust, &openapi, &openapi, &config); assert!(!report.has_drift(), "{}", report.render_text()); + assert!(report.unsupported_enum_constraints.is_empty()); + } + + #[test] + fn checks_integer_enum_values_bidirectionally() { + let report = analyze_fixture( + r#" + pub struct Widget { pub count: Count } + #[repr(i64)] + #[serde(from = "i64", into = "i64")] + pub enum Count { + Zero = 0, + Twelve = 12, + #[serde(untagged)] Unknown(i64), + } + "#, + serde_json::json!({ + "required": ["count"], + "properties": {"count": {"type": "integer", "enum": [0, 6]}} + }), + AnalyzerConfig::default(), + ); + + assert!(report.findings.iter().any(|finding| { + finding.kind == FindingKind::MissingEnumValue + && finding.details.get("value").map(String::as_str) == Some("6") + })); + assert!(report.findings.iter().any(|finding| { + finding.kind == FindingKind::ExtraEnumValue + && finding.details.get("value").map(String::as_str) == Some("12") + })); + assert!(report.unsupported_enum_constraints.is_empty()); + } + + #[test] + fn integer_enum_requires_explicit_i64_serde_conversion() { + let report = analyze_fixture( + r#" + pub struct Widget { pub count: Count } + #[repr(i64)] + pub enum Count { Zero = 0, Six = 6 } + "#, + serde_json::json!({ + "required": ["count"], + "properties": {"count": {"type": "integer", "enum": [0, 6]}} + }), + AnalyzerConfig::default(), + ); + assert_eq!(report.unsupported_enum_constraints.len(), 1); - assert!(report.unsupported_enum_constraints[0].acknowledged); + assert!( + report.unsupported_enum_constraints[0] + .reason + .contains("Serde conversion") + ); + } + + #[test] + fn acknowledgement_becomes_stale_when_an_integer_enum_is_checkable() { + let pointer = "/components/schemas/Widget/properties/count"; + let mut config = AnalyzerConfig::default(); + config + .acknowledged_unsupported_enum_pointers + .insert(pointer.to_string()); + let report = analyze_fixture( + r#" + pub struct Widget { pub count: Count } + #[repr(i64)] + #[serde(from = "i64", into = "i64")] + pub enum Count { Zero = 0, Six = 6 } + "#, + serde_json::json!({ + "required": ["count"], + "properties": {"count": {"type": "integer", "enum": [0, 6]}} + }), + config, + ); + + assert!(report.unsupported_enum_constraints.is_empty()); + assert!(report.findings.iter().any(|finding| { + finding.kind == FindingKind::StaleExemption + && finding.spec_pointer.as_deref() == Some(pointer) + })); + } + + #[test] + fn non_integer_numeric_enum_remains_unsupported() { + let report = analyze_fixture( + r#" + pub struct Widget { pub ratio: Ratio } + pub enum Ratio { Half } + "#, + serde_json::json!({ + "required": ["ratio"], + "properties": {"ratio": {"type": "number", "enum": [0.5]}} + }), + AnalyzerConfig::default(), + ); + + assert_eq!(report.unsupported_enum_constraints.len(), 1); + assert!( + report.unsupported_enum_constraints[0] + .reason + .contains("non-integer or outside i64") + ); } #[test] diff --git a/crates/clickhouse-openapi-analyzer/src/config.rs b/crates/clickhouse-openapi-analyzer/src/config.rs index 56eebd9..e8bd8ca 100644 --- a/crates/clickhouse-openapi-analyzer/src/config.rs +++ b/crates/clickhouse-openapi-analyzer/src/config.rs @@ -143,23 +143,15 @@ const OPTIONALITY_EXEMPTIONS: &[(&str, &str)] = &[ ("PgConfig", "work_mem"), ]; -// Follow-up remediation is tracked in #296. The inline UDF-attach 424 error -// response has no public client/model type, so its enum constraints remain -// deliberately acknowledged until typed error responses are supported. Remove -// entries as their Rust API types become checkable; stale acknowledgements are -// actionable findings. +// The deprecated API-key `roles` fields are feature-gated, frozen legacy API +// replaced by assigned role IDs. They deliberately remain strings to avoid a +// source-breaking change for deprecated-fields consumers. The inline +// UDF-attach 424 error response has no public client/model type, so its enum +// constraints remain acknowledged until typed error responses are supported. const ACKNOWLEDGED_UNSUPPORTED_ENUM_POINTERS: &[&str] = &[ "/components/schemas/ApiKey/properties/roles/items", "/components/schemas/ApiKeyPatchRequest/properties/roles/items", "/components/schemas/ApiKeyPostRequest/properties/roles/items", - "/components/schemas/ByocInfrastructurePostRequest/properties/availabilityZoneSuffixes/items", - "/components/schemas/InstanceServiceQueryApiEndpointsPostRequest/properties/roles/items", - "/components/schemas/ServiceQueryAPIEndpoint/properties/roles/items", - "/components/schemas/UpgradeWindow/properties/duration", - "/components/schemas/UpgradeWindow/properties/startHourUtc", - "/components/schemas/UpgradeWindowPutRequest/properties/startHourUtc", - "/paths/~1v1~1organizations~1{organizationId}~1postgres~1{postgresId}~1slowQueryPatterns/get/parameters/8/schema", - "/paths/~1v1~1organizations~1{organizationId}~1postgres~1{postgresId}~1slowQueryPatterns/get/parameters/9/schema", "/paths/~1v1~1organizations~1{organizationId}~1udfs~1{functionName}~1attachments~1{serviceId}/put/responses/424/content/application~1json/schema/properties/code", "/paths/~1v1~1organizations~1{organizationId}~1udfs~1{functionName}~1attachments~1{serviceId}/put/responses/424/content/application~1json/schema/properties/serviceState", ]; diff --git a/crates/clickhouse-openapi-analyzer/src/openapi.rs b/crates/clickhouse-openapi-analyzer/src/openapi.rs index eb7ef92..f88148f 100644 --- a/crates/clickhouse-openapi-analyzer/src/openapi.rs +++ b/crates/clickhouse-openapi-analyzer/src/openapi.rs @@ -55,6 +55,7 @@ pub(crate) enum EnumContext { #[derive(Debug, Clone)] pub(crate) enum EnumValues { Strings(BTreeSet), + Integers(BTreeSet), Numeric, Mixed, } @@ -739,6 +740,8 @@ fn walk_schema( .map(str::to_string) .collect(), ) + } else if values.iter().all(|value| value.as_i64().is_some()) { + EnumValues::Integers(values.iter().filter_map(Value::as_i64).collect()) } else if values.iter().all(Value::is_number) { EnumValues::Numeric } else { @@ -974,6 +977,34 @@ mod tests { )); } + #[test] + fn distinguishes_integer_and_non_integer_numeric_enums() { + let spec = serde_json::json!({ + "paths": {}, + "components": {"schemas": { + "Widget": {"properties": { + "integer": {"enum": [-6, 0, 12]}, + "numeric": {"enum": [0.5, 1.5]} + }} + }} + }); + let inventory = OpenApiInventory::build(&spec, &AnalyzerConfig::default()).unwrap(); + + assert!(inventory.enum_constraints.iter().any(|constraint| { + matches!( + &constraint.values, + EnumValues::Integers(values) + if values == &BTreeSet::from([-6, 0, 12]) + ) + })); + assert!( + inventory + .enum_constraints + .iter() + .any(|constraint| matches!(constraint.values, EnumValues::Numeric)) + ); + } + #[test] fn inventories_schema_enums_across_composition_positions() { let spec = serde_json::json!({ diff --git a/crates/clickhouse-openapi-analyzer/src/report.rs b/crates/clickhouse-openapi-analyzer/src/report.rs index d713895..4ecd8a1 100644 --- a/crates/clickhouse-openapi-analyzer/src/report.rs +++ b/crates/clickhouse-openapi-analyzer/src/report.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; -pub const REPORT_SCHEMA_VERSION: u32 = 2; +pub const REPORT_SCHEMA_VERSION: u32 = 3; #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] diff --git a/crates/clickhouse-openapi-analyzer/src/rust_inventory.rs b/crates/clickhouse-openapi-analyzer/src/rust_inventory.rs index d295e32..fb75533 100644 --- a/crates/clickhouse-openapi-analyzer/src/rust_inventory.rs +++ b/crates/clickhouse-openapi-analyzer/src/rust_inventory.rs @@ -460,6 +460,10 @@ pub(crate) struct StructInfo { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct EnumInfo { pub(crate) values: BTreeSet, + pub(crate) integer_values: BTreeSet, + /// Explicit discriminants are treated as wire values only when Serde is + /// configured to convert the enum to and from `i64`. + pub(crate) uses_i64_serde_conversion: bool, pub(crate) is_value_enum: bool, pub(crate) values_const: Option>, /// Named types carried by any variant's payload (union arms), so @@ -625,8 +629,12 @@ impl RustInventory { self.model_types.insert(name.clone()); let container = serde_options(&item_enum.attrs)?; let mut values = BTreeSet::new(); + let mut integer_values = BTreeSet::new(); let mut variant_type_names = BTreeSet::new(); let mut is_value_enum = !container.untagged; + let uses_i64_serde_conversion = container.into.as_deref() == Some("i64") + && (container.from.as_deref() == Some("i64") + || container.try_from.as_deref() == Some("i64")); for variant in &item_enum.variants { for field in variant.fields.iter() { collect_type_names(&field.ty, &mut variant_type_names); @@ -639,13 +647,28 @@ impl RustInventory { is_value_enum = false; continue; } - let rust_name = variant.ident.unraw().to_string(); - values.insert(options.rename.unwrap_or(rust_name)); + if uses_i64_serde_conversion { + let Some((_, discriminant)) = &variant.discriminant else { + // Numeric wire values must stay explicit in model source. + is_value_enum = false; + continue; + }; + if let Some(value) = integer_discriminant(discriminant) { + integer_values.insert(value); + } else { + is_value_enum = false; + } + } else { + let rust_name = variant.ident.unraw().to_string(); + values.insert(options.rename.unwrap_or(rust_name)); + } } self.enums.insert( name, EnumInfo { values, + integer_values, + uses_i64_serde_conversion, is_value_enum, values_const: None, variant_type_names, @@ -859,6 +882,9 @@ fn collect_type_names(ty: &Type, output: &mut BTreeSet) { #[derive(Default)] struct SerdeOptions { rename: Option, + from: Option, + into: Option, + try_from: Option, untagged: bool, other: bool, default: bool, @@ -885,6 +911,12 @@ fn serde_options(attributes: &[Attribute]) -> syn::Result { Ok(()) })?; } + } else if meta.path.is_ident("from") { + options.from = Some(meta.value()?.parse::()?.value()); + } else if meta.path.is_ident("into") { + options.into = Some(meta.value()?.parse::()?.value()); + } else if meta.path.is_ident("try_from") { + options.try_from = Some(meta.value()?.parse::()?.value()); } else if meta.path.is_ident("rename_all") { return Err(meta.error( "rename_all is not allowed in models.rs: wire names must be explicit \ @@ -948,6 +980,33 @@ fn dereference(expression: &Expr) -> &Expr { } } +fn integer_discriminant(expression: &Expr) -> Option { + match expression { + Expr::Paren(paren) => integer_discriminant(&paren.expr), + Expr::Lit(literal) => match &literal.lit { + Lit::Int(value) => value.base10_parse().ok(), + _ => None, + }, + Expr::Unary(unary) if matches!(unary.op, syn::UnOp::Neg(_)) => { + let Expr::Lit(literal) = unary.expr.as_ref() else { + return None; + }; + let Lit::Int(value) = &literal.lit else { + return None; + }; + let magnitude = value.base10_parse::().ok()?; + if magnitude == (i64::MAX as u64) + 1 { + Some(i64::MIN) + } else if magnitude <= i64::MAX as u64 { + Some(-(magnitude as i64)) + } else { + None + } + } + _ => None, + } +} + fn string_array(expression: &Expr) -> BTreeSet { let Expr::Array(array) = dereference(expression) else { return BTreeSet::new(); @@ -1106,6 +1165,19 @@ mod tests { assert_eq!(value("clickhouse_custom"), CfgValue::Unknown); } + #[test] + fn parses_parenthesized_and_minimum_integer_discriminants() { + for (source, expected) in [ + ("(1)", Some(1)), + ("(-1)", Some(-1)), + ("-9223372036854775808", Some(i64::MIN)), + ("-9223372036854775809", None), + ] { + let expression = syn::parse_str(source).unwrap(); + assert_eq!(integer_discriminant(&expression), expected, "{source}"); + } + } + #[test] fn inventories_structural_rust_and_serde_details() { let client = r#" @@ -1131,6 +1203,28 @@ mod tests { } #[serde(untagged)] pub enum Union { Text(String), Count(i64) } + #[repr(i64)] + #[serde(from = "i64", into = "i64")] + pub enum NumericValue { + Negative = -6, + Zero = 0, + Twelve = 12, + #[serde(untagged)] + Unknown(i64), + } + #[serde(from = "i64", into = "i64")] + pub enum NumericValueWithImplicitDiscriminant { + Zero = 0, + One, + #[serde(untagged)] + Unknown(i64), + } + pub enum StringValueWithDiscriminants { + #[serde(rename = "ready")] + Ready = 1, + #[serde(rename = "done")] + Done = 2, + } pub type WidgetAlias = Option>>; pub struct Aliased { pub values: WidgetAlias } "#; @@ -1154,6 +1248,27 @@ mod tests { ); assert!(inventory.enums["WidgetType"].is_value_enum); assert!(!inventory.enums["Union"].is_value_enum); + assert_eq!( + inventory.enums["NumericValue"].integer_values, + BTreeSet::from([-6, 0, 12]) + ); + assert!(inventory.enums["NumericValue"].values.is_empty()); + assert!(inventory.enums["NumericValue"].is_value_enum); + assert!(inventory.enums["NumericValue"].uses_i64_serde_conversion); + assert!(!inventory.enums["NumericValueWithImplicitDiscriminant"].is_value_enum); + assert_eq!( + inventory.enums["NumericValueWithImplicitDiscriminant"].integer_values, + BTreeSet::from([0]) + ); + assert_eq!( + inventory.enums["StringValueWithDiscriminants"].values, + BTreeSet::from(["done".to_string(), "ready".to_string()]) + ); + assert!( + inventory.enums["StringValueWithDiscriminants"] + .integer_values + .is_empty() + ); assert_eq!( inventory.array_item_type(&inventory.structs["Aliased"].fields["values"].rust_type), Some("WidgetType".to_string()) diff --git a/crates/clickhousectl/src/cloud/service_query.rs b/crates/clickhousectl/src/cloud/service_query.rs index d6b257e..fdf2358 100644 --- a/crates/clickhousectl/src/cloud/service_query.rs +++ b/crates/clickhousectl/src/cloud/service_query.rs @@ -12,16 +12,9 @@ use crate::cloud::credentials::{self, ServiceQueryKey}; use chrono::{DateTime, Utc}; use clickhouse_cloud_api::models::{ ApiKeyPostRequest, ApiKeyPostRequestState, ApiKeyPostResponse, - InstanceServiceQueryApiEndpointsPostRequest, IpAccessListEntry, + InstanceServiceQueryApiEndpointsPostRequest, IpAccessListEntry, QueryEndpointRole, }; -/// The role attached to the query endpoint binding. Grants the key read + -/// write SQL access through the query endpoint, scoped to this single -/// service. The binding (not the API key) is what enforces the scope, so the -/// key cannot reach other services in the org regardless of any future -/// org-level role assignments. -const QUERY_ENDPOINT_ROLE: &str = "sql_console_admin"; - /// Default `allowedOrigins` for the query endpoint. The CLI is a non-browser /// caller so CORS doesn't apply, but the API still requires a value. const ALLOWED_ORIGINS: &str = "*"; @@ -192,7 +185,9 @@ async fn bind_query_endpoint( } let endpoint_request = InstanceServiceQueryApiEndpointsPostRequest { - roles: vec![QUERY_ENDPOINT_ROLE.to_string()], + // The binding grants read/write SQL access only through this service's + // endpoint; it does not assign an organization-level role to the key. + roles: vec![QueryEndpointRole::SqlConsoleAdmin], open_api_keys, allowed_origins: ALLOWED_ORIGINS.to_string(), }; diff --git a/crates/clickhousectl/src/cloud/services.rs b/crates/clickhousectl/src/cloud/services.rs index 024121a..52e52ee 100644 --- a/crates/clickhousectl/src/cloud/services.rs +++ b/crates/clickhousectl/src/cloud/services.rs @@ -9,13 +9,13 @@ use clap::Subcommand; use clap::builder::PossibleValuesParser; use clickhouse_cloud_api::models::{ AutoscalingMode, InstancePrivateEndpointsPatch, InstanceServiceQueryApiEndpointsPostRequest, - InstanceTagsPatch, IpAccessListEntry, IpAccessListPatch, ServicPrivateEndpointePostRequest, - Service, ServiceEndpoint, ServiceEndpointChange, ServiceEndpointChangeProtocol, - ServicePasswordPatchRequest, ServicePatchRequest, ServicePatchRequestReleasechannel, - ServicePostRequest, ServicePostRequestCompliancetype, ServicePostRequestProfile, - ServicePostRequestProvider, ServicePostRequestRegion, ServicePostRequestReleasechannel, - ServiceReplicaScalingPatchRequest, ServiceState, ServiceStatePatchRequest, - ServiceStatePatchRequestCommand, + InstanceTagsPatch, IpAccessListEntry, IpAccessListPatch, QueryEndpointRole, + ServicPrivateEndpointePostRequest, Service, ServiceEndpoint, ServiceEndpointChange, + ServiceEndpointChangeProtocol, ServicePasswordPatchRequest, ServicePatchRequest, + ServicePatchRequestReleasechannel, ServicePostRequest, ServicePostRequestCompliancetype, + ServicePostRequestProfile, ServicePostRequestProvider, ServicePostRequestRegion, + ServicePostRequestReleasechannel, ServiceReplicaScalingPatchRequest, ServiceState, + ServiceStatePatchRequest, ServiceStatePatchRequestCommand, }; use std::io::IsTerminal; use tabled::{Table, Tabled, settings::Style}; @@ -470,14 +470,14 @@ pub enum QueryEndpointCommands { service_id: String, /// Roles to grant access (can be specified multiple times) - #[arg(long)] + #[arg(long, value_parser = PossibleValuesParser::new(QueryEndpointRole::VALUES))] role: Vec, /// OpenAPI key IDs to authorize #[arg(long = "open-api-key")] open_api_key: Vec, - /// Allowed origins string for browser access + /// Allowed origins string for browser access (defaults to "*") #[arg(long)] allowed_origins: Option, @@ -1221,12 +1221,19 @@ fn build_service_password_patch_request( fn build_query_endpoint_create_request( options: &QueryEndpointCreateOptions, -) -> InstanceServiceQueryApiEndpointsPostRequest { - InstanceServiceQueryApiEndpointsPostRequest { - roles: options.roles.clone(), +) -> Result> { + Ok(InstanceServiceQueryApiEndpointsPostRequest { + roles: options + .roles + .iter() + .map(|role| parse_serde_enum(role, "role", QueryEndpointRole::VALUES)) + .collect::>()?, open_api_keys: options.open_api_keys.clone(), - allowed_origins: options.allowed_origins.clone().unwrap_or_default(), - } + allowed_origins: options + .allowed_origins + .clone() + .unwrap_or_else(|| "*".to_string()), + }) } fn build_private_endpoint_create_request( @@ -1646,7 +1653,7 @@ async fn query_endpoint_create( json: bool, ) -> Result<(), Box> { let org_id = resolve_org_id(client, options.org_id.as_deref()).await?; - let request = build_query_endpoint_create_request(&options); + let request = build_query_endpoint_create_request(&options)?; let endpoint = client .create_query_endpoint(&org_id, service_id, &request) .await?; @@ -1658,7 +1665,13 @@ async fn query_endpoint_create( println!(" ID: {}", or_absent(endpoint.id.as_deref())); println!( " Roles: {}", - or_absent(endpoint.roles.as_ref().map(|roles| roles.join(", "))) + or_absent(endpoint.roles.as_ref().map(|roles| { + roles + .iter() + .map(ToString::to_string) + .collect::>() + .join(", ") + })) ); } Ok(()) @@ -3216,9 +3229,9 @@ mod tests { "create", "svc-1", "--role", - "admin", + "sql_console_read_only", "--role", - "developer", + "sql_console_admin", "--open-api-key", "key-1", "--open-api-key", @@ -3242,7 +3255,7 @@ mod tests { panic!("expected query-endpoint create"); }; assert_eq!(service_id, "svc-1"); - assert_eq!(role, vec!["admin", "developer"]); + assert_eq!(role, vec!["sql_console_read_only", "sql_console_admin"]); assert_eq!(open_api_key, vec!["key-1", "key-2"]); assert_eq!(allowed_origins.as_deref(), Some("https://example.com")); assert_eq!(org_id.as_deref(), Some("org-1")); @@ -3274,6 +3287,22 @@ mod tests { } } + #[test] + fn rejects_unknown_query_endpoint_role() { + let result = Cli::try_parse_from([ + "clickhousectl", + "cloud", + "service", + "query-endpoint", + "create", + "svc-1", + "--role", + "admin", + ]); + + assert!(result.is_err()); + } + #[test] fn parses_private_endpoint_create_values() { let command = parse_service(&[ @@ -4211,23 +4240,34 @@ mod tests { #[test] fn build_query_endpoint_create_request_supports_minimal_fields() { - let request = build_query_endpoint_create_request(&QueryEndpointCreateOptions::default()); + let request = + build_query_endpoint_create_request(&QueryEndpointCreateOptions::default()).unwrap(); assert!(request.roles.is_empty()); assert!(request.open_api_keys.is_empty()); - assert!(request.allowed_origins.is_empty()); + assert_eq!(request.allowed_origins, "*"); } #[test] fn build_query_endpoint_create_request_supports_maximal_fields() { let request = build_query_endpoint_create_request(&QueryEndpointCreateOptions { - roles: vec!["admin".to_string(), "developer".to_string()], + roles: vec![ + "sql_console_read_only".to_string(), + "sql_console_admin".to_string(), + ], open_api_keys: vec!["key-1".to_string(), "key-2".to_string()], allowed_origins: Some("https://example.com".to_string()), org_id: None, - }); + }) + .unwrap(); - assert_eq!(request.roles, vec!["admin", "developer"]); + assert_eq!( + request.roles, + vec![ + QueryEndpointRole::SqlConsoleReadOnly, + QueryEndpointRole::SqlConsoleAdmin, + ] + ); assert_eq!(request.open_api_keys, vec!["key-1", "key-2"]); assert_eq!(request.allowed_origins, "https://example.com"); } diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 5edf903..6bcd3fe 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -209,6 +209,53 @@ fn write_project_api_credentials(root: &Path, key: &str, secret: &str) { // ── Organization-scoped error context (issue #334) ───────────────────────── +#[tokio::test] +async fn query_endpoint_create_sends_typed_roles() { + let mock = MockServer::start().await; + Mock::given(method("POST")) + .and(path( + "/v1/organizations/org-1/services/svc-1/serviceQueryEndpoint", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "result": { + "id": "endpoint-1", + "roles": ["sql_console_read_only", "sql_console_admin"] + }, + "status": 200, + "requestId": "stub-query-endpoint-create" + }))) + .mount(&mock) + .await; + + let body = invoke_cli_capture_body( + &mock, + &[ + "service", + "query-endpoint", + "create", + "svc-1", + "--role", + "sql_console_read_only", + "--role", + "sql_console_admin", + "--open-api-key", + "key-1", + "--org-id", + "org-1", + ], + ) + .await; + + assert_eq!( + body, + serde_json::json!({ + "roles": ["sql_console_read_only", "sql_console_admin"], + "openApiKeys": ["key-1"], + "allowedOrigins": "*" + }) + ); +} + #[tokio::test] async fn service_list_bare_not_found_includes_the_requested_organization() { const WRONG_ORG_ID: &str = "00000000-0000-4000-8000-000000000001"; @@ -2974,6 +3021,7 @@ async fn provision_against_endpoint_with_keys(existing_keys: Value) -> (tempfile .find(|r| r.method == wiremock::http::Method::POST && r.url.path() == endpoint_path) .expect("the endpoint upsert must be sent"); let body: Value = serde_json::from_slice(&upsert.body).unwrap(); + assert_eq!(body["roles"], serde_json::json!(["sql_console_admin"])); (dir, body["openApiKeys"].clone()) } diff --git a/scripts/check-openapi-drift.py b/scripts/check-openapi-drift.py index caa6276..a110104 100755 --- a/scripts/check-openapi-drift.py +++ b/scripts/check-openapi-drift.py @@ -91,7 +91,7 @@ def run_analyzer(spec: dict) -> dict: report = json.loads(result.stdout) except json.JSONDecodeError as error: raise RuntimeError("OpenAPI analyzer emitted invalid JSON") from error - if report.get("schema_version") != 2: + if report.get("schema_version") != 3: raise RuntimeError( f"Unsupported DriftReport schema version: {report.get('schema_version')!r}" ) diff --git a/scripts/tests/test_check_openapi_drift.py b/scripts/tests/test_check_openapi_drift.py index f0db9ac..9250dfb 100644 --- a/scripts/tests/test_check_openapi_drift.py +++ b/scripts/tests/test_check_openapi_drift.py @@ -16,7 +16,7 @@ class DriftScriptTests(unittest.TestCase): def test_groups_findings_and_renders_spec_snippets(self): report = { - "schema_version": 2, + "schema_version": 3, "findings": [ { "kind": "missing_client_method", @@ -245,7 +245,7 @@ def fake_run(cmd, *args, **kwargs): def test_analyzer_receives_the_rust_source_tree(self, run): run.return_value = SimpleNamespace( returncode=0, - stdout=json.dumps({"schema_version": 2, "findings": []}), + stdout=json.dumps({"schema_version": 3, "findings": []}), stderr="", ) @@ -269,7 +269,7 @@ def test_analyzer_subprocess_failure_is_fatal(self, run): def test_analyzer_report_schema_is_validated(self, run): run.return_value = SimpleNamespace( returncode=0, - stdout=json.dumps({"schema_version": 3}), + stdout=json.dumps({"schema_version": 2}), stderr="", ) with self.assertRaisesRegex(RuntimeError, "schema version"):