WIP: Use checker framework to check for nulls #373
Conversation
| @@ -33,6 +33,7 @@ public AwsWrapperProperty( | |||
| this(name, defaultValue, description, required, (String[]) null); | |||
| } | |||
|
|
|||
| @SuppressWarnings({"nullness:argument","nullness:assignment"}) | |||
| public AwsWrapperProperty( | |||
| String name, | |||
There was a problem hiding this comment.
Probably it makes sense to make it @nonnull
There was a problem hiding this comment.
There are a number of places where the default value is null.
We would have to change the code to be compliant.
| final String methodName, | ||
| final JdbcCallable<T, E> jdbcMethodFunc, | ||
| final Object[] jdbcMethodArgs) | ||
| final @Nullable Class<T> resultClass, |
There was a problem hiding this comment.
I believe that @nonnull makes more sense here.
There was a problem hiding this comment.
Agreed, but I'm fairly certain we call it somewhere with a null
| @@ -222,10 +232,10 @@ public void init( | |||
| this.plugins.add(defaultPlugin); | |||
| } | |||
|
|
|||
| protected <T, E extends Exception> T executeWithSubscribedPlugins( | |||
| protected @Nullable <T, E extends Exception> T executeWithSubscribedPlugins( | |||
| final String methodName, | |||
| @@ -26,7 +27,7 @@ | |||
| * An object representing connection info for a given host. Modifiable fields are thread-safe to support sharing this | |||
| * object with the EFM monitor thread. | |||
| */ | |||
| public class HostSpec { | |||
| public final class HostSpec { | |||
There was a problem hiding this comment.
Would mind to explain why this class is final? Usually final classes are used in sensitive context like identity, user permissions, etc where altering class behaviour (by inheriting a new class) may create a vulnerability.
There was a problem hiding this comment.
So if you don't declare this final than a class that extends this can create a constructor that does not initialize all the fields
| @@ -100,7 +103,7 @@ public static void setPropertyOnTarget( | |||
| () -> | |||
| Messages.get( | |||
| "PropertyUtils.failedToSetPropertyWithReason", | |||
| new Object[] {propName, target.getClass(), ex.getCause().getMessage()})); | |||
| new Object[] {propName, target.getClass() == null ? Messages.emptyArgs : target.getClass(), ex.getCause() == null ? Messages.emptyArgs : ex.getCause().getMessage()})); | |||
There was a problem hiding this comment.
target == null || target.getClass() == null ?
We want to make sure we haven't introduced any subtle bugs by passing nulls where we shouldn't be.
Also reduce the number of places we are checking for nulls
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.