Skip to content

Commit c744ebb

Browse files
scordioscordio
committed
Decorate all Assert implementations with @CheckReturnValue
Signed-off-by: Stefano Cordio <[email protected]>
1 parent 4b2c6fa commit c744ebb

File tree

14 files changed

+188
-31
lines changed

14 files changed

+188
-31
lines changed

‎build-plugin/spring-boot-maven-plugin/src/intTest/java/org/springframework/boot/maven/AbstractArchiveIntegrationTests.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@
3838
import org.assertj.core.api.ListAssert;
3939
import org.jspecify.annotations.Nullable;
4040

41+
import org.springframework.lang.CheckReturnValue;
42+
4143
import static org.assertj.core.api.Assertions.assertThat;
4244
import static org.assertj.core.api.Assertions.contentOf;
4345

@@ -172,6 +174,7 @@ JarAssert doesNotHaveEntryWithNameStartingWith(String prefix) {
172174
return this;
173175
}
174176

177+
@CheckReturnValue
175178
ListAssert<String> entryNamesInPath(String path) {
176179
List<String> matches = new ArrayList<>();
177180
withJarFile((jarFile) -> withEntries(jarFile,

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@
6969
* @author Phillip Webb
7070
* @author Dmytro Nosan
7171
* @author Moritz Halbritter
72+
* @author Stefano Cordio
7273
*/
7374
public abstract class ArchitectureCheck extends DefaultTask {
7475

@@ -85,6 +86,8 @@ public ArchitectureCheck() {
8586
getRules().addAll(whenMainSources(() -> List
8687
.of(ArchitectureRules.allBeanMethodsShouldReturnNonPrivateType(), ArchitectureRules
8788
.allBeanMethodsShouldNotHaveConditionalOnClassAnnotation(getConditionalOnClassAnnotation().get()))));
89+
getRules().addAll(whenMainSources(() -> Collections.singletonList(
90+
ArchitectureRules.allCustomAssertionMethodsNotReturningSelfShouldBeAnnotatedWithCheckReturnValue())));
8891
getRules().addAll(and(getNullMarkedEnabled(), isMainSourceSet()).map(whenTrue(() -> Collections.singletonList(
8992
ArchitectureRules.packagesShouldBeAnnotatedWithNullMarked(getNullMarkedIgnoredPackages().get())))));
9093
getRuleDescriptions().set(getRules().map(this::asDescriptions));

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

Lines changed: 46 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363

6464
import org.springframework.beans.factory.config.BeanDefinition;
6565
import org.springframework.context.annotation.Role;
66+
import org.springframework.lang.CheckReturnValue;
6667
import org.springframework.util.ResourceUtils;
6768

6869
/**
@@ -75,6 +76,7 @@
7576
* @author Phillip Webb
7677
* @author Ngoc Nhan
7778
* @author Moritz Halbritter
79+
* @author Stefano Cordio
7880
*/
7981
final class ArchitectureRules {
8082

@@ -139,6 +141,30 @@ static ArchRule allBeanMethodsShouldNotHaveConditionalOnClassAnnotation(String a
139141
.allowEmptyShould(true);
140142
}
141143

144+
static ArchRule allCustomAssertionMethodsNotReturningSelfShouldBeAnnotatedWithCheckReturnValue() {
145+
return ArchRuleDefinition.methods()
146+
.that()
147+
.areDeclaredInClassesThat()
148+
.implement("org.assertj.core.api.Assert")
149+
.and()
150+
.arePublic()
151+
.and(dontReturnSelfType())
152+
.should()
153+
.beAnnotatedWith(CheckReturnValue.class)
154+
.allowEmptyShould(true);
155+
}
156+
157+
private static DescribedPredicate<JavaMethod> dontReturnSelfType() {
158+
return DescribedPredicate.describe("don't return self type",
159+
(method) -> !method.getRawReturnType().equals(method.getOwner())
160+
|| isOverridingMethodNotReturningSelfType(method));
161+
}
162+
163+
private static boolean isOverridingMethodNotReturningSelfType(JavaMethod method) {
164+
return superMethods(method).anyMatch((superMethod) -> isOverridden(superMethod, method)
165+
&& !superMethod.getOwner().equals(method.getRawReturnType()));
166+
}
167+
142168
private static ArchRule allPackagesShouldBeFreeOfTangles() {
143169
return SlicesRuleDefinition.slices()
144170
.matching("(**)")
@@ -554,39 +580,39 @@ public void check(JavaPackage item, ConditionEvents events) {
554580
};
555581
}
556582

557-
private static class OverridesPublicMethod<T extends JavaMember> extends DescribedPredicate<T> {
583+
private static Stream<JavaMethod> superMethods(JavaMethod method) {
584+
Stream<JavaMethod> superClassMethods = method.getOwner()
585+
.getAllRawSuperclasses()
586+
.stream()
587+
.flatMap((superClass) -> superClass.getMethods().stream());
588+
Stream<JavaMethod> interfaceMethods = method.getOwner()
589+
.getAllRawInterfaces()
590+
.stream()
591+
.flatMap((iface) -> iface.getMethods().stream());
592+
return Stream.concat(superClassMethods, interfaceMethods);
593+
}
558594

559-
OverridesPublicMethod() {
560-
super("overrides public method");
561-
}
595+
private static boolean isOverridden(JavaMethod superMethod, JavaMethod method) {
596+
return superMethod.getName().equals(method.getName())
597+
&& superMethod.getRawParameterTypes().size() == method.getRawParameterTypes().size()
598+
&& superMethod.getDescriptor().equals(method.getDescriptor());
599+
}
600+
601+
private static final class OverridesPublicMethod<T extends JavaMember> implements Predicate<T> {
562602

563603
@Override
564604
public boolean test(T member) {
565605
if (!(member instanceof JavaMethod javaMethod)) {
566606
return false;
567607
}
568-
Stream<JavaMethod> superClassMethods = member.getOwner()
569-
.getAllRawSuperclasses()
570-
.stream()
571-
.flatMap((superClass) -> superClass.getMethods().stream());
572-
Stream<JavaMethod> interfaceMethods = member.getOwner()
573-
.getAllRawInterfaces()
574-
.stream()
575-
.flatMap((iface) -> iface.getMethods().stream());
576-
return Stream.concat(superClassMethods, interfaceMethods)
608+
return superMethods(javaMethod)
577609
.anyMatch((superMethod) -> isPublic(superMethod) && isOverridden(superMethod, javaMethod));
578610
}
579611

580-
private boolean isPublic(JavaMethod method) {
612+
private static boolean isPublic(JavaMethod method) {
581613
return method.getModifiers().contains(JavaModifier.PUBLIC);
582614
}
583615

584-
private boolean isOverridden(JavaMethod superMethod, JavaMethod method) {
585-
return superMethod.getName().equals(method.getName())
586-
&& superMethod.getRawParameterTypes().size() == method.getRawParameterTypes().size()
587-
&& superMethod.getDescriptor().equals(method.getDescriptor());
588-
}
589-
590616
}
591617

592618
}

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,16 @@
5656
* @author Scott Frederick
5757
* @author Ivan Malutin
5858
* @author Dmytro Nosan
59+
* @author Stefano Cordio
5960
*/
6061
class ArchitectureCheckTests {
6162

63+
private static final String ASSERTJ_CORE = "org.assertj:assertj-core:3.27.4";
64+
6265
private static final String SPRING_CONTEXT = "org.springframework:spring-context:6.2.9";
6366

67+
private static final String SPRING_CORE = "org.springframework:spring-core:6.2.9";
68+
6469
private static final String JUNIT_JUPITER = "org.junit.jupiter:junit-jupiter:5.12.0";
6570

6671
private static final String SPRING_INTEGRATION_JMX = "org.springframework.integration:spring-integration-jmx:6.5.1";
@@ -340,6 +345,22 @@ void whenConditionalOnClassUsedOnBeanMethodsWithTestSourcesShouldSucceedAndWrite
340345
build(gradleBuild, Task.CHECK_ARCHITECTURE_TEST);
341346
}
342347

348+
@Test
349+
void whenCustomAssertionMethodNotReturningSelfIsAnnotatedWithCheckReturnValueSucceedAndWriteEmptyReport()
350+
throws IOException {
351+
prepareTask(Task.CHECK_ARCHITECTURE_MAIN, "assertj/checkReturnValue");
352+
build(this.gradleBuild.withDependencies(ASSERTJ_CORE, SPRING_CORE), Task.CHECK_ARCHITECTURE_MAIN);
353+
}
354+
355+
@Test
356+
void whenCustomAssertionMethodNotReturningSelfIsNotAnnotatedWithCheckReturnValueShouldFailAndWriteReport()
357+
throws IOException {
358+
prepareTask(Task.CHECK_ARCHITECTURE_MAIN, "assertj/noCheckReturnValue");
359+
buildAndFail(this.gradleBuild.withDependencies(ASSERTJ_CORE), Task.CHECK_ARCHITECTURE_MAIN,
360+
"methods that are declared in classes that implement org.assertj.core.api.Assert"
361+
+ " and are public and don't return self type should be annotated with" + " @CheckReturnValue");
362+
}
363+
343364
private void prepareTask(Task task, String... sourceDirectories) throws IOException {
344365
for (String sourceDirectory : sourceDirectories) {
345366
FileSystemUtils.copyRecursively(

‎buildSrc/src/test/java/org/springframework/boot/build/architecture/assertj/checkReturnValue/WithCheckReturnValue.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
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.assertj.checkReturnValue;
18+
19+
import org.assertj.core.api.AbstractAssert;
20+
21+
import org.springframework.lang.CheckReturnValue;
22+
23+
public class WithCheckReturnValue extends AbstractAssert<WithCheckReturnValue, Object> {
24+
25+
WithCheckReturnValue() {
26+
super(null, WithCheckReturnValue.class);
27+
}
28+
29+
@CheckReturnValue
30+
public Object notReturningSelf() {
31+
return new Object();
32+
}
33+
34+
@Override
35+
public WithCheckReturnValue isEqualTo(Object expected) {
36+
return super.isEqualTo(expected);
37+
}
38+
39+
}

‎buildSrc/src/test/java/org/springframework/boot/build/architecture/assertj/noCheckReturnValue/NoCheckReturnValue.java‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
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.assertj.noCheckReturnValue;
18+
19+
import org.assertj.core.api.AbstractAssert;
20+
21+
public class NoCheckReturnValue extends AbstractAssert<NoCheckReturnValue, Object> {
22+
23+
NoCheckReturnValue() {
24+
super(null, NoCheckReturnValue.class);
25+
}
26+
27+
public Object notReturningSelf() {
28+
return new Object();
29+
}
30+
31+
}

‎config/checkstyle/import-control.xml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@
66
<allow pkg="io.micrometer.observation" />
77
<disallow pkg="io.micrometer" />
88

9+
<!-- Improve DevEx with fluent APIs -->
10+
<allow class="org.springframework.lang.CheckReturnValue" />
11+
912
<!-- Use JSpecify for nullability (not Spring) -->
1013
<allow class="org.springframework.lang.Contract" />
1114
<disallow pkg="org.springframework.lang" />

‎core/spring-boot-test/src/main/java/org/springframework/boot/test/context/assertj/ApplicationContextAssert.java‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import org.assertj.core.api.AbstractObjectArrayAssert;
2727
import org.assertj.core.api.AbstractObjectAssert;
2828
import org.assertj.core.api.AbstractThrowableAssert;
29-
import org.assertj.core.api.Assertions;
3029
import org.assertj.core.api.MapAssert;
3130
import org.assertj.core.error.BasicErrorMessageFactory;
3231
import org.jspecify.annotations.Nullable;
@@ -37,6 +36,7 @@
3736
import org.springframework.boot.test.context.runner.ApplicationContextRunner;
3837
import org.springframework.context.ApplicationContext;
3938
import org.springframework.context.ConfigurableApplicationContext;
39+
import org.springframework.lang.CheckReturnValue;
4040
import org.springframework.util.Assert;
4141

4242
import static org.assertj.core.api.Assertions.assertThat;
@@ -224,13 +224,14 @@ public ApplicationContextAssert<C> doesNotHaveBean(String name) {
224224
* @return array assertions for the bean names
225225
* @throws AssertionError if the application context did not start
226226
*/
227+
@CheckReturnValue
227228
public <T> AbstractObjectArrayAssert<?, String> getBeanNames(Class<T> type) {
228229
if (this.startupFailure != null) {
229230
throwAssertionError(contextFailedToStartWhenExpecting(this.startupFailure,
230231
"to get beans names with type:%n <%s>", type));
231232
}
232-
return Assertions.assertThat(getApplicationContext().getBeanNamesForType(type))
233-
.as("Bean names of type <%s> from <%s>", type, getApplicationContext());
233+
return assertThat(getApplicationContext().getBeanNamesForType(type)).as("Bean names of type <%s> from <%s>",
234+
type, getApplicationContext());
234235
}
235236

236237
/**
@@ -249,6 +250,7 @@ public <T> AbstractObjectArrayAssert<?, String> getBeanNames(Class<T> type) {
249250
* @throws AssertionError if the application context contains multiple beans of the
250251
* given type
251252
*/
253+
@CheckReturnValue
252254
public <T> AbstractObjectAssert<?, T> getBean(Class<T> type) {
253255
return getBean(type, Scope.INCLUDE_ANCESTORS);
254256
}
@@ -270,6 +272,7 @@ public <T> AbstractObjectAssert<?, T> getBean(Class<T> type) {
270272
* @throws AssertionError if the application context contains multiple beans of the
271273
* given type
272274
*/
275+
@CheckReturnValue
273276
public <T> AbstractObjectAssert<?, T> getBean(Class<T> type, Scope scope) {
274277
Assert.notNull(scope, "'scope' must not be null");
275278
if (this.startupFailure != null) {
@@ -284,7 +287,7 @@ public <T> AbstractObjectAssert<?, T> getBean(Class<T> type, Scope scope) {
284287
getApplicationContext(), type, names));
285288
}
286289
T bean = (name != null) ? getApplicationContext().getBean(name, type) : null;
287-
return Assertions.assertThat(bean).as("Bean of type <%s> from <%s>", type, getApplicationContext());
290+
return assertThat(bean).as("Bean of type <%s> from <%s>", type, getApplicationContext());
288291
}
289292

290293
private @Nullable String getPrimary(String[] names, Scope scope) {
@@ -330,13 +333,14 @@ private boolean isPrimary(String name, Scope scope) {
330333
* is found
331334
* @throws AssertionError if the application context did not start
332335
*/
336+
@CheckReturnValue
333337
public AbstractObjectAssert<?, Object> getBean(String name) {
334338
if (this.startupFailure != null) {
335339
throwAssertionError(
336340
contextFailedToStartWhenExpecting(this.startupFailure, "to contain a bean of name:%n <%s>", name));
337341
}
338342
Object bean = findBean(name);
339-
return Assertions.assertThat(bean).as("Bean of name <%s> from <%s>", name, getApplicationContext());
343+
return assertThat(bean).as("Bean of name <%s> from <%s>", name, getApplicationContext());
340344
}
341345

342346
/**
@@ -357,6 +361,7 @@ public AbstractObjectAssert<?, Object> getBean(String name) {
357361
* name but a different type
358362
*/
359363
@SuppressWarnings("unchecked")
364+
@CheckReturnValue
360365
public <T> AbstractObjectAssert<?, T> getBean(String name, Class<T> type) {
361366
if (this.startupFailure != null) {
362367
throwAssertionError(contextFailedToStartWhenExpecting(this.startupFailure,
@@ -368,8 +373,8 @@ public <T> AbstractObjectAssert<?, T> getBean(String name, Class<T> type) {
368373
"%nExpecting:%n <%s>%nto contain a bean of name:%n <%s> (%s)%nbut found:%n <%s> of type <%s>",
369374
getApplicationContext(), name, type, bean, bean.getClass()));
370375
}
371-
return Assertions.assertThat((T) bean)
372-
.as("Bean of name <%s> and type <%s> from <%s>", name, type, getApplicationContext());
376+
return assertThat((T) bean).as("Bean of name <%s> and type <%s> from <%s>", name, type,
377+
getApplicationContext());
373378
}
374379

375380
private @Nullable Object findBean(String name) {
@@ -395,6 +400,7 @@ public <T> AbstractObjectAssert<?, T> getBean(String name, Class<T> type) {
395400
* no beans are found
396401
* @throws AssertionError if the application context did not start
397402
*/
403+
@CheckReturnValue
398404
public <T> MapAssert<String, T> getBeans(Class<T> type) {
399405
return getBeans(type, Scope.INCLUDE_ANCESTORS);
400406
}
@@ -414,14 +420,15 @@ public <T> MapAssert<String, T> getBeans(Class<T> type) {
414420
* no beans are found
415421
* @throws AssertionError if the application context did not start
416422
*/
423+
@CheckReturnValue
417424
public <T> MapAssert<String, T> getBeans(Class<T> type, Scope scope) {
418425
Assert.notNull(scope, "'scope' must not be null");
419426
if (this.startupFailure != null) {
420427
throwAssertionError(
421428
contextFailedToStartWhenExpecting(this.startupFailure, "to get beans of type:%n <%s>", type));
422429
}
423-
return Assertions.assertThat(scope.getBeansOfType(getApplicationContext(), type))
424-
.as("Beans of type <%s> from <%s>", type, getApplicationContext());
430+
return assertThat(scope.getBeansOfType(getApplicationContext(), type)).as("Beans of type <%s> from <%s>", type,
431+
getApplicationContext());
425432
}
426433

427434
/**
@@ -434,6 +441,7 @@ public <T> MapAssert<String, T> getBeans(Class<T> type, Scope scope) {
434441
* @return assertions on the cause of the failure
435442
* @throws AssertionError if the application context started without a failure
436443
*/
444+
@CheckReturnValue
437445
public AbstractThrowableAssert<?, ? extends Throwable> getFailure() {
438446
hasFailed();
439447
return assertThat(this.startupFailure);

0 commit comments

Comments
 (0)