fix(client-v2): clear error for null in non-nullable Enum column#2932
Open
polyglotAI-bot wants to merge 2 commits into
Open
fix(client-v2): clear error for null in non-nullable Enum column#2932polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
serializeEnumData dereferenced the value before the null check, so inserting null into a non-nullable Enum8/Enum16 column failed the RowBinary serialization path with a confusing NullPointerException. Add a null guard that throws IllegalArgumentException naming the column, consistent with how other unsupported enum values are rejected. Nullable enum columns are unaffected. Fixes: #2931
Member
|
@cursor review |
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in client-v2 RowBinary serialization when a user attempts to insert null into a non-nullable Enum8/Enum16 column (issue #2931). Instead of throwing a confusing NullPointerException, the serializer now fails fast with a clear IllegalArgumentException that identifies the target column and type.
Changes:
- Add a
nullguard inSerializerUtils.serializeEnumData(...)to throw a clearIllegalArgumentExceptionfor non-nullable enum columns. - Add TestNG coverage to verify the new behavior and ensure nullable-enum and normal enum serialization paths remain unchanged.
- Document the bug fix in
CHANGELOG.mdunder0.11.0-rc1.
Compatibility impact: Changes behavior only for previously-crashing invalid input (null into non-nullable enum): it now throws IllegalArgumentException instead of NullPointerException. Valid inserts and nullable-enum behavior are unchanged.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java | Adds a null guard in enum serialization to produce a clear domain error instead of NPE. |
| client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java | Adds regression tests for null into non-nullable enums and checks unaffected paths. |
| CHANGELOG.md | Adds a client-v2 bug-fix entry describing the behavior change and linking the issue. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 507be2b. Configure here.
…y changelog Address review feedback on #2932: - Strengthen testNullIntoNonNullableEnumThrowsIllegalArgument to assert the error message names the offending column and includes the enum type string, not just the generic prefix, so a future refactor that drops either detail fails the test. Use a distinctive `bs_flag` column name (matching the issue repro) so the column-name assertion is unambiguous. - Reword CHANGELOG: serializeEnumData had no null guard (there was no prior null check), avoiding the implication of a pre-existing check.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Fixes #2931.
In
client-v2, insertingnullinto a non-nullableEnum8/Enum16column crashed with a confusingNullPointerExceptioninstead of a clear, actionable error.SerializerUtils.serializeEnumDatainspects the value with threeinstanceofchecks (String/Number/Enum) and, when none match, falls into anelsebranch that dereferencesvalue.getClass(). For anullvalue all three checks arefalse, so control reaches that branch and NPEs atvalue.getClass(). Nullable columns never hit this becauseserializePrimitiveDataearly-returns (writes the null marker) forvalue == null && column.isNullable()before enum serialization is reached; only the non-nullable case falls through.The fix adds a
nullguard at the top ofserializeEnumDatathat throwsIllegalArgumentExceptionnaming the column, consistent with theIllegalArgumentExceptionalready thrown in the same method for other unsupported enum value classes.Changes
client-v2/.../internal/SerializerUtils.java— guardserializeEnumDataagainst anullvalue and throwIllegalArgumentException("Cannot insert null into non-nullable column <name> of type <type>")instead of letting it reach thevalue.getClass()dereference.CHANGELOG.md— bug-fix entry under0.11.0-rc1.Test
client-v2/.../internal/SerializerUtilsTest.java:testNullIntoNonNullableEnumThrowsIllegalArgument—@DataProvider-driven over non-nullableEnum8andEnum16; assertsserializeData(out, null, col)throwsIllegalArgumentExceptionwith the clear message. This fails onmain(throwsNullPointerExceptionatSerializerUtils.java:717) and passes with the fix.testEnumSerializationUnaffectedByNullGuard— contrast case pinning the two modes the bug does not cover:Nullable(Enum8)+nullstill takes the early null-marker path (writes a single0x01byte), and a present value ("S") in a non-nullableEnum8('B'=1,'S'=2)still serializes to its mapped value (0x02). Both pass with or without the fix.Verified locally (
unitgroup):SerializerUtilsTest,SerializerUtilsTests,SerializerUtilsPrimitiveSerializationTests— 51 tests, 0 failures.changes_checklist.mdwalkthroughNullPointerExceptioninto the intended domain error for an invalid insert; it does not conflatenullwith empty/zero, and it changes no nullable-column behavior.NullPointerException(a bug) toIllegalArgumentException, matching the siblingelsebranch in the same method; no valid path changes exception behavior.null) is handled first; no resources are opened before the throw; the success path is unchanged.Pre-PR validation gate
mainwith NPE, passes on branch)AGENTS.md/changes_checklist.mdserializeData→serializePrimitiveData→serializeEnumData)Compatibility
Behavior changes only for a previously-crashing invalid insert (
nullinto a non-nullable enum): it now throwsIllegalArgumentExceptioninstead ofNullPointerException. Both are unchecked; nullable enum columns and all valid inserts are unaffected.serializeEnumDatais part of the...data_formats.internalpackage (internal API).Cross-language note
The issue notes this is the Java analogue of the original
clickhouse-connect(Python) bug wherenullin a non-nullable Enum surfaced as a low-level type error rather than a clear domain error. This PR only addressesclickhouse-java client-v2.