Port to diesel 2.0.3

[jtgeibel]

With the latest upstream diesel changes in the 2.0.3 release, our final failing test is now passing!

Original PR Text

This is a nearly complete port to diesel 2, with the following exceptions:

• It relies on git branches of diesel_full_text_search (an upstream PR) and swirl (a development branch here).
• Due to the upstream change from &PgConnection to &mut PgConnection, it seems to no longer be possible to run the test suite with a single connection, as required by our background job tests. The current design uses a re-entrant mutex , but this no longer works and unless I'm missing something, there is (for good reason) no safe API to hand out multiple &mut to the same location even if we're on the same thread. Maybe we can rework the swirl backend to add support for our test harness.
• A temporary workaround to the previous issue is in jtgeibel/crates.io@diesel2-crimes which switches to a regular mutex which fails upon the second attempt to take the lock. This allows the branch to build and most tests pass. There are currently 60 test failures, though most of these are due this hack.

Any suggestions on how to resolve this are greatly appreciated!

[weiznich]

To just leave a comment here: Feel free to reach out to me for anything diesel related. I'm happy to provide any additional information that would be required for this update (or any other feature).
Additionally you might want to wait till the final 2.0 release as the RC contains an issue around r2d2 pools: diesel-rs/diesel#3156

That would also allow us to release an updated diesel_full_text_search at that point.

[bors]

☔ The latest upstream changes (presumably 1efe1d5) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably 3d71217) made this pull request unmergeable. Please resolve the merge conflicts.

[weiznich]

@jtgeibel Is there anything I can help with here?

[jtgeibel]

Thanks @weiznich. I've finally been able to make some good progress with updating the background job logic to reuse an existing database connection in a85f62c. I'm a little unsure about the AssertUnwindSafe<&mut PgConnection> and if this could possibly leave the connection in a bad state during an unwind. Though I've tweaked an existing test to now panic and it seems to work as expected. Your review of that commit would be much appreciated.

Other than that, I think this is finally getting close to moving out of draft. There are just a few remaining test failures that I haven't had a chance to look into yet. And we'll need a better long-term fix for cb19137. (cc @Turbo87 the new token scopes / auth is causing a re-entrant access to the test connection in the publish endpoint. For now I just drop the connection before auth and obtain a new one after.)

[weiznich]

👍 I will have a look at a85f62c next week

[bors]

☔ The latest upstream changes (presumably 5ceed51) made this pull request unmergeable. Please resolve the merge conflicts.

[weiznich]

I left a comment on the implications of AssertUnwindSafe on the corresponding commit: a85f62c#r95034771

I'm happy to help with further input resolving this issue.

[jtgeibel]

Thanks @weiznich! I've decided to break off this part of the change into #5837 and have incorporated your suggestions (backported to diesel 1.0) in 2e3637d.

[jtgeibel]

I've rebased and squashed this PR on top of latest master. There are 5 failing unhealthy_database tests. I haven't looked into those yet. I also plan to look at improving the connection rollback logic now that diesel2 has built-in safeguards. Otherwise, this is ready for review.

[weiznich]

I also plan to look at improving the connection rollback logic now that diesel2 has built-in safeguards.

The rollback code is here. The main change there is that we now handle cases where we cannot rollback to safe points and have a much stricter tracking of the transaction depth. In addition we've introduced additional checks to see whether a transaction manager contains open transactions or not. These information is used by the r2d2 implementation to mark any connection with open transactions as broken. That results in the pool discarding these connections and opening a new one instead. In addition any connection returned during a panic is marked as broken

[jtgeibel]

I've pushed a commit simplifying the background worker rollback logic and another commit with a few cleanups I found while reviewing the diff. There is just one failing test.

It seems there is a subtle difference in how transaction rollback works when the commit fails due to a ReadOnlyTransaction error. The or_else() fallback path in this logic now also fails, as shown by inserting a few dbg!s:

[src/models/token.rs:83] update(tokens).set(last_used_at.eq(now.nullable())).get_result(conn) = Err(
    DatabaseError(
        ReadOnlyTransaction,
        "cannot execute UPDATE in a read-only transaction",
    ),
)
[src/models/token.rs:87] tokens.first(conn) = Err(
    DatabaseError(
        Unknown,
        "current transaction is aborted, commands ignored until end of transaction block",
    ),
)

This appears to only be an issue with the test, as when I ran in readonly mode locally cargo gave me the following expected error when I attempted to yank a crate:

Caused by:
  the remote server responded with an error (status 503 Service Unavailable): Crates.io is currently in read-only mode for maintenance. Please try again later.

In production, this code runs outside of any transactions, so the transaction used to attempt the update is a real transaction, however when testing we are already in a transaction, so the update is attempted within a SAVEPOINT. In the test we also create our own SAVEPOINT which diesel isn't aware of..

I did some testing in psql and I was only able to replicate the current transaction is aborted, commands ignored until end of transaction block error if I attempted the SELECT within the (failing) savepoint. After a ROLLBACK TO SAVEPOINT the SELECT returned results as expected. So I'm thinking maybe diesel isn't rolling back the savepoint in this scenario, but I'm not sure exactly what is going on.

[bors]

☔ The latest upstream changes (presumably #5903) made this pull request unmergeable. Please resolve the merge conflicts.

[jtgeibel]

I've opened diesel-rs/diesel#3470 to track the remaining test failure, as I believe this is an unintended change in upstream behavior.

[Ten0]

diesel-rs/diesel#3470 (comment)

[bors]

☔ The latest upstream changes (presumably bac756e) made this pull request unmergeable. Please resolve the merge conflicts.

[jtgeibel]

With the latest upstream diesel changes in the 2.0.3 release, our final failing test is now passing! Moving out of the draft state. 🎉

[jtgeibel]

Last week I made another pass over this PR and realized that at some point I downgraded back to deisel_cli 1 so this was missing changes to the schema.rs file. That is now fixed and I've regenerated the schema.patch file so that it applies cleanly.

Included in the schema.patch file, we now make a few additional tweaks to the generated schema. Array<Nullable<_>> is changed to Array<_> in api_tokens.crate_scopes, api_tokens.endpoint_scopes, dependencies.features. I've confirmed there there are no existing null values in any of these arrays in the production database.

I've rebased this on master so that I can deploy this on staging for some additional testing. We should be able to merge this in a day or two.

[jtgeibel]

Merged and deployed!