Stop using @ConditionalOnClass on @bean methods

Signed-off-by: Dmytro Nosan <[email protected]>

Author: nosan

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

@@ -77,8 +77,8 @@ public ArchitectureCheck() {
 		getRules().addAll(getProhibitObjectsRequireNonNull().convention(true)
 			.map(whenTrue(ArchitectureRules::noClassesShouldCallObjectsRequireNonNull)));
 		getRules().addAll(ArchitectureRules.standard());
-		getRules().addAll(whenMainSources(
-				() -> Collections.singletonList(ArchitectureRules.allBeanMethodsShouldReturnNonPrivateType())));
+		getRules().addAll(whenMainSources(() -> List.of(ArchitectureRules.allBeanMethodsShouldReturnNonPrivateType(),
+				ArchitectureRules.allBeanMethodsShouldNotHaveConditionalOnClass())));
 		getRuleDescriptions().set(getRules().map(this::asDescriptions));
 	}
 

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

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

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

@@ -0,0 +1,32 @@
+/*
+ * Copyright 2012-present the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.boot.autoconfigure.condition;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Target({ ElementType.TYPE, ElementType.METHOD })
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+public @interface ConditionalOnClass {
+
+	Class<?>[] value() default {};
+
+}

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

@@ -270,22 +270,41 @@ void whenBeanMethodExposesNonPrivateTypeShouldSucceedAndWriteEmptyReport(Task ta
 		build(this.gradleBuild.withDependencies(SPRING_CONTEXT), task);
 	}
 
+	@Test
+	void whenConditionalOnClassUsedOnBeanMethodsWithMainSourcesShouldFailAndWriteReport() throws IOException {
+		prepareTask(Task.CHECK_ARCHITECTURE_MAIN, "conditionalonclass/onbeanmethod", "../../autoconfigure");
+		buildAndFail(this.gradleBuild.withDependencies(SPRING_CONTEXT), Task.CHECK_ARCHITECTURE_MAIN,
+				"methods that are annotated with @Bean should not be annotated with @ConditionalOnClass,"
+						+ " because conditions on @Bean methods are ineffective - they don't prevent"
+						+ " the method signature from being loaded. Such conditions need to be placed"
+						+ " on a @Configuration class, allowing the condition to back off before the type is loaded");
+	}
+
+	@Test
+	void whenConditionalOnClassUsedOnBeanMethodsWithTestSourcesShouldSucceedAndWriteEmptyReport() throws IOException {
+		prepareTask(Task.CHECK_ARCHITECTURE_TEST, "conditionalonclass/onbeanmethod", "../../autoconfigure");
+		build(this.gradleBuild.withDependencies(SPRING_CONTEXT), Task.CHECK_ARCHITECTURE_TEST);
+	}
+
 	private void prepareTask(Task task, String... sourceDirectories) throws IOException {
 		for (String sourceDirectory : sourceDirectories) {
 			FileSystemUtils.copyRecursively(
 					Paths.get("src/test/java")
 						.resolve(ClassUtils.classPackageAsResourcePath(getClass()))
-						.resolve(sourceDirectory),
+						.resolve(sourceDirectory)
+						.normalize(),
 					task.getSourceDirectory(this.gradleBuild.getProjectDir())
 						.resolve(ClassUtils.classPackageAsResourcePath(getClass()))
-						.resolve(sourceDirectory));
+						.resolve(sourceDirectory)
+						.normalize());
 		}
 	}
 
 	private void build(GradleBuild gradleBuild, Task task) throws IOException {
 		try {
 			BuildResult buildResult = gradleBuild.build(task.toString());
-			assertThat(buildResult.taskPaths(TaskOutcome.SUCCESS)).contains(":" + task);
+			assertThat(buildResult.taskPaths(TaskOutcome.SUCCESS)).describedAs(buildResult.getOutput())
+				.contains(":" + task);
 			assertThat(task.getFailureReport(gradleBuild.getProjectDir())).isEmpty();
 		}
 		catch (UnexpectedBuildFailure ex) {
@@ -301,7 +320,8 @@ private void build(GradleBuild gradleBuild, Task task) throws IOException {
 	private void buildAndFail(GradleBuild gradleBuild, Task task, String... messages) throws IOException {
 		try {
 			BuildResult buildResult = gradleBuild.buildAndFail(task.toString());
-			assertThat(buildResult.taskPaths(TaskOutcome.FAILED)).contains(":" + task);
+			assertThat(buildResult.taskPaths(TaskOutcome.FAILED)).describedAs(buildResult.getOutput())
+				.contains(":" + task);
 			assertThat(task.getFailureReport(gradleBuild.getProjectDir())).contains(messages);
 		}
 		catch (UnexpectedBuildSuccess ex) {

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

@@ -0,0 +1,30 @@
+/*
+ * Copyright 2012-present the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.boot.build.architecture.conditionalonclass.onbeanmethod;
+
+import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
+import org.springframework.context.annotation.Bean;
+
+class ConditionalOnClassOnBeanMethod {
+
+	@Bean
+	@ConditionalOnClass(String.class)
+	String helloWorld() {
+		return "Hello World";
+	}
+
+}

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

@@ -31,6 +31,7 @@
 import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
 import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
 
 /**
  * {@link EnableAutoConfiguration Auto-configuration} for {@link AuditEvent}s.
@@ -50,18 +51,28 @@ public AuditListener auditListener(AuditEventRepository auditEventRepository) {
 		return new AuditListener(auditEventRepository);
 	}
 
-	@Bean
+	@Configuration(proxyBeanMethods = false)
 	@ConditionalOnClass(name = "org.springframework.security.authentication.event.AbstractAuthenticationEvent")
-	@ConditionalOnMissingBean(AbstractAuthenticationAuditListener.class)
-	public AuthenticationAuditListener authenticationAuditListener() {
-		return new AuthenticationAuditListener();
+	static class AuthenticationAuditConfiguration {
+
+		@Bean
+		@ConditionalOnMissingBean(AbstractAuthenticationAuditListener.class)
+		AuthenticationAuditListener authenticationAuditListener() {
+			return new AuthenticationAuditListener();
+		}
+
 	}
 
-	@Bean
+	@Configuration(proxyBeanMethods = false)
 	@ConditionalOnClass(name = "org.springframework.security.access.event.AbstractAuthorizationEvent")
-	@ConditionalOnMissingBean(AbstractAuthorizationAuditListener.class)
-	public AuthorizationAuditListener authorizationAuditListener() {
-		return new AuthorizationAuditListener();
+	static class AuthorizationAuditConfiguration {
+
+		@Bean
+		@ConditionalOnMissingBean(AbstractAuthorizationAuditListener.class)
+		AuthorizationAuditListener authorizationAuditListener() {
+			return new AuthorizationAuditListener();
+		}
+
 	}
 
 }

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

@@ -36,12 +36,12 @@
  * @since 3.0.0
  */
 @AutoConfiguration(after = JacksonAutoConfiguration.class)
+@ConditionalOnClass({ ObjectMapper.class, Jackson2ObjectMapperBuilder.class })
 public class JacksonEndpointAutoConfiguration {
 
 	@Bean
 	@ConditionalOnProperty(name = "management.endpoints.jackson.isolated-object-mapper", matchIfMissing = true)
-	@ConditionalOnClass({ ObjectMapper.class, Jackson2ObjectMapperBuilder.class })
-	public EndpointObjectMapper endpointObjectMapper() {
+	EndpointObjectMapper endpointObjectMapper() {
 		ObjectMapper objectMapper = Jackson2ObjectMapperBuilder.json()
 			.featuresToDisable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS,
 					SerializationFeature.WRITE_DURATIONS_AS_TIMESTAMPS)

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

@@ -53,7 +53,6 @@ public static class MvcRequestMatcherConfiguration {
 
 		@Bean
 		@ConditionalOnMissingBean
-		@ConditionalOnClass(DispatcherServlet.class)
 		public RequestMatcherProvider requestMatcherProvider(DispatcherServletPath servletPath) {
 			return new AntPathRequestMatcherProvider(servletPath::getRelativePath);
 		}

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

@@ -81,22 +81,37 @@ ServletManagementWebServerFactoryCustomizer servletManagementWebServerFactoryCus
 		return new ServletManagementWebServerFactoryCustomizer(beanFactory);
 	}
 
-	@Bean
+	@Configuration(proxyBeanMethods = false)
 	@ConditionalOnClass(name = "io.undertow.Undertow")
-	UndertowAccessLogCustomizer undertowManagementAccessLogCustomizer() {
-		return new UndertowAccessLogCustomizer();
+	static class UndertowConfiguration {
+
+		@Bean
+		UndertowAccessLogCustomizer undertowManagementAccessLogCustomizer() {
+			return new UndertowAccessLogCustomizer();
+		}
+
 	}
 
-	@Bean
+	@Configuration(proxyBeanMethods = false)
 	@ConditionalOnClass(name = "org.apache.catalina.valves.AccessLogValve")
-	TomcatAccessLogCustomizer tomcatManagementAccessLogCustomizer() {
-		return new TomcatAccessLogCustomizer();
+	static class TomcatConfiguration {
+
+		@Bean
+		TomcatAccessLogCustomizer tomcatManagementAccessLogCustomizer() {
+			return new TomcatAccessLogCustomizer();
+		}
+
 	}
 
-	@Bean
+	@Configuration(proxyBeanMethods = false)
 	@ConditionalOnClass(name = "org.eclipse.jetty.server.Server")
-	JettyAccessLogCustomizer jettyManagementAccessLogCustomizer() {
-		return new JettyAccessLogCustomizer();
+	static class JettyConfiguration {
+
+		@Bean
+		JettyAccessLogCustomizer jettyManagementAccessLogCustomizer() {
+			return new JettyAccessLogCustomizer();
+		}
+
 	}
 
 	@Configuration(proxyBeanMethods = false)

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

@@ -144,24 +144,6 @@ PropertiesFlywayConnectionDetails flywayConnectionDetails() {
 			return new PropertiesFlywayConnectionDetails(this.properties);
 		}
 
-		@Bean
-		@ConditionalOnClass(name = "org.flywaydb.database.sqlserver.SQLServerConfigurationExtension")
-		SqlServerFlywayConfigurationCustomizer sqlServerFlywayConfigurationCustomizer() {
-			return new SqlServerFlywayConfigurationCustomizer(this.properties);
-		}
-
-		@Bean
-		@ConditionalOnClass(name = "org.flywaydb.database.oracle.OracleConfigurationExtension")
-		OracleFlywayConfigurationCustomizer oracleFlywayConfigurationCustomizer() {
-			return new OracleFlywayConfigurationCustomizer(this.properties);
-		}
-
-		@Bean
-		@ConditionalOnClass(name = "org.flywaydb.database.postgresql.PostgreSQLConfigurationExtension")
-		PostgresqlFlywayConfigurationCustomizer postgresqlFlywayConfigurationCustomizer() {
-			return new PostgresqlFlywayConfigurationCustomizer(this.properties);
-		}
-
 		@Bean
 		Flyway flyway(FlywayConnectionDetails connectionDetails, ResourceLoader resourceLoader,
 				ObjectProvider<DataSource> dataSource, @FlywayDataSource ObjectProvider<DataSource> flywayDataSource,
@@ -355,6 +337,40 @@ public FlywayMigrationInitializer flywayInitializer(Flyway flyway,
 			return new FlywayMigrationInitializer(flyway, migrationStrategy.getIfAvailable());
 		}
 
+		@ConditionalOnClass(name = "org.flywaydb.database.sqlserver.SQLServerConfigurationExtension")
+		@Configuration(proxyBeanMethods = false)
+		static class SqlServerConfiguration {
+
+			@Bean
+			SqlServerFlywayConfigurationCustomizer sqlServerFlywayConfigurationCustomizer(FlywayProperties properties) {
+				return new SqlServerFlywayConfigurationCustomizer(properties);
+			}
+
+		}
+
+		@Configuration(proxyBeanMethods = false)
+		@ConditionalOnClass(name = "org.flywaydb.database.oracle.OracleConfigurationExtension")
+		static class OracleConfiguration {
+
+			@Bean
+			OracleFlywayConfigurationCustomizer oracleFlywayConfigurationCustomizer(FlywayProperties properties) {
+				return new OracleFlywayConfigurationCustomizer(properties);
+			}
+
+		}
+
+		@ConditionalOnClass(name = "org.flywaydb.database.postgresql.PostgreSQLConfigurationExtension")
+		@Configuration(proxyBeanMethods = false)
+		static class PostgresqlConfiguration {
+
+			@Bean
+			PostgresqlFlywayConfigurationCustomizer postgresqlFlywayConfigurationCustomizer(
+					FlywayProperties properties) {
+				return new PostgresqlFlywayConfigurationCustomizer(properties);
+			}
+
+		}
+
 	}
 
 	private static class LocationResolver {