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.

Sign up for GitHub

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

Jump to bottom

Expose last_insert_id from Ok packet on MySQL #4271

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+77 −1
Open

Expose last_insert_id from Ok packet on MySQL #4271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6062345
diesel/src/query_dsl/load_dsl.rs: + ExecuteInsertWithReturningId
mcronce Aug 16, 2024
a907165
diesel/src/connection/mod.rs: + ConnectionWithReturningId
mcronce Aug 16, 2024
edb451d
diesel/src/mysql/connection/stmt/mod.rs: + StatementUse::insert_id()
mcronce Aug 16, 2024
c6639ec
diesel/src/mysql/connection/mod.rs: impl ConnectionWithReturningId f…
mcronce Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions diesel/src/connection/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,19 @@ pub trait BoxableConnection<DB: Backend>: SimpleConnection + std::any::Any {
fn as_any_mut(&mut self) -> &mut dyn std::any::Any;
}

/// An extension of the [`Connection`](trait.Connection.html) trait that provides the value
/// assigned to an inserted row with an auto-incremented ID column
pub trait ConnectionWithReturningId: Connection {
/// The type that the connection implementation returns for IDs
type ReturnedId;

/// Execute a single INSERT statement given by a query and return the ID that the database
/// assigns
fn execute_returning_id<T>(&mut self, source: &T) -> QueryResult<Self::ReturnedId>
where
T: QueryFragment<Self::Backend> + QueryId;
}

impl<C> BoxableConnection<C::Backend> for C
where
C: Connection + std::any::Any,
Expand Down
28 changes: 28 additions & 0 deletions diesel/src/mysql/connection/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,34 @@ impl Connection for MysqlConnection {
}
}

impl ConnectionWithReturningId for MysqlConnection {
type ReturnedId = u64;

fn execute_returning_id<T>(&mut self, source: &T) -> QueryResult<Self::ReturnedId>
where
T: QueryFragment<Self::Backend> + QueryId,
{
#[allow(unsafe_code)] // call to unsafe function
update_transaction_manager_status(
prepared_query(
&source,
&mut self.statement_cache,
&mut self.raw_connection,
&mut *self.instrumentation,
)
.and_then(|stmt| {
// we have not called result yet, so calling `execute` is
// fine
let stmt_use = unsafe { stmt.execute() }?;
Ok(unsafe { stmt_use.insert_id() })
Copy link
Member

Choose a reason for hiding this comment

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

There should be a Safety: explain how the invariants of the called function are uphold comment here.

Additionally as far as I can see the insert_id() function states that it is only safe to call this function on insert statements, while we can call it on arbitrary statements here. That sounds like the current implementation is unsound.

In the end we want to replace the T: QueryFragement<Self::Backend> bound above with essentially InsertStatement to restrict the allowed statements to only insert statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

}),
&mut self.transaction_state,
&mut self.instrumentation,
&crate::debug_query(source),
)
}
}

#[inline(always)]
fn update_transaction_manager_status<T>(
query_result: QueryResult<T>,
Expand Down
5 changes: 5 additions & 0 deletions diesel/src/mysql/connection/stmt/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ impl<'a> StatementUse<'a> {
.map_err(|e| Error::DeserializationError(Box::new(e)))
}

/// SAFETY: This should only be called for INSERT queries.
pub(in crate::mysql::connection) unsafe fn insert_id(&self) -> u64 {
ffi::mysql_stmt_insert_id(self.inner.stmt.as_ptr())
Copy link
Member

Choose a reason for hiding this comment

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

The mysql documentation states that:

Return value is undefined if statement does not set AUTO_INCREMENT value.

https://dev.mysql.com/doc/c-api/8.0/en/mysql-stmt-insert-id.html

Given that it is possible to have insert statements that do not have a AUTO_INCREMENT value I cannot see how the safety comment above is sufficient. We do need some sort of additional checks here to ensure that the return value is defined, but I'm not sure what would be a sufficient check for that.

Copy link
Contributor Author

@mcronce mcronce Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

While that's how the documentation is written, I don't think that the result is UB. The value is always present in the Ok packet and will always be a valid byte sequence for a u64. The value absolutely might not make any sense if no AUTO_INCREMENT column is present, but that's not the same as the "spooky action at a difference" type of Undefined Behavior.

Looking at what mysql_common does, for example, the only check it performs is during packet deserialization; last_insert_id is an Option<u64> in their representation, and if last_insert_id on the wire is 0, they consider that None

I also, FWIW, don't have any idea what kind of check we'd be able to perform in a query context for that beyond possibly matching that 0 check

Copy link
Member

Choose a reason for hiding this comment

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

I'm still wary to relay on something that's explicitly documented as not being defined as that could change at any point in the future. Remember that we want to provide a reasonable level of stability that should be decoupled from our dependencies. So yes, it might not be undefined behavior in terms of programming to read this value, the result still stays undefined and might change at any point. It's something that at least I do not feel comfortable with exposing in a stable API.

What we can easily do is to restrict it to insert statements with Integer primary keys at compile time by adding the relevant trait bounds on the target type for InsertStatement. What's harder to do is to check if the column is actually marked as AUTO_INCREMENT, as that would additional changes to include this information in the generated schema. That's still not impossible, although I think it might be to much trouble in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to quickly get this feature is expose it under some unstable feature flag, if you not against that.

Copy link
Member

Choose a reason for hiding this comment

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

I would rather prefer restricting it at least to Integer primary keys, as that isn't much harder than marking it as unstable feature. After all that should just require one or two lines of trait bounds on the relevant method.

}

/// This function should be called after `execute` only
/// otherwise it's not guaranteed to return a valid result
pub(in crate::mysql::connection) unsafe fn result_size(&mut self) -> QueryResult<usize> {
Expand Down
32 changes: 31 additions & 1 deletion diesel/src/query_dsl/load_dsl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use self::private::LoadIter;
use super::RunQueryDsl;
use crate::backend::Backend;
use crate::connection::{Connection, DefaultLoadingMode, LoadConnection};
use crate::connection::{
Connection, ConnectionWithReturningId, DefaultLoadingMode, LoadConnection,
};
use crate::deserialize::FromSqlRow;
use crate::expression::QueryMetadata;
use crate::query_builder::{AsQuery, QueryFragment, QueryId};
Expand Down Expand Up @@ -91,6 +93,34 @@ where
}
}

/// The `execute_with_returning_id` method
///
/// This is currently only implemented for [`MySQL`], because MySQL provides a
/// `last_insert_id` field as part of its response packet to `INSERT` queries, but
/// it's in a trait to avoid a backwards-incompatible change in the future if such
/// functionality becomes available in other back-ends.
///
/// [`MySQL`]: crate::mysql::Mysql
pub trait ExecuteInsertWithReturningId<
Conn: ConnectionWithReturningId<Backend = DB>,
DB: Backend = <Conn as Connection>::Backend,
>: Sized
{
/// Execute this command
Copy link
Member

Choose a reason for hiding this comment

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

There should be a doc test/example on this function to show how this works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

fn execute_with_returning_id(query: Self, conn: &mut Conn) -> QueryResult<Conn::ReturnedId>;
}

impl<Conn, DB, T> ExecuteInsertWithReturningId<Conn, DB> for T
where
Conn: ConnectionWithReturningId<Backend = DB>,
DB: Backend,
T: QueryFragment<DB> + QueryId,
{
fn execute_with_returning_id(query: Self, conn: &mut Conn) -> QueryResult<Conn::ReturnedId> {
conn.execute_returning_id(&query)
}
}

// These types and traits are not part of the public API.
//
// * CompatibleType as we consider this as "sealed" trait. It shouldn't
Expand Down
Loading