diff --git a/docs/src/python/python.md b/docs/src/python/python.md index e39c1da50..d249aaea8 100644 --- a/docs/src/python/python.md +++ b/docs/src/python/python.md @@ -748,10 +748,45 @@ so the fetch APIs cannot read their bytes yet. ## Exceptions +Remote HTTP responses with structured resource details raise a subclass of +`HttpError`. Older servers that omit those details raise plain `HttpError`, so +`except HttpError` remains a compatible catch-all: + +```python +from lancedb.remote.errors import ( + AlreadyExistsError, + FunctionNotFoundError, + HttpError, + NamespaceNotFoundError, + SecretNotFoundError, +) + +try: + db.describe_namespace(["analytics"]) +except (SecretNotFoundError, FunctionNotFoundError, NamespaceNotFoundError) as error: + print(error.error_code, error.resource_kind, error.resource_name) +except AlreadyExistsError as error: + print("resource already exists:", error.resource_name) +except HttpError as error: + print("request failed:", error.request_id, error.status_code) +``` + +Secret and function lifecycle APIs currently use Flight SQL and may still raise +`RuntimeError`. The typed subclasses cover HTTP failures, including dependency +errors during column refresh. + ::: lancedb.remote.errors.HttpError ::: lancedb.remote.errors.TableNotFoundError +::: lancedb.remote.errors.SecretNotFoundError + +::: lancedb.remote.errors.FunctionNotFoundError + +::: lancedb.remote.errors.NamespaceNotFoundError + +::: lancedb.remote.errors.AlreadyExistsError + ::: lancedb.exceptions.MissingValueError ::: lancedb.exceptions.MissingColumnError diff --git a/python/python/lancedb/remote/errors.py b/python/python/lancedb/remote/errors.py index 3eeb9bbd2..5401cde17 100644 --- a/python/python/lancedb/remote/errors.py +++ b/python/python/lancedb/remote/errors.py @@ -46,9 +46,93 @@ class HttpError(LanceDBClientError): status_code: int The HTTP status code of the response. May be None if the request failed before the response was received. + error_code: str + A machine-readable error code reported by the server, if available. + resource_kind: str + The kind of resource reported by the server, if available. + resource_name: str + The name of the resource reported by the server, if available. + namespace_path: tuple[str, ...] + The parent namespace path of the resource, if available. """ - pass + def __new__( + cls, + message: str, + request_id: str, + status_code: Optional[int] = None, + error_code: Optional[str] = None, + resource_kind: Optional[str] = None, + resource_name: Optional[str] = None, + namespace_path: Optional[tuple[str, ...]] = None, + ): + if cls is HttpError: + mapped_cls = _http_error_class(error_code, resource_kind) + if mapped_cls is not None: + cls = mapped_cls + return RuntimeError.__new__(cls) + + def __init__( + self, + message: str, + request_id: str, + status_code: Optional[int] = None, + error_code: Optional[str] = None, + resource_kind: Optional[str] = None, + resource_name: Optional[str] = None, + namespace_path: Optional[tuple[str, ...]] = None, + ): + super().__init__(message, request_id, status_code) + self.error_code = error_code + self.resource_kind = resource_kind + self.resource_name = resource_name + self.namespace_path = ( + tuple(namespace_path) if namespace_path is not None else None + ) + + def __reduce__(self) -> tuple[type, tuple]: + return ( + self.__class__, + ( + str(self), + self.request_id, + self.status_code, + self.error_code, + self.resource_kind, + self.resource_name, + self.namespace_path, + ), + ) + + +def _http_error_class( + error_code: Optional[str], resource_kind: Optional[str] +) -> Optional[type[HttpError]]: + if error_code == "already_exists": + return AlreadyExistsError + if error_code != "not_found" or resource_kind is None: + return None + return { + "secret": SecretNotFoundError, + "function": FunctionNotFoundError, + "namespace": NamespaceNotFoundError, + }.get(resource_kind) + + +class SecretNotFoundError(HttpError): + """A remote secret was not found.""" + + +class FunctionNotFoundError(HttpError): + """A remote function was not found.""" + + +class NamespaceNotFoundError(HttpError): + """A remote namespace was not found.""" + + +class AlreadyExistsError(HttpError): + """A remote resource already exists.""" class TableNotFoundError(HttpError, ValueError, _NamespaceTableNotFoundError): diff --git a/python/python/tests/test_errors.py b/python/python/tests/test_errors.py index 35c831777..6878a4aea 100644 --- a/python/python/tests/test_errors.py +++ b/python/python/tests/test_errors.py @@ -9,9 +9,13 @@ from lance_namespace.errors import TableNotFoundError as NamespaceTableNotFoundE from lancedb.db import DBConnection from lancedb.remote.errors import ( + AlreadyExistsError, + FunctionNotFoundError, HttpError, LanceDBClientError, + NamespaceNotFoundError, RetryError, + SecretNotFoundError, TableNotFoundError, ) from lancedb.table import Table @@ -59,6 +63,97 @@ def test_pickle_http_error(): assert restored.status_code == 404 +@pytest.mark.parametrize( + "error_code,resource_kind,status_code,expected_type", + [ + ("not_found", "secret", 404, SecretNotFoundError), + ("not_found", "function", 404, FunctionNotFoundError), + ("not_found", "namespace", 404, NamespaceNotFoundError), + ("already_exists", "secret", 409, AlreadyExistsError), + ], +) +def test_http_error_structured_resource_error( + error_code, resource_kind, status_code, expected_type +): + err = HttpError( + "resource request failed", + "req-structured", + status_code, + error_code, + resource_kind, + "resource-name", + ["parent", "child"], + ) + + assert type(err) is expected_type + assert isinstance(err, HttpError) + assert err.error_code == error_code + assert err.resource_kind == resource_kind + assert err.resource_name == "resource-name" + assert err.namespace_path == ("parent", "child") + assert err.request_id == "req-structured" + assert err.status_code == status_code + + +@pytest.mark.parametrize( + "error_code,resource_kind", + [ + (None, None), + ("not_found", None), + ("not_found", "unknown"), + ("invalid", "secret"), + ], +) +def test_http_error_without_known_structured_fields_stays_generic( + error_code, resource_kind +): + err = HttpError( + "not found", + "req-generic", + 404, + error_code, + resource_kind, + "resource-name", + [], + ) + + assert type(err) is HttpError + assert err.error_code == error_code + assert err.resource_kind == resource_kind + + +@pytest.mark.parametrize( + "exception_type,error_code,resource_kind", + [ + (SecretNotFoundError, "not_found", "secret"), + (FunctionNotFoundError, "not_found", "function"), + (NamespaceNotFoundError, "not_found", "namespace"), + (AlreadyExistsError, "already_exists", "namespace"), + ], +) +def test_pickle_structured_http_error(exception_type, error_code, resource_kind): + err = HttpError( + "resource request failed", + "req-pickle", + 409 if error_code == "already_exists" else 404, + error_code, + resource_kind, + "resource-name", + ["parent"], + ) + assert type(err) is exception_type + + restored = pickle.loads(pickle.dumps(err)) + + assert type(restored) is exception_type + assert restored.error_code == error_code + assert restored.resource_kind == resource_kind + assert restored.resource_name == "resource-name" + assert restored.namespace_path == ("parent",) + assert restored.request_id == "req-pickle" + assert restored.status_code == err.status_code + + def test_pickle_table_not_found_error(): err = TableNotFoundError("Table 'missing' was not found", "req-missing", 404) restored = pickle.loads(pickle.dumps(err)) diff --git a/python/python/tests/test_remote_db.py b/python/python/tests/test_remote_db.py index d1529c9d8..ef48d8eff 100644 --- a/python/python/tests/test_remote_db.py +++ b/python/python/tests/test_remote_db.py @@ -42,7 +42,12 @@ from lancedb.query import ( MatchQuery, ) from lancedb.remote import ClientConfig -from lancedb.remote.errors import HttpError, RetryError +from lancedb.remote.errors import ( + AlreadyExistsError, + HttpError, + NamespaceNotFoundError, + RetryError, +) import pytest import pyarrow as pa from utils import start_server_thread @@ -258,6 +263,111 @@ async def test_async_remote_exists(method, identifier, status, existence_handler assert exc_info.value.status_code == status +def test_remote_describe_namespace_typed_not_found(): + request_ids = [] + + def handler(request): + assert request.command == "POST" + assert request.path == "/v1/namespace/parent$child/describe" + request_ids.append(request.headers["x-request-id"]) + request.send_response(404) + request.send_header("Content-Type", "application/json") + request.end_headers() + request.wfile.write( + json.dumps( + { + "code": 5, + "error": "Namespace not found", + "error_code": "not_found", + "resource": { + "kind": "namespace", + "name": "child", + "namespace_path": ["parent"], + }, + } + ).encode() + ) + + with mock_lancedb_connection(handler) as db: + with pytest.raises(NamespaceNotFoundError) as exc_info: + db.describe_namespace(["parent", "child"]) + + error = exc_info.value + assert isinstance(error, HttpError) + assert error.error_code == "not_found" + assert error.resource_kind == "namespace" + assert error.resource_name == "child" + assert error.namespace_path == ("parent",) + assert error.status_code == 404 + assert error.request_id == request_ids[-1] + + +def test_remote_create_namespace_typed_already_exists(): + requests = [] + + def handler(request): + requests.append(request.path) + assert request.command == "POST" + assert request.path == "/v1/namespace/parent$child/create" + request.send_response(409) + request.send_header("Content-Type", "application/json") + request.end_headers() + request.wfile.write( + json.dumps( + { + "code": 6, + "error": "Namespace already exists", + "error_code": "already_exists", + "resource": { + "kind": "namespace", + "name": "child", + "namespace_path": ["parent"], + }, + } + ).encode() + ) + + with mock_lancedb_connection(handler) as db: + with pytest.raises(AlreadyExistsError) as exc_info: + db.create_namespace(["parent", "child"]) + + error = exc_info.value + assert error.error_code == "already_exists" + assert error.resource_kind == "namespace" + assert error.resource_name == "child" + assert error.namespace_path == ("parent",) + assert error.status_code == 409 + assert len(requests) == 1 + + +def test_remote_legacy_namespace_error_stays_generic_http_error(): + def handler(request): + assert request.path == "/v1/namespace/parent$child/describe" + request.send_response(404) + request.send_header("Content-Type", "application/json") + request.end_headers() + request.wfile.write( + json.dumps( + { + "code": 5, + "error": "Namespace not found", + } + ).encode() + ) + + with mock_lancedb_connection(handler) as db: + with pytest.raises(HttpError) as exc_info: + db.describe_namespace(["parent", "child"]) + + error = exc_info.value + assert type(error) is HttpError + assert error.error_code is None + assert error.resource_kind is None + assert error.resource_name is None + assert error.namespace_path is None + assert error.status_code == 404 + + @pytest.mark.parametrize("table_id", [[], ["invalid/name"]]) def test_remote_table_exists_rejects_invalid_identifier(table_id): requests = [] diff --git a/python/src/error.rs b/python/src/error.rs index ba164b5db..2b7179365 100644 --- a/python/src/error.rs +++ b/python/src/error.rs @@ -2,13 +2,13 @@ // SPDX-FileCopyrightText: Copyright The LanceDB Authors use pyo3::{ - PyErr, PyResult, Python, + Bound, PyAny, PyErr, PyResult, Python, exceptions::{PyIOError, PyNotImplementedError, PyOSError, PyRuntimeError, PyValueError}, intern, types::{PyAnyMethods, PyNone}, }; -use lancedb::error::Error as LanceError; +use lancedb::error::{Error as LanceError, HttpErrorDetails}; pub trait PythonErrorExt { /// Convert to a python error based on the Lance error type @@ -65,14 +65,13 @@ impl PythonErrorExt for std::result::Result { status_code, } => Python::attach(|py| { let message = err.to_string(); - let http_err_cls = py - .import(intern!(py, "lancedb.remote.errors"))? - .getattr(intern!(py, "HttpError"))?; - let err = http_err_cls.call1(( + let err = new_py_http_error( + py, message, request_id, status_code.map(|s| s.as_u16()), - ))?; + err.http_error_details(), + )?; if let Some(cause) = source.source() { // The HTTP error already includes the first cause. But @@ -163,13 +162,15 @@ impl PythonErrorExt for std::result::Result { fn http_from_rust_error( py: Python<'_>, - err: &dyn std::error::Error, + err: &(dyn std::error::Error + 'static), request_id: &str, status_code: Option, ) -> PyResult { let message = err.to_string(); - let http_err_cls = py.import("lancedb.remote.errors")?.getattr("HttpError")?; - let py_err = http_err_cls.call1((message, request_id, status_code))?; + let details = err + .downcast_ref::() + .and_then(LanceError::http_error_details); + let py_err = new_py_http_error(py, message, request_id, status_code, details)?; // Reset the traceback since it doesn't provide additional information. let py_err = py_err.call_method1(intern!(py, "with_traceback"), (PyNone::get(py),))?; @@ -181,3 +182,28 @@ fn http_from_rust_error( Ok(PyErr::from_value(py_err)) } + +fn new_py_http_error<'py>( + py: Python<'py>, + message: String, + request_id: &str, + status_code: Option, + details: Option<&HttpErrorDetails>, +) -> PyResult> { + let error_code = details.map(|details| details.error_code.as_str()); + let resource_kind = details.map(|details| details.resource_kind.as_str()); + let resource_name = details.map(|details| details.resource_name.as_str()); + let namespace_path = details.map(|details| details.namespace_path.clone()); + + py.import("lancedb.remote.errors")? + .getattr("HttpError")? + .call1(( + message, + request_id, + status_code, + error_code, + resource_kind, + resource_name, + namespace_path, + )) +} diff --git a/rust/lancedb/src/error.rs b/rust/lancedb/src/error.rs index 1b1f5b0a5..ddd04fa45 100644 --- a/rust/lancedb/src/error.rs +++ b/rust/lancedb/src/error.rs @@ -6,10 +6,78 @@ use std::sync::{Arc, PoisonError}; use arrow_schema::ArrowError; use datafusion_common::DataFusionError; +#[cfg(feature = "remote")] +use serde::Deserialize; use snafu::Snafu; pub(crate) type BoxError = Box; +/// Stable resource details carried by a structured remote HTTP error. +#[cfg(feature = "remote")] +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub struct HttpErrorDetails { + /// Machine-readable error code from the server. + pub error_code: String, + /// The kind of resource named by the error. + pub resource_kind: String, + /// The leaf name of the resource. + pub resource_name: String, + /// The parent namespace path of the resource. + pub namespace_path: Vec, +} + +#[cfg(feature = "remote")] +#[derive(Debug)] +pub(crate) struct HttpErrorSource { + message: String, + details: Option, +} + +#[cfg(feature = "remote")] +impl HttpErrorSource { + pub(crate) fn new(message: String, response_body: Option<&str>) -> Self { + let details = response_body.and_then(parse_http_error_details); + Self { message, details } + } +} + +#[cfg(feature = "remote")] +impl Display for HttpErrorSource { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + f.write_str(&self.message) + } +} + +#[cfg(feature = "remote")] +impl std::error::Error for HttpErrorSource {} + +#[cfg(feature = "remote")] +#[derive(Deserialize)] +struct HttpResponseBody { + error_code: String, + resource: HttpResponseResource, +} + +#[cfg(feature = "remote")] +#[derive(Deserialize)] +struct HttpResponseResource { + kind: String, + name: String, + namespace_path: Vec, +} + +#[cfg(feature = "remote")] +fn parse_http_error_details(response_body: &str) -> Option { + let body: HttpResponseBody = serde_json::from_str(response_body).ok()?; + Some(HttpErrorDetails { + error_code: body.error_code, + resource_kind: body.resource.kind, + resource_name: body.resource.name, + namespace_path: body.resource.namespace_path, + }) +} + // reqwest's Display omits the timeout cause. Add guidance at display time so // callers still receive the original typed source and its full error chain. #[cfg(feature = "remote")] @@ -244,6 +312,23 @@ fn has_unsupported_local_filesystem_source(error: &(dyn std::error::Error + 'sta } impl Error { + /// Structured resource details reported by a remote HTTP error. + /// + /// Older servers do not include these fields, in which case this returns + /// `None` and callers should treat the error as a generic HTTP failure. + #[cfg(feature = "remote")] + pub fn http_error_details(&self) -> Option<&HttpErrorDetails> { + match self { + Self::Http { source, .. } => source + .downcast_ref::() + .and_then(|source| source.details.as_ref()), + Self::Retry { source, .. } => source + .downcast_ref::() + .and_then(Self::http_error_details), + _ => None, + } + } + fn from_box_error(mut source: Box) -> Self { source = match source.downcast::() { Ok(e) => match *e { diff --git a/rust/lancedb/src/remote/client.rs b/rust/lancedb/src/remote/client.rs index cec6696c9..38104645d 100644 --- a/rust/lancedb/src/remote/client.rs +++ b/rust/lancedb/src/remote/client.rs @@ -10,7 +10,7 @@ use reqwest::{ use std::{collections::HashMap, future::Future, str::FromStr, sync::Arc, time::Duration}; use tokio::sync::Semaphore; -use crate::error::{Error, Result}; +use crate::error::{Error, HttpErrorSource, Result}; use crate::remote::db::RemoteOptions; use crate::remote::retry::{ResolvedRetryConfig, RetryCounter}; use crate::utils::validate_database_name; @@ -957,6 +957,11 @@ impl RestfulLanceDbClient { .check_response(&retry_counter.request_id, response) .await .unwrap_err(); + // A structured resource error is a domain result, not a + // transient failure. Retrying it would hide the typed error. + if source.http_error_details().is_some() { + return Err(source); + } retry_counter.increment_request_failures(source)?; } Err(err) if err.is_connect() => { @@ -1033,13 +1038,15 @@ impl RestfulLanceDbClient { Ok(response) } else { let response_text = response.text().await.ok(); - let message = if let Some(response_text) = response_text { - format!("{}: {}", status, response_text) - } else { - status.to_string() + let source = match response_text { + Some(response_text) => HttpErrorSource::new( + format!("{}: {}", status, response_text), + Some(&response_text), + ), + None => HttpErrorSource::new(status.to_string(), None), }; Err(Error::Http { - source: message.into(), + source: Box::new(source), request_id: request_id.into(), status_code: Some(status), }) @@ -1215,6 +1222,120 @@ pub mod test_utils { #[cfg(test)] mod tests { + #[tokio::test] + async fn check_response_extracts_structured_resource_error() { + use crate::remote::client::test_utils::client_with_handler; + + let client = + client_with_handler(|_| http::Response::builder().status(404).body("").unwrap()); + let cases = [ + (404, "not_found", "secret", "openai"), + (404, "not_found", "function", "embed"), + (404, "not_found", "namespace", "child"), + (409, "already_exists", "namespace", "analytics"), + ]; + + for (status, error_code, resource_kind, resource_name) in cases { + let body = serde_json::json!({ + "code": if error_code == "not_found" { 5 } else { 6 }, + "error": format!("{resource_kind} {resource_name} error"), + "error_code": error_code, + "resource": { + "kind": resource_kind, + "name": resource_name, + "namespace_path": ["analytics"], + }, + }) + .to_string(); + let response = http::Response::builder() + .status(status) + .body(body.clone()) + .unwrap() + .into(); + + let error = client + .check_response("request-1", response) + .await + .unwrap_err(); + let details = error + .http_error_details() + .expect("structured HTTP error details"); + assert_eq!(details.error_code, error_code); + assert_eq!(details.resource_kind, resource_kind); + assert_eq!(details.resource_name, resource_name); + assert_eq!(details.namespace_path, ["analytics"]); + assert!(error.to_string().contains(&body)); + } + } + + #[tokio::test] + async fn check_response_keeps_legacy_body_as_generic_http_error() { + use crate::remote::client::test_utils::client_with_handler; + + let client = + client_with_handler(|_| http::Response::builder().status(404).body("").unwrap()); + let body = serde_json::json!({ + "code": 4, + "error": "Not found: Secret \"openai\" does not exist", + }) + .to_string(); + let response = http::Response::builder() + .status(404) + .body(body.clone()) + .unwrap() + .into(); + + let error = client + .check_response("request-1", response) + .await + .unwrap_err(); + assert!(error.http_error_details().is_none()); + assert!(error.to_string().contains(&body)); + } + + #[tokio::test] + async fn send_with_retry_does_not_retry_structured_resource_error() { + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + + use crate::remote::client::test_utils::client_with_handler; + + let attempts = Arc::new(AtomicUsize::new(0)); + let handler_attempts = Arc::clone(&attempts); + let client = client_with_handler(move |_| { + handler_attempts.fetch_add(1, Ordering::Relaxed); + http::Response::builder() + .status(409) + .body( + serde_json::json!({ + "code": 6, + "error": "already exists", + "error_code": "already_exists", + "resource": { + "kind": "namespace", + "name": "analytics", + "namespace_path": [], + }, + }) + .to_string(), + ) + .unwrap() + }); + + let request = client.client.post("http://localhost/"); + let error = client + .send_with_retry(request, None, true) + .await + .unwrap_err(); + + assert_eq!(attempts.load(Ordering::Relaxed), 1); + let details = error + .http_error_details() + .expect("structured HTTP error details"); + assert_eq!(details.error_code, "already_exists"); + assert_eq!(details.resource_kind, "namespace"); + } + #[tokio::test] async fn test_http_timeout_error_is_actionable() { use super::*;