You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@opendal.apache.org by "BoWuGit (via GitHub)" <gi...@apache.org> on 2023/08/23 07:35:55 UTC

[GitHub] [incubator-opendal] BoWuGit opened a new pull request, #2909: fix: S3 copy error on non-ascii file path (#2908)

BoWuGit opened a new pull request, #2909:
URL: https://github.com/apache/incubator-opendal/pull/2909

   Just refer issue https://github.com/apache/incubator-opendal/issues/2908.


-- 
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 #2909: fix: S3 copy error on non-ascii file path

Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.

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


##########
core/src/services/s3/core.rs:
##########
@@ -433,7 +433,7 @@ impl S3Core {
         }
 
         let mut req = req
-            .header(constants::X_AMZ_COPY_SOURCE, percent_encode_path(&source))
+            .header(constants::X_AMZ_COPY_SOURCE, &source)

Review Comment:
   You can add a test case here: https://github.com/apache/incubator-opendal/blob/main/core/tests/behavior/write.rs
   
   > NOTE: This PR is good enough for merging. It's okay if you don't want to do any additional work. Please let me know.



-- 
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 #2909: fix: S3 copy error on non-ascii file path

Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.

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


##########
core/tests/behavior/copy.rs:
##########
@@ -40,20 +40,24 @@ pub fn behavior_copy_tests(op: &Operator) -> Vec<Trial> {
 
 /// Copy a file and test with stat.
 pub async fn test_copy_file(op: Operator) -> Result<()> {
-    let source_path = uuid::Uuid::new_v4().to_string();
-    let (source_content, _) = gen_bytes();
-
-    op.write(&source_path, source_content.clone()).await?;
+    let ascii_path_pair = (

Review Comment:
   Sorry for not making my review more clear.
   
   ---
   
   What I expect is
   
   - Don't touch `test_copy_file`
   - Add a new test case called `test_copy_file_with_non_ascii`



-- 
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 #2909: fix: S3 copy error on non-ascii file path (#2908)

Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.

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


##########
core/src/services/s3/core.rs:
##########
@@ -433,7 +433,7 @@ impl S3Core {
         }
 
         let mut req = req
-            .header(constants::X_AMZ_COPY_SOURCE, percent_encode_path(&source))
+            .header(constants::X_AMZ_COPY_SOURCE, &source)

Review Comment:
   Thanks a lot! Would you like to add a behavior test case for us to avoid this breaking again?



-- 
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] BoWuGit commented on a diff in pull request #2909: fix: S3 copy error on non-ascii file path

Posted by "BoWuGit (via GitHub)" <gi...@apache.org>.

BoWuGit commented on code in PR #2909:
URL: https://github.com/apache/incubator-opendal/pull/2909#discussion_r1302612477


##########
core/src/services/s3/core.rs:
##########
@@ -433,7 +433,7 @@ impl S3Core {
         }
 
         let mut req = req
-            .header(constants::X_AMZ_COPY_SOURCE, percent_encode_path(&source))
+            .header(constants::X_AMZ_COPY_SOURCE, &source)

Review Comment:
   OK, I would compliment it later this week, I want to add UT for it originally, but didn't know how at first, and thought it's mocked as to not coveraged.



-- 
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 #2909: fix: S3 copy error on non-ascii file path

Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.

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


##########
core/tests/behavior/copy.rs:
##########
@@ -40,12 +40,12 @@ pub fn behavior_copy_tests(op: &Operator) -> Vec<Trial> {
 
 /// Copy a file and test with stat.
 pub async fn test_copy_file(op: Operator) -> Result<()> {
-    let source_path = uuid::Uuid::new_v4().to_string();
+    let source_path = format!("{}🐂🍺中文.docx", uuid::Uuid::new_v4());

Review Comment:
   It's better to add a new test case instead of changing existing test.



-- 
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 #2909: fix: S3 copy error on non-ascii file path

Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.

Xuanwo merged PR #2909:
URL: https://github.com/apache/incubator-opendal/pull/2909


-- 
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