Adopt jspecify + NullAway null-checking on fdb-relational-api - #4584
Conversation
| return (SQLException) getCause(); | ||
| } | ||
| return new ContextualSQLException(getMessage(), getErrorCode().getErrorCode(), this, errorContext); | ||
| return new ContextualSQLException(Objects.requireNonNullElse(getMessage(), ""), getErrorCode().getErrorCode(), this, errorContext); |
There was a problem hiding this comment.
getMessage() can legitimately return null (e.g. for exceptions constructed without a message), but ContextualSQLException's constructor doesn't tolerate a null message. toSqlException() was passing it through unchecked, so converting such a RelationalException to a SQLException could NPE. Now it falls back to "" via Objects.requireNonNullElse, so the conversion always succeeds.
| public int getJdbcSqlCode() { | ||
| return typeCodeJdbcTypeMap.get(Objects.requireNonNull(getCode())); | ||
| final Integer sqlCode = typeCodeJdbcTypeMap.get(getCode()); | ||
| if (sqlCode == null) { |
There was a problem hiding this comment.
getJdbcSqlCode() used to unbox typeCodeJdbcTypeMap.get(getCode()) directly. If a Code enum constant were ever added without a matching entry in typeCodeJdbcTypeMap, this would NPE on the unboxing with no useful diagnostic. Now the lookup result is null-checked and a clear IllegalStateException naming the missing code is thrown instead.
| return ((DataType) resolutionMap.get(name)).withNullable(isNullable()); | ||
| public DataType resolve(final Map<String, Named> resolutionMap) { | ||
| final Named resolved = resolutionMap.get(name); | ||
| if (resolved == null) { |
There was a problem hiding this comment.
UnresolvedType.resolve() previously checked resolutionMap.containsKey(name) and then called .get(name) separately; since the map is documented to allow null values, a key mapped to an explicit null would pass the containsKey check but return null from get(), NPEing on the subsequent cast/withNullable call. The fix does a single get() and null-checks the result directly, raising a clear internal-error exception if the type genuinely isn't resolvable.
| public void validate(final Options.Name name, @Nullable final Object value) throws SQLException { | ||
| if (!(value instanceof Collection<?>)) { | ||
| throw new SQLException("Option " + name + " should be of a collection type instead of " + value.getClass().getName(), ErrorCode.INVALID_PARAMETER.getErrorCode()); | ||
| throw new SQLException("Option " + name + " should be of a collection type instead of " + (value == null ? "null" : value.getClass().getName()), ErrorCode.INVALID_PARAMETER.getErrorCode()); |
There was a problem hiding this comment.
validate() called value.getClass() in the error path when value failed the instanceof Collection check. Options.Builder#withOption can pass a null value through to this method, which would NPE here instead of surfacing the intended "not a collection" SQLException. Now null is handled explicitly in the message.
| public void validate(final Options.Name name, @Nullable final Object value) throws SQLException { | ||
| if (!(value instanceof List<?>)) { | ||
| throw new SQLException("Option " + name + " should be of a list type instead of " + value.getClass().getName(), ErrorCode.INVALID_PARAMETER.getErrorCode()); | ||
| throw new SQLException("Option " + name + " should be of a list type instead of " + (value == null ? "null" : value.getClass().getName()), ErrorCode.INVALID_PARAMETER.getErrorCode()); |
There was a problem hiding this comment.
Same issue as CollectionContract#validate(): this called value.getClass() unconditionally when the instanceof List check failed, which would NPE on a null option value instead of throwing the intended SQLException. Fixed the same way, by handling null explicitly when building the error message.
| @SuppressWarnings("unchecked") | ||
| public void validate(Options.Name name, Object value) throws SQLException { | ||
| public void validate(Options.Name name, @Nullable Object value) throws SQLException { | ||
| if (value == null) { |
There was a problem hiding this comment.
validate() cast value straight to T and called min.compareTo(val) without a null check. A null option value reaching this contract (e.g. via Options.Builder#withOption) would NPE inside compareTo rather than producing a clean SQLException. Now null is rejected up front with an explicit error.
| case Types.NCHAR: | ||
| case Types.NVARCHAR: | ||
| fieldEquals = actual.getString(i).equals(expected.getString(i)); | ||
| fieldEquals = Objects.equals(actual.getString(i), expected.getString(i)); |
There was a problem hiding this comment.
This comparison called .equals() directly on the result of actual.getString(i)/actual.getObject(i), both of which can legitimately return null for a SQL NULL column. Whenever actual's value was null (even if expected's was also null), this test helper would throw an NPE instead of correctly asserting equality. Now uses Objects.equals(...), which handles both-null and one-null cases correctly. The same fix is applied at the parallel checkPartlyEquals site further down in the file.
| rowNumber, cellRef, expectedArray.size(), i, expected, actual)); | ||
| } | ||
| if (isMap(expectedArray.get(i))) { | ||
| final var actualArrayStruct = actualArrayContent.getStruct(2); |
There was a problem hiding this comment.
actualArrayContent.getStruct(2) can return null when an array element being matched against an expected map is itself NULL. Previously that null was passed straight into matchMap/valueByName/valueByIndex, which would NPE deep inside the matcher instead of producing a readable yaml-test failure. Now it's checked explicitly and reported as a clean ResultSetMatchResult.fail(...) with a descriptive message.
Companion change to the fdb-relational-grpc and fdb-relational-jdbc null-checking adoptions. Wires jspecify + NullAway via net.ltgt.errorprone, scoped to com.apple.foundationdb.relational.api and com.apple.foundationdb.relational.util; javax.annotation.Nonnull/Nullable null-checking usages replaced with jspecify's @nullable across all 9 packages (main, test, testFixtures). javax.annotation.concurrent.Immutable/ NotThreadSafe usages are untouched, so jsr305 remains a compileOnly dependency alongside jspecify. As the shared interface layer, this module's methods are implemented by several already-migrated or not-yet-migrated downstream modules, so compiling with NullAway surfaced many previously undocumented nullable returns/fields across the API surface (RelationalStruct's column accessors, RelationalConnection/RelationalDriver's getPath()/connect(), Continuation.getReason(), Options.getOption(), KeySet's lazily-initialized map, BuildVersion's lazy singleton, etc.), plus a few genuine bugs fixed forward rather than suppressed: - RelationalException.toSqlException() and DataType.resolve()/ getJdbcSqlCode() could dereference/unbox null in edge cases; now guarded with clear exceptions or Objects.requireNonNullElse. - CollectionContract/OrderedCollectionContract/RangeContract.validate() could NPE instead of raising a clean SQLException when a null option value reached them (a real path through Options.Builder#withOption). CollectionContract#fromString now fails fast on an unconvertible element instead of silently admitting nulls into the collection. Running the full repo-wide build surfaced expected cross-module ripple in already-migrated/unmigrated downstream modules that call into this API's now-explicit nullability contracts: fdb-relational-grpc's TypeConversion (nullable value flowing into protobuf column conversion), fdb-relational-jdbc's JDBCRelationalStatement (a suppression that became useless once RelationalDirectAccessStatement stopped needing an explicit @nonnull) and a test helper, fdb-relational-server's FRL and fdb-relational-core's EmbeddedRelationalConnection/EmbeddedRelationalStruct/ RelationalStructAssert (unguarded dereferences of now-nullable getPath()/connect()/getObject()/getString()), and yaml-tests' Matchers and EmbeddedYamlConnectionFactory (same pattern). All fixed with either a null check/Objects.requireNonNull or, where SpotBugs and jspecify annotations aren't mutually recognized across a migrated/unmigrated module boundary, a targeted @SpotBugsSuppressWarnings with justification.
3626ddf to
8185982
Compare
1 participant
11th of a 14-PR stack adopting jspecify + NullAway null-checking, stacked on #4583 (
fdb-record-layer-lucene). Same treatment applied tofdb-relational-api. See inline comments for specific findings.