Add Check for Objects.requireNonNull Usage and Corresponding Tests
This pull request for issue ticket #41603 introduces a new architecture check for ensuring that Objects.requireNonNull is not used directly. Instead, the Assert.notNull method from Spring Framework should be used. Additionally, this update includes new test cases and necessary files to validate this architecture rule.
Changes:
New Architecture Check:
Added a method to the ArchitectureCheck class to verify that Objects.requireNonNull is not used within the codebase.
Test Cases:
Implemented tests to validate the new architecture check: RequireNonNullUsage: Tests that the direct use of Objects.requireNonNull in the code results in a failure. AssertNotNullUsage: Tests that using Assert.notNull is allowed and does not cause a failure.
File Updates:
Created the necessary files and classes for testing: RequireNonNullUsage: Contains a method using Objects.requireNonNull to trigger the architecture check. AssertNotNullUsage: Contains a method using Assert.notNull to verify that it does not violate the rule.
These changes help enforce best practices for null-checking within the project and ensure that the code adheres to the preferred use of Spring utilities.
Comment From: pivotal-cla
@ivamly Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: pivotal-cla
@ivamly Thank you for signing the Contributor License Agreement!
Comment From: ivamly
I've reviewed the contribution guidelines and considered adding Javadoc comments to ArchitectureCheck.java and ArchitectureCheckTests.java. However, I wasn't sure if it was necessary to include Javadoc for these classes, so I have refrained from adding them for now. If it's required, please let me know, and I will update the code accordingly.
Comment From: ivamly
@wilkinsona I noticed from the build report that the build is failing due to missing newline characters at the end of my files. What should I do to resolve this issue? Do I need to make another commit, update the pull request, or take any other action?
Apologies if this seems like a basic question—I'm still new to this process. Thank you for your help!
Comment From: wilkinsona
Either we can take care of it as part of merging the changes, or you can push another commit to your main branch and it will then automatically update this PR.
Comment From: ivamly
I'll make a commit tonight.
Comment From: wilkinsona
It looks like there's a bug in ArchUnit that's causing it to report many false positives. It trips up on code that uses a method reference such as this:
public void someCodeThatUsesAMethodReference() {
List.of("1", "2", "3").forEach(System.out::println);
}
We'll have to see if there's a workaround or if there's a fix available.
Comment From: ivamly
Thank you for the update. That's interesting. Is there anything I can do? I’m happy to assist if needed.
Comment From: wilkinsona
It isn't a bug in ArchUnit, but an unfortunate side effort of the bytecode that's produced by a method reference. The example above looks like this:
public void someCodeThatUsesAMethodReference();
descriptor: ()V
flags: (0x0001) ACC_PUBLIC
Code:
stack=3, locals=1, args_size=1
0: ldc #15 // String 1
2: ldc #17 // String 2
4: ldc #19 // String 3
6: invokestatic #21 // InterfaceMethod java/util/List.of:(Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)Ljava/util/List;
9: getstatic #27 // Field java/lang/System.out:Ljava/io/PrintStream;
12: dup
13: invokestatic #33 // Method java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object;
16: pop
17: invokedynamic #39, 0 // InvokeDynamic #0:accept:(Ljava/io/PrintStream;)Ljava/util/function/Consumer;
22: invokeinterface #43, 2 // InterfaceMethod java/util/List.forEach:(Ljava/util/function/Consumer;)V
27: return
LineNumberTable:
line 30: 0
line 31: 27
LocalVariableTable:
Start Length Slot Name Signature
0 28 0 this Lorg/springframework/boot/build/architecture/objects/noRequireNonNull/AssertNotNullUsage;
Note 13 where it's calling java.lang.Objects.requireNonNull(Object).
Is there anything I can do? I’m happy to assist if needed.
Thanks for the offer.
Could you please update the PR so that it doesn't prohibit Objects.requireNonNull(Object)? We can still prohibit Objects.requireNonNull(Object, String) with Assert.notNull(Object, String) being the recommend replacement. We should also prohibit Objects.requireNonNull(Object, Supplier) with Assert.notNull(Object, Supplier) being the recommend replacement. These two will probably have to be separate rules as the because reason will be different.
Comment From: ivamly
Thanks for the clarification! This is really interesting. I’ll update the PR within an hour.
Comment From: wilkinsona
Thanks very much, @ivamly.