Make equality checks defensive to null reference #19540

jason-liu475 · 2020-01-06T06:40:16Z

When I used the "inspection code" function of idea, I found that "the equals method of Object is easy to throw a null pointer exception, and a constant or a valued object should be used to call equals".

…, and a constant or a valued object should be used to call equals

pivotal-issuemaster · 2020-01-06T06:40:18Z

@tellmenow Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2020-01-06T08:57:35Z

@jason-liu475 Thank you for signing the Contributor License Agreement!

snicoll

Thanks for the PR but we prefer to apply such automated inspection ourselves as usually, a number of false positives surface. This is the case, with a large number of changes not necessary. There are a few worth discussing though. Can you please revert those I've flagged for a start so that we focus on what seems legit?

snicoll · 2020-01-06T12:41:18Z

.../src/main/java/org/springframework/boot/actuate/management/PlainTextThreadDumpFormatter.java

@@ -82,7 +82,7 @@ private void writeStackTraceElement(PrintWriter writer, StackTraceElement elemen
 		writer.printf("\tat %s%n", element.toString());
 		LockInfo lockInfo = info.getLockInfo();
 		if (firstElement && lockInfo != null) {
-			if (element.getClassName().equals(Object.class.getName()) && element.getMethodName().equals("wait")) {
+			if (element.getClassName().equals(Object.class.getName()) && "wait".equals(element.getMethodName())) {


element.getMethodName() does not return null so that change is not necessary.

snicoll · 2020-01-06T12:41:55Z

.../src/main/java/org/springframework/boot/autoconfigure/web/servlet/DispatcherServletPath.java

@@ -74,7 +74,7 @@ default String getPrefix() {
 	 * @return the path as a servlet URL mapping
 	 */
 	default String getServletUrlMapping() {
-		if (getPath().equals("") || getPath().equals("/")) {
+		if ("".equals(getPath()) || "/".equals(getPath())) {


getPath does not return null.

snicoll · 2020-01-06T12:42:23Z

.../src/main/java/org/springframework/boot/autoconfigure/web/servlet/JerseyApplicationPath.java

@@ -74,7 +74,7 @@ default String getUrlMapping() {
 		if (!path.startsWith("/")) {
 			path = "/" + path;
 		}
-		if (path.equals("/")) {
+		if ("/".equals(path)) {


There is a usage of path right above so that change isn't necessary.

snicoll · 2020-01-06T12:43:11Z

...igure/src/main/java/org/springframework/boot/autoconfigure/web/servlet/WebMvcProperties.java

@@ -265,7 +265,7 @@ public void setLoadOnStartup(int loadOnStartup) {
 		}

 		public String getServletMapping() {
-			if (this.path.equals("") || this.path.equals("/")) {
+			if ("".equals(this.path) || "/".equals(this.path)) {


this.path is initialized to a value so this is not necessary.

snicoll · 2020-01-06T12:44:16Z

...ring-boot-cli/src/main/java/org/springframework/boot/cli/command/archive/ArchiveCommand.java

@@ -306,7 +306,7 @@ private void disableGrabResolvers(List<? extends AnnotatedNode> nodes) {
 			for (AnnotatedNode classNode : nodes) {
 				List<AnnotationNode> annotations = classNode.getAnnotations();
 				for (AnnotationNode node : new ArrayList<>(annotations)) {
-					if (node.getClassNode().getNameWithoutPackage().equals("GrabResolver")) {
+					if ("GrabResolver".equals(node.getClassNode().getNameWithoutPackage())) {


This doesn't return null either.

snicoll · 2020-01-06T12:54:26Z

...ols/spring-boot-maven-plugin/src/main/java/org/springframework/boot/maven/RepackageMojo.java

@@ -210,7 +210,7 @@

 	@Override
 	public void execute() throws MojoExecutionException, MojoFailureException {
-		if (this.project.getPackaging().equals("pom")) {
+		if ("pom".equals(this.project.getPackaging())) {


packaging has a default so there is no need to do that.

snicoll · 2020-01-06T12:54:34Z

...boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/Handler.java

@@ -256,7 +256,7 @@ private int hashCode(String protocol, String file) {

 	@Override
 	protected boolean sameFile(URL u1, URL u2) {
-		if (!u1.getProtocol().equals("jar") || !u2.getProtocol().equals("jar")) {
+		if (!"jar".equals(u1.getProtocol()) || !"jar".equals(u2.getProtocol())) {


protocol is mandatory

snicoll · 2020-01-06T12:55:00Z

...oot-tools/spring-boot-maven-plugin/src/main/java/org/springframework/boot/maven/RunMojo.java

@@ -91,7 +91,7 @@ protected RunArguments resolveJvmArguments() {

 	private boolean isJava13OrLater() {
 		for (Method method : String.class.getMethods()) {
-			if (method.getName().equals("stripIndent")) {
+			if ("stripIndent".equals(method.getName())) {


No need to do this either.

snicoll · 2020-01-06T12:56:10Z

...-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java

@@ -303,7 +303,7 @@ private void initializeEarlyLoggingLevel(ConfigurableEnvironment environment) {

 	private boolean isSet(ConfigurableEnvironment environment, String property) {
 		String value = environment.getProperty(property);
-		return (value != null && !value.equals("false"));
+		return (value != null && !"false".equals(value));


Can be replaced by Boolean.parseBoolean

!"false".equals(value) not equals s.equalsIgnoreCase("true"),eg.If enter an empty string, the former return true but the latter return false.

Yeah, you're right there. Maybe not the best idea. I'll make up my mind as part of polishing this.

snicoll · 2020-01-06T12:56:30Z

...roject/spring-boot/src/main/java/org/springframework/boot/env/RandomValuePropertySource.java

@@ -82,10 +82,10 @@ public Object getProperty(String name) {
 	}

 	private Object getRandomValue(String type) {
-		if (type.equals("int")) {
+		if ("int".equals(type)) {


I don't think those are necessary either.

philwebb · 2020-01-09T19:35:30Z

At least some of these could be considered potential bugs, especially the fact that ...getURI().getScheme() can return null.

See gh-19540

snicoll · 2020-01-13T16:45:37Z

@jason-liu475 thank you for making your first contribution to Spring Boot.

the equals method of Object is easy to throw a null pointer exception…

e9e0fc8

…, and a constant or a valued object should be used to call equals

jason-liu475 changed the base branch from master to 2.2.x January 6, 2020 06:41

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 6, 2020

snicoll requested changes Jan 6, 2020

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jan 6, 2020

liuhuan added 2 commits January 7, 2020 00:43

revert some unnecessary changes

2f771c7

revert some unnecessary changes

b39bd08

philwebb added type: bug A general bug and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Jan 9, 2020

philwebb added this to the 2.1.x milestone Jan 9, 2020

snicoll changed the title ~~the equals method of Object is easy to throw a null pointer exception, and a constant or a valued object should be used to call equals~~ Make equality checks defensive to null reference Jan 13, 2020

snicoll self-assigned this Jan 13, 2020

snicoll pushed a commit that referenced this pull request Jan 13, 2020

Make equality checks defensive to null reference

6d8b849

See gh-19540

snicoll added a commit that referenced this pull request Jan 13, 2020

Polish "Make equality checks defensive to null reference"

8b8a4ee

See gh-19540

This was referenced Jan 13, 2020

Make equality checks defensive to null reference #19685

Closed

Make equality checks defensive to null reference #19686

Closed

snicoll modified the milestones: 2.1.x, 2.1.12 Jan 13, 2020

snicoll closed this in 91fff91 Jan 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make equality checks defensive to null reference #19540

Make equality checks defensive to null reference #19540

jason-liu475 commented Jan 6, 2020

pivotal-issuemaster commented Jan 6, 2020

pivotal-issuemaster commented Jan 6, 2020

snicoll left a comment

snicoll Jan 6, 2020

snicoll Jan 6, 2020

snicoll Jan 6, 2020

snicoll Jan 6, 2020

snicoll Jan 6, 2020

snicoll Jan 6, 2020

snicoll Jan 6, 2020

snicoll Jan 6, 2020

snicoll Jan 6, 2020

jason-liu475 Jan 6, 2020

snicoll Jan 6, 2020

snicoll Jan 6, 2020

philwebb commented Jan 9, 2020

snicoll commented Jan 13, 2020

Make equality checks defensive to null reference #19540

Make equality checks defensive to null reference #19540

Conversation

jason-liu475 commented Jan 6, 2020

pivotal-issuemaster commented Jan 6, 2020

pivotal-issuemaster commented Jan 6, 2020

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philwebb commented Jan 9, 2020

snicoll commented Jan 13, 2020