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

Issue #5304 HTTP2 HostHeader #5307

Merged
merged 6 commits into from
Sep 23, 2020
Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Sep 21, 2020

#5304
Updated HostHeaderCustomizer to actually add the Host header, either from values passed in the custructor or from the getServerName and getServerPort methods.

The HttpURI is no longer updated.

Signed-off-by: Greg Wilkins gregw@webtide.com

Updated HostHeaderCustomizer to actually add the Host header, either from values passed in the custructor or from the getServerName and getServerPort methods.

The HttpURI is no longer updated.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

This probably needs test cases, as there is a flaw in the null serverName case in this code.

@gregw gregw marked this pull request as draft September 21, 2020 20:41
 + Found and fixed bug in HttpFields
 + Added port normalization support to HttpScheme
 + added test

Signed-off-by: Greg Wilkins <gregw@webtide.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
 + refixed bug in HttpFields

Signed-off-by: Greg Wilkins <gregw@webtide.com>
 + still fixing HttpFields bug

Signed-off-by: Greg Wilkins <gregw@webtide.com>
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Some minor concerns.

updates from review
@gregw gregw marked this pull request as ready for review September 22, 2020 13:06
@gregw gregw requested a review from joakime September 22, 2020 13:06
@gregw
Copy link
Contributor Author

gregw commented Sep 22, 2020

@joakime nudge

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

accidentally Mutable?

@gregw
Copy link
Contributor Author

gregw commented Sep 22, 2020

@joakim, it's not accidentally mutable. The style we mostly use is that the setter decides if it wants the mutable or immutable reference. So in this case request will call asImutable on the passed HttpField.

The idea is that the callee decides and the caller can never make the request fields mutable

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

LGTM

@gregw gregw merged commit 0ac34ff into jetty-10.0.x Sep 23, 2020
@gregw gregw deleted the jetty-10.0.x-5304-Http2Host branch September 23, 2020 13:05
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.

HTTP/2 with HttpServletRequest.getHeader("Host") returns null on Jetty 10, but a valid value on Jetty 9
2 participants