Fix generated-ID row mismatches in PrepAndExpectedTestCase - #907
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideIntroduce an opt-in sort-on-filtered-columns-only mode in DefaultPrepAndExpectedTestCase to fix false DbComparisonFailure mismatches on tables with generated/identity key columns, wire it through VerifyTableDefinition, add integration coverage using a new IDENTITY_TABLE fixture across multiple databases, and document the new behavior in code and site docs including the changelog. Sequence diagram for sortOnFilteredColumnsOnly behavior in DefaultPrepAndExpectedTestCase.verifyDatasequenceDiagram
actor Test
participant VerifyTableDefinition
participant DefaultPrepAndExpectedTestCase
participant expectedTable as ITable_expected
participant actualTable as ITable_actual
participant SortedExpected as SortedTable_expected
participant SortedActual as SortedTable_actual
Test->>VerifyTableDefinition: setSortOnFilteredColumnsOnly(true|false)
Test->>DefaultPrepAndExpectedTestCase: verifyData(connection, verifyTableDefinition)
DefaultPrepAndExpectedTestCase->>DefaultPrepAndExpectedTestCase: loadTableDataFromDatabase(tableName, connection)
DefaultPrepAndExpectedTestCase->>VerifyTableDefinition: isSortOnFilteredColumnsOnly()
VerifyTableDefinition-->>DefaultPrepAndExpectedTestCase: sortOnFilteredColumnsOnly
DefaultPrepAndExpectedTestCase->>DefaultPrepAndExpectedTestCase: makeExpectedTableColumns(actualTableColumns, expectedTableMetaData)
alt [sortOnFilteredColumnsOnly true]
DefaultPrepAndExpectedTestCase->>DefaultPrepAndExpectedTestCase: makeSortColumns(actualTableColumns, excludeColumns, includeColumns, tableName)
DefaultPrepAndExpectedTestCase-->>DefaultPrepAndExpectedTestCase: actualSortColumns
DefaultPrepAndExpectedTestCase->>DefaultPrepAndExpectedTestCase: makeSortColumns(expectedTableColumns, excludeColumns, includeColumns, tableName)
DefaultPrepAndExpectedTestCase-->>DefaultPrepAndExpectedTestCase: expectedSortColumns
else [sortOnFilteredColumnsOnly false]
DefaultPrepAndExpectedTestCase-->>DefaultPrepAndExpectedTestCase: actualSortColumns = actualTableColumns
DefaultPrepAndExpectedTestCase-->>DefaultPrepAndExpectedTestCase: expectedSortColumns = expectedTableColumns
end
DefaultPrepAndExpectedTestCase->>SortedExpected: new SortedTable(expectedTable, expectedSortColumns, true)
DefaultPrepAndExpectedTestCase->>SortedActual: new SortedTable(actualTable, actualSortColumns)
DefaultPrepAndExpectedTestCase->>DefaultPrepAndExpectedTestCase: assertion.verifyTables(SortedExpected, SortedActual, columnValueComparers)
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 47 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 (18)
📝 WalkthroughWalkthrough
ChangesFiltered-column verification sorting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VerifyTableDefinition
participant DefaultPrepAndExpectedTestCase
participant DefaultColumnFilter
VerifyTableDefinition->>DefaultPrepAndExpectedTestCase: provide sorting option
DefaultPrepAndExpectedTestCase->>DefaultColumnFilter: apply include/exclude filters
DefaultColumnFilter-->>DefaultPrepAndExpectedTestCase: return retained columns
DefaultPrepAndExpectedTestCase->>DefaultPrepAndExpectedTestCase: sort expected and actual tables
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 |
![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 left some high level feedback:
- The new makeSortColumns logic duplicates the column-filtering semantics in applyColumnFilters; consider refactoring to reuse a single implementation so that future changes to filter behavior stay consistent between sorting and comparison.
- When sortOnFilteredColumnsOnly is true and all columns are excluded (or includeColumns yields an empty set), makeSortColumns will return an empty array—verify that SortedTable handles this as expected or add a guard to fall back to the full column set to avoid surprising behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new makeSortColumns logic duplicates the column-filtering semantics in applyColumnFilters; consider refactoring to reuse a single implementation so that future changes to filter behavior stay consistent between sorting and comparison.
- When sortOnFilteredColumnsOnly is true and all columns are excluded (or includeColumns yields an empty set), makeSortColumns will return an empty array—verify that SortedTable handles this as expected or add a guard to fall back to the full column set to avoid surprising behavior.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
🤖 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/DefaultPrepAndExpectedTestCase.java`:
- Around line 710-715: Update the verifyData invocation in
DefaultPrepAndExpectedTestCase so it preserves subclass overrides: call the
existing six-argument verifyData overload when sortOnFilteredColumnsOnly is
false, and call the new seven-argument overload only when it is true.
In `@src/site/asciidoc/components/verifytabledefinition.adoc`:
- Around line 95-97: Revise the default-behavior explanation near the
sort-by-all-columns setting to avoid claiming that non-generated or included
primary keys always sort correctly. State that the default is safe only when the
distinguishing sort columns are present in both tables, and that per-table
opt-in is required when such a column is excluded from the expected table.
In
`@src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseGeneratedIdRowOrderIT.java`:
- Around line 82-142: Add targeted unit tests in
DefaultPrepAndExpectedTestCaseTest covering
VerifyTableDefinition#setSortOnFilteredColumnsOnly(false) and true, and assert
that the resulting sort-key selection uses all columns by default and only
filtered columns when enabled. Reuse the existing test fixtures and verification
setup, keeping the tests focused on configuration behavior rather than database
integration.
In `@src/test/resources/sql/mysql.sql`:
- Around line 65-67: Update the IDENTITY_TABLE header comment to use valid MySQL
single-line comment syntax, ensuring each comment line begins with “--” followed
by whitespace while preserving the existing label and separator structure.
🪄 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: e777387c-4320-4e41-80b1-feb4441471ef
📒 Files selected for processing (15)
src/changes/changes.xmlsrc/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.javasrc/main/java/org/dbunit/VerifyTableDefinition.javasrc/site/asciidoc/components/verifytabledefinition.adocsrc/site/asciidoc/datacomparisons/equality.adocsrc/site/asciidoc/datasets/decorators.adocsrc/site/asciidoc/testcases/PrepAndExpectedTestCase.adocsrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseGeneratedIdRowOrderIT.javasrc/test/resources/sql/derby.sqlsrc/test/resources/sql/mysql.sqlsrc/test/resources/sql/oracle.sqlsrc/test/resources/sql/postgresql.sqlsrc/test/resources/xml/generatedIdRowOrderExpectedMatch.xmlsrc/test/resources/xml/generatedIdRowOrderExpectedMismatch.xmlsrc/test/resources/xml/generatedIdRowOrderPrep.xml
Member
Author
|
Addressing Sourcery's two points from the initial review:
Also fixed CodeRabbit's four inline findings (replied individually on each thread) — one blocking (subclass override hook bypass), two docs/test-coverage suggestions applied, one (MySQL |
jeffjensen
force-pushed
the
672-prepandexpected-sort-filtered-columns
branch
from
August 4, 2026 11:56
98cda0f to
d2ec29e
Compare
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/AbstractDataSetTest.java`:
- Around line 85-96: Update the JavaDoc for removeExtraTestTables to describe
the identity-column test tables as cross-database rather than MSSQL-specific,
and document that PostgreSQL folds unquoted identifiers to lowercase while other
vendors use uppercase. Keep the existing table-removal behavior unchanged.
🪄 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: 16aa04d9-3cc1-4f2a-9022-8832048b3e06
📒 Files selected for processing (18)
src/changes/changes.xmlsrc/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.javasrc/main/java/org/dbunit/VerifyTableDefinition.javasrc/site/asciidoc/components/verifytabledefinition.adocsrc/site/asciidoc/datacomparisons/equality.adocsrc/site/asciidoc/datasets/decorators.adocsrc/site/asciidoc/testcases/PrepAndExpectedTestCase.adocsrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseGeneratedIdRowOrderIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.javasrc/test/java/org/dbunit/VerifyTableDefinitionTest.javasrc/test/java/org/dbunit/dataset/AbstractDataSetTest.javasrc/test/resources/sql/derby.sqlsrc/test/resources/sql/mysql.sqlsrc/test/resources/sql/oracle.sqlsrc/test/resources/sql/postgresql.sqlsrc/test/resources/xml/generatedIdRowOrderExpectedMatch.xmlsrc/test/resources/xml/generatedIdRowOrderExpectedMismatch.xmlsrc/test/resources/xml/generatedIdRowOrderPrep.xml
🚧 Files skipped from review as they are similar to previous changes (13)
- src/test/resources/sql/postgresql.sql
- src/test/resources/sql/derby.sql
- src/test/resources/sql/mysql.sql
- src/test/resources/xml/generatedIdRowOrderExpectedMismatch.xml
- src/test/resources/sql/oracle.sql
- src/test/resources/xml/generatedIdRowOrderExpectedMatch.xml
- src/site/asciidoc/datasets/decorators.adoc
- src/test/resources/xml/generatedIdRowOrderPrep.xml
- src/site/asciidoc/testcases/PrepAndExpectedTestCase.adoc
- src/changes/changes.xml
- src/site/asciidoc/datacomparisons/equality.adoc
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseGeneratedIdRowOrderIT.java
- src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java
The enhancement fixes DefaultPrepAndExpectedTestCase table comparison row mismatches with situations such as generated-IDs. DefaultPrepAndExpectedTestCase#verifyData always sorted the actual table by all of its native columns - including a generated/identity first column - while sorting the expected table by only the columns its file declares, which typically omits that column since its value is unknown ahead of time. excludeColumns/includeColumns were applied to the comparison but never to the sort, so when production code does not guarantee row insertion order (e.g. Hibernate reordering a batch insert), the database's generated-ID assignment order diverges from the data-content order the expected table sorts by, misaligning same-data rows and failing the comparison despite both sides holding identical data. * Add VerifyTableDefinition#sortOnFilteredColumnsOnly (default false, preserving prior behavior), implementing the opt-in toggle proposed in issue 676: when true, both tables sort by only their excludeColumns/includeColumns-filtered columns instead of all native columns. verifyData(IDatabaseConnection, VerifyTableDefinition) only routes through the new seven-argument verifyData overload when the flag is true; it keeps calling the existing six-argument overload otherwise, so a subclass overriding that overload is still invoked in the (default) common case. * Add DefaultPrepAndExpectedTestCaseGeneratedIdRowOrderIT reproducing the defect and proving the fix (default still reproduces it, opt-in fixes it, opt-in still catches genuine mismatches), using the existing IDENTITY_TABLE fixture. Add matching IDENTITY_TABLE DDL to derby.sql, mysql.sql, postgresql.sql, and oracle.sql (previously only in hypersonic.sql, h2.sql, mssql.sql, and db2xml.sql) so it runs on all 9 database profiles; also add the table's lowercase form to AbstractDataSetTest's cross-vendor test-table exclusion list, since PostgreSQL - unlike the other vendors here - folds unquoted identifiers to lowercase. * Add unit tests in DefaultPrepAndExpectedTestCaseTest exercising sortOnFilteredColumnsOnly true/false directly against mock tables (no database), including confirming an all-columns-excluded table sorts as a safe no-op instead of throwing. * Document sortOnFilteredColumnsOnly in Javadoc and a new "Sort Mode" site section, cross-linked from PrepAndExpectedTestCase.adoc's row-ordering description and the general row-ordering guidance in equality.adoc/decorators.adoc. Refs: 672 Refs: 676 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AebUvmVvD9HpqAxDnKEBqK
jeffjensen
force-pushed
the
672-prepandexpected-sort-filtered-columns
branch
from
August 4, 2026 12:08
d2ec29e to
51d35a3
Compare
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.
Summary
DefaultPrepAndExpectedTestCase#verifyDataalways sorted the actual table by all of its native columns (generated/identity column included) but the expected table by only the columns its file declares, which typically omits that column since its value is unknown ahead of time —excludeColumns/includeColumnswere applied to the comparison but never to the sort. When production code doesn't guarantee row insertion order (e.g. Hibernate reordering a batch insert), this misaligns same-data rows and fails the comparison with a falseDbComparisonFailure.VerifyTableDefinition#sortOnFilteredColumnsOnly(defaultfalse, preserving prior behavior), the opt-in toggle proposed in Allow PrepAndExpectedTestCase to sort only on filtered columns instead of all columns #676: whentrue, both tables sort by only theirexcludeColumns/includeColumns-filtered columns instead of all native columns.DefaultPrepAndExpectedTestCaseGeneratedIdRowOrderITreproducing the defect and proving the fix (default still reproduces it, opt-in fixes it, opt-in still catches genuine mismatches), using the existingIDENTITY_TABLEfixture; added that DDL to derby/mysql/postgresql/oracle so it runs on all 9 database profiles.sortOnFilteredColumnsOnlyin Javadoc and a new site "Sort Mode" section, cross-linked from the row-ordering guidance inPrepAndExpectedTestCase.adoc,equality.adoc, anddecorators.adoc.Fixes #672
Closes #676
Test plan
./mvnw clean test— 1935 unit tests pass./mvnw clean verify -Phsqldb-2-7— 351 ITs pass./mvnw clean verify -Ph2-1-4— 351 ITs pass./mvnw clean verify -Pderby-10-14— 351 ITs pass./mvnw clean install site— no new Asciidoctor/Javadoc warnings🤖 Generated with Claude Code
https://claude.ai/code/session_01AebUvmVvD9HpqAxDnKEBqK
Summary by Sourcery
Add an opt-in mode to DefaultPrepAndExpectedTestCase to sort tables on filtered columns only, preventing false comparison failures when the first column is a generated/identity value, and document and test this behavior across supported databases.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests