aws: s3sink: Fix handling of special characters in key

Properly URL-encode the string if needed, and add some tests for a
couple of cases.

Fixes: https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/issues/431
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/merge_requests/1388>

2 files changed, 89 insertions, 70 deletions

diff --git a/net/aws/src/s3sink/imp.rs b/net/aws/src/s3sink/imp.rs
index a86cc4189..02fd22d1f 100644
--- a/net/aws/src/s3sink/imp.rs
+++ b/net/aws/src/s3sink/imp.rs
@@ -116,12 +116,13 @@ struct Settings {
 
 impl Settings {
     fn to_uri(&self) -> String {
-        format!(
-            "s3://{}/{}/{}",
-            self.region,
-            self.bucket.as_ref().unwrap(),
-            self.key.as_ref().unwrap()
-        )
+        GstS3Url {
+            region: self.region.clone(),
+            bucket: self.bucket.clone().unwrap(),
+            object: self.key.clone().unwrap(),
+            version: None,
+        }
+        .to_string()
     }
 
     fn to_metadata(&self, imp: &S3Sink) -> Option<HashMap<String, String>> {
diff --git a/net/aws/tests/s3.rs b/net/aws/tests/s3.rs
index 65e4d7bee..fafd4a24d 100644
--- a/net/aws/tests/s3.rs
+++ b/net/aws/tests/s3.rs
@@ -8,72 +8,90 @@
 // SPDX-License-Identifier: MPL-2.0
 //
 
-use gst::prelude::*;
-
-const DEFAULT_S3_REGION: &str = "us-west-2";
-
-fn init() {
-    use std::sync::Once;
-    static INIT: Once = Once::new();
-
-    INIT.call_once(|| {
-        gst::init().unwrap();
-        gstaws::plugin_register_static().unwrap();
-    });
-}
-
 // The test times out on Windows for some reason, skip until we figure out why
 #[cfg(not(target_os = "windows"))]
 #[test_with::env(AWS_ACCESS_KEY_ID)]
 #[test_with::env(AWS_SECRET_ACCESS_KEY)]
-#[tokio::test]
-async fn test_s3() {
-    init();
-    // Makes it easier to get AWS SDK logs if needed
-    env_logger::init();
-
-    let region = std::env::var("AWS_REGION").unwrap_or_else(|_| DEFAULT_S3_REGION.to_string());
-    let bucket =
-        std::env::var("AWS_S3_BUCKET").unwrap_or_else(|_| "gst-plugins-rs-tests".to_string());
-    let key = format!("s3-test-{:?}.txt", chrono::Utc::now());
-    let uri = format!("s3://{region}/{bucket}/{key}");
-    let content = "Hello, world!\n".as_bytes();
-
-    // Manually add the element so we can configure it before it goes to PLAYING
-    let mut h1 = gst_check::Harness::new_empty();
-    // Need to add_parse() because the Harness API / Rust bindings aren't conducive to creating and
-    // adding an element manually
-    h1.add_parse(format!("awss3sink uri={uri}").as_str());
-
-    h1.set_src_caps(gst::Caps::builder("text/plain").build());
-    h1.play();
-
-    h1.push(gst::Buffer::from_slice(content)).unwrap();
-    h1.push_event(gst::event::Eos::new());
-
-    let mut h2 = gst_check::Harness::new("awss3src");
-    h2.element().unwrap().set_property("uri", uri.clone());
-    h2.play();
-
-    let buf = h2.pull_until_eos().unwrap().unwrap();
-    assert_eq!(
-        content,
-        buf.into_mapped_buffer_readable().unwrap().as_slice()
-    );
-
-    let region_provider = aws_config::meta::region::RegionProviderChain::first_try(
-        aws_sdk_s3::config::Region::new(region.clone()),
-    )
-    .or_default_provider();
-
-    let config = aws_config::from_env().region(region_provider).load().await;
-    let client = aws_sdk_s3::Client::new(&config);
-
-    client
-        .delete_object()
-        .bucket(bucket)
-        .key(key)
-        .send()
-        .await
-        .unwrap();
+#[cfg(test)]
+mod tests {
+    use gst::prelude::*;
+
+    const DEFAULT_S3_REGION: &str = "us-west-2";
+
+    fn init() {
+        use std::sync::Once;
+        static INIT: Once = Once::new();
+
+        INIT.call_once(|| {
+            gst::init().unwrap();
+            gstaws::plugin_register_static().unwrap();
+            // Makes it easier to get AWS SDK logs if needed
+            env_logger::init();
+        });
+    }
+
+    // Common helper
+    async fn do_s3_test(key_prefix: &str) {
+        init();
+
+        let region = std::env::var("AWS_REGION").unwrap_or_else(|_| DEFAULT_S3_REGION.to_string());
+        let bucket =
+            std::env::var("AWS_S3_BUCKET").unwrap_or_else(|_| "gst-plugins-rs-tests".to_string());
+        let key = format!("{key_prefix}-{:?}.txt", chrono::Utc::now());
+        let uri = format!("s3://{region}/{bucket}/{key}");
+        let content = "Hello, world!\n".as_bytes();
+
+        // Manually add the element so we can configure it before it goes to PLAYING
+        let mut h1 = gst_check::Harness::new_empty();
+        // Need to add_parse() because the Harness API / Rust bindings aren't conducive to creating and
+        // adding an element manually
+        h1.add_parse(format!("awss3sink uri=\"{uri}\"").as_str());
+
+        h1.set_src_caps(gst::Caps::builder("text/plain").build());
+        h1.play();
+
+        h1.push(gst::Buffer::from_slice(content)).unwrap();
+        h1.push_event(gst::event::Eos::new());
+
+        let mut h2 = gst_check::Harness::new("awss3src");
+        h2.element().unwrap().set_property("uri", uri.clone());
+        h2.play();
+
+        let buf = h2.pull_until_eos().unwrap().unwrap();
+        assert_eq!(
+            content,
+            buf.into_mapped_buffer_readable().unwrap().as_slice()
+        );
+
+        let region_provider = aws_config::meta::region::RegionProviderChain::first_try(
+            aws_sdk_s3::config::Region::new(region.clone()),
+        )
+        .or_default_provider();
+
+        let config = aws_config::from_env().region(region_provider).load().await;
+        let client = aws_sdk_s3::Client::new(&config);
+
+        client
+            .delete_object()
+            .bucket(bucket)
+            .key(key)
+            .send()
+            .await
+            .unwrap();
+    }
+
+    #[tokio::test]
+    async fn test_s3_simple() {
+        do_s3_test("s3-test").await;
+    }
+
+    #[tokio::test]
+    async fn test_s3_whitespace() {
+        do_s3_test("s3 test").await;
+    }
+
+    #[tokio::test]
+    async fn test_s3_unicode() {
+        do_s3_test("s3 🧪 😱").await;
+    }
 }