You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR guards against a nil-pointer panic that occurs when a *T (where T implements driver.Valuer with a value receiver) is passed as a bind argument. A nil *T satisfies the interface but calling fn.Value() dereferences the nil pointer and panics. The fix adds an isNilPointer helper and calls it before fn.Value() in all three bind paths (bindPositional, bindNumeric, bindNamed). It also opportunistically refactors the pre-existing nil-Stringer guard in format() to use the same helper. The core fix is correct and the approach is sound.
Should fix
Missing regression test in tests/issues/: Per the project convention (CLAUDE.md), regression tests for bug fixes must live in tests/issues/issue_1823_test.go. The unit tests added to bind_test.go are valuable and should stay, but they only test the bind() helper in isolation. An integration test exercising the full query path is also needed — one that passes a nil driver.Valuer arg to an actual Exec or Query call — covering both TCP and HTTP protocols, and both the native (clickhouse.Open) and database/sql (clickhouse_std) API surfaces.
Nits
nit: The refactoring of the fmt.Stringer case in format() is a subtle behavioral change. The original code also checked v.Type().Elem().Implements(...), which meant a nil *T where String() is a pointer receiver (only *T satisfies Stringer, not T) would NOT get the "NULL" short-circuit and would fall through to v.String(). The new isNilPointer(v) returns "NULL" for all nil pointers unconditionally. This is arguably more correct (calling a nil pointer receiver panics unless the method explicitly handles nil), but the change is worth a one-line comment so future readers understand why the elem-implements check was dropped.
nit (pre-existing, not introduced here): bindPositional (line 128) and bindNumeric (line 170) both do return "", nil instead of return "", err when fn.Value() fails. The caller silently sees a success with an empty string rather than the error. Worth a follow-up fix — noting it here so it doesn't get buried.
Verdict
Approve — the nil-pointer guard is correct and the unit tests confirm the fix. Add the tests/issues/issue_1823_test.go integration test before or shortly after merge to meet the project's regression-test convention.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Solve #1823