fix: Apply six confirmed fixes from the open-issue code review - #880
Merged
Conversation
…RowCount() getRowCount(String, String) built its QualifiedTableName with the 2-arg constructor, which never applies DatabaseConfig.PROPERTY_ESCAPE_PATTERN, so a configured escape pattern was silently dropped from the generated SQL. createTable() already looks up and applies the escape pattern; getRowCount() now does the same. Refs: 492
…umns DatabaseTableMetaData.getPrimaryKeys() returned an empty array whenever a configured PROPERTY_PRIMARY_KEY_FILTER matched zero columns for a table (e.g. a naming-convention filter that doesn't recognize that table's PK column), instead of falling back to the table's actual database-declared primary key. Add that fallback, matching the no-filter-configured branch's existing behavior. Behavior change: this makes the fallback unconditional whenever a filter yields zero columns, which now overrides a filter deliberately configured to declare that a table has no PK. Refs: 628
…seDataSet.initialize() The debug-only SQLHelper.getDatabaseInfo() call ran after metadataHandler.getTables() had already opened the table metadata ResultSet. On JDBC drivers that allow only a single active cursor per connection (e.g. SQL Anywhere, some legacy Sybase drivers), those intervening DatabaseMetaData calls can silently invalidate the still-open ResultSet. Move the call above ResultSet creation, reusing the already-held databaseMetaData variable instead of calling jdbcConnection.getMetaData() again. None of this project's 9 CI database profiles have the single-cursor constraint, so this was not reproducible locally; verified instead via a mocked IMetadataHandler/ DatabaseMetaData test asserting call order. Refs: 460
…ry key DatabaseDataSet.getSelectStatement() emitted no ORDER BY clause for a table with no primary key, leaving row order database-defined and thus nondeterministic. Add DatabaseConfig.FEATURE_SORT_ALL_COLUMNS_WHEN_NO_PRIMARY_KEY, defaulting to off to preserve existing behavior; when enabled, such a table is sorted by all of its non-LOB columns instead. CLOB/BLOB columns are always excluded from that fallback sort since some databases (notably Oracle) reject LOB columns in ORDER BY outright. Refs: 171
FlatDtdWriter.write() emitted "<!ELEMENT dataset (\n)>" for a
zero-table dataset - an empty parenthesized content model, which is
not valid DTD syntax. FlatDtdProducer then failed reading it back
with a SAXParseException ("A '(' character or an element type is
required...").
Emit "<!ELEMENT dataset ANY>" instead when there are no tables. ANY is
preferred over EMPTY because a pretty-printed empty dataset file
typically contains whitespace between <dataset> and </dataset>, and
EMPTY rejects any content including whitespace, while ANY tolerates
both.
Refs: 542
…a connections MySqlMetadataHandler.getTables()/getColumns()/getPrimaryKeys() pass a null schema straight through as the JDBC catalog argument. Per the JDBC spec catalog=null means "search every catalog", but MySQL Connector/J's default nullCatalogMeansCurrent setting instead treats it as "the connection's current catalog only", so a connection not restricted to a single schema (e.g. connecting as "root" specifically to work across several schemas, with FEATURE_QUALIFIED_TABLE_NAMES enabled) silently sees only one catalog's tables and gets NoSuchTableException for tables in every other one. Add MultiSchemaMySqlMetadataHandler: whenever no single schema is configured, it enumerates the connection's visible catalogs (skipping information_schema/mysql/performance_schema/sys) and unions the per-catalog getTables()/getColumns()/getPrimaryKeys()/tableExists() results itself instead of ever passing a null catalog to the driver. The union is backed by a minimal in-memory ResultSet/ResultSetMetaData Proxy supporting only the handful of methods dbunit itself calls against a metadata-handler result, copied out of and closing each per-catalog result set eagerly. The original reported fix was only ever a SourceForge file attachment and was not retrievable; this is a fresh implementation built from the confirmed diagnosis, verified via mocked multi-catalog DatabaseMetaData unit tests and a full mysql-9-20 regression run (347 tests, all green) rather than a recovered, untested patch. Refs: 533
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (8)
📝 WalkthroughWalkthroughChangesDatabase query and metadata behavior
MySQL multi-schema metadata
Empty dataset DTD output
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DatabaseConfig
participant AbstractResultSetTable
participant DatabaseDataSet
DatabaseConfig->>AbstractResultSetTable: provide sorting feature
AbstractResultSetTable->>DatabaseDataSet: request select statement
DatabaseDataSet->>DatabaseDataSet: select non-LOB columns when no primary key
sequenceDiagram
participant DatabaseMetaData
participant MultiSchemaMySqlMetadataHandler
participant MetadataResultSetMerger
MultiSchemaMySqlMetadataHandler->>DatabaseMetaData: enumerate visible catalogs
MultiSchemaMySqlMetadataHandler->>DatabaseMetaData: query tables, columns, and primary keys
MultiSchemaMySqlMetadataHandler->>MetadataResultSetMerger: merge result sets
MetadataResultSetMerger-->>MultiSchemaMySqlMetadataHandler: return combined metadata
🚥 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 |
![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 (3)
src/test/java/org/dbunit/dataset/xml/FlatDtdProducerTest.java (1)
222-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the nested test setup into local variables.
Create separate
DefaultDataSetandFlatDtdWritervariables before callingwrite. This makes the setup easier to inspect and follows the repository Java style rule.Suggested change
- new FlatDtdWriter(dtdWriter).write(new DefaultDataSet()); + final DefaultDataSet dataSet = new DefaultDataSet(); + final FlatDtdWriter flatDtdWriter = new FlatDtdWriter(dtdWriter); + flatDtdWriter.write(dataSet);As per coding guidelines, use separate local variables and avoid deeply compounded nested calls.
🤖 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/xml/FlatDtdProducerTest.java` around lines 222 - 223, In the test setup around FlatDtdWriter.write, create local variables for the DefaultDataSet and FlatDtdWriter instances before invoking write. Pass those variables to write instead of nesting constructors inside the call, while preserving the existing behavior.Source: Coding guidelines
src/main/java/org/dbunit/dataset/xml/FlatDtdWriter.java (1)
72-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a positive condition before the
else.Change
if (tableNames.length == 0)toif (tableNames.length > 0). Move theANYoutput into theelsebranch. This preserves the output and follows the repository condition rule.As per coding guidelines, when an
ifstatement has anelse, prefer a positive condition.🤖 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/xml/FlatDtdWriter.java` around lines 72 - 87, Update the condition guarding the dataset content-model output to check for tableNames.length > 0, keeping the existing table-listing logic in the if branch and moving the dataset ANY output into the else branch without changing either output.Source: Coding guidelines
src/main/java/org/dbunit/ext/mysql/MultiSchemaMySqlMetadataHandler.java (1)
159-184: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching the catalog list to avoid repeated enumeration.
listUserCatalogs()re-queriesmetaData.getCatalogs()every timegetTables,getColumns,getPrimaryKeys, ortableExistsruns without a single schema.DatabaseTableMetaDatacallsgetColumns/getPrimaryKeys/tableExistsonce per table (seeDatabaseTableMetaData.javacontext snippets), so for a schema with many tables this handler issues a repeatedSHOW DATABASES-style query per table. Cache the catalog list per instance, since the class Javadoc's usage pattern ties one handler instance to one connection.🤖 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/mysql/MultiSchemaMySqlMetadataHandler.java` around lines 159 - 184, Cache the result produced by listUserCatalogs() on the handler instance so repeated calls from getTables, getColumns, getPrimaryKeys, and tableExists reuse the same catalog list instead of invoking metaData.getCatalogs() again. Initialize the cache on the first call, preserve the existing filtering and returned catalog contents, and keep the per-instance behavior aligned with the handler’s connection lifecycle.
🤖 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/database/DatabaseConfig.java`:
- Around line 91-92: Update the ALL_FEATURES array in DatabaseConfig to include
FEATURE_SORT_ALL_COLUMNS_WHEN_NO_PRIMARY_KEY, preserving the existing feature
identifiers and ordering conventions so clients enumerating ALL_FEATURES can
discover it.
In `@src/main/java/org/dbunit/ext/mysql/MultiSchemaMySqlMetadataHandler.java`:
- Around line 90-97: Update getTables, getColumns, and getPrimaryKeys to close
every ResultSet already added to perCatalog when a later super.getX call fails,
while preserving normal merge behavior. In merge, move source cleanup to an
outer finally that closes all input ResultSets, ensuring failures from
getMetaData, next, or getObject cannot leak any source.
In `@src/test/java/org/dbunit/dataset/xml/FlatDtdWriterTest.java`:
- Around line 117-118: Update the assertion in
src/test/java/org/dbunit/dataset/xml/FlatDtdWriterTest.java:117-118 to use the
period-terminated failure description “Generated DTD output.” instead of
“output”. In src/test/java/org/dbunit/dataset/xml/FlatDtdProducerTest.java:233,
add the period-terminated AssertJ description “The empty DTD must produce no
tables.”.
In `@src/test/java/org/dbunit/ext/mysql/MultiSchemaMySqlMetadataHandlerTest.java`:
- Around line 73-237: Update every AssertJ .as() failure message in the test
methods, including those in
testGetTables_withSchemaConfigured_delegatesWithoutEnumeratingCatalogs,
testTableExists_withNoSchemaConfigured_returnsFalseWhenNotFoundInAnyUserCatalog,
and the surrounding tests, so each message ends with a trailing period. Preserve
the assertion logic and message wording otherwise.
---
Nitpick comments:
In `@src/main/java/org/dbunit/dataset/xml/FlatDtdWriter.java`:
- Around line 72-87: Update the condition guarding the dataset content-model
output to check for tableNames.length > 0, keeping the existing table-listing
logic in the if branch and moving the dataset ANY output into the else branch
without changing either output.
In `@src/main/java/org/dbunit/ext/mysql/MultiSchemaMySqlMetadataHandler.java`:
- Around line 159-184: Cache the result produced by listUserCatalogs() on the
handler instance so repeated calls from getTables, getColumns, getPrimaryKeys,
and tableExists reuse the same catalog list instead of invoking
metaData.getCatalogs() again. Initialize the cache on the first call, preserve
the existing filtering and returned catalog contents, and keep the per-instance
behavior aligned with the handler’s connection lifecycle.
In `@src/test/java/org/dbunit/dataset/xml/FlatDtdProducerTest.java`:
- Around line 222-223: In the test setup around FlatDtdWriter.write, create
local variables for the DefaultDataSet and FlatDtdWriter instances before
invoking write. Pass those variables to write instead of nesting constructors
inside the call, while preserving the existing behavior.
🪄 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: ca1bc80b-2beb-4287-a9a9-f110e08e6c57
📒 Files selected for processing (17)
src/changes/changes.xmlsrc/main/java/org/dbunit/database/AbstractDatabaseConnection.javasrc/main/java/org/dbunit/database/AbstractResultSetTable.javasrc/main/java/org/dbunit/database/DatabaseConfig.javasrc/main/java/org/dbunit/database/DatabaseDataSet.javasrc/main/java/org/dbunit/database/DatabaseTableMetaData.javasrc/main/java/org/dbunit/dataset/xml/FlatDtdWriter.javasrc/main/java/org/dbunit/ext/mysql/MultiSchemaMySqlMetadataHandler.javasrc/site/asciidoc/databases/mysql.adocsrc/site/asciidoc/properties.adocsrc/test/java/org/dbunit/database/AbstractDatabaseConnectionTest.javasrc/test/java/org/dbunit/database/DatabaseDataSetIT.javasrc/test/java/org/dbunit/database/DatabaseDataSetTest.javasrc/test/java/org/dbunit/database/DatabaseTableMetaDataIT.javasrc/test/java/org/dbunit/dataset/xml/FlatDtdProducerTest.javasrc/test/java/org/dbunit/dataset/xml/FlatDtdWriterTest.javasrc/test/java/org/dbunit/ext/mysql/MultiSchemaMySqlMetadataHandlerTest.java
![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: c52b575498
ℹ️ 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".
…review CodeRabbit's review of PR #880 found one real bug and several style gaps in that PR's new code: * MultiSchemaMySqlMetadataHandler.getTables()/getColumns()/ getPrimaryKeys() leaked already-opened per-catalog ResultSets when a later catalog's metadata call failed, since the failure happened before merge() (whose per-source cleanup never got a chance to run). merge() itself had the same gap: a mid-loop failure left not-yet-reached sources unclosed. Both now close every collected source on failure. * Cache the per-instance catalog list instead of re-querying getCatalogs() on every getTables()/getColumns()/getPrimaryKeys()/ tableExists() call - one handler instance is configured per connection, so the visible catalogs aren't expected to change over its lifetime. * Add the new FEATURE_SORT_ALL_COLUMNS_WHEN_NO_PRIMARY_KEY constant to DatabaseConfig.ALL_FEATURES, which had drifted out of sync with ALL_PROPERTIES. * FlatDtdWriter: use a positive if-condition paired with its else, per this project's own stated code style. * Split a nested constructor-chain call into local variables in FlatDtdProducerTest, and add period-terminated AssertJ .as() failure messages across the new test files, per this project's own stated test style. Verified via the full unit suite (1937 tests) and a full mysql-9-20 Docker regression run (347 tests), both green; added a dedicated regression test proving the leak fix (asserts the first catalog's ResultSet is closed when the second catalog's query throws).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Member
Author
|
Addressed CodeRabbit's review in ca979a5 (pushed). Replied inline to the 4 actionable comments; the 3 remaining nitpicks (bundled in the review summary rather than posted as separate threads) are also fixed:
Verified with the full unit suite (1937 tests) and a full |
DatabaseDataSet.nonLobColumns() excluded a column from the FEATURE_SORT_ALL_COLUMNS_WHEN_NO_PRIMARY_KEY fallback sort only when its DataType was reference-equal to the generic DataType.CLOB/ DataType.BLOB singletons. A vendor data type factory - e.g. OracleDataTypeFactory, whose CLOB/BLOB columns are typed OracleClobDataType/OracleBlobDataType - produces distinct DataType instances that never equal those singletons, so this check missed them: a no-PK Oracle table with those columns still got them in its ORDER BY, causing exactly the SQL error the feature exists to avoid. Check by type instead: OracleClobDataType extends ClobDataType and OracleBlobDataType/OracleXMLTypeDataType extend BlobDataType, so an instanceof check catches the generic types, Oracle's vendor types, and any future subtype without needing a per-vendor identity list. Found by Codex's automated review of PR #880. Refs: 171
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
jeffjensen
added a commit
that referenced
this pull request
Aug 2, 2026
…review CodeRabbit's review of PR #880 found one real bug and several style gaps in that PR's new code: * MultiSchemaMySqlMetadataHandler.getTables()/getColumns()/ getPrimaryKeys() leaked already-opened per-catalog ResultSets when a later catalog's metadata call failed, since the failure happened before merge() (whose per-source cleanup never got a chance to run). merge() itself had the same gap: a mid-loop failure left not-yet-reached sources unclosed. Both now close every collected source on failure. * Cache the per-instance catalog list instead of re-querying getCatalogs() on every getTables()/getColumns()/getPrimaryKeys()/ tableExists() call - one handler instance is configured per connection, so the visible catalogs aren't expected to change over its lifetime. * Add the new FEATURE_SORT_ALL_COLUMNS_WHEN_NO_PRIMARY_KEY constant to DatabaseConfig.ALL_FEATURES, which had drifted out of sync with ALL_PROPERTIES. * FlatDtdWriter: use a positive if-condition paired with its else, per this project's own stated code style. * Split a nested constructor-chain call into local variables in FlatDtdProducerTest, and add period-terminated AssertJ .as() failure messages across the new test files, per this project's own stated test style. Verified via the full unit suite (1937 tests) and a full mysql-9-20 Docker regression run (347 tests), both green; added a dedicated regression test proving the leak fix (asserts the first catalog's ResultSet is closed when the second catalog's query throws).
This was referenced Aug 6, 2026
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
Six independent fixes selected from
plan-docs/apply-issue-codes-plan.adoc's review of open GitHub issues that contain a concrete, still-applicable code suggestion:AbstractDatabaseConnection.getRowCount()now appliesDatabaseConfig.PROPERTY_ESCAPE_PATTERN, mirroringcreateTable().DatabaseTableMetaData.getPrimaryKeys()now falls back to the database-declared primary key when a configuredPROPERTY_PRIMARY_KEY_FILTERmatches zero columns for a table.DatabaseDataSet.initialize()now queries driver/database info for debug logging before opening the table-metadataResultSet, not after, avoiding a single-cursor-driver hazard.DatabaseConfig.FEATURE_SORT_ALL_COLUMNS_WHEN_NO_PRIMARY_KEYsorts a primary-key-less table'sSELECTby all non-LOB columns instead of leaving row order database-defined.FlatDtdWriternow emits valid<!ELEMENT dataset ANY>instead of an invalid, unparseable empty content model for a zero-table dataset.MultiSchemaMySqlMetadataHandlerworks around MySQL Connector/J'snullCatalogMeansCurrentdefault, which otherwise silently hides every catalog but one for a connection not restricted to a single schema.Each fix has its own commit with an accompanying test and
changes.xmlentry.Test plan
./mvnw test— full unit suite, 1936 tests, 0 failures./mvnw verify -Ph2-1-4— targeted IT runs for the DB-touching fixes (492, 628, 460, 171)./mvnw verify -Pmysql-9-20(Docker) — full regression pass, 347 tests, 0 failures, validating the new MySQL handler and confirming no regressions from the other five fixes./mvnw install site— confirms the package builds and the two updated doc pages (properties.adoc,databases/mysql.adoc) render correctly🤖 Generated with Claude Code
https://claude.ai/code/session_01P8mZjBXrYpB3zWNpN1wdC9
Summary by CodeRabbit