CTTY commented on code in PR #2692:
URL: https://github.com/apache/iceberg-rust/pull/2692#discussion_r3532640346
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -412,14 +415,33 @@ 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?;
+ // Use the advertised endpoints as-is, falling back to
+ // `DEFAULT_ENDPOINTS` when absent or empty.
+ let endpoints = match &catalog_config.endpoints {
+ Some(advertised) if !advertised.is_empty() => {
+ advertised.iter().cloned().collect()
+ }
+ _ => 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 server supports `endpoint`, per the `endpoints` it
+ /// advertised in `GET /v1/config` (or a default base set when it
advertised
+ /// none).
+ pub async fn supports_endpoint(&self, endpoint: &Endpoint) -> Result<bool>
{
Review Comment:
Why do we need this api public?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -412,14 +415,33 @@ 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?;
+ // Use the advertised endpoints as-is, falling back to
+ // `DEFAULT_ENDPOINTS` when absent or empty.
+ let endpoints = match &catalog_config.endpoints {
+ Some(advertised) if !advertised.is_empty() => {
+ advertised.iter().cloned().collect()
+ }
+ _ => 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 server supports `endpoint`, per the `endpoints` it
+ /// advertised in `GET /v1/config` (or a default base set when it
advertised
+ /// none).
+ pub async fn supports_endpoint(&self, endpoint: &Endpoint) -> Result<bool>
{
Review Comment:
Also not sure if there is any blockers that are preventing us from enforcing
the supported endpoint? (Check supported endpoints for every catalog
operations) Just storing the endpoints while not using them seems weird.
If we do plan to enforce them, when should we do that?
##########
crates/catalog/rest/src/endpoint.rs:
##########
@@ -0,0 +1,218 @@
+// 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's
+/// `GET /v1/config` response omits the `endpoints` field or sends an empty
list
+/// (the two are treated alike). These are the minimum a server is expected to
+/// support; a server that advertises a non-empty list is taken at its word
+/// instead.
+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"),
+ ]
Review Comment:
Some comments would make this more readable like
```
(Method::GET, "/v1/{prefix}/namespaces"), // v1 list namespaces
```
We can refer to [java's
naming](https://github.com/apache/iceberg/blob/334dd95535b4170fe8b2c75d48204ac3413a5cd6/core/src/main/java/org/apache/iceberg/rest/Endpoint.java#L39-L70)
--
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]
