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

Support yank a package with a message #9193

Open
1 of 2 tasks
Rustin170506 opened this issue Jul 30, 2024 · 15 comments
Open
1 of 2 tasks

Support yank a package with a message #9193

Rustin170506 opened this issue Jul 30, 2024 · 15 comments
Assignees
Labels
A-backend ⚙️ A-frontend 🐹 C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works

Comments

@Rustin170506
Copy link
Member

Rustin170506 commented Jul 30, 2024

As we proposed in https://rust-lang.github.io/rust-project-goals/2024h2/yank-crates-with-a-reason.html.
We want to allow other owners to yank a package with a message.

The motivation here is that when a crate is updated to address a critical issue — such as a fix for a soundness bug or a security vulnerability—it is beneficial to yank previous versions and prompts users to upgrade with a yank reason. Additionally, if a crate is renamed or deprecated, the yank message can provide guidance on the new recommended crate or version. This ensures that users are aware of necessary updates and can maintain the security and stability of their projects.

So in crates.io, we need to at least support passing a message to the yank API.

I believe we should begin with a simple, length-limited plain text first. But we should also consider the extensibility to allow us to support more structured messages.

  • Add support to the crates.io's yank API
  • Add support on the browser frontend for giving a reason
@Rustin170506 Rustin170506 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-frontend 🐹 A-backend ⚙️ labels Jul 30, 2024
@Turbo87
Copy link
Member

Turbo87 commented Jul 30, 2024

FWIW I'm not sure whether we should add this to the existing yank API (DELETE /api/v1/crates/:crate_id/:version/yank and PUT /api/v1/crates/:crate_id/:version/unyank). I think it might be better to implement something like PATCH /api/v1/crates/:crate_id/:version with yank and yank_reason fields and treat the old API as legacy.

@Rustin170506
Copy link
Member Author

FWIW I'm not sure whether we should add this to the existing yank API (DELETE /api/v1/crates/:crate_id/:version/yank and PUT /api/v1/crates/:crate_id/:version/unyank).

I kind of agree with this because we don't actually delete anything; we just update the yank status.

I think it might be better to implement something like PATCH /api/v1/crates/:crate_id/:version with yank and yank_reason fields and treat the old API as legacy.

I am a little worried about going this way. It means that if the other registries want to follow this API spec, they will need to introduce a new API or method as well.

@Turbo87
Copy link
Member

Turbo87 commented Aug 6, 2024

yes, that would be the consequence, but I don't think fixing the mistakes of the past is necessarily a bad idea in this case.

@Rustin170506
Copy link
Member Author

Rustin170506 commented Aug 15, 2024

Because this API change could potentially introduce a new API to all registries, I want to summarize the current situation and specify the new API we want to add.

Current API

  1. /api/v1/crates/:crate_id/:version/yank: DELETE method without any Body or parameters
  2. /api/v1/crates/:crate_id/:version/unyank: PUT method without any Body or parameters

New API

/api/v1/crates/:crate_id/:version: PATCH method with a Body.

In this new API, we can ask the client to send an HTTP body:

{
  "yanked"?: true,
  "yank_reason"?: "some-reason"
}

Also, in the future, if we want to add more fields or information to provide users with additional details about this version of yanked, we can easily include it in this JSON.

Motivation

The new API is being introduced because the DELETE method typically implies that resources will be removed. However, in this instance, nothing is actually being deleted. Instead, we are simply updating the status of that particular version to yanked.

Same as the 'unyank' API, the 'PUT' method usually indicates that we want to add new resources. However, in this case, we are just changing the state of that particular version to unyanked.

If we can use the PATCH method, it makes more sense because it updates the state of that particular version.

Pros

  • The PATCH method is considered more accurate in terms of HTTP semantics or REST semantics.
    • From the client's perspective, it makes more sense to update the yank message with a new patch rather than deleting it multiple times.
  • It doesn't lead us to the argument of 'whether the HTTP delete method should have a body'.
  • We can only maintain one API instead of two APIs.
  • It's easier to extend this endpoint to support additional updates of the specific version. (Not sure if this will actually happen, as we should try to make sure that the rest of the meta information is the same as it was at the time of the release.)

Cons

  • This will introduce a new API and we still need to support the old APIs forever.
  • This would require other registries to adhere to this specification if we want to designate this API as a standard API.

@Eh2406
Copy link
Contributor

Eh2406 commented Aug 15, 2024

What is the plan for other "patch like" changes to a crate version? For example editing the "description" or "maintenance"? I only ask because the same argument would suggest that they also are /api/v1/crates/:crate_id/:version: PATCH

  • If there using the same API endpoint, what's the plan for this ambiguity in what is being requested?
  • Alternatively if they're using a different endpoint, then why does yank get the primal API?

Separately, how should cargo determine when to use the new endpoint? This needs to be made explicit because it also applies to all other registries.

@Rustin170506
Copy link
Member Author

What is the plan for other "patch like" changes to a crate version? For example editing the "description" or "maintenance"? I only ask because the same argument would suggest that they also are /api/v1/crates/:crate_id/:version: PATCH

I think we should try not to introduce such a feature, because we should try to make sure that its meta information is consistent with the information in Cargo.toml. For yank it's not in Cargo.toml, so it's more like extra information in the database.

If we do want to update this information in the future, I don't think it affects the semantics of our API, you can use an API to update any information about this version(crate).

@Rustin170506
Copy link
Member Author

Separately, how should cargo determine when to use the new endpoint? This needs to be made explicit because it also applies to all other registries.

This is a really good question, I guess we can use https://doc.rust-lang.org/cargo/reference/registry-index.html#index-configuration to set it explicitly.

@epage
Copy link

epage commented Aug 15, 2024

I think we should try not to introduce such a feature, because we should try to make sure that its meta information is consistent with the information in Cargo.toml. For yank it's not in Cargo.toml, so it's more like extra information in the database.

There is a strong need for mutable metadata in addition to yank. Interest in exploring that is one of the reasons we had pushed back on a yank-specific solution for the entire stack of this to cargo and wanted to hold off to see how things evolved.

Besides that, making a generic end point only work with one specific purpose is very limiting. We don't think we need it at this point but we need to specify how we expect to evolve this end point.

Also, the specification for a new API should also specify when a client should know that the registry supports the new API.

Overall, I wonder if we should go through an RFC for this to make sure we are gettting wider input on the process of evolving the registry API, at least we need to make sure we get alternative registries involved.

@Turbo87
Copy link
Member

Turbo87 commented Aug 16, 2024

What is the plan for other "patch like" changes to a crate version? For example editing the "description" or "maintenance"?

I agree with @Rustin170506 that anything parsed from the Cargo.toml file will likely stay read-only for versions. Something like "maintenance" however could be considered, however I expect this would likely be per-crate instead of per-version.

  • If they're using the same API endpoint, what's the plan for this ambiguity in what is being requested?

the API would be idempotent. if the requested value change is already in the database then nothing happens, if an update is necessary then that "field" of the version will be updated accordingly. you can either only change the yank field, or change another field (tbd), or change both at the same, etc.

how should cargo determine when to use the new endpoint? This needs to be made explicit because it also applies to all other registries.

my understanding is that cargo has special cases for crates.io, so for our specific registry this shouldn't be a problem. how to discover this for other registries I don't know. we may end up having to build a full registry capabilities system or go with @Rustin170506's suggestion of adding this to the config.json file, but IMHO that is out of scope for adding this functionality to crates.io.

Besides that, making a generic end point only work with one specific purpose is very limiting. We don't think we need it at this point but we need to specify how we expect to evolve this end point.

I 💯 agree, and that is precisely why I suggested the more generic PATCH endpoint instead of piling more stuff onto the old yank API. this endpoint is supposed to be usable for all writeable fields of a version, even though this is currently only limited to yank and would then also support yank_reason in the future.

the specification for a new API should also specify when a client should know that the registry supports the new API

IMHO that depends on whether we make this API part of the registry contract or not. for experimentation purposes on the crates.io API and the crates.io frontend we don't need to specify these things yet. these will only become relevant once cargo wants to interact with it, and even then it is my understanding that cargo already special-cases crates.io for various reasons?

I wonder if we should go through an RFC for this to make sure we are gettting wider input on the process of evolving the registry API, at least we need to make sure we get alternative registries involved.

I agree that a change to the registry API contract should probably go through the RFC process, but I don't think that is necessarily blocking experimentation on our side and building the functionality into our frontend. in fact, such experimentation would probably improve the RFC since it isn't written in a vacuum and can benefit from first-hand experience with a suggested solution.

@epage
Copy link

epage commented Aug 16, 2024

What is the plan for other "patch like" changes to a crate version? For example editing the "description" or "maintenance"?

I agree with @Rustin170506 that anything parsed from the Cargo.toml file will likely stay read-only for versions. Something like "maintenance" however could be considered, however I expect this would likely be per-crate instead of per-version.

Going back to https://blog.rust-lang.org/inside-rust/2024/03/26/this-development-cycle-in-cargo-1.78.html#why-is-this-yanked, we list out

  • Unmaintained status (overlaps with rustsec)
  • Deprecation status (crates.io#7146), especially if you can point to a replacement (like rustsec's "unmaintained"), e.g. helping structopt users discover that their upgrade path is switching to clap, similar for rlua to mlua
  • Prepare for broken builds due to bug-compatibility hacks being removed (rust#106060)
  • Maybe even allow third-party registries to distribute rules for dependency resolution hooks

We at least have "broken builds" as a per-version use case. rustsec security advisories is not on the list but that would also be per-version (unsure if we'd want to pull from a single source which might be better for third-party registries or multiple sources).

For today's workflows, unmaintained and deprecated are per-crate but I think there is value in exploring supporting per-version. Example: Today, some people treat "non-latest" as unmaintained / deprecated which was a point of content when discussing an MSRV-aware resolver. With an MSRV-aware resolver, I'm considering maintaining LTS releases where it makes sense to bump a minor version's MSRV above my project's overall MSRV. It would be a big help to explicitly state the maintenance status to the user, rather than assuming. For clap, I already have LTS support (only on major versions) but I only have documentation to report that to my users and Cargo can only treat it like any other stale version.

@epage
Copy link

epage commented Aug 16, 2024

my understanding is that cargo has special cases for crates.io, so for our specific registry this shouldn't be a problem. how to discover this for other registries I don't know. we may end up having to build a full registry capabilities system or go with @Rustin170506's suggestion of adding this to the config.json file, but IMHO that is out of scope for adding this functionality to crates.io.

So the question is whether the Cargo team is ok with special casing our yanked calls for crates.io until we have third-party registry support for this API.

For myself and on my initial pass, I'm against this

  • We'd be stabilizing an API that hasn't been fully vetted
  • This greatly lowers the incentive for supporting this for third-party registries and makes them a second class citizen in more ways than "users have to opt-in to them"

@Rustin170506
Copy link
Member Author

Rustin170506 commented Aug 17, 2024

During yesterday's meeting regarding crates.io, we discussed the issue briefly. The crates.io team is still considering conducting some experimentation on the crates.io platform first. We will only allow cookie authentication to access this API. This precaution will help us prevent exposing this new API to other clients (who access crates.io with a token), in order to avoid dependency and usage.

So the question is whether the Cargo team is ok with special casing our yanked calls for crates.io until we have third-party registry support for this API.

I don't think we will do it. After we implemented it and tested it on crates.io. I will file an RFC to discuss how it interacts with Cargo and what the final API looks like.

if an update is necessary then that "field" of the version will be updated accordingly. you can either only change the yank field, or change another field (tbd), or change both at the same, etc.

As @Turbo87 mentioned, this API can be used not only for the yank message feature but also to update other statuses. (if we need/have, right now we only have yanked status and yank message).
For the `PATCH' method, we definitely have the ability to update more fields. But discussing what status we can introduce to it is off the topic. Let's just focus on the Yank message, and we are not going to limit this API only to the Yank message.

@Rustin170506
Copy link
Member Author

@Turbo87 For "Add support on the browser frontend for giving a reason," do you have any thoughts on the UI/UX for this? My current idea is to have a modal where users can enter the yank message after clicking the yank button. What do you think? Any suggestions?

@Turbo87
Copy link
Member

Turbo87 commented Nov 20, 2024

I'm personally not a big fan of modal dialogs since they are very tricky to implement correctly, especially from an accessibility point of view. I've worked on https://github.com/mainmatter/ember-promise-modals in the past, and I'm still not 100% sure if we're doing everything correctly there 😅

My first thought was to use a new full-page route for the message input and the confirmation button, but I'm not sure how well that would integrate into the existing page structure, so on second thought I'm not so sure anymore about that plan 😂

in other words: I'm open to suggestions :)

@eth3lbert
Copy link
Contributor

that's also the reason why the crate report and the support page were ultimately implemented as new pages 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ A-frontend 🐹 C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
Archived in project
Development

No branches or pull requests

5 participants