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.

Sign up for GitHub

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

Jump to bottom

Decouple preserving header case from FFI (fixes #2313) #2480

Merged
merged 1 commit into from
Apr 21, 2021
Merged

Decouple preserving header case from FFI (fixes #2313) #2480

merged 1 commit into from
Apr 21, 2021
+656 −181

Conversation

nox
Copy link
Contributor

@nox nox commented Mar 26, 2021

No description provided.

@nox nox force-pushed the preserve-header-case branch from ca6a4fa to bd3d5cf Compare March 26, 2021 11:26
@nox nox changed the title Decouple preserving header case from the FFI feature (fixes #2313) Decouple preserving header case from FFI (fixes #2313) Mar 26, 2021
@nox nox force-pushed the preserve-header-case branch from bd3d5cf to 39c33ee Compare March 26, 2021 11:46
@nox
Copy link
Contributor Author

nox commented Mar 26, 2021

This probably needs more tests, but I don't know where you want me to write those and which kind of tests you want.

@nox nox force-pushed the preserve-header-case branch 3 times, most recently from 7f0e312 to e339223 Compare March 29, 2021 08:21
@nox
Copy link
Contributor Author

nox commented Mar 29, 2021

It helps if I actually commit the damn file… Now HeaderCaseMap lives in crate::ext.

@nox nox force-pushed the preserve-header-case branch 4 times, most recently from 0865e6c to 38ea26e Compare March 29, 2021 10:26
@nox
Copy link
Contributor Author

nox commented Apr 7, 2021

Mmh this is incomplete, I just realised Server::encode hardcodes some lowercased header names.

@nox nox force-pushed the preserve-header-case branch 6 times, most recently from 54011d8 to 82cb94c Compare April 9, 2021 07:40
@zonyitoo zonyitoo mentioned this pull request Apr 11, 2021
Closed
@nox
Copy link
Contributor Author

nox commented Apr 11, 2021

Ah, I just realised out of nowhere I didn't implement preserving the original case of received requests in the server role.

@zonyitoo
Copy link
Contributor

zonyitoo commented Apr 11, 2021 •
edited
Loading

Proxy developers are looking forward to the day that this PR was merged into master.

@nox nox force-pushed the preserve-header-case branch 2 times, most recently from caf0bbe to 395a17e Compare April 11, 2021 15:34
@nox
Copy link
Contributor Author

nox commented Apr 11, 2021

Ah, I just realised out of nowhere I didn't implement preserving the original case of received requests in the server role.

I implemented that and added a test.

seanmonstar
seanmonstar reviewed Apr 12, 2021
View reviewed changes
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful work, thanks for all the care you put into it!

src/client/client.rs Show resolved Hide resolved
src/ext.rs Outdated Show resolved Hide resolved
src/ext.rs Outdated Show resolved Hide resolved
src/proto/h1/conn.rs Outdated Show resolved Hide resolved
src/proto/h1/role.rs Show resolved Hide resolved
src/proto/h1/role.rs Outdated Show resolved Hide resolved
src/proto/h1/role.rs Show resolved Hide resolved
@nox nox force-pushed the preserve-header-case branch 3 times, most recently from 05925da to 6638086 Compare April 15, 2021 07:53
seanmonstar
seanmonstar reviewed Apr 16, 2021
View reviewed changes
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, just some minor API points left. I'll try to grab the benchmarks of cargo bench --bench pipeline for before and after, just to include as a data point in the PR.

src/ext.rs Outdated Show resolved Hide resolved
src/ext.rs Outdated Show resolved Hide resolved
@nox nox force-pushed the preserve-header-case branch from 6638086 to f5e093e Compare April 16, 2021 07:15
@nox
Copy link
Contributor Author

nox commented Apr 16, 2021

This is done, I also removed Default for HeaderCaseMap while at it.

@nox nox force-pushed the preserve-header-case branch 4 times, most recently from fc1d2a0 to 1936470 Compare April 17, 2021 07:24
@seanmonstar
Copy link
Member

Well crap, didn't realize merging the other would cause conflicts. If you could resolve them, I can merge right after and get a release out.

@nox nox force-pushed the preserve-header-case branch from 1936470 to e585606 Compare April 21, 2021 07:14
@nox
Copy link
Contributor Author

nox commented Apr 21, 2021

@seanmonstar Rebased!

@nox nox force-pushed the preserve-header-case branch from e585606 to 890a63d Compare April 21, 2021 07:25
@nox
feat(http1): decouple preserving header case from FFI (fixes hyperium…
d853f52 
…#2313)

The feature is now supported in both the server and the client
and can be combined with the title case feature, for headers
which don't have entries in the header case map.
@nox nox force-pushed the preserve-header-case branch from 890a63d to d853f52 Compare April 21, 2021 10:53
@seanmonstar seanmonstar merged commit dbea771 into hyperium:master Apr 21, 2021
@nox nox deleted the preserve-header-case branch April 21, 2021 17:44
@zonyitoo zonyitoo mentioned this pull request Apr 22, 2021
Closed
@nox nox mentioned this pull request Apr 22, 2021
Merged
BenxiangGe pushed a commit to BenxiangGe/hyper that referenced this pull request Jul 26, 2021
@nox
feat(http1): add options to preserve header casing (hyperium#2480)
ce1c71d 
Decouple preserving header case from FFI:

The feature is now supported in both the server and the client
and can be combined with the title case feature, for headers
which don't have entries in the header case map.

Closes hyperium#2313
@seanmonstar seanmonstar mentioned this pull request Nov 16, 2021
Open
4 tasks
@seanmonstar seanmonstar mentioned this pull request May 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanmonstar seanmonstar seanmonstar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nox @zonyitoo @seanmonstar