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

@@ -70,15 +70,19 @@
  */
 public abstract class ArchitectureCheck extends DefaultTask {
 
+	private static final String CONDITIONAL_ON_CLASS_ANNOTATION = "org.springframework.boot.autoconfigure.condition.ConditionalOnClass";
+
 	private FileCollection classes;
 
 	public ArchitectureCheck() {
 		getOutputDirectory().convention(getProject().getLayout().getBuildDirectory().dir(getName()));
+		getConditionalOnClassAnnotation().convention(CONDITIONAL_ON_CLASS_ANNOTATION);
 		getRules().addAll(getProhibitObjectsRequireNonNull().convention(true)
 			.map(whenTrue(ArchitectureRules::noClassesShouldCallObjectsRequireNonNull)));
 		getRules().addAll(ArchitectureRules.standard());
 		getRules().addAll(whenMainSources(
-				() -> Collections.singletonList(ArchitectureRules.allBeanMethodsShouldReturnNonPrivateType())));
+				() -> List.of(ArchitectureRules.allBeanMethodsShouldReturnNonPrivateType(), ArchitectureRules
+					.allBeanMethodsShouldNotHaveConditionalOnClass(getConditionalOnClassAnnotation().get()))));
 		getRuleDescriptions().set(getRules().map(this::asDescriptions));
 	}
 
@@ -186,4 +190,7 @@ final FileTree getInputClasses() {
 	@Input // Use descriptions as input since rules aren't serializable
 	abstract ListProperty<String> getRuleDescriptions();
 
+	@Internal
+	abstract Property<String> getConditionalOnClassAnnotation();
+
 }

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

@@ -108,6 +108,15 @@ static ArchRule allBeanMethodsShouldReturnNonPrivateType() {
 			.allowEmptyShould(true);
 	}
 
+	static ArchRule allBeanMethodsShouldNotHaveConditionalOnClass(String annotationName) {
+		return methodsThatAreAnnotatedWith("org.springframework.context.annotation.Bean").should()
+			.notBeAnnotatedWith(annotationName)
+			.because("@ConditionalOnClass on @Bean methods is ineffective - it doesn't prevent "
+					+ "the method signature from being loaded. Such condition 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/build/architecture/ArchitectureCheckTests.java

@@ -22,9 +22,7 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
-import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
-import java.util.Map;
 import java.util.Set;
 
 import org.gradle.api.tasks.SourceSet;
@@ -39,6 +37,7 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.EnumSource;
 
+import org.springframework.boot.build.architecture.annotations.TestConditionalOnClass;
 import org.springframework.util.ClassUtils;
 import org.springframework.util.FileSystemUtils;
 import org.springframework.util.StringUtils;
@@ -180,7 +179,7 @@ void whenClassCallsObjectsRequireNonNullWithMessageShouldFailAndWriteReport(Task
 	void whenClassCallsObjectsRequireNonNullWithMessageAndProhibitObjectsRequireNonNullIsFalseShouldSucceedAndWriteEmptyReport(
 			Task task) throws IOException {
 		prepareTask(task, "objects/requireNonNullWithString");
-		build(this.gradleBuild.withProhibitObjectsRequireNonNull(task, false), task);
+		build(this.gradleBuild.withProhibitObjectsRequireNonNull(false), task);
 	}
 
 	@ParameterizedTest(name = "{0}")
@@ -195,7 +194,7 @@ void whenClassCallsObjectsRequireNonNullWithSupplierShouldFailAndWriteReport(Tas
 	void whenClassCallsObjectsRequireNonNullWithSupplierAndProhibitObjectsRequireNonNullIsFalseShouldSucceedAndWriteEmptyReport(
 			Task task) throws IOException {
 		prepareTask(task, "objects/requireNonNullWithSupplier");
-		build(this.gradleBuild.withProhibitObjectsRequireNonNull(task, false), task);
+		build(this.gradleBuild.withProhibitObjectsRequireNonNull(false), task);
 	}
 
 	@ParameterizedTest(name = "{0}")
@@ -295,6 +294,25 @@ void whenEnumSourceValueIsSameAsTypeOfMethodsFirstParameterShouldFailAndWriteRep
 				"should not have a value that is the same as the type of the method's first parameter");
 	}
 
+	@Test
+	void whenConditionalOnClassUsedOnBeanMethodsWithMainSourcesShouldFailAndWriteReport() throws IOException {
+		prepareTask(Task.CHECK_ARCHITECTURE_MAIN, "conditionalonclass", "annotations");
+		GradleBuild gradleBuild = this.gradleBuild.withDependencies(SPRING_CONTEXT)
+			.withConditionalOnClassAnnotation(TestConditionalOnClass.class.getName());
+		buildAndFail(gradleBuild, Task.CHECK_ARCHITECTURE_MAIN,
+				"because @ConditionalOnClass on @Bean methods is ineffective - it doesn't prevent"
+						+ " the method signature from being loaded. Such condition 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", "annotations");
+		GradleBuild gradleBuild = this.gradleBuild.withDependencies(SPRING_CONTEXT)
+			.withConditionalOnClassAnnotation(TestConditionalOnClass.class.getName());
+		build(gradleBuild, Task.CHECK_ARCHITECTURE_TEST);
+	}
+
 	private void prepareTask(Task task, String... sourceDirectories) throws IOException {
 		for (String sourceDirectory : sourceDirectories) {
 			FileSystemUtils.copyRecursively(
@@ -310,7 +328,7 @@ private void prepareTask(Task task, String... sourceDirectories) throws IOExcept
 	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)).as(buildResult.getOutput()).contains(":" + task);
 			assertThat(task.getFailureReport(gradleBuild.getProjectDir())).isEmpty();
 		}
 		catch (UnexpectedBuildFailure ex) {
@@ -326,7 +344,7 @@ 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)).as(buildResult.getOutput()).contains(":" + task);
 			assertThat(task.getFailureReport(gradleBuild.getProjectDir())).contains(messages);
 		}
 		catch (UnexpectedBuildSuccess ex) {
@@ -371,7 +389,9 @@ private static final class GradleBuild {
 
 		private final Set<String> dependencies = new LinkedHashSet<>();
 
-		private final Map<Task, Boolean> prohibitObjectsRequireNonNull = new LinkedHashMap<>();
+		private String conditionalOnClassAnnotation;
+
+		private Boolean prohibitObjectsRequireNonNull;
 
 		private GradleBuild(Path projectDir) {
 			this.projectDir = projectDir;
@@ -381,8 +401,13 @@ Path getProjectDir() {
 			return this.projectDir;
 		}
 
-		GradleBuild withProhibitObjectsRequireNonNull(Task task, boolean prohibitObjectsRequireNonNull) {
-			this.prohibitObjectsRequireNonNull.put(task, prohibitObjectsRequireNonNull);
+		GradleBuild withConditionalOnClassAnnotation(String annotationName) {
+			this.conditionalOnClassAnnotation = annotationName;
+			return this;
+		}
+
+		GradleBuild withProhibitObjectsRequireNonNull(Boolean prohibitObjectsRequireNonNull) {
+			this.prohibitObjectsRequireNonNull = prohibitObjectsRequireNonNull;
 			return this;
 		}
 
@@ -417,13 +442,20 @@ private GradleRunner prepareRunner(String... arguments) throws IOException {
 				for (String dependency : this.dependencies) {
 					buildFile.append("    implementation '%s'\n".formatted(dependency));
 				}
-				buildFile.append("}\n");
+				buildFile.append("}\n\n");
+			}
+			for (Task task : Task.values()) {
+				buildFile.append(task).append(" {");
+				if (this.conditionalOnClassAnnotation != null) {
+					buildFile.append("\n    conditionalOnClassAnnotation = ")
+						.append(StringUtils.quote(this.conditionalOnClassAnnotation));
+				}
+				if (this.prohibitObjectsRequireNonNull != null) {
+					buildFile.append("\n    prohibitObjectsRequireNonNull = ")
+						.append(this.prohibitObjectsRequireNonNull);
+				}
+				buildFile.append("\n}\n\n");
 			}
-			this.prohibitObjectsRequireNonNull.forEach((task, prohibitObjectsRequireNonNull) -> buildFile.append(task)
-				.append(" {\n")
-				.append("    prohibitObjectsRequireNonNull = ")
-				.append(prohibitObjectsRequireNonNull)
-				.append("\n}\n\n"));
 			Files.writeString(this.projectDir.resolve("build.gradle"), buildFile, StandardCharsets.UTF_8);
 			return GradleRunner.create()
 				.withProjectDir(this.projectDir.toFile())

buildSrc/src/test/java/org/springframework/boot/build/architecture/annotations/TestConditionalOnClass.java

@@ -0,0 +1,36 @@
+/*
+ * 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.annotations;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * {@code @ConditionalOnClass} analogue for architecture checks.
+ *
+ */
+@Target({ ElementType.TYPE, ElementType.METHOD })
+@Retention(RetentionPolicy.RUNTIME)
+public @interface TestConditionalOnClass {
+
+	Class<?>[] value() default {};
+
+	String[] name() default {};
+
+}

buildSrc/src/test/java/org/springframework/boot/build/architecture/conditionalonclass/OnBeanMethod.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;
+
+import org.springframework.boot.build.architecture.annotations.TestConditionalOnClass;
+import org.springframework.context.annotation.Bean;
+
+class OnBeanMethod {
+
+	@Bean
+	@TestConditionalOnClass(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)