Improve Env::get, add Env::get_string_lossy

CHANGELOG.md

@@ -6,6 +6,14 @@ separate changelogs for each crate were used. If you need to refer to these old
 
 ## [Unreleased]
 
+### Changed
+
+- `Env::get` now returns `Option<&OsString>` instead of `Option<OsString>`. This is more in line with expectations users have when dealing with a collection type. This is a breaking change, compile errors can be fixed by adding a [`Option::cloned`](https://doc.rust-lang.org/std/option/enum.Option.html#method.cloned-1) call after `Env::get` to get the old behaviour. In some cases, cloning might not be necessary, slightly improving the code that uses `Env::get`. ([#565](https://github.com/heroku/libcnb.rs/pull/565))
+
+### Added
+
+- `Env::get_string_lossy` as a convenience method to work with environment variables directly. Getting a value out of an `Env` and treating its contents as unicode is a common case. Using this new method can simplify buildpack code. ([#565](https://github.com/heroku/libcnb.rs/pull/565))
+
 ## [0.11.5] 2023-02-07
 
 ### Changed

libcnb/src/env.rs

@@ -56,10 +56,24 @@ impl Env {
         self
     }
 
-    /// Returns a cloned value corresponding to the given key.
+    /// Returns the value corresponding to the given key.
     #[must_use]
-    pub fn get(&self, key: impl AsRef<OsStr>) -> Option<OsString> {
-        self.inner.get(key.as_ref()).cloned()
+    pub fn get(&self, key: impl AsRef<OsStr>) -> Option<&OsString> {
+        self.inner.get(key.as_ref())
+    }
+
+    /// Returns the value corresponding to the given key, interpreted as Unicode data.
+    ///
+    /// Any non-Unicode sequences are replaced with
+    /// [`U+FFFD REPLACEMENT CHARACTER`][U+FFFD].
+    ///
+    /// [U+FFFD]: std::char::REPLACEMENT_CHARACTER
+    ///
+    /// See [`OsStr::to_string_lossy`] for more details.
+    #[must_use]
+    pub fn get_string_lossy(&self, key: impl AsRef<OsStr>) -> Option<String> {
+        self.get(key)
+            .map(|os_string| os_string.to_string_lossy().to_string())
     }
 
     /// Returns true if the environment contains a value for the specified key.

libcnb/src/layer/handling.rs

@@ -857,13 +857,13 @@ mod tests {
 
         let applied_layer_env = layer_data.env.apply_to_empty(Scope::Build);
         assert_eq!(
-            applied_layer_env.get("PATH"),
+            applied_layer_env.get("PATH").cloned(),
             Some(layer_dir.join("bin").into())
         );
 
         assert_eq!(
             applied_layer_env.get("CUSTOM_ENV"),
-            Some(OsString::from("CUSTOM_ENV_VALUE"))
+            Some(&OsString::from("CUSTOM_ENV_VALUE"))
         );
     }
 

libcnb/src/layer_env.rs

@@ -75,7 +75,7 @@ use std::path::Path;
 /// let layer_env = LayerEnv::read_from_layer_dir(&layer_dir).unwrap();
 /// let env = layer_env.apply_to_empty(Scope::Launch);
 ///
-/// assert_eq!(env.get("PATH").unwrap(), layer_dir.join("bin"));
+/// assert_eq!(env.get("PATH").cloned().unwrap(), layer_dir.join("bin"));
 /// assert_eq!(env.get("CPATH"), None); // None, because CPATH is only added during build
 /// ```
 #[derive(Eq, PartialEq, Debug, Default, Clone)]
@@ -276,8 +276,8 @@ impl LayerEnv {
     /// let layer_env = LayerEnv::read_from_layer_dir(&layer_dir).unwrap();
     /// let env = layer_env.apply_to_empty(Scope::Launch);
     ///
-    /// assert_eq!(env.get("PATH").unwrap(), layer_dir.join("bin"));
-    /// assert_eq!(env.get("ZERO_WING").unwrap(), "ALL_YOUR_BASE_ARE_BELONG_TO_US");
+    /// assert_eq!(env.get("PATH").cloned().unwrap(), layer_dir.join("bin"));
+    /// assert_eq!(env.get("ZERO_WING").cloned().unwrap(), "ALL_YOUR_BASE_ARE_BELONG_TO_US");
     /// ```
     pub fn read_from_layer_dir(layer_dir: impl AsRef<Path>) -> Result<Self, std::io::Error> {
         let mut result_layer_env = Self::new();
@@ -438,7 +438,7 @@ impl LayerEnvDelta {
                     }
                 }
                 ModificationBehavior::Append => {
-                    let mut previous_value = result_env.get(name).unwrap_or_default();
+                    let mut previous_value = result_env.get(name).cloned().unwrap_or_default();
 
                     if previous_value.len() > 0 {
                         previous_value.push(self.delimiter_for(name));
@@ -449,7 +449,7 @@ impl LayerEnvDelta {
                     result_env.insert(name, previous_value);
                 }
                 ModificationBehavior::Prepend => {
-                    let previous_value = result_env.get(name).unwrap_or_default();
+                    let previous_value = result_env.get(name).cloned().unwrap_or_default();
 
                     let mut new_value = OsString::new();
                     new_value.push(value);
@@ -876,8 +876,8 @@ mod tests {
         let layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap();
 
         let env = layer_env.apply_to_empty(Scope::Launch);
-        assert_eq!(env.get("PATH").unwrap(), layer_dir.join("bin"));
-        assert_eq!(env.get("LD_LIBRARY_PATH").unwrap(), layer_dir.join("lib"));
+        assert_eq!(env.get("PATH").unwrap(), &layer_dir.join("bin"));
+        assert_eq!(env.get("LD_LIBRARY_PATH").unwrap(), &layer_dir.join("lib"));
         assert_eq!(env.get("LIBRARY_PATH"), None);
         assert_eq!(env.get("CPATH"), None);
         assert_eq!(env.get("PKG_CONFIG_PATH"), None);
@@ -897,13 +897,13 @@ mod tests {
         let layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap();
 
         let env = layer_env.apply_to_empty(Scope::Build);
-        assert_eq!(env.get("PATH").unwrap(), layer_dir.join("bin"));
-        assert_eq!(env.get("LD_LIBRARY_PATH").unwrap(), layer_dir.join("lib"));
-        assert_eq!(env.get("LIBRARY_PATH").unwrap(), layer_dir.join("lib"));
-        assert_eq!(env.get("CPATH").unwrap(), layer_dir.join("include"));
+        assert_eq!(env.get("PATH").unwrap(), &layer_dir.join("bin"));
+        assert_eq!(env.get("LD_LIBRARY_PATH").unwrap(), &layer_dir.join("lib"));
+        assert_eq!(env.get("LIBRARY_PATH").unwrap(), &layer_dir.join("lib"));
+        assert_eq!(env.get("CPATH").unwrap(), &layer_dir.join("include"));
         assert_eq!(
             env.get("PKG_CONFIG_PATH").unwrap(),
-            layer_dir.join("pkgconfig")
+            &layer_dir.join("pkgconfig")
         );
     }
 

libcnb/src/platform.rs

@@ -77,8 +77,8 @@ mod tests {
         fs::write(env_dir.join("HELLO"), "World!").unwrap();
 
         let env = read_platform_env(tmpdir.path()).unwrap();
-        assert_eq!(env.get("FOO"), Some(OsString::from("BAR")));
-        assert_eq!(env.get("HELLO"), Some(OsString::from("World!")));
+        assert_eq!(env.get("FOO"), Some(&OsString::from("BAR")));
+        assert_eq!(env.get("HELLO"), Some(&OsString::from("World!")));
     }
 
     #[test]