923 h2 information schema leak - #924
Conversation
…maMySqlMetadataHandler * Move the private nested InMemoryMetadataResultSet proxy out to its own public class in org.dbunit.database so other IMetadataHandler implementations can reuse it, not just MySQL's. * No behavior change; MultiSchemaMySqlMetadataHandler's merge() usage is unaffected. Refs: 923
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideIntroduce a reusable in-memory metadata ResultSet helper and an H2-specific metadata handler to filter out INFORMATION_SCHEMA tables when using H2 2.x, wire it into H2 connections and tests, and update documentation and build configuration accordingly. Sequence diagram for H2MetadataHandler getTables filtering INFORMATION_SCHEMAsequenceDiagram
participant Client
participant H2Connection
participant DatabaseConfig
participant H2MetadataHandler
participant DefaultMetadataHandler
participant InMemoryMetadataResultSet
Client->>H2Connection: new H2Connection(connection, schema)
H2Connection->>DatabaseConfig: setProperty(PROPERTY_METADATA_HANDLER, new H2MetadataHandler())
Client->>H2MetadataHandler: getTables(metaData, null, {"TABLE"})
H2MetadataHandler->>DefaultMetadataHandler: getTables(metaData, null, {"TABLE"})
DefaultMetadataHandler-->>H2MetadataHandler: ResultSet rawTables
H2MetadataHandler->>InMemoryMetadataResultSet: filter(rawTables, row -> !"INFORMATION_SCHEMA".equalsIgnoreCase(getSchema(row)))
InMemoryMetadataResultSet-->>H2MetadataHandler: ResultSet filteredTables
H2MetadataHandler-->>Client: ResultSet filteredTables
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: 8 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 (4)
📝 WalkthroughWalkthroughThe PR upgrades H2, adds H2-specific metadata filtering for ChangesH2 metadata compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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 left some high level feedback:
- Consider marking InMemoryMetadataResultSet.RowFilter as a @FunctionalInterface to make its intended use with lambdas clearer and catch accidental signature changes at compile time.
- In H2MetadataHandler.getTables, the INFORMATION_SCHEMA filtering currently relies on DefaultMetadataHandler#getSchema; if H2’s metadata shape ever diverges, a more direct check against TABLE_SCHEM via column index/label in the RowFilter would be more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider marking InMemoryMetadataResultSet.RowFilter as a @FunctionalInterface to make its intended use with lambdas clearer and catch accidental signature changes at compile time.
- In H2MetadataHandler.getTables, the INFORMATION_SCHEMA filtering currently relies on DefaultMetadataHandler#getSchema; if H2’s metadata shape ever diverges, a more direct check against TABLE_SCHEM via column index/label in the RowFilter would be more robust.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: 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/database/InMemoryMetadataResultSet.java`:
- Around line 163-175: Update closeAll to attempt SQLHelper.close for every
ResultSet even when a close fails, recording the first SQLException and
continuing through the remaining sources, then rethrowing that first failure
after the loop completes. Preserve the existing null/already-closed-safe
behavior and ensure the copy finally block does not lose the original exception
when cleanup also fails.
In `@src/main/java/org/dbunit/ext/h2/H2Connection.java`:
- Around line 52-53: Update the constructor JavaDoc associated with H2Connection
to document that its initialization configures both H2DataTypeFactory and
H2MetadataHandler, while preserving the existing description of the data type
factory.
In `@src/test/java/org/dbunit/ext/h2/H2MetadataHandlerTest.java`:
- Line 103: Rename the test method testGetTables_closesTheUnderlyingResultSet to
include the relevant starting state, while preserving its existing assertion
that getTables closes the underlying result set.
- Line 84: Update the assertion in H2MetadataHandlerTest by adding an AssertJ
.as() failure description before the existing isEqualTo("foo") call; ensure the
message clearly identifies the assertion and ends with a period.
🪄 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: f460a926-2333-4382-9d75-309717173511
📒 Files selected for processing (16)
pom.xmlsrc/changes/changes.xmlsrc/main/java/org/dbunit/database/InMemoryMetadataResultSet.javasrc/main/java/org/dbunit/ext/h2/H2Connection.javasrc/main/java/org/dbunit/ext/h2/H2MetadataHandler.javasrc/main/java/org/dbunit/ext/mysql/MultiSchemaMySqlMetadataHandler.javasrc/site/asciidoc/codingstandards/testconventions.adocsrc/site/asciidoc/components.adocsrc/site/asciidoc/components/imetadatahandler.adocsrc/site/asciidoc/databases.adocsrc/site/asciidoc/databases/h2.adocsrc/site/asciidoc/properties.adocsrc/site/site.xmlsrc/test/java/org/dbunit/database/DatabaseDataSet_MultiSchemaTest.javasrc/test/java/org/dbunit/database/DatabaseSequenceFilterIT.javasrc/test/java/org/dbunit/ext/h2/H2MetadataHandlerTest.java
* InMemoryMetadataResultSet#closeAll() aborted on the first ResultSet that failed to close, leaking every source after it. Track the first failure, attempt every source's close, then rethrow it. Add InMemoryMetadataResultSetTest covering it directly. * Mark RowFilter @FunctionalInterface. * Add Javadoc to invoke(), and to H2Connection's constructor, which now also configures H2MetadataHandler, not just H2DataTypeFactory. * H2MetadataHandlerTest: add a missing assertion .as() message, and the missing starting-state segment to a test method name. Addresses CodeRabbit/Sourcery feedback on PR #924. Refs: 923
|
Addressing Sourcery's review feedback:
|
Bump h2DriverVersion from 1.4.200 to 2.4.240 and adjust issues caused
by it.
* H2 2.x rewrote INFORMATION_SCHEMA to be SQL-standard-compliant, and
15 of its tables now report JDBC TABLE_TYPE = "BASE TABLE" instead
of the "SYSTEM TABLE" type H2 1.x used.
DatabaseConfig#PROPERTY_TABLE_TYPE defaults to {"TABLE"}, so those
tables now pass dbunit's default system-table filter and leak into
DatabaseDataSet's table listing whenever a query is not scoped to a
single schema (schema is null, e.g. an admin/multi-schema
connection).
* Add H2MetadataHandler, wired into H2Connection, whose getTables()
excludes the INFORMATION_SCHEMA schema. It builds the filtered
result via InMemoryMetadataResultSet#filter(ResultSet, RowFilter),
a new factory alongside the existing merge() one.
* Update DatabaseDataSet_MultiSchemaTest and DatabaseSequenceFilterIT,
both of which hit the same leak, to exercise/use H2MetadataHandler.
* Fix InMemoryMetadataResultSet#closeAll(), it aborted on the first
ResultSet that failed to close, leaking every source after it.
Track the first failure, attempt every source's close, then
rethrow it. Add InMemoryMetadataResultSetTest covering it directly.
Refs: 923
* Add components/imetadatahandler.adoc: the interface's method groups, a built-in-implementations table (DefaultMetadataHandler, Db2/MySql/MultiSchemaMySql/Netezza/H2MetadataHandler), and InMemoryMetadataResultSet's two factories (merge() for combining several real result sets, filter() for dropping rows out of one), with guidance on writing a custom handler. Cross-referenced from components.adoc, properties.adoc's metadataHandler entry, databases/h2.adoc, and site.xml's Core Components nav. * Update databases.adoc and databases/h2.adoc's IMetadataHandler/Connection Preconfiguration Class/Known Quirks sections for H2MetadataHandler. * Add the previously-undocumented MultiSchemaMySqlMetadataHandler to properties.adoc's metadataHandler entry alongside H2MetadataHandler. * Add a Test Conventions section (codingstandards/testconventions.adoc) on wrapping a delegate instead of subclassing a concrete vendor handler for test doubles, using DatabaseDataSet_MultiSchemaTest's TestMetadataHandler as the worked example. * Re-type changes.xml's existing 923 entry from fix to add, matching the GitHub issue's Bug-to-Feature retype, and add a second 923 entry for this documentation. Refs: 923
53601d7 to
977e44f
Compare
Summary by Sourcery
Add H2-specific metadata handling and shared in-memory ResultSet utilities to prevent H2 INFORMATION_SCHEMA tables from appearing as user tables, and update tests, documentation, and build configuration accordingly.
New Features:
Enhancements:
Build:
Documentation:
Summary by CodeRabbit
Bug Fixes
INFORMATION_SCHEMAsystem tables from metadata results, preserving visibility of user tables.Documentation