fix(postgresql): Fix NullPointerException comparing or writing a null… - #934
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideEnsure PostgreSQL-specific DataTypes (GenericEnumType, UuidType, InetType, CitextType) safely handle null values in both typeCast() and setSqlValue(), and add unit/integration tests plus documentation and changelog entries to prove and describe the behavior and associated quirks. Sequence diagram for AbstractDataType.compare with null and PostgreSQL DataTypessequenceDiagram
participant Test as GenericEnumTypeTest
participant ADT as AbstractDataType
participant EnumType as GenericEnumType
Test->>ADT: compare(null, nonNullValue)
ADT->>EnumType: typeCast(null)
EnumType-->>ADT: null
ADT->>EnumType: typeCast(nonNullValue)
EnumType-->>ADT: "nonNullValue"
ADT-->>Test: comparison result (no NullPointerException)
Sequence diagram for setSqlValue(null) on PostgreSQL DataTypessequenceDiagram
participant Test as PostgresqlNullableOtherTypesIT
participant UuidType as UuidType
participant PS as PreparedStatement
Test->>UuidType: setSqlValue(null, column, PS)
alt value is null
UuidType->>PS: setNull(column, Types.OTHER)
else value is non-null
UuidType->>PS: setObject(column, getUUID(uuid, PS.getConnection()))
end
PS-->>Test: CLEAN_INSERT succeeds without NullPointerException
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPostgreSQL enum, UUID, inet, and citext types now handle null values safely. Unit and integration tests cover null casting, comparison, SQL NULL binding, and nullable round trips. PostgreSQL enum metadata limitations are documented. ChangesPostgreSQL nullable type handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The Javadoc for CitextType.setSqlValue and InetType.setSqlValue still refer to the parameter as "uuid", which looks like a copy/paste artifact and should be updated to match the actual semantics to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Javadoc for CitextType.setSqlValue and InetType.setSqlValue still refer to the parameter as "uuid", which looks like a copy/paste artifact and should be updated to match the actual semantics to avoid confusion.
## Individual Comments
### Comment 1
<location path="src/main/java/org/dbunit/ext/postgresql/InetType.java" line_range="62-63" />
<code_context>
return resultSet.getString(column);
}
+ /**
+ * {@inheritDoc} Binds sql {@code NULL} when {@code uuid} is
+ * {@code null}, instead of dereferencing it while building the
+ * PGobject.
</code_context>
<issue_to_address>
**suggestion (typo):** Javadoc for InetType#setSqlValue uses `uuid` terminology, which doesn’t match the inet type.
The Javadoc here still refers to "uuid", which is inconsistent with `InetType` and the inet parameter semantics. Please update the documented parameter name to something inet-specific or a generic name like `value` to avoid confusion.
Suggested implementation:
```java
/**
* {@inheritDoc} Binds SQL {@code NULL} when the inet {@code value} is
* {@code null}, instead of dereferencing it while building the
* {@code PGobject}.
*/
public void setSqlValue(Object uuid, int column,
PreparedStatement statement) throws SQLException, TypeCastException {
```
```java
if (uuid == null) {
```
```java
statement.setObject(column, getInet(uuid, statement.getConnection()));
```
If there are other Javadocs or comments in this class (or related types) that still use `uuid` terminology for inet values, they should be updated similarly to use `inet`-specific wording or a generic term like `value` for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/dbunit/ext/postgresql/CitextType.java`:
- Around line 64-69: Correct the type-specific JavaDoc for setSqlValue: in
src/main/java/org/dbunit/ext/postgresql/CitextType.java lines 64-69, replace
UUID terminology with value and describe a citext value; make the equivalent
terminology correction in src/main/java/org/dbunit/ext/postgresql/InetType.java
lines 62-67, describing an inet value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cfe44d60-4f59-4038-a698-e1ce1645dcdd
📒 Files selected for processing (11)
src/changes/changes.xmlsrc/main/java/org/dbunit/ext/postgresql/CitextType.javasrc/main/java/org/dbunit/ext/postgresql/GenericEnumType.javasrc/main/java/org/dbunit/ext/postgresql/InetType.javasrc/main/java/org/dbunit/ext/postgresql/UuidType.javasrc/site/asciidoc/databases/postgresql.adocsrc/test/java/org/dbunit/ext/postgresql/CitextTypeTest.javasrc/test/java/org/dbunit/ext/postgresql/GenericEnumTypeTest.javasrc/test/java/org/dbunit/ext/postgresql/InetTypeTest.javasrc/test/java/org/dbunit/ext/postgresql/PostgresqlNullableOtherTypesIT.javasrc/test/java/org/dbunit/ext/postgresql/UuidTypeTest.java
… enum, uuid, inet, or citext value GenericEnumType.typeCast() had no null guard, so AbstractDataType.compare() threw NullPointerException instead of reporting a mismatch whenever exactly one side of a compared pair was null. UuidType, InetType, and CitextType shared the same two-part gap: an unguarded typeCast(), plus a setSqlValue() override that bypasses typeCast() entirely to call a private PGobject- building helper that also dereferences the value unconditionally. * Return null from typeCast() for a null input in all four classes, matching every other DataType implementation. * Bind sql NULL from setSqlValue() before ever reaching the PGobject- building helper, matching the pattern JsonType already established. * Add unit coverage per class: typeCast(null) returns null, compare(null, nonNullValue) no longer throws (the literal reported crash), and a Mockito-based setSqlValue(null, ...) proof that sql NULL is bound instead of thrown. * Add PostgresqlNullableOtherTypesIT, round-tripping a null uuid/inet/citext row through CLEAN_INSERT against a live PostgreSQL 16 container. GenericEnumType is proven at the unit level only: a separate, pre-existing defect (issue 933, filed but not fixed here) leaves it unreachable for a real table column regardless of null handling, discovered while writing this test. * Document the issue 933 caveat in postgresql.adoc's Known Quirks. Refs: 677 Refs: 930 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XBjSJ2bgVtwqyxGv2nvwP9
05054e2 to
a32bf4e
Compare
… enum, uuid, inet, or citext value
GenericEnumType.typeCast() had no null guard, so AbstractDataType.compare() threw NullPointerException instead of reporting a mismatch whenever exactly one side of a compared pair was null. UuidType, InetType, and CitextType shared the same two-part gap: an unguarded typeCast(), plus a setSqlValue() override that bypasses typeCast() entirely to call a private PGobject- building helper that also dereferences the value unconditionally.
Refs: 677
Refs: 930
Claude-Session: https://claude.ai/code/session_01XBjSJ2bgVtwqyxGv2nvwP9
Summary by Sourcery
Handle PostgreSQL enum, UUID, inet, and citext null values safely to avoid NullPointerExceptions and verify behavior with unit and integration tests.
Bug Fixes:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
NULLsafely.NULLvalues are correctly written to PostgreSQL without errors.Documentation
Tests