Make Drive deal with bytes

A storage drive shouldn't be restricted to containing text files, and in
fact, we need to support binary files if we want to store images.
Switch the Drive interface to expose byte streams instead to support
them.

Author: jmmv

client/src/cmds.rs

@@ -901,7 +901,7 @@ mod tests {
     #[tokio::test]
     async fn test_share_print_no_acls() {
         let mut t = ClientTester::default();
-        t.get_storage().borrow_mut().put("MEMORY:/FOO", "").await.unwrap();
+        t.get_storage().borrow_mut().put("MEMORY:/FOO", b"").await.unwrap();
         t.run(r#"SHARE "MEMORY:/FOO""#)
             .expect_prints(["", "    No ACLs on MEMORY:/FOO", ""])
             .expect_file("MEMORY:/FOO", "")
@@ -914,7 +914,7 @@ mod tests {
         {
             let storage = t.get_storage();
             let mut storage = storage.borrow_mut();
-            storage.put("MEMORY:/FOO", "").await.unwrap();
+            storage.put("MEMORY:/FOO", b"").await.unwrap();
             storage
                 .update_acls(
                     "MEMORY:/FOO",
@@ -933,7 +933,7 @@ mod tests {
     #[tokio::test]
     async fn test_share_make_public() {
         let mut t = ClientTester::default();
-        t.get_storage().borrow_mut().put("MEMORY:/FOO.BAS", "").await.unwrap();
+        t.get_storage().borrow_mut().put("MEMORY:/FOO.BAS", b"").await.unwrap();
         t.get_service().borrow_mut().do_login().await;
         let mut checker = t.run(r#"SHARE "MEMORY:/FOO.BAS", "Public+r""#);
         let output = flatten_output(checker.take_captured_out());

client/src/drive.rs

@@ -61,18 +61,12 @@ impl Drive for CloudDrive {
         ))
     }
 
-    async fn get(&self, filename: &str) -> io::Result<String> {
+    async fn get(&self, filename: &str) -> io::Result<Vec<u8>> {
         let request = GetFileRequest::default().with_get_content();
         let response =
             self.service.borrow_mut().get_file(&self.username, filename, &request).await?;
         match response.decoded_content()? {
-            Some(content) => match String::from_utf8(content) {
-                Ok(s) => Ok(s),
-                Err(e) => Err(io::Error::new(
-                    io::ErrorKind::InvalidData,
-                    format!("Requested file is not valid UTF-8: {}", e),
-                )),
-            },
+            Some(content) => Ok(content),
             None => Err(io::Error::new(
                 io::ErrorKind::InvalidData,
                 "Server response is missing the file content".to_string(),
@@ -93,8 +87,8 @@ impl Drive for CloudDrive {
         }
     }
 
-    async fn put(&mut self, filename: &str, content: &str) -> io::Result<()> {
-        let request = PatchFileRequest::default().with_content(content.as_bytes());
+    async fn put(&mut self, filename: &str, content: &[u8]) -> io::Result<()> {
+        let request = PatchFileRequest::default().with_content(content);
         self.service.borrow_mut().patch_file(&self.username, filename, &request).await
     }
 
@@ -214,7 +208,7 @@ mod tests {
         };
         service.borrow_mut().add_mock_get_file("the-user", "the-filename", request, Ok(response));
         let result = drive.get("the-filename").await.unwrap();
-        assert_eq!("some content", result);
+        assert_eq!("some content".as_bytes(), result);
 
         service.take().verify_all_used();
     }
@@ -235,21 +229,24 @@ mod tests {
         service.take().verify_all_used();
     }
 
+    #[allow(invalid_from_utf8)]
     #[tokio::test]
     async fn test_clouddrive_get_invalid_utf8() {
+        const BAD_UTF8: &[u8] = &[0x00, 0xc3, 0x28];
+        assert!(str::from_utf8(BAD_UTF8).is_err());
+
         let service = Rc::from(RefCell::from(MockService::default()));
         service.borrow_mut().do_login().await;
         let drive = CloudDrive::new(service.clone(), "the-user");
 
         let request = GetFileRequest::default().with_get_content();
         let response = GetFileResponse {
-            content: Some(BASE64_STANDARD.encode([0x00, 0xc3, 0x28])),
+            content: Some(BASE64_STANDARD.encode(BAD_UTF8)),
             ..Default::default()
         };
         service.borrow_mut().add_mock_get_file("the-user", "the-filename", request, Ok(response));
-        let err = drive.get("the-filename").await.unwrap_err();
-        assert_eq!(io::ErrorKind::InvalidData, err.kind());
-        assert!(format!("{}", err).contains("not valid UTF-8"));
+        let result = drive.get("the-filename").await.unwrap();
+        assert_eq!(BAD_UTF8, result);
 
         service.take().verify_all_used();
     }
@@ -296,7 +293,7 @@ mod tests {
 
         let request = PatchFileRequest::default().with_content("some content");
         service.borrow_mut().add_mock_patch_file("the-user", "the-filename", request, Ok(()));
-        drive.put("the-filename", "some content").await.unwrap();
+        drive.put("the-filename", b"some content").await.unwrap();
 
         service.take().verify_all_used();
     }
@@ -309,11 +306,11 @@ mod tests {
 
         let request = PatchFileRequest::default().with_content("some content");
         service.borrow_mut().add_mock_patch_file("the-user", "the-filename", request, Ok(()));
-        drive.put("the-filename", "some content").await.unwrap();
+        drive.put("the-filename", b"some content").await.unwrap();
 
         let request = PatchFileRequest::default().with_content("some other content");
         service.borrow_mut().add_mock_patch_file("the-user", "the-filename", request, Ok(()));
-        drive.put("the-filename", "some other content").await.unwrap();
+        drive.put("the-filename", b"some other content").await.unwrap();
 
         service.take().verify_all_used();
     }

repl/src/demos.rs

@@ -125,18 +125,18 @@ impl Drive for DemosDrive {
         Ok(DriveFiles::new(entries, disk_quota, disk_free))
     }
 
-    async fn get(&self, name: &str) -> io::Result<String> {
+    async fn get(&self, name: &str) -> io::Result<Vec<u8>> {
         let uc_name = name.to_ascii_uppercase();
         match self.demos.get(&uc_name.as_ref()) {
             Some(value) => {
                 let (_metadata, content) = value;
-                Ok(content.to_string())
+                Ok(content.as_bytes().to_owned())
             }
             None => Err(io::Error::new(io::ErrorKind::NotFound, "Demo not found")),
         }
     }
 
-    async fn put(&mut self, _name: &str, _content: &str) -> io::Result<()> {
+    async fn put(&mut self, _name: &str, _content: &[u8]) -> io::Result<()> {
         Err(io::Error::new(io::ErrorKind::PermissionDenied, "The demos drive is read-only"))
     }
 }
@@ -206,12 +206,12 @@ mod tests {
         assert_eq!(io::ErrorKind::NotFound, block_on(drive.get("unknown.bas")).unwrap_err().kind());
 
         assert_eq!(
-            process_demo(include_bytes!("../examples/hello.bas")),
-            block_on(drive.get("hello.bas")).unwrap()
+            process_demo(include_bytes!("../examples/hello.bas")).as_bytes(),
+            block_on(drive.get("hello.bas")).unwrap().as_slice()
         );
         assert_eq!(
-            process_demo(include_bytes!("../examples/hello.bas")),
-            block_on(drive.get("Hello.Bas")).unwrap()
+            process_demo(include_bytes!("../examples/hello.bas")).as_bytes(),
+            block_on(drive.get("Hello.Bas")).unwrap().as_slice()
         );
     }
 
@@ -221,16 +221,16 @@ mod tests {
 
         assert_eq!(
             io::ErrorKind::PermissionDenied,
-            block_on(drive.put("hello.bas", "")).unwrap_err().kind()
+            block_on(drive.put("hello.bas", b"")).unwrap_err().kind()
         );
         assert_eq!(
             io::ErrorKind::PermissionDenied,
-            block_on(drive.put("Hello.BAS", "")).unwrap_err().kind()
+            block_on(drive.put("Hello.BAS", b"")).unwrap_err().kind()
         );
 
         assert_eq!(
             io::ErrorKind::PermissionDenied,
-            block_on(drive.put("unknown.bas", "")).unwrap_err().kind()
+            block_on(drive.put("unknown.bas", b"")).unwrap_err().kind()
         );
     }
 

repl/src/lib.rs

@@ -70,7 +70,7 @@ pub async fn try_load_autoexec(
         }
     };
 
-    match machine.exec(&mut code.as_bytes()).await {
+    match machine.exec(&mut code.as_slice()).await {
         Ok(_) => Ok(()),
         Err(e) => {
             console.borrow_mut().print(&format!("AUTOEXEC.BAS failed: {}", e))?;
@@ -107,6 +107,14 @@ pub async fn run_from_cloud(
 
     console.borrow_mut().print(&format!("Loading {}...", path))?;
     let content = storage.borrow().get(&path).await?;
+    let content = match String::from_utf8(content) {
+        Ok(text) => text,
+        Err(e) => {
+            let mut console = console.borrow_mut();
+            console.print(&format!("Invalid program to run '{}': {}", path, e))?;
+            return Ok(1);
+        }
+    };
     program.borrow_mut().load(Some(&path), &content);
 
     console.borrow_mut().print("Starting...")?;
@@ -360,7 +368,7 @@ mod tests {
     impl DriveFactory for MockDriveFactory {
         fn create(&self, target: &str) -> io::Result<Box<dyn Drive>> {
             let mut drive = InMemoryDrive::default();
-            block_on(drive.put(self.exp_file, Self::SCRIPT)).unwrap();
+            block_on(drive.put(self.exp_file, Self::SCRIPT.as_bytes())).unwrap();
             assert_eq!(self.exp_username, target);
             Ok(Box::from(drive))
         }

std/src/program.rs

@@ -394,6 +394,15 @@ impl Callable for LoadCommand {
                     .make_canonical_with_extension(&pathname, DEFAULT_EXTENSION)
                     .map_err(|e| scope.io_error(e))?;
                 let content = storage.get(&full_name).await.map_err(|e| scope.io_error(e))?;
+                let content = match String::from_utf8(content) {
+                    Ok(text) => text,
+                    Err(e) => {
+                        return Err(scope.io_error(io::Error::new(
+                            io::ErrorKind::InvalidData,
+                            format!("Invalid file content: {}", e),
+                        )));
+                    }
+                };
                 (full_name, content)
             };
             self.program.borrow_mut().load(Some(&full_name), &content);
@@ -593,7 +602,11 @@ impl Callable for SaveCommand {
             .make_canonical_with_extension(&name, DEFAULT_EXTENSION)
             .map_err(|e| scope.io_error(e))?;
         let content = self.program.borrow().text();
-        self.storage.borrow_mut().put(&full_name, &content).await.map_err(|e| scope.io_error(e))?;
+        self.storage
+            .borrow_mut()
+            .put(&full_name, content.as_bytes())
+            .await
+            .map_err(|e| scope.io_error(e))?;
         self.program.borrow_mut().set_name(&full_name);
 
         self.console

std/src/storage/cmds.rs

@@ -503,7 +503,7 @@ mod tests {
     #[test]
     fn test_dir_other_by_argument() {
         let mut other = InMemoryDrive::default();
-        block_on(other.put("foo.bas", "hello")).unwrap();
+        block_on(other.put("foo.bas", b"hello")).unwrap();
 
         let mut t = Tester::default().write_file("empty.bas", "");
         t.get_storage().borrow_mut().attach("other", "z://", Box::from(other)).unwrap();
@@ -544,7 +544,7 @@ mod tests {
     #[test]
     fn test_dir_other_by_cwd() {
         let mut other = InMemoryDrive::default();
-        block_on(other.put("foo.bas", "hello")).unwrap();
+        block_on(other.put("foo.bas", b"hello")).unwrap();
 
         let mut t = Tester::default().write_file("empty.bas", "");
         t.get_storage().borrow_mut().attach("other", "z://", Box::from(other)).unwrap();

std/src/storage/fs.rs

@@ -98,18 +98,18 @@ impl Drive for DirectoryDrive {
         Ok(DriveFiles::new(entries, None, None))
     }
 
-    async fn get(&self, name: &str) -> io::Result<String> {
+    async fn get(&self, name: &str) -> io::Result<Vec<u8>> {
         let path = self.dir.join(name);
         let input = File::open(path)?;
-        let mut content = String::new();
-        io::BufReader::new(input).read_to_string(&mut content)?;
+        let mut content = vec![];
+        io::BufReader::new(input).read_to_end(&mut content)?;
         Ok(content)
     }
 
-    async fn put(&mut self, name: &str, content: &str) -> io::Result<()> {
+    async fn put(&mut self, name: &str, content: &[u8]) -> io::Result<()> {
         let path = self.dir.join(name);
         let mut output = OpenOptions::new().create(true).write(true).truncate(true).open(path)?;
-        output.write_all(content.as_bytes())?;
+        output.write_all(content)?;
         output.sync_all()
     }
 
@@ -266,15 +266,18 @@ mod tests {
         write_file(&dir.path().join("some file.bas"), &["one line", "two lines"]);
 
         let drive = DirectoryDrive::new(dir.path()).unwrap();
-        assert_eq!("one line\ntwo lines\n", block_on(drive.get("some file.bas")).unwrap());
+        assert_eq!(
+            b"one line\ntwo lines\n",
+            block_on(drive.get("some file.bas")).unwrap().as_slice()
+        );
     }
 
     #[test]
     fn test_directorydrive_put() {
         let dir = tempfile::tempdir().unwrap();
 
         let mut drive = DirectoryDrive::new(dir.path()).unwrap();
-        block_on(drive.put("some file.bas", "a b c\nd e\n")).unwrap();
+        block_on(drive.put("some file.bas", b"a b c\nd e\n")).unwrap();
         check_file(&dir.path().join("some file.bas"), &["a b c", "d e"]);
     }
 

std/src/storage/mem.rs

@@ -24,7 +24,7 @@ use std::str;
 /// A drive that records all data in memory only.
 #[derive(Default)]
 pub struct InMemoryDrive {
-    programs: HashMap<String, (String, HashSet<String>)>,
+    programs: HashMap<String, (Vec<u8>, HashSet<String>)>,
 
     // TODO(jmmv): These fields are currently exposed only to allow testing for the consumers of
     // these details and are not enforced in the drive.  It might be nice to actually implement
@@ -52,7 +52,7 @@ impl Drive for InMemoryDrive {
         Ok(DriveFiles::new(entries, self.fake_disk_quota, self.fake_disk_free))
     }
 
-    async fn get(&self, name: &str) -> io::Result<String> {
+    async fn get(&self, name: &str) -> io::Result<Vec<u8>> {
         match self.programs.get(name) {
             Some((content, _readers)) => Ok(content.to_owned()),
             None => Err(io::Error::new(io::ErrorKind::NotFound, "Entry not found")),
@@ -72,7 +72,7 @@ impl Drive for InMemoryDrive {
         }
     }
 
-    async fn put(&mut self, name: &str, content: &str) -> io::Result<()> {
+    async fn put(&mut self, name: &str, content: &[u8]) -> io::Result<()> {
         if let Some((prev_content, _readers)) = self.programs.get_mut(name) {
             content.clone_into(prev_content);
             return Ok(());
@@ -130,30 +130,30 @@ mod tests {
     #[tokio::test]
     async fn test_inmemorydrive_put_respects_acls() {
         let mut drive = InMemoryDrive::default();
-        drive.put("untouched", "some content").await.unwrap();
+        drive.put("untouched", b"some content").await.unwrap();
 
-        drive.put("file", "before").await.unwrap();
+        drive.put("file", b"before").await.unwrap();
         drive.update_acls("file", &readers(&["r1"]), &FileAcls::default()).await.unwrap();
 
-        assert_eq!("before", drive.get("file").await.unwrap());
+        assert_eq!(b"before", drive.get("file").await.unwrap().as_slice());
         assert_eq!(readers(&["r1"]), drive.get_acls("file").await.unwrap());
 
-        drive.put("file", "after").await.unwrap();
+        drive.put("file", b"after").await.unwrap();
 
-        assert_eq!("after", drive.get("file").await.unwrap());
+        assert_eq!(b"after", drive.get("file").await.unwrap().as_slice());
         assert_eq!(readers(&["r1"]), drive.get_acls("file").await.unwrap());
     }
 
     #[tokio::test]
     async fn test_inmemorydrive_get_update_acls() {
         let mut drive = InMemoryDrive::default();
-        drive.put("untouched", "some content").await.unwrap();
+        drive.put("untouched", b"some content").await.unwrap();
 
         let err =
             drive.update_acls("file", &readers(&["r0"]), &FileAcls::default()).await.unwrap_err();
         assert_eq!(io::ErrorKind::NotFound, err.kind());
 
-        drive.put("file", "some content").await.unwrap();
+        drive.put("file", b"some content").await.unwrap();
         assert_eq!(FileAcls::default(), drive.get_acls("file").await.unwrap());
 
         // Add some new readers and try to remove a non-existing one.