Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Use checker framework to check for nulls #373

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

WIP: Use checker framework to check for nulls #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1431401
WIP: Use checker framework to check for nulls
davecramer Mar 27, 2023
e122cc1
Latest changes
davecramer Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import com.github.vlsi.gradle.dsl.configureEach
import com.github.vlsi.gradle.properties.dsl.props
import software.amazon.jdbc.buildtools.JavaCommentPreprocessorTask
import com.github.vlsi.gradle.publishing.dsl.simplifyXml
import org.checkerframework.gradle.plugin.CheckerFrameworkExtension

plugins {
java
Expand All @@ -26,6 +27,8 @@ plugins {
id("com.github.vlsi.gradle-extensions")
id("com.github.vlsi.stage-vote-release")
id("com.github.vlsi.ide")
id("org.checkerframework")

}

val versionMajor = project.property("aws-advanced-jdbc-wrapper.version.major")
Expand All @@ -45,6 +48,7 @@ allprojects {
apply(plugin = "signing")
apply(plugin = "maven-publish")
apply(plugin = "com.github.vlsi.ide")
apply(plugin = "org.checkerframework")

tasks {
configureEach<JavaCommentPreprocessorTask> {
Expand Down Expand Up @@ -81,6 +85,16 @@ allprojects {
)
}

configure<CheckerFrameworkExtension> {
checkers = listOf(
"org.checkerframework.checker.nullness.NullnessChecker",
"org.checkerframework.checker.optional.OptionalChecker",
"org.checkerframework.checker.regex.RegexChecker"
)
}
checkerFramework {
excludeTests = true
}
publishing {
publications {
if (project.props.bool("nexus.publish", default = true)) {
Expand Down
7 changes: 7 additions & 0 deletions wrapper/src/main/java/software/amazon/jdbc/AwsWrapperProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public AwsWrapperProperty(
this(name, defaultValue, description, required, (String[]) null);
}

@SuppressWarnings({"nullness:argument","nullness:assignment"})
public AwsWrapperProperty(
String name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it makes sense to make it @nonnull

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a number of places where the default value is null.
We would have to change the code to be compliant.

@Nullable String defaultValue,
Expand All @@ -46,10 +47,12 @@ public AwsWrapperProperty(
this.choices = choices;
}

@SuppressWarnings("nullness:argument")
public @Nullable String getString(Properties properties) {
return properties.getProperty(name, defaultValue);
}

@SuppressWarnings("nullness:argument")
public boolean getBoolean(Properties properties) {
Object value = properties.get(name);
if (value instanceof Boolean) {
Expand All @@ -58,6 +61,7 @@ public boolean getBoolean(Properties properties) {
return Boolean.parseBoolean(properties.getProperty(name, defaultValue));
}

@SuppressWarnings("nullness:argument")
public int getInteger(Properties properties) {
Object value = properties.get(name);
if (value instanceof Integer) {
Expand All @@ -66,6 +70,7 @@ public int getInteger(Properties properties) {
return Integer.parseInt(properties.getProperty(name, defaultValue));
}

@SuppressWarnings("nullness:argument")
public long getLong(Properties properties) {
Object value = properties.get(name);
if (value instanceof Long) {
Expand All @@ -74,6 +79,7 @@ public long getLong(Properties properties) {
return Long.parseLong(properties.getProperty(name, defaultValue));
}

@SuppressWarnings("nullness:argument")
public void set(Properties properties, @Nullable String value) {
if (value == null) {
properties.remove(name);
Expand All @@ -82,6 +88,7 @@ public void set(Properties properties, @Nullable String value) {
}
}

@SuppressWarnings("nullness:argument")
public DriverPropertyInfo toDriverPropertyInfo(Properties properties) {
DriverPropertyInfo propertyInfo = new DriverPropertyInfo(name, getString(properties));
propertyInfo.required = required;
Expand Down
32 changes: 17 additions & 15 deletions wrapper/src/main/java/software/amazon/jdbc/ConnectionPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package software.amazon.jdbc;

import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;
import java.sql.Connection;
import java.sql.SQLException;
import java.util.EnumSet;
Expand All @@ -32,28 +34,28 @@ public interface ConnectionPlugin {
Set<String> getSubscribedMethods();

<T, E extends Exception> T execute(
final Class<T> resultClass,
final Class<E> exceptionClass,
final Object methodInvokeOn,
final String methodName,
final JdbcCallable<T, E> jdbcMethodFunc,
final Object[] jdbcMethodArgs)
final @Nullable Class<T> resultClass,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that @nonnull makes more sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but I'm fairly certain we call it somewhere with a null

final @NonNull Class<E> exceptionClass,
final @NonNull Object methodInvokeOn,
final @NonNull String methodName,
final @NonNull JdbcCallable<T, E> jdbcMethodFunc,
final @Nullable Object @NonNull[] jdbcMethodArgs)
throws E;

Connection connect(
final String driverProtocol,
final HostSpec hostSpec,
final Properties props,
final @NonNull String driverProtocol,
final @NonNull HostSpec hostSpec,
final @NonNull Properties props,
final boolean isInitialConnection,
final JdbcCallable<Connection, SQLException> connectFunc)
final @NonNull JdbcCallable<Connection, SQLException> connectFunc)
throws SQLException;

void initHostProvider(
final String driverProtocol,
final String initialUrl,
final Properties props,
final HostListProviderService hostListProviderService,
final JdbcCallable<Void, SQLException> initHostProviderFunc)
final @NonNull String driverProtocol,
final @NonNull String initialUrl,
final @NonNull Properties props,
final @NonNull HostListProviderService hostListProviderService,
final @Nullable JdbcCallable<Void, SQLException> initHostProviderFunc)
throws SQLException;

OldConnectionSuggestedAction notifyConnectionChanged(EnumSet<NodeChangeOptions> changes);
Expand Down
88 changes: 50 additions & 38 deletions wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,14 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Properties;
import java.util.Set;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.Logger;
import org.checkerframework.checker.initialization.qual.NotOnlyInitialized;
import org.checkerframework.checker.initialization.qual.UnderInitialization;
import org.checkerframework.checker.initialization.qual.UnknownInitialization;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;
import software.amazon.jdbc.cleanup.CanReleaseResources;
Expand Down Expand Up @@ -58,6 +62,7 @@
*/
public class ConnectionPluginManager implements CanReleaseResources {

@SuppressWarnings("method.invocation")
protected static final Map<String, Class<? extends ConnectionPluginFactory>> pluginFactoriesByCode =
new HashMap<String, Class<? extends ConnectionPluginFactory>>() {
{
Expand Down Expand Up @@ -88,21 +93,23 @@ public class ConnectionPluginManager implements CanReleaseResources {

protected Properties props = new Properties();
protected List<ConnectionPlugin> plugins;
protected final ConnectionProvider connectionProvider;
protected final ConnectionWrapper connectionWrapper;
protected PluginService pluginService;
protected final @NotOnlyInitialized @NonNull ConnectionProvider connectionProvider;
protected final @NotOnlyInitialized @NonNull ConnectionWrapper connectionWrapper;
@SuppressWarnings ("initialization.fields.uninitialized")
protected @NotOnlyInitialized PluginService pluginService;

@SuppressWarnings("rawtypes")
protected final Map<String, PluginChainJdbcCallable> pluginChainFuncMap = new HashMap<>();

public ConnectionPluginManager(ConnectionProvider connectionProvider, ConnectionWrapper connectionWrapper) {
@SuppressWarnings("initialization.fields.uninitialized")
public ConnectionPluginManager(ConnectionProvider connectionProvider, @UnderInitialization ConnectionWrapper connectionWrapper) {
this.connectionProvider = connectionProvider;
this.connectionWrapper = connectionWrapper;
}

/**
* This constructor is for testing purposes only.
*/
*/
ConnectionPluginManager(
ConnectionProvider connectionProvider,
Properties props,
Expand All @@ -116,6 +123,7 @@ public ConnectionPluginManager(ConnectionProvider connectionProvider, Connection
/**
* This constructor is for testing purposes only.
*/
@SuppressWarnings("initialization.fields.uninitialized")
ConnectionPluginManager(
ConnectionProvider connectionProvider,
Properties props,
Expand Down Expand Up @@ -149,7 +157,9 @@ public void unlock() {
* @throws SQLException if errors occurred during the execution.
*/
public void init(
PluginService pluginService, Properties props, PluginManagerService pluginManagerService)
@UnderInitialization @NonNull PluginService pluginService,
@NonNull Properties props,
@NonNull PluginManagerService pluginManagerService)
throws SQLException {

this.props = props;
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nonnull ?

final PluginPipeline<T, E> pluginPipeline,
final JdbcCallable<T, E> jdbcMethodFunc)
final @NonNull PluginPipeline<T, E> pluginPipeline,
final @NonNull JdbcCallable<T, E> jdbcMethodFunc)
throws E {

if (pluginPipeline == null) {
Expand All @@ -241,7 +251,9 @@ protected <T, E extends Exception> T executeWithSubscribedPlugins(

if (pluginChainFunc == null) {
pluginChainFunc = this.makePluginChainFunc(methodName);
this.pluginChainFuncMap.put(methodName, pluginChainFunc);
if (pluginChainFunc != null) {
this.pluginChainFuncMap.put(methodName, pluginChainFunc);
}
}

if (pluginChainFunc == null) {
Expand All @@ -251,8 +263,7 @@ protected <T, E extends Exception> T executeWithSubscribedPlugins(
return pluginChainFunc.call(pluginPipeline, jdbcMethodFunc);
}

@Nullable
protected <T, E extends Exception> PluginChainJdbcCallable<T, E> makePluginChainFunc(
protected @Nullable <T, E extends Exception> PluginChainJdbcCallable<T, E> makePluginChainFunc(
final @NonNull String methodName) {

PluginChainJdbcCallable<T, E> pluginChainFunc = null;
Expand All @@ -266,21 +277,22 @@ protected <T, E extends Exception> PluginChainJdbcCallable<T, E> makePluginChain

if (isSubscribed) {
if (pluginChainFunc == null) {
pluginChainFunc = (pipelineFunc, jdbcFunc) -> pipelineFunc.call(plugin, jdbcFunc);
pluginChainFunc = (pipelineFunc, jdbcFunc) -> Objects.requireNonNull(pipelineFunc.call(plugin, jdbcFunc));
} else {
final PluginChainJdbcCallable<T, E> finalPluginChainFunc = pluginChainFunc;
pluginChainFunc = (pipelineFunc, jdbcFunc) ->
pipelineFunc.call(plugin, () -> finalPluginChainFunc.call(pipelineFunc, jdbcFunc));
Objects.requireNonNull(pipelineFunc.call(plugin,
() -> finalPluginChainFunc.call(pipelineFunc, jdbcFunc)));
}
}
}
return pluginChainFunc;
}

protected <E extends Exception> void notifySubscribedPlugins(
final String methodName,
final PluginPipeline<Void, E> pluginPipeline,
final ConnectionPlugin skipNotificationForThisPlugin)
final @NonNull String methodName,
final @Nullable PluginPipeline<Void, E> pluginPipeline,
final @Nullable ConnectionPlugin skipNotificationForThisPlugin)
throws E {

if (pluginPipeline == null) {
Expand All @@ -302,16 +314,16 @@ protected <E extends Exception> void notifySubscribedPlugins(
}
}

public ConnectionWrapper getConnectionWrapper() {
public @NonNull @UnknownInitialization ConnectionWrapper getConnectionWrapper() {
return this.connectionWrapper;
}

public <T, E extends Exception> T execute(
final Class<T> resultType,
final Class<E> exceptionClass,
final Object methodInvokeOn,
final String methodName,
final JdbcCallable<T, E> jdbcMethodFunc,
public @Nullable <T, E extends Exception> T execute(
final @Nullable Class<T> resultType,
final @NonNull Class<E> exceptionClass,
final @NonNull Object methodInvokeOn,
final @NonNull String methodName,
final @NonNull JdbcCallable<T, E> jdbcMethodFunc,
final Object[] jdbcMethodArgs)
throws E {

Expand All @@ -328,25 +340,25 @@ public <T, E extends Exception> T execute(
methodName,
(plugin, func) ->
plugin.execute(
resultType, exceptionClass, methodInvokeOn, methodName, func, jdbcMethodArgs),
resultType, exceptionClass, methodInvokeOn, methodName, Objects.requireNonNull(func), jdbcMethodArgs),
jdbcMethodFunc);
}

public Connection connect(
final String driverProtocol,
final HostSpec hostSpec,
final Properties props,
public @NonNull Connection connect(
final @NonNull String driverProtocol,
final @NonNull HostSpec hostSpec,
final @NonNull Properties props,
final boolean isInitialConnection)
throws SQLException {

try {
return executeWithSubscribedPlugins(
return Objects.requireNonNull(executeWithSubscribedPlugins(
CONNECT_METHOD,
(plugin, func) ->
plugin.connect(driverProtocol, hostSpec, props, isInitialConnection, func),
plugin.connect(driverProtocol, hostSpec, props, isInitialConnection, Objects.requireNonNull(func)),
() -> {
throw new SQLException("Shouldn't be called.");
});
}));
} catch (SQLException | RuntimeException e) {
throw e;
} catch (Exception e) {
Expand All @@ -355,10 +367,10 @@ public Connection connect(
}

public void initHostProvider(
final String driverProtocol,
final String initialUrl,
final Properties props,
final HostListProviderService hostListProviderService)
final @NonNull String driverProtocol,
final @NonNull String initialUrl,
final @NonNull Properties props,
final @NonNull HostListProviderService hostListProviderService)
throws SQLException {

executeWithSubscribedPlugins(
Expand Down Expand Up @@ -425,11 +437,11 @@ public void releaseResources() {

private interface PluginPipeline<T, E extends Exception> {

T call(final @NonNull ConnectionPlugin plugin, final @Nullable JdbcCallable<T, E> jdbcMethodFunc) throws E;
@Nullable T call(final @NonNull ConnectionPlugin plugin, final @Nullable JdbcCallable<T, E> jdbcMethodFunc) throws E;
}

private interface PluginChainJdbcCallable<T, E extends Exception> {

T call(final @NonNull PluginPipeline<T, E> pipelineFunc, final @NonNull JdbcCallable<T, E> jdbcMethodFunc) throws E;
@NonNull T call(final @NonNull PluginPipeline<T, E> pipelineFunc, final @NonNull JdbcCallable<T, E> jdbcMethodFunc) throws E;
}
}
4 changes: 2 additions & 2 deletions wrapper/src/main/java/software/amazon/jdbc/DataSourceConnectionProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@
public class DataSourceConnectionProvider implements ConnectionProvider {

private final @NonNull DataSource dataSource;
private final @Nullable String serverPropertyName;
private final @NonNull String serverPropertyName;
private final @Nullable String portPropertyName;
private final @Nullable String urlPropertyName;
private final @Nullable String databasePropertyName;

public DataSourceConnectionProvider(
final @NonNull DataSource dataSource,
final @Nullable String serverPropertyName,
final @NonNull String serverPropertyName,
final @Nullable String portPropertyName,
final @Nullable String urlPropertyName,
final @Nullable String databasePropertyName) {
Expand Down
Loading