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

Sqlx 0.7 update #2604

Closed
wants to merge 10 commits into from
Closed

Sqlx 0.7 update #2604

wants to merge 10 commits into from

Conversation

auipga
Copy link
Contributor

@auipga auipga commented Aug 25, 2023

Changelog of sqlx
TL;DR:

Fixes: "use two different sqlx versions?" when using shuttle.rs

  • All tests are passing

I'm new to Rust. This is my first contribution to the Rust community. Please tell me, if you would have done this better. I'm willing to learn.

@SergioBenitez
Copy link
Member

This appears to be doing something similar to what #2597 does. Is there a reason to prefer one or the other? Does #2597 actually subsume the changes in this PR?

@auipga
Copy link
Contributor Author

auipga commented Aug 25, 2023

Oh man, same work done twice 🤦🏻‍♂️ At least I learned something by doing it myself.

The only differences to #2597 that I spotted are options for tls:

To your questions:

  • both PRs will do the job. Though mine has no reformats.
  • yes, plus the added tls fields (I'd think this shouldn't be included in an regular update)

@SergioBenitez
Copy link
Member

I was about to merge (a version of) this when I noticed spurious failures caused by a change I made to the sqlx example:

 #[post("/", data = "<post>")]
-async fn create(mut db: Connection<Db>, post: Json<Post>) -> Result<Created<Json<Post>>> {
-    // There is no support for `RETURNING`.
-    sqlx::query!("INSERT INTO posts (title, text) VALUES (?, ?)", post.title, post.text)
-        .execute(&mut *db)
-        .await?;
+async fn create(mut db: Connection<Db>, mut post: Json<Post>) -> Result<Created<Json<Post>>> {
+    let query = sqlx::query! {
+        "INSERT INTO posts (title, text) VALUES (?, ?) RETURNING id",
+        post.title, post.text
+    };

+    post.id = Some(query.fetch_one(&mut **db).await?.id);
     Ok(Created::new("/").body(post))
 }

This is something I had been wanting the example to do for some time but we lacked an updated version of sqlite, which we now have. Unfortunately, this causes spurious test failures because an insertion is at times not reflected:

DELETE /sqlx:
   >> Matched: (destroy) DELETE /sqlx
   >> Outcome: Success
POST /sqlx application/json:
   >> Matched: (create) POST /sqlx
   >> Outcome: Success
GET /sqlx:
   >> Matched: (list) GET /sqlx
   >> Outcome: Success
POST /sqlx application/json:
   >> Matched: (create) POST /sqlx
   >> Outcome: Success
GET /sqlx:
   >> Matched: (list) GET /sqlx
   >> Outcome: Success
thread 'tests::test_sqlx' panicked at databases/src/tests.rs:38:9:
assertion `left == right` failed
  left: 1
 right: 2

The sqlx version of the example is the only one that fails even though I've updated the other examples to use RETURNING as well. We can't merge this change until we understand why this is happening and get a fix in wherever appropriate. (P.S: I considered whether the defaults for sqlite connections had changed in sqlx, e.g, whether WAL is still default, but it seems they have not.)

I've pushed the was-ready-to-merge sqlx-0.7-update for your investigation.

@SergioBenitez
Copy link
Member

Looks like the issue I outline above is launchbadge/sqlx#2543. I'm rather surprised that this seemingly very serious issue has been completely ignored for over two months, or longer (launchbadge/sqlx#1648).

@SergioBenitez
Copy link
Member

Worked around the issue. Merged in aa7805a. Thank you!

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.

2 participants