Skip to content

Commit a28a7d3

Browse files
authored
refactor(catalog): add ErrorCodeResultExt trait for cleaner error handling (#18545)
* refactor(catalog): add ErrorCodeResultExt trait for cleaner error handling Implement extension trait for `Result<T, ErrorCode>` that converts specific error codes to None while propagating other errors. This eliminates repetitive match statements throughout the codebase. Key improvements: - Add `ErrorCodeResultExt` trait with `or_error_codes(&[u16])` method - Add convenience methods: `or_unknown_database`, `or_unknown_table` - Refactor 6 DatabaseCatalog methods to use clean if let Some patterns - Separate result fetching from result handling for better readability - Maintain precise error semantics (`UNKNOWN_DATABASE` vs `UNKNOWN_DATABASE_ID`) * chore: fix review comments
1 parent b144eab commit a28a7d3

File tree

8 files changed

+639
-103
lines changed

8 files changed

+639
-103
lines changed

src/common/exception/src/exception.rs

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,3 +328,55 @@ impl<C> Clone for ErrorCode<C> {
328328
.set_stacks(self.stacks().to_vec())
329329
}
330330
}
331+
332+
/// Common error code groups for use with ErrorCodeResultExt
333+
pub mod error_code_groups {
334+
use super::ErrorCode;
335+
336+
/// All "unknown resource" errors that typically mean "not found"
337+
pub const UNKNOWN_RESOURCE: &[u16] = &[
338+
ErrorCode::UNKNOWN_DATABASE,
339+
ErrorCode::UNKNOWN_TABLE,
340+
ErrorCode::UNKNOWN_CATALOG,
341+
ErrorCode::UNKNOWN_VIEW,
342+
ErrorCode::UNKNOWN_COLUMN,
343+
ErrorCode::UNKNOWN_SEQUENCE,
344+
];
345+
}
346+
347+
/// Extension trait for Result<T, ErrorCode> to handle common error patterns
348+
pub trait ErrorCodeResultExt<T> {
349+
/// Converts any of the specified error codes to None, other errors propagate, success becomes Some(T)
350+
fn or_error_codes(self, codes: &[u16]) -> Result<Option<T>>;
351+
352+
/// Converts UNKNOWN_DATABASE errors to None, other errors propagate, success becomes Some(T)
353+
fn or_unknown_database(self) -> Result<Option<T>>;
354+
355+
/// Converts UNKNOWN_TABLE errors to None, other errors propagate, success becomes Some(T)
356+
fn or_unknown_table(self) -> Result<Option<T>>;
357+
358+
/// Converts common "not found" errors to None (database, table, sequence, etc.)
359+
fn or_unknown_resource(self) -> Result<Option<T>>;
360+
}
361+
362+
impl<T> ErrorCodeResultExt<T> for Result<T> {
363+
fn or_error_codes(self, codes: &[u16]) -> Result<Option<T>> {
364+
match self {
365+
Ok(value) => Ok(Some(value)),
366+
Err(e) if codes.iter().any(|c| *c == e.code()) => Ok(None),
367+
Err(e) => Err(e),
368+
}
369+
}
370+
371+
fn or_unknown_database(self) -> Result<Option<T>> {
372+
self.or_error_codes(&[ErrorCode::UNKNOWN_DATABASE])
373+
}
374+
375+
fn or_unknown_table(self) -> Result<Option<T>> {
376+
self.or_error_codes(&[ErrorCode::UNKNOWN_TABLE])
377+
}
378+
379+
fn or_unknown_resource(self) -> Result<Option<T>> {
380+
self.or_error_codes(error_code_groups::UNKNOWN_RESOURCE)
381+
}
382+
}

src/common/exception/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,9 @@ pub use context::ErrorFrame;
3030
pub use context::ResultExt;
3131
pub use databend_common_ast::span;
3232
pub use databend_common_ast::ParseError;
33+
pub use exception::error_code_groups;
3334
pub use exception::ErrorCode;
35+
pub use exception::ErrorCodeResultExt;
3436
pub use exception::Result;
3537
pub use exception::ToErrorCode;
3638
pub use exception_backtrace::set_backtrace;
Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
// Copyright 2025 Datafuse Labs.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
//! Tests for ErrorCodeResultExt trait functionality
16+
17+
use databend_common_exception::error_code_groups;
18+
use databend_common_exception::ErrorCode;
19+
use databend_common_exception::ErrorCodeResultExt;
20+
use databend_common_exception::Result;
21+
22+
fn create_unknown_database_error() -> Result<String> {
23+
Err(ErrorCode::UnknownDatabase("test_db"))
24+
}
25+
26+
fn create_unknown_table_error() -> Result<String> {
27+
Err(ErrorCode::UnknownTable("test_table"))
28+
}
29+
30+
fn create_unknown_sequence_error() -> Result<String> {
31+
Err(ErrorCode::UnknownSequence("test_sequence"))
32+
}
33+
34+
fn create_internal_error() -> Result<String> {
35+
Err(ErrorCode::Internal("internal error"))
36+
}
37+
38+
fn create_success() -> Result<String> {
39+
Ok("success".to_string())
40+
}
41+
42+
#[test]
43+
fn test_or_error_codes_with_matching_error() {
44+
let result = create_unknown_database_error().or_error_codes(&[ErrorCode::UNKNOWN_DATABASE]);
45+
assert!(result.is_ok());
46+
assert_eq!(result.unwrap(), None);
47+
}
48+
49+
#[test]
50+
fn test_or_error_codes_with_non_matching_error() {
51+
let result = create_internal_error().or_error_codes(&[ErrorCode::UNKNOWN_DATABASE]);
52+
assert!(result.is_err());
53+
assert_eq!(result.unwrap_err().code(), ErrorCode::INTERNAL);
54+
}
55+
56+
#[test]
57+
fn test_or_error_codes_with_success() {
58+
let result = create_success().or_error_codes(&[ErrorCode::UNKNOWN_DATABASE]);
59+
assert!(result.is_ok());
60+
assert_eq!(result.unwrap(), Some("success".to_string()));
61+
}
62+
63+
#[test]
64+
fn test_or_error_codes_with_multiple_codes() {
65+
let codes = &[ErrorCode::UNKNOWN_DATABASE, ErrorCode::UNKNOWN_TABLE];
66+
67+
let result1 = create_unknown_database_error().or_error_codes(codes);
68+
assert!(result1.is_ok());
69+
assert_eq!(result1.unwrap(), None);
70+
71+
let result2 = create_unknown_table_error().or_error_codes(codes);
72+
assert!(result2.is_ok());
73+
assert_eq!(result2.unwrap(), None);
74+
75+
let result3 = create_internal_error().or_error_codes(codes);
76+
assert!(result3.is_err());
77+
assert_eq!(result3.unwrap_err().code(), ErrorCode::INTERNAL);
78+
}
79+
80+
#[test]
81+
fn test_or_unknown_database() {
82+
let result1 = create_unknown_database_error().or_unknown_database();
83+
assert!(result1.is_ok());
84+
assert_eq!(result1.unwrap(), None);
85+
86+
let result2 = create_unknown_table_error().or_unknown_database();
87+
assert!(result2.is_err());
88+
assert_eq!(result2.unwrap_err().code(), ErrorCode::UNKNOWN_TABLE);
89+
90+
let result3 = create_success().or_unknown_database();
91+
assert!(result3.is_ok());
92+
assert_eq!(result3.unwrap(), Some("success".to_string()));
93+
}
94+
95+
#[test]
96+
fn test_or_unknown_table() {
97+
let result1 = create_unknown_table_error().or_unknown_table();
98+
assert!(result1.is_ok());
99+
assert_eq!(result1.unwrap(), None);
100+
101+
let result2 = create_unknown_database_error().or_unknown_table();
102+
assert!(result2.is_err());
103+
assert_eq!(result2.unwrap_err().code(), ErrorCode::UNKNOWN_DATABASE);
104+
105+
let result3 = create_success().or_unknown_table();
106+
assert!(result3.is_ok());
107+
assert_eq!(result3.unwrap(), Some("success".to_string()));
108+
}
109+
110+
#[test]
111+
fn test_or_unknown_resource() {
112+
// Test with database error (should convert to None)
113+
let result1 = create_unknown_database_error().or_unknown_resource();
114+
assert!(result1.is_ok());
115+
assert_eq!(result1.unwrap(), None);
116+
117+
// Test with table error (should convert to None)
118+
let result2 = create_unknown_table_error().or_unknown_resource();
119+
assert!(result2.is_ok());
120+
assert_eq!(result2.unwrap(), None);
121+
122+
// Test with sequence error (should convert to None)
123+
let result3 = create_unknown_sequence_error().or_unknown_resource();
124+
assert!(result3.is_ok());
125+
assert_eq!(result3.unwrap(), None);
126+
127+
// Test with non-resource error (should propagate)
128+
let result4 = create_internal_error().or_unknown_resource();
129+
assert!(result4.is_err());
130+
assert_eq!(result4.unwrap_err().code(), ErrorCode::INTERNAL);
131+
132+
// Test with success
133+
let result5 = create_success().or_unknown_resource();
134+
assert!(result5.is_ok());
135+
assert_eq!(result5.unwrap(), Some("success".to_string()));
136+
}
137+
138+
#[test]
139+
fn test_error_code_groups() {
140+
// Verify that the error code groups contain expected values
141+
assert!(error_code_groups::UNKNOWN_RESOURCE.contains(&ErrorCode::UNKNOWN_DATABASE));
142+
assert!(error_code_groups::UNKNOWN_RESOURCE.contains(&ErrorCode::UNKNOWN_TABLE));
143+
assert!(error_code_groups::UNKNOWN_RESOURCE.contains(&ErrorCode::UNKNOWN_SEQUENCE));
144+
assert!(error_code_groups::UNKNOWN_RESOURCE.contains(&ErrorCode::UNKNOWN_CATALOG));
145+
assert!(error_code_groups::UNKNOWN_RESOURCE.contains(&ErrorCode::UNKNOWN_VIEW));
146+
assert!(error_code_groups::UNKNOWN_RESOURCE.contains(&ErrorCode::UNKNOWN_COLUMN));
147+
148+
// Verify that it doesn't contain unrelated error codes
149+
assert!(!error_code_groups::UNKNOWN_RESOURCE.contains(&ErrorCode::INTERNAL));
150+
assert!(!error_code_groups::UNKNOWN_RESOURCE.contains(&ErrorCode::UNKNOWN_DATABASE_ID));
151+
assert!(!error_code_groups::UNKNOWN_RESOURCE.contains(&ErrorCode::UNKNOWN_TABLE_ID));
152+
}
153+
154+
#[test]
155+
fn test_method_chaining() {
156+
// Test that methods can be chained with other Result operations
157+
let result = create_success().or_unknown_database().map(|opt| match opt {
158+
Some(val) => val.to_uppercase(),
159+
None => "DEFAULT".to_string(),
160+
});
161+
162+
assert!(result.is_ok());
163+
assert_eq!(result.unwrap(), "SUCCESS");
164+
165+
let result2 = create_unknown_database_error()
166+
.or_unknown_database()
167+
.map(|opt| match opt {
168+
Some(val) => val.to_uppercase(),
169+
None => "DEFAULT".to_string(),
170+
});
171+
172+
assert!(result2.is_ok());
173+
assert_eq!(result2.unwrap(), "DEFAULT");
174+
}
175+
176+
#[test]
177+
fn test_precise_error_semantics() {
178+
// Verify that UNKNOWN_DATABASE and UNKNOWN_DATABASE_ID are treated differently
179+
let unknown_db_id_error: Result<String> = Err(ErrorCode::UnknownDatabaseId("test_id"));
180+
181+
// or_unknown_database should NOT convert UNKNOWN_DATABASE_ID to None
182+
let result = unknown_db_id_error.or_unknown_database();
183+
assert!(result.is_err());
184+
assert_eq!(result.unwrap_err().code(), ErrorCode::UNKNOWN_DATABASE_ID);
185+
186+
// Similarly for table IDs
187+
let unknown_table_id_error: Result<String> = Err(ErrorCode::UnknownTableId("test_id"));
188+
let result2 = unknown_table_id_error.or_unknown_table();
189+
assert!(result2.is_err());
190+
assert_eq!(result2.unwrap_err().code(), ErrorCode::UNKNOWN_TABLE_ID);
191+
}

src/common/exception/tests/it/main.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15+
mod error_code_result_ext;
1516
mod exception;
1617
mod exception_flight;
1718
mod prelude;

0 commit comments

Comments
 (0)