Skip to content

Commit d045ca2

Browse files
scordioscordio
committed
Decorate all Assert implementations with @CheckReturnValue
Signed-off-by: Stefano Cordio <stefano.cordio@gmail.com>
1 parent 3b98af3 commit d045ca2

File tree

11 files changed

+97
-31
lines changed

11 files changed

+97
-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
@@ -37,6 +37,8 @@
3737
import org.assertj.core.api.AssertProvider;
3838
import org.assertj.core.api.ListAssert;
3939

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

@@ -176,6 +178,7 @@ JarAssert doesNotHaveEntryWithNameStartingWith(String prefix) {
176178
return this;
177179
}
178180

181+
@CheckReturnValue
179182
ListAssert<String> entryNamesInPath(String path) {
180183
List<String> matches = new ArrayList<>();
181184
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
@@ -68,6 +68,7 @@
6868
* @author Phillip Webb
6969
* @author Dmytro Nosan
7070
* @author Moritz Halbritter
71+
* @author Stefano Cordio
7172
*/
7273
public abstract class ArchitectureCheck extends DefaultTask {
7374

@@ -80,6 +81,8 @@ public ArchitectureCheck() {
8081
getRules().addAll(ArchitectureRules.standard());
8182
getRules().addAll(whenMainSources(
8283
() -> Collections.singletonList(ArchitectureRules.allBeanMethodsShouldReturnNonPrivateType())));
84+
getRules().addAll(whenMainSources(() -> Collections.singletonList(
85+
ArchitectureRules.allCustomAssertionMethodsNotReturningSelfShouldBeAnnotatedWithCheckReturnValue())));
8386
getRules().addAll(and(getNullMarked(), isMainSourceSet()).map(whenTrue(
8487
() -> Collections.singletonList(ArchitectureRules.packagesShouldBeAnnotatedWithNullMarked()))));
8588
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

@@ -129,6 +131,30 @@ static ArchRule allBeanMethodsShouldReturnNonPrivateType() {
129131
.allowEmptyShould(true);
130132
}
131133

134+
static ArchRule allCustomAssertionMethodsNotReturningSelfShouldBeAnnotatedWithCheckReturnValue() {
135+
return ArchRuleDefinition.methods()
136+
.that()
137+
.areDeclaredInClassesThat()
138+
.implement("org.assertj.core.api.Assert")
139+
.and()
140+
.arePublic()
141+
.and(dontReturnSelfType())
142+
.should()
143+
.beAnnotatedWith(CheckReturnValue.class)
144+
.allowEmptyShould(true);
145+
}
146+
147+
private static DescribedPredicate<JavaMethod> dontReturnSelfType() {
148+
return DescribedPredicate.describe("don't return self type",
149+
(method) -> !method.getRawReturnType().equals(method.getOwner())
150+
|| isOverridingMethodNotReturningSelfType(method));
151+
}
152+
153+
private static boolean isOverridingMethodNotReturningSelfType(JavaMethod method) {
154+
return superMethods(method).anyMatch((superMethod) -> isOverridden(superMethod, method)
155+
&& !superMethod.getOwner().equals(method.getRawReturnType()));
156+
}
157+
132158
private static ArchRule allPackagesShouldBeFreeOfTangles() {
133159
return SlicesRuleDefinition.slices().matching("(**)").should().beFreeOfCycles();
134160
}
@@ -501,39 +527,39 @@ public void check(JavaPackage item, ConditionEvents events) {
501527
};
502528
}
503529

504-
private static class OverridesPublicMethod<T extends JavaMember> extends DescribedPredicate<T> {
530+
private static Stream<JavaMethod> superMethods(JavaMethod method) {
531+
Stream<JavaMethod> superClassMethods = method.getOwner()
532+
.getAllRawSuperclasses()
533+
.stream()
534+
.flatMap((superClass) -> superClass.getMethods().stream());
535+
Stream<JavaMethod> interfaceMethods = method.getOwner()
536+
.getAllRawInterfaces()
537+
.stream()
538+
.flatMap((iface) -> iface.getMethods().stream());
539+
return Stream.concat(superClassMethods, interfaceMethods);
540+
}
541+
542+
private static boolean isOverridden(JavaMethod superMethod, JavaMethod method) {
543+
return superMethod.getName().equals(method.getName())
544+
&& superMethod.getRawParameterTypes().size() == method.getRawParameterTypes().size()
545+
&& superMethod.getDescriptor().equals(method.getDescriptor());
546+
}
505547

506-
OverridesPublicMethod() {
507-
super("overrides public method");
508-
}
548+
private static final class OverridesPublicMethod<T extends JavaMember> implements Predicate<T> {
509549

510550
@Override
511551
public boolean test(T member) {
512552
if (!(member instanceof JavaMethod javaMethod)) {
513553
return false;
514554
}
515-
Stream<JavaMethod> superClassMethods = member.getOwner()
516-
.getAllRawSuperclasses()
517-
.stream()
518-
.flatMap((superClass) -> superClass.getMethods().stream());
519-
Stream<JavaMethod> interfaceMethods = member.getOwner()
520-
.getAllRawInterfaces()
521-
.stream()
522-
.flatMap((iface) -> iface.getMethods().stream());
523-
return Stream.concat(superClassMethods, interfaceMethods)
555+
return superMethods(javaMethod)
524556
.anyMatch((superMethod) -> isPublic(superMethod) && isOverridden(superMethod, javaMethod));
525557
}
526558

527-
private boolean isPublic(JavaMethod method) {
559+
private static boolean isPublic(JavaMethod method) {
528560
return method.getModifiers().contains(JavaModifier.PUBLIC);
529561
}
530562

531-
private boolean isOverridden(JavaMethod superMethod, JavaMethod method) {
532-
return superMethod.getName().equals(method.getName())
533-
&& superMethod.getRawParameterTypes().size() == method.getRawParameterTypes().size()
534-
&& superMethod.getDescriptor().equals(method.getDescriptor());
535-
}
536-
537563
}
538564

539565
}

‎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);

‎core/spring-boot-test/src/main/java/org/springframework/boot/test/json/JsonContentAssert.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.skyscreamer.jsonassert.comparator.JSONComparator;
4141

4242
import org.springframework.core.io.Resource;
43+
import org.springframework.lang.CheckReturnValue;
4344
import org.springframework.util.ObjectUtils;
4445
import org.springframework.util.StringUtils;
4546
import org.springframework.util.function.ThrowingFunction;
@@ -917,6 +918,7 @@ public JsonContentAssert doesNotHaveEmptyJsonPathValue(CharSequence expression,
917918
* @return a new assertion object whose object under test is the extracted item
918919
* @throws AssertionError if the path is not valid
919920
*/
921+
@CheckReturnValue
920922
public AbstractObjectAssert<?, Object> extractingJsonPathValue(CharSequence expression, Object... args) {
921923
return Assertions.assertThat(new JsonPathValue(expression, args).getValue(false));
922924
}
@@ -929,6 +931,7 @@ public AbstractObjectAssert<?, Object> extractingJsonPathValue(CharSequence expr
929931
* @return a new assertion object whose object under test is the extracted item
930932
* @throws AssertionError if the path is not valid or does not result in a string
931933
*/
934+
@CheckReturnValue
932935
public AbstractCharSequenceAssert<?, String> extractingJsonPathStringValue(CharSequence expression,
933936
Object... args) {
934937
return Assertions.assertThat(extractingJsonPathValue(expression, args, String.class, "a string"));
@@ -942,6 +945,7 @@ public AbstractCharSequenceAssert<?, String> extractingJsonPathStringValue(CharS
942945
* @return a new assertion object whose object under test is the extracted item
943946
* @throws AssertionError if the path is not valid or does not result in a number
944947
*/
948+
@CheckReturnValue
945949
public AbstractObjectAssert<?, Number> extractingJsonPathNumberValue(CharSequence expression, Object... args) {
946950
return Assertions.assertThat(extractingJsonPathValue(expression, args, Number.class, "a number"));
947951
}
@@ -954,6 +958,7 @@ public AbstractObjectAssert<?, Number> extractingJsonPathNumberValue(CharSequenc
954958
* @return a new assertion object whose object under test is the extracted item
955959
* @throws AssertionError if the path is not valid or does not result in a boolean
956960
*/
961+
@CheckReturnValue
957962
public AbstractBooleanAssert<?> extractingJsonPathBooleanValue(CharSequence expression, Object... args) {
958963
return Assertions.assertThat(extractingJsonPathValue(expression, args, Boolean.class, "a boolean"));
959964
}
@@ -968,6 +973,7 @@ public AbstractBooleanAssert<?> extractingJsonPathBooleanValue(CharSequence expr
968973
* @throws AssertionError if the path is not valid or does not result in an array
969974
*/
970975
@SuppressWarnings("unchecked")
976+
@CheckReturnValue
971977
public <E> ListAssert<E> extractingJsonPathArrayValue(CharSequence expression, Object... args) {
972978
return Assertions.assertThat(extractingJsonPathValue(expression, args, List.class, "an array"));
973979
}
@@ -983,6 +989,7 @@ public <E> ListAssert<E> extractingJsonPathArrayValue(CharSequence expression, O
983989
* @throws AssertionError if the path is not valid or does not result in a map
984990
*/
985991
@SuppressWarnings("unchecked")
992+
@CheckReturnValue
986993
public <K, V> MapAssert<K, V> extractingJsonPathMapValue(CharSequence expression, Object... args) {
987994
return Assertions.assertThat(extractingJsonPathValue(expression, args, Map.class, "a map"));
988995
}

‎core/spring-boot-test/src/main/java/org/springframework/boot/test/json/ObjectContentAssert.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import org.assertj.core.api.Assert;
2323
import org.assertj.core.api.InstanceOfAssertFactories;
2424

25+
import org.springframework.lang.CheckReturnValue;
26+
2527
/**
2628
* AssertJ {@link Assert} for {@link ObjectContent}.
2729
*
@@ -41,6 +43,7 @@ protected ObjectContentAssert(A actual) {
4143
* allow chaining of array-specific assertions from this call.
4244
* @return an array assertion object
4345
*/
46+
@CheckReturnValue
4447
public AbstractObjectArrayAssert<?, Object> asArray() {
4548
return asInstanceOf(InstanceOfAssertFactories.ARRAY);
4649
}
@@ -50,6 +53,7 @@ public AbstractObjectArrayAssert<?, Object> asArray() {
5053
* chaining of map-specific assertions from this call.
5154
* @return a map assertion object
5255
*/
56+
@CheckReturnValue
5357
public AbstractMapAssert<?, ?, Object, Object> asMap() {
5458
return asInstanceOf(InstanceOfAssertFactories.MAP);
5559
}

0 commit comments

Comments
 (0)