Add PATCH /crates/:crate/:version route

[Rustin170506]

ref #9193

This pull request introduces a new patch API for yanking and unyanking crates with a message.

The logic is very similar to the current yank and unyank API, but it supports setting a yank message.

[codecov]

Codecov Report

Attention: Patch coverage is 97.94872% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.14%. Comparing base (3678583) to head (3d5b4f8).
Report is 35 commits behind head on main.

Files with missing lines	Patch %	Lines
src/controllers/version/metadata.rs	96.07%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9423      +/-   ##
==========================================
+ Coverage   89.06%   89.14%   +0.08%     
==========================================
  Files         285      286       +1     
  Lines       28941    29195     +254     
==========================================
+ Hits        25777    26027     +250     
- Misses       3164     3168       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Rustin170506]

🔢 Self-check (PR reviewed by myself and ready for feedback.)

[eth3lbert]

I've left some suggestions before a full review.

[eth3lbert]

If I understand correctly, the current implementation will not trigger a rate limit with invalid data. However, I believe we should check for a rate limit first, as data validation can (and should?) also be performed on the frontend before making a request. This can also prevent potential malicious requests.

[Rustin170506]

/// Changes `yanked` flag on a crate version record
async fn modify_yank(
    crate_name: String,
    version: String,
    state: AppState,
    req: Parts,
    yanked: bool,
) -> AppResult<Response> {
    // FIXME: Should reject bad requests before authentication, but can't due to
    // lifetime issues with `req`.

    if semver::Version::parse(&version).is_err() {
        return Err(version_not_found(&crate_name, &version));
    }

    let conn = state.db_write().await?;
    spawn_blocking(move || {
        let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into();

        let auth = AuthCheck::default()
            .with_endpoint_scope(EndpointScope::Yank)
            .for_crate(&crate_name)
            .check(&req, conn)?;

        state
            .rate_limiter
            .check_rate_limit(auth.user_id(), LimitedAction::YankUnyank, conn)?;

From the old Yank implementation, it seems we check the parse error(validation) first. Also as you can see from the comment, it seems we want to reject the bad requests before authentication.

[eth3lbert]

Regarding the version part, I think both options are acceptable, as an invalid version string will result in a 404 page.
However, for invalid payload, as I previously mentioned, I recommend consuming the rate limit token.

[eth3lbert]

I just noticed that this function is used in two places, I'm wondering if it would be better to have something similar to the following:

enum YankAction {
    UpdateWithReason {
        yanked: Option<bool>,
        yank_message: Option<String>,
    },
    Toggle {
        yanked: bool,
    },
}

impl YankAction {
    pub fn from_update(data: &VersionUpdate) -> Self {
        ...
    }

    fn validate_with_version(&self, version: &Version) -> AppResult<()> {
        ...
    }

    fn apply(
        &self,
        state: &AppState,
        req: &Parts,
        conn: &mut impl Conn,
        version: &mut Version,
        krate: &Crate,
    ) -> AppResult<()> {
      ...
    }
}

I'm not sure if this would overcomplicate things, but IMHO, with this approach, we could distinguish the operations more easily, making it easier to follow and debug. Additionally, it would provide the potential opportunity to optimize each case individually while still sharing most of the functionality.

(Note: this is just a brief example. The naming, methods, and their arguments can be further modified as needed.)

[Rustin170506]

However, for invalid payload, as I previously mentioned, I recommend consuming the rate limit token.

From our current API design, it seems we prefer do it before the auth check.

crates.io/src/controllers/krate/publish.rs

Line 62 in 5f8fbb9

let metadata: PublishMetadata = serde_json::from_slice(&json_bytes)

[Rustin170506]

I'm not sure if this would overcomplicate things, but IMHO, with this approach, we could distinguish the operations more easily, making it easier to follow and debug.

I just simplified the API design to make all updates more obvious. Directly pass the 'yanked' and 'yank_message' to the function, instead of implicitly depending on the updated behavior of the 'version' parameter. What do you think?

[Turbo87]

I've rebased this on top of #9437 to get that first part out of the way and make the PR a bit easier to review :)

[Turbo87]

we should probably make the request and response JSON consistent by including the version wrapper. something like this:

Suggested change

	#[derive(Deserialize)]
	pub struct VersionUpdate {
	yanked: Option<bool>,
	yank_message: Option<String>,
	}
	#[derive(Deserialize)]
	pub struct VersionUpdate {
	yanked: Option<bool>,
	yank_message: Option<String>,
	}
	#[derive(Deserialize)]
	pub struct VersionUpdateRequest {
	version: VersionUpdate,
	}

[Rustin170506]

From the design of GitHub's patch API, it seems they don't have this kind of wrapper. I'm not sure what the purpose of this kind of wrapper is.
See: https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#update-a-pull-request

[Turbo87]

GitHub is using a different API response design in general. If you look at https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#get-a-pull-request you will see that there is also no wrapper object for the GET method.

If you compare that to https://crates.io/api/v1/crates/ripgrep/14.1.0, you can see that our GET responses are using a wrapper object. For this particular URL/endpoint this might not be relevant, but if you look at https://crates.io/api/v1/crates/ripgrep, you can see that it not only include the crate field with the primary object in the response, but also secondary included objects like categories, keywords and versions.

While GitHub may not use the wrapper object for responses, I think it's more important to stay somewhat consistent with our own API and use it here.

[Rustin170506]

🔢 Self-check (PR reviewed by myself and ready for feedback.)