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

chore: add Into conversions for Blob to [u8] #3877

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

ajewellamz
Copy link
Contributor

Motivation and Context

Often I have a &[u8] or a Vec, but the sdk function takes a Blob.
It would be convenient to pass the my_vec instead of aws_smithy_types::Blob::new(my_vec)

Description

In blob.rs I added
Into<Vec> for Blob
Into for Vec
Into for &[u8]

Testing

Added a test to blob.rs to test the various scenarios.


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

@ysaito1001
Copy link
Contributor

Thanks for contributing. I have two questions.
Q1.

but the sdk function takes a Blob.

Is it possible that you could point us to an example function, just to be on the same page?

Q2. What's the reason for also adding

impl From<Blob> for Vec<u8> {
    fn from(value: Blob) -> Self {
        value.into_inner()
    }
}

when Blob already has .into_inner? I see the other two impl From make it more ergonomic when passing Vec<u8> and &[u8] to a function taking impl Into<Blob>. Do you also have a use case where there is a function that takes impl Into<Vec<u8>> and you need to pass a Blob?

@ajewellamz
Copy link
Contributor Author

ajewellamz commented Oct 15, 2024

Thanks for contributing. I have two questions. Q1.

but the sdk function takes a Blob.

Is it possible that you could point us to an example function, just to be on the same page?

How about aws_sdk_dynamodb::types::AttributeValue::B?

Better yet, look at the docs for aws-sdk-kms and search for Blob as a parameter.
You'll find many many things that take blobs as input and return blobs as output.

Q2. What's the reason for also adding impl From<Blob> for Vec<u8>

I dunno. Completeness I guess, or symmetry . I'll delete it, if it's a problem.
But why provide only a non-standard way, when a standard way exists?
Many constructors, including Blob, look like this :
pub fn new<T: Into<Vec<u8>>>(input: T) -> Self
and so into_inner does no good. I has to be Into

@ysaito1001
Copy link
Contributor

How about aws_sdk_dynamodb::types::AttributeValue::B?

Better yet, look at the docs for aws-sdk-kms and search for Blob as a parameter. You'll find many many things that take blobs as input and return blobs as output.

Hmm in aws-sdk-kms, I don't see methods that take specifically impl Into<Blob> for some reason.

➜  kms git:(main) pwd
/Users/awsaito/src/aws-sdk-rust/sdk/kms
➜  kms git:(main) rg Blob | wc -l  
     329
➜  kms git:(main) rg Blob | tail -n 10
src/operation/generate_mac/_generate_mac_input.rs:    pub fn get_message(&self) -> &::std::option::Option<::aws_smithy_types::Blob> {
src/operation/generate_mac/builders.rs:    pub fn message(mut self, input: ::aws_smithy_types::Blob) -> Self {
src/operation/generate_mac/builders.rs:    pub fn set_message(mut self, input: ::std::option::Option<::aws_smithy_types::Blob>) -> Self {
src/operation/generate_mac/builders.rs:    pub fn get_message(&self) -> &::std::option::Option<::aws_smithy_types::Blob> {
src/types/_recipient_info.rs:    pub attestation_document: ::std::option::Option<::aws_smithy_types::Blob>,
src/types/_recipient_info.rs:    pub fn attestation_document(&self) -> ::std::option::Option<&::aws_smithy_types::Blob> {
src/types/_recipient_info.rs:    pub(crate) attestation_document: ::std::option::Option<::aws_smithy_types::Blob>,
src/types/_recipient_info.rs:    pub fn attestation_document(mut self, input: ::aws_smithy_types::Blob) -> Self {
src/types/_recipient_info.rs:    pub fn set_attestation_document(mut self, input: ::std::option::Option<::aws_smithy_types::Blob>) -> Self {
src/types/_recipient_info.rs:    pub fn get_attestation_document(&self) -> &::std::option::Option<::aws_smithy_types::Blob> {
➜  kms git:(main) rg Blob | rg Into | wc -l
       0
➜  kms git:(main) rg Blob | rg impl | wc -l
       0

(I don't either in /Users/awsaito/src/aws-sdk-rust/sdk/kms/dynamodb)

Can you send us a URL to the doc for a function/method that takes impl Into<Blob>, otherwise even after providing impl Into<Vec> for Blob or impl From<&[u8]> for Blob, callers still have to call .into() at the call sites before calling the method in question (which seems as ergonomic as calling Blob::new, IMO).

@ajewellamz
Copy link
Contributor Author

Can you send us a URL to the doc for a function/method that takes impl Into<Blob>, otherwise even after providing impl Into<Vec> for Blob or impl From<&[u8]> for Blob, callers still have to call .into() at the call sites before calling the method in question (which seems as ergonomic as calling Blob::new, IMO).

I was only referring to functions that that a Blob, not Into

There's a chicken and egg problem. Why support an Into parameter type, if there aren't any types that support Into yet.

Personally, I think .into() is much preferable to Blob::new, because it's more idiomatic and more discoverable. The compiler will tell you "use .into()" whereas without it, one must dig through the documentation to find Blob::new.

@ysaito1001
Copy link
Contributor

ysaito1001 commented Oct 16, 2024

I was only referring to functions that that a Blob, not Into

There's a chicken and egg problem. Why support an Into parameter type, if there aren't any types that support Into yet.

Personally, I think .into() is much preferable to Blob::new, because it's more idiomatic and more discoverable. The compiler will tell you "use .into()" whereas without it, one must dig through the documentation to find Blob::new.

Ah, I see where you're coming from. These methods in kms are code generated, so even after this PR has been merged, call sites still have to call .into() to pass a Blob to the methods. We'll create a backlog task to update the methods signatures to take impl Into<Blob> (no estimated delivery time at this time).

Thanks again for contributing. Could you bump the version of aws-smithy-types (as shown in CI error) to maybe 1.2.8 and push the change? We'll merge it in.

Canary passed

@ysaito1001 ysaito1001 added this pull request to the merge queue Oct 16, 2024
Merged via the queue into smithy-lang:main with commit ef07c88 Oct 16, 2024
41 of 42 checks passed
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.

2 participants