to_url broken with url > 2.1.1

[stevendanna]

We've recently observed the to_string method returning None with the following warning:

[2020-03-04T10:46:59Z WARN  env_proxy] could not set URL scheme back to https 

I think I've tracked this down to an incompatibility between env_proxy and the 2.1.1 version of the URL crate.

The following code constructs a new Url with the scheme xhttp or xhttps and then asssume we can call set_scheme to set it back to the original scheme:

env_proxy/src/lib.rs

Lines 175 to 197 in 8bcfd3a

	if let Some(Ok(mut url)) = self.0.map(|mut s| {
	if !s.contains("://") {
	s.insert_str(0, "http://");
	orig_scheme = Some("http");
	}
	if orig_scheme.is_some() {
	s = s.replacen("http", "xttp", 1);
	}
	Url::parse(&s).map_err(|e| {
	warn!("url parse error: {}", e);
	e
	})
	}) {
	if url.host_str().is_none() {
	warn!("host part of the URL is empty");
	return None;
	}
	if let Some(orig_scheme) = orig_scheme {
	if url.set_scheme(orig_scheme).is_err() {
	warn!("could not set URL scheme back to {}", orig_scheme);
	return None;
	}
	}

As of this commit in the url crate, you cannot set the scheme from a non-canonical scheme to a canonical scheme:

servo/rust-url@7efdc53#diff-b4aea3e418ccdb71239b96952d9cddb6R2029-R2077

As a result, the env_proxy to_url function fails in all of the use cases I've tried. The crate's tests also fail when using url 2.1.1+:

> rg url Cargo.lock 
14: "url 2.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
72:name = "url"
91:"checksum url 2.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "829d4a8476c35c9bf0bbce5a3b23f4106f79728039b726d292bb93bc106787cb"
> cargo +nightly test
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running target/debug/deps/env_proxy-e799bf1b7b6e89e3

running 13 tests
test tests::default_proxy_url_scheme ... FAILED
test tests::http_proxy_fallback ... FAILED
test tests::ftp_proxy_fallback ... FAILED
test tests::ftp_proxy_specific ... FAILED
test tests::no_proxy_subdomain ... ok
test tests::no_proxy_subdomain_dot ... ok
test tests::https_proxy_fallback ... FAILED
test tests::no_proxy_global ... ok
test tests::no_proxy_multiple_list ... ok
test tests::no_proxy_simple_name ... ok
test tests::http_proxy_specific ... FAILED
test tests::https_proxy_specific ... FAILED
test tests::proxy_url_with_explicit_scheme_port ... FAILED

failures:

---- tests::default_proxy_url_scheme stdout ----
thread 'tests::default_proxy_url_scheme' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(("proxy.example.com", 8080))`', src/lib.rs:405:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

---- tests::http_proxy_fallback stdout ----
thread 'tests::http_proxy_fallback' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(("proxy.example.com", 8080))`', src/lib.rs:437:9

---- tests::ftp_proxy_fallback stdout ----
thread 'tests::ftp_proxy_fallback' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(("proxy.example.org", 8081))`', src/lib.rs:515:9

---- tests::ftp_proxy_specific stdout ----
thread 'tests::ftp_proxy_specific' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(("proxy.example.com", 8080))`', src/lib.rs:496:9

---- tests::https_proxy_fallback stdout ----
thread 'tests::https_proxy_fallback' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(("proxy.example.org", 8081))`', src/lib.rs:476:9

---- tests::http_proxy_specific stdout ----
thread 'tests::http_proxy_specific' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(("proxy.example.com", 8080))`', src/lib.rs:392:9

---- tests::https_proxy_specific stdout ----
thread 'tests::https_proxy_specific' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(("proxy.example.com", 8080))`', src/lib.rs:457:9

---- tests::proxy_url_with_explicit_scheme_port stdout ----
thread 'tests::proxy_url_with_explicit_scheme_port' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(("proxy.example.com", 80))`', src/lib.rs:418:9


failures:
    tests::default_proxy_url_scheme
    tests::ftp_proxy_fallback
    tests::ftp_proxy_specific
    tests::http_proxy_fallback
    tests::http_proxy_specific
    tests::https_proxy_fallback
    tests::https_proxy_specific
    tests::proxy_url_with_explicit_scheme_port

test result: FAILED. 5 passed; 8 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--lib'

But pass if you run them with URL 2.0:

> rg url Cargo.lock 
14: "url 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
72:name = "url"
91:"checksum url 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "77ddaf52e65c6b81c56b7e957c0b1970f7937f21c5c6774c4e56fcb4e20b48c6"
> cargo +nightly test
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running target/debug/deps/env_proxy-c6ac0eaf955226c7

running 13 tests
test tests::default_proxy_url_scheme ... ok
test tests::http_proxy_fallback ... ok
test tests::ftp_proxy_fallback ... ok
test tests::ftp_proxy_specific ... ok
test tests::http_proxy_specific ... ok
test tests::https_proxy_fallback ... ok
test tests::https_proxy_specific ... ok
test tests::no_proxy_global ... ok
test tests::no_proxy_multiple_list ... ok
test tests::no_proxy_simple_name ... ok
test tests::no_proxy_subdomain ... ok
test tests::no_proxy_subdomain_dot ... ok
test tests::proxy_url_with_explicit_scheme_port ... ok

test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests env_proxy

running 2 tests
test src/lib.rs -  (line 23) ... ok
test src/lib.rs -  (line 31) ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

[stevendanna]

More discussion around the url change here: servo/rust-url#577

[inejge]

Uh, that change is not something I'd expect from a patch release. I created a workaround, try 5591cc7.

[stevendanna]

Thanks! I'll give it a try.

[stevendanna]

Your fix works for me locally, thanks again.

I started looking into whether there was a clean way to add some features upstream for this use case, I'll try to remember to follow up if I make progress on that.