Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: support flexible checksums #1561

Merged
merged 17 commits into from
Jul 26, 2022

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Jul 20, 2022

Motivation and Context

#1482

Description

This PR adds all the inlineables and codegen necessary to support flexible checksums. It also updates the S3 model to the latest version and tests the new checksum functionality against that model.

Testing

I have written many tests to ensure the code is correct.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Velfi Velfi requested a review from a team as a code owner July 20, 2022 21:00
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting to see all this come together! It looks like there's a server codegen compile error that's preventing CI from running.

aws/rust-runtime/aws-inlineable/src/http_body_checksum.rs Outdated Show resolved Hide resolved
aws/rust-runtime/aws-inlineable/src/http_body_checksum.rs Outdated Show resolved Hide resolved
use http_body::Body;

// Ensure that a valid `checksum_algorithm` was passed, emitting an error if that's not the case
if let Err(err) = aws_smithy_checksums::http::new_from_algorithm(&checksum_algorithm) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we should create the algorithm once to avoid an unnecessary allocation since it returns a boxed value. It's being created up here for validation, and then again below for the actual checksum calculation.

Edit: I see the reason for this now in the code comment below. I think we should create an intermediate Algorithm enum type that is easily converted to/from algorithm names and header names, and can easily create algorithm implementations. I prototyped this a bit and made it a macro so that there's no risk of any of the functions on the enum getting out of sync with each other:

macro_rules! associate_algorithms {
    (pub enum $enum_name:ident { $($algorithm_ident:ident => { name: $algorithm_name:ident, header_name: $header_name:ident },)+ }) => {
        #[derive(Copy, Clone)]
        pub enum $enum_name {
            $($algorithm_ident,)+
        }

        impl $enum_name {
            pub fn to_header_name(&self) -> Result<HeaderName, Box<dyn std::error::Error>> {
                match self {
                    $(Self::$algorithm_ident => Ok($header_name.clone()),)+
                }
            }

            pub fn from_header_name(checksum_header_name: &HeaderName) -> Result<Self, Box<dyn std::error::Error>> {
                $(if checksum_header_name == $header_name { return Ok(Self::$algorithm_ident) })+
                Err(Box::new(Error::UnknownChecksumHeaderName(
                    checksum_header_name.to_owned(),
                )))
            }

            pub fn to_algorithm_name(&self) -> &str {
                match self {
                    $(Self::$algorithm_ident => $algorithm_name,)+
                }
            }

            pub fn from_algorithm_name(algorithm_name: &str) -> Result<Self, Box<dyn std::error::Error>> {
                $(if algorithm_name == $algorithm_name { return Ok(Self::$algorithm_ident) })+
                Err(Box::new(Error::UnknownChecksumAlgorithm(
                    algorithm_name.to_owned(),
                )))
            }

            pub fn to_impl(&self) -> Box<dyn HttpChecksum> {
                match self {
                    $(Self::$algorithm_ident => Box::new($algorithm_ident::default()),)+
                }
            }
        }
    };
}

associate_algorithms!(
    pub enum Algorithm {
        Crc32 => { name: CRC_32_NAME, header_name: CRC_32_HEADER_NAME },
        Crc32c => { name: CRC_32_C_NAME, header_name: CRC_32_C_HEADER_NAME },
        Md5 => { name: MD5_NAME, header_name: MD5_HEADER_NAME },
        Sha1 => { name: SHA_1_NAME, header_name: SHA_1_HEADER_NAME },
        Sha256 => { name: SHA_256_NAME, header_name: SHA_256_HEADER_NAME },
    }
);

Then we can verify an algorithm is supported with:

let algorithm = Algorithm::from_algorithm_name(&checksum_algorithm)
    .map_err(|err| BuildError::Other(err))?;

This Algorithm enum is Copy, so we don't have to worry about it moving anywhere. Then inside the body checksumming, we easily get a fresh implementation when we need it:

let checksum = algorithm.to_impl();

No error checking required since we already error checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the enum idea, but not the macro. Let me know what you think.

@Velfi Velfi mentioned this pull request Jul 21, 2022
2 tasks
add: customizations arg to ServerHttpBoundProtocolTraitImplGenerator.generateTraitImpls
@Velfi Velfi requested a review from a team as a code owner July 21, 2022 15:17
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

update: allow dead code in http_body_checksum.rs so we can use pub(crate)
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! just some small stylistic changes

aws/rust-runtime/aws-inlineable/src/http_body_checksum.rs Outdated Show resolved Hide resolved
rust-runtime/aws-smithy-checksums/src/lib.rs Outdated Show resolved Hide resolved
rust-runtime/aws-smithy-checksums/src/lib.rs Outdated Show resolved Hide resolved
rust-runtime/aws-smithy-checksums/src/lib.rs Outdated Show resolved Hide resolved
rust-runtime/aws-smithy-http/src/operation.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@crisidev crisidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from a server perspective. The diff is also empty, which is a good indicator!

@Velfi Velfi enabled auto-merge (squash) July 26, 2022 14:37
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@Velfi Velfi merged commit 2ad87c0 into main Jul 26, 2022
@Velfi Velfi deleted the feature/checksum-inlineables-and-codegen branch July 26, 2022 15:14
@Velfi Velfi mentioned this pull request Jul 26, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants