Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Issue Report] Remove all .unwrap()s #424

Open
Antosser opened this issue Feb 28, 2024 · 3 comments · May be fixed by #426
Open

[Issue Report] Remove all .unwrap()s #424

Antosser opened this issue Feb 28, 2024 · 3 comments · May be fixed by #426
Assignees
Labels
bug Something isn't working tech debt Leftovers from feature implementations that needs improvement

Comments

@Antosser
Copy link
Contributor

This project contains A LOT of unwraps, which can be easily avoided and removed, making the code much rustier. I'd like to do that after #423 gets merged.

To handle all possible errors, we would need a proper Result type; Result<T, Box<dyn Error>> won't do. We could use anyhow, but I'd suggest color_eyre because of the fancier look when an error happens. Let me know what you think

Here's a list of all unwraps or expects I found using ripgrep:

tests\gzip.rs
10:        let headers = request.headers_mut().unwrap();
15:                http::HeaderValue::from_str(accept_encoding).unwrap(),
21:            .request(request.body(Body::empty()).unwrap())
23:            .unwrap()

tests\cors.rs
9:        client.get(url.parse().unwrap()).await.unwrap()

benches\file_explorer.rs
13:    HTTP_CLIENT.get(uri.parse().unwrap()).await.unwrap();
17:    let rt = Runtime::new().unwrap();
25:    let rt = Runtime::new().unwrap();
34:    let rt = Runtime::new().unwrap();

tests\basic_auth.rs
10:        client.get(url.parse().unwrap()).await.unwrap()
18:        let headers = request.headers_mut().unwrap();
22:            HeaderValue::from_str(credentials.as_http_header().as_str()).unwrap(),
27:            .request(request.body(Body::empty()).unwrap())
29:            .unwrap()

src\utils\url_encode.rs
16:            let component = component.to_str().unwrap();
46:        let file_path = PathBuf::from_str(file_path).unwrap();
59:        let file_path = file_path.to_str().unwrap();

src\cli.rs
81:            host: "127.0.0.1".parse().unwrap(),
85:            root_dir: PathBuf::from_str("./").unwrap(),
88:            tls_cert: PathBuf::from_str("cert.pem").unwrap(),
89:            tls_key: PathBuf::from_str("key.rsa").unwrap(),
119:            host: "0.0.0.0".parse().unwrap(),
136:            host: "192.168.0.1".parse().unwrap(),
148:            root_dir: PathBuf::from_str("~/User/sources/http-server").unwrap(),
235:            tls_cert: PathBuf::from_str("~/secrets/cert").unwrap(),
236:            tls_key: PathBuf::from_str("~/secrets/key").unwrap(),

tests\defacto.rs
9:        client.get(url.parse().unwrap()).await.unwrap()

src\addon\proxy.rs
25:        let upstream = Uri::from_str(upstream).unwrap();
41:            HeaderValue::from_str(self.upstream.authority().unwrap().as_str()).unwrap(),
46:        tokio::spawn(async move { client.request(outogoing).await.unwrap() })
48:            .unwrap()
66:        let via_header = HeaderValue::from_str(&via_header_str).unwrap();
69:            let current_via_header = current_via_header.to_str().unwrap();
84:                HeaderValue::from_str(proxies_list.as_str()).unwrap(),
139:            .unwrap();
152:            HeaderValue::from_str(self.upstream.authority().unwrap().as_str()).unwrap(),
156:        headers.remove(USER_AGENT).unwrap();
163:        let scheme = self.upstream.scheme_str().unwrap();
164:        let authority = self.upstream.authority().unwrap().as_str();
176:            .unwrap()
206:        let via_header_value = headers.get(&HeaderName::from_static("via")).unwrap();
207:        let via_header_value = via_header_value.to_str().unwrap();
220:            HeaderValue::from_str("HTTP/1.1 GoodProxy").unwrap(),
232:        let via_header_value = headers.get(&HeaderName::from_static("via")).unwrap();
233:        let via_header_value = via_header_value.to_str().unwrap();
247:            (CONNECTION, HeaderValue::from_str("keep-alive").unwrap()),

src\addon\cors.rs
98:                HeaderValue::from_str("true").unwrap(),
107:                HeaderValue::from_str(allow_headers.as_str()).unwrap(),
116:                HeaderValue::from_str(allow_methods.as_str()).unwrap(),
123:                HeaderValue::from_str(allow_origin.as_str()).unwrap(),
132:                HeaderValue::from_str(expose_headers.as_str()).unwrap(),
139:                HeaderValue::from_str(max_age.to_string().as_str()).unwrap(),
148:                HeaderValue::from_str(request_headers.as_str()).unwrap(),
155:                HeaderValue::from_str(request_method.as_str()).unwrap(),
375:        assert_eq!(cors, Cors::try_from(config).unwrap());

src\addon\compression\gzip.rs
93:                let mut buffer_cursor = aggregate(body).await.unwrap();
106:                HeaderValue::from_str("gzip").unwrap(),
140:                HeaderValue::from_str("gzip, deflate").unwrap(),
151:        let response = response_builder.body(response_body).unwrap();
160:        let compressed = compress(raw).unwrap();
175:            .unwrap();
183:                .unwrap(),
198:            let mut buffer_cursor = aggregate(body).await.unwrap();
207:            .unwrap();
213:            let mut buffer_cursor = aggregate(compressed_body).await.unwrap();

src\utils\error.rs
19:            .unwrap(),
21:        .unwrap()

src\addon\file_server\query_params.rs
71:        let have = QueryParams::from_str(uri_string).unwrap();

src\config\util\tls.rs
35:        path.to_str().unwrap()
38:    let cert_bytes = &rustls_pemfile::certs(&mut buf_reader).unwrap()[0];
45:        .with_context(|| format!("Unable to find the TLS keys on: {}", path.to_str().unwrap()))?;
49:            let path = path.to_str().unwrap();
54:            let path = path.to_str().unwrap();

src\config\mod.rs
47:        let root_dir = current_dir().unwrap();
73:        let root_dir = if cli_arguments.root_dir.to_str().unwrap() == "./" {
74:            current_dir().unwrap()
76:            let root_dir = cli_arguments.root_dir.to_str().unwrap();
112:                    cli_arguments.username.unwrap(),
113:                    cli_arguments.password.unwrap(),
126:            let proxy_url = cli_arguments.proxy.unwrap();

src\server\mod.rs
48:            let https_config = config.tls.clone().unwrap();
73:            server_task.await.unwrap();
92:            if self.config.address.ip() == Ipv4Addr::from_str("0.0.0.0").unwrap() {
122:        let server = https_server_builder.make_server(address).await.unwrap();
137:            if self.config.address.ip() == Ipv4Addr::from_str("0.0.0.0").unwrap() {

src\config\file.rs
57:    let path = PathBuf::from_str(value).unwrap();
58:    let canon = fs::canonicalize(path).unwrap();
88:        let config = ConfigFile::parse_toml(file_contents).unwrap();
89:        let mut root_dir = std::env::current_dir().unwrap();
112:        ConfigFile::parse_toml(file_contents).unwrap();
129:        let root_dir = Some(std::env::current_dir().unwrap());
131:            cert: PathBuf::from_str("cert_123.pem").unwrap(),
132:            key: PathBuf::from_str("key_123.pem").unwrap(),
135:        let config = ConfigFile::parse_toml(file_contents).unwrap();
140:        assert_eq!(config.tls.unwrap(), tls);
157:        let root_dir = Some(std::env::current_dir().unwrap());
159:            cert: PathBuf::from_str("cert_123.pem").unwrap(),
160:            key: PathBuf::from_str("key_123.pem").unwrap(),
163:        let config = ConfigFile::parse_toml(file_contents).unwrap();
168:        assert_eq!(config.tls.unwrap(), tls);
205:        let config = ConfigFile::parse_toml(file_contents).unwrap();
206:        let root_dir = Some(std::env::current_dir().unwrap());
211:        assert_eq!(config.cors.unwrap(), cors);
232:        let root_dir = Some(std::env::current_dir().unwrap());
253:        let config = ConfigFile::parse_toml(file_contents).unwrap();
258:        assert_eq!(config.cors.unwrap(), cors);
270:        let config = ConfigFile::parse_toml(file_contents).unwrap();
272:        assert!(config.compression.unwrap().gzip);
285:        let config = ConfigFile::parse_toml(file_contents).unwrap();
289:        let basic_auth = config.basic_auth.unwrap();
302:        let config = ConfigFile::parse_toml(file_contents).unwrap();
316:        let config = ConfigFile::parse_toml(file_contents).unwrap();
320:        let proxy = config.proxy.unwrap();
332:        let config = ConfigFile::parse_toml(file_contents).unwrap();
335:        assert!(config.graceful_shutdown.unwrap());

src\addon\file_server\scoped_file_system.rs
153:        let sfs = ScopedFileSystem::new(PathBuf::from("")).unwrap();
167:        let sfs = ScopedFileSystem::new(PathBuf::from("")).unwrap();
175:        let sfs = ScopedFileSystem::new(PathBuf::from("")).unwrap();
183:        let sfs = ScopedFileSystem::new(PathBuf::from("")).unwrap();
196:            normalized.to_str().unwrap(),
203:        let sfs = ScopedFileSystem::new(PathBuf::from("")).unwrap();
204:        let resolved_entry = sfs.resolve(PathBuf::from("assets/logo.svg")).await.unwrap();
215:        let sfs = ScopedFileSystem::new(PathBuf::from("")).unwrap();
216:        let resolved_entry = sfs.resolve(PathBuf::from("assets/")).await.unwrap();
223:        let sfs = ScopedFileSystem::new(PathBuf::from("")).unwrap();
224:        let resolved_entry = sfs.resolve(PathBuf::from("assets")).await.unwrap();
231:        let sfs = ScopedFileSystem::new(PathBuf::from("")).unwrap();

src\server\handler\mod.rs
49:            let middleware = Middleware::try_from(config).unwrap();
60:        let middleware = Middleware::try_from(config).unwrap();

src\addon\file_server\mod.rs
45:        let scoped_file_system = ScopedFileSystem::new(config.root_dir.clone()).unwrap();
59:        let template = std::str::from_utf8(template).unwrap();
63:            .unwrap();
224:            .unwrap();
254:            .unwrap()
377:        let path = entry_path.strip_prefix(root_dir).unwrap();
393:        let root_dir = PathBuf::from_str("/Users/bob/sources/http-server").unwrap();
396:                .unwrap();
421:            let sanitized_path = FileServer::parse_path(req_uri).unwrap().0;
422:            let wanted_path = PathBuf::from_str(want[idx]).unwrap();
430:        let root_dir = PathBuf::from_str("/Users/bob/sources/http-server").unwrap();
433:                .unwrap();
434:        let breadcrumbs = FileServer::breadcrumbs_from_path(&root_dir, &entry_path).unwrap();

src\server\handler\file_server.rs
32:            let response = self.file_server.resolve(req_path).await.unwrap();

src\server\middleware\cors.rs
27:    let cors = Cors::try_from(cors_config).unwrap();
@EstebanBorai EstebanBorai changed the title [Feature Request] Remove all .unwrap()s [Issue Report] Remove all .unwrap()s Feb 28, 2024
@EstebanBorai
Copy link
Member

Totally agree! Go for it!

@EstebanBorai EstebanBorai added bug Something isn't working tech debt Leftovers from feature implementations that needs improvement labels Feb 28, 2024
@EstebanBorai
Copy link
Member

I think having unwraps in tests is fine at least for the needs on this project because in tests we are validating specific scenarios.

If test runs on panic due to unwrap, its a good signal of either:

  • Invalid assumption
  • Invalid state

Both determines failure in any case so I would keep unwraps in tests for simplicity sake.

I cant say the same with end user code/source. No unwraps/cases of panic should exist there.

@Antosser
Copy link
Contributor Author

@EstebanBorai Please review #423 first to avoid collisions

I think having unwraps in tests is fine at least for the needs on this project because in tests we are validating specific scenarios.

I agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tech debt Leftovers from feature implementations that needs improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants