-
Notifications
You must be signed in to change notification settings - Fork 183
fix(rust/core): Handle the ASCII characters from sqlstate instead of their decimal values #4141
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,7 +79,7 @@ pub struct Error { | |
| /// A vendor-specific error code, if applicable. | ||
| pub vendor_code: i32, | ||
| /// A SQLSTATE error code, if provided, as defined by the SQL:2003 standard. | ||
| /// If not set, it should be set to `\0\0\0\0\0`. | ||
| /// If not set, it should be set to `\0\0\0\0\0` or "00000". | ||
| pub sqlstate: [c_char; 5], // TODO(alexandreyc): should we move to something else than c_char? (see https://github.com/apache/arrow-adbc/pull/1725#discussion_r1567531539) | ||
| /// Additional metadata. Introduced in ADBC 1.1.0. | ||
| pub details: Option<Vec<(String, Vec<u8>)>>, | ||
|
|
@@ -102,10 +102,26 @@ impl Error { | |
|
|
||
| impl Display for Error { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| let safe_ascii = |c: c_char| -> char { | ||
| if c == 0 { | ||
| '0' | ||
| } else if c >= 32 && c <= 126 { | ||
| char::from(c as u8) | ||
| } else { | ||
| '\u{FFFD}' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can still display the value somehow instead of hiding it? (Just stringify the integer value instead?) |
||
| } | ||
| }; | ||
| write!( | ||
| f, | ||
| "{:?}: {} (sqlstate: {:?}, vendor_code: {})", | ||
| self.status, self.message, self.sqlstate, self.vendor_code | ||
| "{:?}: {} (sqlstate: {}{}{}{}{}, vendor_code: {})", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe what'd be better is: if sqlstate is all-NUL, omit it; if all-ASCII, stringify it; else fall back to the current representation? |
||
| self.status, | ||
| self.message, | ||
| safe_ascii(self.sqlstate[0]), | ||
| safe_ascii(self.sqlstate[1]), | ||
| safe_ascii(self.sqlstate[2]), | ||
| safe_ascii(self.sqlstate[3]), | ||
| safe_ascii(self.sqlstate[4]), | ||
| self.vendor_code | ||
| ) | ||
| } | ||
| } | ||
|
|
@@ -207,3 +223,66 @@ impl From<Status> for AdbcStatusCode { | |
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn display_unset_sqlstate() { | ||
| let err = Error::with_message_and_status("something failed", Status::Unknown); | ||
| let msg = err.to_string(); | ||
| assert_eq!( | ||
| msg, | ||
| "Unknown: something failed (sqlstate: 00000, vendor_code: 0)" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn display_ascii_sqlstate() { | ||
| let err = Error { | ||
| message: "constraint violation".into(), | ||
| status: Status::Integrity, | ||
| vendor_code: 42, | ||
| sqlstate: [b'2' as c_char, b'3' as c_char, b'5' as c_char, b'0' as c_char, b'5' as c_char], | ||
| details: None, | ||
| }; | ||
| let msg = err.to_string(); | ||
| assert_eq!( | ||
| msg, | ||
| "Integrity: constraint violation (sqlstate: 23505, vendor_code: 42)" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn display_non_printable_sqlstate() { | ||
| let err = Error { | ||
| message: "bad state".into(), | ||
| status: Status::Internal, | ||
| vendor_code: 0, | ||
| sqlstate: [1, 2, 3, 4, 5], | ||
| details: None, | ||
| }; | ||
| let msg = err.to_string(); | ||
| assert_eq!( | ||
| msg, | ||
| "Internal: bad state (sqlstate: �����, vendor_code: 0)" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn display_mixed_sqlstate() { | ||
| let err = Error { | ||
| message: "mixed".into(), | ||
| status: Status::InvalidData, | ||
| vendor_code: 7, | ||
| sqlstate: [b'H' as c_char, b'V' as c_char, 0, 0, 1], | ||
| details: None, | ||
| }; | ||
| let msg = err.to_string(); | ||
| assert_eq!( | ||
| msg, | ||
| "InvalidData: mixed (sqlstate: HV00�, vendor_code: 7)" | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer something that isn't ambiguous with an actual sqlstate of '00000'.