fix(dataset): Stop TimestampDataType double-shifting TIMESTAMP WITH T… - #834
Merged
Merged
Conversation
📝 WalkthroughWalkthrough
ChangesTimezone-aware timestamp support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TimestampDataType
participant PreparedStatement
participant ParameterMetaData
participant Database
TimestampDataType->>PreparedStatement: Request parameter metadata
PreparedStatement->>ParameterMetaData: Read JDBC parameter type
ParameterMetaData-->>TimestampDataType: TIMESTAMP_WITH_TIMEZONE or other type
TimestampDataType->>PreparedStatement: Bind OffsetDateTime or setTimestamp
PreparedStatement->>Database: Store timestamp value
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.java (1)
899-907: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer asserting the full
OffsetDateTimeobject rather than just.toInstant().The mock-based sibling test (lines 848-853) compares the whole captured
OffsetDateTimeagainst the expected value, but this H2 test narrows the assertion to.toInstant(). Per H2 documentation,TIMESTAMP WITH TIME ZONEpreserves the exact offset it was given, so asserting the full object here would also validate offset preservation (central to this fix) rather than instant-equality alone. As per coding guidelines,**/*Test.java: "Prefer asserting the actual object against an expected object rather than asserting individual fields separately."🤖 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/dataset/datatype/TimestampDataTypeTest.java` around lines 899 - 907, The H2 timestamp test currently validates only the instant, leaving the stored offset unchecked. Update the assertion around resultSet.getObject(1, OffsetDateTime.class) to compare the complete OffsetDateTime directly with the expected value, preserving validation of both instant and offset.Source: Coding guidelines
src/main/java/org/dbunit/dataset/datatype/TimestampDataType.java (1)
274-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueZone-detection and OffsetDateTime binding logic is correct.
isTimestampWithTimeZoneColumnplus theOffsetDateTime.ofInstant(ts.toInstant(), cal.getTimeZone().toZoneId())binding correctly preserves the dataset's own offset forTIMESTAMP WITH TIME ZONEcolumns, and the broadcatch (Exception e)is actually load-bearing: several pre-existing tests never stubgetParameterMetaData(), so the mock returnsnulland the resultingNullPointerExceptionis what routes them back to the legacy Calendar path.Types.TIMESTAMP_WITH_TIMEZONEis also a valid, correctly-spelled JDK constant, andsetObject(OffsetDateTime)/getObject(Class)is the JDBC-recommended pattern for this SQL type.One small style nit:
cal.getTimeZone().toZoneId()(line 277) andstatement.getParameterMetaData().getParameterType(column)(lines 313-314) each chain two calls together. As per coding guidelines,**/*.{java,xml}should "use clear code, sparse inline comments, separate local variables, and single statements instead of deeply compounded nested calls."♻️ Optional: extract intermediate locals
- final OffsetDateTime offsetDateTime = OffsetDateTime - .ofInstant(ts.toInstant(), cal.getTimeZone().toZoneId()); + final ZoneId zoneId = cal.getTimeZone().toZoneId(); + final OffsetDateTime offsetDateTime = + OffsetDateTime.ofInstant(ts.toInstant(), zoneId);- final int parameterType = - statement.getParameterMetaData().getParameterType(column); + final ParameterMetaData metaData = statement.getParameterMetaData(); + final int parameterType = metaData.getParameterType(column);🤖 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/dataset/datatype/TimestampDataType.java` around lines 274 - 327, Refactor the chained calls in the timestamp-with-time-zone handling without changing behavior: in the binding branch, extract cal.getTimeZone() and its ZoneId before OffsetDateTime.ofInstant, and in isTimestampWithTimeZoneColumn extract the ParameterMetaData before calling getParameterType(column). Preserve the existing fallback and exception handling.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/changes/changes.xml`:
- Line 16: Align the final TimestampDataType release-note summary in the
3.4.0-SNAPSHOT description with the detailed issue 831 entry: describe the JDBC
driver behavior of tagging setTimestamp values with the JVM default time zone
instead of the dataset time zone, and remove the inaccurate
double-application-of-offset wording. Keep the rest of the release description
unchanged.
In `@src/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.java`:
- Around line 871-944: Move both H2-backed tests from the Surefire test class
into an IT-suffixed integration-test class, following the existing
DatabaseEnvironment-based setup and active Maven profile database properties
instead of hardcoding jdbc:h2 URLs. Preserve the existing assertions for
timezone-aware and timezone-less timestamp behavior while ensuring all database
resources use the project’s established integration-test conventions.
- Around line 871-944: Update both H2 tests,
testSetSqlValue_withOffsetDifferentFromLocalZone_writesCorrectInstantToTimestampWithTimeZoneColumn
and
testSetSqlValue_withOffsetDifferentFromLocalZone_writesLiteralDigitsToTimestampColumnWithoutTimeZone,
to assert the prepared statement parameter metadata before binding. Verify the
zoned column reports Types.TIMESTAMP_WITH_TIMEZONE, preserving the pinned H2
binding path before the existing storage assertions.
---
Nitpick comments:
In `@src/main/java/org/dbunit/dataset/datatype/TimestampDataType.java`:
- Around line 274-327: Refactor the chained calls in the
timestamp-with-time-zone handling without changing behavior: in the binding
branch, extract cal.getTimeZone() and its ZoneId before
OffsetDateTime.ofInstant, and in isTimestampWithTimeZoneColumn extract the
ParameterMetaData before calling getParameterType(column). Preserve the existing
fallback and exception handling.
In `@src/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.java`:
- Around line 899-907: The H2 timestamp test currently validates only the
instant, leaving the stored offset unchecked. Update the assertion around
resultSet.getObject(1, OffsetDateTime.class) to compare the complete
OffsetDateTime directly with the expected value, preserving validation of both
instant and offset.
🪄 Autofix (Beta)
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: 5277ebf9-d126-4a7d-8907-c5efbe758b20
📒 Files selected for processing (3)
src/changes/changes.xmlsrc/main/java/org/dbunit/dataset/datatype/TimestampDataType.javasrc/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.java
Contributor
Author
|
Also addressed the two nitpicks from the review body (these weren't posted as separate inline threads, so replying here):
All pushed in 1a93ec9, 68f204b, and 0bfb9dd (fixup commits against 1e0a37f). Local verification: full unit suite green, and |
Contributor
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/test/java/org/dbunit/dataset/datatype/TimestampDataTypeIT.java`:
- Around line 147-155: Update the table DDL in TimestampDataTypeIT setup to
derive the temporal column type from DatabaseEnvironment and the active Maven
profile’s database system properties, using DATETIME2 for SQL Server and
retaining TIMESTAMP for compatible profiles. Keep the existing
TimestampDataType.setSqlValue and insert flow unchanged.
- Around line 117-120: Update the parameter metadata handling in
TimestampDataTypeIT so failures from getParameterMetaData() or
getParameterType(1), including SQLFeatureNotSupportedException, are caught and
cause the test to skip via the existing assumption mechanism, then return.
Preserve the current skip when the reported type is not TIMESTAMP_WITH_TIMEZONE,
mirroring TimestampDataType’s metadata-failure fallback.
🪄 Autofix (Beta)
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: 0d2a1dd9-33e3-4d81-9933-72e6aa678223
📒 Files selected for processing (4)
src/changes/changes.xmlsrc/main/java/org/dbunit/dataset/datatype/TimestampDataType.javasrc/test/java/org/dbunit/dataset/datatype/TimestampDataTypeIT.javasrc/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.java
💤 Files with no reviewable changes (1)
- src/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/changes/changes.xml
- src/main/java/org/dbunit/dataset/datatype/TimestampDataType.java
jeffjensen
force-pushed
the
fix/831-timestamp-with-timezone-double-shift
branch
from
July 27, 2026 21:45
167c427 to
4180980
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/org/dbunit/dataset/datatype/TimestampDataTypeIT.java (1)
158-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the database metadata lookup into local variables.
The chained call obscures the setup logic and violates the guideline requiring separate local variables.
Proposed refactor
- final boolean isSqlServer = jdbcConnection.getMetaData().getDatabaseProductName() - .startsWith("Microsoft SQL Server"); + final DatabaseMetaData databaseMetaData = jdbcConnection.getMetaData(); + final String databaseProductName = databaseMetaData.getDatabaseProductName(); + final boolean isSqlServer = databaseProductName.startsWith("Microsoft SQL Server");🤖 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/dataset/datatype/TimestampDataTypeIT.java` around lines 158 - 159, In the setup surrounding the isSqlServer calculation in TimestampDataTypeIT, split the chained jdbcConnection metadata access into separate local variables: first store the database metadata, then retrieve and store the database product name, and use that variable for the SQL Server check.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/test/java/org/dbunit/dataset/datatype/TimestampDataTypeIT.java`:
- Around line 110-115: Update the SQLException handling in the TIMESTAMP WITH
TIME ZONE table-creation setup to skip only when the exception clearly indicates
the active database does not support that column type; rethrow or otherwise fail
on permission, connection, malformed-DDL, and other unexpected setup errors.
Preserve the existing assumption message for recognized unsupported-type
failures.
---
Nitpick comments:
In `@src/test/java/org/dbunit/dataset/datatype/TimestampDataTypeIT.java`:
- Around line 158-159: In the setup surrounding the isSqlServer calculation in
TimestampDataTypeIT, split the chained jdbcConnection metadata access into
separate local variables: first store the database metadata, then retrieve and
store the database product name, and use that variable for the SQL Server check.
🪄 Autofix (Beta)
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: ef54614a-fbef-4ac1-84c9-5730ca9108e8
📒 Files selected for processing (4)
src/changes/changes.xmlsrc/main/java/org/dbunit/dataset/datatype/TimestampDataType.javasrc/test/java/org/dbunit/dataset/datatype/TimestampDataTypeIT.javasrc/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/dbunit/dataset/datatype/TimestampDataType.java
- src/changes/changes.xml
- src/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.java
jeffjensen
force-pushed
the
fix/831-timestamp-with-timezone-double-shift
branch
from
July 27, 2026 22:28
3099403 to
b2752e6
Compare
…IME ZONE writes Issue #711's fix made setSqlValue() pass a Calendar built from the dataset's own timezone offset to statement.setTimestamp(column, ts, cal). That is correct for TIMESTAMP WITHOUT TIME ZONE columns, but some JDBC drivers (confirmed with H2) use the Calendar only to compute the wall-clock digits sent to a TIMESTAMP WITH TIME ZONE column, then tag the stored value with the JVM's default timezone instead of the Calendar's - silently discarding the dataset's own timezone and storing the wrong instant whenever the JVM and dataset timezones differ. * Detect a TIMESTAMP WITH TIME ZONE destination column via PreparedStatement.getParameterMetaData() and write it with setObject(OffsetDateTime) instead, which drivers supporting the SQL type map unambiguously. Fall back to the existing Calendar-based behavior if parameter metadata is unavailable or unsupported. * Columns without a timezone keep the existing #711 behavior unchanged. * Add TimestampDataTypeTest coverage against a real H2 TIMESTAMP WITH TIME ZONE column reproducing the bug (ported from yoshida16729438's dbunit-320-timestamp reproducer), plus a paired TIMESTAMP-without-zone test guarding against regressing #711. * Add TimestampDataTypeIT, a Failsafe companion that runs both cases through DatabaseEnvironment against whichever database profile is active. The plain-TIMESTAMP case runs unconditionally, substituting DATETIME2 on SQL Server, whose TIMESTAMP type is an unrelated, unwritable row-versioning type rather than a date/time type. * Gate the zoned-column case behind a new TestFeature.TIMESTAMP_WITH_TIMEZONE flag, declared unsupported via dbunit.profile.unsupportedFeatures on Derby, MySQL, MSSQL, and DB2 (whose dialects reject the column syntax outright) and on Oracle (whose JDBC driver throws SQLFeatureNotSupportedException from getParameterMetaData() itself, a known driver limitation rather than a dialect gap). Declaring the gap in the profile up front, rather than discovering it by catching whatever exception a given driver happens to throw, means any other setup failure surfaces as a real test error instead of being silently treated as unsupported. H2, HSQLDB, and PostgreSQL need no such declaration: H2 and HSQLDB run the assertion for real, and PostgreSQL already skips itself cleanly via the pre-existing parameter-metadata type check (pgjdbc reports a different constant without throwing). * Document TIMESTAMP_WITH_TIMEZONE in the "Unsupported Features Values" table in src/site/asciidoc/integrationtests.adoc, alongside the existing feature entries. Refs: 831 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fr791v2SnTRnyb1eU7LNB
jeffjensen
force-pushed
the
fix/831-timestamp-with-timezone-double-shift
branch
from
July 27, 2026 22:38
b2752e6 to
32d94c9
Compare
Contributor
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
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/test/java/org/dbunit/dataset/datatype/TimestampDataTypeIT.java`:
- Around line 123-129: Update the parameter-metadata lookup in the zoned
timestamp test to catch SQLException, invoke the existing assumption mechanism
with a skip message, and return when metadata is unsupported. Preserve the
current assumeTrue check for drivers that provide the metadata, so Oracle
integration tests skip rather than fail.
🪄 Autofix (Beta)
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: 4ca6f95d-6fcf-4ac8-a85a-f97557f8c08a
📒 Files selected for processing (13)
src/changes/changes.xmlsrc/main/java/org/dbunit/dataset/datatype/TimestampDataType.javasrc/site/asciidoc/integrationtests.adocsrc/test/java/org/dbunit/TestFeature.javasrc/test/java/org/dbunit/dataset/datatype/TimestampDataTypeIT.javasrc/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.javasrc/test/resources/db2-dbunit.propertiessrc/test/resources/derby-dbunit.propertiessrc/test/resources/mssql2022-dbunit.propertiessrc/test/resources/mssql41-dbunit.propertiessrc/test/resources/mysql-dbunit.propertiessrc/test/resources/oracle-free-dbunit.propertiessrc/test/resources/oracle-xe-dbunit.properties
🚧 Files skipped from review as they are similar to previous changes (3)
- src/changes/changes.xml
- src/main/java/org/dbunit/dataset/datatype/TimestampDataType.java
- src/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.java
jeffjensen
added a commit
that referenced
this pull request
Jul 28, 2026
…eTrue with @EnabledIfSystemProperty
testExecute_withDefaultValueNotNullColumnTurningNull_appliesDefaultOnSecondRow
only runs against HSQLDB, since DEFAULT_VALUE_TABLE is defined solely in that
profile's fixture DDL. It expressed that as an inline runtime check:
Assumptions.assumeTrue(
_connection.getConnection().getMetaData()
.getDatabaseProductName().startsWith("HSQL"), ...);
Replace it with a method-level @EnabledIfSystemProperty(named = "dbunit.profile",
matches = "hsqldb") annotation, matching the class-level precedent already used
elsewhere in this suite (InsertIdentityOperationIT, PostgresqlUuidIT,
PostgresSQLOidIT, SQLHelperDomainPostgreSQLIT). Drop the now-unused Assumptions
import. Behavior unchanged: verified the target test still runs for real on
hsqldb-2-7 and is cleanly disabled on h2-1-4.
Found while surveying the codebase for the same in-test special-case-logic
pattern addressed in #831/PR #834. This one doesn't fit that PR's
dbunit.profile.unsupportedFeatures/TestFeature mechanism, since it's an
inclusion check (only-HSQLDB) rather than an exclusion list, but the
underlying goal is the same: replace an inline runtime database check with
declarative JUnit config.
Refs: 835
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011fr791v2SnTRnyb1eU7LNB
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
1 participant
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.
…IME ZONE writes
Issue #711's fix made setSqlValue() pass a Calendar built from the dataset's own timezone offset to statement.setTimestamp(column, ts, cal). That is correct for TIMESTAMP WITHOUT TIME ZONE columns, but some JDBC drivers (confirmed with H2) use the Calendar only to compute the wall-clock digits sent to a TIMESTAMP WITH TIME ZONE column, then tag the stored value with the JVM's default timezone instead of the Calendar's - silently discarding the dataset's own timezone and storing the wrong instant whenever the JVM and dataset timezones differ.
Refs: 831
Claude-Session: https://claude.ai/code/session_01QqQTzFd5qsXiMTpFsReD12
Summary by CodeRabbit
Bug Fixes
TIMESTAMP WITH TIME ZONEby binding values asOffsetDateTime(based on declared parameter metadata) to ensure the correct instant/offset is stored.TIMESTAMPcolumns, maintaining consistent stored wall-clock digits.Tests
TIMESTAMP WITH TIME ZONEvs fallback scenarios.Documentation
3.4.0-SNAPSHOTchangelog entry for this fix (issue 831).