feat(database): Add FEATURE_SKIP_CYCLE_CHECK for cyclic FK schemas - #917
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 GuideAdds a new DatabaseConfig feature flag, FEATURE_SKIP_CYCLE_CHECK, that allows DatabaseSequenceFilter to bypass foreign-key cycle rejection and instead append cyclic tables in input order so all requested tables are returned once, with accompanying unit/integration tests and documentation/changelog updates. Sequence diagram for DatabaseSequenceFilter cycle handling with FEATURE_SKIP_CYCLE_CHECKsequenceDiagram
participant Caller
participant IDatabaseConnection as IDatabaseConnection
participant DatabaseSequenceFilter as DatabaseSequenceFilter
participant DatabaseConfig as DatabaseConfig
participant DependencyInfo as DependencyInfo
Caller->>DatabaseSequenceFilter: sortTableNames(connection, tableNames)
DatabaseSequenceFilter->>IDatabaseConnection: getConfig()
IDatabaseConnection-->>DatabaseSequenceFilter: DatabaseConfig
DatabaseSequenceFilter->>DatabaseConfig: getFeature(FEATURE_SKIP_CYCLE_CHECK)
DatabaseConfig-->>DatabaseSequenceFilter: skipCycleCheck
loop for each DependencyInfo in dependencies.values()
DatabaseSequenceFilter->>DependencyInfo: checkCycles()
alt CyclicTablesDependencyException thrown
opt skipCycleCheck is false
DependencyInfo-->>DatabaseSequenceFilter: CyclicTablesDependencyException
DatabaseSequenceFilter-->>Caller: throw CyclicTablesDependencyException
end
opt skipCycleCheck is true
DependencyInfo-->>DatabaseSequenceFilter: CyclicTablesDependencyException
DatabaseSequenceFilter->>DatabaseSequenceFilter: logger.warn(...)
Note right of DatabaseSequenceFilter: cycle detected but ignored
end
end
end
DatabaseSequenceFilter->>DatabaseSequenceFilter: sort(connection, tableNames, dependencies)
alt some tables never reach in-degree 0
DatabaseSequenceFilter->>DatabaseSequenceFilter: append remaining tables in input order
end
DatabaseSequenceFilter-->>Caller: sortedTableNames
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 17 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 (3)
📝 WalkthroughWalkthroughThe change adds the opt-in ChangesCyclic dependency handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DatabaseSequenceFilter
participant DatabaseConfig
participant Logger
participant CyclicTablesDependencyException
DatabaseSequenceFilter->>DatabaseConfig: read FEATURE_SKIP_CYCLE_CHECK
alt cycle checking enabled
DatabaseSequenceFilter->>CyclicTablesDependencyException: throw for cyclic dependencies
else cycle checking skipped
DatabaseSequenceFilter->>Logger: log one warning per cycle
DatabaseSequenceFilter-->>DatabaseSequenceFilter: order components and restore input order
end
🚥 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 left some high level feedback:
- When FEATURE_SKIP_CYCLE_CHECK is enabled you still invoke DependencyInfo.checkCycles() for every table and then catch/ignore CyclicTablesDependencyException, which keeps the full cycle-detection cost; consider short-circuiting or providing a cheaper ‘presence-only’ check so opt-in callers don’t pay for work they explicitly chose to skip.
- The cycle-handling branch in sort() appends all remaining tables purely in their original input order; if there are multiple disjoint cycles or chains reachable through cycles, you might want to document or enforce a more predictable ordering (e.g., per-cycle grouping) to make behavior easier to reason about for callers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When FEATURE_SKIP_CYCLE_CHECK is enabled you still invoke DependencyInfo.checkCycles() for every table and then catch/ignore CyclicTablesDependencyException, which keeps the full cycle-detection cost; consider short-circuiting or providing a cheaper ‘presence-only’ check so opt-in callers don’t pay for work they explicitly chose to skip.
- The cycle-handling branch in sort() appends all remaining tables purely in their original input order; if there are multiple disjoint cycles or chains reachable through cycles, you might want to document or enforce a more predictable ordering (e.g., per-cycle grouping) to make behavior easier to reason about for callers.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: 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/main/java/org/dbunit/database/DatabaseSequenceFilter.java`:
- Around line 254-274: The fallback in DatabaseSequenceFilter must not append
all unresolved tables in raw input order; replace it with strongly connected
component collapse and topological ordering so cyclic component members retain
input order while acyclic descendants remain after their dependencies. In
src/main/java/org/dbunit/database/DatabaseSequenceFilter.java#L254-L274,
implement this ordering change. In
src/test/java/org/dbunit/database/DatabaseSequenceFilterTest.java#L230-L260, add
C -> A with A <-> B and assert A precedes C. In
src/changes/changes.xml#L245-L247, remove the claim that tables reachable
through a cycle are appended in input order.
🪄 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: 168ee5d1-bc90-4b59-871d-bd261c34efff
📒 Files selected for processing (8)
src/changes/changes.xmlsrc/main/java/org/dbunit/database/DatabaseConfig.javasrc/main/java/org/dbunit/database/DatabaseSequenceFilter.javasrc/site/asciidoc/filters.adocsrc/site/asciidoc/properties.adocsrc/test/java/org/dbunit/database/DatabaseConfigTest.javasrc/test/java/org/dbunit/database/DatabaseSequenceFilterIT.javasrc/test/java/org/dbunit/database/DatabaseSequenceFilterTest.java
Member
Author
|
Addressing @sourcery-ai's two review points:
|
jeffjensen
force-pushed
the
501-skip-cyclic-dependency-check
branch
2 times, most recently
from
August 8, 2026 03:01
b500959 to
4c63e4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/test/java/org/dbunit/database/DatabaseSequenceFilterTest.java (2)
208-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse AssertJ for the exception assertion.
The rest of this file asserts with AssertJ. The coding guidelines prefer AssertJ and
.as()failure messages. ReplaceassertThrowswithassertThatThrownBy.♻️ Proposed refactor
- assertThrows(CyclicTablesDependencyException.class, - () -> DatabaseSequenceFilter.sortTableNames(connection, - new String[] {"A", "B"}), - "A foreign key dependency cycle between A and B must be rejected by default."); + assertThatThrownBy(() -> DatabaseSequenceFilter.sortTableNames(connection, + new String[] {"A", "B"})) + .as("A foreign key dependency cycle between A and B must be rejected by default.") + .isInstanceOf(CyclicTablesDependencyException.class);🤖 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/database/DatabaseSequenceFilterTest.java` around lines 208 - 211, In DatabaseSequenceFilterTest, replace the assertThrows assertion around DatabaseSequenceFilter.sortTableNames with AssertJ’s assertThatThrownBy, assert the CyclicTablesDependencyException type, and preserve the existing failure description using AssertJ’s .as() message.Source: Coding guidelines
315-335: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet the logger level explicitly so the test does not depend on the ambient logging configuration.
Logback applies the effective level before it calls appenders. If a logback configuration on the test classpath sets the level for
org.dbunitor the root logger aboveWARN, the warning never reachesappender, andhasSize(1)receives 0. Set the level toWARNfor the duration of the test and restore the previous value. Stop the appender in the samefinallyblock.🛠️ Proposed change
final Logger filterLogger = (Logger) LoggerFactory.getLogger(DatabaseSequenceFilter.class); + final Level previousLevel = filterLogger.getLevel(); final ListAppender<ILoggingEvent> appender = new ListAppender<>(); appender.start(); + filterLogger.setLevel(Level.WARN); filterLogger.addAppender(appender); try { @@ finally { filterLogger.detachAppender(appender); + appender.stop(); + filterLogger.setLevel(previousLevel); }🤖 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/database/DatabaseSequenceFilterTest.java` around lines 315 - 335, Update the logger setup in the cycle-warning test around DatabaseSequenceFilter.sortTableNames to save the filterLogger’s existing level, set it explicitly to WARN before invoking the code under test, and restore the saved level in finally. Also stop the ListAppender in that same cleanup block after detaching it.src/main/java/org/dbunit/database/DatabaseSequenceFilter.java (2)
334-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
getCyclicDependencies()here.Lines 335-336 repeat the intersection that
DependencyInfo.getCyclicDependencies()already performs. Call the accessor to keep one definition of "mutually reachable".♻️ Proposed refactor
DependencyInfo info = (DependencyInfo) dependencies.get(tableNames[i]); - Set mutuallyReachable = new HashSet(info.getAllTableDependsOn()); - mutuallyReachable.retainAll(info.getAllTableDependent()); + Set mutuallyReachable = info.getCyclicDependencies();🤖 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/database/DatabaseSequenceFilter.java` around lines 334 - 336, In the table dependency processing around DependencyInfo, replace the manual HashSet intersection of getAllTableDependsOn() and getAllTableDependent() with a call to info.getCyclicDependencies(). Preserve the existing mutuallyReachable usage and behavior while relying on the accessor as the single definition of mutually reachable dependencies.
494-524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the JavaDoc for the changed constructor and the new accessor.
The constructor signature now takes
directDependsOnTablesSetas the second parameter. The JavaDoc does not document it, and the listed parameter order does not match the signature.@param tableNamehas no description. The existing descriptions do not end with a period.getDirectDependsOnTablesSet()has no JavaDoc.The coding guidelines require complete sentences that start with a capital letter and end with a period for topic text, parameters, and return descriptions.
📝 Proposed documentation
/** - * `@param` tableName - * `@param` allTableDependsOn Tables that are required as prerequisite so that this one can exist - * `@param` allTableDependent Tables that need this one in order to be able to exist + * Creates the dependency information for one table. + * + * `@param` tableName The name of the table this information describes. + * `@param` directDependsOnTablesSet The tables this one references directly through a + * foreign key. + * `@param` allTableDependsOn The tables that are required as a prerequisite so that this + * one can exist. + * `@param` allTableDependent The tables that need this one in order to be able to exist. */+ /** + * Returns the tables this one references directly through a foreign key. + * + * `@return` The direct prerequisite tables. + */ public Set getDirectDependsOnTablesSet() { return directDependsOnTablesSet; }🤖 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/database/DatabaseSequenceFilter.java` around lines 494 - 524, Complete the JavaDoc for DependencyInfo’s constructor by documenting parameters in signature order, including directDependsOnTablesSet, and give tableName a capitalized, period-terminated description; update all existing parameter descriptions to complete sentences ending with periods. Add JavaDoc to getDirectDependsOnTablesSet() with a complete return description following the same sentence style.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/main/java/org/dbunit/database/DatabaseSequenceFilter.java`:
- Around line 268-299: In the topological-sort expansion surrounding
sortedComponentCount and sortedTableNames, detect when Kahn’s algorithm emits
fewer components than componentCount and fail immediately with an appropriate
exception instead of returning trailing null entries. Keep the existing
expansion behavior unchanged for complete sorts, and ensure the guard runs
before the incomplete sortedTableNames array can propagate.
In `@src/test/java/org/dbunit/database/DatabaseSequenceFilterTest.java`:
- Line 237: Update the test method name and failure message in
testSort_cyclicPairWithNonCyclicParentAndSkipCycleCheckEnabled to remove the
stale “appended” wording and describe the cycle as following its parent in input
order. Preserve the existing assertions and expected behavior; change only the
naming and message text.
---
Nitpick comments:
In `@src/main/java/org/dbunit/database/DatabaseSequenceFilter.java`:
- Around line 334-336: In the table dependency processing around DependencyInfo,
replace the manual HashSet intersection of getAllTableDependsOn() and
getAllTableDependent() with a call to info.getCyclicDependencies(). Preserve the
existing mutuallyReachable usage and behavior while relying on the accessor as
the single definition of mutually reachable dependencies.
- Around line 494-524: Complete the JavaDoc for DependencyInfo’s constructor by
documenting parameters in signature order, including directDependsOnTablesSet,
and give tableName a capitalized, period-terminated description; update all
existing parameter descriptions to complete sentences ending with periods. Add
JavaDoc to getDirectDependsOnTablesSet() with a complete return description
following the same sentence style.
In `@src/test/java/org/dbunit/database/DatabaseSequenceFilterTest.java`:
- Around line 208-211: In DatabaseSequenceFilterTest, replace the assertThrows
assertion around DatabaseSequenceFilter.sortTableNames with AssertJ’s
assertThatThrownBy, assert the CyclicTablesDependencyException type, and
preserve the existing failure description using AssertJ’s .as() message.
- Around line 315-335: Update the logger setup in the cycle-warning test around
DatabaseSequenceFilter.sortTableNames to save the filterLogger’s existing level,
set it explicitly to WARN before invoking the code under test, and restore the
saved level in finally. Also stop the ListAppender in that same cleanup block
after detaching it.
🪄 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: 02592dcf-3709-4bfe-87fe-e99dc3df0f93
📒 Files selected for processing (6)
src/changes/changes.xmlsrc/main/java/org/dbunit/database/DatabaseSequenceFilter.javasrc/site/asciidoc/filters.adocsrc/site/asciidoc/properties.adocsrc/test/java/org/dbunit/database/DatabaseSequenceFilterIT.javasrc/test/java/org/dbunit/database/DatabaseSequenceFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/changes/changes.xml
- src/site/asciidoc/filters.adoc
- src/test/java/org/dbunit/database/DatabaseSequenceFilterIT.java
- src/site/asciidoc/properties.adoc
DatabaseSequenceFilter unconditionally rejected any schema with a foreign-key dependency cycle via CyclicTablesDependencyException, with no way to opt out even when the cycle is handled another way (nullable FK columns backfilled later, or database-side deferred constraint checking). * Add DatabaseConfig.FEATURE_SKIP_CYCLE_CHECK (default false, preserving prior behavior). When enabled, DatabaseSequenceFilter.DependencyInfo.checkCycles() logs a warning instead of throwing for each detected cycle. * Make DatabaseSequenceFilter.sort() handle a cycle correctly instead of just avoiding the exception: tables are first grouped into strongly connected components (reusing the same mutual-reachability check checkCycles() already performs), each cycle is condensed into a single component, and the condensed component graph - always acyclic by construction - is topologically sorted before expanding each component back into its member tables in their original input order. A table that merely depends on a cyclic table, without itself being part of the cycle, is therefore still ordered correctly after the whole component it depends on; only the relative order of the tables making up the cycle itself falls back to input order. * Add unit coverage in DatabaseSequenceFilterTest (default still throws; a two-table cycle with the feature enabled; a non-cyclic parent still sorting before an appended cycle; and a table that depends on, but is not part of, a cyclic component still sorting after the whole component) and DatabaseConfigTest (default value), plus a DatabaseSequenceFilterIT case reusing the existing 5-table hypersonic_cyclic.sql fixture, verified against the h2-1-4 and hsqldb-2-7 profiles. * Document the feature in filters.adoc (new "Cyclic foreign-key dependencies" subsection) and properties.adoc's Feature Flags table. Refs: 501 Refs: 517 Refs: 411 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gmZhDioasFdPtnqxs1ZuP
jeffjensen
force-pushed
the
501-skip-cyclic-dependency-check
branch
from
August 8, 2026 03:41
4c63e4c to
8f670b0
Compare
Member
Author
|
@coderabbitai review |
|
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
DatabaseConfig.FEATURE_SKIP_CYCLE_CHECK(defaultfalse), an opt-in escape hatch lettingDatabaseSequenceFilterproceed on a schema with a foreign-key dependency cycle instead of unconditionally rejecting it withCyclicTablesDependencyException.filters.adocandproperties.adoc;changes.xmlupdated.Fixes #501
Fixes #517
Fixes #411
Test plan
DatabaseSequenceFilterTest(default still throws on a cyclic pair; skip-enabled two-table cycle does not throw and returns both tables; skip-enabled three-table case proves a non-cyclic parent still sorts before the appended cycle) andDatabaseConfigTest(default value isfalse)DatabaseSequenceFilterITcase reusing the existing 5-tablehypersonic_cyclic.sqlfixture, green against theh2-1-4andhsqldb-2-7profilesmvnw clean install sitegreen (Javadoc/Checkstyle/AsciiDoc site build, no new warnings)🤖 Generated with Claude Code
https://claude.ai/code/session_012gmZhDioasFdPtnqxs1ZuP
Summary by Sourcery
Introduce a configurable escape hatch to allow DatabaseSequenceFilter to operate on schemas with circular foreign-key dependencies without failing fast.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests