Skip to content

fix: guard nullable primitive column reads with wasNull() - #1

Open
wileyatnelo wants to merge 1 commit into
mainfrom
wiley/wasnull-nullable-primitives
Open

fix: guard nullable primitive column reads with wasNull()#1
wileyatnelo wants to merge 1 commit into
mainfrom
wiley/wasnull-nullable-primitives

Conversation

@wileyatnelo

Copy link
Copy Markdown
Owner

The bug

JDBC's primitive getters — getInt, getLong, getShort, getByte, getDouble, getFloat, getBoolean — return 0/0.0/false for a SQL NULL. Reading a nullable column through one, with no wasNull() check, makes "the value is absent" indistinguishable from "the value is zero".

The generator already got the type right. makeType sets IsNull: !col.NotNull and ktType.String() appends ?. But jdbcGet branched on IsNull only for JSON and UUID columns before falling through to a bare results.get<Type>(n). So the two halves disagreed: a nullable column was emitted as Long? and then filled with a value that could never be null.

That combination is worse than a plainly wrong type. Because the declared type says the value can be null, callers write ?: fallback and x == null branches — and those branches are dead code that never fires. A non-null type would at least have been honest.

Found while auditing nelo/api-v2, where a nullable marketplace_shipping_option.amount (the "ask the carrier for a live rate" sentinel) was reading back as 0. Downstream code detects that state with amount == null, so it silently took the "already priced at zero" path. In that repo the value happens to be discarded before it reaches a customer, but the same table is read correctly as null via Ebean two lines away in ProductRepository.resolveShippingProfiles — one read path returning null and the other 0 for the same row.

The fix

One branch in jdbcGet, plus an isPrimitive() predicate alongside the existing IsTime()/IsUUID()/IsBigDecimal() ones:

if t.IsNull && t.isPrimitive() {
    return fmt.Sprintf(`results.get%s(%d).takeUnless { results.wasNull() }`, t.Name, idx)
}
Reading(
    results.getLong(1).takeUnless { results.wasNull() },  // nullable -> guarded
    results.getLong(2)                                    // NOT NULL -> plain read
)

takeUnless keeps the read a single expression, which is what ResultSet() splices in per constructor argument, and it evaluates the getter before the lambda — so wasNull() still refers to this column, per the JDBC contract.

Untouched: NOT NULL columns, array columns (read via getArray), and getters that already return a reference type (getString, getBigDecimal, getObject), which report NULL as null on their own.

Is this breaking for consumers?

No. No emitted type changes — the field was already Long?. Only the runtime value changes, 0null, and only for rows that were previously returning a wrong answer for a case the declared type already forced every caller to handle. Kotlin that compiles today still compiles.

Verified against the api-v2 monolith: regenerated all 10 sqlc modules with this build and ./gradlew compileKotlin passes. Seven call sites gained the guard (one nullable bigint, six nullable lat/long floats); the NOT NULL booleans and the CAST(... AS bigint) aggregate balances were correctly left alone.

The one thing to check before adopting is any consumer that has come to depend on the 0.

Tests

Unit (internal/core/wasnull_test.go) — jdbcGet for all seven primitives nullable and non-null, plus regression guards that String/BigDecimal/UUID/OffsetDateTime and primitive arrays stay unwrapped.

Integration (examples/.../nulltest) — a new example against real Postgres, covering bigint, integer, smallint, double precision, real, boolean, with NOT NULL controls. Three tests: NULLs read as null, genuine zeros still read as 0, and the two stay distinguishable across a multi-row read.

The zero-valued row is the half that makes this meaningful — without it, a mapper that returned null for everything would also pass.

Confirmed the integration test actually catches the bug by reverting jdbcGet and re-running: 2 of 3 fail with bigint NULL must not read as 0L ==> expected: <null> but was: <0>. The zero-value test correctly still passes.

The example uses the kt-local process plugin, so sqlc diff in CI exercises this branch's generator rather than the released WASM.

Verification run

  • gofmt -l internal plugin — clean
  • make test — pass
  • sqlc diff (v1.28.0, matching CI) — clean, no churn in existing golden files
  • examples: ./gradlew test — 15/15 across 8 classes, against Postgres 11 and MySQL 8
  • api-v2: regenerate + compileKotlin — pass; targeted repository test asserting NULL → null — pass

go vet reports 3 protobuf lock-copy findings; all 3 are pre-existing on main and none are in code this PR touches.

Two unrelated pre-existing bugs found on the way

Both were surfaced by the nulltest example and are out of scope here — the example is deliberately read-only to avoid them. Worth separate issues:

  1. Nullable scalar :one emits invalid Kotlin. A SELECT nullable_col ... :one generates return type Long?? — the :one wrapper appends ? to a type that is already nullable. Does not compile.
  2. Nullable parameters use primitive setters. A Long? parameter emits stmt.setLong(i, x), which does not compile. This is the write-side mirror of the bug fixed here and needs setNull/setObject handling.

Rollout for api-v2

api-v2 pins this plugin as a WASM release, not via sqlc/run-sqlc.sh:

buildSrc/src/main/kotlin/co/nelo/api/GenerateSqlcConfigTask.kt:53pluginUrl.../releases/download/v1.4.0/sqlc-gen-kotlin.wasm, with a pinned sha256 alongside it.

So adopting this needs a new tag with sqlc-gen-kotlin.wasm attached, then bumping that URL and sha256.

JDBC's primitive getters return 0/0.0/false for a SQL NULL, so reading a
nullable column with getLong/getInt/getShort/getByte/getDouble/getFloat/
getBoolean cannot distinguish "absent" from "zero".

The generator already got the *type* right -- makeType sets
`IsNull: !col.NotNull` and ktType.String() appends `?` -- but jdbcGet
branched on IsNull only for JSON and UUID columns before falling through
to a bare `results.get<Type>(n)`. So a nullable column was emitted as
`Long?` and then filled with a value that could never be null.

That combination is worse than a plainly wrong type: the declared type
tells callers to handle null, so they write `?: fallback` or
`x == null` branches that are dead code and never fire.

Guard the primitive getters with wasNull(). takeUnless keeps the read a
single expression, which is what ResultSet() splices in per constructor
argument, and it evaluates the getter before the lambda so wasNull()
still refers to this column.

Not a breaking change for consumers: no emitted type changes, only the
runtime value for rows that were previously returning a wrong answer.
A genuine 0 still reads as 0. NOT NULL columns are untouched, as are
getters that already return a reference type (getString, getBigDecimal,
getObject), which report NULL as null on their own.

Adds a `nulltest` example covering every affected type against a real
Postgres, including the zero-valued control row -- without it a mapper
that returned null for everything would also pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant