amogh-jahagirdar commented on code in PR #2692: URL: https://github.com/apache/iceberg-rust/pull/2692#discussion_r3487082079
########## crates/catalog/rest/src/endpoint.rs: ########## @@ -0,0 +1,245 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Server capability negotiation via the `endpoints` field of `GET /v1/config`. +//! +//! A REST server may advertise the set of routes it supports in the `endpoints` +//! field of its configuration response, letting clients negotiate optional +//! capabilities instead of assuming every server implements every operation. +//! Each entry is a `"{method} {path}"` string, for example +//! `"POST /v1/{prefix}/namespaces/{namespace}/tables"`; parse one through +//! [`Endpoint`]'s [`FromStr`] implementation. +//! +//! Use [`RestCatalog::supports_endpoint`](crate::RestCatalog::supports_endpoint) +//! to check whether the connected server advertised a given [`Endpoint`]. + +use std::collections::HashSet; +use std::fmt::{self, Display, Formatter}; +use std::str::FromStr; +use std::sync::LazyLock; + +use iceberg::{Error, ErrorKind}; +use reqwest::Method; +use serde::de::{Error as DeError, Visitor}; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; + +/// A single route a REST server advertises support for, parsed from the +/// `endpoints` field of `GET /v1/config`. +/// +/// The wire form is `"{method} {path}"` — an HTTP method and a path template +/// separated by a single space, e.g. +/// `"POST /v1/{prefix}/namespaces/{namespace}/tables"`. Parse one with +/// [`str::parse`]: the method is validated and normalized, and the path is the +/// template the server advertises (with `{prefix}`, `{namespace}`, … +/// placeholders), compared verbatim. +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub struct Endpoint { + method: Method, + path: String, +} + +impl Endpoint { + /// Builds an endpoint from a known-valid method and path template. + /// + /// Intended for internal constants and tests; untrusted input (such as a + /// server's config response) is parsed through [`FromStr`], which validates + /// it. + pub(crate) fn new(method: Method, path: impl Into<String>) -> Self { + Self { + method, + path: path.into(), + } + } + + /// The HTTP method, e.g. `GET` or `POST`. + pub fn method(&self) -> &str { + self.method.as_str() + } + + /// The path template, e.g. `/v1/{prefix}/namespaces`. + pub fn path(&self) -> &str { + &self.path + } +} + +impl FromStr for Endpoint { + type Err = Error; + + fn from_str(s: &str) -> std::result::Result<Self, Self::Err> { + // The wire form is exactly `"<method> <path>"` separated by a single + // space; paths never contain spaces, so require exactly two non-empty + // parts and a valid HTTP method. + let mut parts = s.split(' '); + match (parts.next(), parts.next(), parts.next()) { + (Some(method), Some(path), None) if !method.is_empty() && !path.is_empty() => { + let method = Method::from_str(&method.to_ascii_uppercase()).map_err(|_| { + Error::new( + ErrorKind::DataInvalid, + format!("invalid HTTP method in endpoint: {s:?}"), + ) + })?; + Ok(Self { + method, + path: path.to_string(), + }) + } + _ => Err(Error::new( + ErrorKind::DataInvalid, + format!( + r#"invalid endpoint {s:?}: expected "<method> <path>" separated by a single space"# + ), + )), + } + } +} + +impl Display for Endpoint { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{} {}", self.method, self.path) + } +} + +impl Serialize for Endpoint { + fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error> + where S: Serializer { + serializer.collect_str(self) + } +} + +impl<'de> Deserialize<'de> for Endpoint { + fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error> + where D: Deserializer<'de> { + struct EndpointVisitor; + + impl Visitor<'_> for EndpointVisitor { + type Value = Endpoint; + + fn expecting(&self, f: &mut Formatter<'_>) -> fmt::Result { + f.write_str(r#"an endpoint string of the form "<method> <path>""#) + } + + fn visit_str<E>(self, v: &str) -> std::result::Result<Endpoint, E> + where E: DeError { + Endpoint::from_str(v).map_err(E::custom) + } + } + + deserializer.deserialize_str(EndpointVisitor) + } +} + +/// The standard v1 endpoints assumed to be supported when a server does not +/// advertise an `endpoints` list in its `GET /v1/config` response. +/// +/// A server that omits the field (for example one that predates it) is treated +/// as supporting this base set of namespace and table operations; a server that +/// sends an explicit list — even an empty one — is taken at its word. +pub(crate) static DEFAULT_ENDPOINTS: LazyLock<HashSet<Endpoint>> = LazyLock::new(|| { + [ + (Method::GET, "/v1/{prefix}/namespaces"), + (Method::POST, "/v1/{prefix}/namespaces"), + (Method::GET, "/v1/{prefix}/namespaces/{namespace}"), + (Method::DELETE, "/v1/{prefix}/namespaces/{namespace}"), + ( + Method::POST, + "/v1/{prefix}/namespaces/{namespace}/properties", + ), + (Method::GET, "/v1/{prefix}/namespaces/{namespace}/tables"), + (Method::POST, "/v1/{prefix}/namespaces/{namespace}/tables"), + ( + Method::GET, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}", + ), + ( + Method::POST, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}", + ), + ( + Method::DELETE, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}", + ), + (Method::POST, "/v1/{prefix}/tables/rename"), + (Method::POST, "/v1/{prefix}/namespaces/{namespace}/register"), + ( + Method::POST, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics", + ), + (Method::POST, "/v1/{prefix}/transactions/commit"), + ] + .into_iter() + .map(|(method, path)| Endpoint::new(method, path)) + .collect() +}); + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_and_round_trips() { + let ep: Endpoint = "POST /v1/{prefix}/namespaces/{namespace}/tables" + .parse() + .unwrap(); + assert_eq!(ep.method(), "POST"); + assert_eq!(ep.path(), "/v1/{prefix}/namespaces/{namespace}/tables"); + + let json = serde_json::to_string(&ep).unwrap(); + assert_eq!(json, r#""POST /v1/{prefix}/namespaces/{namespace}/tables""#); + assert_eq!(serde_json::from_str::<Endpoint>(&json).unwrap(), ep); + } + + #[test] + fn rejects_malformed_endpoints() { + // Must be exactly "<method> <path>" with a single separating space. + for invalid in [ + "GET", // no space + "GET /v1", // two spaces + " GET /v1", // leading space (empty method) + "GET ", // trailing space (empty path) + " /v1", // empty method + "", // empty + ] { + assert!( + invalid.parse::<Endpoint>().is_err(), + "expected {invalid:?} to be rejected" + ); + } + } + + #[test] + fn normalizes_http_method_to_uppercase() { + assert_eq!("get /v1/x".parse::<Endpoint>().unwrap().method(), "GET"); + } + + #[test] + fn default_endpoints_cover_base_ops_but_not_optional_ones() { Review Comment: I'm not sure tests like this are really helpful, we're basically just asserting what's in the DEFAULT_ENDPOINTS list. ########## crates/catalog/rest/src/endpoint.rs: ########## @@ -0,0 +1,245 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Server capability negotiation via the `endpoints` field of `GET /v1/config`. +//! +//! A REST server may advertise the set of routes it supports in the `endpoints` +//! field of its configuration response, letting clients negotiate optional +//! capabilities instead of assuming every server implements every operation. +//! Each entry is a `"{method} {path}"` string, for example +//! `"POST /v1/{prefix}/namespaces/{namespace}/tables"`; parse one through +//! [`Endpoint`]'s [`FromStr`] implementation. +//! +//! Use [`RestCatalog::supports_endpoint`](crate::RestCatalog::supports_endpoint) +//! to check whether the connected server advertised a given [`Endpoint`]. + +use std::collections::HashSet; +use std::fmt::{self, Display, Formatter}; +use std::str::FromStr; +use std::sync::LazyLock; + +use iceberg::{Error, ErrorKind}; +use reqwest::Method; +use serde::de::{Error as DeError, Visitor}; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; + +/// A single route a REST server advertises support for, parsed from the +/// `endpoints` field of `GET /v1/config`. +/// +/// The wire form is `"{method} {path}"` — an HTTP method and a path template +/// separated by a single space, e.g. +/// `"POST /v1/{prefix}/namespaces/{namespace}/tables"`. Parse one with +/// [`str::parse`]: the method is validated and normalized, and the path is the +/// template the server advertises (with `{prefix}`, `{namespace}`, … +/// placeholders), compared verbatim. +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub struct Endpoint { + method: Method, + path: String, +} + +impl Endpoint { + /// Builds an endpoint from a known-valid method and path template. + /// + /// Intended for internal constants and tests; untrusted input (such as a + /// server's config response) is parsed through [`FromStr`], which validates + /// it. + pub(crate) fn new(method: Method, path: impl Into<String>) -> Self { + Self { + method, + path: path.into(), + } + } + + /// The HTTP method, e.g. `GET` or `POST`. + pub fn method(&self) -> &str { + self.method.as_str() + } + + /// The path template, e.g. `/v1/{prefix}/namespaces`. + pub fn path(&self) -> &str { + &self.path + } +} + +impl FromStr for Endpoint { + type Err = Error; + + fn from_str(s: &str) -> std::result::Result<Self, Self::Err> { + // The wire form is exactly `"<method> <path>"` separated by a single + // space; paths never contain spaces, so require exactly two non-empty + // parts and a valid HTTP method. + let mut parts = s.split(' '); + match (parts.next(), parts.next(), parts.next()) { + (Some(method), Some(path), None) if !method.is_empty() && !path.is_empty() => { + let method = Method::from_str(&method.to_ascii_uppercase()).map_err(|_| { + Error::new( + ErrorKind::DataInvalid, + format!("invalid HTTP method in endpoint: {s:?}"), + ) + })?; + Ok(Self { + method, + path: path.to_string(), + }) + } + _ => Err(Error::new( + ErrorKind::DataInvalid, + format!( + r#"invalid endpoint {s:?}: expected "<method> <path>" separated by a single space"# + ), + )), + } + } +} + +impl Display for Endpoint { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{} {}", self.method, self.path) + } +} + +impl Serialize for Endpoint { + fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error> + where S: Serializer { + serializer.collect_str(self) + } +} + +impl<'de> Deserialize<'de> for Endpoint { + fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error> + where D: Deserializer<'de> { + struct EndpointVisitor; + + impl Visitor<'_> for EndpointVisitor { + type Value = Endpoint; + + fn expecting(&self, f: &mut Formatter<'_>) -> fmt::Result { + f.write_str(r#"an endpoint string of the form "<method> <path>""#) + } + + fn visit_str<E>(self, v: &str) -> std::result::Result<Endpoint, E> + where E: DeError { + Endpoint::from_str(v).map_err(E::custom) + } + } + + deserializer.deserialize_str(EndpointVisitor) + } +} + +/// The standard v1 endpoints assumed to be supported when a server does not +/// advertise an `endpoints` list in its `GET /v1/config` response. +/// +/// A server that omits the field (for example one that predates it) is treated +/// as supporting this base set of namespace and table operations; a server that +/// sends an explicit list — even an empty one — is taken at its word. +pub(crate) static DEFAULT_ENDPOINTS: LazyLock<HashSet<Endpoint>> = LazyLock::new(|| { + [ + (Method::GET, "/v1/{prefix}/namespaces"), + (Method::POST, "/v1/{prefix}/namespaces"), + (Method::GET, "/v1/{prefix}/namespaces/{namespace}"), + (Method::DELETE, "/v1/{prefix}/namespaces/{namespace}"), + ( + Method::POST, + "/v1/{prefix}/namespaces/{namespace}/properties", + ), + (Method::GET, "/v1/{prefix}/namespaces/{namespace}/tables"), + (Method::POST, "/v1/{prefix}/namespaces/{namespace}/tables"), + ( + Method::GET, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}", + ), + ( + Method::POST, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}", + ), + ( + Method::DELETE, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}", + ), + (Method::POST, "/v1/{prefix}/tables/rename"), + (Method::POST, "/v1/{prefix}/namespaces/{namespace}/register"), + ( + Method::POST, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics", + ), + (Method::POST, "/v1/{prefix}/transactions/commit"), + ] + .into_iter() + .map(|(method, path)| Endpoint::new(method, path)) + .collect() +}); + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_and_round_trips() { + let ep: Endpoint = "POST /v1/{prefix}/namespaces/{namespace}/tables" + .parse() + .unwrap(); + assert_eq!(ep.method(), "POST"); + assert_eq!(ep.path(), "/v1/{prefix}/namespaces/{namespace}/tables"); + + let json = serde_json::to_string(&ep).unwrap(); + assert_eq!(json, r#""POST /v1/{prefix}/namespaces/{namespace}/tables""#); + assert_eq!(serde_json::from_str::<Endpoint>(&json).unwrap(), ep); + } + + #[test] + fn rejects_malformed_endpoints() { + // Must be exactly "<method> <path>" with a single separating space. + for invalid in [ + "GET", // no space + "GET /v1", // two spaces + " GET /v1", // leading space (empty method) + "GET ", // trailing space (empty path) + " /v1", // empty method + "", // empty Review Comment: Minor: I'd leave out the inline comments, it's apparent from the strings themselves why these are malformed. ########## crates/catalog/rest/src/types.rs: ########## @@ -25,10 +25,18 @@ use iceberg::{ }; use serde_derive::{Deserialize, Serialize}; +use crate::endpoint::Endpoint; + #[derive(Clone, Debug, Serialize, Deserialize)] pub(super) struct CatalogConfig { pub(super) overrides: HashMap<String, String>, pub(super) defaults: HashMap<String, String>, + /// Routes the server advertises support for, for capability negotiation + /// (the `endpoints` field of `GET /v1/config`). `None` when the field is + /// absent (e.g. an older server); `Some` — including an empty list — when + /// the server sends it explicitly. + #[serde(default)] Review Comment: Minor: Not a rust expert but my understanding is this serde(default) is redundant ########## crates/catalog/rest/src/catalog.rs: ########## @@ -347,6 +348,10 @@ struct RestContext { /// /// It's could be different from the user config. config: RestCatalogConfig, + /// Endpoints the server advertised in its `GET /v1/config` response, used + /// for capability negotiation. Empty when the server does not advertise an Review Comment: Is the "Empty when the server does not advertise an endpoints list" comment accurate? We set it to the default endpoints list no? ########## crates/catalog/rest/src/catalog.rs: ########## @@ -412,14 +417,41 @@ impl RestCatalog { .get_or_try_init(|| async { let client = HttpClient::new(&self.user_config)?; let catalog_config = RestCatalog::load_config(&client, &self.user_config).await?; + // The `endpoints` field is optional. An absent field (`None`) + // means an older server, so fall back to the standard base set; + // an explicit list — even an empty one — is taken verbatim. + let endpoints = match &catalog_config.endpoints { + Some(advertised) => advertised.iter().cloned().collect(), + None => crate::endpoint::DEFAULT_ENDPOINTS.clone(), + }; Review Comment: In case the server returns an explicit empty list , I think the right thing to do is still just use the default endpoints, those were established to be a minimum to be supported by a server so maybe smth like: ########## crates/catalog/rest/src/endpoint.rs: ########## @@ -0,0 +1,245 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Server capability negotiation via the `endpoints` field of `GET /v1/config`. +//! +//! A REST server may advertise the set of routes it supports in the `endpoints` +//! field of its configuration response, letting clients negotiate optional +//! capabilities instead of assuming every server implements every operation. +//! Each entry is a `"{method} {path}"` string, for example +//! `"POST /v1/{prefix}/namespaces/{namespace}/tables"`; parse one through +//! [`Endpoint`]'s [`FromStr`] implementation. +//! +//! Use [`RestCatalog::supports_endpoint`](crate::RestCatalog::supports_endpoint) +//! to check whether the connected server advertised a given [`Endpoint`]. + +use std::collections::HashSet; +use std::fmt::{self, Display, Formatter}; +use std::str::FromStr; +use std::sync::LazyLock; + +use iceberg::{Error, ErrorKind}; +use reqwest::Method; +use serde::de::{Error as DeError, Visitor}; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; + +/// A single route a REST server advertises support for, parsed from the +/// `endpoints` field of `GET /v1/config`. +/// +/// The wire form is `"{method} {path}"` — an HTTP method and a path template +/// separated by a single space, e.g. +/// `"POST /v1/{prefix}/namespaces/{namespace}/tables"`. Parse one with +/// [`str::parse`]: the method is validated and normalized, and the path is the +/// template the server advertises (with `{prefix}`, `{namespace}`, … +/// placeholders), compared verbatim. +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub struct Endpoint { + method: Method, + path: String, +} + +impl Endpoint { + /// Builds an endpoint from a known-valid method and path template. + /// + /// Intended for internal constants and tests; untrusted input (such as a + /// server's config response) is parsed through [`FromStr`], which validates + /// it. + pub(crate) fn new(method: Method, path: impl Into<String>) -> Self { + Self { + method, + path: path.into(), + } + } + + /// The HTTP method, e.g. `GET` or `POST`. + pub fn method(&self) -> &str { + self.method.as_str() + } + + /// The path template, e.g. `/v1/{prefix}/namespaces`. + pub fn path(&self) -> &str { + &self.path + } +} + +impl FromStr for Endpoint { + type Err = Error; + + fn from_str(s: &str) -> std::result::Result<Self, Self::Err> { + // The wire form is exactly `"<method> <path>"` separated by a single + // space; paths never contain spaces, so require exactly two non-empty + // parts and a valid HTTP method. + let mut parts = s.split(' '); + match (parts.next(), parts.next(), parts.next()) { + (Some(method), Some(path), None) if !method.is_empty() && !path.is_empty() => { + let method = Method::from_str(&method.to_ascii_uppercase()).map_err(|_| { + Error::new( + ErrorKind::DataInvalid, + format!("invalid HTTP method in endpoint: {s:?}"), + ) + })?; + Ok(Self { + method, + path: path.to_string(), + }) + } + _ => Err(Error::new( + ErrorKind::DataInvalid, + format!( + r#"invalid endpoint {s:?}: expected "<method> <path>" separated by a single space"# + ), + )), + } + } +} + +impl Display for Endpoint { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{} {}", self.method, self.path) + } +} + +impl Serialize for Endpoint { + fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error> + where S: Serializer { + serializer.collect_str(self) + } +} + +impl<'de> Deserialize<'de> for Endpoint { + fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error> + where D: Deserializer<'de> { + struct EndpointVisitor; + + impl Visitor<'_> for EndpointVisitor { + type Value = Endpoint; + + fn expecting(&self, f: &mut Formatter<'_>) -> fmt::Result { + f.write_str(r#"an endpoint string of the form "<method> <path>""#) + } + + fn visit_str<E>(self, v: &str) -> std::result::Result<Endpoint, E> + where E: DeError { + Endpoint::from_str(v).map_err(E::custom) + } + } + + deserializer.deserialize_str(EndpointVisitor) + } +} + +/// The standard v1 endpoints assumed to be supported when a server does not +/// advertise an `endpoints` list in its `GET /v1/config` response. +/// +/// A server that omits the field (for example one that predates it) is treated +/// as supporting this base set of namespace and table operations; a server that +/// sends an explicit list — even an empty one — is taken at its word. +pub(crate) static DEFAULT_ENDPOINTS: LazyLock<HashSet<Endpoint>> = LazyLock::new(|| { + [ + (Method::GET, "/v1/{prefix}/namespaces"), + (Method::POST, "/v1/{prefix}/namespaces"), + (Method::GET, "/v1/{prefix}/namespaces/{namespace}"), + (Method::DELETE, "/v1/{prefix}/namespaces/{namespace}"), + ( + Method::POST, + "/v1/{prefix}/namespaces/{namespace}/properties", + ), + (Method::GET, "/v1/{prefix}/namespaces/{namespace}/tables"), + (Method::POST, "/v1/{prefix}/namespaces/{namespace}/tables"), + ( + Method::GET, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}", + ), + ( + Method::POST, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}", + ), + ( + Method::DELETE, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}", + ), + (Method::POST, "/v1/{prefix}/tables/rename"), + (Method::POST, "/v1/{prefix}/namespaces/{namespace}/register"), + ( + Method::POST, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics", + ), + (Method::POST, "/v1/{prefix}/transactions/commit"), + ] + .into_iter() + .map(|(method, path)| Endpoint::new(method, path)) + .collect() +}); + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_and_round_trips() { + let ep: Endpoint = "POST /v1/{prefix}/namespaces/{namespace}/tables" + .parse() + .unwrap(); + assert_eq!(ep.method(), "POST"); + assert_eq!(ep.path(), "/v1/{prefix}/namespaces/{namespace}/tables"); + + let json = serde_json::to_string(&ep).unwrap(); + assert_eq!(json, r#""POST /v1/{prefix}/namespaces/{namespace}/tables""#); + assert_eq!(serde_json::from_str::<Endpoint>(&json).unwrap(), ep); + } + + #[test] + fn rejects_malformed_endpoints() { + // Must be exactly "<method> <path>" with a single separating space. + for invalid in [ + "GET", // no space + "GET /v1", // two spaces + " GET /v1", // leading space (empty method) + "GET ", // trailing space (empty path) + " /v1", // empty method + "", // empty + ] { + assert!( + invalid.parse::<Endpoint>().is_err(), + "expected {invalid:?} to be rejected" + ); + } + } + + #[test] + fn normalizes_http_method_to_uppercase() { + assert_eq!("get /v1/x".parse::<Endpoint>().unwrap().method(), "GET"); + } + + #[test] + fn default_endpoints_cover_base_ops_but_not_optional_ones() { Review Comment: Yeah `test_config_without_endpoints_falls_back_to_default_set` tests the default fallback behavior how I'd expect, so I think I'd just remove this test. ########## crates/catalog/rest/src/catalog.rs: ########## @@ -412,14 +417,41 @@ impl RestCatalog { .get_or_try_init(|| async { let client = HttpClient::new(&self.user_config)?; let catalog_config = RestCatalog::load_config(&client, &self.user_config).await?; + // The `endpoints` field is optional. An absent field (`None`) + // means an older server, so fall back to the standard base set; + // an explicit list — even an empty one — is taken verbatim. + let endpoints = match &catalog_config.endpoints { + Some(advertised) => advertised.iter().cloned().collect(), + None => crate::endpoint::DEFAULT_ENDPOINTS.clone(), + }; Review Comment: ``` let endpoints = match &catalog_config.endpoints { Some(advertised) if !advertised.is_empty() => advertised.iter().cloned().collect(), _ => crate::endpoint::DEFAULT_ENDPOINTS.clone(), } ``` ########## crates/catalog/rest/src/catalog.rs: ########## @@ -412,14 +417,41 @@ impl RestCatalog { .get_or_try_init(|| async { let client = HttpClient::new(&self.user_config)?; let catalog_config = RestCatalog::load_config(&client, &self.user_config).await?; + // The `endpoints` field is optional. An absent field (`None`) + // means an older server, so fall back to the standard base set; + // an explicit list — even an empty one — is taken verbatim. + let endpoints = match &catalog_config.endpoints { + Some(advertised) => advertised.iter().cloned().collect(), + None => crate::endpoint::DEFAULT_ENDPOINTS.clone(), + }; let config = self.user_config.clone().merge_with_config(catalog_config); let client = client.update_with(&config)?; - Ok(RestContext { config, client }) + Ok(RestContext { + config, + client, + endpoints, + }) }) .await } + /// Returns whether the connected server supports `endpoint`. + /// + /// Servers advertise the routes they support in the `endpoints` field of + /// the `GET /v1/config` response. When that field is present it is used + /// verbatim (an explicit empty list therefore advertises nothing). When a Review Comment: Commented above, but I'm not sure about using verbatim the empty list case; why not just fallback to the default list because that's largely been established as the minimal support required? and even if a server happens to not support it, it'd just reject any subsequent requests. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
