Add minimum version tracking to deleted crates #10016
Conversation
This calculates the next semver-incompatible version of the crate being deleted and inserts it as `deleted_crates.min_version`.
LawnGnome
added
C-enhancement ✨
| macro_rules! assert_result_failed { | ||
| ($result:expr) => {{ | ||
| let text = assert_result_status!($result); | ||
| assert_snapshot!(text); |
There was a problem hiding this comment.
I decided to write the snapshots to files because long lines make my eye twitch, but I don't feel very strongly about this.
| let response = $result.unwrap_err().response(); | ||
| assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY); | ||
|
|
||
| String::from_utf8( | ||
| axum::body::to_bytes(response.into_body(), usize::MAX) | ||
| .await | ||
| .unwrap() | ||
| .into(), | ||
| ) | ||
| .unwrap() | ||
| }}; |
There was a problem hiding this comment.
I thought about extending tests::util to be able to use the same response helpers on a bare axum::response::Response without the full boilerplate to fake a full request, but decided to keep this more self-contained for now. I can bring that code back if that sounds useful, though.
| "A crate with the name `{name}` was previously deleted.\n\n* {}", | ||
| messages.join("\n* "), |
There was a problem hiding this comment.
This is basically the shitty version of our conversation about representing multiple errors in a single response in #9904. 💩
|
I guess now comes the question: is it worth it? 😂 one potential issue with this I discovered yesterday: when someone name-squats as mentioned in #9912 (comment), I'm curious what the situation at other registries looks like regarding this situation, for user-issued deletions, but also when admins delete packages/versions for whatever reason. |
If a crate has been deleted before, `deleted_crates.min_version` is enforced when validating a crate version that is being published. This also refactors the deleted crate check in general into a new module to make it easier to unit test and reason about. Mostly the latter, honestly.
LawnGnome
force-pushed
the
deleted-crates-min-version
branch
from
9c7f396 to
edbf30a
Compare
All right, all right, I'll go actually ask some people. grumbles about this being as bad as having to phone someone on an actual phone |
So, yeah, that's an issue. On the other hand, there's also kind of an issue here with the "specifically allow semver-incompatible versions" rule as well, which is that it's potentially hard to explain. If someone publishes, say: Of course, a crate having that set of versions in the first place might be a sign that the name is associated with someone with... unusual ideas on versioning. 😆 |
|
☔ The latest upstream changes (presumably e04e4e2) made this pull request unmergeable. Please resolve the merge conflicts. |
Here are two very specific examples. I think if we allow version re-release, we will encounter these two issues. However, if we don't allow re-releasing the same version, perhaps the two issues will merge into one: it will just become "not found." |
|
IMHO these seem like bugs on the cargo side, though admittedly they are caused by the assumption mismatch that versions/crates will just never ever disappear from crates.io, which for various reasons we cannot guarantee. but again, with the set of requirements that we enforce for people to be able to delete crates themselves, it seems unlikely for people to regularly hit these sorts of issues. I'm not saying it will never happen, but I don't see it as being a very likely scenario for most people. |
Yes, I'm not suggesting that crates.io completely fix these two issues. I'm just saying that, as the default registry for Cargo, we have an opportunity to reduce the likelihood of these situations occurring. Because, as you can see, fixing mismatches requires significant effort from Cargo (e.g., modifying the fingerprints). So I'm wondering if we could reduce the chances of this happening in the default registry, wouldn't that minimize the potential impact on users as much as possible? |
yeah, we could. but ultimately it's a tradeoff that needs to be balanced with all of the other arguments for and against this that were brought up in the comments of this PR (and the others). I'm personally not convinced yet that it's worth the extra complexity for our (publishing) users, but ultimately the crates.io team needs to decide this. |
I see this is already on the agenda for Friday, so I guess we can defer this until then. |
|
OK, so we discussed this at the team meeting, and I think I'm convinced enough that the restrictions on crate deletion in the RFC are sufficient to make the risk low enough that we can just scan for this ex post facto, thereby sidestepping awkward questions around how we communicate this to users. Closing. |
to expand and summarize a bit more on this for those that were not involved in that meeting:
Due to these factors above the crates.io team concluded that we will not implement a hard block on republishing previously existing versions of crates that were deleted, and will rely on the out-of-band detection by the Rust Foundation and other third parties instead. Note that this is not a permanent decision, and if we see that these rules are insufficient we can adjust them accordingly. |
As discussed in #9904 and #9912, this PR:
deleted_cratestable that indicates the first version that can be used when publishing a new crate version with the same name.crates-admin delete-crateis run.Implementation wise, most of the noise here is actually moving the
deleted_cratespublish checks into a sub-module, mostly because the original version I wrote inline in the controller was ugly as hell. Instead, we now get to enjoy a slightly overengineered state machine. I'm open to suggestions.(Although, on the bright side, if we ever do need the more complex semver compatibility check rather than just checking against a minimum version, having the validation logic separated out already should make that easier to implement.)
I added new integration tests for the controller, but most of the specifics are tested in unit tests in said new sub-module. Redundant? Probably. 🤷
As for the deletion side of things, we're going to need the same
min_versionpopulation behaviour (specifically the use ofTopVersions) that I added tocrates-adminwhen #9904 lands, but I was struggling a bit to figure out how to integrate that intomodels::krate, sincecrates-adminnever actually constructs amodels::krate::Cratewhen deleting a crate. I played around with doing a partial build ofNewDeletedCrateBuilderbased on aCrate, but the type signatures were driving me slightly bonkers, and it didn't really feel cleaner anyway, so I just did it inline for now.Honestly, it might be simple enough that it's easiest to just copy/paste/adapt the same general logic into #9904.
I have a couple of other, more specific open questions that I'll throw into the inline review after I publish this, but I guess the main question here is whether this feels directionally correct.