You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "baszalmstra (via GitHub)" <gi...@apache.org> on 2023/03/23 14:36:23 UTC
[GitHub] [incubator-opendal] baszalmstra opened a new pull request, #1742: feat: adds override_content_disposition field
baszalmstra opened a new pull request, #1742:
URL: https://github.com/apache/incubator-opendal/pull/1742
Adds the `override_content_disposition` field as described in #1735
--
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
[GitHub] [incubator-opendal] baszalmstra commented on a diff in pull request #1742: feat: Add override_content_disposition for OpRead
Posted by "baszalmstra (via GitHub)" <gi...@apache.org>.
baszalmstra commented on code in PR #1742:
URL: https://github.com/apache/incubator-opendal/pull/1742#discussion_r1147474088
##########
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:
Done!
--
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
[GitHub] [incubator-opendal] baszalmstra commented on a diff in pull request #1742: feat: Add override_content_disposition for OpRead
Posted by "baszalmstra (via GitHub)" <gi...@apache.org>.
baszalmstra commented on code in PR #1742:
URL: https://github.com/apache/incubator-opendal/pull/1742#discussion_r1147369443
##########
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 followed the same syntax as `presign_write_with(path, op, expire)`. Doing like you suggested would make them inconsistent. Do you still prefer that?
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1742: feat: Add override_content_disposition for OpRead
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1742:
URL: https://github.com/apache/incubator-opendal/pull/1742#discussion_r1147372919
##########
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:
Oh, you are right, please keep the same style. Thanks!
--
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
[GitHub] [incubator-opendal] baszalmstra commented on a diff in pull request #1742: feat: Add override_content_disposition for OpRead
Posted by "baszalmstra (via GitHub)" <gi...@apache.org>.
baszalmstra commented on code in PR #1742:
URL: https://github.com/apache/incubator-opendal/pull/1742#discussion_r1147373159
##########
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:
Im not sure I understand what you mean. Do you mean not adding the query string args to a vec first? How would you then add the logic to add the `?` and the `&` in between subsequent query arguments? Or did I over-engineer it already by adding support for adding multiple query string arguments? 😅
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1742: feat: adds override_content_disposition field
Posted by "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
[GitHub] [incubator-opendal] Xuanwo merged pull request #1742: feat: Add override_content_disposition for OpRead
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #1742:
URL: https://github.com/apache/incubator-opendal/pull/1742
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1742: feat: Add override_content_disposition for OpRead
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1742:
URL: https://github.com/apache/incubator-opendal/pull/1742#discussion_r1147374155
##########
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:
> Or did I over-engineer it already by adding support for adding multiple query string arguments?
Aha, I mean this. we can simply add a `?xxx=yyy` after the url.
--
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