Expose last_insert_id from Ok packet on MySQL #4271
Conversation
mcronce
force-pushed
the
mysql-last-insert-id
branch
from
c196e7e
to
6a2277e
Compare
mcronce
force-pushed
the
mysql-last-insert-id
branch
from
6a2277e
to
c6639ec
Compare
|
I don't think that that test failure has anything to do with my changes? |
There was a problem hiding this comment.
Thanks for working on this. I've added a few comments around the Safety remarks of the new unsafe function. I think we do need additional restrictions/checks there to make sure the result is defined.
As for the implementation: I personally think we don't need to introduce these two new traits there, as we could just implement the relevant methods directly on InsertStatement and MysqlConnection. We don't need to be super generic there as these methods are only relevant for this combination. (That's nothing I would block the merge on, although I would be happy to see that resolved)
| // we have not called result yet, so calling `execute` is | ||
| // fine | ||
| let stmt_use = unsafe { stmt.execute() }?; | ||
| Ok(unsafe { stmt_use.insert_id() }) |
There was a problem hiding this comment.
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.
| @@ -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()) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Another way to quickly get this feature is expose it under some unstable feature flag, if you not against that.
There was a problem hiding this comment.
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.
| DB: Backend = <Conn as Connection>::Backend, | ||
| >: Sized | ||
| { | ||
| /// Execute this command |
There was a problem hiding this comment.
There should be a doc test/example on this function to show how this works
Took a whack at implementing what we discussed in #3605 (finally); I'm sure I need to add some documentation at minimum.
Notably, I copied most of the body of
execute_returning_id()fromexecute_returning_count(), not sure if that's how you want to handle that or notMight have overabstracted; I implemented it using traits for extensibility.
I'll have a follow-up in
diesel_asynconce this is merged.