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

parse request version in extractor, refactor match_version #2383

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

syphar
Copy link
Member

@syphar syphar commented Jan 10, 2024

So, this is a somewhat mid-sized refactor in how we handle & match version requirements in requests.

The general idea is:

  • we are using a new axum extractor to parse the version requirement (web::ReqVersion). It can parse static names like /latest/, but also specific semver::Version and semver::VersionReq. This changes the status code for an invalid version identifier.. Previously we returned a 404, now it will be a 400. I added a custom Path extractor so we have a nice error page on 400 too, so for users it should be fine.
  • All places where we use the extracted version identifier we can now use a semver::Version
  • match_version and its response types are refactored to use the new ReqVersion.
  • in our handlers there was much repetition around how we handle different ReqVersion and redirect in some cases. This is replaced by introducing AxumNope::Redirect and into_canonical_req_version_or_else for the match_version result. This unifies the version identifier canonicalization in more places.
  • In there I also added a redirect from newest or semver * to /latest/, which IMO makes more sense.

I think there will also be more edge cases where the exact redirects for version identifiers are not slightly different than before, but IMO this is fine for a (mostly) web interface.

@syphar syphar requested a review from a team as a code owner January 10, 2024 12:55
@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Jan 10, 2024
@syphar syphar changed the title Refactor match version try 2 parse request version in extractor, refactor match_version Jan 10, 2024
@syphar syphar force-pushed the refactor-match-version-try-2 branch 2 times, most recently from 3a67a76 to d793584 Compare January 17, 2024 07:54
@syphar syphar force-pushed the refactor-match-version-try-2 branch 2 times, most recently from b70ffe0 to 6a811cb Compare January 31, 2024 06:59
@syphar
Copy link
Member Author

syphar commented Jan 31, 2024

@Nemo157 @GuillaumeGomez any chance for some feedback? I'm aware it's a big PR, but I hope it will improve some readability in the web handlers.

@GuillaumeGomez
Copy link
Member

Looks good to me but please wait for @Nemo157 review. :)

@syphar
Copy link
Member Author

syphar commented Feb 14, 2024

@Nemo157 and chance you can find time for at least some design feedback? I would highly appreciate it

@syphar syphar force-pushed the refactor-match-version-try-2 branch 3 times, most recently from a4dfbcf to 40ac35a Compare February 21, 2024 09:18
clippy.toml Outdated Show resolved Hide resolved
src/web/build_details.rs Outdated Show resolved Hide resolved
src/web/crate_details.rs Outdated Show resolved Hide resolved
@syphar syphar force-pushed the refactor-match-version-try-2 branch 4 times, most recently from ed54de4 to 6ef7bd3 Compare February 23, 2024 14:34
@syphar syphar requested a review from Nemo157 February 23, 2024 14:50
@syphar
Copy link
Member Author

syphar commented Feb 23, 2024

@Nemo157 this is ready for another round.

After removing the package_name rename commit and pushing this branch I realized that this might be hard to review because some files with more changes appear as changed again, sorry about that.

@syphar syphar force-pushed the refactor-match-version-try-2 branch from 9f5d167 to d37eceb Compare February 28, 2024 14:46
@syphar syphar merged commit 70b46ac into rust-lang:master Feb 29, 2024
8 of 9 checks passed
@syphar syphar deleted the refactor-match-version-try-2 branch February 29, 2024 12:59
@github-actions github-actions bot added the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Feb 29, 2024
@github-actions github-actions bot removed the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Feb 29, 2024
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants