[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1742: feat: adds override_content_disposition field

["Xuanwo (via GitHub)" <gi...@apache.org>]

Xuanwo commented on code in PR #1742:
URL: https://github.com/apache/incubator-opendal/pull/1742#discussion_r1146383998


##########
core/src/types/operator/operator.rs:
##########
@@ -950,6 +950,41 @@ impl Operator {
         Ok(rp.into_presigned_request())
     }
 
+    /// Presign an operation for read option described in OpenDAL [rfc-1735](../../docs/rfcs/1735_operation_extension.md).
+    ///
+    /// You can pass `OpRead` to this method to specify the content disposition.
+    ///
+    /// # Example
+    ///
+    /// ```no_run
+    /// use anyhow::Result;
+    /// use futures::io;
+    /// use opendal::Operator;
+    /// use time::Duration;
+    /// use opendal::ops::OpRead;
+    ///
+    /// #[tokio::main]
+    /// async fn test(op: Operator) -> Result<()> {
+    ///     let args = OpRead::new()
+    ///         .with_override_content_disposition("attachment; filename=\"othertext.txt\"");
+    ///     let signed_req = op.presign_read_with("test.txt", args, Duration::hours(1))?;
+    /// #    Ok(())
+    /// # }
+    /// ```
+    pub fn presign_read_with(

Review Comment:
   I prefer to use `presign_read_with(path, expire, op)`



##########
core/src/services/s3/backend.rs:
##########
@@ -1325,10 +1329,30 @@ impl S3Backend {
         Ok(req)
     }
 
-    fn s3_get_object_request(&self, path: &str, range: BytesRange) -> Result<Request<AsyncBody>> {
+    fn s3_get_object_request(
+        &self,
+        path: &str,
+        range: BytesRange,
+        override_content_disposition: Option<&str>,
+    ) -> Result<Request<AsyncBody>> {
         let p = build_abs_path(&self.root, path);
 
-        let url = format!("{}/{}", self.endpoint, percent_encode_path(&p));
+        // Construct headers to add to the request
+        let mut query_args = Vec::new();
+        if let Some(override_content_disposition) = override_content_disposition {
+            query_args.push(format!(
+                "{}={}",
+                constants::RESPONSE_CONTENT_DISPOSITION,
+                percent_encode_path(override_content_disposition)
+            ))
+        }
+
+        let mut url = format!("{}/{}", self.endpoint, percent_encode_path(&p));

Review Comment:
   I prefer to simplify them by directly writing the string.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org