From 77e2dab016344cdab8df26b1e0636738f8f03269 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Thu, 6 Aug 2026 12:19:18 +0200 Subject: [PATCH 01/22] feat(server): Add resumable upload API --- objectstore-server/docs/architecture.md | 53 +- objectstore-server/src/auth/service.rs | 60 +++ objectstore-server/src/endpoints/common.rs | 14 + objectstore-server/src/endpoints/mod.rs | 109 ++++ objectstore-server/src/endpoints/objects.rs | 51 +- objectstore-server/src/endpoints/resumable.rs | 491 ++++++++++++++++++ objectstore-server/tests/resumable.rs | 312 +++++++++++ objectstore-service/docs/architecture.md | 24 + objectstore-service/src/backend/common.rs | 109 ++++ objectstore-service/src/backend/counting.rs | 49 ++ objectstore-service/src/backend/testing.rs | 93 ++++ objectstore-service/src/backend/tiered.rs | 10 + objectstore-service/src/error.rs | 26 + objectstore-service/src/lib.rs | 1 + objectstore-service/src/resumable.rs | 46 ++ objectstore-service/src/service.rs | 224 +++++++- objectstore-types/src/lib.rs | 1 + objectstore-types/src/resumable.rs | 231 ++++++++ 18 files changed, 1856 insertions(+), 48 deletions(-) create mode 100644 objectstore-server/src/endpoints/resumable.rs create mode 100644 objectstore-server/tests/resumable.rs create mode 100644 objectstore-service/src/resumable.rs create mode 100644 objectstore-types/src/resumable.rs diff --git a/objectstore-server/docs/architecture.md b/objectstore-server/docs/architecture.md index e665fc96..6dfc4fc0 100644 --- a/objectstore-server/docs/architecture.md +++ b/objectstore-server/docs/architecture.md @@ -7,49 +7,16 @@ core storage operations. ## Endpoints -All object operations live under the `/v1/` prefix: - -| Method | Path | Description | -|----------|-------------------------------------------|------------------------------| -| `POST` | `/v1/objects/{usecase}/{scopes}/` | Insert with server-generated key | -| `GET` | `/v1/objects/{usecase}/{scopes}/{*key}` | Retrieve object | -| `HEAD` | `/v1/objects/{usecase}/{scopes}/{*key}` | Retrieve metadata only | -| `PUT` | `/v1/objects/{usecase}/{scopes}/{*key}` | Insert or overwrite with key | -| `DELETE` | `/v1/objects/{usecase}/{scopes}/{*key}` | Delete object | -| `POST` | `/v1/objects:batch/{usecase}/{scopes}/` | Batch operations (multipart) | - -### Multipart Upload Endpoints - -| Method | Path | Description | -|-----------|--------------------------------------------------------------|--------------------------------------| -| `POST` | `/v1/objects:multipart/{usecase}/{scopes}/` | Initiate upload (server-generated key) | -| `PUT` | `/v1/objects:multipart/{usecase}/{scopes}/{*key}` | Initiate upload (user-provided key) | -| `PUT` | `/v1/objects:multipart:parts/{usecase}/{scopes}/{*key}` | Upload a part (`uploadId`, `partNumber` query params) | -| `GET` | `/v1/objects:multipart:parts/{usecase}/{scopes}/{*key}` | List uploaded parts (`uploadId` query param) | -| `POST` | `/v1/objects:multipart:complete/{usecase}/{scopes}/{*key}` | Complete upload (`uploadId` query param) | -| `DELETE` | `/v1/objects:multipart/{usecase}/{scopes}/{*key}` | Abort upload (`uploadId` query param) | - -The initiate POST endpoint accepts both trailing-slash and non-trailing-slash forms. - -The complete endpoint returns `200 OK` immediately, with a streaming body that -will contain the error (if any) as JSON. Whitespace is sent in the streaming body -to keep the connection open. -Clients must parse the body to determine the actual outcome, and not rely on the -status code. - -Scopes are encoded in the URL path using Matrix URI syntax: -`org=123;project=456`. An underscore (`_`) represents empty scopes. - -### Internal Endpoints - -Internal endpoints are exempt from authentication, rate limiting, and the web -concurrency limit so they remain available when the server is under load. - -| Method | Path | Description | -|--------|------|-------------| -| `GET` | `/health` | Liveness probe (always returns 200) | -| `GET` | `/ready` | Readiness probe (returns 503 when `/tmp/objectstore.down` exists, enabling graceful drain) | -| `GET` | `/keda` | Prometheus text-format gauges for KEDA autoscaling (see [KEDA Metrics](#keda-metrics)) | +All object operations live under the `/v1/` prefix. Objects are addressed by a +usecase, a set of scopes, and a key; scopes are encoded in the URL path using +Matrix URI syntax (`org=123;project=456`). + +Four families of routes exist: object operations, resumable uploads, multipart +uploads (being replaced by resumable uploads), and internal probes that stay +available when the server is under load. + +See the [`endpoints`] module for the routing table and the request and response +shape of every route. ## Request Flow diff --git a/objectstore-server/src/auth/service.rs b/objectstore-server/src/auth/service.rs index f4643bda..cee66915 100644 --- a/objectstore-server/src/auth/service.rs +++ b/objectstore-server/src/auth/service.rs @@ -3,6 +3,9 @@ use objectstore_service::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; +use objectstore_service::resumable::{ + CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, +}; use objectstore_service::service::{DeleteResponse, GetResponse, InsertResponse, MetadataResponse}; use objectstore_service::{ClientStream, StorageService}; @@ -186,4 +189,61 @@ impl AuthAwareService { .complete_multipart(id, upload_id, parts) .await?) } + + // --- Resumable upload operations --- + // + // Every operation requires `ObjectWrite`, including the two that do not obviously write: + // an offset query can commit an assembled object, and terminating a session discards an + // in-progress upload rather than deleting an object. So `DELETE ?session=` needs write + // permission where a plain `DELETE` on the same path needs delete permission. + + /// Auth-aware wrapper around [`StorageService::create_upload_session`]. + pub async fn create_upload_session( + &self, + id: ObjectId, + metadata: Metadata, + total_length: u64, + ) -> ApiResult { + self.check_permission(Permission::ObjectWrite, id.context())?; + Ok(self + .service + .create_upload_session(id, metadata, total_length) + .await?) + } + + /// Auth-aware wrapper around [`StorageService::put_chunk`]. + pub async fn put_chunk( + &self, + id: ObjectId, + session: SessionToken, + offset: u64, + content_length: u64, + body: ClientStream, + ) -> ApiResult { + self.check_permission(Permission::ObjectWrite, id.context())?; + Ok(self + .service + .put_chunk(id, session, offset, content_length, body) + .await?) + } + + /// Auth-aware wrapper around [`StorageService::upload_offset`]. + pub async fn upload_offset( + &self, + id: ObjectId, + session: SessionToken, + ) -> ApiResult { + self.check_permission(Permission::ObjectWrite, id.context())?; + Ok(self.service.upload_offset(id, session).await?) + } + + /// Auth-aware wrapper around [`StorageService::terminate_upload`]. + pub async fn terminate_upload( + &self, + id: ObjectId, + session: SessionToken, + ) -> ApiResult { + self.check_permission(Permission::ObjectWrite, id.context())?; + Ok(self.service.terminate_upload(id, session).await?) + } } diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 97468764..75b89cbe 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -52,6 +52,17 @@ pub struct ApiErrorResponse { } impl ApiErrorResponse { + /// Creates an error response carrying only a message, with no cause chain. + /// + /// For outcomes that are not errors in the service layer and therefore have no + /// [`Error`] to wrap, such as a denied resumable upload session. + pub fn message(detail: impl Into) -> Self { + Self { + detail: Some(detail.into()), + causes: Vec::new(), + } + } + /// Creates an error response from an error, extracting the full cause chain. pub fn from_error(error: &E) -> Self { let detail = Some(error.to_string()); @@ -96,6 +107,9 @@ impl ApiError { StatusCode::RANGE_NOT_SATISFIABLE } ApiError::Service(ServiceError::InvalidUploadId(_)) => StatusCode::BAD_REQUEST, + ApiError::Service(ServiceError::InvalidUploadRequest(_)) => StatusCode::BAD_REQUEST, + ApiError::Service(ServiceError::UploadOffsetMismatch { .. }) => StatusCode::CONFLICT, + ApiError::Service(ServiceError::UploadSessionGone) => StatusCode::GONE, ApiError::Service(ServiceError::AtCapacity) => StatusCode::TOO_MANY_REQUESTS, ApiError::Service(ServiceError::NotImplemented) => StatusCode::NOT_IMPLEMENTED, ApiError::Service(_) => StatusCode::INTERNAL_SERVER_ERROR, diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index ab460b69..da96257a 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -1,5 +1,113 @@ //! Contains all HTTP endpoint handlers. //! +//! This module documents the request and response shape of every route; see the [crate +//! documentation](crate) for the layers a request passes through before reaching a handler. +//! +//! Scopes are encoded in the URL path using Matrix URI syntax: `org=123;project=456`. An +//! underscore (`_`) represents empty scopes. +//! +//! # Object Endpoints +//! +//! All object operations live under the `/v1/` prefix: +//! +//! | Method | Path | Description | +//! |----------|-------------------------------------------|------------------------------| +//! | `POST` | `/v1/objects/{usecase}/{scopes}/` | Insert with server-generated key | +//! | `GET` | `/v1/objects/{usecase}/{scopes}/{*key}` | Retrieve object | +//! | `HEAD` | `/v1/objects/{usecase}/{scopes}/{*key}` | Retrieve metadata only | +//! | `PUT` | `/v1/objects/{usecase}/{scopes}/{*key}` | Insert or overwrite with key | +//! | `DELETE` | `/v1/objects/{usecase}/{scopes}/{*key}` | Delete object | +//! | `POST` | `/v1/objects:batch/{usecase}/{scopes}/` | Batch operations (multipart) | +//! +//! Object metadata travels in request and response headers; see +//! [`objectstore_types::metadata`] for the mapping. +//! +//! # Resumable Upload Endpoints +//! +//! A resumable upload transfers a single object across several requests. The client opens a +//! session, declaring the object's total size and metadata upfront, and then sends the payload +//! as a sequence of chunks at increasing byte offsets. If a chunk fails, the client asks the +//! server which offset it holds and continues from there, so an interrupted transfer resumes +//! where it stopped instead of starting over. The server knows the total size from the +//! session, so it recognizes the chunk carrying the last byte and commits the object itself. +//! +//! Resumable uploads use the object endpoints above, selected by a query parameter: +//! `upload_type=resumable` opens a session, and `session=` addresses it from then on. +//! The object is named by the request path as usual, and [`objectstore_types::resumable`] +//! holds the protocol types. +//! +//! | Method | Path | Description | +//! |----------|------------------------------------------------------------|----------------------------------------------| +//! | `POST` | `/v1/objects/{usecase}/{scopes}/?upload_type=resumable` | Create session (server-generated key) | +//! | `PUT` | `/v1/objects/{usecase}/{scopes}/{*key}?upload_type=resumable` | Create session (user-provided key) | +//! | `PUT` | `/v1/objects/{usecase}/{scopes}/{*key}?session=` | Upload a chunk, or query the offset | +//! | `DELETE` | `/v1/objects/{usecase}/{scopes}/{*key}?session=` | Terminate session, discarding what was sent | +//! +//! Session creation requires an `Upload-Length` header carrying the total size of the object +//! in bytes, and takes the same metadata headers as a regular upload. It answers `200 OK` +//! with `{"key", "session"}` and a `Location` header pointing at the object path with the +//! session appended. Metadata is fixed at this point and does not change afterwards. +//! +//! Chunk uploads and offset queries share one request shape, distinguished by the +//! `Upload-Offset` header: a byte offset submits the body as the chunk starting there, while +//! the `*` wildcard submits an empty body and asks which offset the server holds. Both answer +//! `204 No Content` with the authoritative `Upload-Offset` while bytes remain, and +//! `201 Created` with `{"key"}` once the object is committed. **The offset in the response +//! may be lower than the end of the chunk that was sent** — backends persist only aligned +//! prefixes and discard the remainder — so clients always continue from the returned offset. +//! +//! An offset query can commit an object that was assembled but not yet committed, so it +//! requires write permission despite being read-shaped. Termination likewise needs write +//! rather than delete permission: it releases an in-progress upload, not an object. +//! +//! | Status | Meaning | Client action | +//! |--------|---------|---------------| +//! | `400` | Malformed: unusable session, missing `Upload-Length`, or a chunk exceeding the declared length | Terminal | +//! | `409` | On creation: resumable uploads are unavailable for this object. On a chunk: offset mismatch, with the authoritative offset in `Upload-Offset` | Fall back to a regular upload, or resynchronize | +//! | `410` | The session expired or was terminated; nothing was retained | Start a new session | +//! | `501` | The configured backend does not implement resumable uploads | Fall back to a regular upload | +//! +//! Not every backend can support this. Session creation asks the backend that would store the +//! object to open one, and a backend that cannot declines, which the server reports as +//! `409 Conflict`. No backend implements resumable uploads yet, so every session creation is +//! currently denied. +//! +//! # Multipart Upload Endpoints +//! +//! Multipart uploads are being replaced by [resumable +//! uploads](#resumable-upload-endpoints) and will be removed once all consumers have +//! migrated. See [`objectstore_types::multipart`] for the protocol types. +//! +//! | Method | Path | Description | +//! |-----------|--------------------------------------------------------------|--------------------------------------| +//! | `POST` | `/v1/objects:multipart/{usecase}/{scopes}/` | Initiate upload (server-generated key) | +//! | `PUT` | `/v1/objects:multipart/{usecase}/{scopes}/{*key}` | Initiate upload (user-provided key) | +//! | `PUT` | `/v1/objects:multipart:parts/{usecase}/{scopes}/{*key}` | Upload a part (`upload_id`, `part_number` query params) | +//! | `GET` | `/v1/objects:multipart:parts/{usecase}/{scopes}/{*key}` | List uploaded parts (`upload_id` query param) | +//! | `POST` | `/v1/objects:multipart:complete/{usecase}/{scopes}/{*key}` | Complete upload (`upload_id` query param) | +//! | `DELETE` | `/v1/objects:multipart/{usecase}/{scopes}/{*key}` | Abort upload (`upload_id` query param) | +//! +//! The initiate POST endpoint accepts both trailing-slash and non-trailing-slash forms. +//! +//! The complete endpoint returns `200 OK` immediately, with a streaming body that will +//! contain the error (if any) as JSON. Whitespace is sent in the streaming body to keep the +//! connection open. Clients must parse the body to determine the actual outcome, and not rely +//! on the status code. +//! +//! # Internal Endpoints +//! +//! Internal endpoints are exempt from authentication, rate limiting, and the web concurrency +//! limit so they remain available when the server is under load. [`is_internal_route`] +//! identifies them. +//! +//! | Method | Path | Description | +//! |--------|------|-------------| +//! | `GET` | `/health` | Liveness probe (always returns 200) | +//! | `GET` | `/ready` | Readiness probe (returns 503 when `/tmp/objectstore.down` exists, enabling graceful drain) | +//! | `GET` | `/keda` | Prometheus text-format gauges for KEDA autoscaling (see [KEDA Metrics](crate#keda-metrics)) | +//! +//! # Code Usage +//! //! Use [`routes`] to create a router with all endpoints. use axum::Router; @@ -14,6 +122,7 @@ mod multipart; mod objects; #[cfg(all(target_os = "linux", feature = "profiling"))] mod profiling; +mod resumable; /// Returns `true` for internal endpoints that are exempt from metrics and concurrency limits. pub fn is_internal_route(route: &str) -> bool { diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index 1aafcbac..7ee6728e 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -1,7 +1,7 @@ use std::fmt::Write as _; use axum::body::Body; -use axum::extract::State; +use axum::extract::{OriginalUri, Query, State}; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::routing; @@ -15,6 +15,7 @@ use serde::Serialize; use crate::auth::AuthAwareService; use crate::endpoints::common::{ApiError, ApiResult, insert_accept_ranges}; +use crate::endpoints::resumable::{self, RequestPath, ResumableQuery, ResumableRoute}; use crate::extractors::byte_range::OptionalByteRange; use crate::extractors::{Xt, body::MeteredBody}; use crate::state::ServiceState; @@ -43,9 +44,27 @@ async fn objects_post( service: AuthAwareService, State(state): State, Xt(context): Xt, + OriginalUri(uri): OriginalUri, + Query(query): Query, headers: HeaderMap, MeteredBody(body): MeteredBody, ) -> ApiResult { + // A chunk always addresses a resolved key, so `?session=` has no meaning on the + // collection route. `?upload_type=resumable` creates a session for a generated key. + match query.classify()? { + ResumableRoute::Create => { + let id = ObjectId::optional(context, None); + let path = RequestPath::Collection; + return resumable::create_session(service, state, path, uri.path(), id, headers).await; + } + ResumableRoute::Session(_) => { + return Err(ApiError::Client( + "`session` requires an object key; use PUT on the object path".into(), + )); + } + ResumableRoute::Regular => {} + } + let metadata = Metadata::from_insert_headers(&headers, "").map_err(ServiceError::from)?; state @@ -199,9 +218,26 @@ async fn object_put( service: AuthAwareService, State(state): State, Xt(id): Xt, + OriginalUri(uri): OriginalUri, + Query(query): Query, headers: HeaderMap, - MeteredBody(body): MeteredBody, + body: MeteredBody, ) -> ApiResult { + // `PUT` carries all three write shapes: create a session, write a chunk, query the + // offset. `MeteredBody` is extracted unconditionally and dropped unread on the two + // bodyless paths. + match query.classify()? { + ResumableRoute::Create => { + let path = RequestPath::Object; + return resumable::create_session(service, state, path, uri.path(), id, headers).await; + } + ResumableRoute::Session(session) => { + return resumable::session_request(service, id, session, headers, body).await; + } + ResumableRoute::Regular => {} + } + + let MeteredBody(body) = body; let metadata = Metadata::from_insert_headers(&headers, "").map_err(ServiceError::from)?; let ObjectId { context, key } = id; @@ -226,7 +262,14 @@ async fn object_put( async fn object_delete( service: AuthAwareService, Xt(id): Xt, -) -> ApiResult { + Query(query): Query, +) -> ApiResult { + // With a session this terminates the upload; without one it deletes the object, as it + // always has. Note the two need different permissions — see `AuthAwareService`. + if let ResumableRoute::Session(session) = query.classify_session_only("DELETE")? { + return resumable::terminate(service, id, session).await; + } + service.delete_object(id).await?; - Ok(StatusCode::NO_CONTENT) + Ok(StatusCode::NO_CONTENT.into_response()) } diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs new file mode 100644 index 00000000..897e0207 --- /dev/null +++ b/objectstore-server/src/endpoints/resumable.rs @@ -0,0 +1,491 @@ +//! Resumable upload endpoints. +//! +//! Resumable uploads are a variation of the regular object endpoints rather than a separate +//! resource, following GCS and S3 rather than [TUS]. Every request addresses the same object +//! path with the session in the query string, so these handlers have no router of their own: +//! [`objects`](super::objects) dispatches to them based on [`ResumableQuery`]. +//! +//! | Operation | Request | Success | +//! |---|---|---| +//! | Create | `POST /objects/{usecase}/{scopes}/?upload_type=resumable` | `200` + `Location` + `{"key","session"}` | +//! | Create | `PUT /objects/{usecase}/{scopes}/{key}?upload_type=resumable` | `200` + `Location` + `{"key","session"}` | +//! | Chunk | `PUT …/{key}?session=` with `Upload-Offset: ` | `204` + `Upload-Offset`, or `201` + `{"key"}` | +//! | Offset query | `PUT …/{key}?session=` with `Upload-Offset: *` | `204` + `Upload-Offset`, or `201` + `{"key"}` | +//! | Terminate | `DELETE …/{key}?session=` | `204` | +//! +//! There is no completion request. The total size is known from session creation, so the +//! backend recognizes the chunk carrying the last byte and commits the object itself. +//! +//! Not every backend supports this. When one declines, session creation answers +//! `409 Conflict` and the client performs a regular upload instead. +//! +//! [TUS]: https://tus.io/protocols/resumable-upload + +use axum::http::{HeaderMap, StatusCode}; +use axum::response::{IntoResponse, Response}; +use axum::{Json, http}; +use objectstore_service::error::Error as ServiceError; +use objectstore_service::id::ObjectId; +use objectstore_service::resumable::{SessionToken, UploadOffset, UploadProgress}; +use objectstore_types::metadata::Metadata; +use objectstore_types::resumable::{ + CommitResponse, CreateSessionResponse, HEADER_UPLOAD_LENGTH, HEADER_UPLOAD_OFFSET, +}; +use serde::Deserialize; + +use crate::auth::AuthAwareService; +use crate::endpoints::common::{ApiError, ApiErrorResponse, ApiResult}; +use crate::extractors::body::MeteredBody; +use crate::state::ServiceState; + +/// The `upload_type` query parameter. +/// +/// Only one value is accepted, so an unrecognized upload type is a deserialization failure +/// and therefore a `400` rather than being silently treated as a regular upload. +#[derive(Clone, Copy, Debug, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub(super) enum UploadType { + /// Create a resumable upload session. + Resumable, +} + +/// The resumable protocol's query parameters, as seen on a regular object route. +/// +/// Both fields are optional and unknown parameters are ignored, because pre-signed URLs put +/// their own `os_*` parameters into the same query string. +#[derive(Debug, Default, Deserialize)] +pub(super) struct ResumableQuery { + /// Present on a session creation request. + upload_type: Option, + /// Present on a chunk write, offset query, or termination. + session: Option, +} + +/// What a request on an object route is addressing. +#[derive(Debug)] +pub(super) enum ResumableRoute { + /// Create a session for the object named by the request path. + Create, + /// Act on the identified session: write a chunk, query the offset, or terminate. + Session(SessionToken), + /// A regular object request that does not involve the resumable protocol. + Regular, +} + +impl ResumableQuery { + /// Classifies a request that may create a session or act on one. + /// + /// # Errors + /// + /// Returns [`ApiError::Client`] if both parameters are present. They address different + /// operations, so a request carrying both is ambiguous rather than defaulted. + pub fn classify(self) -> ApiResult { + match (self.upload_type, self.session) { + (Some(_), Some(_)) => Err(ApiError::Client( + "`upload_type` and `session` are mutually exclusive".into(), + )), + (Some(UploadType::Resumable), None) => Ok(ResumableRoute::Create), + (None, Some(session)) => Ok(ResumableRoute::Session(session)), + (None, None) => Ok(ResumableRoute::Regular), + } + } + + /// Classifies a request that may only act on an existing session. + /// + /// Used by routes where session creation is not defined: `DELETE`, which terminates, and + /// the collection `POST`, whose generated key is only known once a session exists. + /// + /// # Errors + /// + /// Returns [`ApiError::Client`] if `upload_type` is present. + pub fn classify_session_only(self, operation: &str) -> ApiResult { + if self.upload_type.is_some() { + return Err(ApiError::Client(format!( + "`upload_type` is not supported on {operation}" + ))); + } + + match self.session { + Some(session) => Ok(ResumableRoute::Session(session)), + None => Ok(ResumableRoute::Regular), + } + } +} + +/// Reads the required [`HEADER_UPLOAD_LENGTH`] header. +fn upload_length(headers: &HeaderMap) -> ApiResult { + let value = headers + .get(HEADER_UPLOAD_LENGTH) + .ok_or_else(|| ApiError::Client(format!("{HEADER_UPLOAD_LENGTH} header is required")))?; + + value + .to_str() + .ok() + .filter(|v| v.bytes().all(|b| b.is_ascii_digit())) + .and_then(|v| v.parse().ok()) + .ok_or_else(|| ApiError::Client(format!("{HEADER_UPLOAD_LENGTH} must be a byte count"))) +} + +/// Reads the required [`HEADER_UPLOAD_OFFSET`] header. +fn upload_offset(headers: &HeaderMap) -> ApiResult { + let value = headers + .get(HEADER_UPLOAD_OFFSET) + .ok_or_else(|| ApiError::Client(format!("{HEADER_UPLOAD_OFFSET} header is required")))?; + + value + .to_str() + .map_err(|_| ApiError::Client(format!("{HEADER_UPLOAD_OFFSET} must be ASCII")))? + .parse() + .map_err(|e: objectstore_types::resumable::InvalidUploadOffset| { + ApiError::Client(e.to_string()) + }) +} + +/// Reads the required `Content-Length` header. +/// +/// Chunks declare their length so the server can forward only the prefix a backend accepts +/// without buffering the body to find out how long it is. +fn content_length(headers: &HeaderMap) -> ApiResult { + headers + .get(http::header::CONTENT_LENGTH) + .and_then(|v| v.to_str().ok()) + .and_then(|v| v.parse::().ok()) + .ok_or_else(|| ApiError::Client("Content-Length header is required".into())) +} + +/// How the request path relates to the object a session is being created for. +/// +/// Needed to build the `Location` header, which must always name the object even when the +/// request did not. +#[derive(Clone, Copy, Debug)] +pub(super) enum RequestPath { + /// The request path names the object, as on a `PUT` to the object route. + Object, + /// The request path is the collection the object lives in, as on a `POST` whose key was + /// generated by the server and therefore does not appear in the path. + Collection, +} + +/// Creates a session for the object at `id`. +/// +/// Answers `409 Conflict` when the backend declines, which tells the client to fall back to a +/// regular upload. Metadata is declared here and does not change afterwards. +pub(super) async fn create_session( + service: AuthAwareService, + state: ServiceState, + request_path: RequestPath, + uri_path: &str, + id: ObjectId, + headers: HeaderMap, +) -> ApiResult { + let total_length = upload_length(&headers)?; + let metadata = Metadata::from_insert_headers(&headers, "").map_err(ServiceError::from)?; + + state + .config + .usecases + .validate(&id.context().usecase, &metadata) + .map_err(|e| ApiError::Client(e.to_string()))?; + + let Some(session) = service + .create_upload_session(id.clone(), metadata, total_length) + .await? + else { + let body = ApiErrorResponse::message("resumable uploads are unavailable for this object"); + return Ok((StatusCode::CONFLICT, Json(body)).into_response()); + }; + + let mut headers = HeaderMap::new(); + if let Some(location) = session_location(uri_path, request_path, id.key(), &session) { + headers.insert(http::header::LOCATION, location); + } + + let body = Json(CreateSessionResponse { + key: id.key().to_owned(), + session, + }); + Ok((StatusCode::OK, headers, body).into_response()) +} + +/// Builds the `Location` header pointing at the session. +/// +/// The value is the object path with the session appended, so a client that persisted the key +/// and the session can rebuild it without having stored a URL. `Router::nest` strips the `/v1` +/// prefix from the request URI, so callers pass the path from +/// [`OriginalUri`](axum::extract::OriginalUri) — and on the collection route that path does +/// not name the object, so the key is appended to it. +/// +/// Returns `None` if the result is not a valid header value, in which case the header is +/// omitted: it is a convenience, and the response body carries the same information. +fn session_location( + uri_path: &str, + request_path: RequestPath, + key: &str, + session: &SessionToken, +) -> Option { + let object_path = match request_path { + RequestPath::Object => uri_path.to_owned(), + RequestPath::Collection => format!("{}/{key}", uri_path.trim_end_matches('/')), + }; + + http::HeaderValue::from_str(&format!("{object_path}?session={session}")).ok() +} + +/// Acts on an open session: writes a chunk, or reports the offset the server holds. +/// +/// [`HEADER_UPLOAD_OFFSET`] selects between the two. A concrete offset submits the request +/// body as the chunk starting there; the `*` wildcard submits nothing and asks where the +/// server stands, which also commits an object that was assembled but not yet committed. +/// +/// Both answer `204 No Content` with the authoritative offset while bytes remain, and +/// `201 Created` with the key once the object is committed. +pub(super) async fn session_request( + service: AuthAwareService, + id: ObjectId, + session: SessionToken, + headers: HeaderMap, + MeteredBody(body): MeteredBody, +) -> ApiResult { + let offset = upload_offset(&headers)?; + let content_length = content_length(&headers)?; + let key = id.key().to_owned(); + + let progress = match offset { + UploadOffset::At(offset) => { + service + .put_chunk(id, session, offset, content_length, body) + .await + } + UploadOffset::Unknown => { + // The wildcard carries no payload. A body would be silently discarded, so + // reject it rather than let a client believe those bytes were written. + if content_length != 0 { + return Err(ApiError::Client(format!( + "{HEADER_UPLOAD_OFFSET}: * must be sent with an empty body" + ))); + } + + service.upload_offset(id, session).await + } + }; + + progress_response(progress, key) +} + +/// Terminates a session, discarding whatever was uploaded. +pub(super) async fn terminate( + service: AuthAwareService, + id: ObjectId, + session: SessionToken, +) -> ApiResult { + service.terminate_upload(id, session).await?; + Ok(StatusCode::NO_CONTENT.into_response()) +} + +/// Turns an [`UploadProgress`] outcome into the response shared by chunks and offset queries. +/// +/// An offset mismatch is answered here rather than through [`ApiError::status`], because the +/// authoritative offset has to travel in a header that a generic error response cannot set. +fn progress_response(progress: ApiResult, key: String) -> ApiResult { + let progress = match progress { + Ok(progress) => progress, + Err(ApiError::Service(ServiceError::UploadOffsetMismatch { offset })) => { + let body = ApiErrorResponse::message(format!("expected offset {offset}")); + let response = ( + StatusCode::CONFLICT, + [(HEADER_UPLOAD_OFFSET, http::HeaderValue::from(offset))], + Json(body), + ); + return Ok(response.into_response()); + } + Err(e) => return Err(e), + }; + + let response = match progress { + UploadProgress::Incomplete { offset } => ( + StatusCode::NO_CONTENT, + [(HEADER_UPLOAD_OFFSET, http::HeaderValue::from(offset))], + ) + .into_response(), + UploadProgress::Committed => { + (StatusCode::CREATED, Json(CommitResponse { key })).into_response() + } + }; + + Ok(response) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn query(upload_type: Option, session: Option<&str>) -> ResumableQuery { + ResumableQuery { + upload_type, + session: session.map(|s| SessionToken::new(s.into()).unwrap()), + } + } + + #[test] + fn classify_recognizes_each_operation() { + assert!(matches!( + query(Some(UploadType::Resumable), None).classify(), + Ok(ResumableRoute::Create) + )); + assert!(matches!( + query(None, Some("token")).classify(), + Ok(ResumableRoute::Session(_)) + )); + assert!(matches!( + query(None, None).classify(), + Ok(ResumableRoute::Regular) + )); + } + + #[test] + fn classify_rejects_both_parameters() { + let result = query(Some(UploadType::Resumable), Some("token")).classify(); + assert!(matches!(result, Err(ApiError::Client(_))), "{result:?}"); + } + + #[test] + fn classify_session_only_rejects_upload_type() { + let result = query(Some(UploadType::Resumable), None).classify_session_only("DELETE"); + assert!(matches!(result, Err(ApiError::Client(_))), "{result:?}"); + + assert!(matches!( + query(None, Some("token")).classify_session_only("DELETE"), + Ok(ResumableRoute::Session(_)) + )); + assert!(matches!( + query(None, None).classify_session_only("DELETE"), + Ok(ResumableRoute::Regular) + )); + } + + #[test] + fn upload_length_requires_a_byte_count() { + let mut headers = HeaderMap::new(); + assert!(upload_length(&headers).is_err(), "missing header"); + + for invalid in ["", "-1", "+1", "1.5", "abc", " 1"] { + headers.insert(HEADER_UPLOAD_LENGTH, invalid.parse().unwrap()); + assert!(upload_length(&headers).is_err(), "accepted {invalid:?}"); + } + + headers.insert(HEADER_UPLOAD_LENGTH, "1048576".parse().unwrap()); + assert_eq!(upload_length(&headers).unwrap(), 1_048_576); + } + + #[test] + fn upload_offset_parses_chunk_and_wildcard() { + let mut headers = HeaderMap::new(); + assert!(upload_offset(&headers).is_err(), "missing header"); + + headers.insert(HEADER_UPLOAD_OFFSET, "*".parse().unwrap()); + assert_eq!(upload_offset(&headers).unwrap(), UploadOffset::Unknown); + + headers.insert(HEADER_UPLOAD_OFFSET, "262144".parse().unwrap()); + assert_eq!(upload_offset(&headers).unwrap(), UploadOffset::At(262_144)); + + headers.insert(HEADER_UPLOAD_OFFSET, "nope".parse().unwrap()); + assert!(upload_offset(&headers).is_err()); + } + + /// Reads a response's status, `Upload-Offset` header, and body. + async fn parts_of(response: Response) -> (StatusCode, Option, String) { + let status = response.status(); + let offset = response + .headers() + .get(HEADER_UPLOAD_OFFSET) + .map(|v| v.to_str().unwrap().to_owned()); + + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + + (status, offset, String::from_utf8(body.to_vec()).unwrap()) + } + + #[tokio::test] + async fn incomplete_progress_answers_no_content_with_the_offset() { + let progress = Ok(UploadProgress::Incomplete { offset: 262_144 }); + let response = progress_response(progress, "my-key".into()).unwrap(); + + let (status, offset, body) = parts_of(response).await; + assert_eq!(status, StatusCode::NO_CONTENT); + assert_eq!(offset.as_deref(), Some("262144")); + assert!(body.is_empty(), "204 must not carry a body: {body:?}"); + } + + #[tokio::test] + async fn commit_answers_created_with_the_key() { + let response = progress_response(Ok(UploadProgress::Committed), "my-key".into()).unwrap(); + + let (status, offset, body) = parts_of(response).await; + assert_eq!(status, StatusCode::CREATED); + assert_eq!(offset, None, "a commit reports no offset"); + assert_eq!(body, r#"{"key":"my-key"}"#); + } + + #[tokio::test] + async fn offset_mismatch_answers_conflict_with_the_authoritative_offset() { + let mismatch = ServiceError::UploadOffsetMismatch { offset: 786_432 }; + let response = + progress_response(Err(ApiError::Service(mismatch)), "my-key".into()).unwrap(); + + let (status, offset, body) = parts_of(response).await; + assert_eq!(status, StatusCode::CONFLICT); + assert_eq!( + offset.as_deref(), + Some("786432"), + "the client resynchronizes from this header" + ); + assert!(body.contains("786432"), "{body:?}"); + } + + #[tokio::test] + async fn other_errors_propagate_unchanged() { + let gone = ApiError::Service(ServiceError::UploadSessionGone); + let error = progress_response(Err(gone), "my-key".into()).unwrap_err(); + assert_eq!(error.status(), StatusCode::GONE); + + let invalid = ApiError::Service(ServiceError::InvalidUploadRequest("bad".into())); + let error = progress_response(Err(invalid), "my-key".into()).unwrap_err(); + assert_eq!(error.status(), StatusCode::BAD_REQUEST); + } + + #[test] + fn session_location_appends_the_session_to_the_object_path() { + let session = SessionToken::new("tok3n".into()).unwrap(); + let location = session_location( + "/v1/objects/testing/org=1/my-key", + RequestPath::Object, + "my-key", + &session, + ); + + assert_eq!( + location.unwrap(), + "/v1/objects/testing/org=1/my-key?session=tok3n" + ); + } + + #[test] + fn session_location_appends_a_generated_key_to_the_collection_path() { + let session = SessionToken::new("tok3n".into()).unwrap(); + + // The `POST` route matches with and without a trailing slash, and the generated key + // never appears in the request path — so it has to be appended either way. + for collection in ["/v1/objects/testing/org=1/", "/v1/objects/testing/org=1"] { + let location = + session_location(collection, RequestPath::Collection, "generated", &session); + + assert_eq!( + location.unwrap(), + "/v1/objects/testing/org=1/generated?session=tok3n", + "for request path {collection:?}" + ); + } + } +} diff --git a/objectstore-server/tests/resumable.rs b/objectstore-server/tests/resumable.rs new file mode 100644 index 00000000..72d6a4bc --- /dev/null +++ b/objectstore-server/tests/resumable.rs @@ -0,0 +1,312 @@ +//! Integration tests for the resumable upload endpoints. +//! +//! No backend implements resumable uploads yet, so the reachable surface is session denial +//! and request validation. That is deliberate: a deployment must answer `409 Conflict` to +//! every session creation so clients fall back to a regular upload, and it must reject a +//! malformed request before it reaches a backend. +//! +//! The `501 Not Implemented` assertions are the proof that dispatch and header parsing work: +//! the only way to reach a declining backend method is through a well-formed request. + +use anyhow::Result; +use objectstore_server::config::{AuthZ, Config}; +use objectstore_test::server::TestServer; +use objectstore_types::resumable::{HEADER_UPLOAD_LENGTH, HEADER_UPLOAD_OFFSET}; +use reqwest::StatusCode; + +async fn test_server() -> TestServer { + TestServer::with_config(Config { + auth: AuthZ { + enforce: false, + ..Default::default() + }, + ..Default::default() + }) + .await +} + +// --- Session creation --- + +#[tokio::test] +async fn create_session_is_denied_with_client_key() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?upload_type=resumable")) + .header(HEADER_UPLOAD_LENGTH, "1048576") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::CONFLICT); + Ok(()) +} + +#[tokio::test] +async fn create_session_is_denied_with_generated_key() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .post(server.url("/v1/objects/test/org=1/?upload_type=resumable")) + .header(HEADER_UPLOAD_LENGTH, "1048576") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::CONFLICT); + Ok(()) +} + +#[tokio::test] +async fn create_session_requires_upload_length() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?upload_type=resumable")) + .send() + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +#[tokio::test] +async fn create_session_rejects_malformed_upload_length() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + + for invalid in ["", "-1", "1.5", "lots"] { + let response = client + .put(server.url("/v1/objects/test/org=1/my-key?upload_type=resumable")) + .header(HEADER_UPLOAD_LENGTH, invalid) + .send() + .await?; + + assert_eq!( + response.status(), + StatusCode::BAD_REQUEST, + "accepted {HEADER_UPLOAD_LENGTH}: {invalid:?}" + ); + } + + Ok(()) +} + +#[tokio::test] +async fn unknown_upload_type_is_rejected() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?upload_type=multipart")) + .header(HEADER_UPLOAD_LENGTH, "1048576") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +// --- Chunks and offset queries --- + +#[tokio::test] +async fn chunk_reaches_the_declining_backend() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .header(HEADER_UPLOAD_OFFSET, "0") + .body("payload") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); + Ok(()) +} + +#[tokio::test] +async fn offset_query_reaches_the_declining_backend() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .header(HEADER_UPLOAD_OFFSET, "*") + .header(reqwest::header::CONTENT_LENGTH, "0") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); + Ok(()) +} + +#[tokio::test] +async fn chunk_requires_upload_offset() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .body("payload") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +#[tokio::test] +async fn chunk_rejects_malformed_upload_offset() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + + for invalid in ["", "-1", "1.5", "**", "here"] { + let response = client + .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .header(HEADER_UPLOAD_OFFSET, invalid) + .body("payload") + .send() + .await?; + + assert_eq!( + response.status(), + StatusCode::BAD_REQUEST, + "accepted {HEADER_UPLOAD_OFFSET}: {invalid:?}" + ); + } + + Ok(()) +} + +#[tokio::test] +async fn offset_query_rejects_a_payload() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .header(HEADER_UPLOAD_OFFSET, "*") + .body("payload") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +#[tokio::test] +async fn session_token_with_path_traversal_is_rejected() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?session=../escape")) + .header(HEADER_UPLOAD_OFFSET, "0") + .body("payload") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +// --- Termination --- + +#[tokio::test] +async fn terminate_reaches_the_declining_backend() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .delete(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .send() + .await?; + + assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); + Ok(()) +} + +#[tokio::test] +async fn delete_rejects_upload_type() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .delete(server.url("/v1/objects/test/org=1/my-key?upload_type=resumable")) + .send() + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +// --- Parameter combinations --- + +#[tokio::test] +async fn upload_type_and_session_are_mutually_exclusive() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?upload_type=resumable&session=some-token")) + .header(HEADER_UPLOAD_LENGTH, "1048576") + .header(HEADER_UPLOAD_OFFSET, "0") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +#[tokio::test] +async fn session_on_the_collection_route_is_rejected() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .post(server.url("/v1/objects/test/org=1/?session=some-token")) + .header(HEADER_UPLOAD_OFFSET, "0") + .body("payload") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + +// --- Regular uploads are unaffected --- + +#[tokio::test] +async fn regular_object_operations_still_work() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + + let response = client + .put(server.url("/v1/objects/test/org=1/my-key")) + .body("payload") + .send() + .await?; + assert_eq!(response.status(), StatusCode::OK); + + let response = client + .get(server.url("/v1/objects/test/org=1/my-key")) + .send() + .await?; + assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.text().await?, "payload"); + + let response = client + .delete(server.url("/v1/objects/test/org=1/my-key")) + .send() + .await?; + assert_eq!(response.status(), StatusCode::NO_CONTENT); + + Ok(()) +} + +#[tokio::test] +async fn regular_upload_ignores_resumable_headers() -> Result<()> { + let server = test_server().await; + + // Without a query parameter the request is a regular upload, and the protocol headers + // carry no meaning. They must not accidentally engage the resumable path. + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key")) + .header(HEADER_UPLOAD_LENGTH, "7") + .header(HEADER_UPLOAD_OFFSET, "0") + .body("payload") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::OK); + Ok(()) +} diff --git a/objectstore-service/docs/architecture.md b/objectstore-service/docs/architecture.md index 0b745d46..6f98a241 100644 --- a/objectstore-service/docs/architecture.md +++ b/objectstore-service/docs/architecture.md @@ -190,6 +190,30 @@ The default execution limit is [`DEFAULT_CONCURRENCY_LIMIT`](service::DEFAULT_CONCURRENCY_LIMIT). See [`StorageService::with_concurrency`] for configuration. +## Resumable Uploads + +A resumable upload writes one object across several requests: the payload arrives +as a sequence of chunks at increasing byte offsets, so an interrupted transfer +continues where it stopped instead of starting over. That is worth the extra round +trips for objects large enough that re-sending the whole payload is expensive. + +[`StorageService`] exposes four operations, each a method on +[`Backend`](backend::common::Backend), which run in this sequence: + +1. [`create_upload_session`](backend::common::Backend::create_upload_session) + declares the total size and metadata, and returns a session token. +2. [`put_chunk`](backend::common::Backend::put_chunk) writes bytes at an offset and + reports the offset now persisted. +3. After a failure, [`upload_offset`](backend::common::Backend::upload_offset) + reports where the backend stands, so the caller resumes from there. +4. The chunk carrying the last byte commits the object. There is no finalize call — + the backend recognizes that chunk from the declared total size. +5. At any time, a session can be terminated, which discards what it holds. + +Not all backends support resumable uploads and can decline creating a session. +Support can depend on the declared size, the metadata, or whether resuming is +possible in principle. + ## Multipart Uploads When the configured backend supports it, [`StorageService`] exposes multipart diff --git a/objectstore-service/src/backend/common.rs b/objectstore-service/src/backend/common.rs index 45b9b4a0..e29e27dd 100644 --- a/objectstore-service/src/backend/common.rs +++ b/objectstore-service/src/backend/common.rs @@ -13,6 +13,9 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; +use crate::resumable::{ + CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, +}; use crate::stream::{ClientStream, PayloadStream}; /// User agent string used for outgoing requests. @@ -72,6 +75,112 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { fn as_multipart_upload_backend(&self) -> Result<&dyn MultipartUploadBackend> { Err(Error::NotImplemented) } + + /// Opens a resumable upload session for the object at `id`. + /// + /// `total_length` is the complete size of the object in bytes, declared by the client + /// when the session is created. It is a parameter of its own rather than part of + /// `metadata`, because [`Metadata::size`] is materialized by the server and never + /// trusted from a client. The backend needs it to recognize the final chunk, and a + /// tiering backend needs it to decide where the object would be placed. + /// + /// `metadata` is fixed for the lifetime of the session and does not change afterwards. + /// Compression is recorded rather than applied: the payload must already be compressed, + /// since its total length has to be known at this point. + /// + /// Returns `Ok(None)` when this backend cannot store the described object resumably. + /// Declining is a routine outcome, not an error — the server denies the session and the + /// client falls back to a regular upload. The default implementation declines, so a + /// backend opts in simply by overriding this method. There is deliberately no separate + /// capability trait and no probe: support can depend on the size, the metadata and the + /// routing result at once, all of which are only known here. + /// + /// # Errors + /// + /// Returns an error only when the backend supports resumable uploads but failed to open + /// the session. + async fn create_upload_session( + &self, + id: &ObjectId, + metadata: &Metadata, + total_length: u64, + ) -> Result { + let _ = (id, metadata, total_length); + Ok(None) + } + + /// Writes a chunk of `content_length` bytes at `offset` into an open session. + /// + /// `offset` must equal the offset the backend currently holds. Backends persist only + /// aligned prefixes and discard the remainder, so the offset in the returned + /// [`UploadProgress::Incomplete`] is authoritative and may be lower than + /// `offset + content_length`. + /// + /// A session has a single writer. Concurrent chunk writes are not coordinated: one of + /// them wins and the others fail with [`Error::UploadOffsetMismatch`]. + /// + /// Once the chunk carrying the last byte is persisted, the backend assembles and commits + /// the object and returns [`UploadProgress::Committed`]. + /// + /// # Errors + /// + /// - [`Error::NotImplemented`] if this backend does not support resumable uploads. The + /// default implementation returns this, which is unreachable through the API because a + /// backend that declines in [`Self::create_upload_session`] never hands out a session. + /// - [`Error::UploadOffsetMismatch`] if `offset` is not the offset the backend holds. + /// - [`Error::UploadSessionGone`] if the session expired or was terminated. + /// - [`Error::InvalidUploadRequest`] if the session is unusable, or the chunk would + /// exceed the length declared at creation. + async fn put_chunk( + &self, + id: &ObjectId, + session: &SessionToken, + offset: u64, + content_length: u64, + stream: ClientStream, + ) -> Result { + let _ = (id, session, offset, content_length, stream); + Err(Error::NotImplemented) + } + + /// Reports how far the session has progressed, committing the object if it is assembled. + /// + /// This is the recovery path: after any failed chunk the client calls this and continues + /// from the returned offset. It is also the only read-shaped operation that mutates + /// state. Making an object visible can outlive the request that triggered it, so a + /// session whose payload fully landed may still be uncommitted; this operation finishes + /// that work and returns [`UploadProgress::Committed`]. Callers must therefore treat it + /// as a write. + /// + /// A session whose object was assembled but not yet committed never reports + /// [`UploadProgress::Committed`], so a client that observes completion can always read + /// the object back. + /// + /// # Errors + /// + /// The same conditions as [`Self::put_chunk`], except for the offset mismatch. + async fn upload_offset(&self, id: &ObjectId, session: &SessionToken) -> Result { + let _ = (id, session); + Err(Error::NotImplemented) + } + + /// Terminates a session, discarding whatever was uploaded. + /// + /// Idempotent. Not required for correctness, since sessions expire on their own, but it + /// lets a caller release an abandoned upload immediately. + /// + /// # Errors + /// + /// - [`Error::NotImplemented`] if this backend does not support resumable uploads. + /// - [`Error::InvalidUploadRequest`] if the session token is unusable. + async fn terminate_upload( + &self, + id: &ObjectId, + session: &SessionToken, + ) -> Result { + let _ = (id, session); + Err(Error::NotImplemented) + } } /// Trait for backends that support our S3-style multipart upload protocol. diff --git a/objectstore-service/src/backend/counting.rs b/objectstore-service/src/backend/counting.rs index 4e597e0f..7f8b25f3 100644 --- a/objectstore-service/src/backend/counting.rs +++ b/objectstore-service/src/backend/counting.rs @@ -28,6 +28,9 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; +use crate::resumable::{ + CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, +}; use crate::stream::ClientStream; /// Increments `cogs.usage` by one operation for the given `usecase`. @@ -46,6 +49,12 @@ fn count(usecase: &str) { /// `Arc`s that point to the inner backend: /// - `inner: Arc` /// - `inner_multipart: Option>` if `inner` supports it +/// +/// Resumable uploads avoid this problem: their operations live on [`Backend`] itself and express +/// support by declining in +/// [`create_upload_session`](Backend::create_upload_session), so this decorator forwards them like +/// any other method. Forwarding is mandatory — without it the decorator's declining default would +/// shadow an inner backend that does support resumable uploads. #[derive(Debug)] pub struct CountingBackend { inner: Arc, @@ -99,6 +108,46 @@ impl Backend for CountingBackend { self.inner.as_multipart_upload_backend()?; Ok(self) } + + async fn create_upload_session( + &self, + id: &ObjectId, + metadata: &Metadata, + total_length: u64, + ) -> Result { + count(&id.context.usecase); + self.inner + .create_upload_session(id, metadata, total_length) + .await + } + + async fn put_chunk( + &self, + id: &ObjectId, + session: &SessionToken, + offset: u64, + content_length: u64, + stream: ClientStream, + ) -> Result { + count(&id.context.usecase); + self.inner + .put_chunk(id, session, offset, content_length, stream) + .await + } + + async fn upload_offset(&self, id: &ObjectId, session: &SessionToken) -> Result { + count(&id.context.usecase); + self.inner.upload_offset(id, session).await + } + + async fn terminate_upload( + &self, + id: &ObjectId, + session: &SessionToken, + ) -> Result { + count(&id.context.usecase); + self.inner.terminate_upload(id, session).await + } } #[async_trait::async_trait] diff --git a/objectstore-service/src/backend/testing.rs b/objectstore-service/src/backend/testing.rs index 24034f24..71e56250 100644 --- a/objectstore-service/src/backend/testing.rs +++ b/objectstore-service/src/backend/testing.rs @@ -52,6 +52,9 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; +use crate::resumable::{ + CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, +}; use crate::stream::ClientStream; /// Hooks for [`TestBackend`]. @@ -237,6 +240,60 @@ pub trait Hooks: fmt::Debug + Send + Sync + 'static { ) -> Result { inner.complete_multipart(id, upload_id, parts).await } + + // --- Resumable upload methods --- + // + // `InMemoryBackend` does not implement resumable uploads, so these delegate to the + // declining `Backend` defaults. A test that exercises the resumable protocol has to + // override them. + + /// Intercepts [`Backend::create_upload_session`]. Default delegates to `inner`. + async fn create_upload_session( + &self, + inner: &InMemoryBackend, + id: &ObjectId, + metadata: &Metadata, + total_length: u64, + ) -> Result { + inner + .create_upload_session(id, metadata, total_length) + .await + } + + /// Intercepts [`Backend::put_chunk`]. Default delegates to `inner`. + async fn put_chunk( + &self, + inner: &InMemoryBackend, + id: &ObjectId, + session: &SessionToken, + offset: u64, + content_length: u64, + stream: ClientStream, + ) -> Result { + inner + .put_chunk(id, session, offset, content_length, stream) + .await + } + + /// Intercepts [`Backend::upload_offset`]. Default delegates to `inner`. + async fn upload_offset( + &self, + inner: &InMemoryBackend, + id: &ObjectId, + session: &SessionToken, + ) -> Result { + inner.upload_offset(id, session).await + } + + /// Intercepts [`Backend::terminate_upload`]. Default delegates to `inner`. + async fn terminate_upload( + &self, + inner: &InMemoryBackend, + id: &ObjectId, + session: &SessionToken, + ) -> Result { + inner.terminate_upload(id, session).await + } } /// Generic test backend that implements both [`Backend`] and [`HighVolumeBackend`]. @@ -311,6 +368,42 @@ impl Backend for TestBackend { async fn join(&self) { self.hooks.join(&self.inner).await } + + async fn create_upload_session( + &self, + id: &ObjectId, + metadata: &Metadata, + total_length: u64, + ) -> Result { + self.hooks + .create_upload_session(&self.inner, id, metadata, total_length) + .await + } + + async fn put_chunk( + &self, + id: &ObjectId, + session: &SessionToken, + offset: u64, + content_length: u64, + stream: ClientStream, + ) -> Result { + self.hooks + .put_chunk(&self.inner, id, session, offset, content_length, stream) + .await + } + + async fn upload_offset(&self, id: &ObjectId, session: &SessionToken) -> Result { + self.hooks.upload_offset(&self.inner, id, session).await + } + + async fn terminate_upload( + &self, + id: &ObjectId, + session: &SessionToken, + ) -> Result { + self.hooks.terminate_upload(&self.inner, id, session).await + } } #[async_trait::async_trait] diff --git a/objectstore-service/src/backend/tiered.rs b/objectstore-service/src/backend/tiered.rs index a6af1342..43c0e96d 100644 --- a/objectstore-service/src/backend/tiered.rs +++ b/objectstore-service/src/backend/tiered.rs @@ -96,6 +96,16 @@ //! already-mutated state and still returns `true` — so callers do not mistakenly //! treat a successful commit as a lost race and clean up data that was actually //! persisted. +//! +//! # Resumable Uploads +//! +//! Not implemented here yet, so [`TieredStorage`] inherits the declining defaults from +//! [`Backend`] and every session creation is denied. A resumable upload will be a regular +//! long-term write whose payload arrives across several requests, reusing the revision keys, +//! changelog phases and compare-and-write commit described above: session creation decides +//! the tier from the declared total length and declines if that tier cannot support it, +//! non-final chunks pass straight through to the upstream session, and the final chunk runs +//! the long-term write sequence. use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; diff --git a/objectstore-service/src/error.rs b/objectstore-service/src/error.rs index 9643dd43..678cf94b 100644 --- a/objectstore-service/src/error.rs +++ b/objectstore-service/src/error.rs @@ -151,6 +151,29 @@ pub enum Error { /// Invalid upload ID (e.g. path traversal attempt). #[error(transparent)] InvalidUploadId(#[from] objectstore_types::multipart::InvalidUploadId), + + /// A resumable chunk was submitted at an offset the backend does not hold. + /// + /// The client resynchronizes by continuing from [`offset`](Self::UploadOffsetMismatch::offset), + /// which is authoritative and may be lower than the end of a previously acknowledged chunk. + #[error("upload offset mismatch (server holds {offset} bytes)")] + UploadOffsetMismatch { + /// The offset the backend currently holds. + offset: u64, + }, + + /// The resumable upload session expired or was terminated, retaining nothing. + /// + /// The client has to start a new session. + #[error("upload session gone")] + UploadSessionGone, + + /// A resumable upload request is unusable for the session it addresses. + /// + /// Covers an unparseable or unknown session token and a chunk that would exceed the + /// length declared when the session was created. + #[error("invalid upload request: {0}")] + InvalidUploadRequest(String), } impl Error { @@ -197,6 +220,9 @@ impl Error { Self::Client(_) => Level::DEBUG, Self::Metadata(_) => Level::DEBUG, Self::RangeNotSatisfiable { .. } => Level::DEBUG, + Self::UploadOffsetMismatch { .. } => Level::DEBUG, + Self::UploadSessionGone => Level::DEBUG, + Self::InvalidUploadRequest(_) => Level::DEBUG, // Like rate limits, we treat capacity errors as warnings Self::AtCapacity => Level::WARN, // All other errors are service or backend failures diff --git a/objectstore-service/src/lib.rs b/objectstore-service/src/lib.rs index e33a2f7c..d1a2d364 100644 --- a/objectstore-service/src/lib.rs +++ b/objectstore-service/src/lib.rs @@ -8,6 +8,7 @@ pub mod error; mod gcp_auth; pub mod id; pub mod multipart; +pub mod resumable; pub mod service; pub mod stream; pub mod streaming; diff --git a/objectstore-service/src/resumable.rs b/objectstore-service/src/resumable.rs new file mode 100644 index 00000000..3039e2b9 --- /dev/null +++ b/objectstore-service/src/resumable.rs @@ -0,0 +1,46 @@ +//! Shared types for Objectstore's resumable upload protocol. +//! +//! A resumable upload is a regular write whose payload arrives across several requests. +//! A session declares the object's total size and metadata upfront; chunks then arrive at +//! increasing byte offsets, and the backend commits the object itself once the last byte +//! lands. See [`objectstore_types::resumable`] for the wire-level types. +//! +//! Not every backend can support this. Session creation therefore asks the backend that +//! would store the object to open one, and a backend that cannot declines by returning +//! `None` from +//! [`Backend::create_upload_session`](crate::backend::common::Backend::create_upload_session). +//! Declining is a routine outcome rather than an error: the server denies the session and +//! the client falls back to a regular upload. + +pub use objectstore_types::resumable::{InvalidSessionToken, SessionToken, UploadOffset}; + +/// How far a resumable upload has progressed. +/// +/// Returned by both +/// [`Backend::put_chunk`](crate::backend::common::Backend::put_chunk) and +/// [`Backend::upload_offset`](crate::backend::common::Backend::upload_offset), because an +/// offset query commits an object that was assembled but not yet committed and therefore +/// has the same two outcomes as a chunk write. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum UploadProgress { + /// More bytes are expected. The client continues from `offset`. + /// + /// This offset is authoritative and may be lower than the end of the chunk that was + /// just written: backends persist only aligned prefixes and discard the remainder. + Incomplete { + /// The offset the backend has persisted. + offset: u64, + }, + /// The last byte arrived and the object is committed and readable. + Committed, +} + +/// Response for +/// [`Backend::create_upload_session`](crate::backend::common::Backend::create_upload_session). +/// +/// `None` means the backend declines resumable uploads for this object. +pub type CreateSessionResponse = Option; + +/// Response for +/// [`Backend::terminate_upload`](crate::backend::common::Backend::terminate_upload). +pub type TerminateUploadResponse = (); diff --git a/objectstore-service/src/service.rs b/objectstore-service/src/service.rs index 0953fcd8..ca8dc68c 100644 --- a/objectstore-service/src/service.rs +++ b/objectstore-service/src/service.rs @@ -20,6 +20,9 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; +use crate::resumable::{ + CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, +}; use crate::stream::{ClientStream, PayloadStream}; use crate::streaming::StreamExecutor; @@ -359,6 +362,82 @@ impl StorageService { }) .await } + + // --- Resumable upload operations --- + + /// Opens a resumable upload session for an object of `total_length` bytes. + /// + /// Returns `Ok(None)` when the backend declines resumable uploads for this object, in + /// which case the caller should fall back to [`Self::insert_object`]. Unlike the + /// multipart operations there is no eager capability probe: support is the return value. + pub async fn create_upload_session( + &self, + id: ObjectId, + metadata: Metadata, + total_length: u64, + ) -> Result { + metadata.validate()?; + let inner = Arc::clone(&self.inner); + self.spawn("create_upload_session", async move { + inner + .create_upload_session(&id, &metadata, total_length) + .await + }) + .await + } + + /// Writes a chunk of `content_length` bytes at `offset` into an open session. + /// + /// Commits the object once the chunk carrying the last byte is persisted. + /// + /// # Run-to-completion + /// + /// Once called, the operation runs to completion even if the returned future is dropped. + /// This matters most for the final chunk, which commits the object. + pub async fn put_chunk( + &self, + id: ObjectId, + session: SessionToken, + offset: u64, + content_length: u64, + body: ClientStream, + ) -> Result { + let inner = Arc::clone(&self.inner); + self.spawn("put_chunk", async move { + inner + .put_chunk(&id, &session, offset, content_length, body) + .await + }) + .await + } + + /// Reports how far a session has progressed, committing the object if it is assembled. + /// + /// This mutates state and therefore requires write permission at the API layer. + pub async fn upload_offset( + &self, + id: ObjectId, + session: SessionToken, + ) -> Result { + let inner = Arc::clone(&self.inner); + self.spawn("upload_offset", async move { + inner.upload_offset(&id, &session).await + }) + .await + } + + /// Terminates a session, discarding whatever was uploaded. + pub async fn terminate_upload( + &self, + id: ObjectId, + session: SessionToken, + ) -> Result { + let inner = Arc::clone(&self.inner); + self.spawn("terminate_upload", async move { + inner.terminate_upload(&id, &session).await + }) + .await + } } #[cfg(test)] @@ -368,7 +447,7 @@ mod tests { use bytes::BytesMut; use futures_util::TryStreamExt; - use objectstore_types::metadata::Metadata; + use objectstore_types::metadata::{ExpirationPolicy, Metadata}; use objectstore_types::range::ByteRange; use objectstore_types::scope::{Scope, Scopes}; @@ -781,4 +860,147 @@ mod tests { "permit was not released after panic" ); } + + // --- Resumable uploads --- + + #[tokio::test] + async fn resumable_declines_by_default() { + let service = make_service(); + let id = ObjectId::new(make_context(), "resumable".into()); + let session = SessionToken::new("session".into()).unwrap(); + + let denied = service + .create_upload_session(id.clone(), Metadata::default(), 1024) + .await + .unwrap(); + assert!(denied.is_none(), "expected the backend to decline"); + + // Without a session no other operation is reachable through the API, but the + // declining defaults must still be wired up rather than panicking. + let chunk = service + .put_chunk(id.clone(), session.clone(), 0, 4, stream::single("data")) + .await; + assert!(matches!(chunk, Err(Error::NotImplemented))); + let offset = service.upload_offset(id.clone(), session.clone()).await; + assert!(matches!(offset, Err(Error::NotImplemented))); + let terminated = service.terminate_upload(id, session).await; + assert!(matches!(terminated, Err(Error::NotImplemented))); + } + + #[tokio::test] + async fn resumable_create_validates_metadata() { + let service = make_service(); + let id = ObjectId::new(make_context(), "resumable".into()); + + // A timeout policy with no resolved `time_expires` is rejected before the backend + // is consulted, exactly as it is for a regular insert. + let metadata = Metadata { + expiration_policy: ExpirationPolicy::TimeToLive(Duration::from_secs(60)), + ..Default::default() + }; + + let result = service.create_upload_session(id, metadata, 1024).await; + assert!(matches!(result, Err(Error::Metadata(_))), "{result:?}"); + } + + /// Backend that accepts resumable uploads and reports a fixed progression. + /// + /// Records nothing: it exists to prove that [`StorageService`] forwards arguments and + /// returns backend outcomes untouched. + #[derive(Clone, Debug)] + struct AcceptResumable { + progress: UploadProgress, + } + + #[async_trait::async_trait] + impl Hooks for AcceptResumable { + async fn create_upload_session( + &self, + _inner: &InMemoryBackend, + _id: &ObjectId, + _metadata: &Metadata, + total_length: u64, + ) -> Result { + Ok(Some( + SessionToken::new(format!("session-{total_length}")).unwrap(), + )) + } + + async fn put_chunk( + &self, + _inner: &InMemoryBackend, + _id: &ObjectId, + _session: &SessionToken, + _offset: u64, + _content_length: u64, + _stream: ClientStream, + ) -> Result { + Ok(self.progress) + } + + async fn upload_offset( + &self, + _inner: &InMemoryBackend, + _id: &ObjectId, + _session: &SessionToken, + ) -> Result { + Ok(self.progress) + } + + async fn terminate_upload( + &self, + _inner: &InMemoryBackend, + _id: &ObjectId, + _session: &SessionToken, + ) -> Result { + Ok(()) + } + } + + fn resumable_service(progress: UploadProgress) -> StorageService { + StorageService::new(Box::new(TestBackend::new(AcceptResumable { progress }))) + } + + #[tokio::test] + async fn resumable_reports_incomplete_progress() { + let service = resumable_service(UploadProgress::Incomplete { offset: 262_144 }); + let id = ObjectId::new(make_context(), "resumable".into()); + + let session = service + .create_upload_session(id.clone(), Metadata::default(), 1024) + .await + .unwrap() + .expect("session was declined"); + assert_eq!(session.as_str(), "session-1024"); + + let progress = service + .put_chunk(id.clone(), session.clone(), 0, 4, stream::single("data")) + .await + .unwrap(); + assert_eq!(progress, UploadProgress::Incomplete { offset: 262_144 }); + + let progress = service.upload_offset(id.clone(), session.clone()).await; + assert_eq!( + progress.unwrap(), + UploadProgress::Incomplete { offset: 262_144 } + ); + + service.terminate_upload(id, session).await.unwrap(); + } + + #[tokio::test] + async fn resumable_reports_commit() { + let service = resumable_service(UploadProgress::Committed); + let id = ObjectId::new(make_context(), "resumable".into()); + let session = SessionToken::new("session".into()).unwrap(); + + let progress = service + .put_chunk(id.clone(), session.clone(), 0, 4, stream::single("data")) + .await + .unwrap(); + assert_eq!(progress, UploadProgress::Committed); + + let progress = service.upload_offset(id, session).await.unwrap(); + assert_eq!(progress, UploadProgress::Committed); + } } diff --git a/objectstore-types/src/lib.rs b/objectstore-types/src/lib.rs index 910f293e..79ce8870 100644 --- a/objectstore-types/src/lib.rs +++ b/objectstore-types/src/lib.rs @@ -11,4 +11,5 @@ pub mod metadata; pub mod multipart; pub mod presign; pub mod range; +pub mod resumable; pub mod scope; diff --git a/objectstore-types/src/resumable.rs b/objectstore-types/src/resumable.rs new file mode 100644 index 00000000..a346d6e3 --- /dev/null +++ b/objectstore-types/src/resumable.rs @@ -0,0 +1,231 @@ +//! Types for the resumable upload protocol. +//! +//! A resumable upload declares the object's total size and metadata upfront, then +//! sends the payload as a sequence of chunks at increasing byte offsets. If a chunk +//! fails, the client asks the server which offset it holds and continues from there. +//! There is no finalize request: the server knows the total length from the session, +//! so it recognizes the chunk carrying the last byte and commits the object itself. +//! +//! Every request addresses the regular object endpoints with the session in the query +//! string. Header names are borrowed from [TUS] where they fit, but this is not a TUS +//! implementation: there is no version negotiation, no capability discovery, and no +//! support for uploads of unknown length. +//! +//! Key types: +//! - [`SessionToken`] — opaque identifier for an in-progress upload session. +//! - [`UploadOffset`] — the value of the [`HEADER_UPLOAD_OFFSET`] header. +//! - [`CreateSessionResponse`] — returned when a new session is created. +//! - [`CommitResponse`] — returned by the request that commits the object. +//! +//! [TUS]: https://tus.io/protocols/resumable-upload + +use std::fmt; +use std::ops::Deref; +use std::path::{Component, Path}; +use std::str::FromStr; + +use serde::{Deserialize, Deserializer, Serialize}; + +/// Request header declaring the total size of the object, in bytes. +/// +/// Required when creating a session. The server needs the total size to select a +/// backend and to recognize the final chunk. +pub const HEADER_UPLOAD_LENGTH: &str = "upload-length"; + +/// Header carrying the byte offset of a chunk, or the offset the server holds. +/// +/// On a request this is the offset of the chunk's first byte, or `*` to query the +/// server's authoritative offset. On a response it is the offset the server has +/// persisted. See [`UploadOffset`]. +pub const HEADER_UPLOAD_OFFSET: &str = "upload-offset"; + +/// The wildcard [`HEADER_UPLOAD_OFFSET`] value that queries the server's offset. +const OFFSET_WILDCARD: &str = "*"; + +/// Identifier for an in-progress resumable upload session. +/// +/// The token is opaque to the client: it is minted by the storage backend and carries +/// whatever that backend needs to continue or commit the upload without shared state. +/// It is neither signed nor encrypted, which is safe because the usecase, scopes and +/// key travel in the request path rather than in the token, so a request cannot address +/// an object other than the one it names. +/// +/// Validated on construction: non-empty and free of path-traversal components (`..`, +/// leading `/`, etc.), so a backend can safely use it as a single path segment. +#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize)] +#[serde(transparent)] +pub struct SessionToken(String); + +/// Error returned when a [`SessionToken`] fails validation. +#[derive(Debug, thiserror::Error)] +#[error("invalid session token: {0}")] +pub struct InvalidSessionToken(String); + +impl SessionToken { + /// Creates a new `SessionToken` after validating the input. + /// + /// # Errors + /// + /// Returns [`InvalidSessionToken`] if the string is empty or contains a component + /// that is not a plain path segment. + pub fn new(s: String) -> Result { + if s.is_empty() { + return Err(InvalidSessionToken("must not be empty".into())); + } + for component in Path::new(&s).components() { + if !matches!(component, Component::Normal(_)) { + return Err(InvalidSessionToken(s)); + } + } + Ok(Self(s)) + } + + /// Returns the session token as a string slice. + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl Deref for SessionToken { + type Target = str; + + fn deref(&self) -> &str { + &self.0 + } +} + +impl fmt::Display for SessionToken { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.fmt(f) + } +} + +impl<'de> Deserialize<'de> for SessionToken { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + Self::new(s).map_err(serde::de::Error::custom) + } +} + +/// The value of the [`HEADER_UPLOAD_OFFSET`] request header. +/// +/// A concrete offset submits a chunk starting at that byte. The wildcard `*` submits +/// no payload and instead asks the server which offset it holds, which is also the +/// request that commits an object that was assembled but not yet committed. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum UploadOffset { + /// `Upload-Offset: ` — a chunk whose first byte sits at this offset. + At(u64), + /// `Upload-Offset: *` — a query for the server's authoritative offset. + Unknown, +} + +/// Error returned when an [`UploadOffset`] header value cannot be parsed. +#[derive(Debug, thiserror::Error)] +#[error("invalid {HEADER_UPLOAD_OFFSET} value: {0}")] +pub struct InvalidUploadOffset(String); + +impl FromStr for UploadOffset { + type Err = InvalidUploadOffset; + + fn from_str(s: &str) -> Result { + if s == OFFSET_WILDCARD { + return Ok(Self::Unknown); + } + + // Rejects the `+` sign and leading whitespace that `u64::from_str` would + // otherwise be lenient about, keeping the header canonical. + if !s.bytes().all(|b| b.is_ascii_digit()) { + return Err(InvalidUploadOffset(s.to_owned())); + } + + let offset = s.parse().map_err(|_| InvalidUploadOffset(s.to_owned()))?; + Ok(Self::At(offset)) + } +} + +impl fmt::Display for UploadOffset { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::At(offset) => offset.fmt(f), + Self::Unknown => f.write_str(OFFSET_WILDCARD), + } + } +} + +/// Response from creating a resumable upload session. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CreateSessionResponse { + /// The object key (server-generated or client-provided). + pub key: String, + /// The session token for subsequent requests. + pub session: SessionToken, +} + +/// Response from the request that commits the object. +/// +/// This is either the chunk carrying the last byte, or an offset query against a +/// session whose object was already assembled. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CommitResponse { + /// The object key. + pub key: String, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn session_token_accepts_opaque_values() -> Result<(), InvalidSessionToken> { + assert_eq!(SessionToken::new("abc123".into())?.as_str(), "abc123"); + assert_eq!( + SessionToken::new("eyJyZXZpc2lvbiI6ImEifQ".into())?.as_str(), + "eyJyZXZpc2lvbiI6ImEifQ" + ); + Ok(()) + } + + #[test] + fn session_token_rejects_empty_and_traversal() { + for invalid in ["", "..", "/abs", "a/../b", "./a"] { + assert!( + SessionToken::new(invalid.into()).is_err(), + "expected {invalid:?} to be rejected" + ); + } + } + + #[test] + fn upload_offset_parses_wildcard_and_offsets() -> Result<(), InvalidUploadOffset> { + assert_eq!("*".parse::()?, UploadOffset::Unknown); + assert_eq!("0".parse::()?, UploadOffset::At(0)); + assert_eq!("262144".parse::()?, UploadOffset::At(262144)); + Ok(()) + } + + #[test] + fn upload_offset_rejects_malformed_values() { + for invalid in ["", "-1", "+1", " 1", "1 ", "1.5", "0x10", "**", "abc"] { + assert!( + invalid.parse::().is_err(), + "expected {invalid:?} to be rejected" + ); + } + } + + #[test] + fn upload_offset_round_trips_through_display() -> Result<(), InvalidUploadOffset> { + for offset in [ + UploadOffset::Unknown, + UploadOffset::At(0), + UploadOffset::At(7), + ] { + assert_eq!(offset.to_string().parse::()?, offset); + } + Ok(()) + } +} From 0c6f8f3077e9495ff808f8f652341df7bc56e262 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 11:45:30 +0200 Subject: [PATCH 02/22] fix(server): Refine resumable upload protocol Encode opaque session tokens as canonical unpadded base64url at the API boundary. Remove Location construction and its OriginalUri plumbing. Return HTTP 501 when the selected backend declines resumable uploads. Allow wildcard offset queries without Content-Length and verify their bodies are empty. Reject wildcard requests carrying payload bytes with HTTP 400. Update protocol documentation and endpoint tests for the revised wire behavior. Refs FS-470 --- objectstore-server/src/endpoints/mod.rs | 13 +- objectstore-server/src/endpoints/objects.rs | 12 +- objectstore-server/src/endpoints/resumable.rs | 115 +++--------------- objectstore-server/tests/resumable.rs | 106 +++++++++++++--- objectstore-types/src/resumable.rs | 90 +++++++++----- 5 files changed, 184 insertions(+), 152 deletions(-) diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index da96257a..982a30df 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -33,6 +33,7 @@ //! //! Resumable uploads use the object endpoints above, selected by a query parameter: //! `upload_type=resumable` opens a session, and `session=` addresses it from then on. +//! Session tokens are unpadded base64url at the API boundary. //! The object is named by the request path as usual, and [`objectstore_types::resumable`] //! holds the protocol types. //! @@ -45,8 +46,8 @@ //! //! Session creation requires an `Upload-Length` header carrying the total size of the object //! in bytes, and takes the same metadata headers as a regular upload. It answers `200 OK` -//! with `{"key", "session"}` and a `Location` header pointing at the object path with the -//! session appended. Metadata is fixed at this point and does not change afterwards. +//! with `{"key", "session"}`; the session field is the token to use in subsequent query +//! parameters. Metadata is fixed at this point and does not change afterwards. //! //! Chunk uploads and offset queries share one request shape, distinguished by the //! `Upload-Offset` header: a byte offset submits the body as the chunk starting there, while @@ -55,6 +56,8 @@ //! `201 Created` with `{"key"}` once the object is committed. **The offset in the response //! may be lower than the end of the chunk that was sent** — backends persist only aligned //! prefixes and discard the remainder — so clients always continue from the returned offset. +//! A chunk requires `Content-Length`; an offset query does not, but its body must still be empty. +//! The server rejects an offset query carrying any body bytes with `400 Bad Request`. //! //! An offset query can commit an object that was assembled but not yet committed, so it //! requires write permission despite being read-shaped. Termination likewise needs write @@ -62,14 +65,14 @@ //! //! | Status | Meaning | Client action | //! |--------|---------|---------------| -//! | `400` | Malformed: unusable session, missing `Upload-Length`, or a chunk exceeding the declared length | Terminal | -//! | `409` | On creation: resumable uploads are unavailable for this object. On a chunk: offset mismatch, with the authoritative offset in `Upload-Offset` | Fall back to a regular upload, or resynchronize | +//! | `400` | Malformed: unusable session, missing `Upload-Length`, nonempty offset query, or a chunk exceeding the declared length | Terminal | +//! | `409` | A chunk's offset does not match, with the authoritative offset in `Upload-Offset` | Resynchronize | //! | `410` | The session expired or was terminated; nothing was retained | Start a new session | //! | `501` | The configured backend does not implement resumable uploads | Fall back to a regular upload | //! //! Not every backend can support this. Session creation asks the backend that would store the //! object to open one, and a backend that cannot declines, which the server reports as -//! `409 Conflict`. No backend implements resumable uploads yet, so every session creation is +//! `501 Not Implemented`. No backend implements resumable uploads yet, so every session creation is //! currently denied. //! //! # Multipart Upload Endpoints diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index 7ee6728e..77e02a34 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -1,7 +1,7 @@ use std::fmt::Write as _; use axum::body::Body; -use axum::extract::{OriginalUri, Query, State}; +use axum::extract::{Query, State}; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::routing; @@ -15,7 +15,7 @@ use serde::Serialize; use crate::auth::AuthAwareService; use crate::endpoints::common::{ApiError, ApiResult, insert_accept_ranges}; -use crate::endpoints::resumable::{self, RequestPath, ResumableQuery, ResumableRoute}; +use crate::endpoints::resumable::{self, ResumableQuery, ResumableRoute}; use crate::extractors::byte_range::OptionalByteRange; use crate::extractors::{Xt, body::MeteredBody}; use crate::state::ServiceState; @@ -44,7 +44,6 @@ async fn objects_post( service: AuthAwareService, State(state): State, Xt(context): Xt, - OriginalUri(uri): OriginalUri, Query(query): Query, headers: HeaderMap, MeteredBody(body): MeteredBody, @@ -54,8 +53,7 @@ async fn objects_post( match query.classify()? { ResumableRoute::Create => { let id = ObjectId::optional(context, None); - let path = RequestPath::Collection; - return resumable::create_session(service, state, path, uri.path(), id, headers).await; + return resumable::create_session(service, state, id, headers).await; } ResumableRoute::Session(_) => { return Err(ApiError::Client( @@ -218,7 +216,6 @@ async fn object_put( service: AuthAwareService, State(state): State, Xt(id): Xt, - OriginalUri(uri): OriginalUri, Query(query): Query, headers: HeaderMap, body: MeteredBody, @@ -228,8 +225,7 @@ async fn object_put( // bodyless paths. match query.classify()? { ResumableRoute::Create => { - let path = RequestPath::Object; - return resumable::create_session(service, state, path, uri.path(), id, headers).await; + return resumable::create_session(service, state, id, headers).await; } ResumableRoute::Session(session) => { return resumable::session_request(service, id, session, headers, body).await; diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index 897e0207..ff10b689 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -4,11 +4,12 @@ //! resource, following GCS and S3 rather than [TUS]. Every request addresses the same object //! path with the session in the query string, so these handlers have no router of their own: //! [`objects`](super::objects) dispatches to them based on [`ResumableQuery`]. +//! Session tokens are encoded as unpadded base64url at this API boundary. //! //! | Operation | Request | Success | //! |---|---|---| -//! | Create | `POST /objects/{usecase}/{scopes}/?upload_type=resumable` | `200` + `Location` + `{"key","session"}` | -//! | Create | `PUT /objects/{usecase}/{scopes}/{key}?upload_type=resumable` | `200` + `Location` + `{"key","session"}` | +//! | Create | `POST /objects/{usecase}/{scopes}/?upload_type=resumable` | `200` + `{"key","session"}` | +//! | Create | `PUT /objects/{usecase}/{scopes}/{key}?upload_type=resumable` | `200` + `{"key","session"}` | //! | Chunk | `PUT …/{key}?session=` with `Upload-Offset: ` | `204` + `Upload-Offset`, or `201` + `{"key"}` | //! | Offset query | `PUT …/{key}?session=` with `Upload-Offset: *` | `204` + `Upload-Offset`, or `201` + `{"key"}` | //! | Terminate | `DELETE …/{key}?session=` | `204` | @@ -17,13 +18,14 @@ //! backend recognizes the chunk carrying the last byte and commits the object itself. //! //! Not every backend supports this. When one declines, session creation answers -//! `409 Conflict` and the client performs a regular upload instead. +//! `501 Not Implemented` and the client performs a regular upload instead. //! //! [TUS]: https://tus.io/protocols/resumable-upload use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::{Json, http}; +use futures_util::TryStreamExt; use objectstore_service::error::Error as ServiceError; use objectstore_service::id::ObjectId; use objectstore_service::resumable::{SessionToken, UploadOffset, UploadProgress}; @@ -153,28 +155,13 @@ fn content_length(headers: &HeaderMap) -> ApiResult { .ok_or_else(|| ApiError::Client("Content-Length header is required".into())) } -/// How the request path relates to the object a session is being created for. -/// -/// Needed to build the `Location` header, which must always name the object even when the -/// request did not. -#[derive(Clone, Copy, Debug)] -pub(super) enum RequestPath { - /// The request path names the object, as on a `PUT` to the object route. - Object, - /// The request path is the collection the object lives in, as on a `POST` whose key was - /// generated by the server and therefore does not appear in the path. - Collection, -} - /// Creates a session for the object at `id`. /// -/// Answers `409 Conflict` when the backend declines, which tells the client to fall back to a -/// regular upload. Metadata is declared here and does not change afterwards. +/// Answers `501 Not Implemented` when the backend declines, which tells the client to fall back +/// to a regular upload. Metadata is declared here and does not change afterwards. pub(super) async fn create_session( service: AuthAwareService, state: ServiceState, - request_path: RequestPath, - uri_path: &str, id: ObjectId, headers: HeaderMap, ) -> ApiResult { @@ -187,48 +174,16 @@ pub(super) async fn create_session( .validate(&id.context().usecase, &metadata) .map_err(|e| ApiError::Client(e.to_string()))?; - let Some(session) = service + let session = service .create_upload_session(id.clone(), metadata, total_length) .await? - else { - let body = ApiErrorResponse::message("resumable uploads are unavailable for this object"); - return Ok((StatusCode::CONFLICT, Json(body)).into_response()); - }; - - let mut headers = HeaderMap::new(); - if let Some(location) = session_location(uri_path, request_path, id.key(), &session) { - headers.insert(http::header::LOCATION, location); - } + .ok_or(ServiceError::NotImplemented)?; let body = Json(CreateSessionResponse { key: id.key().to_owned(), session, }); - Ok((StatusCode::OK, headers, body).into_response()) -} - -/// Builds the `Location` header pointing at the session. -/// -/// The value is the object path with the session appended, so a client that persisted the key -/// and the session can rebuild it without having stored a URL. `Router::nest` strips the `/v1` -/// prefix from the request URI, so callers pass the path from -/// [`OriginalUri`](axum::extract::OriginalUri) — and on the collection route that path does -/// not name the object, so the key is appended to it. -/// -/// Returns `None` if the result is not a valid header value, in which case the header is -/// omitted: it is a convenience, and the response body carries the same information. -fn session_location( - uri_path: &str, - request_path: RequestPath, - key: &str, - session: &SessionToken, -) -> Option { - let object_path = match request_path { - RequestPath::Object => uri_path.to_owned(), - RequestPath::Collection => format!("{}/{key}", uri_path.trim_end_matches('/')), - }; - - http::HeaderValue::from_str(&format!("{object_path}?session={session}")).ok() + Ok((StatusCode::OK, body).into_response()) } /// Acts on an open session: writes a chunk, or reports the offset the server holds. @@ -236,6 +191,8 @@ fn session_location( /// [`HEADER_UPLOAD_OFFSET`] selects between the two. A concrete offset submits the request /// body as the chunk starting there; the `*` wildcard submits nothing and asks where the /// server stands, which also commits an object that was assembled but not yet committed. +/// Chunks require `Content-Length`. Offset queries may omit it, but the body stream is checked +/// and any bytes are rejected as a malformed request. /// /// Both answer `204 No Content` with the authoritative offset while bytes remain, and /// `201 Created` with the key once the object is committed. @@ -244,14 +201,14 @@ pub(super) async fn session_request( id: ObjectId, session: SessionToken, headers: HeaderMap, - MeteredBody(body): MeteredBody, + MeteredBody(mut body): MeteredBody, ) -> ApiResult { let offset = upload_offset(&headers)?; - let content_length = content_length(&headers)?; let key = id.key().to_owned(); let progress = match offset { UploadOffset::At(offset) => { + let content_length = content_length(&headers)?; service .put_chunk(id, session, offset, content_length, body) .await @@ -259,10 +216,12 @@ pub(super) async fn session_request( UploadOffset::Unknown => { // The wildcard carries no payload. A body would be silently discarded, so // reject it rather than let a client believe those bytes were written. - if content_length != 0 { - return Err(ApiError::Client(format!( - "{HEADER_UPLOAD_OFFSET}: * must be sent with an empty body" - ))); + while let Some(chunk) = body.try_next().await.map_err(ServiceError::from)? { + if !chunk.is_empty() { + return Err(ApiError::Client(format!( + "{HEADER_UPLOAD_OFFSET}: * must be sent with an empty body" + ))); + } } service.upload_offset(id, session).await @@ -454,38 +413,4 @@ mod tests { let error = progress_response(Err(invalid), "my-key".into()).unwrap_err(); assert_eq!(error.status(), StatusCode::BAD_REQUEST); } - - #[test] - fn session_location_appends_the_session_to_the_object_path() { - let session = SessionToken::new("tok3n".into()).unwrap(); - let location = session_location( - "/v1/objects/testing/org=1/my-key", - RequestPath::Object, - "my-key", - &session, - ); - - assert_eq!( - location.unwrap(), - "/v1/objects/testing/org=1/my-key?session=tok3n" - ); - } - - #[test] - fn session_location_appends_a_generated_key_to_the_collection_path() { - let session = SessionToken::new("tok3n".into()).unwrap(); - - // The `POST` route matches with and without a trailing slash, and the generated key - // never appears in the request path — so it has to be appended either way. - for collection in ["/v1/objects/testing/org=1/", "/v1/objects/testing/org=1"] { - let location = - session_location(collection, RequestPath::Collection, "generated", &session); - - assert_eq!( - location.unwrap(), - "/v1/objects/testing/org=1/generated?session=tok3n", - "for request path {collection:?}" - ); - } - } } diff --git a/objectstore-server/tests/resumable.rs b/objectstore-server/tests/resumable.rs index 72d6a4bc..63b072c7 100644 --- a/objectstore-server/tests/resumable.rs +++ b/objectstore-server/tests/resumable.rs @@ -1,19 +1,25 @@ //! Integration tests for the resumable upload endpoints. //! //! No backend implements resumable uploads yet, so the reachable surface is session denial -//! and request validation. That is deliberate: a deployment must answer `409 Conflict` to +//! and request validation. That is deliberate: a deployment must answer `501 Not Implemented` to //! every session creation so clients fall back to a regular upload, and it must reject a //! malformed request before it reaches a backend. //! //! The `501 Not Implemented` assertions are the proof that dispatch and header parsing work: //! the only way to reach a declining backend method is through a well-formed request. +use std::io::{Read, Write}; +use std::net::TcpStream; + use anyhow::Result; use objectstore_server::config::{AuthZ, Config}; use objectstore_test::server::TestServer; use objectstore_types::resumable::{HEADER_UPLOAD_LENGTH, HEADER_UPLOAD_OFFSET}; use reqwest::StatusCode; +/// Unpadded base64url for the opaque backend token `some-token`. +const SESSION: &str = "c29tZS10b2tlbg"; + async fn test_server() -> TestServer { TestServer::with_config(Config { auth: AuthZ { @@ -25,10 +31,41 @@ async fn test_server() -> TestServer { .await } +/// Sends a raw HTTP/1.1 `PUT`, preserving the caller's exact body framing headers. +async fn raw_put(server: &TestServer, path: &str, headers: &str, body: &str) -> Result { + let url = reqwest::Url::parse(&server.url(path))?; + let host = url + .host_str() + .expect("test server URL has a host") + .to_owned(); + let port = url + .port_or_known_default() + .expect("test server URL has a port"); + let target = match url.query() { + Some(query) => format!("{}?{query}", url.path()), + None => url.path().to_owned(), + }; + let headers = headers.to_owned(); + let body = body.to_owned(); + + tokio::task::spawn_blocking(move || -> Result { + let mut stream = TcpStream::connect((host.as_str(), port))?; + write!( + stream, + "PUT {target} HTTP/1.1\r\nHost: {host}:{port}\r\n{headers}Connection: close\r\n\r\n{body}" + )?; + + let mut response = String::new(); + stream.read_to_string(&mut response)?; + Ok(response) + }) + .await? +} + // --- Session creation --- #[tokio::test] -async fn create_session_is_denied_with_client_key() -> Result<()> { +async fn unsupported_create_session_with_client_key_is_not_implemented() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() @@ -37,12 +74,12 @@ async fn create_session_is_denied_with_client_key() -> Result<()> { .send() .await?; - assert_eq!(response.status(), StatusCode::CONFLICT); + assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); Ok(()) } #[tokio::test] -async fn create_session_is_denied_with_generated_key() -> Result<()> { +async fn unsupported_create_session_with_generated_key_is_not_implemented() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() @@ -51,7 +88,7 @@ async fn create_session_is_denied_with_generated_key() -> Result<()> { .send() .await?; - assert_eq!(response.status(), StatusCode::CONFLICT); + assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); Ok(()) } @@ -111,7 +148,7 @@ async fn chunk_reaches_the_declining_backend() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() - .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .put(server.url(&format!("/v1/objects/test/org=1/my-key?session={SESSION}"))) .header(HEADER_UPLOAD_OFFSET, "0") .body("payload") .send() @@ -126,9 +163,8 @@ async fn offset_query_reaches_the_declining_backend() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() - .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .put(server.url(&format!("/v1/objects/test/org=1/my-key?session={SESSION}"))) .header(HEADER_UPLOAD_OFFSET, "*") - .header(reqwest::header::CONTENT_LENGTH, "0") .send() .await?; @@ -136,12 +172,48 @@ async fn offset_query_reaches_the_declining_backend() -> Result<()> { Ok(()) } +#[tokio::test] +async fn offset_query_allows_missing_content_length() -> Result<()> { + let server = test_server().await; + let response = raw_put( + &server, + &format!("/v1/objects/test/org=1/my-key?session={SESSION}"), + &format!("{HEADER_UPLOAD_OFFSET}: *\r\n"), + "", + ) + .await?; + + assert!( + response.starts_with("HTTP/1.1 501 Not Implemented\r\n"), + "unexpected response: {response}" + ); + Ok(()) +} + +#[tokio::test] +async fn offset_query_rejects_chunked_body_without_content_length() -> Result<()> { + let server = test_server().await; + let response = raw_put( + &server, + &format!("/v1/objects/test/org=1/my-key?session={SESSION}"), + &format!("{HEADER_UPLOAD_OFFSET}: *\r\nTransfer-Encoding: chunked\r\n"), + "7\r\npayload\r\n0\r\n\r\n", + ) + .await?; + + assert!( + response.starts_with("HTTP/1.1 400 Bad Request\r\n"), + "unexpected response: {response}" + ); + Ok(()) +} + #[tokio::test] async fn chunk_requires_upload_offset() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() - .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .put(server.url(&format!("/v1/objects/test/org=1/my-key?session={SESSION}"))) .body("payload") .send() .await?; @@ -157,7 +229,7 @@ async fn chunk_rejects_malformed_upload_offset() -> Result<()> { for invalid in ["", "-1", "1.5", "**", "here"] { let response = client - .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .put(server.url(&format!("/v1/objects/test/org=1/my-key?session={SESSION}"))) .header(HEADER_UPLOAD_OFFSET, invalid) .body("payload") .send() @@ -178,7 +250,7 @@ async fn offset_query_rejects_a_payload() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() - .put(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .put(server.url(&format!("/v1/objects/test/org=1/my-key?session={SESSION}"))) .header(HEADER_UPLOAD_OFFSET, "*") .body("payload") .send() @@ -189,11 +261,11 @@ async fn offset_query_rejects_a_payload() -> Result<()> { } #[tokio::test] -async fn session_token_with_path_traversal_is_rejected() -> Result<()> { +async fn session_token_requires_base64url() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() - .put(server.url("/v1/objects/test/org=1/my-key?session=../escape")) + .put(server.url("/v1/objects/test/org=1/my-key?session=%25%25%25")) .header(HEADER_UPLOAD_OFFSET, "0") .body("payload") .send() @@ -210,7 +282,7 @@ async fn terminate_reaches_the_declining_backend() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() - .delete(server.url("/v1/objects/test/org=1/my-key?session=some-token")) + .delete(server.url(&format!("/v1/objects/test/org=1/my-key?session={SESSION}"))) .send() .await?; @@ -238,7 +310,9 @@ async fn upload_type_and_session_are_mutually_exclusive() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() - .put(server.url("/v1/objects/test/org=1/my-key?upload_type=resumable&session=some-token")) + .put(server.url(&format!( + "/v1/objects/test/org=1/my-key?upload_type=resumable&session={SESSION}" + ))) .header(HEADER_UPLOAD_LENGTH, "1048576") .header(HEADER_UPLOAD_OFFSET, "0") .send() @@ -253,7 +327,7 @@ async fn session_on_the_collection_route_is_rejected() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() - .post(server.url("/v1/objects/test/org=1/?session=some-token")) + .post(server.url(&format!("/v1/objects/test/org=1/?session={SESSION}"))) .header(HEADER_UPLOAD_OFFSET, "0") .body("payload") .send() diff --git a/objectstore-types/src/resumable.rs b/objectstore-types/src/resumable.rs index a346d6e3..befbaf13 100644 --- a/objectstore-types/src/resumable.rs +++ b/objectstore-types/src/resumable.rs @@ -7,9 +7,10 @@ //! so it recognizes the chunk carrying the last byte and commits the object itself. //! //! Every request addresses the regular object endpoints with the session in the query -//! string. Header names are borrowed from [TUS] where they fit, but this is not a TUS -//! implementation: there is no version negotiation, no capability discovery, and no -//! support for uploads of unknown length. +//! string. [`SessionToken`] serializes as unpadded base64url at that API boundary. Header +//! names are borrowed from [TUS] where they fit, but this is not a TUS implementation: there +//! is no version negotiation, no capability discovery, and no support for uploads of unknown +//! length. //! //! Key types: //! - [`SessionToken`] — opaque identifier for an in-progress upload session. @@ -21,10 +22,11 @@ use std::fmt; use std::ops::Deref; -use std::path::{Component, Path}; use std::str::FromStr; -use serde::{Deserialize, Deserializer, Serialize}; +use base64::Engine as _; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; /// Request header declaring the total size of the object, in bytes. /// @@ -50,10 +52,11 @@ const OFFSET_WILDCARD: &str = "*"; /// key travel in the request path rather than in the token, so a request cannot address /// an object other than the one it names. /// -/// Validated on construction: non-empty and free of path-traversal components (`..`, -/// leading `/`, etc.), so a backend can safely use it as a single path segment. -#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize)] -#[serde(transparent)] +/// Validated on construction only to ensure it is non-empty. Its contents are otherwise opaque: +/// backends may use any UTF-8 string, including path separators and traversal-like text. +/// At the API boundary it is serialized as unpadded base64url, keeping the opaque value out of +/// URL parsing and escaping rules. +#[derive(Clone, Debug, PartialEq, Eq, Hash)] pub struct SessionToken(String); /// Error returned when a [`SessionToken`] fails validation. @@ -66,17 +69,11 @@ impl SessionToken { /// /// # Errors /// - /// Returns [`InvalidSessionToken`] if the string is empty or contains a component - /// that is not a plain path segment. + /// Returns [`InvalidSessionToken`] if the string is empty. pub fn new(s: String) -> Result { if s.is_empty() { return Err(InvalidSessionToken("must not be empty".into())); } - for component in Path::new(&s).components() { - if !matches!(component, Component::Normal(_)) { - return Err(InvalidSessionToken(s)); - } - } Ok(Self(s)) } @@ -100,13 +97,32 @@ impl fmt::Display for SessionToken { } } +impl Serialize for SessionToken { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&URL_SAFE_NO_PAD.encode(self.0.as_bytes())) + } +} + impl<'de> Deserialize<'de> for SessionToken { fn deserialize(deserializer: D) -> Result where D: Deserializer<'de>, { - let s = String::deserialize(deserializer)?; - Self::new(s).map_err(serde::de::Error::custom) + let encoded = String::deserialize(deserializer)?; + let bytes = URL_SAFE_NO_PAD + .decode(&encoded) + .map_err(serde::de::Error::custom)?; + if URL_SAFE_NO_PAD.encode(&bytes) != encoded { + return Err(serde::de::Error::custom( + "session token must use canonical unpadded base64url", + )); + } + + let token = String::from_utf8(bytes).map_err(serde::de::Error::custom)?; + Self::new(token).map_err(serde::de::Error::custom) } } @@ -161,7 +177,7 @@ impl fmt::Display for UploadOffset { pub struct CreateSessionResponse { /// The object key (server-generated or client-provided). pub key: String, - /// The session token for subsequent requests. + /// The session token for subsequent requests, serialized as unpadded base64url. pub session: SessionToken, } @@ -181,20 +197,38 @@ mod tests { #[test] fn session_token_accepts_opaque_values() -> Result<(), InvalidSessionToken> { - assert_eq!(SessionToken::new("abc123".into())?.as_str(), "abc123"); - assert_eq!( - SessionToken::new("eyJyZXZpc2lvbiI6ImEifQ".into())?.as_str(), - "eyJyZXZpc2lvbiI6ImEifQ" - ); + for value in ["abc123", "..", "/abs", "a/../b", "./a", "a/", "opaque +? ü"] { + assert_eq!(SessionToken::new(value.into())?.as_str(), value); + } Ok(()) } #[test] - fn session_token_rejects_empty_and_traversal() { - for invalid in ["", "..", "/abs", "a/../b", "./a"] { + fn session_token_rejects_empty() { + assert!(SessionToken::new(String::new()).is_err()); + } + + #[test] + fn session_token_serializes_as_unpadded_base64url() -> Result<(), Box> { + let token = SessionToken::new("tok3n".into())?; + assert_eq!(serde_json::to_string(&token)?, r#""dG9rM24""#); + + let decoded: SessionToken = serde_json::from_str(r#""dG9rM24""#)?; + assert_eq!(decoded, token); + + let opaque = SessionToken::new("../escape".into())?; + assert_eq!(serde_json::to_string(&opaque)?, r#""Li4vZXNjYXBl""#); + let decoded: SessionToken = serde_json::from_str(r#""Li4vZXNjYXBl""#)?; + assert_eq!(decoded, opaque); + Ok(()) + } + + #[test] + fn session_token_rejects_invalid_api_encodings() { + for invalid in [r#""%%%""#, r#""dG9rM24=""#, r#""_w""#] { assert!( - SessionToken::new(invalid.into()).is_err(), - "expected {invalid:?} to be rejected" + serde_json::from_str::(invalid).is_err(), + "accepted {invalid}" ); } } From 0df327459b1eb24ff7606da2f7b95a671d3d3221 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:19:06 +0200 Subject: [PATCH 03/22] ref(resumable): Dispatch requests through dedicated handlers Route query-selected object operations through dedicated Axum handlers. Give each operation a focused extractor set. Model resumable selection as an optional target. Forward decoded session tokens through request extensions to avoid parsing them twice. Align endpoint and service terminology for object insertion and upload cancellation. Update backend hooks, response types, telemetry labels, documentation, and tests. --- objectstore-server/src/auth/service.rs | 12 +- objectstore-server/src/endpoints/mod.rs | 4 +- objectstore-server/src/endpoints/objects.rs | 141 +++++++++++------- objectstore-server/src/endpoints/resumable.rs | 120 +++++++-------- objectstore-server/tests/resumable.rs | 18 ++- objectstore-service/docs/architecture.md | 2 +- objectstore-service/src/backend/common.rs | 12 +- objectstore-service/src/backend/counting.rs | 10 +- objectstore-service/src/backend/testing.rs | 18 +-- objectstore-service/src/error.rs | 2 +- objectstore-service/src/resumable.rs | 5 +- objectstore-service/src/service.rs | 24 ++- 12 files changed, 198 insertions(+), 170 deletions(-) diff --git a/objectstore-server/src/auth/service.rs b/objectstore-server/src/auth/service.rs index cee66915..e956a31a 100644 --- a/objectstore-server/src/auth/service.rs +++ b/objectstore-server/src/auth/service.rs @@ -4,7 +4,7 @@ use objectstore_service::multipart::{ ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; use objectstore_service::resumable::{ - CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, + CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress, }; use objectstore_service::service::{DeleteResponse, GetResponse, InsertResponse, MetadataResponse}; @@ -193,7 +193,7 @@ impl AuthAwareService { // --- Resumable upload operations --- // // Every operation requires `ObjectWrite`, including the two that do not obviously write: - // an offset query can commit an assembled object, and terminating a session discards an + // an offset query can commit an assembled object, and canceling a session discards an // in-progress upload rather than deleting an object. So `DELETE ?session=` needs write // permission where a plain `DELETE` on the same path needs delete permission. @@ -237,13 +237,13 @@ impl AuthAwareService { Ok(self.service.upload_offset(id, session).await?) } - /// Auth-aware wrapper around [`StorageService::terminate_upload`]. - pub async fn terminate_upload( + /// Auth-aware wrapper around [`StorageService::cancel_upload`]. + pub async fn cancel_upload( &self, id: ObjectId, session: SessionToken, - ) -> ApiResult { + ) -> ApiResult { self.check_permission(Permission::ObjectWrite, id.context())?; - Ok(self.service.terminate_upload(id, session).await?) + Ok(self.service.cancel_upload(id, session).await?) } } diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index 982a30df..4b4cb71d 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -42,7 +42,7 @@ //! | `POST` | `/v1/objects/{usecase}/{scopes}/?upload_type=resumable` | Create session (server-generated key) | //! | `PUT` | `/v1/objects/{usecase}/{scopes}/{*key}?upload_type=resumable` | Create session (user-provided key) | //! | `PUT` | `/v1/objects/{usecase}/{scopes}/{*key}?session=` | Upload a chunk, or query the offset | -//! | `DELETE` | `/v1/objects/{usecase}/{scopes}/{*key}?session=` | Terminate session, discarding what was sent | +//! | `DELETE` | `/v1/objects/{usecase}/{scopes}/{*key}?session=` | Cancel upload, discarding what was sent | //! //! Session creation requires an `Upload-Length` header carrying the total size of the object //! in bytes, and takes the same metadata headers as a regular upload. It answers `200 OK` @@ -67,7 +67,7 @@ //! |--------|---------|---------------| //! | `400` | Malformed: unusable session, missing `Upload-Length`, nonempty offset query, or a chunk exceeding the declared length | Terminal | //! | `409` | A chunk's offset does not match, with the authoritative offset in `Upload-Offset` | Resynchronize | -//! | `410` | The session expired or was terminated; nothing was retained | Start a new session | +//! | `410` | The session expired or was canceled; nothing was retained | Start a new session | //! | `501` | The configured backend does not implement resumable uploads | Fall back to a regular upload | //! //! Not every backend can support this. Session creation asks the backend that would store the diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index 77e02a34..16b3a519 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -1,11 +1,12 @@ use std::fmt::Write as _; use axum::body::Body; -use axum::extract::{Query, State}; +use axum::extract::{Query, Request, State}; +use axum::handler::Handler; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::routing; -use axum::{Json, Router}; +use axum::{Json, RequestExt, Router}; use objectstore_service::error::Error as ServiceError; use objectstore_service::id::{ObjectContext, ObjectId}; use objectstore_types::headers::ExtValue; @@ -15,18 +16,18 @@ use serde::Serialize; use crate::auth::AuthAwareService; use crate::endpoints::common::{ApiError, ApiResult, insert_accept_ranges}; -use crate::endpoints::resumable::{self, ResumableQuery, ResumableRoute}; +use crate::endpoints::resumable::{self, ResumableQuery, ResumableTarget}; use crate::extractors::byte_range::OptionalByteRange; use crate::extractors::{Xt, body::MeteredBody}; use crate::state::ServiceState; pub fn router() -> Router { - let collection_routes = routing::post(objects_post); + let collection_routes = routing::post(dispatch_objects_post); let object_routes = routing::get(object_get) .head(object_head) - .put(object_put) + .put(dispatch_object_put) // TODO(ja): Implement PATCH (metadata update w/o body) - .delete(object_delete); + .delete(dispatch_object_delete); Router::new() .route("/objects/{usecase}/{scopes}", collection_routes.clone()) @@ -34,35 +35,98 @@ pub fn router() -> Router { .route("/objects/{usecase}/{scopes}/{*key}", object_routes) } +/// Extracts which resumable session, if any, the request targets. +/// +/// Returns `None` without parsing when the URI has no query string. +/// +/// Parsing a query does not consume the request body, so the selected handler can still extract +/// it. +async fn extract_resumable_target( + request: &mut Request, +) -> Result, Response> { + if request.uri().query().is_none() { + return Ok(None); + } + + let Query(query) = request + .extract_parts::>() + .await + .map_err(|rejection| rejection.into_response())?; + + query.classify().map_err(IntoResponse::into_response) +} + +async fn dispatch_objects_post( + State(state): State, + mut request: Request, +) -> Response { + let target = match extract_resumable_target(&mut request).await { + Ok(target) => target, + Err(response) => return response, + }; + + match target { + Some(ResumableTarget::NewSession) => resumable::create_session.call(request, state).await, + Some(ResumableTarget::ExistingSession(_)) => { + ApiError::Client("`session` requires an object key; use PUT on the object path".into()) + .into_response() + } + None => create_object.call(request, state).await, + } +} + +async fn dispatch_object_put(State(state): State, mut request: Request) -> Response { + let target = match extract_resumable_target(&mut request).await { + Ok(target) => target, + Err(response) => return response, + }; + + match target { + Some(ResumableTarget::NewSession) => { + resumable::create_session_for_key.call(request, state).await + } + Some(ResumableTarget::ExistingSession(session)) => { + request.extensions_mut().insert(session); + resumable::continue_session.call(request, state).await + } + None => insert_object.call(request, state).await, + } +} + +async fn dispatch_object_delete( + State(state): State, + mut request: Request, +) -> Response { + let target = match extract_resumable_target(&mut request).await { + Ok(target) => target, + Err(response) => return response, + }; + + match target { + Some(ResumableTarget::ExistingSession(session)) => { + request.extensions_mut().insert(session); + resumable::cancel_session.call(request, state).await + } + Some(ResumableTarget::NewSession) => { + ApiError::Client("`upload_type` is not supported on DELETE".into()).into_response() + } + None => delete_object.call(request, state).await, + } +} + /// Response returned when inserting an object. #[derive(Debug, Serialize)] pub struct InsertObjectResponse { pub key: String, } -async fn objects_post( +async fn create_object( service: AuthAwareService, State(state): State, Xt(context): Xt, - Query(query): Query, headers: HeaderMap, MeteredBody(body): MeteredBody, ) -> ApiResult { - // A chunk always addresses a resolved key, so `?session=` has no meaning on the - // collection route. `?upload_type=resumable` creates a session for a generated key. - match query.classify()? { - ResumableRoute::Create => { - let id = ObjectId::optional(context, None); - return resumable::create_session(service, state, id, headers).await; - } - ResumableRoute::Session(_) => { - return Err(ApiError::Client( - "`session` requires an object key; use PUT on the object path".into(), - )); - } - ResumableRoute::Regular => {} - } - let metadata = Metadata::from_insert_headers(&headers, "").map_err(ServiceError::from)?; state @@ -212,28 +276,13 @@ fn format_content_disposition(filename: &str) -> http::HeaderValue { http::HeaderValue::from_str(&result).expect("content disposition is a valid header value") } -async fn object_put( +async fn insert_object( service: AuthAwareService, State(state): State, Xt(id): Xt, - Query(query): Query, headers: HeaderMap, - body: MeteredBody, + MeteredBody(body): MeteredBody, ) -> ApiResult { - // `PUT` carries all three write shapes: create a session, write a chunk, query the - // offset. `MeteredBody` is extracted unconditionally and dropped unread on the two - // bodyless paths. - match query.classify()? { - ResumableRoute::Create => { - return resumable::create_session(service, state, id, headers).await; - } - ResumableRoute::Session(session) => { - return resumable::session_request(service, id, session, headers, body).await; - } - ResumableRoute::Regular => {} - } - - let MeteredBody(body) = body; let metadata = Metadata::from_insert_headers(&headers, "").map_err(ServiceError::from)?; let ObjectId { context, key } = id; @@ -255,17 +304,7 @@ async fn object_put( Ok((StatusCode::OK, response).into_response()) } -async fn object_delete( - service: AuthAwareService, - Xt(id): Xt, - Query(query): Query, -) -> ApiResult { - // With a session this terminates the upload; without one it deletes the object, as it - // always has. Note the two need different permissions — see `AuthAwareService`. - if let ResumableRoute::Session(session) = query.classify_session_only("DELETE")? { - return resumable::terminate(service, id, session).await; - } - +async fn delete_object(service: AuthAwareService, Xt(id): Xt) -> ApiResult { service.delete_object(id).await?; Ok(StatusCode::NO_CONTENT.into_response()) } diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index ff10b689..ba1e23be 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -3,7 +3,8 @@ //! Resumable uploads are a variation of the regular object endpoints rather than a separate //! resource, following GCS and S3 rather than [TUS]. Every request addresses the same object //! path with the session in the query string, so these handlers have no router of their own: -//! [`objects`](super::objects) dispatches to them based on [`ResumableQuery`]. +//! thin handlers in [`objects`](super::objects) inspect [`ResumableQuery`] and dispatch the +//! original request here, where each operation runs with its own Axum extractors. //! Session tokens are encoded as unpadded base64url at this API boundary. //! //! | Operation | Request | Success | @@ -12,7 +13,7 @@ //! | Create | `PUT /objects/{usecase}/{scopes}/{key}?upload_type=resumable` | `200` + `{"key","session"}` | //! | Chunk | `PUT …/{key}?session=` with `Upload-Offset: ` | `204` + `Upload-Offset`, or `201` + `{"key"}` | //! | Offset query | `PUT …/{key}?session=` with `Upload-Offset: *` | `204` + `Upload-Offset`, or `201` + `{"key"}` | -//! | Terminate | `DELETE …/{key}?session=` | `204` | +//! | Cancel | `DELETE …/{key}?session=` | `204` | //! //! There is no completion request. The total size is known from session creation, so the //! backend recognizes the chunk carrying the last byte and commits the object itself. @@ -22,12 +23,13 @@ //! //! [TUS]: https://tus.io/protocols/resumable-upload +use axum::extract::{Extension, State}; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::{Json, http}; use futures_util::TryStreamExt; use objectstore_service::error::Error as ServiceError; -use objectstore_service::id::ObjectId; +use objectstore_service::id::{ObjectContext, ObjectId}; use objectstore_service::resumable::{SessionToken, UploadOffset, UploadProgress}; use objectstore_types::metadata::Metadata; use objectstore_types::resumable::{ @@ -37,7 +39,7 @@ use serde::Deserialize; use crate::auth::AuthAwareService; use crate::endpoints::common::{ApiError, ApiErrorResponse, ApiResult}; -use crate::extractors::body::MeteredBody; +use crate::extractors::{Xt, body::MeteredBody}; use crate::state::ServiceState; /// The `upload_type` query parameter. @@ -59,19 +61,20 @@ pub(super) enum UploadType { pub(super) struct ResumableQuery { /// Present on a session creation request. upload_type: Option, - /// Present on a chunk write, offset query, or termination. + /// Present on a chunk write, offset query, or cancellation. session: Option, } -/// What a request on an object route is addressing. +/// Which resumable session a request on an object route targets. #[derive(Debug)] -pub(super) enum ResumableRoute { - /// Create a session for the object named by the request path. - Create, - /// Act on the identified session: write a chunk, query the offset, or terminate. - Session(SessionToken), - /// A regular object request that does not involve the resumable protocol. - Regular, +pub(super) enum ResumableTarget { + /// A new session to create for the object addressed by the request. + NewSession, + /// An existing session to continue or cancel. + /// + /// The dispatcher moves the token into the request extensions before calling the selected + /// handler, avoiding a second query-string deserialization. + ExistingSession(SessionToken), } impl ResumableQuery { @@ -81,35 +84,14 @@ impl ResumableQuery { /// /// Returns [`ApiError::Client`] if both parameters are present. They address different /// operations, so a request carrying both is ambiguous rather than defaulted. - pub fn classify(self) -> ApiResult { + pub fn classify(self) -> ApiResult> { match (self.upload_type, self.session) { (Some(_), Some(_)) => Err(ApiError::Client( "`upload_type` and `session` are mutually exclusive".into(), )), - (Some(UploadType::Resumable), None) => Ok(ResumableRoute::Create), - (None, Some(session)) => Ok(ResumableRoute::Session(session)), - (None, None) => Ok(ResumableRoute::Regular), - } - } - - /// Classifies a request that may only act on an existing session. - /// - /// Used by routes where session creation is not defined: `DELETE`, which terminates, and - /// the collection `POST`, whose generated key is only known once a session exists. - /// - /// # Errors - /// - /// Returns [`ApiError::Client`] if `upload_type` is present. - pub fn classify_session_only(self, operation: &str) -> ApiResult { - if self.upload_type.is_some() { - return Err(ApiError::Client(format!( - "`upload_type` is not supported on {operation}" - ))); - } - - match self.session { - Some(session) => Ok(ResumableRoute::Session(session)), - None => Ok(ResumableRoute::Regular), + (Some(UploadType::Resumable), None) => Ok(Some(ResumableTarget::NewSession)), + (None, Some(session)) => Ok(Some(ResumableTarget::ExistingSession(session))), + (None, None) => Ok(None), } } } @@ -155,11 +137,31 @@ fn content_length(headers: &HeaderMap) -> ApiResult { .ok_or_else(|| ApiError::Client("Content-Length header is required".into())) } +/// Creates a session with a server-generated object key. +pub(super) async fn create_session( + service: AuthAwareService, + State(state): State, + Xt(context): Xt, + headers: HeaderMap, +) -> ApiResult { + create_session_for_id(service, state, ObjectId::optional(context, None), headers).await +} + +/// Creates a session for the object key in the request path. +pub(super) async fn create_session_for_key( + service: AuthAwareService, + State(state): State, + Xt(id): Xt, + headers: HeaderMap, +) -> ApiResult { + create_session_for_id(service, state, id, headers).await +} + /// Creates a session for the object at `id`. /// /// Answers `501 Not Implemented` when the backend declines, which tells the client to fall back /// to a regular upload. Metadata is declared here and does not change afterwards. -pub(super) async fn create_session( +async fn create_session_for_id( service: AuthAwareService, state: ServiceState, id: ObjectId, @@ -196,10 +198,10 @@ pub(super) async fn create_session( /// /// Both answer `204 No Content` with the authoritative offset while bytes remain, and /// `201 Created` with the key once the object is committed. -pub(super) async fn session_request( +pub(super) async fn continue_session( service: AuthAwareService, - id: ObjectId, - session: SessionToken, + Xt(id): Xt, + Extension(session): Extension, headers: HeaderMap, MeteredBody(mut body): MeteredBody, ) -> ApiResult { @@ -231,13 +233,13 @@ pub(super) async fn session_request( progress_response(progress, key) } -/// Terminates a session, discarding whatever was uploaded. -pub(super) async fn terminate( +/// Cancels a session, discarding whatever was uploaded. +pub(super) async fn cancel_session( service: AuthAwareService, - id: ObjectId, - session: SessionToken, + Xt(id): Xt, + Extension(session): Extension, ) -> ApiResult { - service.terminate_upload(id, session).await?; + service.cancel_upload(id, session).await?; Ok(StatusCode::NO_CONTENT.into_response()) } @@ -289,16 +291,13 @@ mod tests { fn classify_recognizes_each_operation() { assert!(matches!( query(Some(UploadType::Resumable), None).classify(), - Ok(ResumableRoute::Create) + Ok(Some(ResumableTarget::NewSession)) )); assert!(matches!( query(None, Some("token")).classify(), - Ok(ResumableRoute::Session(_)) - )); - assert!(matches!( - query(None, None).classify(), - Ok(ResumableRoute::Regular) + Ok(Some(ResumableTarget::ExistingSession(_))) )); + assert!(matches!(query(None, None).classify(), Ok(None))); } #[test] @@ -307,21 +306,6 @@ mod tests { assert!(matches!(result, Err(ApiError::Client(_))), "{result:?}"); } - #[test] - fn classify_session_only_rejects_upload_type() { - let result = query(Some(UploadType::Resumable), None).classify_session_only("DELETE"); - assert!(matches!(result, Err(ApiError::Client(_))), "{result:?}"); - - assert!(matches!( - query(None, Some("token")).classify_session_only("DELETE"), - Ok(ResumableRoute::Session(_)) - )); - assert!(matches!( - query(None, None).classify_session_only("DELETE"), - Ok(ResumableRoute::Regular) - )); - } - #[test] fn upload_length_requires_a_byte_count() { let mut headers = HeaderMap::new(); diff --git a/objectstore-server/tests/resumable.rs b/objectstore-server/tests/resumable.rs index 63b072c7..b1613fdf 100644 --- a/objectstore-server/tests/resumable.rs +++ b/objectstore-server/tests/resumable.rs @@ -275,10 +275,10 @@ async fn session_token_requires_base64url() -> Result<()> { Ok(()) } -// --- Termination --- +// --- Cancellation --- #[tokio::test] -async fn terminate_reaches_the_declining_backend() -> Result<()> { +async fn cancel_upload_reaches_the_declining_backend() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() @@ -384,3 +384,17 @@ async fn regular_upload_ignores_resumable_headers() -> Result<()> { assert_eq!(response.status(), StatusCode::OK); Ok(()) } + +#[tokio::test] +async fn regular_upload_ignores_unrelated_query_parameters() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?unrelated=value")) + .body("payload") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::OK); + Ok(()) +} diff --git a/objectstore-service/docs/architecture.md b/objectstore-service/docs/architecture.md index 6f98a241..65c50a8c 100644 --- a/objectstore-service/docs/architecture.md +++ b/objectstore-service/docs/architecture.md @@ -208,7 +208,7 @@ trips for objects large enough that re-sending the whole payload is expensive. reports where the backend stands, so the caller resumes from there. 4. The chunk carrying the last byte commits the object. There is no finalize call — the backend recognizes that chunk from the declared total size. -5. At any time, a session can be terminated, which discards what it holds. +5. At any time, an upload can be canceled, which discards what its session holds. Not all backends support resumable uploads and can decline creating a session. Support can depend on the declared size, the metadata, or whether resuming is diff --git a/objectstore-service/src/backend/common.rs b/objectstore-service/src/backend/common.rs index e29e27dd..7d67326d 100644 --- a/objectstore-service/src/backend/common.rs +++ b/objectstore-service/src/backend/common.rs @@ -13,9 +13,7 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; -use crate::resumable::{ - CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, -}; +use crate::resumable::{CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress}; use crate::stream::{ClientStream, PayloadStream}; /// User agent string used for outgoing requests. @@ -128,7 +126,7 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { /// default implementation returns this, which is unreachable through the API because a /// backend that declines in [`Self::create_upload_session`] never hands out a session. /// - [`Error::UploadOffsetMismatch`] if `offset` is not the offset the backend holds. - /// - [`Error::UploadSessionGone`] if the session expired or was terminated. + /// - [`Error::UploadSessionGone`] if the session expired or was canceled. /// - [`Error::InvalidUploadRequest`] if the session is unusable, or the chunk would /// exceed the length declared at creation. async fn put_chunk( @@ -164,7 +162,7 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { Err(Error::NotImplemented) } - /// Terminates a session, discarding whatever was uploaded. + /// Cancels an upload session, discarding whatever was uploaded. /// /// Idempotent. Not required for correctness, since sessions expire on their own, but it /// lets a caller release an abandoned upload immediately. @@ -173,11 +171,11 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { /// /// - [`Error::NotImplemented`] if this backend does not support resumable uploads. /// - [`Error::InvalidUploadRequest`] if the session token is unusable. - async fn terminate_upload( + async fn cancel_upload( &self, id: &ObjectId, session: &SessionToken, - ) -> Result { + ) -> Result { let _ = (id, session); Err(Error::NotImplemented) } diff --git a/objectstore-service/src/backend/counting.rs b/objectstore-service/src/backend/counting.rs index 7f8b25f3..b0471244 100644 --- a/objectstore-service/src/backend/counting.rs +++ b/objectstore-service/src/backend/counting.rs @@ -28,9 +28,7 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; -use crate::resumable::{ - CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, -}; +use crate::resumable::{CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress}; use crate::stream::ClientStream; /// Increments `cogs.usage` by one operation for the given `usecase`. @@ -140,13 +138,13 @@ impl Backend for CountingBackend { self.inner.upload_offset(id, session).await } - async fn terminate_upload( + async fn cancel_upload( &self, id: &ObjectId, session: &SessionToken, - ) -> Result { + ) -> Result { count(&id.context.usecase); - self.inner.terminate_upload(id, session).await + self.inner.cancel_upload(id, session).await } } diff --git a/objectstore-service/src/backend/testing.rs b/objectstore-service/src/backend/testing.rs index 71e56250..26156bc2 100644 --- a/objectstore-service/src/backend/testing.rs +++ b/objectstore-service/src/backend/testing.rs @@ -52,9 +52,7 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; -use crate::resumable::{ - CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, -}; +use crate::resumable::{CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress}; use crate::stream::ClientStream; /// Hooks for [`TestBackend`]. @@ -285,14 +283,14 @@ pub trait Hooks: fmt::Debug + Send + Sync + 'static { inner.upload_offset(id, session).await } - /// Intercepts [`Backend::terminate_upload`]. Default delegates to `inner`. - async fn terminate_upload( + /// Intercepts [`Backend::cancel_upload`]. Default delegates to `inner`. + async fn cancel_upload( &self, inner: &InMemoryBackend, id: &ObjectId, session: &SessionToken, - ) -> Result { - inner.terminate_upload(id, session).await + ) -> Result { + inner.cancel_upload(id, session).await } } @@ -397,12 +395,12 @@ impl Backend for TestBackend { self.hooks.upload_offset(&self.inner, id, session).await } - async fn terminate_upload( + async fn cancel_upload( &self, id: &ObjectId, session: &SessionToken, - ) -> Result { - self.hooks.terminate_upload(&self.inner, id, session).await + ) -> Result { + self.hooks.cancel_upload(&self.inner, id, session).await } } diff --git a/objectstore-service/src/error.rs b/objectstore-service/src/error.rs index 678cf94b..5e68aa77 100644 --- a/objectstore-service/src/error.rs +++ b/objectstore-service/src/error.rs @@ -162,7 +162,7 @@ pub enum Error { offset: u64, }, - /// The resumable upload session expired or was terminated, retaining nothing. + /// The resumable upload session expired or was canceled, retaining nothing. /// /// The client has to start a new session. #[error("upload session gone")] diff --git a/objectstore-service/src/resumable.rs b/objectstore-service/src/resumable.rs index 3039e2b9..2b264e4f 100644 --- a/objectstore-service/src/resumable.rs +++ b/objectstore-service/src/resumable.rs @@ -41,6 +41,5 @@ pub enum UploadProgress { /// `None` means the backend declines resumable uploads for this object. pub type CreateSessionResponse = Option; -/// Response for -/// [`Backend::terminate_upload`](crate::backend::common::Backend::terminate_upload). -pub type TerminateUploadResponse = (); +/// Response for [`Backend::cancel_upload`](crate::backend::common::Backend::cancel_upload). +pub type CancelUploadResponse = (); diff --git a/objectstore-service/src/service.rs b/objectstore-service/src/service.rs index ca8dc68c..d33a84fa 100644 --- a/objectstore-service/src/service.rs +++ b/objectstore-service/src/service.rs @@ -20,9 +20,7 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; -use crate::resumable::{ - CreateSessionResponse, SessionToken, TerminateUploadResponse, UploadProgress, -}; +use crate::resumable::{CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress}; use crate::stream::{ClientStream, PayloadStream}; use crate::streaming::StreamExecutor; @@ -426,15 +424,15 @@ impl StorageService { .await } - /// Terminates a session, discarding whatever was uploaded. - pub async fn terminate_upload( + /// Cancels an upload session, discarding whatever was uploaded. + pub async fn cancel_upload( &self, id: ObjectId, session: SessionToken, - ) -> Result { + ) -> Result { let inner = Arc::clone(&self.inner); - self.spawn("terminate_upload", async move { - inner.terminate_upload(&id, &session).await + self.spawn("cancel_upload", async move { + inner.cancel_upload(&id, &session).await }) .await } @@ -883,8 +881,8 @@ mod tests { assert!(matches!(chunk, Err(Error::NotImplemented))); let offset = service.upload_offset(id.clone(), session.clone()).await; assert!(matches!(offset, Err(Error::NotImplemented))); - let terminated = service.terminate_upload(id, session).await; - assert!(matches!(terminated, Err(Error::NotImplemented))); + let canceled = service.cancel_upload(id, session).await; + assert!(matches!(canceled, Err(Error::NotImplemented))); } #[tokio::test] @@ -947,12 +945,12 @@ mod tests { Ok(self.progress) } - async fn terminate_upload( + async fn cancel_upload( &self, _inner: &InMemoryBackend, _id: &ObjectId, _session: &SessionToken, - ) -> Result { + ) -> Result { Ok(()) } } @@ -985,7 +983,7 @@ mod tests { UploadProgress::Incomplete { offset: 262_144 } ); - service.terminate_upload(id, session).await.unwrap(); + service.cancel_upload(id, session).await.unwrap(); } #[tokio::test] From d094ee70d368b530b52140a2011bffb80e7bb13b Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:28:08 +0200 Subject: [PATCH 04/22] ref(resumable): Defer route error responses Parse resumable query parameters synchronously from the request URI and return ApiError from route classification. This keeps response rendering in the dispatchers and avoids carrying a large Response in the Result error variant. --- objectstore-server/src/endpoints/objects.rs | 31 ++++++++------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index 16b3a519..32dcc054 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -6,7 +6,7 @@ use axum::handler::Handler; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::routing; -use axum::{Json, RequestExt, Router}; +use axum::{Json, Router}; use objectstore_service::error::Error as ServiceError; use objectstore_service::id::{ObjectContext, ObjectId}; use objectstore_types::headers::ExtValue; @@ -41,28 +41,21 @@ pub fn router() -> Router { /// /// Parsing a query does not consume the request body, so the selected handler can still extract /// it. -async fn extract_resumable_target( - request: &mut Request, -) -> Result, Response> { +fn extract_resumable_target(request: &Request) -> ApiResult> { if request.uri().query().is_none() { return Ok(None); } - let Query(query) = request - .extract_parts::>() - .await - .map_err(|rejection| rejection.into_response())?; + let Query(query) = Query::::try_from_uri(request.uri()) + .map_err(|error| ApiError::Client(error.to_string()))?; - query.classify().map_err(IntoResponse::into_response) + query.classify() } -async fn dispatch_objects_post( - State(state): State, - mut request: Request, -) -> Response { - let target = match extract_resumable_target(&mut request).await { +async fn dispatch_objects_post(State(state): State, request: Request) -> Response { + let target = match extract_resumable_target(&request) { Ok(target) => target, - Err(response) => return response, + Err(error) => return error.into_response(), }; match target { @@ -76,9 +69,9 @@ async fn dispatch_objects_post( } async fn dispatch_object_put(State(state): State, mut request: Request) -> Response { - let target = match extract_resumable_target(&mut request).await { + let target = match extract_resumable_target(&request) { Ok(target) => target, - Err(response) => return response, + Err(error) => return error.into_response(), }; match target { @@ -97,9 +90,9 @@ async fn dispatch_object_delete( State(state): State, mut request: Request, ) -> Response { - let target = match extract_resumable_target(&mut request).await { + let target = match extract_resumable_target(&request) { Ok(target) => target, - Err(response) => return response, + Err(error) => return error.into_response(), }; match target { From b4b4f87d6c2ab6d662e3bdebef0aa48c6a6bd5a4 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:34:39 +0200 Subject: [PATCH 05/22] fix(resumable): Reject declared wildcard request bodies Reject Upload-Offset: * requests immediately when Content-Length declares a non-empty body. Continue inspecting the body stream so chunked or otherwise undeclared payloads are also rejected. --- objectstore-server/src/endpoints/resumable.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index ba1e23be..a724f325 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -218,6 +218,12 @@ pub(super) async fn continue_session( UploadOffset::Unknown => { // The wildcard carries no payload. A body would be silently discarded, so // reject it rather than let a client believe those bytes were written. + if headers.contains_key(http::header::CONTENT_LENGTH) && content_length(&headers)? > 0 { + return Err(ApiError::Client(format!( + "{HEADER_UPLOAD_OFFSET}: * must be sent with an empty body" + ))); + } + while let Some(chunk) = body.try_next().await.map_err(ServiceError::from)? { if !chunk.is_empty() { return Err(ApiError::Client(format!( From 3f41938ca364b6c2baa1d13f520547a70c04411c Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:49:16 +0200 Subject: [PATCH 06/22] ref(server): Centralize protocol error responses Render range and resumable offset errors through ApiError so their status, protocol headers, and response bodies are defined in one place. Let object and resumable endpoints propagate the structured service errors directly. --- objectstore-server/src/endpoints/common.rs | 46 +++++++++++++------ objectstore-server/src/endpoints/objects.rs | 21 +-------- objectstore-server/src/endpoints/resumable.rs | 24 ++-------- 3 files changed, 40 insertions(+), 51 deletions(-) diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 75b89cbe..d063f159 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -7,6 +7,8 @@ use axum::http::StatusCode; use axum::response::{IntoResponse, Response}; use http::HeaderValue; use objectstore_service::error::Error as ServiceError; +use objectstore_types::range::ContentRange; +use objectstore_types::resumable::HEADER_UPLOAD_OFFSET; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -52,17 +54,6 @@ pub struct ApiErrorResponse { } impl ApiErrorResponse { - /// Creates an error response carrying only a message, with no cause chain. - /// - /// For outcomes that are not errors in the service layer and therefore have no - /// [`Error`] to wrap, such as a denied resumable upload session. - pub fn message(detail: impl Into) -> Self { - Self { - detail: Some(detail.into()), - causes: Vec::new(), - } - } - /// Creates an error response from an error, extracting the full cause chain. pub fn from_error(error: &E) -> Self { let detail = Some(error.to_string()); @@ -137,8 +128,37 @@ impl ApiError { impl IntoResponse for ApiError { fn into_response(self) -> Response { self.capture(); - let body = ApiErrorResponse::from_error(&self); - (self.status(), Json(body)).into_response() + + match self { + ApiError::Service(ServiceError::RangeNotSatisfiable { total }) => { + let mut response = ( + StatusCode::RANGE_NOT_SATISFIABLE, + [( + http::header::CONTENT_RANGE, + ContentRange::unsatisfiable_total_to_header_value(total), + )], + ) + .into_response(); + insert_accept_ranges(&mut response); + response + } + ApiError::Service(ServiceError::UploadOffsetMismatch { offset }) => { + let body = ApiErrorResponse { + detail: Some(format!("expected offset {offset}")), + causes: Vec::new(), + }; + ( + StatusCode::CONFLICT, + [(HEADER_UPLOAD_OFFSET, HeaderValue::from(offset))], + Json(body), + ) + .into_response() + } + error => { + let body = ApiErrorResponse::from_error(&error); + (error.status(), Json(body)).into_response() + } + } } } diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index 32dcc054..1c7b493a 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -11,7 +11,6 @@ use objectstore_service::error::Error as ServiceError; use objectstore_service::id::{ObjectContext, ObjectId}; use objectstore_types::headers::ExtValue; use objectstore_types::metadata::Metadata; -use objectstore_types::range::ContentRange; use serde::Serialize; use crate::auth::AuthAwareService; @@ -144,24 +143,8 @@ async fn object_get( _headers: HeaderMap, ) -> ApiResult { let context = id.context().clone(); - let result = service.get_object(id, byte_range).await; - - let (metadata, content_range, stream) = match result { - Ok(Some(result)) => result, - Ok(None) => return Ok(StatusCode::NOT_FOUND.into_response()), - Err(ApiError::Service(ServiceError::RangeNotSatisfiable { total })) => { - let mut response = ( - StatusCode::RANGE_NOT_SATISFIABLE, - [( - http::header::CONTENT_RANGE, - ContentRange::unsatisfiable_total_to_header_value(total), - )], - ) - .into_response(); - insert_accept_ranges(&mut response); - return Ok(response); - } - Err(e) => return Err(e), + let Some((metadata, content_range, stream)) = service.get_object(id, byte_range).await? else { + return Ok(StatusCode::NOT_FOUND.into_response()); }; let stream = state.meter_stream(stream, &context); diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index a724f325..a22260e5 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -38,7 +38,7 @@ use objectstore_types::resumable::{ use serde::Deserialize; use crate::auth::AuthAwareService; -use crate::endpoints::common::{ApiError, ApiErrorResponse, ApiResult}; +use crate::endpoints::common::{ApiError, ApiResult}; use crate::extractors::{Xt, body::MeteredBody}; use crate::state::ServiceState; @@ -250,23 +250,8 @@ pub(super) async fn cancel_session( } /// Turns an [`UploadProgress`] outcome into the response shared by chunks and offset queries. -/// -/// An offset mismatch is answered here rather than through [`ApiError::status`], because the -/// authoritative offset has to travel in a header that a generic error response cannot set. fn progress_response(progress: ApiResult, key: String) -> ApiResult { - let progress = match progress { - Ok(progress) => progress, - Err(ApiError::Service(ServiceError::UploadOffsetMismatch { offset })) => { - let body = ApiErrorResponse::message(format!("expected offset {offset}")); - let response = ( - StatusCode::CONFLICT, - [(HEADER_UPLOAD_OFFSET, http::HeaderValue::from(offset))], - Json(body), - ); - return Ok(response.into_response()); - } - Err(e) => return Err(e), - }; + let progress = progress?; let response = match progress { UploadProgress::Incomplete { offset } => ( @@ -380,8 +365,9 @@ mod tests { #[tokio::test] async fn offset_mismatch_answers_conflict_with_the_authoritative_offset() { let mismatch = ServiceError::UploadOffsetMismatch { offset: 786_432 }; - let response = - progress_response(Err(ApiError::Service(mismatch)), "my-key".into()).unwrap(); + let error = + progress_response(Err(ApiError::Service(mismatch)), "my-key".into()).unwrap_err(); + let response = error.into_response(); let (status, offset, body) = parts_of(response).await; assert_eq!(status, StatusCode::CONFLICT); From 69a0db50df9e0799655bab2bb3429ee76a9667f2 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:19:02 +0200 Subject: [PATCH 07/22] fix(resumable): Enforce session request contracts Reject bodies on session creation through shared empty-body validation, and treat unsupported functionality as a routine debug-level outcome. Clarify that session tokens are opaque, chunks require a declared length over HTTP/2, and fully uploaded sessions must commit or return an error. --- objectstore-server/src/endpoints/mod.rs | 11 ++-- objectstore-server/src/endpoints/resumable.rs | 59 +++++++++++++------ objectstore-server/tests/resumable.rs | 15 +++++ objectstore-service/src/backend/common.rs | 19 +++--- objectstore-service/src/error.rs | 3 +- objectstore-service/src/resumable.rs | 4 +- objectstore-types/src/resumable.rs | 13 +--- 7 files changed, 80 insertions(+), 44 deletions(-) diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index 4b4cb71d..831aa6e7 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -45,9 +45,9 @@ //! | `DELETE` | `/v1/objects/{usecase}/{scopes}/{*key}?session=` | Cancel upload, discarding what was sent | //! //! Session creation requires an `Upload-Length` header carrying the total size of the object -//! in bytes, and takes the same metadata headers as a regular upload. It answers `200 OK` -//! with `{"key", "session"}`; the session field is the token to use in subsequent query -//! parameters. Metadata is fixed at this point and does not change afterwards. +//! in bytes, takes the same metadata headers as a regular upload, and requires an empty body. +//! It answers `200 OK` with `{"key", "session"}`; the session field is the token to use in +//! subsequent query parameters. Metadata is fixed at this point and does not change afterwards. //! //! Chunk uploads and offset queries share one request shape, distinguished by the //! `Upload-Offset` header: a byte offset submits the body as the chunk starting there, while @@ -56,8 +56,9 @@ //! `201 Created` with `{"key"}` once the object is committed. **The offset in the response //! may be lower than the end of the chunk that was sent** — backends persist only aligned //! prefixes and discard the remainder — so clients always continue from the returned offset. -//! A chunk requires `Content-Length`; an offset query does not, but its body must still be empty. -//! The server rejects an offset query carrying any body bytes with `400 Bad Request`. +//! A chunk requires `Content-Length` even over HTTP/2; an offset query does not, but its body must +//! still be empty. The server rejects a session creation or offset query carrying any body bytes +//! with `400 Bad Request`. //! //! An offset query can commit an object that was assembled but not yet committed, so it //! requires write permission despite being read-shaped. Termination likewise needs write diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index a22260e5..f11966b0 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -31,6 +31,7 @@ use futures_util::TryStreamExt; use objectstore_service::error::Error as ServiceError; use objectstore_service::id::{ObjectContext, ObjectId}; use objectstore_service::resumable::{SessionToken, UploadOffset, UploadProgress}; +use objectstore_service::stream::ClientStream; use objectstore_types::metadata::Metadata; use objectstore_types::resumable::{ CommitResponse, CreateSessionResponse, HEADER_UPLOAD_LENGTH, HEADER_UPLOAD_OFFSET, @@ -137,14 +138,45 @@ fn content_length(headers: &HeaderMap) -> ApiResult { .ok_or_else(|| ApiError::Client("Content-Length header is required".into())) } +/// Confirms that a request neither declares nor streams a non-empty body. +async fn require_empty_body( + headers: &HeaderMap, + mut body: ClientStream, + request: &str, +) -> ApiResult<()> { + if headers.contains_key(http::header::CONTENT_LENGTH) && content_length(headers)? > 0 { + return Err(ApiError::Client(format!( + "{request} must be sent with an empty body" + ))); + } + + while let Some(chunk) = body.try_next().await.map_err(ServiceError::from)? { + if !chunk.is_empty() { + return Err(ApiError::Client(format!( + "{request} must be sent with an empty body" + ))); + } + } + + Ok(()) +} + /// Creates a session with a server-generated object key. pub(super) async fn create_session( service: AuthAwareService, State(state): State, Xt(context): Xt, headers: HeaderMap, + MeteredBody(body): MeteredBody, ) -> ApiResult { - create_session_for_id(service, state, ObjectId::optional(context, None), headers).await + create_session_for_id( + service, + state, + ObjectId::optional(context, None), + headers, + body, + ) + .await } /// Creates a session for the object key in the request path. @@ -153,8 +185,9 @@ pub(super) async fn create_session_for_key( State(state): State, Xt(id): Xt, headers: HeaderMap, + MeteredBody(body): MeteredBody, ) -> ApiResult { - create_session_for_id(service, state, id, headers).await + create_session_for_id(service, state, id, headers, body).await } /// Creates a session for the object at `id`. @@ -166,8 +199,10 @@ async fn create_session_for_id( state: ServiceState, id: ObjectId, headers: HeaderMap, + body: ClientStream, ) -> ApiResult { let total_length = upload_length(&headers)?; + require_empty_body(&headers, body, "resumable session creation").await?; let metadata = Metadata::from_insert_headers(&headers, "").map_err(ServiceError::from)?; state @@ -193,8 +228,8 @@ async fn create_session_for_id( /// [`HEADER_UPLOAD_OFFSET`] selects between the two. A concrete offset submits the request /// body as the chunk starting there; the `*` wildcard submits nothing and asks where the /// server stands, which also commits an object that was assembled but not yet committed. -/// Chunks require `Content-Length`. Offset queries may omit it, but the body stream is checked -/// and any bytes are rejected as a malformed request. +/// Chunks require `Content-Length`, including over HTTP/2. Offset queries may omit it, but the +/// body stream is checked and any bytes are rejected as a malformed request. /// /// Both answer `204 No Content` with the authoritative offset while bytes remain, and /// `201 Created` with the key once the object is committed. @@ -203,7 +238,7 @@ pub(super) async fn continue_session( Xt(id): Xt, Extension(session): Extension, headers: HeaderMap, - MeteredBody(mut body): MeteredBody, + MeteredBody(body): MeteredBody, ) -> ApiResult { let offset = upload_offset(&headers)?; let key = id.key().to_owned(); @@ -218,19 +253,7 @@ pub(super) async fn continue_session( UploadOffset::Unknown => { // The wildcard carries no payload. A body would be silently discarded, so // reject it rather than let a client believe those bytes were written. - if headers.contains_key(http::header::CONTENT_LENGTH) && content_length(&headers)? > 0 { - return Err(ApiError::Client(format!( - "{HEADER_UPLOAD_OFFSET}: * must be sent with an empty body" - ))); - } - - while let Some(chunk) = body.try_next().await.map_err(ServiceError::from)? { - if !chunk.is_empty() { - return Err(ApiError::Client(format!( - "{HEADER_UPLOAD_OFFSET}: * must be sent with an empty body" - ))); - } - } + require_empty_body(&headers, body, "Upload-Offset: *").await?; service.upload_offset(id, session).await } diff --git a/objectstore-server/tests/resumable.rs b/objectstore-server/tests/resumable.rs index b1613fdf..794d6a10 100644 --- a/objectstore-server/tests/resumable.rs +++ b/objectstore-server/tests/resumable.rs @@ -127,6 +127,21 @@ async fn create_session_rejects_malformed_upload_length() -> Result<()> { Ok(()) } +#[tokio::test] +async fn create_session_rejects_a_payload() -> Result<()> { + let server = test_server().await; + + let response = reqwest::Client::new() + .put(server.url("/v1/objects/test/org=1/my-key?upload_type=resumable")) + .header(HEADER_UPLOAD_LENGTH, "7") + .body("payload") + .send() + .await?; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + Ok(()) +} + #[tokio::test] async fn unknown_upload_type_is_rejected() -> Result<()> { let server = test_server().await; diff --git a/objectstore-service/src/backend/common.rs b/objectstore-service/src/backend/common.rs index 7d67326d..b3f1a385 100644 --- a/objectstore-service/src/backend/common.rs +++ b/objectstore-service/src/backend/common.rs @@ -109,6 +109,9 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { /// Writes a chunk of `content_length` bytes at `offset` into an open session. /// + /// The application protocol requires the caller to declare `content_length` before the body + /// is consumed, including over HTTP/2. Backends may rely on it without buffering the stream. + /// /// `offset` must equal the offset the backend currently holds. Backends persist only /// aligned prefixes and discard the remainder, so the offset in the returned /// [`UploadProgress::Incomplete`] is authoritative and may be lower than @@ -144,15 +147,13 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { /// Reports how far the session has progressed, committing the object if it is assembled. /// /// This is the recovery path: after any failed chunk the client calls this and continues - /// from the returned offset. It is also the only read-shaped operation that mutates - /// state. Making an object visible can outlive the request that triggered it, so a - /// session whose payload fully landed may still be uncommitted; this operation finishes - /// that work and returns [`UploadProgress::Committed`]. Callers must therefore treat it - /// as a write. - /// - /// A session whose object was assembled but not yet committed never reports - /// [`UploadProgress::Committed`], so a client that observes completion can always read - /// the object back. + /// from the returned offset. It is also the only read-shaped operation that mutates state. + /// Making an object visible can outlive the request that triggered it, so a session whose + /// payload fully landed may still be uncommitted; this operation must finish that work. It + /// returns [`UploadProgress::Committed`] only after the object is committed and readable, or + /// returns an error if committing fails. It must not return [`UploadProgress::Incomplete`] + /// with the session's total length, because the client would have no bytes left to send. + /// Callers must therefore treat it as a write. /// /// # Errors /// diff --git a/objectstore-service/src/error.rs b/objectstore-service/src/error.rs index 5e68aa77..e1158c57 100644 --- a/objectstore-service/src/error.rs +++ b/objectstore-service/src/error.rs @@ -223,6 +223,8 @@ impl Error { Self::UploadOffsetMismatch { .. } => Level::DEBUG, Self::UploadSessionGone => Level::DEBUG, Self::InvalidUploadRequest(_) => Level::DEBUG, + // Unsupported optional functionality is a routine capability outcome. + Self::NotImplemented => Level::DEBUG, // Like rate limits, we treat capacity errors as warnings Self::AtCapacity => Level::WARN, // All other errors are service or backend failures @@ -234,7 +236,6 @@ impl Error { Self::Panic(_) => Level::ERROR, Self::Dropped => Level::ERROR, Self::UnexpectedTombstone => Level::ERROR, - Self::NotImplemented => Level::ERROR, Self::InvalidUploadId(_) => Level::DEBUG, Self::Generic { .. } => Level::ERROR, } diff --git a/objectstore-service/src/resumable.rs b/objectstore-service/src/resumable.rs index 2b264e4f..bd857fac 100644 --- a/objectstore-service/src/resumable.rs +++ b/objectstore-service/src/resumable.rs @@ -26,7 +26,9 @@ pub enum UploadProgress { /// More bytes are expected. The client continues from `offset`. /// /// This offset is authoritative and may be lower than the end of the chunk that was - /// just written: backends persist only aligned prefixes and discard the remainder. + /// just written: backends persist only aligned prefixes and discard the remainder. It must + /// remain below the session's total length; once every byte has landed, the backend commits + /// the object or returns an error instead. Incomplete { /// The offset the backend has persisted. offset: u64, diff --git a/objectstore-types/src/resumable.rs b/objectstore-types/src/resumable.rs index befbaf13..321175bb 100644 --- a/objectstore-types/src/resumable.rs +++ b/objectstore-types/src/resumable.rs @@ -46,16 +46,9 @@ const OFFSET_WILDCARD: &str = "*"; /// Identifier for an in-progress resumable upload session. /// -/// The token is opaque to the client: it is minted by the storage backend and carries -/// whatever that backend needs to continue or commit the upload without shared state. -/// It is neither signed nor encrypted, which is safe because the usecase, scopes and -/// key travel in the request path rather than in the token, so a request cannot address -/// an object other than the one it names. -/// -/// Validated on construction only to ensure it is non-empty. Its contents are otherwise opaque: -/// backends may use any UTF-8 string, including path separators and traversal-like text. -/// At the API boundary it is serialized as unpadded base64url, keeping the opaque value out of -/// URL parsing and escaping rules. +/// The token is an opaque identifier whose contents are defined by the storage backend. It is +/// validated on construction only to ensure it is non-empty. At the API boundary it is serialized +/// as unpadded base64url, keeping the opaque value out of URL parsing and escaping rules. #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub struct SessionToken(String); From a53faae93a40525ad560b422cb16d2687bfce5d9 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:45:15 +0200 Subject: [PATCH 08/22] ref(resumable): Extract targets from request parts Implement ResumableTarget as an optional Axum parts extractor so dispatch handlers receive the classified target directly. Keep the loose query representation private and pass only existing session tokens through request extensions. --- objectstore-server/src/endpoints/objects.rs | 49 +++++-------------- objectstore-server/src/endpoints/resumable.rs | 38 ++++++++++---- 2 files changed, 41 insertions(+), 46 deletions(-) diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index 1c7b493a..57bc1b79 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -1,7 +1,7 @@ use std::fmt::Write as _; use axum::body::Body; -use axum::extract::{Query, Request, State}; +use axum::extract::{Request, State}; use axum::handler::Handler; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; @@ -15,7 +15,7 @@ use serde::Serialize; use crate::auth::AuthAwareService; use crate::endpoints::common::{ApiError, ApiResult, insert_accept_ranges}; -use crate::endpoints::resumable::{self, ResumableQuery, ResumableTarget}; +use crate::endpoints::resumable::{self, ResumableTarget}; use crate::extractors::byte_range::OptionalByteRange; use crate::extractors::{Xt, body::MeteredBody}; use crate::state::ServiceState; @@ -34,29 +34,11 @@ pub fn router() -> Router { .route("/objects/{usecase}/{scopes}/{*key}", object_routes) } -/// Extracts which resumable session, if any, the request targets. -/// -/// Returns `None` without parsing when the URI has no query string. -/// -/// Parsing a query does not consume the request body, so the selected handler can still extract -/// it. -fn extract_resumable_target(request: &Request) -> ApiResult> { - if request.uri().query().is_none() { - return Ok(None); - } - - let Query(query) = Query::::try_from_uri(request.uri()) - .map_err(|error| ApiError::Client(error.to_string()))?; - - query.classify() -} - -async fn dispatch_objects_post(State(state): State, request: Request) -> Response { - let target = match extract_resumable_target(&request) { - Ok(target) => target, - Err(error) => return error.into_response(), - }; - +async fn dispatch_objects_post( + State(state): State, + target: Option, + request: Request, +) -> Response { match target { Some(ResumableTarget::NewSession) => resumable::create_session.call(request, state).await, Some(ResumableTarget::ExistingSession(_)) => { @@ -67,12 +49,11 @@ async fn dispatch_objects_post(State(state): State, request: Reque } } -async fn dispatch_object_put(State(state): State, mut request: Request) -> Response { - let target = match extract_resumable_target(&request) { - Ok(target) => target, - Err(error) => return error.into_response(), - }; - +async fn dispatch_object_put( + State(state): State, + target: Option, + mut request: Request, +) -> Response { match target { Some(ResumableTarget::NewSession) => { resumable::create_session_for_key.call(request, state).await @@ -87,13 +68,9 @@ async fn dispatch_object_put(State(state): State, mut request: Req async fn dispatch_object_delete( State(state): State, + target: Option, mut request: Request, ) -> Response { - let target = match extract_resumable_target(&request) { - Ok(target) => target, - Err(error) => return error.into_response(), - }; - match target { Some(ResumableTarget::ExistingSession(session)) => { request.extensions_mut().insert(session); diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index f11966b0..1f6f463a 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -3,8 +3,8 @@ //! Resumable uploads are a variation of the regular object endpoints rather than a separate //! resource, following GCS and S3 rather than [TUS]. Every request addresses the same object //! path with the session in the query string, so these handlers have no router of their own: -//! thin handlers in [`objects`](super::objects) inspect [`ResumableQuery`] and dispatch the -//! original request here, where each operation runs with its own Axum extractors. +//! [`ResumableTarget`] classifies the query before thin handlers in [`objects`](super::objects) +//! dispatch the original request here, where each operation runs with its own Axum extractors. //! Session tokens are encoded as unpadded base64url at this API boundary. //! //! | Operation | Request | Success | @@ -23,8 +23,8 @@ //! //! [TUS]: https://tus.io/protocols/resumable-upload -use axum::extract::{Extension, State}; -use axum::http::{HeaderMap, StatusCode}; +use axum::extract::{Extension, OptionalFromRequestParts, Query, State}; +use axum::http::{HeaderMap, StatusCode, request::Parts}; use axum::response::{IntoResponse, Response}; use axum::{Json, http}; use futures_util::TryStreamExt; @@ -58,8 +58,8 @@ pub(super) enum UploadType { /// /// Both fields are optional and unknown parameters are ignored, because pre-signed URLs put /// their own `os_*` parameters into the same query string. -#[derive(Debug, Default, Deserialize)] -pub(super) struct ResumableQuery { +#[derive(Debug, Deserialize)] +struct ResumableQuery { /// Present on a session creation request. upload_type: Option, /// Present on a chunk write, offset query, or cancellation. @@ -72,9 +72,6 @@ pub(super) enum ResumableTarget { /// A new session to create for the object addressed by the request. NewSession, /// An existing session to continue or cancel. - /// - /// The dispatcher moves the token into the request extensions before calling the selected - /// handler, avoiding a second query-string deserialization. ExistingSession(SessionToken), } @@ -85,7 +82,7 @@ impl ResumableQuery { /// /// Returns [`ApiError::Client`] if both parameters are present. They address different /// operations, so a request carrying both is ambiguous rather than defaulted. - pub fn classify(self) -> ApiResult> { + fn classify(self) -> ApiResult> { match (self.upload_type, self.session) { (Some(_), Some(_)) => Err(ApiError::Client( "`upload_type` and `session` are mutually exclusive".into(), @@ -97,6 +94,27 @@ impl ResumableQuery { } } +impl OptionalFromRequestParts for ResumableTarget +where + S: Send + Sync, +{ + type Rejection = ApiError; + + async fn from_request_parts( + parts: &mut Parts, + _state: &S, + ) -> ApiResult> { + if parts.uri.query().is_none() { + return Ok(None); + } + + let Query(query) = Query::::try_from_uri(&parts.uri) + .map_err(|error| ApiError::Client(error.to_string()))?; + + query.classify() + } +} + /// Reads the required [`HEADER_UPLOAD_LENGTH`] header. fn upload_length(headers: &HeaderMap) -> ApiResult { let value = headers From 910b019b652005371fad6fe04b4b81b280b4f02a Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:58:20 +0200 Subject: [PATCH 09/22] ref(server): Keep protocol headers in endpoints Build unsatisfiable range responses in the object endpoint and attach authoritative offsets in the resumable continuation path. Keep generic ApiError conversion free of operation-specific response headers. --- objectstore-server/src/endpoints/common.rs | 35 ++----------------- objectstore-server/src/endpoints/objects.rs | 21 +++++++++-- objectstore-server/src/endpoints/resumable.rs | 20 ++++++++--- 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index d063f159..104f2ba4 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -7,8 +7,6 @@ use axum::http::StatusCode; use axum::response::{IntoResponse, Response}; use http::HeaderValue; use objectstore_service::error::Error as ServiceError; -use objectstore_types::range::ContentRange; -use objectstore_types::resumable::HEADER_UPLOAD_OFFSET; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -128,37 +126,8 @@ impl ApiError { impl IntoResponse for ApiError { fn into_response(self) -> Response { self.capture(); - - match self { - ApiError::Service(ServiceError::RangeNotSatisfiable { total }) => { - let mut response = ( - StatusCode::RANGE_NOT_SATISFIABLE, - [( - http::header::CONTENT_RANGE, - ContentRange::unsatisfiable_total_to_header_value(total), - )], - ) - .into_response(); - insert_accept_ranges(&mut response); - response - } - ApiError::Service(ServiceError::UploadOffsetMismatch { offset }) => { - let body = ApiErrorResponse { - detail: Some(format!("expected offset {offset}")), - causes: Vec::new(), - }; - ( - StatusCode::CONFLICT, - [(HEADER_UPLOAD_OFFSET, HeaderValue::from(offset))], - Json(body), - ) - .into_response() - } - error => { - let body = ApiErrorResponse::from_error(&error); - (error.status(), Json(body)).into_response() - } - } + let body = ApiErrorResponse::from_error(&self); + (self.status(), Json(body)).into_response() } } diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index 57bc1b79..83f0d9b3 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -11,6 +11,7 @@ use objectstore_service::error::Error as ServiceError; use objectstore_service::id::{ObjectContext, ObjectId}; use objectstore_types::headers::ExtValue; use objectstore_types::metadata::Metadata; +use objectstore_types::range::ContentRange; use serde::Serialize; use crate::auth::AuthAwareService; @@ -120,8 +121,24 @@ async fn object_get( _headers: HeaderMap, ) -> ApiResult { let context = id.context().clone(); - let Some((metadata, content_range, stream)) = service.get_object(id, byte_range).await? else { - return Ok(StatusCode::NOT_FOUND.into_response()); + let result = service.get_object(id, byte_range).await; + + let (metadata, content_range, stream) = match result { + Ok(Some(result)) => result, + Ok(None) => return Ok(StatusCode::NOT_FOUND.into_response()), + Err(ApiError::Service(ServiceError::RangeNotSatisfiable { total })) => { + let mut response = ( + StatusCode::RANGE_NOT_SATISFIABLE, + [( + http::header::CONTENT_RANGE, + ContentRange::unsatisfiable_total_to_header_value(total), + )], + ) + .into_response(); + insert_accept_ranges(&mut response); + return Ok(response); + } + Err(error) => return Err(error), }; let stream = state.meter_stream(stream, &context); diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index 1f6f463a..a899fd00 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -291,8 +291,21 @@ pub(super) async fn cancel_session( } /// Turns an [`UploadProgress`] outcome into the response shared by chunks and offset queries. +/// +/// An offset mismatch is answered here rather than through [`ApiError::status`], because the +/// authoritative offset has to travel in a header that a generic error response cannot set. fn progress_response(progress: ApiResult, key: String) -> ApiResult { - let progress = progress?; + let progress = match progress { + Ok(progress) => progress, + Err(error @ ApiError::Service(ServiceError::UploadOffsetMismatch { offset })) => { + let mut response = error.into_response(); + response + .headers_mut() + .insert(HEADER_UPLOAD_OFFSET, http::HeaderValue::from(offset)); + return Ok(response); + } + Err(error) => return Err(error), + }; let response = match progress { UploadProgress::Incomplete { offset } => ( @@ -406,9 +419,8 @@ mod tests { #[tokio::test] async fn offset_mismatch_answers_conflict_with_the_authoritative_offset() { let mismatch = ServiceError::UploadOffsetMismatch { offset: 786_432 }; - let error = - progress_response(Err(ApiError::Service(mismatch)), "my-key".into()).unwrap_err(); - let response = error.into_response(); + let response = + progress_response(Err(ApiError::Service(mismatch)), "my-key".into()).unwrap(); let (status, offset, body) = parts_of(response).await; assert_eq!(status, StatusCode::CONFLICT); From d9a56f9720d82feb13b25a1874cd2eb0b240d9e9 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:10:08 +0200 Subject: [PATCH 10/22] ref(server): Simplify object handler extraction Reparse and validate session tokens in continuation and cancellation leaf handlers, removing the request-extension handoff. Keep route classification as a marker and return the regular delete status directly through IntoResponse. --- objectstore-server/src/endpoints/objects.rs | 19 +++++----- objectstore-server/src/endpoints/resumable.rs | 38 +++++++++++++++---- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index 83f0d9b3..b13c189c 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -42,7 +42,7 @@ async fn dispatch_objects_post( ) -> Response { match target { Some(ResumableTarget::NewSession) => resumable::create_session.call(request, state).await, - Some(ResumableTarget::ExistingSession(_)) => { + Some(ResumableTarget::ExistingSession) => { ApiError::Client("`session` requires an object key; use PUT on the object path".into()) .into_response() } @@ -53,14 +53,13 @@ async fn dispatch_objects_post( async fn dispatch_object_put( State(state): State, target: Option, - mut request: Request, + request: Request, ) -> Response { match target { Some(ResumableTarget::NewSession) => { resumable::create_session_for_key.call(request, state).await } - Some(ResumableTarget::ExistingSession(session)) => { - request.extensions_mut().insert(session); + Some(ResumableTarget::ExistingSession) => { resumable::continue_session.call(request, state).await } None => insert_object.call(request, state).await, @@ -70,11 +69,10 @@ async fn dispatch_object_put( async fn dispatch_object_delete( State(state): State, target: Option, - mut request: Request, + request: Request, ) -> Response { match target { - Some(ResumableTarget::ExistingSession(session)) => { - request.extensions_mut().insert(session); + Some(ResumableTarget::ExistingSession) => { resumable::cancel_session.call(request, state).await } Some(ResumableTarget::NewSession) => { @@ -274,7 +272,10 @@ async fn insert_object( Ok((StatusCode::OK, response).into_response()) } -async fn delete_object(service: AuthAwareService, Xt(id): Xt) -> ApiResult { +async fn delete_object( + service: AuthAwareService, + Xt(id): Xt, +) -> ApiResult { service.delete_object(id).await?; - Ok(StatusCode::NO_CONTENT.into_response()) + Ok(StatusCode::NO_CONTENT) } diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index a899fd00..508ff115 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -23,7 +23,7 @@ //! //! [TUS]: https://tus.io/protocols/resumable-upload -use axum::extract::{Extension, OptionalFromRequestParts, Query, State}; +use axum::extract::{FromRequestParts, OptionalFromRequestParts, Query, State}; use axum::http::{HeaderMap, StatusCode, request::Parts}; use axum::response::{IntoResponse, Response}; use axum::{Json, http}; @@ -63,7 +63,7 @@ struct ResumableQuery { /// Present on a session creation request. upload_type: Option, /// Present on a chunk write, offset query, or cancellation. - session: Option, + session: Option, } /// Which resumable session a request on an object route targets. @@ -72,7 +72,7 @@ pub(super) enum ResumableTarget { /// A new session to create for the object addressed by the request. NewSession, /// An existing session to continue or cancel. - ExistingSession(SessionToken), + ExistingSession, } impl ResumableQuery { @@ -88,12 +88,34 @@ impl ResumableQuery { "`upload_type` and `session` are mutually exclusive".into(), )), (Some(UploadType::Resumable), None) => Ok(Some(ResumableTarget::NewSession)), - (None, Some(session)) => Ok(Some(ResumableTarget::ExistingSession(session))), + (None, Some(_)) => Ok(Some(ResumableTarget::ExistingSession)), (None, None) => Ok(None), } } } +/// A validated session token extracted by a continuation or cancellation handler. +#[derive(Debug)] +pub(super) struct Session(SessionToken); + +#[derive(Debug, Deserialize)] +struct SessionQuery { + session: SessionToken, +} + +impl FromRequestParts for Session +where + S: Send + Sync, +{ + type Rejection = ApiError; + + async fn from_request_parts(parts: &mut Parts, _state: &S) -> ApiResult { + let Query(SessionQuery { session }) = Query::::try_from_uri(&parts.uri) + .map_err(|error| ApiError::Client(error.to_string()))?; + Ok(Session(session)) + } +} + impl OptionalFromRequestParts for ResumableTarget where S: Send + Sync, @@ -254,7 +276,7 @@ async fn create_session_for_id( pub(super) async fn continue_session( service: AuthAwareService, Xt(id): Xt, - Extension(session): Extension, + Session(session): Session, headers: HeaderMap, MeteredBody(body): MeteredBody, ) -> ApiResult { @@ -284,7 +306,7 @@ pub(super) async fn continue_session( pub(super) async fn cancel_session( service: AuthAwareService, Xt(id): Xt, - Extension(session): Extension, + Session(session): Session, ) -> ApiResult { service.cancel_upload(id, session).await?; Ok(StatusCode::NO_CONTENT.into_response()) @@ -328,7 +350,7 @@ mod tests { fn query(upload_type: Option, session: Option<&str>) -> ResumableQuery { ResumableQuery { upload_type, - session: session.map(|s| SessionToken::new(s.into()).unwrap()), + session: session.map(str::to_owned), } } @@ -340,7 +362,7 @@ mod tests { )); assert!(matches!( query(None, Some("token")).classify(), - Ok(Some(ResumableTarget::ExistingSession(_))) + Ok(Some(ResumableTarget::ExistingSession)) )); assert!(matches!(query(None, None).classify(), Ok(None))); } From 85280246155912197b551c58b119d1a91bbb9ff8 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:24:49 +0200 Subject: [PATCH 11/22] improve --- objectstore-service/src/backend/common.rs | 72 ++------------------- objectstore-service/src/backend/counting.rs | 6 -- 2 files changed, 6 insertions(+), 72 deletions(-) diff --git a/objectstore-service/src/backend/common.rs b/objectstore-service/src/backend/common.rs index b3f1a385..f1d24d57 100644 --- a/objectstore-service/src/backend/common.rs +++ b/objectstore-service/src/backend/common.rs @@ -74,29 +74,12 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { Err(Error::NotImplemented) } - /// Opens a resumable upload session for the object at `id`. + /// Creates a resumable upload session for the object at `id`. /// - /// `total_length` is the complete size of the object in bytes, declared by the client - /// when the session is created. It is a parameter of its own rather than part of - /// `metadata`, because [`Metadata::size`] is materialized by the server and never - /// trusted from a client. The backend needs it to recognize the final chunk, and a - /// tiering backend needs it to decide where the object would be placed. + /// Object metadata and its total length are declared upfront and cannot be mutated + /// during the upload. /// - /// `metadata` is fixed for the lifetime of the session and does not change afterwards. - /// Compression is recorded rather than applied: the payload must already be compressed, - /// since its total length has to be known at this point. - /// - /// Returns `Ok(None)` when this backend cannot store the described object resumably. - /// Declining is a routine outcome, not an error — the server denies the session and the - /// client falls back to a regular upload. The default implementation declines, so a - /// backend opts in simply by overriding this method. There is deliberately no separate - /// capability trait and no probe: support can depend on the size, the metadata and the - /// routing result at once, all of which are only known here. - /// - /// # Errors - /// - /// Returns an error only when the backend supports resumable uploads but failed to open - /// the session. + /// Returns `Ok(None)` when the backend refuses to store the described object resumably. async fn create_upload_session( &self, id: &ObjectId, @@ -109,29 +92,7 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { /// Writes a chunk of `content_length` bytes at `offset` into an open session. /// - /// The application protocol requires the caller to declare `content_length` before the body - /// is consumed, including over HTTP/2. Backends may rely on it without buffering the stream. - /// - /// `offset` must equal the offset the backend currently holds. Backends persist only - /// aligned prefixes and discard the remainder, so the offset in the returned - /// [`UploadProgress::Incomplete`] is authoritative and may be lower than - /// `offset + content_length`. - /// - /// A session has a single writer. Concurrent chunk writes are not coordinated: one of - /// them wins and the others fail with [`Error::UploadOffsetMismatch`]. - /// - /// Once the chunk carrying the last byte is persisted, the backend assembles and commits - /// the object and returns [`UploadProgress::Committed`]. - /// - /// # Errors - /// - /// - [`Error::NotImplemented`] if this backend does not support resumable uploads. The - /// default implementation returns this, which is unreachable through the API because a - /// backend that declines in [`Self::create_upload_session`] never hands out a session. - /// - [`Error::UploadOffsetMismatch`] if `offset` is not the offset the backend holds. - /// - [`Error::UploadSessionGone`] if the session expired or was canceled. - /// - [`Error::InvalidUploadRequest`] if the session is unusable, or the chunk would - /// exceed the length declared at creation. + /// `offset` must equal the offset the backend currently holds. async fn put_chunk( &self, id: &ObjectId, @@ -144,34 +105,13 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { Err(Error::NotImplemented) } - /// Reports how far the session has progressed, committing the object if it is assembled. - /// - /// This is the recovery path: after any failed chunk the client calls this and continues - /// from the returned offset. It is also the only read-shaped operation that mutates state. - /// Making an object visible can outlive the request that triggered it, so a session whose - /// payload fully landed may still be uncommitted; this operation must finish that work. It - /// returns [`UploadProgress::Committed`] only after the object is committed and readable, or - /// returns an error if committing fails. It must not return [`UploadProgress::Incomplete`] - /// with the session's total length, because the client would have no bytes left to send. - /// Callers must therefore treat it as a write. - /// - /// # Errors - /// - /// The same conditions as [`Self::put_chunk`], except for the offset mismatch. + /// Reports how far the session has progressed. async fn upload_offset(&self, id: &ObjectId, session: &SessionToken) -> Result { let _ = (id, session); Err(Error::NotImplemented) } /// Cancels an upload session, discarding whatever was uploaded. - /// - /// Idempotent. Not required for correctness, since sessions expire on their own, but it - /// lets a caller release an abandoned upload immediately. - /// - /// # Errors - /// - /// - [`Error::NotImplemented`] if this backend does not support resumable uploads. - /// - [`Error::InvalidUploadRequest`] if the session token is unusable. async fn cancel_upload( &self, id: &ObjectId, diff --git a/objectstore-service/src/backend/counting.rs b/objectstore-service/src/backend/counting.rs index b0471244..c5307aae 100644 --- a/objectstore-service/src/backend/counting.rs +++ b/objectstore-service/src/backend/counting.rs @@ -47,12 +47,6 @@ fn count(usecase: &str) { /// `Arc`s that point to the inner backend: /// - `inner: Arc` /// - `inner_multipart: Option>` if `inner` supports it -/// -/// Resumable uploads avoid this problem: their operations live on [`Backend`] itself and express -/// support by declining in -/// [`create_upload_session`](Backend::create_upload_session), so this decorator forwards them like -/// any other method. Forwarding is mandatory — without it the decorator's declining default would -/// shadow an inner backend that does support resumable uploads. #[derive(Debug)] pub struct CountingBackend { inner: Arc, From 17d21b9dc2f41f1c0d809e1e672c561f4460727b Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:26:52 +0200 Subject: [PATCH 12/22] improve --- objectstore-service/src/backend/testing.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/objectstore-service/src/backend/testing.rs b/objectstore-service/src/backend/testing.rs index 26156bc2..8804cfd7 100644 --- a/objectstore-service/src/backend/testing.rs +++ b/objectstore-service/src/backend/testing.rs @@ -240,10 +240,6 @@ pub trait Hooks: fmt::Debug + Send + Sync + 'static { } // --- Resumable upload methods --- - // - // `InMemoryBackend` does not implement resumable uploads, so these delegate to the - // declining `Backend` defaults. A test that exercises the resumable protocol has to - // override them. /// Intercepts [`Backend::create_upload_session`]. Default delegates to `inner`. async fn create_upload_session( From 25b7b0feb9e2ebf2cf43304bc10bd0e945a26df9 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:29:07 +0200 Subject: [PATCH 13/22] improve --- objectstore-service/src/error.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/objectstore-service/src/error.rs b/objectstore-service/src/error.rs index e1158c57..ecc613a9 100644 --- a/objectstore-service/src/error.rs +++ b/objectstore-service/src/error.rs @@ -223,8 +223,10 @@ impl Error { Self::UploadOffsetMismatch { .. } => Level::DEBUG, Self::UploadSessionGone => Level::DEBUG, Self::InvalidUploadRequest(_) => Level::DEBUG, - // Unsupported optional functionality is a routine capability outcome. - Self::NotImplemented => Level::DEBUG, + // Indicates that optional functionality is not supported. + // We don't want a rogue client spamming us with Sentry errors just by calling an API + // that the server doesn't support, so we just log it. + Self::NotImplemented => Level::INFO, // Like rate limits, we treat capacity errors as warnings Self::AtCapacity => Level::WARN, // All other errors are service or backend failures From 8fe6160a4ad8524e836aa2f89dd823967714b185 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:54:06 +0200 Subject: [PATCH 14/22] ref(resumable): Treat session tokens as opaque Remove service-side token validation and let backends decide whether a token identifies an upload session. Split the catch-all request error into unknown-session and structured chunk-length outcomes. --- objectstore-server/src/endpoints/common.rs | 5 +- objectstore-server/src/endpoints/mod.rs | 2 +- objectstore-server/src/endpoints/resumable.rs | 10 ++-- objectstore-service/src/backend/common.rs | 7 +++ objectstore-service/src/error.rs | 33 ++++++++----- objectstore-service/src/resumable.rs | 2 +- objectstore-service/src/service.rs | 8 ++-- objectstore-types/src/resumable.rs | 48 +++++-------------- 8 files changed, 55 insertions(+), 60 deletions(-) diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 104f2ba4..96fcde0e 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -96,7 +96,10 @@ impl ApiError { StatusCode::RANGE_NOT_SATISFIABLE } ApiError::Service(ServiceError::InvalidUploadId(_)) => StatusCode::BAD_REQUEST, - ApiError::Service(ServiceError::InvalidUploadRequest(_)) => StatusCode::BAD_REQUEST, + ApiError::Service(ServiceError::UnknownUploadSession) => StatusCode::BAD_REQUEST, + ApiError::Service(ServiceError::ChunkExceedsUploadLength { .. }) => { + StatusCode::BAD_REQUEST + } ApiError::Service(ServiceError::UploadOffsetMismatch { .. }) => StatusCode::CONFLICT, ApiError::Service(ServiceError::UploadSessionGone) => StatusCode::GONE, ApiError::Service(ServiceError::AtCapacity) => StatusCode::TOO_MANY_REQUESTS, diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index 831aa6e7..90dff29f 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -66,7 +66,7 @@ //! //! | Status | Meaning | Client action | //! |--------|---------|---------------| -//! | `400` | Malformed: unusable session, missing `Upload-Length`, nonempty offset query, or a chunk exceeding the declared length | Terminal | +//! | `400` | Malformed: unknown upload session, missing `Upload-Length`, nonempty offset query, or a chunk exceeding the declared length | Terminal | //! | `409` | A chunk's offset does not match, with the authoritative offset in `Upload-Offset` | Resynchronize | //! | `410` | The session expired or was canceled; nothing was retained | Start a new session | //! | `501` | The configured backend does not implement resumable uploads | Fall back to a regular upload | diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index 508ff115..c2ddbe83 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -94,7 +94,7 @@ impl ResumableQuery { } } -/// A validated session token extracted by a continuation or cancellation handler. +/// A session token decoded by a continuation or cancellation handler. #[derive(Debug)] pub(super) struct Session(SessionToken); @@ -460,8 +460,12 @@ mod tests { let error = progress_response(Err(gone), "my-key".into()).unwrap_err(); assert_eq!(error.status(), StatusCode::GONE); - let invalid = ApiError::Service(ServiceError::InvalidUploadRequest("bad".into())); - let error = progress_response(Err(invalid), "my-key".into()).unwrap_err(); + let oversized = ApiError::Service(ServiceError::ChunkExceedsUploadLength { + offset: 8, + content_length: 4, + upload_length: 10, + }); + let error = progress_response(Err(oversized), "my-key".into()).unwrap_err(); assert_eq!(error.status(), StatusCode::BAD_REQUEST); } } diff --git a/objectstore-service/src/backend/common.rs b/objectstore-service/src/backend/common.rs index f1d24d57..ccb59111 100644 --- a/objectstore-service/src/backend/common.rs +++ b/objectstore-service/src/backend/common.rs @@ -93,6 +93,9 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { /// Writes a chunk of `content_length` bytes at `offset` into an open session. /// /// `offset` must equal the offset the backend currently holds. + /// Returns [`Error::UnknownUploadSession`] when `session` does not identify an open session, + /// and [`Error::ChunkExceedsUploadLength`] when the chunk would exceed the total length + /// declared when the session was created. async fn put_chunk( &self, id: &ObjectId, @@ -106,12 +109,16 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { } /// Reports how far the session has progressed. + /// + /// Returns [`Error::UnknownUploadSession`] when `session` does not identify an open session. async fn upload_offset(&self, id: &ObjectId, session: &SessionToken) -> Result { let _ = (id, session); Err(Error::NotImplemented) } /// Cancels an upload session, discarding whatever was uploaded. + /// + /// Returns [`Error::UnknownUploadSession`] when `session` does not identify an open session. async fn cancel_upload( &self, id: &ObjectId, diff --git a/objectstore-service/src/error.rs b/objectstore-service/src/error.rs index ecc613a9..3f8dd71d 100644 --- a/objectstore-service/src/error.rs +++ b/objectstore-service/src/error.rs @@ -152,10 +152,8 @@ pub enum Error { #[error(transparent)] InvalidUploadId(#[from] objectstore_types::multipart::InvalidUploadId), - /// A resumable chunk was submitted at an offset the backend does not hold. - /// - /// The client resynchronizes by continuing from [`offset`](Self::UploadOffsetMismatch::offset), - /// which is authoritative and may be lower than the end of a previously acknowledged chunk. + /// A resumable chunk was submitted at an offset that's different from the one held by the + /// backend. #[error("upload offset mismatch (server holds {offset} bytes)")] UploadOffsetMismatch { /// The offset the backend currently holds. @@ -163,17 +161,25 @@ pub enum Error { }, /// The resumable upload session expired or was canceled, retaining nothing. - /// - /// The client has to start a new session. #[error("upload session gone")] UploadSessionGone, - /// A resumable upload request is unusable for the session it addresses. - /// - /// Covers an unparseable or unknown session token and a chunk that would exceed the - /// length declared when the session was created. - #[error("invalid upload request: {0}")] - InvalidUploadRequest(String), + /// The backend does not recognize the addressed resumable upload session. + #[error("unknown upload session")] + UnknownUploadSession, + + /// A resumable upload chunk would exceed the length declared for the session. + #[error( + "chunk at offset {offset} with length {content_length} exceeds upload length {upload_length}" + )] + ChunkExceedsUploadLength { + /// The offset at which the chunk would be written. + offset: u64, + /// The declared length of the chunk. + content_length: u64, + /// The total upload length declared when the session was created. + upload_length: u64, + }, } impl Error { @@ -222,7 +228,8 @@ impl Error { Self::RangeNotSatisfiable { .. } => Level::DEBUG, Self::UploadOffsetMismatch { .. } => Level::DEBUG, Self::UploadSessionGone => Level::DEBUG, - Self::InvalidUploadRequest(_) => Level::DEBUG, + Self::UnknownUploadSession => Level::DEBUG, + Self::ChunkExceedsUploadLength { .. } => Level::DEBUG, // Indicates that optional functionality is not supported. // We don't want a rogue client spamming us with Sentry errors just by calling an API // that the server doesn't support, so we just log it. diff --git a/objectstore-service/src/resumable.rs b/objectstore-service/src/resumable.rs index bd857fac..1f006834 100644 --- a/objectstore-service/src/resumable.rs +++ b/objectstore-service/src/resumable.rs @@ -12,7 +12,7 @@ //! Declining is a routine outcome rather than an error: the server denies the session and //! the client falls back to a regular upload. -pub use objectstore_types::resumable::{InvalidSessionToken, SessionToken, UploadOffset}; +pub use objectstore_types::resumable::{SessionToken, UploadOffset}; /// How far a resumable upload has progressed. /// diff --git a/objectstore-service/src/service.rs b/objectstore-service/src/service.rs index d33a84fa..a74c5f41 100644 --- a/objectstore-service/src/service.rs +++ b/objectstore-service/src/service.rs @@ -865,7 +865,7 @@ mod tests { async fn resumable_declines_by_default() { let service = make_service(); let id = ObjectId::new(make_context(), "resumable".into()); - let session = SessionToken::new("session".into()).unwrap(); + let session = SessionToken::from("session".to_owned()); let denied = service .create_upload_session(id.clone(), Metadata::default(), 1024) @@ -919,9 +919,7 @@ mod tests { _metadata: &Metadata, total_length: u64, ) -> Result { - Ok(Some( - SessionToken::new(format!("session-{total_length}")).unwrap(), - )) + Ok(Some(SessionToken::from(format!("session-{total_length}")))) } async fn put_chunk( @@ -990,7 +988,7 @@ mod tests { async fn resumable_reports_commit() { let service = resumable_service(UploadProgress::Committed); let id = ObjectId::new(make_context(), "resumable".into()); - let session = SessionToken::new("session".into()).unwrap(); + let session = SessionToken::from("session".to_owned()); let progress = service .put_chunk(id.clone(), session.clone(), 0, 4, stream::single("data")) diff --git a/objectstore-types/src/resumable.rs b/objectstore-types/src/resumable.rs index 321175bb..8c7604a9 100644 --- a/objectstore-types/src/resumable.rs +++ b/objectstore-types/src/resumable.rs @@ -46,36 +46,25 @@ const OFFSET_WILDCARD: &str = "*"; /// Identifier for an in-progress resumable upload session. /// -/// The token is an opaque identifier whose contents are defined by the storage backend. It is -/// validated on construction only to ensure it is non-empty. At the API boundary it is serialized -/// as unpadded base64url, keeping the opaque value out of URL parsing and escaping rules. +/// The token is an opaque identifier whose contents are defined and interpreted by the storage +/// backend. At the API boundary it is serialized as unpadded base64url, keeping the opaque value +/// out of URL parsing and escaping rules. #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub struct SessionToken(String); -/// Error returned when a [`SessionToken`] fails validation. -#[derive(Debug, thiserror::Error)] -#[error("invalid session token: {0}")] -pub struct InvalidSessionToken(String); - impl SessionToken { - /// Creates a new `SessionToken` after validating the input. - /// - /// # Errors - /// - /// Returns [`InvalidSessionToken`] if the string is empty. - pub fn new(s: String) -> Result { - if s.is_empty() { - return Err(InvalidSessionToken("must not be empty".into())); - } - Ok(Self(s)) - } - /// Returns the session token as a string slice. pub fn as_str(&self) -> &str { &self.0 } } +impl From for SessionToken { + fn from(value: String) -> Self { + Self(value) + } +} + impl Deref for SessionToken { type Target = str; @@ -115,7 +104,7 @@ impl<'de> Deserialize<'de> for SessionToken { } let token = String::from_utf8(bytes).map_err(serde::de::Error::custom)?; - Self::new(token).map_err(serde::de::Error::custom) + Ok(Self(token)) } } @@ -188,28 +177,15 @@ pub struct CommitResponse { mod tests { use super::*; - #[test] - fn session_token_accepts_opaque_values() -> Result<(), InvalidSessionToken> { - for value in ["abc123", "..", "/abs", "a/../b", "./a", "a/", "opaque +? ü"] { - assert_eq!(SessionToken::new(value.into())?.as_str(), value); - } - Ok(()) - } - - #[test] - fn session_token_rejects_empty() { - assert!(SessionToken::new(String::new()).is_err()); - } - #[test] fn session_token_serializes_as_unpadded_base64url() -> Result<(), Box> { - let token = SessionToken::new("tok3n".into())?; + let token = SessionToken::from("tok3n".to_owned()); assert_eq!(serde_json::to_string(&token)?, r#""dG9rM24""#); let decoded: SessionToken = serde_json::from_str(r#""dG9rM24""#)?; assert_eq!(decoded, token); - let opaque = SessionToken::new("../escape".into())?; + let opaque = SessionToken::from("../escape".to_owned()); assert_eq!(serde_json::to_string(&opaque)?, r#""Li4vZXNjYXBl""#); let decoded: SessionToken = serde_json::from_str(r#""Li4vZXNjYXBl""#)?; assert_eq!(decoded, opaque); From f0eefde9bd0c5a38869ba341978b3fcd676d662e Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Mon, 31 Aug 2026 10:24:49 +0200 Subject: [PATCH 15/22] improve --- objectstore-server/src/endpoints/mod.rs | 38 ++++++++++++------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index 90dff29f..0103f97d 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -24,12 +24,15 @@ //! //! # Resumable Upload Endpoints //! -//! A resumable upload transfers a single object across several requests. The client opens a -//! session, declaring the object's total size and metadata upfront, and then sends the payload -//! as a sequence of chunks at increasing byte offsets. If a chunk fails, the client asks the -//! server which offset it holds and continues from there, so an interrupted transfer resumes -//! where it stopped instead of starting over. The server knows the total size from the -//! session, so it recognizes the chunk carrying the last byte and commits the object itself. +//! A resumable upload transfers a single object across several requests. +//! The client opens a session, declaring the object's total size and metadata upfront, and +//! then sends the payload as a sequence of chunks at increasing byte offsets. +//! If a chunk fails, the client can ask the server which offset it holds and continue from there, +//! so an interrupted transfer resumes where it stopped instead of starting over. +//! Clients are still encouraged to send the whole payload in a single request, as that's the most +//! efficient and reliable approach. +//! The server knows the total size from the session, so it recognizes the chunk carrying the last +//! byte and commits the object itself. //! //! Resumable uploads use the object endpoints above, selected by a query parameter: //! `upload_type=resumable` opens a session, and `session=` addresses it from then on. @@ -53,16 +56,16 @@ //! `Upload-Offset` header: a byte offset submits the body as the chunk starting there, while //! the `*` wildcard submits an empty body and asks which offset the server holds. Both answer //! `204 No Content` with the authoritative `Upload-Offset` while bytes remain, and -//! `201 Created` with `{"key"}` once the object is committed. **The offset in the response -//! may be lower than the end of the chunk that was sent** — backends persist only aligned -//! prefixes and discard the remainder — so clients always continue from the returned offset. -//! A chunk requires `Content-Length` even over HTTP/2; an offset query does not, but its body must -//! still be empty. The server rejects a session creation or offset query carrying any body bytes -//! with `400 Bad Request`. +//! `201 Created` with `{"key"}` once the object is committed. +//! The offset in the response may be lower than the end of the last chunk that was sent. +//! Backends can e.g. persist only aligned prefixes and discard the remainder, so clients must +//! always continue from the returned offset. +//! Every chunk requires `Content-Length`, even over HTTP/2, while creation and offset queries +//! must not carry a request body. //! -//! An offset query can commit an object that was assembled but not yet committed, so it -//! requires write permission despite being read-shaped. Termination likewise needs write -//! rather than delete permission: it releases an in-progress upload, not an object. +//! An offset query can commit an object, so it requires write permission despite being read-shaped. +//! Termination likewise needs write rather than delete permission: it releases an in-progress upload, +//! not an object. //! //! | Status | Meaning | Client action | //! |--------|---------|---------------| @@ -71,11 +74,6 @@ //! | `410` | The session expired or was canceled; nothing was retained | Start a new session | //! | `501` | The configured backend does not implement resumable uploads | Fall back to a regular upload | //! -//! Not every backend can support this. Session creation asks the backend that would store the -//! object to open one, and a backend that cannot declines, which the server reports as -//! `501 Not Implemented`. No backend implements resumable uploads yet, so every session creation is -//! currently denied. -//! //! # Multipart Upload Endpoints //! //! Multipart uploads are being replaced by [resumable From f537fdc81575eb7cad667385c1f8eafb6df0fcde Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Mon, 31 Aug 2026 10:26:52 +0200 Subject: [PATCH 16/22] improve --- objectstore-server/src/endpoints/objects.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index b13c189c..b4fd0320 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -136,7 +136,7 @@ async fn object_get( insert_accept_ranges(&mut response); return Ok(response); } - Err(error) => return Err(error), + Err(e) => return Err(e), }; let stream = state.meter_stream(stream, &context); From a62a39d9b1afef177be4ef055657fbcb3bfab4b6 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Mon, 31 Aug 2026 11:04:48 +0200 Subject: [PATCH 17/22] ref(resumable): Simplify session operation results Return session tokens and unit values directly instead of hiding them behind response aliases. Unsupported session creation now originates as NotImplemented in the backend, while the HTTP behavior remains 501. --- objectstore-server/src/auth/service.rs | 12 ++---- objectstore-server/src/endpoints/resumable.rs | 13 ++++--- objectstore-server/tests/resumable.rs | 10 ++--- objectstore-service/docs/architecture.md | 6 +-- objectstore-service/src/backend/common.rs | 14 +++---- objectstore-service/src/backend/counting.rs | 10 ++--- objectstore-service/src/backend/testing.rs | 14 +++---- objectstore-service/src/backend/tiered.rs | 8 ++-- objectstore-service/src/resumable.rs | 17 ++------- objectstore-service/src/service.rs | 38 ++++++++----------- 10 files changed, 55 insertions(+), 87 deletions(-) diff --git a/objectstore-server/src/auth/service.rs b/objectstore-server/src/auth/service.rs index e956a31a..43c03701 100644 --- a/objectstore-server/src/auth/service.rs +++ b/objectstore-server/src/auth/service.rs @@ -3,9 +3,7 @@ use objectstore_service::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; -use objectstore_service::resumable::{ - CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress, -}; +use objectstore_service::resumable::{SessionToken, UploadProgress}; use objectstore_service::service::{DeleteResponse, GetResponse, InsertResponse, MetadataResponse}; use objectstore_service::{ClientStream, StorageService}; @@ -203,7 +201,7 @@ impl AuthAwareService { id: ObjectId, metadata: Metadata, total_length: u64, - ) -> ApiResult { + ) -> ApiResult { self.check_permission(Permission::ObjectWrite, id.context())?; Ok(self .service @@ -238,11 +236,7 @@ impl AuthAwareService { } /// Auth-aware wrapper around [`StorageService::cancel_upload`]. - pub async fn cancel_upload( - &self, - id: ObjectId, - session: SessionToken, - ) -> ApiResult { + pub async fn cancel_upload(&self, id: ObjectId, session: SessionToken) -> ApiResult<()> { self.check_permission(Permission::ObjectWrite, id.context())?; Ok(self.service.cancel_upload(id, session).await?) } diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index c2ddbe83..7ab96ad3 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -18,8 +18,9 @@ //! There is no completion request. The total size is known from session creation, so the //! backend recognizes the chunk carrying the last byte and commits the object itself. //! -//! Not every backend supports this. When one declines, session creation answers -//! `501 Not Implemented` and the client performs a regular upload instead. +//! Not every backend supports this. When one refuses to create a session, it returns +//! `NotImplemented`; the endpoint answers `501 Not Implemented` and the client performs a regular +//! upload instead. //! //! [TUS]: https://tus.io/protocols/resumable-upload @@ -232,8 +233,9 @@ pub(super) async fn create_session_for_key( /// Creates a session for the object at `id`. /// -/// Answers `501 Not Implemented` when the backend declines, which tells the client to fall back -/// to a regular upload. Metadata is declared here and does not change afterwards. +/// Answers `501 Not Implemented` when the backend refuses to create a session, which tells the +/// client to fall back to a regular upload. Metadata is declared here and does not change +/// afterwards. async fn create_session_for_id( service: AuthAwareService, state: ServiceState, @@ -253,8 +255,7 @@ async fn create_session_for_id( let session = service .create_upload_session(id.clone(), metadata, total_length) - .await? - .ok_or(ServiceError::NotImplemented)?; + .await?; let body = Json(CreateSessionResponse { key: id.key().to_owned(), diff --git a/objectstore-server/tests/resumable.rs b/objectstore-server/tests/resumable.rs index 794d6a10..70d348c6 100644 --- a/objectstore-server/tests/resumable.rs +++ b/objectstore-server/tests/resumable.rs @@ -1,12 +1,12 @@ //! Integration tests for the resumable upload endpoints. //! -//! No backend implements resumable uploads yet, so the reachable surface is session denial +//! No backend implements resumable uploads yet, so the reachable surface is unsupported sessions //! and request validation. That is deliberate: a deployment must answer `501 Not Implemented` to //! every session creation so clients fall back to a regular upload, and it must reject a //! malformed request before it reaches a backend. //! //! The `501 Not Implemented` assertions are the proof that dispatch and header parsing work: -//! the only way to reach a declining backend method is through a well-formed request. +//! the only way to reach an unsupported backend method is through a well-formed request. use std::io::{Read, Write}; use std::net::TcpStream; @@ -159,7 +159,7 @@ async fn unknown_upload_type_is_rejected() -> Result<()> { // --- Chunks and offset queries --- #[tokio::test] -async fn chunk_reaches_the_declining_backend() -> Result<()> { +async fn chunk_reaches_the_unsupported_backend() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() @@ -174,7 +174,7 @@ async fn chunk_reaches_the_declining_backend() -> Result<()> { } #[tokio::test] -async fn offset_query_reaches_the_declining_backend() -> Result<()> { +async fn offset_query_reaches_the_unsupported_backend() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() @@ -293,7 +293,7 @@ async fn session_token_requires_base64url() -> Result<()> { // --- Cancellation --- #[tokio::test] -async fn cancel_upload_reaches_the_declining_backend() -> Result<()> { +async fn cancel_upload_reaches_the_unsupported_backend() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() diff --git a/objectstore-service/docs/architecture.md b/objectstore-service/docs/architecture.md index 65c50a8c..09246076 100644 --- a/objectstore-service/docs/architecture.md +++ b/objectstore-service/docs/architecture.md @@ -210,9 +210,9 @@ trips for objects large enough that re-sending the whole payload is expensive. the backend recognizes that chunk from the declared total size. 5. At any time, an upload can be canceled, which discards what its session holds. -Not all backends support resumable uploads and can decline creating a session. -Support can depend on the declared size, the metadata, or whether resuming is -possible in principle. +Not all backends support resumable uploads. A backend that refuses to create a session +returns `NotImplemented`; support can depend on the declared size, the metadata, or whether +resuming is possible in principle. ## Multipart Uploads diff --git a/objectstore-service/src/backend/common.rs b/objectstore-service/src/backend/common.rs index ccb59111..69c8c771 100644 --- a/objectstore-service/src/backend/common.rs +++ b/objectstore-service/src/backend/common.rs @@ -13,7 +13,7 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; -use crate::resumable::{CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress}; +use crate::resumable::{SessionToken, UploadProgress}; use crate::stream::{ClientStream, PayloadStream}; /// User agent string used for outgoing requests. @@ -79,15 +79,15 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { /// Object metadata and its total length are declared upfront and cannot be mutated /// during the upload. /// - /// Returns `Ok(None)` when the backend refuses to store the described object resumably. + /// Returns [`Error::NotImplemented`] when the backend refuses to create a session. async fn create_upload_session( &self, id: &ObjectId, metadata: &Metadata, total_length: u64, - ) -> Result { + ) -> Result { let _ = (id, metadata, total_length); - Ok(None) + Err(Error::NotImplemented) } /// Writes a chunk of `content_length` bytes at `offset` into an open session. @@ -119,11 +119,7 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { /// Cancels an upload session, discarding whatever was uploaded. /// /// Returns [`Error::UnknownUploadSession`] when `session` does not identify an open session. - async fn cancel_upload( - &self, - id: &ObjectId, - session: &SessionToken, - ) -> Result { + async fn cancel_upload(&self, id: &ObjectId, session: &SessionToken) -> Result<()> { let _ = (id, session); Err(Error::NotImplemented) } diff --git a/objectstore-service/src/backend/counting.rs b/objectstore-service/src/backend/counting.rs index c5307aae..5ebcc6b5 100644 --- a/objectstore-service/src/backend/counting.rs +++ b/objectstore-service/src/backend/counting.rs @@ -28,7 +28,7 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; -use crate::resumable::{CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress}; +use crate::resumable::{SessionToken, UploadProgress}; use crate::stream::ClientStream; /// Increments `cogs.usage` by one operation for the given `usecase`. @@ -106,7 +106,7 @@ impl Backend for CountingBackend { id: &ObjectId, metadata: &Metadata, total_length: u64, - ) -> Result { + ) -> Result { count(&id.context.usecase); self.inner .create_upload_session(id, metadata, total_length) @@ -132,11 +132,7 @@ impl Backend for CountingBackend { self.inner.upload_offset(id, session).await } - async fn cancel_upload( - &self, - id: &ObjectId, - session: &SessionToken, - ) -> Result { + async fn cancel_upload(&self, id: &ObjectId, session: &SessionToken) -> Result<()> { count(&id.context.usecase); self.inner.cancel_upload(id, session).await } diff --git a/objectstore-service/src/backend/testing.rs b/objectstore-service/src/backend/testing.rs index 8804cfd7..ac2573cc 100644 --- a/objectstore-service/src/backend/testing.rs +++ b/objectstore-service/src/backend/testing.rs @@ -52,7 +52,7 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; -use crate::resumable::{CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress}; +use crate::resumable::{SessionToken, UploadProgress}; use crate::stream::ClientStream; /// Hooks for [`TestBackend`]. @@ -248,7 +248,7 @@ pub trait Hooks: fmt::Debug + Send + Sync + 'static { id: &ObjectId, metadata: &Metadata, total_length: u64, - ) -> Result { + ) -> Result { inner .create_upload_session(id, metadata, total_length) .await @@ -285,7 +285,7 @@ pub trait Hooks: fmt::Debug + Send + Sync + 'static { inner: &InMemoryBackend, id: &ObjectId, session: &SessionToken, - ) -> Result { + ) -> Result<()> { inner.cancel_upload(id, session).await } } @@ -368,7 +368,7 @@ impl Backend for TestBackend { id: &ObjectId, metadata: &Metadata, total_length: u64, - ) -> Result { + ) -> Result { self.hooks .create_upload_session(&self.inner, id, metadata, total_length) .await @@ -391,11 +391,7 @@ impl Backend for TestBackend { self.hooks.upload_offset(&self.inner, id, session).await } - async fn cancel_upload( - &self, - id: &ObjectId, - session: &SessionToken, - ) -> Result { + async fn cancel_upload(&self, id: &ObjectId, session: &SessionToken) -> Result<()> { self.hooks.cancel_upload(&self.inner, id, session).await } } diff --git a/objectstore-service/src/backend/tiered.rs b/objectstore-service/src/backend/tiered.rs index 43c0e96d..65ba32be 100644 --- a/objectstore-service/src/backend/tiered.rs +++ b/objectstore-service/src/backend/tiered.rs @@ -99,11 +99,13 @@ //! //! # Resumable Uploads //! -//! Not implemented here yet, so [`TieredStorage`] inherits the declining defaults from -//! [`Backend`] and every session creation is denied. A resumable upload will be a regular +//! Not implemented here yet, so [`TieredStorage`] inherits the unsupported defaults from +//! [`Backend`] and every session creation returns [`Error::NotImplemented`]. A resumable upload +//! will be a regular //! long-term write whose payload arrives across several requests, reusing the revision keys, //! changelog phases and compare-and-write commit described above: session creation decides -//! the tier from the declared total length and declines if that tier cannot support it, +//! the tier from the declared total length and returns [`Error::NotImplemented`] if that tier +//! cannot support it, //! non-final chunks pass straight through to the upstream session, and the final chunk runs //! the long-term write sequence. diff --git a/objectstore-service/src/resumable.rs b/objectstore-service/src/resumable.rs index 1f006834..3aba2f7d 100644 --- a/objectstore-service/src/resumable.rs +++ b/objectstore-service/src/resumable.rs @@ -6,11 +6,9 @@ //! lands. See [`objectstore_types::resumable`] for the wire-level types. //! //! Not every backend can support this. Session creation therefore asks the backend that -//! would store the object to open one, and a backend that cannot declines by returning -//! `None` from -//! [`Backend::create_upload_session`](crate::backend::common::Backend::create_upload_session). -//! Declining is a routine outcome rather than an error: the server denies the session and -//! the client falls back to a regular upload. +//! would store the object to open one. A backend that cannot create a session returns +//! [`Error::NotImplemented`](crate::error::Error::NotImplemented), and the client falls back to +//! a regular upload. pub use objectstore_types::resumable::{SessionToken, UploadOffset}; @@ -36,12 +34,3 @@ pub enum UploadProgress { /// The last byte arrived and the object is committed and readable. Committed, } - -/// Response for -/// [`Backend::create_upload_session`](crate::backend::common::Backend::create_upload_session). -/// -/// `None` means the backend declines resumable uploads for this object. -pub type CreateSessionResponse = Option; - -/// Response for [`Backend::cancel_upload`](crate::backend::common::Backend::cancel_upload). -pub type CancelUploadResponse = (); diff --git a/objectstore-service/src/service.rs b/objectstore-service/src/service.rs index a74c5f41..4dda8821 100644 --- a/objectstore-service/src/service.rs +++ b/objectstore-service/src/service.rs @@ -20,7 +20,7 @@ use crate::multipart::{ AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, ListPartsResponse, PartNumber, UploadId, UploadPartResponse, }; -use crate::resumable::{CancelUploadResponse, CreateSessionResponse, SessionToken, UploadProgress}; +use crate::resumable::{SessionToken, UploadProgress}; use crate::stream::{ClientStream, PayloadStream}; use crate::streaming::StreamExecutor; @@ -365,15 +365,15 @@ impl StorageService { /// Opens a resumable upload session for an object of `total_length` bytes. /// - /// Returns `Ok(None)` when the backend declines resumable uploads for this object, in - /// which case the caller should fall back to [`Self::insert_object`]. Unlike the - /// multipart operations there is no eager capability probe: support is the return value. + /// Returns [`Error::NotImplemented`](crate::error::Error::NotImplemented) when the backend + /// refuses to create a session, in which case the caller should fall back to + /// [`Self::insert_object`]. pub async fn create_upload_session( &self, id: ObjectId, metadata: Metadata, total_length: u64, - ) -> Result { + ) -> Result { metadata.validate()?; let inner = Arc::clone(&self.inner); self.spawn("create_upload_session", async move { @@ -425,11 +425,7 @@ impl StorageService { } /// Cancels an upload session, discarding whatever was uploaded. - pub async fn cancel_upload( - &self, - id: ObjectId, - session: SessionToken, - ) -> Result { + pub async fn cancel_upload(&self, id: ObjectId, session: SessionToken) -> Result<()> { let inner = Arc::clone(&self.inner); self.spawn("cancel_upload", async move { inner.cancel_upload(&id, &session).await @@ -862,19 +858,18 @@ mod tests { // --- Resumable uploads --- #[tokio::test] - async fn resumable_declines_by_default() { + async fn resumable_is_not_implemented_by_default() { let service = make_service(); let id = ObjectId::new(make_context(), "resumable".into()); let session = SessionToken::from("session".to_owned()); - let denied = service + let creation = service .create_upload_session(id.clone(), Metadata::default(), 1024) - .await - .unwrap(); - assert!(denied.is_none(), "expected the backend to decline"); + .await; + assert!(matches!(creation, Err(Error::NotImplemented))); - // Without a session no other operation is reachable through the API, but the - // declining defaults must still be wired up rather than panicking. + // Without a session no other operation is reachable through the API, but the defaults + // must still be wired up rather than panicking. let chunk = service .put_chunk(id.clone(), session.clone(), 0, 4, stream::single("data")) .await; @@ -918,8 +913,8 @@ mod tests { _id: &ObjectId, _metadata: &Metadata, total_length: u64, - ) -> Result { - Ok(Some(SessionToken::from(format!("session-{total_length}")))) + ) -> Result { + Ok(SessionToken::from(format!("session-{total_length}"))) } async fn put_chunk( @@ -948,7 +943,7 @@ mod tests { _inner: &InMemoryBackend, _id: &ObjectId, _session: &SessionToken, - ) -> Result { + ) -> Result<()> { Ok(()) } } @@ -965,8 +960,7 @@ mod tests { let session = service .create_upload_session(id.clone(), Metadata::default(), 1024) .await - .unwrap() - .expect("session was declined"); + .unwrap(); assert_eq!(session.as_str(), "session-1024"); let progress = service From 83db06954a0dd61998acce5db882b8ceee223404 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Mon, 31 Aug 2026 11:10:54 +0200 Subject: [PATCH 18/22] improve --- objectstore-service/src/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/objectstore-service/src/service.rs b/objectstore-service/src/service.rs index 4dda8821..e7f18070 100644 --- a/objectstore-service/src/service.rs +++ b/objectstore-service/src/service.rs @@ -411,7 +411,7 @@ impl StorageService { /// Reports how far a session has progressed, committing the object if it is assembled. /// - /// This mutates state and therefore requires write permission at the API layer. + /// This can mutate state and therefore requires write permission at the API layer. pub async fn upload_offset( &self, id: ObjectId, From 186993903738fa3c2963df806cae557124bb14e9 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Mon, 31 Aug 2026 11:18:03 +0200 Subject: [PATCH 19/22] test(service): Remove redundant resumable wrapper tests Keep the metadata-validation regression test while dropping pass-through and default-behavior coverage already exercised by endpoint integration tests. Preserve the reusable backend Hooks infrastructure for future implementations. --- objectstore-service/src/service.rs | 121 ----------------------------- 1 file changed, 121 deletions(-) diff --git a/objectstore-service/src/service.rs b/objectstore-service/src/service.rs index e7f18070..95fbe2b9 100644 --- a/objectstore-service/src/service.rs +++ b/objectstore-service/src/service.rs @@ -857,29 +857,6 @@ mod tests { // --- Resumable uploads --- - #[tokio::test] - async fn resumable_is_not_implemented_by_default() { - let service = make_service(); - let id = ObjectId::new(make_context(), "resumable".into()); - let session = SessionToken::from("session".to_owned()); - - let creation = service - .create_upload_session(id.clone(), Metadata::default(), 1024) - .await; - assert!(matches!(creation, Err(Error::NotImplemented))); - - // Without a session no other operation is reachable through the API, but the defaults - // must still be wired up rather than panicking. - let chunk = service - .put_chunk(id.clone(), session.clone(), 0, 4, stream::single("data")) - .await; - assert!(matches!(chunk, Err(Error::NotImplemented))); - let offset = service.upload_offset(id.clone(), session.clone()).await; - assert!(matches!(offset, Err(Error::NotImplemented))); - let canceled = service.cancel_upload(id, session).await; - assert!(matches!(canceled, Err(Error::NotImplemented))); - } - #[tokio::test] async fn resumable_create_validates_metadata() { let service = make_service(); @@ -895,102 +872,4 @@ mod tests { let result = service.create_upload_session(id, metadata, 1024).await; assert!(matches!(result, Err(Error::Metadata(_))), "{result:?}"); } - - /// Backend that accepts resumable uploads and reports a fixed progression. - /// - /// Records nothing: it exists to prove that [`StorageService`] forwards arguments and - /// returns backend outcomes untouched. - #[derive(Clone, Debug)] - struct AcceptResumable { - progress: UploadProgress, - } - - #[async_trait::async_trait] - impl Hooks for AcceptResumable { - async fn create_upload_session( - &self, - _inner: &InMemoryBackend, - _id: &ObjectId, - _metadata: &Metadata, - total_length: u64, - ) -> Result { - Ok(SessionToken::from(format!("session-{total_length}"))) - } - - async fn put_chunk( - &self, - _inner: &InMemoryBackend, - _id: &ObjectId, - _session: &SessionToken, - _offset: u64, - _content_length: u64, - _stream: ClientStream, - ) -> Result { - Ok(self.progress) - } - - async fn upload_offset( - &self, - _inner: &InMemoryBackend, - _id: &ObjectId, - _session: &SessionToken, - ) -> Result { - Ok(self.progress) - } - - async fn cancel_upload( - &self, - _inner: &InMemoryBackend, - _id: &ObjectId, - _session: &SessionToken, - ) -> Result<()> { - Ok(()) - } - } - - fn resumable_service(progress: UploadProgress) -> StorageService { - StorageService::new(Box::new(TestBackend::new(AcceptResumable { progress }))) - } - - #[tokio::test] - async fn resumable_reports_incomplete_progress() { - let service = resumable_service(UploadProgress::Incomplete { offset: 262_144 }); - let id = ObjectId::new(make_context(), "resumable".into()); - - let session = service - .create_upload_session(id.clone(), Metadata::default(), 1024) - .await - .unwrap(); - assert_eq!(session.as_str(), "session-1024"); - - let progress = service - .put_chunk(id.clone(), session.clone(), 0, 4, stream::single("data")) - .await - .unwrap(); - assert_eq!(progress, UploadProgress::Incomplete { offset: 262_144 }); - - let progress = service.upload_offset(id.clone(), session.clone()).await; - assert_eq!( - progress.unwrap(), - UploadProgress::Incomplete { offset: 262_144 } - ); - - service.cancel_upload(id, session).await.unwrap(); - } - - #[tokio::test] - async fn resumable_reports_commit() { - let service = resumable_service(UploadProgress::Committed); - let id = ObjectId::new(make_context(), "resumable".into()); - let session = SessionToken::from("session".to_owned()); - - let progress = service - .put_chunk(id.clone(), session.clone(), 0, 4, stream::single("data")) - .await - .unwrap(); - assert_eq!(progress, UploadProgress::Committed); - - let progress = service.upload_offset(id, session).await.unwrap(); - assert_eq!(progress, UploadProgress::Committed); - } } From 07fe1133fd977e576d9334872abb45832f7317f7 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Mon, 31 Aug 2026 12:38:28 +0200 Subject: [PATCH 20/22] ref(resumable): Scope token encoding to query parameters Represent session tokens as opaque strings so session creation returns backend values unchanged in JSON. Decode canonical unpadded base64url only when extracting the session query parameter, and update the protocol documentation and focused tests for that boundary. --- Cargo.lock | 1 + objectstore-server/Cargo.toml | 1 + objectstore-server/src/endpoints/mod.rs | 3 +- objectstore-server/src/endpoints/resumable.rs | 39 +++++- objectstore-types/src/resumable.rs | 132 +++--------------- 5 files changed, 59 insertions(+), 117 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a08275d6..c8a90870 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2809,6 +2809,7 @@ dependencies = [ "argh", "async-stream", "axum", + "base64", "bytes", "ed25519-dalek", "elegant-departure", diff --git a/objectstore-server/Cargo.toml b/objectstore-server/Cargo.toml index 3e3d446b..13d4da82 100644 --- a/objectstore-server/Cargo.toml +++ b/objectstore-server/Cargo.toml @@ -15,6 +15,7 @@ anyhow = { workspace = true } argh = { workspace = true } async-stream = { workspace = true } axum = { workspace = true, features = ["multipart"] } +base64 = { workspace = true } bytes = { workspace = true } ed25519-dalek = { workspace = true, features = ["pem"] } elegant-departure = { workspace = true, features = ["tokio"] } diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index 0103f97d..42a9755d 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -36,7 +36,8 @@ //! //! Resumable uploads use the object endpoints above, selected by a query parameter: //! `upload_type=resumable` opens a session, and `session=` addresses it from then on. -//! Session tokens are unpadded base64url at the API boundary. +//! Session creation returns the opaque backend token unchanged; subsequent requests encode it as +//! unpadded base64url in the `session` query parameter. //! The object is named by the request path as usual, and [`objectstore_types::resumable`] //! holds the protocol types. //! diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index 7ab96ad3..678591c0 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -5,7 +5,8 @@ //! path with the session in the query string, so these handlers have no router of their own: //! [`ResumableTarget`] classifies the query before thin handlers in [`objects`](super::objects) //! dispatch the original request here, where each operation runs with its own Axum extractors. -//! Session tokens are encoded as unpadded base64url at this API boundary. +//! Session creation returns the opaque backend token unchanged. Subsequent requests encode that +//! token as unpadded base64url in the `session` query parameter. //! //! | Operation | Request | Success | //! |---|---|---| @@ -28,6 +29,8 @@ use axum::extract::{FromRequestParts, OptionalFromRequestParts, Query, State}; use axum::http::{HeaderMap, StatusCode, request::Parts}; use axum::response::{IntoResponse, Response}; use axum::{Json, http}; +use base64::Engine as _; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; use futures_util::TryStreamExt; use objectstore_service::error::Error as ServiceError; use objectstore_service::id::{ObjectContext, ObjectId}; @@ -101,7 +104,22 @@ pub(super) struct Session(SessionToken); #[derive(Debug, Deserialize)] struct SessionQuery { - session: SessionToken, + session: String, +} + +/// Decodes the session token from its query-string representation. +fn decode_session_token(encoded: &str) -> ApiResult { + let bytes = URL_SAFE_NO_PAD + .decode(encoded) + .map_err(|error| ApiError::Client(error.to_string()))?; + + if URL_SAFE_NO_PAD.encode(&bytes) != encoded { + return Err(ApiError::Client( + "session token must use unpadded base64url encoding".into(), + )); + } + + String::from_utf8(bytes).map_err(|error| ApiError::Client(error.to_string())) } impl FromRequestParts for Session @@ -113,7 +131,7 @@ where async fn from_request_parts(parts: &mut Parts, _state: &S) -> ApiResult { let Query(SessionQuery { session }) = Query::::try_from_uri(&parts.uri) .map_err(|error| ApiError::Client(error.to_string()))?; - Ok(Session(session)) + Ok(Session(decode_session_token(&session)?)) } } @@ -374,6 +392,21 @@ mod tests { assert!(matches!(result, Err(ApiError::Client(_))), "{result:?}"); } + #[test] + fn session_token_decodes_from_unpadded_base64url() { + assert_eq!(decode_session_token("Li4vZXNjYXBl").unwrap(), "../escape"); + } + + #[test] + fn session_token_rejects_invalid_query_encodings() { + for invalid in ["%%%", "dG9rM24=", "_w"] { + assert!( + decode_session_token(invalid).is_err(), + "accepted {invalid:?}" + ); + } + } + #[test] fn upload_length_requires_a_byte_count() { let mut headers = HeaderMap::new(); diff --git a/objectstore-types/src/resumable.rs b/objectstore-types/src/resumable.rs index 8c7604a9..48697aaa 100644 --- a/objectstore-types/src/resumable.rs +++ b/objectstore-types/src/resumable.rs @@ -1,37 +1,13 @@ //! Types for the resumable upload protocol. -//! -//! A resumable upload declares the object's total size and metadata upfront, then -//! sends the payload as a sequence of chunks at increasing byte offsets. If a chunk -//! fails, the client asks the server which offset it holds and continues from there. -//! There is no finalize request: the server knows the total length from the session, -//! so it recognizes the chunk carrying the last byte and commits the object itself. -//! -//! Every request addresses the regular object endpoints with the session in the query -//! string. [`SessionToken`] serializes as unpadded base64url at that API boundary. Header -//! names are borrowed from [TUS] where they fit, but this is not a TUS implementation: there -//! is no version negotiation, no capability discovery, and no support for uploads of unknown -//! length. -//! -//! Key types: -//! - [`SessionToken`] — opaque identifier for an in-progress upload session. -//! - [`UploadOffset`] — the value of the [`HEADER_UPLOAD_OFFSET`] header. -//! - [`CreateSessionResponse`] — returned when a new session is created. -//! - [`CommitResponse`] — returned by the request that commits the object. -//! -//! [TUS]: https://tus.io/protocols/resumable-upload use std::fmt; -use std::ops::Deref; use std::str::FromStr; -use base64::Engine as _; -use base64::engine::general_purpose::URL_SAFE_NO_PAD; -use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use serde::{Deserialize, Serialize}; /// Request header declaring the total size of the object, in bytes. /// -/// Required when creating a session. The server needs the total size to select a -/// backend and to recognize the final chunk. +/// Required when creating a session. pub const HEADER_UPLOAD_LENGTH: &str = "upload-length"; /// Header carrying the byte offset of a chunk, or the offset the server holds. @@ -47,77 +23,18 @@ const OFFSET_WILDCARD: &str = "*"; /// Identifier for an in-progress resumable upload session. /// /// The token is an opaque identifier whose contents are defined and interpreted by the storage -/// backend. At the API boundary it is serialized as unpadded base64url, keeping the opaque value -/// out of URL parsing and escaping rules. -#[derive(Clone, Debug, PartialEq, Eq, Hash)] -pub struct SessionToken(String); - -impl SessionToken { - /// Returns the session token as a string slice. - pub fn as_str(&self) -> &str { - &self.0 - } -} - -impl From for SessionToken { - fn from(value: String) -> Self { - Self(value) - } -} - -impl Deref for SessionToken { - type Target = str; - - fn deref(&self) -> &str { - &self.0 - } -} - -impl fmt::Display for SessionToken { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0.fmt(f) - } -} - -impl Serialize for SessionToken { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - serializer.serialize_str(&URL_SAFE_NO_PAD.encode(self.0.as_bytes())) - } -} - -impl<'de> Deserialize<'de> for SessionToken { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let encoded = String::deserialize(deserializer)?; - let bytes = URL_SAFE_NO_PAD - .decode(&encoded) - .map_err(serde::de::Error::custom)?; - if URL_SAFE_NO_PAD.encode(&bytes) != encoded { - return Err(serde::de::Error::custom( - "session token must use canonical unpadded base64url", - )); - } - - let token = String::from_utf8(bytes).map_err(serde::de::Error::custom)?; - Ok(Self(token)) - } -} +/// backend. +pub type SessionToken = String; /// The value of the [`HEADER_UPLOAD_OFFSET`] request header. /// -/// A concrete offset submits a chunk starting at that byte. The wildcard `*` submits -/// no payload and instead asks the server which offset it holds, which is also the -/// request that commits an object that was assembled but not yet committed. +/// In a request, a concrete offset submits a chunk starting at that byte, +/// while [`UploadOffset::Unknown`] asks the server which offset it holds. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum UploadOffset { - /// `Upload-Offset: ` — a chunk whose first byte sits at this offset. + /// Denotes a chunk whose first byte sits at this offset. At(u64), - /// `Upload-Offset: *` — a query for the server's authoritative offset. + /// Used to query the server for its authoritative offset. Unknown, } @@ -159,7 +76,7 @@ impl fmt::Display for UploadOffset { pub struct CreateSessionResponse { /// The object key (server-generated or client-provided). pub key: String, - /// The session token for subsequent requests, serialized as unpadded base64url. + /// The opaque session token for subsequent requests. pub session: SessionToken, } @@ -178,30 +95,19 @@ mod tests { use super::*; #[test] - fn session_token_serializes_as_unpadded_base64url() -> Result<(), Box> { - let token = SessionToken::from("tok3n".to_owned()); - assert_eq!(serde_json::to_string(&token)?, r#""dG9rM24""#); - - let decoded: SessionToken = serde_json::from_str(r#""dG9rM24""#)?; - assert_eq!(decoded, token); - - let opaque = SessionToken::from("../escape".to_owned()); - assert_eq!(serde_json::to_string(&opaque)?, r#""Li4vZXNjYXBl""#); - let decoded: SessionToken = serde_json::from_str(r#""Li4vZXNjYXBl""#)?; - assert_eq!(decoded, opaque); + fn create_session_response_serializes_token_verbatim() -> Result<(), serde_json::Error> { + let response = CreateSessionResponse { + key: "key".into(), + session: "../opaque +? ü".into(), + }; + + assert_eq!( + serde_json::to_string(&response)?, + r#"{"key":"key","session":"../opaque +? ü"}"# + ); Ok(()) } - #[test] - fn session_token_rejects_invalid_api_encodings() { - for invalid in [r#""%%%""#, r#""dG9rM24=""#, r#""_w""#] { - assert!( - serde_json::from_str::(invalid).is_err(), - "accepted {invalid}" - ); - } - } - #[test] fn upload_offset_parses_wildcard_and_offsets() -> Result<(), InvalidUploadOffset> { assert_eq!("*".parse::()?, UploadOffset::Unknown); From f47758e6e689f275a0f6203925e6698f572f1dcd Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Mon, 31 Aug 2026 12:49:40 +0200 Subject: [PATCH 21/22] improve --- objectstore-server/src/endpoints/resumable.rs | 34 +++---------------- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/objectstore-server/src/endpoints/resumable.rs b/objectstore-server/src/endpoints/resumable.rs index 678591c0..a8d63c34 100644 --- a/objectstore-server/src/endpoints/resumable.rs +++ b/objectstore-server/src/endpoints/resumable.rs @@ -1,12 +1,8 @@ //! Resumable upload endpoints. //! //! Resumable uploads are a variation of the regular object endpoints rather than a separate -//! resource, following GCS and S3 rather than [TUS]. Every request addresses the same object -//! path with the session in the query string, so these handlers have no router of their own: -//! [`ResumableTarget`] classifies the query before thin handlers in [`objects`](super::objects) -//! dispatch the original request here, where each operation runs with its own Axum extractors. -//! Session creation returns the opaque backend token unchanged. Subsequent requests encode that -//! token as unpadded base64url in the `session` query parameter. +//! resource. Every request addresses a standard object path with the session in the query string +//! (or `upload_type=resumable` for creation). //! //! | Operation | Request | Success | //! |---|---|---| @@ -15,15 +11,6 @@ //! | Chunk | `PUT …/{key}?session=` with `Upload-Offset: ` | `204` + `Upload-Offset`, or `201` + `{"key"}` | //! | Offset query | `PUT …/{key}?session=` with `Upload-Offset: *` | `204` + `Upload-Offset`, or `201` + `{"key"}` | //! | Cancel | `DELETE …/{key}?session=` | `204` | -//! -//! There is no completion request. The total size is known from session creation, so the -//! backend recognizes the chunk carrying the last byte and commits the object itself. -//! -//! Not every backend supports this. When one refuses to create a session, it returns -//! `NotImplemented`; the endpoint answers `501 Not Implemented` and the client performs a regular -//! upload instead. -//! -//! [TUS]: https://tus.io/protocols/resumable-upload use axum::extract::{FromRequestParts, OptionalFromRequestParts, Query, State}; use axum::http::{HeaderMap, StatusCode, request::Parts}; @@ -48,9 +35,6 @@ use crate::extractors::{Xt, body::MeteredBody}; use crate::state::ServiceState; /// The `upload_type` query parameter. -/// -/// Only one value is accepted, so an unrecognized upload type is a deserialization failure -/// and therefore a `400` rather than being silently treated as a regular upload. #[derive(Clone, Copy, Debug, Deserialize, PartialEq, Eq)] #[serde(rename_all = "snake_case")] pub(super) enum UploadType { @@ -59,9 +43,6 @@ pub(super) enum UploadType { } /// The resumable protocol's query parameters, as seen on a regular object route. -/// -/// Both fields are optional and unknown parameters are ignored, because pre-signed URLs put -/// their own `os_*` parameters into the same query string. #[derive(Debug, Deserialize)] struct ResumableQuery { /// Present on a session creation request. @@ -84,8 +65,7 @@ impl ResumableQuery { /// /// # Errors /// - /// Returns [`ApiError::Client`] if both parameters are present. They address different - /// operations, so a request carrying both is ambiguous rather than defaulted. + /// Returns [`ApiError::Client`] if both parameters are present. fn classify(self) -> ApiResult> { match (self.upload_type, self.session) { (Some(_), Some(_)) => Err(ApiError::Client( @@ -251,9 +231,8 @@ pub(super) async fn create_session_for_key( /// Creates a session for the object at `id`. /// -/// Answers `501 Not Implemented` when the backend refuses to create a session, which tells the -/// client to fall back to a regular upload. Metadata is declared here and does not change -/// afterwards. +/// Answers `501 Not Implemented` when the backend refuses to create a session or doesn't implement +/// resumable uploads. async fn create_session_for_id( service: AuthAwareService, state: ServiceState, @@ -332,9 +311,6 @@ pub(super) async fn cancel_session( } /// Turns an [`UploadProgress`] outcome into the response shared by chunks and offset queries. -/// -/// An offset mismatch is answered here rather than through [`ApiError::status`], because the -/// authoritative offset has to travel in a header that a generic error response cannot set. fn progress_response(progress: ApiResult, key: String) -> ApiResult { let progress = match progress { Ok(progress) => progress, From 01a2072a202fba14f1727701c9b3aa62aa2582c9 Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Mon, 31 Aug 2026 13:00:40 +0200 Subject: [PATCH 22/22] test(server): Remove unsupported resumable assertions Keep the endpoint integration suite focused on request validation and regular-route behavior until a backend supporting resumable uploads is available for successful protocol tests. --- objectstore-server/tests/resumable.rs | 97 +-------------------------- 1 file changed, 2 insertions(+), 95 deletions(-) diff --git a/objectstore-server/tests/resumable.rs b/objectstore-server/tests/resumable.rs index 70d348c6..8a7a6592 100644 --- a/objectstore-server/tests/resumable.rs +++ b/objectstore-server/tests/resumable.rs @@ -1,12 +1,7 @@ //! Integration tests for the resumable upload endpoints. //! -//! No backend implements resumable uploads yet, so the reachable surface is unsupported sessions -//! and request validation. That is deliberate: a deployment must answer `501 Not Implemented` to -//! every session creation so clients fall back to a regular upload, and it must reject a -//! malformed request before it reaches a backend. -//! -//! The `501 Not Implemented` assertions are the proof that dispatch and header parsing work: -//! the only way to reach an unsupported backend method is through a well-formed request. +//! No backend implements resumable uploads yet. Until a supporting test backend exists, these +//! tests cover request validation and ensure regular object requests remain unaffected. use std::io::{Read, Write}; use std::net::TcpStream; @@ -64,34 +59,6 @@ async fn raw_put(server: &TestServer, path: &str, headers: &str, body: &str) -> // --- Session creation --- -#[tokio::test] -async fn unsupported_create_session_with_client_key_is_not_implemented() -> Result<()> { - let server = test_server().await; - - let response = reqwest::Client::new() - .put(server.url("/v1/objects/test/org=1/my-key?upload_type=resumable")) - .header(HEADER_UPLOAD_LENGTH, "1048576") - .send() - .await?; - - assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); - Ok(()) -} - -#[tokio::test] -async fn unsupported_create_session_with_generated_key_is_not_implemented() -> Result<()> { - let server = test_server().await; - - let response = reqwest::Client::new() - .post(server.url("/v1/objects/test/org=1/?upload_type=resumable")) - .header(HEADER_UPLOAD_LENGTH, "1048576") - .send() - .await?; - - assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); - Ok(()) -} - #[tokio::test] async fn create_session_requires_upload_length() -> Result<()> { let server = test_server().await; @@ -158,53 +125,6 @@ async fn unknown_upload_type_is_rejected() -> Result<()> { // --- Chunks and offset queries --- -#[tokio::test] -async fn chunk_reaches_the_unsupported_backend() -> Result<()> { - let server = test_server().await; - - let response = reqwest::Client::new() - .put(server.url(&format!("/v1/objects/test/org=1/my-key?session={SESSION}"))) - .header(HEADER_UPLOAD_OFFSET, "0") - .body("payload") - .send() - .await?; - - assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); - Ok(()) -} - -#[tokio::test] -async fn offset_query_reaches_the_unsupported_backend() -> Result<()> { - let server = test_server().await; - - let response = reqwest::Client::new() - .put(server.url(&format!("/v1/objects/test/org=1/my-key?session={SESSION}"))) - .header(HEADER_UPLOAD_OFFSET, "*") - .send() - .await?; - - assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); - Ok(()) -} - -#[tokio::test] -async fn offset_query_allows_missing_content_length() -> Result<()> { - let server = test_server().await; - let response = raw_put( - &server, - &format!("/v1/objects/test/org=1/my-key?session={SESSION}"), - &format!("{HEADER_UPLOAD_OFFSET}: *\r\n"), - "", - ) - .await?; - - assert!( - response.starts_with("HTTP/1.1 501 Not Implemented\r\n"), - "unexpected response: {response}" - ); - Ok(()) -} - #[tokio::test] async fn offset_query_rejects_chunked_body_without_content_length() -> Result<()> { let server = test_server().await; @@ -292,19 +212,6 @@ async fn session_token_requires_base64url() -> Result<()> { // --- Cancellation --- -#[tokio::test] -async fn cancel_upload_reaches_the_unsupported_backend() -> Result<()> { - let server = test_server().await; - - let response = reqwest::Client::new() - .delete(server.url(&format!("/v1/objects/test/org=1/my-key?session={SESSION}"))) - .send() - .await?; - - assert_eq!(response.status(), StatusCode::NOT_IMPLEMENTED); - Ok(()) -} - #[tokio::test] async fn delete_rejects_upload_type() -> Result<()> { let server = test_server().await;