Unverified Commit 0248faa2 authored by Copilot's avatar Copilot Committed by GitHub
Browse files

Fix If-None-Match: * wildcard support after typed ETag change (#433) 



* Initial plan

* Add ETagCondition type to support wildcard '*' in If-Match/If-None-Match headers

Co-authored-by: default avatarNugine <30099658+Nugine@users.noreply.github.com>

* Fix clippy warnings and implement If-None-Match wildcard handling in s3s-fs

Co-authored-by: default avatarNugine <30099658+Nugine@users.noreply.github.com>

* Address code review feedback: add Hash trait and improve error handling

Co-authored-by: default avatarNugine <30099658+Nugine@users.noreply.github.com>

* fmt

---------

Co-authored-by: default avatarcopilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: default avatarNugine <30099658+Nugine@users.noreply.github.com>
Co-authored-by: default avatarNugine <nugine@foxmail.com>
parent f2948f8e

codegen/src/v1/dto.rs

+5 −4
Original line number Diff line number Diff line
@@ -46,8 +46,9 @@ pub fn collect_rust_types(model: &smithy::Model, ops: &Operations) -> RustTypes 
            "ETag",          //

        ];



        // ETag-related header types that should be aliased to ETag instead of String

        let etag_alias_types = [

        // ETag-related header types that should be aliased to ETagCondition instead of String

        // These headers support both ETags and the wildcard "*" value

        let etag_condition_alias_types = [

            "IfMatch",               //

            "IfNoneMatch",           //

            "CopySourceIfMatch",     //
@@ -60,9 +61,9 @@ pub fn collect_rust_types(model: &smithy::Model, ops: &Operations) -> RustTypes 
            continue;

        }



        if etag_alias_types.contains(&rs_shape_name.as_str()) {

        if etag_condition_alias_types.contains(&rs_shape_name.as_str()) {

            if let smithy::Shape::String(shape) = shape {

                let ty = rust::Type::alias(&rs_shape_name, "ETag", shape.traits.doc());

                let ty = rust::Type::alias(&rs_shape_name, "ETagCondition", shape.traits.doc());

                insert(rs_shape_name, ty);

                continue;

            }

crates/s3s-aws/src/conv/builtin.rs

+17 −0
Original line number Diff line number Diff line
@@ -212,3 +212,20 @@ impl AwsConversion for s3s::dto::ETag { 
        Ok(format!("\"{}\"", x.value()))

    }

}



impl AwsConversion for s3s::dto::ETagCondition {

    type Target = String;



    type Error = S3Error;



    fn try_from_aws(x: Self::Target) -> S3Result<Self> {

        Self::parse_http_header(x.as_bytes()).map_err(S3Error::internal_error)

    }



    fn try_into_aws(x: Self) -> S3Result<Self::Target> {

        match x {

            s3s::dto::ETagCondition::ETag(etag) => Ok(format!("\"{}\"", etag.value())),

            s3s::dto::ETagCondition::Any => Ok("*".to_string()),

        }

    }

}

crates/s3s-fs/src/s3.rs

+12 −0
Original line number Diff line number Diff line
@@ -474,11 +474,23 @@ impl S3 for FileSystem { 
            metadata,

            content_length,

            content_md5,

            if_none_match,

            ..

        } = input;



        let Some(body) = body else { return Err(s3_error!(IncompleteBody)) };



        // Check If-None-Match condition

        // If-None-Match: * means "only create if the object doesn't exist"

        if let Some(ref condition) = if_none_match {

            if condition.is_any() {

                let object_path = self.get_object_path(&bucket, &key)?;

                if object_path.exists() {

                    return Err(s3_error!(PreconditionFailed, "Object already exists"));

                }

            }

        }



        let mut checksum: s3s::checksum::ChecksumHasher = default();

        if input.checksum_crc32.is_some() {

            checksum.crc32 = Some(default());

crates/s3s-fs/tests/it_aws.rs

+83 −0
Original line number Diff line number Diff line
@@ -826,3 +826,86 @@ async fn test_sts_assume_role_not_implemented() -> Result<()> { 


    Ok(())

}



#[tokio::test]

#[tracing::instrument]

async fn test_if_none_match_wildcard() -> Result<()> {

    let _guard = serial().await;



    let c = Client::new(config());

    let bucket = format!("if-none-match-{}", Uuid::new_v4());

    let bucket = bucket.as_str();

    let key = "test-file.txt";

    let content1 = "initial content";

    let content2 = "updated content";



    create_bucket(&c, bucket).await?;



    // Test 1: PUT with If-None-Match: * should succeed when object doesn't exist

    debug!("Test 1: PUT with If-None-Match: * on non-existent object");

    {

        let body = ByteStream::from_static(content1.as_bytes());

        let result = c

            .put_object()

            .bucket(bucket)

            .key(key)

            .body(body)

            .if_none_match("*")

            .send()

            .await;



        match result {

            Ok(_) => debug!("✓ Successfully created object with If-None-Match: *"),

            Err(e) => panic!("Expected PUT with If-None-Match: * to succeed when object doesn't exist, but got error: {e:?}"),

        }

    }



    // Verify the object was created

    {

        let result = c.get_object().bucket(bucket).key(key).send().await?;

        let body = result.body.collect().await?.into_bytes();

        assert_eq!(body.as_ref(), content1.as_bytes());

        debug!("✓ Verified object was created");

    }



    // Test 2: PUT with If-None-Match: * should fail when object exists

    debug!("Test 2: PUT with If-None-Match: * on existing object");

    {

        let body = ByteStream::from_static(content2.as_bytes());

        let result = c

            .put_object()

            .bucket(bucket)

            .key(key)

            .body(body)

            .if_none_match("*")

            .send()

            .await;



        match result {

            Ok(_) => panic!("Expected PUT with If-None-Match: * to fail when object exists, but it succeeded"),

            Err(e) => {

                let error_str = format!("{e:?}");

                debug!("✓ Expected error when object exists: {error_str}");

                // The error should be a PreconditionFailed (412)

                assert!(

                    error_str.contains("PreconditionFailed") || error_str.contains("412"),

                    "Expected PreconditionFailed error, got: {error_str}"

                );

            }

        }

    }



    // Verify the object wasn't overwritten

    {

        let result = c.get_object().bucket(bucket).key(key).send().await?;

        let body = result.body.collect().await?.into_bytes();

        assert_eq!(body.as_ref(), content1.as_bytes());

        debug!("✓ Verified object was not overwritten");

    }



    // Cleanup

    delete_object(&c, bucket, key).await?;

    delete_bucket(&c, bucket).await?;



    Ok(())

}

crates/s3s/src/dto/etag.rs

+1 −1
Original line number Diff line number Diff line
@@ -11,7 +11,7 @@ use stdx::str::StrExt; 
/// See RFC 9110 §8.8.3 and MDN:

/// + <https://www.rfc-editor.org/rfc/rfc9110#section-8.8.3>

/// + <https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/ETag>

#[derive(Debug, Clone, PartialEq, Eq)]

#[derive(Debug, Clone, PartialEq, Eq, Hash)]

pub enum ETag {

    /// Strong validator: "value"

    Strong(String),