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

Rename max-http-post-size server property #18566

Closed
wants to merge 1 commit into from

Conversation

rhamedy
Copy link
Contributor

@rhamedy rhamedy commented Oct 12, 2019

Fixes #18521

@wilkinsona here is a summary of changes

  • Updated ServerProperties.java to deprecate the max-http-post-size for both tomcat and jersey and introduced replacements max-http-form-post-size
  • Updated the ServerPropertiesTests.java to add new tests for tomcatMaxHttpFormPostSizeMatchesConnectorDefault
  • Updated the ServerPropertiesTests.java to add new test for jettyMaxHttpFormPostSizeMatchesDefault (this test is 99% same as jettyMaxHttpPostSizeMatchesDefault, what do you think about refactoring common code?)
  • Updated the TomcatWebServerFactoryCustomizer.java for the new property maxHttpFormPostSize and added tests for it inTomcatWebServerFactoryCustomizerTests.java
@Test
public void customMaxHttpPostSize() {
	bind("server.tomcat.max-http-post-size=10000");
	bind("server.tomcat.max-http-form-post-size=10000");
	customizeAndRunServer(
		(server) -> assertThat(server.getTomcat().getConnector().getMaxPostSize()).isEqualTo(10000));
}

without bind("server.tomcat.max-http-form-post-size=10000"); this test and customDisableMaxHttpPostSize fails. This is due to addition of the new property but, when binding both as shown above then it passes. Not sure if it's because both these props point to the same property in connector or because I messed up the mapping in customizers.

  • Updated the JettyWebServerFactoryCustomizer for the new property

Not sure if I missed out anything that needs to be updated. I would appreciate a review and will squash my commits once all the code review comments are addressed.

Thanks.

Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

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

Thanks very much for the pull request, @rhamedy. Please let us know if you have the time to make the requested changes in the next few days. No problem at all if you don't, we can make the changes in a polishing commit as part of merging your changes.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Oct 14, 2019
@wilkinsona wilkinsona changed the title Deprecated max-http-post-size for tomcat & jetty and introduced replacements It is unclear that server.tomcat.max-http-post-size and server.jetty.max-http-post-size only apply to POSTed form content Oct 14, 2019
@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 14, 2019
@wilkinsona wilkinsona added this to the 2.1.x milestone Oct 14, 2019
@rhamedy
Copy link
Contributor Author

rhamedy commented Oct 14, 2019

Hi @wilkinsona
I appreciate the clear and very detailed review comments. I believe I have addressed them all, please let me know if any more changes need to be made. I have not yet squashed my commits in order to allow requested changes to be reviewed easily. As soon as you approve the changes then I would squash my commits 👍 I got plenty of time this week 😄

Out of curiosity, is this deprecation any different from other deprecations? I notice that in ServerProperties.java the max-http-header-size does not only have the @DeprecatedConfigurationProperty but, also the @Deprecated. We have not added @Deprecated in the old property getters and setters however, we did change them to use the new field, I wonder if because of this we don't need to mark them as @Deprecated 🤔

Thanks for the help.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 14, 2019
@wilkinsona
Copy link
Member

Things look good now. Thank you for the updates.

Let's mark the old getters and setters as @Deprecated as well, for consistency if nothing else. Let me know if you have time to do that and to squash your commits. If not, we can take care of it when merging.

@rhamedy
Copy link
Contributor Author

rhamedy commented Oct 16, 2019

Hi @wilkinsona
I have applied the requested changes and squashed my commit. It's all yours. Thank you for your help 🙂

Due to the unclarity that properties server.tomcat.max-http-post-size 
and server.jetty.max-http-post-size only apply to POSTed form content, the 
two properties are deprecated in favor of newly introduced server properties 
server.tomcat.max-http-form-post-size & server.jetty.max-http-form-post-size.

Fixes spring-projectsgh-18521
philwebb pushed a commit that referenced this pull request Oct 22, 2019
Rename `max-http-post-size` to `max-http-form-post-size` for Jetty and
Tomcat to make it clearer that they only apply to POSTed form content.

See gh-18566
@philwebb philwebb closed this in b61b7b0 Oct 22, 2019
@philwebb philwebb changed the title It is unclear that server.tomcat.max-http-post-size and server.jetty.max-http-post-size only apply to POSTed form content Rename max-http-post-size server property Oct 22, 2019
@philwebb philwebb modified the milestones: 2.1.x, 2.1.10 Oct 22, 2019
@philwebb
Copy link
Member

Thanks a lot @rhamedy! This is now in 2.1.x and master.

@rhamedy rhamedy deleted the gh-18521v2 branch October 23, 2019 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants