-
-
Notifications
You must be signed in to change notification settings - Fork 221
feat: ExceptionStatement raises custom SQL errors #829
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: master
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 |
---|---|---|
@@ -0,0 +1,46 @@ | ||
//! Custom SQL exceptions and errors | ||
use inherent::inherent; | ||
|
||
use crate::backend::SchemaBuilder; | ||
|
||
/// SQL Exceptions | ||
#[derive(Debug, Clone, PartialEq)] | ||
pub struct ExceptionStatement { | ||
pub(crate) message: String, | ||
} | ||
Comment on lines
+6
to
+10
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. As I look at the PostgreSQL docs, I'd rather have a powerful Postgres-specific #[non_exhaustive]
pub enum PgRaiseLevel {
Exception,
..
} where later we could extend We can keep your minimal "portable" API too, if you document that it's not a portable SQL feature, but an "artifitial" shortcut for the common subset of database-specific exception features. Which all seem very different from each other, judging by the SQL you generate in the implementation. And obviously, it'd switch the implementation to just delegate to the database-specific query builder: Personally, I develop against Postgres and don't care about portability. But I'm not sure how much the SeaQL community cares in general |
||
|
||
impl ExceptionStatement { | ||
pub fn new(message: String) -> Self { | ||
Self { message } | ||
} | ||
} | ||
|
||
pub trait ExceptionStatementBuilder { | ||
/// Build corresponding SQL statement for certain database backend and return SQL string | ||
fn build<T: SchemaBuilder>(&self, schema_builder: T) -> String; | ||
|
||
/// Build corresponding SQL statement for certain database backend and return SQL string | ||
fn build_any(&self, schema_builder: &dyn SchemaBuilder) -> String; | ||
|
||
/// Build corresponding SQL statement for certain database backend and return SQL string | ||
fn to_string<T: SchemaBuilder>(&self, schema_builder: T) -> String { | ||
self.build(schema_builder) | ||
} | ||
} | ||
|
||
#[inherent] | ||
impl ExceptionStatementBuilder for ExceptionStatement { | ||
pub fn build<T: SchemaBuilder>(&self, schema_builder: T) -> String { | ||
let mut sql = String::with_capacity(256); | ||
schema_builder.prepare_exception_statement(self, &mut sql); | ||
sql | ||
} | ||
|
||
pub fn build_any(&self, schema_builder: &dyn SchemaBuilder) -> String { | ||
let mut sql = String::with_capacity(256); | ||
schema_builder.prepare_exception_statement(self, &mut sql); | ||
sql | ||
} | ||
|
||
pub fn to_string<T: SchemaBuilder>(&self, schema_builder: T) -> String; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
use super::*; | ||
use pretty_assertions::assert_eq; | ||
|
||
#[test] | ||
fn signal_sqlstate() { | ||
let message = "Some error occurred"; | ||
assert_eq!( | ||
ExceptionStatement::new(message.to_string()).to_string(MysqlQueryBuilder), | ||
format!("SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = '{message}'") | ||
); | ||
} | ||
|
||
#[test] | ||
fn escapes_message() { | ||
let unescaped_message = "Does this 'break'?"; | ||
assert_eq!( | ||
ExceptionStatement::new(unescaped_message.to_string()).to_string(MysqlQueryBuilder), | ||
format!("SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Does this \\'break\\'?'") | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
use sea_query::{extension::mysql::*, tests_cfg::*, *}; | ||
|
||
mod exception; | ||
mod foreign_key; | ||
mod index; | ||
mod query; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
use super::*; | ||
use pretty_assertions::assert_eq; | ||
|
||
#[test] | ||
fn raise_exception() { | ||
let message = "Some error occurred"; | ||
assert_eq!( | ||
ExceptionStatement::new(message.to_string()).to_string(PostgresQueryBuilder), | ||
format!("RAISE EXCEPTION '{message}'") | ||
); | ||
} | ||
|
||
#[test] | ||
fn escapes_message() { | ||
let unescaped_message = "Does this 'break'?"; | ||
assert_eq!( | ||
ExceptionStatement::new(unescaped_message.to_string()).to_string(PostgresQueryBuilder), | ||
format!("RAISE EXCEPTION E'Does this \\'break\\'?'") | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
use sea_query::{tests_cfg::*, *}; | ||
|
||
mod exception; | ||
mod foreign_key; | ||
mod index; | ||
mod query; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
use super::*; | ||
use pretty_assertions::assert_eq; | ||
|
||
#[test] | ||
fn select_raise_abort() { | ||
let message = "Some error occurred here"; | ||
assert_eq!( | ||
ExceptionStatement::new(message.to_string()).to_string(SqliteQueryBuilder), | ||
format!("SELECT RAISE(ABORT, '{}')", message) | ||
); | ||
} | ||
|
||
#[test] | ||
fn escapes_message() { | ||
let unescaped_message = "Does this 'break'?"; | ||
let escaped_message = "Does this ''break''?"; | ||
assert_eq!( | ||
ExceptionStatement::new(unescaped_message.to_string()).to_string(SqliteQueryBuilder), | ||
format!("SELECT RAISE(ABORT, '{}')", escaped_message) | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
use sea_query::{tests_cfg::*, *}; | ||
|
||
mod exception; | ||
mod foreign_key; | ||
mod index; | ||
mod query; | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
Oh, this doesn't look good to me. Why make it a provided method if you don't, in fact... provide it?
To me, such a provided method would look OK if it returned
Result<(), ExceptionsNotImplemented>
instead of panicking. But I see that the callingprepare_simple_expr_common
doesn't return aResult
anyway. Perhaps, we've hit some problem/limitation of thesea_query
design that I don't fully understand yet. Perhaps, we shouldn't even try to pretend that exceptions are a portable and universally-supported feature (see #829 (comment))