Skip to content

Commit d8144da

Browse files
nosannosan
committed
Stop using @ConditionalOnClass on @bean methods
Signed-off-by: Dmytro Nosan <[email protected]>
1 parent 61acc52 commit d8144da

File tree

14 files changed

+233
-75
lines changed

14 files changed

+233
-75
lines changed

‎buildSrc/src/main/java/org/springframework/boot/build/architecture/ArchitectureCheck.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ public ArchitectureCheck() {
7777
getRules().addAll(getProhibitObjectsRequireNonNull().convention(true)
7878
.map(whenTrue(ArchitectureRules::noClassesShouldCallObjectsRequireNonNull)));
7979
getRules().addAll(ArchitectureRules.standard());
80-
getRules().addAll(whenMainSources(
81-
() -> Collections.singletonList(ArchitectureRules.allBeanMethodsShouldReturnNonPrivateType())));
80+
getRules().addAll(whenMainSources(() -> List.of(ArchitectureRules.allBeanMethodsShouldReturnNonPrivateType(),
81+
ArchitectureRules.allBeanMethodsShouldNotHaveConditionalOnClass())));
8282
getRuleDescriptions().set(getRules().map(this::asDescriptions));
8383
}
8484

‎buildSrc/src/main/java/org/springframework/boot/build/architecture/ArchitectureRules.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,15 @@ static ArchRule allBeanMethodsShouldReturnNonPrivateType() {
108108
.allowEmptyShould(true);
109109
}
110110

111+
static ArchRule allBeanMethodsShouldNotHaveConditionalOnClass() {
112+
return methodsThatAreAnnotatedWith("org.springframework.context.annotation.Bean").should()
113+
.notBeAnnotatedWith("org.springframework.boot.autoconfigure.condition.ConditionalOnClass")
114+
.because("conditions on @Bean methods are ineffective - they don't prevent "
115+
+ "the method signature from being loaded. Such conditions need to be placed"
116+
+ " on a @Configuration class, allowing the condition to back off before the type is loaded.")
117+
.allowEmptyShould(true);
118+
}
119+
111120
private static ArchRule allPackagesShouldBeFreeOfTangles() {
112121
return SlicesRuleDefinition.slices().matching("(**)").should().beFreeOfCycles();
113122
}

‎buildSrc/src/test/java/org/springframework/boot/autoconfigure/condition/ConditionalOnClass.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* Copyright 2012-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.boot.autoconfigure.condition;
18+
19+
import java.lang.annotation.Documented;
20+
import java.lang.annotation.ElementType;
21+
import java.lang.annotation.Retention;
22+
import java.lang.annotation.RetentionPolicy;
23+
import java.lang.annotation.Target;
24+
25+
@Target({ ElementType.TYPE, ElementType.METHOD })
26+
@Retention(RetentionPolicy.RUNTIME)
27+
@Documented
28+
public @interface ConditionalOnClass {
29+
30+
Class<?>[] value() default {};
31+
32+
}

‎buildSrc/src/test/java/org/springframework/boot/build/architecture/ArchitectureCheckTests.java‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -270,22 +270,41 @@ void whenBeanMethodExposesNonPrivateTypeShouldSucceedAndWriteEmptyReport(Task ta
270270
build(this.gradleBuild.withDependencies(SPRING_CONTEXT), task);
271271
}
272272

273+
@Test
274+
void whenConditionalOnClassUsedOnBeanMethodsWithMainSourcesShouldFailAndWriteReport() throws IOException {
275+
prepareTask(Task.CHECK_ARCHITECTURE_MAIN, "conditionalonclass/onbeanmethod", "../../autoconfigure");
276+
buildAndFail(this.gradleBuild.withDependencies(SPRING_CONTEXT), Task.CHECK_ARCHITECTURE_MAIN,
277+
"methods that are annotated with @Bean should not be annotated with @ConditionalOnClass,"
278+
+ " because conditions on @Bean methods are ineffective - they don't prevent"
279+
+ " the method signature from being loaded. Such conditions need to be placed"
280+
+ " on a @Configuration class, allowing the condition to back off before the type is loaded");
281+
}
282+
283+
@Test
284+
void whenConditionalOnClassUsedOnBeanMethodsWithTestSourcesShouldSucceedAndWriteEmptyReport() throws IOException {
285+
prepareTask(Task.CHECK_ARCHITECTURE_TEST, "conditionalonclass/onbeanmethod", "../../autoconfigure");
286+
build(this.gradleBuild.withDependencies(SPRING_CONTEXT), Task.CHECK_ARCHITECTURE_TEST);
287+
}
288+
273289
private void prepareTask(Task task, String... sourceDirectories) throws IOException {
274290
for (String sourceDirectory : sourceDirectories) {
275291
FileSystemUtils.copyRecursively(
276292
Paths.get("src/test/java")
277293
.resolve(ClassUtils.classPackageAsResourcePath(getClass()))
278-
.resolve(sourceDirectory),
294+
.resolve(sourceDirectory)
295+
.normalize(),
279296
task.getSourceDirectory(this.gradleBuild.getProjectDir())
280297
.resolve(ClassUtils.classPackageAsResourcePath(getClass()))
281-
.resolve(sourceDirectory));
298+
.resolve(sourceDirectory)
299+
.normalize());
282300
}
283301
}
284302

285303
private void build(GradleBuild gradleBuild, Task task) throws IOException {
286304
try {
287305
BuildResult buildResult = gradleBuild.build(task.toString());
288-
assertThat(buildResult.taskPaths(TaskOutcome.SUCCESS)).contains(":" + task);
306+
assertThat(buildResult.taskPaths(TaskOutcome.SUCCESS)).describedAs(buildResult.getOutput())
307+
.contains(":" + task);
289308
assertThat(task.getFailureReport(gradleBuild.getProjectDir())).isEmpty();
290309
}
291310
catch (UnexpectedBuildFailure ex) {
@@ -301,7 +320,8 @@ private void build(GradleBuild gradleBuild, Task task) throws IOException {
301320
private void buildAndFail(GradleBuild gradleBuild, Task task, String... messages) throws IOException {
302321
try {
303322
BuildResult buildResult = gradleBuild.buildAndFail(task.toString());
304-
assertThat(buildResult.taskPaths(TaskOutcome.FAILED)).contains(":" + task);
323+
assertThat(buildResult.taskPaths(TaskOutcome.FAILED)).describedAs(buildResult.getOutput())
324+
.contains(":" + task);
305325
assertThat(task.getFailureReport(gradleBuild.getProjectDir())).contains(messages);
306326
}
307327
catch (UnexpectedBuildSuccess ex) {

‎buildSrc/src/test/java/org/springframework/boot/build/architecture/conditionalonclass/onbeanmethod/ConditionalOnClassOnBeanMethod.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/*
2+
* Copyright 2012-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.boot.build.architecture.conditionalonclass.onbeanmethod;
18+
19+
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
20+
import org.springframework.context.annotation.Bean;
21+
22+
class ConditionalOnClassOnBeanMethod {
23+
24+
@Bean
25+
@ConditionalOnClass(String.class)
26+
String helloWorld() {
27+
return "Hello World";
28+
}
29+
30+
}

‎spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/audit/AuditAutoConfiguration.java‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
3232
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
3333
import org.springframework.context.annotation.Bean;
34+
import org.springframework.context.annotation.Configuration;
3435

3536
/**
3637
* {@link EnableAutoConfiguration Auto-configuration} for {@link AuditEvent}s.
@@ -50,18 +51,28 @@ public AuditListener auditListener(AuditEventRepository auditEventRepository) {
5051
return new AuditListener(auditEventRepository);
5152
}
5253

53-
@Bean
54+
@Configuration(proxyBeanMethods = false)
5455
@ConditionalOnClass(name = "org.springframework.security.authentication.event.AbstractAuthenticationEvent")
55-
@ConditionalOnMissingBean(AbstractAuthenticationAuditListener.class)
56-
public AuthenticationAuditListener authenticationAuditListener() {
57-
return new AuthenticationAuditListener();
56+
static class AuthenticationAuditConfiguration {
57+
58+
@Bean
59+
@ConditionalOnMissingBean(AbstractAuthenticationAuditListener.class)
60+
AuthenticationAuditListener authenticationAuditListener() {
61+
return new AuthenticationAuditListener();
62+
}
63+
5864
}
5965

60-
@Bean
66+
@Configuration(proxyBeanMethods = false)
6167
@ConditionalOnClass(name = "org.springframework.security.access.event.AbstractAuthorizationEvent")
62-
@ConditionalOnMissingBean(AbstractAuthorizationAuditListener.class)
63-
public AuthorizationAuditListener authorizationAuditListener() {
64-
return new AuthorizationAuditListener();
68+
static class AuthorizationAuditConfiguration {
69+
70+
@Bean
71+
@ConditionalOnMissingBean(AbstractAuthorizationAuditListener.class)
72+
AuthorizationAuditListener authorizationAuditListener() {
73+
return new AuthorizationAuditListener();
74+
}
75+
6576
}
6677

6778
}

‎spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/endpoint/jackson/JacksonEndpointAutoConfiguration.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,12 @@
3636
* @since 3.0.0
3737
*/
3838
@AutoConfiguration(after = JacksonAutoConfiguration.class)
39+
@ConditionalOnClass({ ObjectMapper.class, Jackson2ObjectMapperBuilder.class })
3940
public class JacksonEndpointAutoConfiguration {
4041

4142
@Bean
4243
@ConditionalOnProperty(name = "management.endpoints.jackson.isolated-object-mapper", matchIfMissing = true)
43-
@ConditionalOnClass({ ObjectMapper.class, Jackson2ObjectMapperBuilder.class })
44-
public EndpointObjectMapper endpointObjectMapper() {
44+
EndpointObjectMapper endpointObjectMapper() {
4545
ObjectMapper objectMapper = Jackson2ObjectMapperBuilder.json()
4646
.featuresToDisable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS,
4747
SerializationFeature.WRITE_DURATIONS_AS_TIMESTAMPS)

‎spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/security/servlet/SecurityRequestMatchersManagementContextConfiguration.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ public static class MvcRequestMatcherConfiguration {
5353

5454
@Bean
5555
@ConditionalOnMissingBean
56-
@ConditionalOnClass(DispatcherServlet.class)
5756
public RequestMatcherProvider requestMatcherProvider(DispatcherServletPath servletPath) {
5857
return new AntPathRequestMatcherProvider(servletPath::getRelativePath);
5958
}

‎spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/web/servlet/ServletManagementChildContextConfiguration.java‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -81,22 +81,37 @@ ServletManagementWebServerFactoryCustomizer servletManagementWebServerFactoryCus
8181
return new ServletManagementWebServerFactoryCustomizer(beanFactory);
8282
}
8383

84-
@Bean
84+
@Configuration(proxyBeanMethods = false)
8585
@ConditionalOnClass(name = "io.undertow.Undertow")
86-
UndertowAccessLogCustomizer undertowManagementAccessLogCustomizer() {
87-
return new UndertowAccessLogCustomizer();
86+
static class UndertowConfiguration {
87+
88+
@Bean
89+
UndertowAccessLogCustomizer undertowManagementAccessLogCustomizer() {
90+
return new UndertowAccessLogCustomizer();
91+
}
92+
8893
}
8994

90-
@Bean
95+
@Configuration(proxyBeanMethods = false)
9196
@ConditionalOnClass(name = "org.apache.catalina.valves.AccessLogValve")
92-
TomcatAccessLogCustomizer tomcatManagementAccessLogCustomizer() {
93-
return new TomcatAccessLogCustomizer();
97+
static class TomcatConfiguration {
98+
99+
@Bean
100+
TomcatAccessLogCustomizer tomcatManagementAccessLogCustomizer() {
101+
return new TomcatAccessLogCustomizer();
102+
}
103+
94104
}
95105

96-
@Bean
106+
@Configuration(proxyBeanMethods = false)
97107
@ConditionalOnClass(name = "org.eclipse.jetty.server.Server")
98-
JettyAccessLogCustomizer jettyManagementAccessLogCustomizer() {
99-
return new JettyAccessLogCustomizer();
108+
static class JettyConfiguration {
109+
110+
@Bean
111+
JettyAccessLogCustomizer jettyManagementAccessLogCustomizer() {
112+
return new JettyAccessLogCustomizer();
113+
}
114+
100115
}
101116

102117
@Configuration(proxyBeanMethods = false)

‎spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/flyway/FlywayAutoConfiguration.java‎

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -144,24 +144,6 @@ PropertiesFlywayConnectionDetails flywayConnectionDetails() {
144144
return new PropertiesFlywayConnectionDetails(this.properties);
145145
}
146146

147-
@Bean
148-
@ConditionalOnClass(name = "org.flywaydb.database.sqlserver.SQLServerConfigurationExtension")
149-
SqlServerFlywayConfigurationCustomizer sqlServerFlywayConfigurationCustomizer() {
150-
return new SqlServerFlywayConfigurationCustomizer(this.properties);
151-
}
152-
153-
@Bean
154-
@ConditionalOnClass(name = "org.flywaydb.database.oracle.OracleConfigurationExtension")
155-
OracleFlywayConfigurationCustomizer oracleFlywayConfigurationCustomizer() {
156-
return new OracleFlywayConfigurationCustomizer(this.properties);
157-
}
158-
159-
@Bean
160-
@ConditionalOnClass(name = "org.flywaydb.database.postgresql.PostgreSQLConfigurationExtension")
161-
PostgresqlFlywayConfigurationCustomizer postgresqlFlywayConfigurationCustomizer() {
162-
return new PostgresqlFlywayConfigurationCustomizer(this.properties);
163-
}
164-
165147
@Bean
166148
Flyway flyway(FlywayConnectionDetails connectionDetails, ResourceLoader resourceLoader,
167149
ObjectProvider<DataSource> dataSource, @FlywayDataSource ObjectProvider<DataSource> flywayDataSource,
@@ -355,6 +337,40 @@ public FlywayMigrationInitializer flywayInitializer(Flyway flyway,
355337
return new FlywayMigrationInitializer(flyway, migrationStrategy.getIfAvailable());
356338
}
357339

340+
@ConditionalOnClass(name = "org.flywaydb.database.sqlserver.SQLServerConfigurationExtension")
341+
@Configuration(proxyBeanMethods = false)
342+
static class SqlServerConfiguration {
343+
344+
@Bean
345+
SqlServerFlywayConfigurationCustomizer sqlServerFlywayConfigurationCustomizer(FlywayProperties properties) {
346+
return new SqlServerFlywayConfigurationCustomizer(properties);
347+
}
348+
349+
}
350+
351+
@Configuration(proxyBeanMethods = false)
352+
@ConditionalOnClass(name = "org.flywaydb.database.oracle.OracleConfigurationExtension")
353+
static class OracleConfiguration {
354+
355+
@Bean
356+
OracleFlywayConfigurationCustomizer oracleFlywayConfigurationCustomizer(FlywayProperties properties) {
357+
return new OracleFlywayConfigurationCustomizer(properties);
358+
}
359+
360+
}
361+
362+
@ConditionalOnClass(name = "org.flywaydb.database.postgresql.PostgreSQLConfigurationExtension")
363+
@Configuration(proxyBeanMethods = false)
364+
static class PostgresqlConfiguration {
365+
366+
@Bean
367+
PostgresqlFlywayConfigurationCustomizer postgresqlFlywayConfigurationCustomizer(
368+
FlywayProperties properties) {
369+
return new PostgresqlFlywayConfigurationCustomizer(properties);
370+
}
371+
372+
}
373+
358374
}
359375

360376
private static class LocationResolver {

0 commit comments

Comments
 (0)