RFC: Portability/Relative Path - Then body_from_file Into type String

Error and/or Documentation Error and/or Relative Path Handling Error

I was using a relative path for the fn call directly per doc example for Then::body_from_file

But environment variable not found error popped up instead:

Cannot create absolute path from string 'x':  "environment variable not found"

I tried to look whether the doc said much about environment variables but nothing to evaluate.

However Looking into I can find the call to crate::util::get_test_resource_file_path() when path.is_absolute is false:

match env::var("CARGO_MANIFEST_DIR") {

Problem is this env may not be visible to cargo sub-commands as I found the hard way with tarpaulin:

env RUN_MODE=development cargo +nightly tarpaulin --run-types Tests,Doctests,Benchmarks,Examples,Lib,Bins -v

Works okay with cargo test but tarpaulin sub-command seems to omit that environment variable.
Omitting the env seems to be the case with cargo sub-command.
Cargo documentation only lists one env passed down without explicitly ruling out others.

Doc wise Mock::return_body_from_file says explicitly either relative/absolute:

resource_file_path - The file path to the file with the response body content. The file path can either be absolute or relative to the project root directory.

As for relative path definition, I would expect this to be the OsStr<> from the Current working directory (CWD) which may be set to be elsewhere than the directory holding the cargo manifest.

For backwards compatibility the document probably should say that the environment variable is used for relative path or behavior could be changed some way to rely on CWD that would break things relying on it to be the manifest path

Plus CWD typically is not UTF-8 guaranteed path if Into is relied (not absolutely sure whether CARGO_MANIFEST_DIR is guaranteed to be either for lossless conversion Into either if it's allowed to be non UTF-8 :1234: ?)

I personally use this pattern and document the var_os OssStr returning environment variable use for my app:

  let mut path_locator = match env::var_os("APP_CONFIG_PATH") {
      Some(val) => PathBuf::from(val),
      None => env::current_dir().unwrap()
};

If I feel I need to enforce UTF-8 paths like Rust ecosystem like cargo does I use camino Utf8Path to see if I can upgrade from Path and match appropriate error.

Happy to submit a PR to fix doc and automatically check the truth whatever is decided when/if there is a decision what to do with this.

Code Blocks Involved

Then::body_from_file(httpmock/0.5.8/source/src/lib.rs)

pub fn body_from_file<S: Into<String>>(self, body: S) -> Self {
        self.mock.set(self.mock.take().return_body_from_file(body));

Mock::return_body_from_file(httpmock/0.5.8/source/src/api/mock.rs):around 1317

 pub fn return_body_from_file<S: Into<String>>(mut self, resource_file_path: S) -> Self {
        let resource_file_path = resource_file_path.into();
        let path = Path::new(&resource_file_path);
        let absolute_path = match path.is_absolute() {
            true => path.to_path_buf(),
            false => get_test_resource_file_path(&resource_file_path).expect(&format!(
                "Cannot create absolute path from string '{}'",
     //--snip--

Dilemma

Documentation advertises/promises both the relative and absolute path use
Relative path is implicitly derived from documentation as current working directory (CWD)
If relative path is used CARGO_MANIFEST_DIR is used as base path which may or may not be lossless Into
Cargo may or may not pass this env down to sub-command
std::String is always UTF-8
std::ffi::OsString implements Into String type but is lossy and breaks relative path guarantee
OsString and Path is supposed to work everywhere safely abstracting it
Environmental variable etc. can be non-UTF8 and requires appropriate handling boilerplate before it hits Then::body_from_file
Lossy conversion is due to POSIX filenames allowing anything except \0
Requires conversion to String for the whole path (if I read the code right:ok_woman:)
String/std::path::Path conversion has been always a pain
Many OS allows wide array of bytes in OsStr

Solution 1: Allow From &std::path::Path without using CARGO env

Pros:

Straightforward pattern is to pass std::path::Path directly e.g. with config::from
Allows relative path handling at ease over "somebody else handles it" abstraction
Library already uses std::Path internally despite Into String
Library using this can handle easily cross-platform TEST_DATA path which may contain "\" or "/" path separators etc.
The world is not perfect problem is easier - less friction to use the library esp re: relative path
Documentation is the source of truth, though implicit Type impl loses (String is only UTF-8)
Doesn't rely on cargo env

Cons:

Rust configured paths are UTF-8
Lossy Display/Debug
Either confusing implementation if String is still allowed
Libs using may/will break if on same method - needs new method
Current working directory derived from var_os needs to be used instead of CARGO env

Example is using config::File::from

impl<'a> From<&'a Path> for File<source::file::FileSourceFile> {
    fn from(path: &'a Path) -> Self {
        File {
            source: source::file::FileSourceFile::new(path.to_path_buf()),
            //--snip--

Where I can easily construct and pass the full Path natively and use relative path if I want

let mut path_locator = match env::var_os("CONFIG_PATH") {
  Some(val) => PathBuf::from(val),
  None => env::current_dir().unwrap()
};

path_locator.push("subdir");

// And then finally merge the file I want with full POSIX compatible Path

s.merge(File::from(path_locator.join("default")).required(false))?;

Solution 2: Enforce and be explicit about UTF-8 and handle Error thru camino

Pros:

Proper taint check and error
Lossless Display/Debug
Less friction for anyone who uses the library as it's checked for validity
Does not need new method as it was always UTF-8
Document implicit Type impl wins to some degree (String is only UTF-8 and we are just enforcing that)

Cons:

Relative path use is a pattern sadly good or bad.
Friction for anyone who uses the library
Library using this have to handle cross platform paths for TEST_DATA path which may contain "\" or "/" path separators etc.
Adds camino crate that handles sanity on Utf8Paths
Documentation which advertised flexibility between relative/absolute loses

Solution 3: New fn pass by Buffer/Stream/Channel oneshot etc.

Pros:

Many async libs just wait-read the whole buffer internally and then process/pass the whole of it
Would allow timing and replaying a server behaviour in timeline basis for writing into kernel buffer out
Implementation could say replay sequence from libpcap file the near timing packets were either sent/recvd
Could differentiate between Content-Length, gzip stream, chunked modes
WebSocket etc. streaming
Can serve as building block for HTTP/2 or gRPC/Protobuffers
More fun than simple singular delay :)

Cons:

Library using this have to handle cross platform paths for TEST_DATA path which may contain "\" or "/" path separators etc.
Library was never intended for buffer/stream handling?
Complexity

Solution 4: Status quo

Pros:

Supposed to have less burden for library

Cons:

Friction to use the library
Library using this have to handle cross platform paths for TEST_DATA path which may contain "\" or "/" path separators etc.
Misleading error (library is not using environment variable, the full path is passed to it)
Still using lossy Display/Debug as it is using Path internally

Solution is probably somekind combination?

I usually do both integration / units along my /// docs or even on README.md that gets included in test run and tarpaulin --run-types DocTests takes those into account

#[cfg(doctest)]
doc_comment::doctest!("../README.md");

I will check with tarpaulin too to expose the environment var but it would be nice to get some clarity as it caused a bit confusion initially for me and not sure if there is anyone doing things without cargo env :)

pinkforest(she/her/hers)

alexliesenfeld / httpmock