Skip to content
This repository was archived by the owner on Jul 17, 2024. It is now read-only.

Upgrade sqlx to 0.7.1 #509

Merged
merged 56 commits into from
Jul 25, 2023
Merged

Upgrade sqlx to 0.7.1 #509

merged 56 commits into from
Jul 25, 2023

Conversation

AndreasHuber
Copy link
Contributor

@AndreasHuber AndreasHuber commented Jul 12, 2023

This PR includes:

  • Upgrade of sqlx
  • Changes to make database-layer and tests compile again

Things to look out for when reviewing:

  • In a few places connection use has been changed to transaction use because using sqlx::Acquire::acquire in fetch leads to higher-ranked lifetime error that with sqlx::Acquire::begin is not there. This should not be a problem for performance but it is important that no transaction commit is forgotten resulting in a change not being saved to the database. The default without commit is rollback. Test cases should catch this but it is possible we have not everything covered by test cases.
    If you wonder how nested transactions work, check https://github.com/launchbadge/sqlx/blob/c70cfaf035b3ffcb1d6f244f6070e92646e83515/sqlx-core/src/transaction.rs#L242C1-L268C2.

Things that could make sense but are excluded here because the PR is already big:

  • Entirely removing the self-made Executor trait
  • Refactoring for execute functions that are not doing much anymore

Related info:

…h to that now that there is no separate Actix-web feature any more
…rently that leads to >100 compilation errors that hopefully disappear when same change applied there as well
@AndreasHuber AndreasHuber changed the title Upgrade sqlx to 0.7.0 Upgrade sqlx to 0.7.1 Jul 18, 2023
@AndreasHuber AndreasHuber force-pushed the upgrade-sqlx-to-0.7.0 branch from 7736c4b to cb21c7b Compare July 18, 2023 13:50
@AndreasHuber AndreasHuber force-pushed the upgrade-sqlx-to-0.7.0 branch from cb21c7b to 6f87e54 Compare July 18, 2023 13:51
@AndreasHuber AndreasHuber marked this pull request as ready for review July 25, 2023 16:16
Copy link
Member

@hugotiburtino hugotiburtino left a comment

Choose a reason for hiding this comment

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

Congrats and thanks a lot. Let's set a new version for the server in this PR in order to deploy it soon.

Comment on lines -53 to -58
Ok(match connection {
Connection::Pool(pool) => fetch(path, instance, pool).await?,
Connection::Transaction(transaction) => {
fetch_via_transaction(path, instance, transaction).await?
}
})
Copy link
Member

Choose a reason for hiding this comment

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

Nice to not have those lines any more :-)

.await?
.map(|result| result.uuid_id as i32)
.ok_or(operation::Error::NotFoundError)?
}
};

let uuid = Uuid::fetch_via_transaction(id, &mut transaction).await?;
let uuid = Uuid::fetch_via_transaction(id, &mut *transaction).await?;

transaction.commit().await?;
Copy link
Member

Choose a reason for hiding this comment

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

doesn't hurt either? Might be also faster since there is no rollback in the mysql...

/// let events = sqlx::query!(r#"SELECT id FROM event_log"#).fetch_all(&mut transaction).await?;
/// let users = sqlx::query!(r#"SELECT id FROM user"#).fetch_all(&mut transaction).await?;
/// let events = sqlx::query!(r#"SELECT id FROM event_log"#).fetch_all(&mut *transaction).await?;
/// let users = sqlx::query!(r#"SELECT id FROM user"#).fetch_all(&mut *transaction).await?;
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this file?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because I have not entirely removed its usage yet, only where it was necessary to make database-layer compile again.

@kulla
Copy link
Member

kulla commented Jul 25, 2023

Congrats and thanks a lot. Let's set a new version for the server in this PR in order to deploy it soon.

@AndreasHuber Can you do this in another PR?

@kulla kulla added this pull request to the merge queue Jul 25, 2023
Merged via the queue into main with commit b7c9bab Jul 25, 2023
@kulla kulla deleted the upgrade-sqlx-to-0.7.0 branch July 25, 2023 21:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants