-
-
Notifications
You must be signed in to change notification settings - Fork 208
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?
Conversation
cef173e
to
c9246bf
Compare
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.
Haven't reviewed the rest yet. Consider running cargo semver-checks
to find and document the breaking changes.
Disclaimer: I'm not a maintainer
@@ -35,6 +35,7 @@ pub enum SimpleExpr { | |||
AsEnum(DynIden, Box<SimpleExpr>), | |||
Case(Box<CaseStatement>), | |||
Constant(Value), | |||
Exception(ExceptionStatement), |
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.
You should mention that this is a breaking change (adding a new variant to a public enum not marked as #[non_exhaustive]
).
Perhaps, we should add #[non_exhaustive]
in the next major version. That's also a breaking change. But after that, changes like yours can me merged anytime with no problems
Expurple is a big contributor :)
I agree, and I am starting to merge breaking changes now |
/// SQL Exceptions | ||
#[derive(Debug, Clone, PartialEq)] | ||
pub struct ExceptionStatement { | ||
pub(crate) message: String, | ||
} |
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.
As I look at the PostgreSQL docs, I'd rather have a powerful Postgres-specific PgRaiseStatement
with an explicit
#[non_exhaustive]
pub enum PgRaiseLevel {
Exception,
..
}
where later we could extend PgRaiseStatement
in a backwards-compatible way and add methods for format args, SQLSTATE
and USING
(I'm not asking to contribute everything under the sun in this one PR).
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: PgRaiseStatement
, etc.
Personally, I develop against Postgres and don't care about portability. But I'm not sure how much the SeaQL community cares in general
// Translate [`Exception`] into SQL statement. | ||
fn prepare_exception_statement( | ||
&self, | ||
_exception: &ExceptionStatement, | ||
_sql: &mut dyn SqlWriter, | ||
) { | ||
panic!("Exception handling not implemented for this backend"); | ||
} |
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 calling prepare_simple_expr_common
doesn't return a Result
anyway. Perhaps, we've hit some problem/limitation of the sea_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))
PR Info
Allows defining SQL errors, for instance in an
IfElseStatement
#828 inside aTriggerStatement
#824 that rejects an update on certain conditions.New Features
ExceptionStatement
raises custom SQL errorsBreaking Changes
SimpleExpr:: Exception
variant to public enum