Skip to content
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

feat: add begin_with to start transaction with custom SQL #3322

Closed
wants to merge 4 commits into from

Conversation

glebpom
Copy link

@glebpom glebpom commented Jul 2, 2024

No description provided.

@glebpom glebpom changed the title feat: add begin_custom feat: add begin_custom to start transaction with custom SQL Jul 2, 2024
@abonander
Copy link
Collaborator

abonander commented Jul 8, 2024

Decent start. Couple of nits:

  • Method name should be begin_with
  • Cow<'static, str> is a clunky type for a user-facing API. impl Into<Cow<'static, str>> would be better for the method(s) that the user is meant to call directly. I have a refactor on a branch that makes it possible to accept all different kinds of string types, but I haven't had the time to work on it.

@glebpom
Copy link
Author

glebpom commented Jul 9, 2024

Hi @abonander, ok, this is not a problem, I'll do it.

@glebpom
Copy link
Author

glebpom commented Jul 9, 2024

  • renamed to begin_with
  • use generic for sql (Into<Cow<..>>)
  • AnyConnectionBackend still uses Cow<..> because it should be object-safe.

@glebpom glebpom changed the title feat: add begin_custom to start transaction with custom SQL feat: add begin_with to start transaction with custom SQL Jul 10, 2024
Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides some nits, it's looking good so far. Do you plan on finishing the work in the SQLite driver?

#[doc(hidden)]
pub fn begin_with<S>(
conn: impl Into<MaybePoolConnection<'c, DB>>,
sql: S,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be Cow<'static, str> here since it's not a public API. That will save on codgen.

S: Into<Cow<'static, str>> + Send + 'a,
{
Box::pin(async move {
let rollback = Rollback::new(conn);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should check and return an error if conn.transaction_depth > 0 because it doesn't make sense to allow this to create and release manual savepoints when a transaction is already in progress; that's just going to break things.

S: Into<Cow<'static, str>> + Send + 'a,
{
Box::pin(async move {
let depth = conn.transaction_depth;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here re. checking transaction_depth.

@abonander
Copy link
Collaborator

@glebpom were you going to finish this?

@abonander
Copy link
Collaborator

Closing due to inactivity. Feel free to open a new PR if you come back to this.

@abonander abonander closed this Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants