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

Relax Sync requirement for storage_objects_insert_ext_stream #146

Closed
zackangelo opened this issue Jul 3, 2024 · 4 comments · Fixed by #147
Closed

Relax Sync requirement for storage_objects_insert_ext_stream #146

zackangelo opened this issue Jul 3, 2024 · 4 comments · Fixed by #147

Comments

@zackangelo
Copy link

The storage_objects_insert_ext_stream extension method for uploading a stream to GCS requires that the Stream is Sync. But there doesn't appear to be a need for it and prevents being able to easily proxy an incoming Body stream from axum (which is not Sync).

pub type BoxStreamWithSync<'a, T> =
std::pin::Pin<Box<dyn futures::Stream<Item = T> + Send + 'a + Sync>>;
/// Stores a new object and metadata.
/// Open API doesn't support binary streams and this particular endpoint uses another base URL.
/// Tha means generated `storage_objects_insert` can't be used.
pub async fn storage_objects_insert_ext_stream(
configuration: &configuration::Configuration,
params: StoragePeriodObjectsPeriodInsertParams,
content_type: Option<String>,
bytes_stream: BoxStreamWithSync<
'static,
std::result::Result<bytes::Bytes, Box<(dyn std::error::Error + Send + Sync + 'static)>>,
>,
) -> Result<
crate::google_rest_apis::storage_v1::models::Object,
Error<StoragePeriodObjectsPeriodInsertError>,
> {

I can open a PR if we're okay with this change.

@abdolence
Copy link
Owner

Right, thanks for reporting. It is an easy fix, I'll handle it with releasing the next version 👍🏻 with this PR:
#147

@abdolence
Copy link
Owner

@zackangelo Seems in the version of Reqwest, this Sync is now required, so I have to revert this in 0.25.

@abdolence
Copy link
Owner

image

@abdolence
Copy link
Owner

They have removed it again in the last commits:
seanmonstar/reqwest#2361

As soon as they release it I'll relax it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants