feat(postgresql): Add json/jsonb type support to PostgresqlDataTypeFa… - #931
Conversation
Reviewer's GuideAdds dedicated PostgreSQL json/jsonb support via a new JsonType, wires it into PostgresqlDataTypeFactory, and validates behavior with unit, integration, and documentation updates, including correct null handling and json/jsonb-specific behaviors. Sequence diagram for JsonType.setSqlValue handling json/jsonbsequenceDiagram
participant DbUnitClient
participant JsonType
participant PreparedStatement
participant Connection
participant PGobject
DbUnitClient->>JsonType: setSqlValue(value, column, statement)
alt [value is null]
JsonType->>PreparedStatement: setNull(column, Types.OTHER)
else [value is not null]
JsonType->>PreparedStatement: getConnection()
PreparedStatement-->>JsonType: connection
JsonType->>JsonType: getJson(value, connection)
JsonType->>Connection: loadClass(org.postgresql.util.PGobject)
Connection-->>JsonType: PGobjectClass
JsonType->>PGobject: new PGobject()
JsonType->>PGobject: setType(sqlTypeName)
JsonType->>PGobject: setValue(value.toString())
JsonType-->>PreparedStatement: jsonObject
JsonType->>PreparedStatement: setObject(column, jsonObject)
end
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: 26 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 (5)
📝 WalkthroughWalkthroughAdds PostgreSQL ChangesPostgreSQL JSON support
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PostgresqlDataTypeFactory
participant JsonType
participant PostgreSQL
Client->>PostgresqlDataTypeFactory: request datatype for json or jsonb
PostgresqlDataTypeFactory-->>Client: return configured JsonType
Client->>JsonType: bind JSON value
JsonType->>PostgreSQL: bind typed PGobject
PostgreSQL-->>JsonType: return JSON column value
JsonType-->>Client: return string value or null
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In JsonType, consider throwing IllegalArgumentException instead of NullPointerException when sqlTypeName is null to more clearly signal invalid caller input rather than a null dereference.
- The getJson() reflection code could be simplified by catching a broader ReflectiveOperationException (or grouping the current catch blocks) to reduce repetition and make error handling easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In JsonType, consider throwing IllegalArgumentException instead of NullPointerException when sqlTypeName is null to more clearly signal invalid caller input rather than a null dereference.
- The getJson() reflection code could be simplified by catching a broader ReflectiveOperationException (or grouping the current catch blocks) to reduce repetition and make error handling easier to maintain.
## Individual Comments
### Comment 1
<location path="src/main/java/org/dbunit/ext/postgresql/JsonType.java" line_range="134-135" />
<code_context>
+
+ try
+ {
+ final Class aPGObjectClass = super.loadClass(
+ "org.postgresql.util.PGobject", connection);
+ final Constructor ct = aPGObjectClass.getConstructor();
+ tempJson = ct.newInstance();
</code_context>
<issue_to_address>
**nitpick:** Avoid using raw Class types in the reflection code.
Using the raw Class type here removes generics-based type safety and leads to unchecked operations later. Please change this and the related Constructor/Method declarations to use `Class<?>` (and typed constructor/method references) to keep the reflection code type-safe and idiomatic without altering runtime behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/test/java/org/dbunit/ext/postgresql/JsonTypeTest.java (1)
117-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse AssertJ instead of
assertDoesNotThrow.The rest of the file uses AssertJ.
assertThatCode(...).doesNotThrowAnyException()keeps one assertion library and supports the.as()message style.♻️ Proposed refactor
- assertDoesNotThrow( - () -> type.setSqlValue(null, 1, statement), - "setSqlValue() should not throw NullPointerException when value is null."); + assertThatCode(() -> type.setSqlValue(null, 1, statement)) + .as("setSqlValue() should not throw NullPointerException when value is null.") + .doesNotThrowAnyException();Replace the import:
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.assertj.core.api.Assertions.assertThatCode;As per coding guidelines: "prefer AssertJ, add
.as()failure messages ending with a period".🤖 Prompt for 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. In `@src/test/java/org/dbunit/ext/postgresql/JsonTypeTest.java` around lines 117 - 119, Replace the JUnit assertDoesNotThrow assertion in JsonTypeTest with AssertJ’s assertThatCode(...).doesNotThrowAnyException(), preserving the null-value behavior check and expressing the failure message through .as() with a trailing period. Remove the now-unused JUnit assertion import.Source: Coding guidelines
src/test/java/org/dbunit/ext/postgresql/PostgresqlJsonIT.java (1)
85-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnd the
.as()messages with a period.The messages on lines 85, 103, 105, 109, and 111 have no trailing period. The messages on lines 125 through 135 do. Align them.
As per coding guidelines: "add
.as()failure messages ending with a period".🤖 Prompt for 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. In `@src/test/java/org/dbunit/ext/postgresql/PostgresqlJsonIT.java` around lines 85 - 111, Update the AssertJ `.as()` messages in the affected PostgreSQL JSON integration test, including the assertions for the connection and DATA/DATA_B column types and names, so each message ends with a period. Preserve the assertion logic and align these messages with the already-punctuated messages later in the test.Source: Coding guidelines
src/main/java/org/dbunit/ext/postgresql/JsonType.java (3)
73-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
sqlTypeNamebefore thesuper()call, or useObjects.requireNonNull.The
super(...)call runs before the null check.AbstractDataTypetherefore receives a null type name first. The thrown exception is still aNullPointerException, so behavior matches the test.Objects.requireNonNullinside thesuper()argument makes the intent explicit and fails earlier.♻️ Proposed refactor
public JsonType(final String sqlTypeName) { - super(sqlTypeName, Types.OTHER, String.class, false); - - if (sqlTypeName == null) - { - throw new NullPointerException( - "The parameter 'sqlTypeName' must not be null"); - } - this.sqlTypeName = sqlTypeName; + super(Objects.requireNonNull(sqlTypeName, + "The parameter 'sqlTypeName' must not be null"), Types.OTHER, + String.class, false); + + this.sqlTypeName = sqlTypeName; }Add the import:
import java.util.Objects;🤖 Prompt for 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. In `@src/main/java/org/dbunit/ext/postgresql/JsonType.java` around lines 73 - 83, Update the JsonType constructor’s super call to validate sqlTypeName before passing it to AbstractDataType, using Objects.requireNonNull or an equivalent inline check. Remove the redundant post-super null check while preserving the existing exception message.
147-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the five catch clauses with one
catch (final ReflectiveOperationException e)clause. Remove the unusedInvocationTargetExceptionimport.🤖 Prompt for 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. In `@src/main/java/org/dbunit/ext/postgresql/JsonType.java` around lines 147 - 162, In JsonType’s reflection handling, replace the five separate catches for ClassNotFoundException, InvocationTargetException, NoSuchMethodException, IllegalAccessException, and InstantiationException with one catch (final ReflectiveOperationException e) that preserves the existing TypeCastException wrapping. Remove the now-unused InvocationTargetException import.
124-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParameterize reflection types and centralize the repeated
PGobjectblock. UseClass<?>andConstructor<?>inJsonType. The same reflection sequence is duplicated inCitextType,GenericEnumType,InetType, andUuidType; extract it into a focused package-private class.src/test/java/org/dbunit/ext/postgresql/PostgresqlDataTypeFactoryTest.java (1)
99-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
.as()failure messages, and considerisInstanceOfSatisfyingto drop the cast.Both new tests cover the
jsonandjsonbbranches correctly. The assertions have no.as()description. The rest of the file also omits them, so this is a small consistency improvement rather than a defect.♻️ Proposed refactor for the `json` test
final DataType result = instance.createDataType(sqlType, sqlTypeName); - assertThat(result).isInstanceOf(JsonType.class); - assertThat(((JsonType) result).getSqlTypeName()).isEqualTo("json"); + assertThat(result).as("createDataType() should return a JsonType for json.") + .isInstanceOfSatisfying(JsonType.class, + jsonType -> assertThat(jsonType.getSqlTypeName()) + .as("The JsonType should keep the json sql type name.") + .isEqualTo("json"));As per coding guidelines: "prefer AssertJ, add
.as()failure messages ending with a period".🤖 Prompt for 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. In `@src/test/java/org/dbunit/ext/postgresql/PostgresqlDataTypeFactoryTest.java` around lines 99 - 125, Add descriptive AssertJ `.as()` messages ending with periods to the assertions in `testCreateJsonType_withJsonTypeName_returnsJsonTypeInstance` and `testCreateJsonType_withJsonbTypeName_returnsJsonTypeInstance`; optionally replace the instance assertion and cast with `isInstanceOfSatisfying` while preserving the existing type-name checks.Source: Coding guidelines
🤖 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/site/asciidoc/databases/postgresql.adoc`:
- Around line 12-15: Update the PostgreSQL type-mapping documentation to state
that `oid` maps to `PostgreSQLOidDataType` only when the JDBC type is
`Types.BIGINT` and the SQL type name is `oid`, while leaving the other mappings
and DefaultDataTypeFactory fallback unchanged.
In `@src/test/java/org/dbunit/ext/postgresql/PostgresqlJsonIT.java`:
- Line 1: Add the project-standard DbUnit LGPL license header at the beginning
of PostgresqlJsonIT.java, before the package declaration, matching the header
used by the other new files in the PR.
- Around line 56-60: Update PostgresqlJsonIT.java lines 56-60 by wrapping the
Statement created in the fixture setup in try-with-resources and removing
stat.close(); update lines 70-79 by applying try-with-resources to the teardown
Statement and moving _connection.close() and _connection = null into a finally
block so cleanup occurs even when execution fails.
- Around line 98-113: Update the metadata assertions in PostgresqlJsonIT’s
column loop to track whether DATA and DATA_B were encountered, then assert after
iteration that both columns were checked. Preserve the existing type and SQL
type-name assertions for each column.
---
Nitpick comments:
In `@src/main/java/org/dbunit/ext/postgresql/JsonType.java`:
- Around line 73-83: Update the JsonType constructor’s super call to validate
sqlTypeName before passing it to AbstractDataType, using Objects.requireNonNull
or an equivalent inline check. Remove the redundant post-super null check while
preserving the existing exception message.
- Around line 147-162: In JsonType’s reflection handling, replace the five
separate catches for ClassNotFoundException, InvocationTargetException,
NoSuchMethodException, IllegalAccessException, and InstantiationException with
one catch (final ReflectiveOperationException e) that preserves the existing
TypeCastException wrapping. Remove the now-unused InvocationTargetException
import.
In `@src/test/java/org/dbunit/ext/postgresql/JsonTypeTest.java`:
- Around line 117-119: Replace the JUnit assertDoesNotThrow assertion in
JsonTypeTest with AssertJ’s assertThatCode(...).doesNotThrowAnyException(),
preserving the null-value behavior check and expressing the failure message
through .as() with a trailing period. Remove the now-unused JUnit assertion
import.
In `@src/test/java/org/dbunit/ext/postgresql/PostgresqlDataTypeFactoryTest.java`:
- Around line 99-125: Add descriptive AssertJ `.as()` messages ending with
periods to the assertions in
`testCreateJsonType_withJsonTypeName_returnsJsonTypeInstance` and
`testCreateJsonType_withJsonbTypeName_returnsJsonTypeInstance`; optionally
replace the instance assertion and cast with `isInstanceOfSatisfying` while
preserving the existing type-name checks.
In `@src/test/java/org/dbunit/ext/postgresql/PostgresqlJsonIT.java`:
- Around line 85-111: Update the AssertJ `.as()` messages in the affected
PostgreSQL JSON integration test, including the assertions for the connection
and DATA/DATA_B column types and names, so each message ends with a period.
Preserve the assertion logic and align these messages with the
already-punctuated messages later in the test.
🪄 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: 2018b18a-965e-4a24-8ef9-43593e3b7fbf
📒 Files selected for processing (7)
src/changes/changes.xmlsrc/main/java/org/dbunit/ext/postgresql/JsonType.javasrc/main/java/org/dbunit/ext/postgresql/PostgresqlDataTypeFactory.javasrc/site/asciidoc/databases/postgresql.adocsrc/test/java/org/dbunit/ext/postgresql/JsonTypeTest.javasrc/test/java/org/dbunit/ext/postgresql/PostgresqlDataTypeFactoryTest.javasrc/test/java/org/dbunit/ext/postgresql/PostgresqlJsonIT.java
|
All fixed in 326ef32f unless noted otherwise. Applied (nitpick-level, no inline thread to reply on):
Declined:
Individual replies posted on the other inline comments (Class/Constructor typing, the oid/BIGINT doc wording, the metadata-loop assert-nothing gap, try-with-resources) with two declined there too (LGPL header on Full unit suite (2070 tests) and the full |
…ctory
PostgresqlDataTypeFactory had no recognition for PostgreSQL's native
json/jsonb columns (reported as sql type OTHER), so they fell through
to DefaultDataTypeFactory with no dedicated read/write handling.
Add JsonType, following the same reflection-based PGobject approach as
UuidType/InetType/CitextType so dbUnit doesn't need a compile-time
dependency on the PostgreSQL driver. One instance handles a single sql
type name ("json" or "jsonb") since PostgreSQL has no implicit cast
between the two; PostgresqlDataTypeFactory constructs the matching
instance per column automatically.
* Unlike the issue reporter's original 2015 patch attachment, which a
prior maintainer reply already flagged as NPEing from setSqlValue()
on a null value, this implementation explicitly null-checks in both
setSqlValue() (binds sql NULL) and typeCast() (returns null).
* Add JsonTypeTest (9 cases, including a Mockito-based regression test
for the null setSqlValue() path) and 2 new PostgresqlDataTypeFactoryTest
cases.
* Add PostgresqlJsonIT, round-tripping json and jsonb columns (including
a null row) through a live PostgreSQL 16 container; jsonb assertions
compare parsed document structure since PostgreSQL reformats jsonb
text on storage.
* Document JsonType in databases/postgresql.adoc, cross-linking
IsActualEqualToExpectedJsonValueComparer for semantic JSON comparison.
Refs: 574
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017rt6YcFdhBCwafLp6n7mcZ
b024389 to
fff0563
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fff05630f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Override | ||
| public Object typeCast(final Object value) throws TypeCastException | ||
| { | ||
| return value == null ? null : value.toString(); |
There was a problem hiding this comment.
When a flat-XML (or other sparse) expected row omits a json/jsonb attribute after the table metadata already includes that column, ITable.getValue() returns ITable.NO_VALUE. Default assertions and SortedTable compare through DataType.compare(), which reaches this typeCast() method; converting the sentinel with toString() makes an omitted expected JSON cell compare as a random object string instead of matching a database NULL, unlike the string-like datatypes this adapter replaces. Please handle ITable.NO_VALUE the same as null here.
Useful? React with 👍 / 👎.
| } else if ("citext".equals(sqlTypeName)) | ||
| { | ||
| return new CitextType(); | ||
| } else if ("json".equals(sqlTypeName) || "jsonb".equals(sqlTypeName)) |
There was a problem hiding this comment.
For a PostgreSQL table without a primary key when DatabaseConfig.FEATURE_SORT_ALL_COLUMNS_WHEN_NO_PRIMARY_KEY is enabled, DatabaseDataSet.getSelectStatement() orders by every non-LOB column. The json half of this branch now makes those columns known non-LOB columns instead of being ignored as unknown, so reading such a table generates ORDER BY on a json column, which PostgreSQL rejects because json has no ordering operator. Please exclude json columns from that fallback ordering or otherwise avoid adding them to the generated ORDER BY.
Useful? React with 👍 / 👎.
1 participant
…ctory
PostgresqlDataTypeFactory had no recognition for PostgreSQL's native json/jsonb columns (reported as sql type OTHER), so they fell through to DefaultDataTypeFactory with no dedicated read/write handling.
Add JsonType, following the same reflection-based PGobject approach as UuidType/InetType/CitextType so dbUnit doesn't need a compile-time dependency on the PostgreSQL driver. One instance handles a single sql type name ("json" or "jsonb") since PostgreSQL has no implicit cast between the two; PostgresqlDataTypeFactory constructs the matching instance per column automatically.
Refs: 574
Claude-Session: https://claude.ai/code/session_017rt6YcFdhBCwafLp6n7mcZ
Summary by Sourcery
Add native PostgreSQL JSON/JSONB column support via a new JsonType and wire it into the PostgreSQL data type factory.
New Features:
Documentation:
Tests:
Summary by CodeRabbit
New Features
jsonandjsonbdata types.NULLvalues.Documentation