Skip to content

Add uri typed headers #3455

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

keithamus
Copy link

@keithamus keithamus commented Aug 17, 2024

PR Type

Feature

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

This adds the Location, Content-Location and Referer typed headers. It also adds the TryIntoHeaderValue trait for Uri which allows Uri to be a typed value for these headers.

While Location technically is a different type than Content-Location and Referer (Location allows for protocol relative URIs) I think it's not a large enough distinction to warrant a separate type, as using protocol relative urls is quite rare. However we could use url::Url instead of Uri for Location if we wanted to allow for that distinction.

The motivation for this PR is to allow for using Referer as an Extractor, and the other two were easy enough to also add, and complete the trifecta of URI related headers (to the best of my knowledge).

@keithamus keithamus force-pushed the add-uri-typed-headers branch 2 times, most recently from c75c46d to 3d17c3f Compare August 17, 2024 22:33
@robjtede robjtede added B-semver-minor A-web project: actix-web labels Aug 19, 2024
@keithamus keithamus force-pushed the add-uri-typed-headers branch from 3d17c3f to f96a21f Compare August 19, 2024 13:13
@keithamus keithamus requested a review from robjtede August 19, 2024 13:13
@keithamus
Copy link
Author

@robjtede those changes have been made. Let me know if you have any more feedback.

@keithamus
Copy link
Author

I'm still interested in this patch, if you think it's worth merging in.

@robjtede
Copy link
Member

robjtede commented May 10, 2025

Just come back to this and I still like it, but I'm going to make the Location (and possibly ContentLocation after some more research) use url::Url instead of http::Uri so that fragments (/path?query#fragment) are supported.

I'll cut a release with what's been merged already and get to this one afterwards. Should be easy to fixup and get out this weekend.

@robjtede
Copy link
Member

Run into a hurdle with that idea, that url::Url doesn't support relative references, e.g.: /new-path. So both types seem incapable of expressing what we actually want from Location/Content-Location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web B-semver-minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants