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

Hitting 'negative last_insert_rowid' panic with Sqlite #1357

Closed
nitnelave opened this issue Jan 3, 2023 · 3 comments · Fixed by #1403
Closed

Hitting 'negative last_insert_rowid' panic with Sqlite #1357

nitnelave opened this issue Jan 3, 2023 · 3 comments · Fixed by #1403
Milestone

Comments

@nitnelave
Copy link

Description

I have a table with a primary ID that is a hash, stored as a signed integer (since unsigned 64 bit is not supported). Auto-increment is turned off for that table.
When storing a new element, if the hash as signed is negative, then sea-orm panics on that line: https://github.com/SeaQL/sea-orm/blob/master/src/executor/execute.rs#L43

Steps to Reproduce

Expected Behavior

Since auto-increment is turned off for that table, last_insert_id should probably not be called to avoid a panic.

Actual Behavior

Panic.

Reproduces How Often

Always.

Versions

Using sea-orm 0.10.3 (no other version override). Running on alpine, using the bundled Sqlite.

Additional Information

The query, for reference: https://github.com/nitnelave/lldap/blob/c64d32e2c0cc698c61ca40e8364ab0cd7934cd46/server/src/infra/sql_backend_handler.rs#L49

@nitnelave
Copy link
Author

For reference, the Sqlite schema info for the table:

sqlite> pragma table_info(jwt_refresh_storage);
0|refresh_token_hash|INTEGER|1||1
1|user_id|text(255)|1||0
2|expiry_date|TEXT|1||0

@billy1624
Copy link
Member

billy1624 commented Jan 3, 2023

Hey @nitnelave, interesting! I never thought anyone could hit this panic.

You're right, since we have auto increment disabled, the primary key value should be given and invocation of last_insert_id() method can be avoided.

#[allow(unused_variables, unreachable_code)]
async fn exec_insert<A, C>(
primary_key: Option<ValueTuple>,
statement: Statement,
db: &C,
) -> Result<InsertResult<A>, DbErr>
where
C: ConnectionTrait,
A: ActiveModelTrait,
{
type PrimaryKey<A> = <<A as ActiveModelTrait>::Entity as EntityTrait>::PrimaryKey;
type ValueTypeOf<A> = <PrimaryKey<A> as PrimaryKeyTrait>::ValueType;
let last_insert_id_opt = match db.support_returning() {
true => {
let cols = PrimaryKey::<A>::iter()
.map(|col| col.to_string())
.collect::<Vec<_>>();
let res = db.query_one(statement).await?.unwrap();
res.try_get_many("", cols.as_ref()).ok()
}
false => {
let last_insert_id = db.execute(statement).await?.last_insert_id();
ValueTypeOf::<A>::try_from_u64(last_insert_id).ok()
}
};
let last_insert_id = match primary_key {
Some(value_tuple) => FromValueTuple::from_value_tuple(value_tuple),
None => match last_insert_id_opt {
Some(last_insert_id) => last_insert_id,
None => return Err(DbErr::UnpackInsertId),
},
};
Ok(InsertResult { last_insert_id })
}

I'll draft a PR tomorrow morning :)

@nitnelave
Copy link
Author

Small style note: a match for a boolean? Why not a if-else block?
Also, the nested match just below can probably be re-written as a match (last_insert_id, last_insert_id_opt) to reduce nesting.

nitnelave added a commit to nitnelave/sea-orm that referenced this issue Jan 13, 2023
tyt2y3 added a commit that referenced this issue Jan 19, 2023
Don't call last_insert_id if not needed
@billy1624 billy1624 added this to the 0.11.x milestone Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants