ref(server): Use "correct" methods and status codes for creation (#215)

Creating an object with or without a key usually uses different HTTP
verbs. PUT writes or overrides an object at a known key, while POST
always adds a new object with a server-defined key. The status codes
also differ, with POST returning 201 CREATED.

If we think this is useful, we could also make PUT return 201, but for
that we'd have to return from the backend whether the object previously
existed.

Internally, we now have two types:

- `ObjectPath` requires a key to be present. It is used for GET and
  DELETE.
- `OptionalObjectPath` has an optional key. It can be converted into
  `ObjectPath`, either requiring or creating a random key.

ObjectPath deserializes via OptionalObjectPath.

Author: jan-auer

clients/python/src/objectstore_client/client.py

@@ -257,11 +257,14 @@ def put(
             for k, v in metadata.items():
                 headers[f"{HEADER_META_PREFIX}{k}"] = v
 
+        if key == "":
+            key = None
+
         with measure_storage_operation(
             self._metrics_backend, "put", self._usecase.name
         ) as metric_emitter:
             response = self._pool.request(
-                "PUT",
+                "POST" if not key else "PUT",
                 self._make_url(key),
                 body=body,
                 headers=headers,

clients/python/tests/test_e2e.py

@@ -103,15 +103,18 @@ def test_full_cycle(server_url: str) -> None:
 
     session = client.session(test_usecase, org=42, project=1337)
 
-    data = b"test data"
-
-    object_key = session.put(data)
+    object_key = session.put(b"test data")
     assert object_key is not None
 
     retrieved = session.get(object_key)
-    assert retrieved.payload.read() == data
+    assert retrieved.payload.read() == b"test data"
     assert retrieved.metadata.time_created is not None
 
+    new_key = session.put(b"new data", key=object_key)
+    assert new_key == object_key
+    retrieved = session.get(object_key)
+    assert retrieved.payload.read() == b"new data"
+
     session.delete(object_key)
 
     with pytest.raises(RequestError, check=lambda e: e.status == 404):
@@ -168,4 +171,4 @@ def test_connect_timeout() -> None:
     )
 
     with pytest.raises(urllib3.exceptions.MaxRetryError):
-        session.put(b"foo")
+        session.get("foo")

clients/rust/src/put.rs

@@ -84,7 +84,7 @@ impl PutBuilder {
     /// If a key is specified, the object will be stored under that key. Otherwise, the Objectstore
     /// server will automatically assign a random key, which is then returned from this request.
     pub fn key(mut self, key: impl Into<String>) -> Self {
-        self.key = Some(key.into());
+        self.key = Some(key.into()).filter(|k| !k.is_empty());
         self
     }
 
@@ -138,10 +138,14 @@ impl PutBuilder {
 impl PutBuilder {
     /// Sends the built PUT request to the upstream service.
     pub async fn send(self) -> crate::Result<PutResponse> {
-        let mut builder = self.session.request(
-            reqwest::Method::PUT,
-            self.key.as_deref().unwrap_or_default(),
-        );
+        let method = match self.key {
+            Some(_) => reqwest::Method::PUT,
+            None => reqwest::Method::POST,
+        };
+
+        let mut builder = self
+            .session
+            .request(method, self.key.as_deref().unwrap_or_default());
 
         let body = match (self.metadata.compression, self.body) {
             (Some(Compression::Zstd), PutBody::Buffer(bytes)) => {

clients/rust/src/tests.rs

@@ -107,6 +107,30 @@ async fn stores_under_given_key() {
     assert_eq!(stored_id, "test-key123!!");
 }
 
+#[tokio::test]
+async fn overwrites_existing_key() {
+    let server = TestServer::new().await;
+
+    let client = ClientBuilder::new(server.url("/")).build().unwrap();
+    let usecase = Usecase::new("usecase");
+    let session = client.session(usecase.for_project(12345, 1337)).unwrap();
+
+    let stored_id = session.put("initial body").send().await.unwrap().key;
+    let overwritten_id = session
+        .put("new body")
+        .key(&stored_id)
+        .send()
+        .await
+        .unwrap()
+        .key;
+
+    assert_eq!(stored_id, overwritten_id);
+
+    let response = session.get(&stored_id).send().await.unwrap().unwrap();
+    let payload = response.payload().await.unwrap();
+    assert_eq!(payload, "new body");
+}
+
 #[derive(Debug)]
 pub struct TestServer {
     handle: tokio::task::JoinHandle<()>,

objectstore-server/src/endpoints.rs

@@ -6,12 +6,12 @@ use std::time::SystemTime;
 use anyhow::Context;
 use axum::body::Body;
 use axum::extract::{Path, State};
-use axum::http::{HeaderMap, StatusCode};
+use axum::http::{HeaderMap, Method, StatusCode};
 use axum::response::{IntoResponse, Response};
-use axum::routing::{get, put};
+use axum::routing;
 use axum::{Json, Router};
 use futures_util::{StreamExt, TryStreamExt};
-use objectstore_service::ObjectPath;
+use objectstore_service::{ObjectPath, OptionalObjectPath};
 use objectstore_types::Metadata;
 use serde::Serialize;
 
@@ -21,11 +21,14 @@ use crate::state::ServiceState;
 pub fn routes() -> Router<ServiceState> {
     let service_routes = Router::new().route(
         "/{*path}",
-        put(put_object).get(get_object).delete(delete_object),
+        routing::post(insert_object)
+            .put(insert_object)
+            .get(get_object)
+            .delete(delete_object),
     );
 
     Router::new()
-        .route("/health", get(health))
+        .route("/health", routing::get(health))
         .nest("/v1/", service_routes)
 }
 
@@ -38,24 +41,37 @@ struct PutBlobResponse {
     key: String,
 }
 
-async fn put_object(
+async fn insert_object(
     State(state): State<ServiceState>,
-    Path(path): Path<ObjectPath>,
+    Path(path): Path<OptionalObjectPath>,
+    method: Method,
     headers: HeaderMap,
     body: Body,
-) -> ApiResult<impl IntoResponse> {
+) -> ApiResult<Response> {
+    let (expected_method, response_status) = match path.key {
+        Some(_) => (Method::PUT, StatusCode::OK),
+        None => (Method::POST, StatusCode::CREATED),
+    };
+
+    // TODO: For now allow PUT everywhere. Remove the second condition when all clients are updated.
+    if method != expected_method && method == Method::POST {
+        return Ok(StatusCode::METHOD_NOT_ALLOWED.into_response());
+    }
+
+    let path = path.create_key();
     populate_sentry_scope(&path);
 
     let mut metadata =
         Metadata::from_headers(&headers, "").context("extracting metadata from headers")?;
     metadata.time_created = Some(SystemTime::now());
 
     let stream = body.into_data_stream().map_err(io::Error::other).boxed();
-    let key = state.service.put_object(path, &metadata, stream).await?;
+    let response_path = state.service.put_object(path, &metadata, stream).await?;
+    let response = Json(PutBlobResponse {
+        key: response_path.key.to_string(),
+    });
 
-    Ok(Json(PutBlobResponse {
-        key: key.key.to_string(),
-    }))
+    Ok((response_status, response).into_response())
 }
 
 async fn get_object(

objectstore-service/src/path.rs

@@ -5,39 +5,66 @@ use serde::de;
 /// Magic URL segment that separates objectstore context from an object's user-provided key.
 const PATH_CONTEXT_SEPARATOR: &str = "objects";
 
-/// The fully scoped path of an object.
-///
-/// This consists of a usecase, the scope, and the user-defined object key.
+/// An [`ObjectPath`] that may or may not have a user-provided key.
+// DO NOT derive Eq, see the implementation of PartialEq below.
 #[derive(Debug, Clone)]
-pub struct ObjectPath {
+pub struct OptionalObjectPath {
     /// The usecase, or "product" this object belongs to.
-    ///
-    /// This can be defined on-the-fly by the client, but special server logic
-    /// (such as the concrete backend/bucket) can be tied to this as well.
     pub usecase: String,
-
     /// The scope of the object, used for compartmentalization.
-    ///
-    /// This is treated as a prefix, and includes such things as the organization and project.
     pub scope: Vec<String>,
+    /// The optional, user-provided key.
+    pub key: Option<String>,
+}
 
-    /// This is a user-defined key, which uniquely identifies the object within its usecase/scope.
-    pub key: String,
+impl OptionalObjectPath {
+    /// Converts to an [`ObjectPath`], generating a unique `key` if none was provided.
+    pub fn create_key(self) -> ObjectPath {
+        ObjectPath {
+            usecase: self.usecase,
+            scope: self.scope,
+            key: self.key.unwrap_or_else(|| uuid::Uuid::new_v4().to_string()),
+        }
+    }
+
+    /// Converts to an [`ObjectPath`], returning an error if no `key` was provided.
+    pub fn require_key(self) -> Result<ObjectPath, &'static str> {
+        Ok(ObjectPath {
+            usecase: self.usecase,
+            scope: self.scope,
+            key: self
+                .key
+                .ok_or("object key is required but was not provided")?,
+        })
+    }
 }
 
-impl Display for ObjectPath {
+impl Display for OptionalObjectPath {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         write!(f, "{}/", self.usecase)?;
         for scope in &self.scope {
             write!(f, "{}/", scope)?;
         }
-        write!(f, "{PATH_CONTEXT_SEPARATOR}/{}", self.key)
+        write!(f, "{PATH_CONTEXT_SEPARATOR}/")?;
+        if let Some(ref key) = self.key {
+            f.write_str(key)?;
+        }
+        Ok(())
+    }
+}
+
+impl PartialEq for OptionalObjectPath {
+    fn eq(&self, other: &Self) -> bool {
+        self.usecase == other.usecase
+            && self.scope == other.scope
+            && self.key == other.key
+            && self.key.is_some() // Two paths without keys are considered unequal!
     }
 }
 
-struct ObjectPathVisitor;
-impl<'de> serde::de::Visitor<'de> for ObjectPathVisitor {
-    type Value = ObjectPath;
+struct OptionalObjectPathVisitor;
+impl<'de> serde::de::Visitor<'de> for OptionalObjectPathVisitor {
+    type Value = OptionalObjectPath;
 
     fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
         write!(
@@ -86,25 +113,65 @@ impl<'de> serde::de::Visitor<'de> for ObjectPathVisitor {
         }
 
         // The rest of the path is a user-provided key.
-        // If no key is provided, generate a UUIDv4 by default.
-        let key = match iter.peek() {
-            None => uuid::Uuid::new_v4().to_string(),
-            Some(_) => iter.collect(),
+        let key = if iter.peek().is_some() {
+            Some(iter.collect())
+        } else {
+            None
         };
 
-        Ok(ObjectPath {
+        Ok(OptionalObjectPath {
             usecase,
             scope,
             key,
         })
     }
 }
 
+impl<'de> de::Deserialize<'de> for OptionalObjectPath {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: de::Deserializer<'de>,
+    {
+        deserializer.deserialize_str(OptionalObjectPathVisitor)
+    }
+}
+
+/// The fully scoped path of an object.
+///
+/// This consists of a usecase, the scope, and the object key.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct ObjectPath {
+    /// The usecase, or "product" this object belongs to.
+    ///
+    /// This can be defined on-the-fly by the client, but special server logic
+    /// (such as the concrete backend/bucket) can be tied to this as well.
+    pub usecase: String,
+
+    /// The scope of the object, used for compartmentalization.
+    ///
+    /// This is treated as a prefix, and includes such things as the organization and project.
+    pub scope: Vec<String>,
+
+    /// This key uniquely identifies the object within its usecase/scope.
+    pub key: String,
+}
+
+impl Display for ObjectPath {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{}/", self.usecase)?;
+        for scope in &self.scope {
+            write!(f, "{}/", scope)?;
+        }
+        write!(f, "{PATH_CONTEXT_SEPARATOR}/{}", self.key)
+    }
+}
+
 impl<'de> de::Deserialize<'de> for ObjectPath {
     fn deserialize<D>(deserializer: D) -> Result<ObjectPath, D::Error>
     where
         D: de::Deserializer<'de>,
     {
-        deserializer.deserialize_str(ObjectPathVisitor)
+        let optional_path = OptionalObjectPath::deserialize(deserializer)?;
+        optional_path.require_key().map_err(de::Error::custom)
     }
 }