3 chore refactor error handling #4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors error handling throughout the codebase by replacing String-based errors with a custom RndcError enum type, providing more structured and type-safe error handling.
- Introduced a new
RndcErrorenum with specific variants for different error types (InvalidAlgorithm, Base64DecodeError, NetworkError, EncodingError, DecodingError, UnknownError) - Updated all public and internal functions to return
Result<T, RndcError>instead ofResult<T, String> - Enhanced
RndcAlg::from_stringto support additional algorithm aliases (hmd5, hsha1, hsha224, etc.) and return proper errors
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/error.rs | New file defining the RndcError enum with Display and Error trait implementations for structured error handling |
| src/lib.rs | Updated RndcClient::new and all methods to use RndcError instead of String errors; added public re-export of RndcError |
| src/internal/constants.rs | Changed RndcAlg::from_string to return Result<Self, RndcError>, added support for additional algorithm aliases, and uncommented ISCCC_ALG_HMAC_MD5 constant |
| src/internal/encoder.rs | Updated encode and make_signature functions to use RndcError; replaced magic number 157 with ISCCC_ALG_HMAC_MD5 constant |
| src/internal/decoder.rs | Updated all decoding functions to return Result<T, RndcError> with proper error wrapping |
| src/internal/mod.rs | Reordered module declarations alphabetically |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl fmt::Display for RndcError { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| RndcError::InvalidAlgorithm(msg) => write!(f, "Invalid algorithm: {}", msg), |
There was a problem hiding this comment.
The error message for InvalidAlgorithm duplicates "Invalid algorithm" in both the Display implementation and the error variant constructor. When from_string is called with an unknown algorithm, the resulting error message will be "Invalid algorithm: Unknown algorithm: sha999". Consider changing line 15 to just write!(f, "{}", msg) or removing "Unknown algorithm:" from the constructor calls.
| RndcError::InvalidAlgorithm(msg) => write!(f, "Invalid algorithm: {}", msg), | |
| RndcError::InvalidAlgorithm(msg) => write!(f, "{}", msg), |
| Base64DecodeError(String), | ||
| NetworkError(String), | ||
| EncodingError(String), | ||
| DecodingError(String), |
There was a problem hiding this comment.
The UnknownError variant is defined but never used in the codebase. Consider removing it if it's not needed, or document when it should be used.
| DecodingError(String), | |
| DecodingError(String), | |
| /// A catch-all error for unexpected or unclassified failures that do not fit other variants. |
| pub fn new(server_url: &str, algorithm: &str, secret_key_b64: &str) -> Result<Self, RndcError> { | ||
| let secret_key = general_purpose::STANDARD | ||
| .decode(secret_key_b64.as_bytes()) | ||
| .expect("Invalid base64 RNDC_SECRET_KEY"); | ||
| .map_err(|e| RndcError::Base64DecodeError(e.to_string()))?; | ||
|
|
||
| RndcClient { | ||
| Ok(RndcClient { | ||
| server_url: server_url.to_string(), | ||
| algorithm: RndcAlg::from_string(algorithm).expect("Invalid RNDC algorithm"), | ||
| algorithm: RndcAlg::from_string(algorithm)?, | ||
| secret_key, | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The new error handling in RndcClient::new lacks test coverage. Consider adding tests to verify that invalid base64 input and invalid algorithm strings properly return the appropriate RndcError variants.
| pub(crate) fn from_string(alg: &str) -> Result<Self, RndcError> { | ||
| match alg { | ||
| "md5" => Some(RndcAlg::MD5), | ||
| "hmac-md5" => Some(RndcAlg::MD5), | ||
| "sha1" => Some(RndcAlg::SHA1), | ||
| "hmac-sha1" => Some(RndcAlg::SHA1), | ||
| "sha224" => Some(RndcAlg::SHA224), | ||
| "hmac-sha224" => Some(RndcAlg::SHA224), | ||
| "sha256" => Some(RndcAlg::SHA256), | ||
| "hmac-sha256" => Some(RndcAlg::SHA256), | ||
| "sha384" => Some(RndcAlg::SHA384), | ||
| "hmac-sha384" => Some(RndcAlg::SHA384), | ||
| "sha512" => Some(RndcAlg::SHA512), | ||
| "hmac-sha512" => Some(RndcAlg::SHA512), | ||
| _ => None, | ||
| "md5" => Ok(RndcAlg::MD5), | ||
| "hmd5" => Ok(RndcAlg::MD5), | ||
| "hmac-md5" => Ok(RndcAlg::MD5), | ||
| "sha1" => Ok(RndcAlg::SHA1), | ||
| "hsha1" => Ok(RndcAlg::SHA1), | ||
| "hmac-sha1" => Ok(RndcAlg::SHA1), | ||
| "sha224" => Ok(RndcAlg::SHA224), | ||
| "hsha224" => Ok(RndcAlg::SHA224), | ||
| "hmac-sha224" => Ok(RndcAlg::SHA224), | ||
| "sha256" => Ok(RndcAlg::SHA256), | ||
| "hsha256" => Ok(RndcAlg::SHA256), | ||
| "hmac-sha256" => Ok(RndcAlg::SHA256), | ||
| "sha384" => Ok(RndcAlg::SHA384), | ||
| "hsha384" => Ok(RndcAlg::SHA384), | ||
| "hmac-sha384" => Ok(RndcAlg::SHA384), | ||
| "sha512" => Ok(RndcAlg::SHA512), | ||
| "hsha512" => Ok(RndcAlg::SHA512), | ||
| "hmac-sha512" => Ok(RndcAlg::SHA512), | ||
| _ => Err(RndcError::InvalidAlgorithm(format!( | ||
| "Unknown algorithm: {}", | ||
| alg | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
The RndcAlg::from_string method now returns Result<Self, RndcError> but lacks test coverage for both valid algorithm strings and error cases. Consider adding tests to verify all supported algorithm aliases work correctly and that unknown algorithms return InvalidAlgorithm error.
src/lib.rs
Outdated
| fn get_stream(&self) -> TcpStream { | ||
| TcpStream::connect(&self.server_url).expect("Failed to connect to RNDC server") | ||
| } |
There was a problem hiding this comment.
The get_stream method still uses expect() which will panic on connection failure. Since this PR refactors error handling to use RndcError, this method should return Result<TcpStream, RndcError> and propagate errors properly instead of panicking.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ => Err(RndcError::InvalidAlgorithm(format!( | ||
| "Unknown algorithm: {}", | ||
| alg | ||
| ))), |
There was a problem hiding this comment.
The error message includes redundant text. The Display implementation already prefixes with "Invalid algorithm:", so the error created here will result in "Invalid algorithm: Unknown algorithm: ". Consider removing "Unknown algorithm:" from the message to avoid redundancy, or adjust the Display implementation.
| _ => Err(RndcError::InvalidAlgorithm(format!( | |
| "Unknown algorithm: {}", | |
| alg | |
| ))), | |
| _ => Err(RndcError::InvalidAlgorithm(alg.to_string())), |
No description provided.