hubcio commented on code in PR #3204:
URL: https://github.com/apache/iggy/pull/3204#discussion_r3180806885
##########
core/server/src/http/http_server.rs:
##########
@@ -310,9 +310,10 @@ async fn build_app_state(
Ok(manager) => manager,
Err(error) => panic!("Failed to initialize JWT manager: {error}"),
};
- if jwt_manager.load_revoked_tokens().await.is_err() {
- panic!("Failed to load revoked access tokens");
- }
+ assert!(
+ jwt_manager.load_revoked_tokens().await.is_ok(),
+ "Failed to load revoked access tokens"
+ );
Review Comment:
`assert!(... .is_ok(), "...")` drops the underlying error from
`load_revoked_tokens()`. the original `if .is_err() { panic!(...) }` had the
same issue, so this isn't a regression from this PR, but worth fixing while
you're in here:
```rust
if let Err(error) = jwt_manager.load_revoked_tokens().await {
panic!("Failed to load revoked access tokens: {error}");
}
```
##########
core/common/src/utils/versioning.rs:
##########
@@ -36,23 +36,17 @@ pub struct SemanticVersion {
/// - If the string is empty
/// - If the string contains non-digit characters
const fn const_parse_u32_range(bytes: &[u8], start: usize, end: usize) -> u32 {
- if start >= end {
- panic!("Cannot parse empty range as u32");
- }
+ assert!(start < end, "Cannot parse empty range as u32");
let mut result = 0u32;
let mut i = start;
- if bytes.is_empty() {
- panic!("Can not parse empty string as u32");
- }
+ assert!(!bytes.is_empty(), "End index out of bounds");
Review Comment:
the message `"End index out of bounds"` doesn't describe the
`!bytes.is_empty()` check - it describes a different condition (matching the
assertion at line 96 about `end <= bytes.len()`). this is a diagnostic
regression: original message was `"Can not parse empty string as u32"`, which
actually matched the check. restore the original message.
##########
core/server/src/bootstrap.rs:
##########
@@ -129,11 +129,10 @@ pub async fn create_directories(config: &SystemConfig) ->
Result<(), IggyError>
pub fn create_root_user() -> User {
let mut username = env::var(IGGY_ROOT_USERNAME_ENV);
let mut password = env::var(IGGY_ROOT_PASSWORD_ENV);
- if (username.is_ok() && password.is_err()) || (username.is_err() &&
password.is_ok()) {
- panic!(
- "When providing the custom root user credentials, both username
and password must be set."
- );
- }
+ assert!(
+ !((username.is_ok() && password.is_err()) || (username.is_err() &&
password.is_ok())),
+ "When providing the custom root user credentials, both username and
password must be set."
+ );
Review Comment:
double-negated XOR is hard to parse. equivalent and clearer:
```rust
assert_eq!(
username.is_ok(),
password.is_ok(),
"When providing the custom root user credentials, both username and
password must be set."
);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
