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

Only generate the presign customize operation method for ops that support it #3918

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Nov 20, 2024

THIS IS A BREAKING CHANGE, DO NOT MERGE

Exactly what it says on the tin.

What it currently generates

impl<E, B> CustomizableOperation<crate::operation::put_object::PutObject, E, B> {
    /// Sends the request and returns the response.
    #[allow(unused_mut)]
    pub async fn presigned(
        mut self,
        presigning_config: crate::presigning::PresigningConfig,
    ) -> ::std::result::Result<crate::presigning::PresignedRequest, crate::error::SdkError<E>>
    where
        E: std::error::Error + ::std::marker::Send + ::std::marker::Sync + 'static,
        B: crate::client::customize::internal::CustomizablePresigned<E>,
    {
        self.execute(move |sender, conf| sender.presign(conf, presigning_config)).await
    }
}

Problems with this approach

  • Will create an impl for each operation that needs one. If all operations need to define a customizable operation impl, then that's potentially a lot of redundant code.
  • We've been generating the presign method for all S3 ops, even though it's only guaranteed to work for a subset. Before merging this, we'll probably want to undo the changes to presigning while keeping the underlying new way of defining customizable op impls.

What I need from you

  • Does this codegen approach look sound?
  • Should we be doing this for things other than presigning?
  • How should we decide when to do impls per-operation vs for all operations at once?

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 review from a team as code owners November 20, 2024 17:00
@Velfi Velfi marked this pull request as draft November 20, 2024 17:04
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@ysaito1001
Copy link
Contributor

Thanks for the PR. This gives us a good starting point for discussion.

We've been generating the presign method for all S3 ops, even though it's only guaranteed to work for a subset. Before merging this, we'll probably want to undo the changes to presigning while keeping the underlying new way of defining customizable op impls.

Talked about this briefly offline, and it might be better to keep the presign method for all operations so we don’t break things for people. There's also an issue (linked off from this TODO) that says All S3 requests should be presignable—this is the behavior of the Ruby SDK. so that's something to keep in mind.

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