-
Notifications
You must be signed in to change notification settings - Fork 678
controllers/krate/publish: Migrate to diesel-async queries
#10041
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10041 +/- ##
==========================================
- Coverage 89.44% 88.94% -0.51%
==========================================
Files 295 295
Lines 31475 31477 +2
==========================================
- Hits 28154 27997 -157
- Misses 3321 3480 +159 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
eth3lbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move back to use diesel::prelude::* and flatten
crates.io/src/controllers/krate/publish.rs
Lines 86 to 124 in 2bad84c
| let (existing_crate, auth) = { | |
| use diesel_async::RunQueryDsl; | |
| let deleted_crate: Option<(String, DateTime<Utc>)> = deleted_crates::table | |
| .filter(canon_crate_name(deleted_crates::name).eq(canon_crate_name(&metadata.name))) | |
| .filter(deleted_crates::available_at.gt(Utc::now())) | |
| .select((deleted_crates::name, deleted_crates::available_at)) | |
| .first(&mut conn) | |
| .await | |
| .optional()?; | |
| if let Some(deleted_crate) = deleted_crate { | |
| return Err(bad_request(format!( | |
| "A crate with the name `{}` was recently deleted. Reuse of this name will be available after {}.", | |
| deleted_crate.0, | |
| deleted_crate.1.to_rfc3339_opts(SecondsFormat::Secs, true) | |
| ))); | |
| } | |
| // this query should only be used for the endpoint scope calculation | |
| // since a race condition there would only cause `publish-new` instead of | |
| // `publish-update` to be used. | |
| let existing_crate: Option<Crate> = Crate::by_name(&metadata.name) | |
| .first::<Crate>(&mut conn) | |
| .await | |
| .optional()?; | |
| let endpoint_scope = match existing_crate { | |
| Some(_) => EndpointScope::PublishUpdate, | |
| None => EndpointScope::PublishNew, | |
| }; | |
| let auth = AuthCheck::default() | |
| .with_endpoint_scope(endpoint_scope) | |
| .for_crate(&metadata.name) | |
| .check(&req, &mut conn) | |
| .await?; | |
| (existing_crate, auth) | |
| }; |
diesel-async.
2bad84c to
f0d35ab
Compare
finally...
now, unfortunately we still have a
spawn_blocking()call in the endpoint since I didn't feel comfortable with processing the tarball on the async worker thread, but at least we're not using theAsyncConnectionWrapperwrapper anymore 🎉This will allow us to throw away a bunch of duplicated code in some of the follow-up PRs :)