Apply TomcatConnectorCustomizer and TomcatContextCustomizer beans automatically #15492
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
Thanks very much for opening your first Spring Boot pull request. I've left a few review comments. Could you please take a look and make the necessary changes? Feel free to ask any questions you have if anything isn't clear or you need some more guidance. Thanks again.
...g/springframework/boot/autoconfigure/web/reactive/ReactiveWebServerFactoryConfiguration.java
Show resolved
Hide resolved
...org/springframework/boot/autoconfigure/web/servlet/ServletWebServerFactoryConfiguration.java
Show resolved
Hide resolved
...ramework/boot/autoconfigure/web/reactive/ReactiveWebServerFactoryAutoConfigurationTests.java
Show resolved
Hide resolved
|
|
||
| @Bean | ||
| public TomcatConnectorCustomizer connectorCustomizer() { | ||
| return (connector) -> connector.setPort(9001); |
There was a problem hiding this comment.
This might cause a port clash if something else is used port 9001. It would be safer if the customizer didn't do anything to the connector.
| runner.run((context) -> { | ||
| TomcatServletWebServerFactory factory = context | ||
| .getBean(TomcatServletWebServerFactory.class); | ||
| assertThat(factory.getTomcatContextCustomizers()).hasSize(1); |
There was a problem hiding this comment.
If this test passes then this assertion isn't quite right as TomcatContextCustomizerConfiguration doesn't define a TomcatContextCustomizer. The assertions needs to be written such that if you remove .withUserConfiguration(TomcatContextCustomizerConfiguration.class) the test will fail.
There was a problem hiding this comment.
assertThat(factory.getTomcatContextCustomizers()).hasSize(1); is working fine in TomcatServletWebServerFactory case. If i remove .withUserConfiguration(TomcatContextCustomizerConfiguration.class) from test assertion going to fail. If we doesn't define TomcatContextCustomizer into TomcatContextCustomizerConfiguration assertion is also failed because tomcatContextCustomizers into TomcatReactiveWebServerFactory list is zero.
...ramework/boot/autoconfigure/web/reactive/ReactiveWebServerFactoryAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
...ramework/boot/autoconfigure/web/reactive/ReactiveWebServerFactoryAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Bean | ||
| public TomcatConnectorCustomizer connectorCustomizer() { | ||
| return (connector) -> connector.setPort(9001); |
There was a problem hiding this comment.
This might cause a port clash if something else is used port 9001. It would be safer if the customizer didn't do anything to the connector.
...gframework/boot/autoconfigure/web/servlet/ServletWebServerFactoryAutoConfigurationTests.java
Show resolved
Hide resolved
| runner.run((context) -> { | ||
| TomcatServletWebServerFactory factory = context | ||
| .getBean(TomcatServletWebServerFactory.class); | ||
| assertThat(factory.getTomcatContextCustomizers()).hasSize(1); |
There was a problem hiding this comment.
If this test passes then this assertion isn't quite right as TomcatContextCustomizerConfiguration doesn't define a TomcatContextCustomizer. The assertions needs to be written such that if you remove .withUserConfiguration(TomcatContextCustomizerConfiguration.class) the test will fail.
There was a problem hiding this comment.
TomcatServletWebServerFactory already have one TomcatContextCustomizer into tomcatConnectorCustomizers list. if i remove .withUserConfiguration(TomcatContextCustomizerConfiguration.class) assertion is not failed.
We have an option that add assert for size 2 because one is already there and other is added by our configuration class. If i am thinking wrong you can guide me towards correcting the assertion.
wilkinsona
changed the title
* gh-15492: Polish "Apply context and connector customizer beans to Tomcat factories" Apply context and connector customizer beans to Tomcat factories
|
@Raheela1024 Thank you very much for making your first contribution to Spring Boot. I have merged the proposed changes into the master branch along with a tiny bit of polish to change the names of a couple of |
No description provided.