Skip to content

Conversation

@ssko1
Copy link

@ssko1 ssko1 commented Aug 29, 2018

When making the jump from Tomcat to Netty, I noticed that my -Dserver.max-http-header-size configuration was not being honored. This PR allows users to configure the netty HttpRequestDecoderSpec using ServerProperties.

Notes on implementation

  • (serverProperties max-http-header-size of <= 0 will defer to serverProperties.netty maxHeaderSize)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 29, 2018
@snicoll
Copy link
Member

snicoll commented Aug 29, 2018

@CinnamonBagels thanks a lot for the PR but we already have an issue for this (#13831) and a PR (#14234). We'd rather fix the server.max-http-header-size situation first.

The other part of this PR look unrelated to the task at hand. We usually expose first class properties based on a concrete scenario. Can you please share why you believe those properties would be useful vs. a programmatic customizer?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Aug 29, 2018
@ssko1
Copy link
Author

ssko1 commented Aug 29, 2018

I added all of the configuration options that HttpRequestDecoderSpec allowed on a nice-to-have basis. Now I understand the reasons why properties are exposed, so my changes don't make sense yet. Thanks for pointing me to the issue and PR and giving feedback.

@snicoll
Copy link
Member

snicoll commented Aug 30, 2018

Alright, thanks for the PR anyway.

@snicoll snicoll closed this Aug 30, 2018
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: declined A suggestion or change that we don't feel we should currently apply

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants