Add a dry_run flag to /crates/new
#1517
Conversation
The way we handled this before with a `Shim` struct breaks any later downcasting we may try to do, and can lead to errors slipping through in a hard to debug way. It's much easier if we just directly implement `CargoError` on those types (and lets us get rid of some trivial impls). This new impl conflicts with `impl<T: CargoError> CargoError for Box<T>`. It's fine to remove that impl. It means that `Box<ConcreteType>` no longer implements `CargoError`, but we wouldn't have boxed the error in the first place unless we needed `dyn CargoError`.
There is interest from Cargo in having the ability to make `cargo publish --dry-run` hit crates.io to see if there are any checks that we perform which Cargo does not. This looks for a `dry_run` query param in the publish function, and will roll back the database transaction before attempting to upload to S3 or update the index, but after all other checks are complete. The code is fairly straightforward, the only hard part here is actually being able to check why we rolled back the transaction in the first place. We have to re-implement the inherent methods from `Any` here, since we can't call inherent methods from `dyn Any` on `dyn CargoError` even though all `CargoError`s implement `Any`.
There was a problem hiding this comment.
The only thing that concerns me a bit is that even if the transaction is rolled back, my understanding is that id numbers can still be consumed. Even the existing code could use a lot of improvement here.
Ideally, we would check all the static things up front, outside of the transaction. Right now, we render the README inside the transaction as the last thing before checking the dry_run flag! We should probably do this before even obtaining a database connection from the pool.
Here is a rough outline of what I'm thinking:
- Verify all possible crate metadata first, and render the README.
- Obtain a connection from the pool and enter the transaction.
- Check that all dependencies exist and have valid version ranges. Do other read only checks.
- Check user permissions to upload. If this is our last step, then we can reply with something like "everything else looks good, but FYI you don't have upload permission".
- Abort if this is a dry run.
- Anything that does UPDATE/INSERT (I think UPDATES sooner are fine, but defer if practical).
We may also be able to collect a list of errors so that we can report as many issues as possible at once.
| if self.is::<T>() { | ||
| unsafe { | ||
| Some(&*(self as *const Self as *const T)) | ||
| } |
There was a problem hiding this comment.
Our own in-tree unsafe! Matches what is in std so should be fine.
There was a problem hiding this comment.
Hah is this really the first use of unsafe? \o/
|
I really don't think we need to worry about even remotely approaching the limit of 64 bit integers here, but you're definitely right that this will use ids for uncommitted rows. I went with this approach because it was the (mostly) shortest path to what cargo is looking for. I would definitely love to have a larger refactoring on this, but it's not something I personally have time for (maybe @joelgallant would be interested in it?) |
|
I've just opened #1522. I don't see the proposed changes as a hard blocker on the |
|
👍 Closing this then |
Note: Includes #1516
There is interest from Cargo in having the ability to make
cargo publish --dry-runhit crates.io to see if there are any checks that weperform which Cargo does not. This looks for a
dry_runquery param inthe publish function, and will roll back the database transaction before
attempting to upload to S3 or update the index, but after all other
checks are complete.
The code is fairly straightforward, the only hard part here is actually
being able to check why we rolled back the transaction in the first
place. We have to re-implement the inherent methods from
Anyhere,since we can't call inherent methods from
dyn Anyondyn CargoErroreven though all
CargoErrors implementAny.Fixes #1515