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

fix: makes Header.to_hashmap() not call unwrap. #396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ArtRand
Copy link

@ArtRand ArtRand commented Jun 14, 2023

I've recently encountered an issue where the to_hashmap method will cause a panic (issue). #276 reports a similar problem. I've refactored the function to return a Result allowing the caller to handle the failure. I understand this changes the API and it may be considered unconventional to have a to_* method return a result, so I'm open to discussion about other ways to aviod the panic.

Regex still calls unwrap, add test to make sure it's safe. Change API to
return a Result.
@ArtRand ArtRand changed the title suggest: makes Header.to_hashmap() not call unwrap. fix: makes Header.to_hashmap() not call unwrap. Jun 14, 2023
@tedil
Copy link
Member

tedil commented Jun 18, 2023

An alternative would be to implement TryFrom (i.e. impl TryFrom<Header> for HashMap<String, String>), such that

let header_map: HashMap<_, _> = header.try_into()?;

API wise, that would be more difficult to discover, but it does leverage existing traits and clearly signals that something during conversion might fail.

(Also, instead of the if let, one could use a combination of .map and .map_err instead, but that boils down to personal preference I guess ;) )

@veldsla
Copy link
Contributor

veldsla commented Jun 19, 2023

This possible panic was already fixed in #384 (released in version 0.43.0) header tags may now be empty.

This is already more lenient than the official spec, it should be an error. Perhaps the unwrap could be replaced with an .expect explaining the file is corrupt/not conforming to specification

@ArtRand
Copy link
Author

ArtRand commented Jun 21, 2023

@tedil

I'm happy to implement it as a TryFrom if you think that's best, I tend to agree that discovery would be difficult.

@veldsla

I appreciate that this particular error would not cause a panic with the new regex pattern. However, we've all experienced out-of-spec data. At the very least I'd expect the method's documentation to mention that it can cause a panic when the header isn't to the spec and (IMO) it's much more ideal to communicate a potential failure with a Result type.

I've refactored this function call out of my project, but would be more than happy to contribute a more reliable solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants