55 bug error handling improvement by kweonminsung · Pull Request #58 · kweonminsung/bindizr

kweonminsung · 2026-01-19T17:17:41Z

No description provided.

Copilot

Pull request overview

This pull request improves error handling across the codebase by introducing strongly-typed error enums using the thiserror crate, replacing the previous String-based error handling. The changes span database repositories, API services, controllers, and the RNDC client module.

Changes:

Introduced typed error enums (DatabaseError, RndcError, ConfigError, ValidationError, ApiError, BindizrError) with proper error messages
Refactored all database repository implementations (SQLite, PostgreSQL, MySQL) to use DatabaseError instead of String errors
Updated API service layer to use ApiError with proper HTTP status code mapping
Improved RNDC client by removing panic catching and using proper error propagation
Updated dependencies (rndc 0.1.3 → 0.1.4, thiserror 2.0.17 added, GitHub actions/checkout v5 → v6)
Fixed test helper code to remove unnecessary reference operators and improved floating-point comparison tests
Minor code formatting improvements in serializer and CLI modules

Reviewed changes

Copilot reviewed 48 out of 49 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
src/error/mod.rs	Top-level BindizrError enum (currently unused)
src/database/error.rs	DatabaseError enum with sqlx::Error conversion
src/database/repository/*.rs	All repository implementations updated to return DatabaseError
src/rndc/error.rs	RndcError enum for RNDC-specific errors
src/rndc/mod.rs	Removed panic catching, improved error handling
src/config/error.rs	ConfigError enum (currently unused)
src/api/error.rs	ApiError enum with HTTP response conversion
src/api/validation.rs	ValidationError enum (currently unused)
src/api/service/*.rs	Updated to use ApiError, but contains semantic errors in error type selection
src/api/controller/*.rs	Simplified error handling using ApiError::into_response()
tests/common/mod.rs	Fixed unnecessary reference operators for Copy types
src/config/tests.rs	Improved floating-point comparison using epsilon
Cargo.toml	Added thiserror dependency, updated rndc version
.github/workflows/*.yml	Updated actions/checkout from v5 to v6
renovate.json	Added automerge configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-19T17:23:13Z

src/api/service/record.rs

+                return Err(ApiError::NotFound(format!(
                    "A record with name '{}' already exists in this zone, so CNAME cannot be used",
                    update_record_request.name
-                ));
+                )));


The error type used here is semantically incorrect. When a CNAME validation fails due to existing records, it should return ApiError::BadRequest or a validation error, not ApiError::NotFound. NotFound status should be reserved for cases where a requested resource doesn't exist, not for validation failures. This same issue exists in lines 323-326.

Copilot · 2026-01-19T17:23:13Z

src/api/service/record.rs

+                return Err(ApiError::NotFound(format!(
                    "A CNAME record with name '{}' already exists in this zone",
                    update_record_request.name
-                ));
+                )));


The error type used here is semantically incorrect. When a CNAME validation fails due to existing records, it should return ApiError::BadRequest or a validation error, not ApiError::NotFound. NotFound status should be reserved for cases where a requested resource doesn't exist, not for validation failures.

Copilot · 2026-01-19T17:23:13Z

src/api/service/record.rs

+                        Err(ApiError::NotFound(format!(
+                            "Failed to fetch records for zone {}",
+                            id
+                        )))


The error type used here is semantically incorrect. When fetching records from the database fails due to a database error, it should return ApiError::InternalServerError rather than ApiError::NotFound. The NotFound status should be reserved for cases where a specific resource doesn't exist, not for database query failures. This same pattern appears in lines 42-45 and line 55-57.

Copilot · 2026-01-19T17:23:14Z

src/api/service/record.rs

                    Err(e) => {
                        log_error!("Failed to fetch all records: {}", e);
-                        Err("Failed to fetch all records".to_string())
+                        Err(ApiError::BadRequest(


The error type used here is semantically incorrect. A database query failure should return ApiError::InternalServerError rather than ApiError::BadRequest. BadRequest is meant for client errors (invalid input), not server-side database failures.

Suggested change

Err(ApiError::BadRequest(

Err(ApiError::InternalServerError(

Copilot · 2026-01-19T17:23:14Z

src/api/service/record.rs

            Err(e) => {
                log_error!("Failed to fetch zone: {}", e);
-                return Err("Failed to create record".to_string());
+                return Err(ApiError::BadRequest("Failed to create record".to_string()));


The error type used here is semantically incorrect. A database query failure should return ApiError::InternalServerError rather than ApiError::BadRequest. BadRequest is meant for client errors (invalid input), not server-side database failures.

Suggested change

return Err(ApiError::BadRequest("Failed to create record".to_string()));

return Err(ApiError::InternalServerError("Failed to create record".to_string()));

Copilot · 2026-01-19T17:23:15Z

src/api/service/record.rs

+                return Err(ApiError::NotFound(format!(
                    "A CNAME record with name '{}' already exists in this zone",
                    create_record_request.name
-                ));
+                )));


The error type used here is semantically incorrect. When a CNAME validation fails due to existing records, it should return ApiError::BadRequest or a validation error, not ApiError::NotFound. NotFound status should be reserved for cases where a requested resource doesn't exist, not for validation failures.

Copilot · 2026-01-19T17:23:15Z

src/api/service/record.rs

            Err(e) => {
                log_error!("Failed to fetch record: {}", e);
-                Err("Failed to fetch record".to_string())
+                Err(ApiError::BadRequest("Failed to fetch record".to_string()))


The error type used here is semantically incorrect. A database query failure should return ApiError::InternalServerError rather than ApiError::BadRequest. BadRequest is meant for client errors (invalid input), not server-side database failures.

Copilot · 2026-01-19T17:23:16Z

src/api/service/record.rs

            Ok(Some(zone)) => zone,
            Ok(None) => {
-                return Err("Zone not found".to_string());
+                return Err(ApiError::BadRequest("Zone not found".to_string()));


The error type used here is semantically incorrect. A database query failure should return ApiError::InternalServerError rather than ApiError::BadRequest. BadRequest is meant for client errors (invalid input), not server-side database failures. This same issue exists in line 253.

Copilot · 2026-01-19T17:23:16Z

src/api/service/record.rs

            Err(e) => {
                log_error!("Failed to fetch record: {}", e);
-                Err("Failed to fetch record".to_string())
+                Err(ApiError::BadRequest("Failed to fetch record".to_string()))


The error type used here is semantically incorrect. A database query failure should return ApiError::InternalServerError rather than ApiError::BadRequest. BadRequest is meant for client errors (invalid input), not server-side database failures.

Suggested change

Err(ApiError::BadRequest("Failed to fetch record".to_string()))

Err(ApiError::InternalServerError("Failed to fetch record".to_string()))

Copilot · 2026-01-19T17:23:16Z

src/api/service/record.rs

            Err(e) => {
                log_error!("Failed to check existing record: {}", e);
-                return Err("Failed to update record".to_string());
+                return Err(ApiError::BadRequest("Failed to update record".to_string()));


The error type used here is semantically incorrect. A database query failure should return ApiError::InternalServerError rather than ApiError::BadRequest. BadRequest is meant for client errors (invalid input), not server-side database failures.

Suggested change

return Err(ApiError::BadRequest("Failed to update record".to_string()));

return Err(ApiError::InternalServerError(

"Failed to update record".to_string(),

));

Copilot

Pull request overview

Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-19T17:51:21Z

src/api/service/dns.rs

+    pub fn get_dns_status() -> Result<String, ApiError> {
        let res = match get_rndc_client().command("status") {
            Ok(response) => response,
            Err(e) => {
                log_error!("Failed to get DNS status: {}", e);
-                return Err("Failed to get DNS status".to_string());
+                return Err(ApiError::InternalServerError("Failed to get DNS status".to_string()));
            }
        };

        if !res.result {
-            return Err("Failed to get DNS status".to_string());
+            return Err(ApiError::InternalServerError("Failed to get DNS status".to_string()));
        }

        match res.text {


The DNS service methods now return ApiError, but the DNS controller is not updated to use ApiError's IntoResponse implementation. The controller should call err.into_response() instead of manually constructing error responses to maintain consistency with other controllers in the codebase.

Copilot

Pull request overview

Copilot reviewed 51 out of 52 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-19T18:00:50Z

src/rndc/mod.rs

+            client: rndc::RndcClient::new(&server_url, &algorithm, &secret_key)
+                .expect("Failed to initialize RNDC client"),


The .expect() call on RNDC client initialization will panic if the client fails to initialize, crashing the application at startup. While this may be intentional for a fail-fast approach, consider whether graceful degradation or a more informative error message would be appropriate. If the RNDC client is required for the application to function, document this requirement clearly.

Copilot · 2026-01-19T18:00:51Z

src/api/validation.rs

+use thiserror::Error;
+
+/// Validation-related errors
+#[derive(Debug, Error)]
+pub enum ValidationError {
+    #[error("Invalid input: {0}")]
+    InvalidInput(String),
+
+    #[error("Missing required field: {0}")]
+    MissingField(String),
+
+    #[error("Invalid format: {0}")]
+    InvalidFormat(String),
+
+    #[error("Constraint violation: {0}")]
+    ConstraintViolation(String),
+}


The ValidationError enum is defined but not currently used in the codebase. Consider marking it with #[allow(dead_code)] if it's intended for future use, or integrate it into the current validation logic if it should be used now.

Copilot · 2026-01-19T18:00:51Z

src/rndc/error.rs

+    #[error("Connection failed: {0}")]
+    ConnectionFailed(String),
+
+    #[error("Panic occurred: {0}")]
+    PanicOccurred(String),
+
+    #[error("Invalid response: {0}")]
+    InvalidResponse(String),
+}


The RndcError::PanicOccurred, RndcError::ConnectionFailed, and RndcError::InvalidResponse variants are defined but not used anywhere in the codebase. The panic handling was removed from the RNDC client and only CommandFailed is currently used. Consider removing unused variants or marking them with #[allow(dead_code)] if they're intended for future use.

Copilot · 2026-01-19T18:00:51Z

src/database/error.rs

+    #[error("Connection failed: {0}")]
+    ConnectionFailed(String),
+
+    #[error("Query failed: {0}")]
+    QueryFailed(String),
+
+    #[error("Record not found: {0}")]
+    NotFound(String),
+
+    #[error("Record already exists: {0}")]
+    AlreadyExists(String),
+
+    #[error("Transaction failed: {0}")]
+    TransactionFailed(String),
+
+    #[error("Migration failed: {0}")]
+    MigrationFailed(String),
+
+    #[error("Pool error: {0}")]
+    PoolError(String),
+}


Several DatabaseError variants are defined but not explicitly used: ConnectionFailed, AlreadyExists, TransactionFailed, and MigrationFailed. While these may be generated automatically through the From<sqlx::Error> implementation, consider whether all these variants are necessary or if some should be marked with #[allow(dead_code)] if they're intended for future use.

Copilot · 2026-01-19T18:00:51Z

renovate.json

  "baseBranches": [
    "develop"
  ],
+  "automerge": true,


Enabling automerge for Renovate will automatically merge dependency updates without manual review. While this can speed up dependency management, it may introduce breaking changes or bugs from updated dependencies. Consider whether this aligns with your project's risk tolerance and testing strategy. You may want to limit automerge to specific types of updates (e.g., only patch versions) using additional configuration.

Suggested change

"automerge": true,

"automerge": false,

Copilot · 2026-01-19T18:00:52Z

src/error/mod.rs

+/// Top-level error type
+#[derive(Debug, Error)]
+pub enum BindizrError {
+    #[error("Database error: {0}")]
+    Database(#[from] DatabaseError),
+
+    #[error("Configuration error: {0}")]
+    Config(#[from] ConfigError),
+
+    #[error("RNDC error: {0}")]
+    Rndc(#[from] RndcError),
+
+    #[error("Validation error: {0}")]
+    Validation(#[from] ValidationError),
+
+    #[error("Internal error: {0}")]
+    Internal(String),
+}
+
+/// Type alias
+pub type Result<T> = std::result::Result<T, BindizrError>;


The BindizrError type and its Result alias are defined but not currently used in the codebase. While this appears to be laying groundwork for future unified error handling, consider whether these should be marked with #[allow(dead_code)] or if there are immediate plans to integrate them.

Copilot · 2026-01-19T18:00:52Z

src/config/error.rs

+use thiserror::Error;
+
+/// Configuration-related errors
+#[derive(Debug, Error)]
+pub enum ConfigError {
+    #[error("Configuration not found: {0}")]
+    NotFound(String),
+
+    #[error("Invalid configuration value: {0}")]
+    InvalidValue(String),
+
+    #[error("Parse error: {0}")]
+    ParseError(String),
+
+    #[error("Missing required configuration: {0}")]
+    MissingRequired(String),
+}


The ConfigError enum is defined but not currently used in the codebase. Consider marking it with #[allow(dead_code)] if it's intended for future use, or integrate it into the current configuration error handling if it should be used now.

Copilot

Pull request overview

Copilot reviewed 52 out of 53 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 52 out of 53 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kweonminsung added 3 commits December 19, 2025 01:35

chore: update renovate

78cea2e

fix: renovate.json

72ff446

feat: migrate to enum based error

955e664

Copilot AI review requested due to automatic review settings January 19, 2026 17:17

Copilot started reviewing on behalf of kweonminsung January 19, 2026 17:18 View session