From 399ee4019398e52722e42a7edce52547452172f3 Mon Sep 17 00:00:00 2001 From: Rune Flobakk Date: Fri, 11 Apr 2025 16:07:23 +0200 Subject: [PATCH] Ensure all mappers offered in Mappers are nullsafe --- src/main/java/no/digipost/jdbc/Mappers.java | 4 +- src/main/java/no/digipost/jdbc/SqlArray.java | 4 +- .../java/no/digipost/jdbc/MappersTest.java | 46 +++++++++++++------ 3 files changed, 38 insertions(+), 16 deletions(-) diff --git a/src/main/java/no/digipost/jdbc/Mappers.java b/src/main/java/no/digipost/jdbc/Mappers.java index 61609d2..709703d 100644 --- a/src/main/java/no/digipost/jdbc/Mappers.java +++ b/src/main/java/no/digipost/jdbc/Mappers.java @@ -197,7 +197,7 @@ public final class Mappers { /** * Combination of {@link #getTimestamp} and a conversion to an {@link Instant} using {@link Timestamp#toInstant()}. */ - public static final BasicColumnMapper getInstant = getTimestamp.andThen(Timestamp::toInstant); + public static final BasicColumnMapper getInstant = getTimestamp.nullFallthrough().andThen(Timestamp::toInstant); /** * Combination of {@link #getNullableTimestamp} and a conversion to an {@link Instant} * using {@link Timestamp#toInstant()}. @@ -231,7 +231,7 @@ public final class Mappers { * @see #getIntArray * @see #getLongArray */ - public static final BasicColumnMapper getArray = (name, rs) -> getSqlArray.andThen(SqlArray::of).map(name, rs).consume(Array::getArray); + public static final BasicColumnMapper getArray = (name, rs) -> getSqlArray.andThen(SqlArray::of).map(name, rs).consume(a -> a != null ? a.getArray() : null); /** * Gets the value of a given SQL {@code ARRAY} column as an {@code String[]} array. diff --git a/src/main/java/no/digipost/jdbc/SqlArray.java b/src/main/java/no/digipost/jdbc/SqlArray.java index a3b01ac..0d6ad67 100644 --- a/src/main/java/no/digipost/jdbc/SqlArray.java +++ b/src/main/java/no/digipost/jdbc/SqlArray.java @@ -46,7 +46,9 @@ R consume(ThrowingFunction sqlArrayConsumer) throws try { return sqlArrayConsumer.apply(array); } finally { - array.free(); + if (array != null) { + array.free(); + } } } diff --git a/src/test/java/no/digipost/jdbc/MappersTest.java b/src/test/java/no/digipost/jdbc/MappersTest.java index 86f9562..ce14d96 100644 --- a/src/test/java/no/digipost/jdbc/MappersTest.java +++ b/src/test/java/no/digipost/jdbc/MappersTest.java @@ -16,13 +16,17 @@ package no.digipost.jdbc; import com.mockrunner.mock.jdbc.MockResultSet; +import no.digipost.collection.NonEmptyList; import org.junit.jupiter.api.Test; import java.sql.SQLException; +import java.util.NoSuchElementException; import java.util.Optional; import java.util.stream.Stream; -import static uk.co.probablyfine.matchers.StreamMatchers.allMatch; +import static java.lang.reflect.Modifier.isPublic; +import static java.lang.reflect.Modifier.isStatic; +import static no.digipost.DiggCollectors.toNonEmptyList; import static no.digipost.DiggExceptions.applyUnchecked; import static no.digipost.jdbc.Mappers.getBoolean; import static no.digipost.jdbc.Mappers.getByte; @@ -40,28 +44,44 @@ import static no.digipost.jdbc.Mappers.getShort; import static no.digipost.jdbc.ResultSetMock.mockSingleColumnResult; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; +import static org.junit.jupiter.api.Assertions.assertAll; +import static uk.co.probablyfine.matchers.Java8Matchers.whereNot; -public class MappersTest { +class MappersTest { @Test - public void recognizesSQL_NULLsInPrimitiveMappers() throws SQLException { + void recognizesSQL_NULLsInPrimitiveMappers() throws SQLException { try (MockResultSet rs = mockSingleColumnResult("value", new Object[] { null })) { - Stream> results = - Stream.of(getNullableInt, getNullableBoolean, getNullableByte, getNullableDouble, getNullableFloat, getNullableLong, getNullableShort) - .map((NullableColumnMapper nullableMapper) -> applyUnchecked(mapper -> mapper.map("value", rs), nullableMapper)); - assertThat(results, allMatch(is(Optional.empty()))); + assertAll(Stream.of(getNullableInt, getNullableBoolean, getNullableByte, getNullableDouble, getNullableFloat, getNullableLong, getNullableShort) + .map(nullableMapper -> applyUnchecked(mapper -> mapper.map("value", rs), nullableMapper)) + .map(mappedValue -> () -> assertThat(mappedValue, whereNot(Optional::isPresent)))); } } @Test - public void correctlyHandlesResultSetIckySQL_NULLHandling() throws SQLException { + void correctlyHandlesResultSetIckySQL_NULLHandling() throws SQLException { try (MockResultSet rs = mockSingleColumnResult("value", new Object[] { null })) { - Stream results = - Stream.of(getInt, getBoolean, getByte, getDouble, getFloat, getLong, getShort) - .map((BasicColumnMapper basicMapper) -> applyUnchecked(mapper -> mapper.map("value", rs), basicMapper)); - assertThat(results, allMatch(nullValue())); + assertAll(Stream.of(getInt, getBoolean, getByte, getDouble, getFloat, getLong, getShort) + .map(primitiveMapper -> applyUnchecked(mapper -> mapper.map("value", rs), primitiveMapper)) + .map(mappedValue -> () -> assertThat(mappedValue, nullValue()))); + } + } + + @Test + void allBasicColumnMappersAreNullSafe() throws SQLException { + NonEmptyList> publicBasicMappers = Stream.of(Mappers.class.getFields()) + .filter(field -> isStatic(field.getModifiers()) && isPublic(field.getModifiers()) && BasicColumnMapper.class.isAssignableFrom(field.getType())) + .map(publicBasicMapperField -> (BasicColumnMapper) applyUnchecked(publicBasicMapperField::get, Mappers.class)) + .collect(toNonEmptyList()) + .orElseThrow(() -> new NoSuchElementException("found no public " + BasicColumnMapper.class.getSimpleName() + " fields in " + Mappers.class.getName())); + + try (MockResultSet rs = mockSingleColumnResult("value", new Object[] { null })) { + assertAll(publicBasicMappers.stream() + .map(basicMapper -> () -> { + Object mappedValue = applyUnchecked(mapper -> mapper.map("value", rs), basicMapper); + assertThat(mappedValue, nullValue()); + })); } } }