fix(bigquery-jdbc): avoid NPE for null values in Storage Write API batch insert - #14083
fix(bigquery-jdbc): avoid NPE for null values in Storage Write API batch insert#14083rachitsgh wants to merge 2 commits into
Conversation
…tch insert BigQueryPreparedStatement.bulkInsertWithWriteAPI threw a NullPointerException when a batch parameter was null: STRING columns called toString() on the null value directly, and other column types serialized null as the literal string "null" instead of a JSON null. Both cases now write a proper JSON null. Fixes googleapis#14066 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request addresses a NullPointerException that occurs during Storage Write API batch inserts when a parameter value is null, specifically handling it by inserting a JsonNull.INSTANCE into the JSON row representation. A regression test was also introduced to verify this fix. The review feedback suggests improving the test implementation by avoiding catching exceptions from the reflective method call and instead ensuring the method can complete normally through proper mocking.
| try { | ||
| bulkInsertMethod.invoke(preparedStatement, mockWriteClient); | ||
| } catch (InvocationTargetException e) { | ||
| assertFalse( | ||
| e.getCause() instanceof NullPointerException, | ||
| "Row construction should not NPE on a null STRING parameter, but got: " + e.getCause()); | ||
| } |
There was a problem hiding this comment.
Avoid catching NullPointerException and treating it as 'Expected' in tests, as it indicates a fragile testing pattern. Instead, mock necessary dependencies to allow the method to complete normally, or use Assertions.assertThrows if the exception is truly expected as part of the test scenario.
bulkInsertMethod.invoke(preparedStatement, mockWriteClient);References
- Avoid catching
NullPointerExceptionand treating it as 'Expected' in tests, as it indicates a fragile testing pattern. Instead, mock necessary dependencies to allow the method to complete normally, or useAssertions.assertThrowsif the exception is truly expected as part of the test scenario.
There was a problem hiding this comment.
Good catch — replaced the try/catch on InvocationTargetException with a full round trip through bulkInsertWithWriteAPI against an in-process fake Storage Write service (MockBigQueryWrite + gax-grpc testlib). The test now asserts the method completes normally (assertDoesNotThrow) rather than inspecting which exception type comes out.
…nd trip Address review feedback on catching NullPointerException as an "expected" outcome in the regression test. Replace the reflection-based try/catch with a full round trip through bulkInsertWithWriteAPI against an in-process fake Storage Write service (MockBigQueryWrite + gax-grpc testlib), so the test now asserts the method completes normally instead of merely checking which exception type comes out. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
BigQueryPreparedStatement.bulkInsertWithWriteAPIthrew aNullPointerExceptionwhen a batch parameter wasnull:STRINGcolumns called.toString()on the null value directly, and other column types serialized null as the literal string"null"instead of a JSON null.JsonNull.INSTANCE.Fixes #14066
Test plan
BigQueryPreparedStatementBulkInsertNullValueTestreproducing the reported NPE against the fixjava-bigquery-jdbcunit test suite (1122 tests) — all pass, no regressions