Skip to content

Adopt jspecify + NullAway null-checking on fdb-relational-grpc #4532

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

Draft
arnaud-lacurie wants to merge 2 commits into from
Draft

Adopt jspecify + NullAway null-checking on fdb-relational-grpc #4532

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
0aa604b
Adopt jspecify + NullAway null-checking on fdb-relational-grpc
arnaud-lacurie Aug 27, 2026
bcb9749
Fix SpotBugs finding in fdb-relational-server surfaced by the grpc ch…
arnaud-lacurie Aug 27, 2026
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
23 changes: 22 additions & 1 deletion fdb-relational-grpc/fdb-relational-grpc.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
* limitations under the License.
*/

plugins {
alias(libs.plugins.errorprone)
}

apply from: rootProject.file('gradle/proto.gradle')
apply from: rootProject.file('gradle/publishing.gradle')

Expand Down Expand Up @@ -46,7 +50,7 @@ dependencies {
// server and client, both of which depend on grpc and api.
implementation project(":fdb-relational-api")
implementation project(":fdb-extensions")
compileOnly(libs.jsr305)
compileOnly(libs.jspecify)
implementation(libs.generatedAnnotation)
implementation(libs.grpc.commonProtos)
implementation(libs.grpc.netty)
Expand All @@ -59,12 +63,29 @@ dependencies {
// slf4j binding. Allows RL dependencies to log to same place as Relational
runtimeOnly(libs.log4j.slf4jBinding)

errorprone(libs.errorprone.core)
errorprone(libs.nullaway)

testImplementation(testFixtures(project(":fdb-relational-api")))
testImplementation(libs.bundles.test.impl)
testRuntimeOnly(libs.bundles.test.runtime)
testImplementation(libs.grpc.testing)
}

// Pilot: jspecify + NullAway, scoped to this module only. See @NullMarked package-info.java
// files in com.apple.foundationdb.relational.jdbc / .jdbc.grpc. Generated protobuf/grpc code
// (com.apple.foundationdb.relational.jdbc.grpc.v1) is carved out via UnannotatedSubPackages.
tasks.withType(JavaCompile).configureEach {
options.errorprone {
disableAllChecks = true
error("NullAway")
option("NullAway:AnnotatedPackages", "com.apple.foundationdb.relational.jdbc")
option("NullAway:UnannotatedSubPackages", "com.apple.foundationdb.relational.jdbc.grpc.v1")
option("NullAway:JSpecifyMode", "true")
option("NullAway:AcknowledgeRestrictiveAnnotations", "true")
}
}

javadoc {
source = sourceSets.main.allJava
exclude '**/grpc/**'
Expand Down
21 changes: 13 additions & 8 deletions ...onal-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalArrayFacade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@
import com.apple.foundationdb.relational.util.PositionalIndex;
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;

Check notice on line 45 in fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalArrayFacade.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 80.0% (48/60 lines) | Changed lines: 66.7% (2/3 lines)
import javax.annotation.Nonnull;
import org.jspecify.annotations.Nullable;

import java.sql.SQLException;
import java.sql.SQLFeatureNotSupportedException;
import java.sql.Types;
Expand Down Expand Up @@ -71,7 +72,7 @@
*/
private final Array delegate;

RelationalArrayFacade(@Nonnull ColumnMetadata delegateMetadata, Array array) {
RelationalArrayFacade(ColumnMetadata delegateMetadata, Array array) {
this.delegateMetadata = delegateMetadata;
this.type = Suppliers.memoize(this::computeType);
this.delegate = array;
Expand Down Expand Up @@ -207,43 +208,47 @@
* Column metadata for the Array Structs.
* Package-private so protobuf is available to serializer (in same package).
*/
@Nullable
private ColumnMetadata metadata;

RelationalArrayFacadeBuilder() {
}

@Override
public RelationalArray build() {
if (metadata == null) {

@arnaud-lacurie arnaud-lacurie Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Latent bug caught by NullAway: metadata was never initialized if addStruct() was never called, so build() could previously hand back a RelationalArrayFacade with a null delegateMetadata — which would NPE later, confusingly, inside computeType(). Now fails fast here instead.

throw new IllegalStateException("Cannot build a RelationalArray with no elements added");
}
return new RelationalArrayFacade(this.metadata, this.builder.build());
}

@Override
public RelationalArrayBuilder addAll(@Nonnull Object... value) throws SQLException {
public RelationalArrayBuilder addAll(Object... value) throws SQLException {
throw new SQLFeatureNotSupportedException();
}

@Override
public RelationalArrayBuilder addBytes(@Nonnull byte[] value) throws SQLException {
public RelationalArrayBuilder addBytes(byte[] value) throws SQLException {
throw new SQLFeatureNotSupportedException();
}

@Override
public RelationalArrayBuilder addString(@Nonnull String value) throws SQLException {
public RelationalArrayBuilder addString(String value) throws SQLException {
throw new SQLFeatureNotSupportedException();
}

@Override
public RelationalArrayBuilder addLong(@Nonnull long value) throws SQLException {
public RelationalArrayBuilder addLong(long value) throws SQLException {
throw new SQLFeatureNotSupportedException();
}

@Override
public RelationalArrayBuilder addUuid(@Nonnull final UUID value) throws SQLException {
public RelationalArrayBuilder addUuid(final UUID value) throws SQLException {
throw new SQLFeatureNotSupportedException();
}

@Override
public RelationalArrayBuilder addObject(@Nonnull final Object value) throws SQLException {
public RelationalArrayBuilder addObject(final Object value) throws SQLException {
throw new SQLFeatureNotSupportedException();
}

Expand Down
32 changes: 22 additions & 10 deletions ...-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalResultSetFacade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@
import com.apple.foundationdb.relational.util.ExcludeFromJacocoGeneratedReport;
import com.apple.foundationdb.relational.util.PositionalIndex;
import com.google.common.base.Suppliers;

Check notice on line 37 in fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalResultSetFacade.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 94.5% (121/128 lines) | Changed lines: 100.0% (5/5 lines)
import javax.annotation.Nonnull;
import org.jspecify.annotations.Nullable;

import java.sql.SQLException;
import java.sql.SQLWarning;
import java.sql.Types;
Expand Down Expand Up @@ -103,15 +104,16 @@
return this.wasNull;
}

private <R> R get(int oneBasedIndex, Function<Column, R> s) {
private <R extends @Nullable Object> R get(int oneBasedIndex, Function<Column, R> s) {

@arnaud-lacurie arnaud-lacurie Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Plain <R> implies a non-null upper bound under JSpecify, so this generic helper (reused by both nullable- and non-nullable-returning callers) needs the bound spelled out explicitly to allow nullable instantiation.

int index = PositionalIndex.toProtobuf(oneBasedIndex);
Column column = this.delegate.getRow(rowIndex).getColumns().getColumn(index);
return s.apply(column);
}

@Override
@Nullable
public String getString(final int oneBasedColumn) throws SQLException {
return get(oneBasedColumn, column -> {
Function<Column, @Nullable String> f = column -> {
if (column.hasString()) {
// Do I need to update lastColumnReadWasNull for String type?
// For primitives only?
Expand All @@ -120,7 +122,8 @@
}
this.wasNull = true;
return null;
});
};
return this.<@Nullable String>get(oneBasedColumn, f);

@arnaud-lacurie arnaud-lacurie Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Needed an explicit type witness here — nullness inference for the type parameter didn't propagate automatically from the lambda's target type through this generic call.

}

@Override
Expand Down Expand Up @@ -184,6 +187,8 @@
}

@Override
@SuppressWarnings("NullAway") // NullAway/JSpecify does not currently track @Nullable on array (byte[]) return types across this unannotated-interface boundary; genuinely returns null for a SQL NULL column, same as before this migration.
@Nullable
public byte[] getBytes(int oneBasedColumn) throws SQLException {
return get(oneBasedColumn, column -> {
if (column.hasBinary()) {
Expand All @@ -210,6 +215,7 @@
}

@Override
@Nullable
public String getString(String columnLabel) throws SQLException {
// TOOD: Do getName for now.
return getString(
Expand Down Expand Up @@ -261,6 +267,7 @@
}

@Override
@Nullable
public SQLWarning getWarnings() throws SQLException {
// TODO: Does nothing for now.
return null;
Expand All @@ -277,7 +284,6 @@
}

@Override
@Nonnull
public Continuation getContinuation() throws SQLException {
if (hasNext()) {
throw new SQLException("Continuation can only be returned for the last row");
Expand All @@ -293,8 +299,9 @@
}

@Override
@Nullable
public RelationalStruct getStruct(int oneBasedColumn) throws SQLException {
RelationalStruct s = TypeConversion.getStruct(this.delegate, this.rowIndex, oneBasedColumn);
@Nullable RelationalStruct s = TypeConversion.getStruct(this.delegate, this.rowIndex, oneBasedColumn);
wasNull = s == null;
return s;
}
Expand All @@ -308,16 +315,19 @@

@Override
@ExcludeFromJacocoGeneratedReport
@Nullable
public UUID getUUID(int oneBasedColumn) throws SQLException {
UUID s = TypeConversion.getUUID(this.delegate, this.rowIndex, oneBasedColumn);
@Nullable UUID s = TypeConversion.getUUID(this.delegate, this.rowIndex, oneBasedColumn);
wasNull = s == null;
return s;
}

Check warning on line 323 in fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalResultSetFacade.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Test Gaps

fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalResultSetFacade.java#L319-L323 

[Test Gap] Changed method `getUUID` has not been tested.
https://fdb.teamscale.io/metrics/code/foundationdb-fdb-record-layer/fdb-relational-grpc%2Fsrc%2Fmain%2Fjava%2Fcom%2Fapple%2Ffoundationdb%2Frelational%2Fjdbc%2FRelationalResultSetFacade.java?coverage-mode=test-gap&t=apple%2Farnaud-lacurie%2Fjspecify-nullaway%2Fgrpc%3AHEAD&selection=319-323&merge-request=FoundationDB%2Ffdb-record-layer%2F4532

@Override
@Nullable
@SuppressWarnings("NullAway") // NullAway/JSpecify does not currently narrow @Nullable array (byte[]) locals across this call into TypeConversion.parseVector, despite the explicit null-check above.
public Object getObject(int oneBasedColumn) throws SQLException {
int type = getMetaData().getColumnType(oneBasedColumn);
final Object o;
final @Nullable Object o;
switch (type) {
case Types.VARCHAR:
o = getString(oneBasedColumn);
Expand Down Expand Up @@ -359,7 +369,7 @@
break;
case VECTOR: {
final var bytes = getBytes(oneBasedColumn);
if (wasNull()) {
if (bytes == null) {
return null;
}
o = TypeConversion.parseVector(bytes, ((DataType.VectorType)relationalType).getPrecision());
Expand All @@ -380,16 +390,18 @@
}

@Override
@Nullable
public Object getObject(String columnLabel) throws SQLException {
return getObject(RelationalStruct.getOneBasedPosition(columnLabel, this));
}

@Override
@Nullable
public RelationalArray getArray(int oneBasedColumn) throws SQLException {
int index = PositionalIndex.toProtobuf(oneBasedColumn);
ColumnMetadata columnMetadata = this.delegate.getMetadata().getColumnMetadata().getColumnMetadata(index);
Column column = this.delegate.getRow(rowIndex).getColumns().getColumn(index);
RelationalArrayFacade array = column == null || !column.hasArray() ? null :
@Nullable RelationalArrayFacade array = column == null || !column.hasArray() ? null :
new RelationalArrayFacade(columnMetadata.getArrayMetadata(), column.getArray());
wasNull = array == null;
return array;
Expand Down
2 changes: 0 additions & 2 deletions ...c/main/java/com/apple/foundationdb/relational/jdbc/RelationalResultSetMetaDataFacade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import com.apple.foundationdb.relational.util.ExcludeFromJacocoGeneratedReport;
import com.apple.foundationdb.relational.util.PositionalIndex;

import javax.annotation.Nonnull;
import java.sql.JDBCType;
import java.sql.SQLException;

Expand All @@ -50,7 +49,6 @@ public ArrayMetaData getArrayMetaData(int oneBasedColumn) throws SQLException {
throw new SQLException("Not implemented", ErrorCode.UNSUPPORTED_OPERATION.getErrorCode());
}

@Nonnull
@Override
public DataType.StructType getRelationalDataType() throws SQLException {
throw new SQLException("Not implemented", ErrorCode.UNSUPPORTED_OPERATION.getErrorCode());
Expand Down
9 changes: 4 additions & 5 deletions ...-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalRpcContinuation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,16 @@

import com.apple.foundationdb.relational.api.Continuation;
import com.apple.foundationdb.relational.jdbc.grpc.v1.RpcContinuation;

Check notice on line 25 in fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalRpcContinuation.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 45.0% (9/20 lines) | Changed lines: 100.0% (1/1 lines)
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.jspecify.annotations.Nullable;
import java.util.Objects;

public class RelationalRpcContinuation implements Continuation {
public static final int CURRENT_VERSION = 1;

@Nonnull
private final RpcContinuation proto;

public RelationalRpcContinuation(@Nonnull RpcContinuation proto) {
public RelationalRpcContinuation(RpcContinuation proto) {
this.proto = proto;
}

Expand All @@ -47,6 +45,7 @@

@Nullable
@Override
@SuppressWarnings("NullAway") // NullAway/JSpecify does not currently track @Nullable on array (byte[]) return types across this unannotated-interface boundary; genuinely returns null, same as before this migration.
public byte[] getExecutionState() {
if (proto.hasInternalState()) {
return proto.getInternalState().toByteArray();
Expand All @@ -56,6 +55,7 @@
}

@Override
@Nullable
public Reason getReason() {
if (proto.hasReason()) {
return TypeConversion.toReason(proto.getReason());
Expand All @@ -74,7 +74,6 @@
return proto.getAtEnd();
}

@Nonnull
public RpcContinuation getProto() {
return proto;
}
Expand Down
Loading
Loading