Refactor topological ordering to support multiple groups of indexers …#108
Merged
daithihearn merged 4 commits intomainfrom Mar 4, 2026
Merged
Refactor topological ordering to support multiple groups of indexers …#108daithihearn merged 4 commits intomainfrom
daithihearn merged 4 commits intomainfrom
Conversation
…based on dependencies
📦 Snapshot PublishedUsageAdd the GitHub Packages repository to your repositories {
maven {
url = uri("https://maven.pkg.github.com/vechain/indexer-core")
credentials {
username = findProperty("gpr.user") as String? ?: System.getenv("GITHUB_ACTOR")
password = findProperty("gpr.key") as String? ?: System.getenv("GITHUB_TOKEN")
}
}
}Then use the dependency: implementation("org.vechain:indexer-core:pr.108.bf79585-SNAPSHOT")Note: Requires a GitHub PAT with |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces dependency-aware ordering utilities: adds Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Topological as TopologicalOrder
participant DepSort as dependencySort
participant CC as connectedComponents
participant Result as Result
Client->>Topological: topologicalOrder(List<Indexer>)
Topological->>DepSort: dependencySort(indexers)
DepSort-->>Topological: ordered Indexer list
Topological->>CC: connectedComponents(indexers)
CC-->>Topological: Map<Indexer, Indexer> (component roots)
Topological->>Topological: partition ordered list by component
Topological-->>Result: List<List<Indexer>> (groups per component)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (3)
src/test/kotlin/org/vechain/indexer/utils/IndexerOrderUtilsTest.kt (2)
292-304: Tighten negative-path assertions with message checks.These tests currently pass for any
IllegalStateException/IllegalArgumentException, including unrelated failures. Assert the message (or stable substring) to verify the intended failure path.Suggested enhancement
- assertThrows<IllegalStateException> { - IndexerOrderUtils.dependencySort(listOf(indexer1, indexer2)) - } + val exception = assertThrows<IllegalStateException> { + IndexerOrderUtils.dependencySort(listOf(indexer1, indexer2)) + } + expectThat(exception.message?.contains("Circular dependency detected")).isEqualTo(true)- assertThrows<IllegalArgumentException> { - IndexerOrderUtils.dependencySort(listOf(indexer1)) - } + val exception = assertThrows<IllegalArgumentException> { + IndexerOrderUtils.dependencySort(listOf(indexer1)) + } + expectThat(exception.message?.contains("not part of the provided indexers")).isEqualTo(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/org/vechain/indexer/utils/IndexerOrderUtilsTest.kt` around lines 292 - 304, Update the two negative-path tests to assert the exception message (or a stable substring) so they only pass for the intended failure: wrap the call to IndexerOrderUtils.dependencySort(...) in assertThrows as before but capture the exception and assert its message contains the expected text (e.g., for the cyclic dependency case in the test named `should reject circular dependency` assert the IllegalStateException message includes "circular" or similar, and for `should reject missing dependency` assert the IllegalArgumentException message includes "missing" or the missing indexer id). Use the exception instance returned by assertThrows to perform the message contains check so the tests fail if a different error is thrown.
58-61: Avoid over-constraining order for disconnected components and sibling nodes.These assertions require a specific component index (
result[0],result[1], …) and sibling order, but topological output is valid as long as dependency precedence is preserved. This can make tests brittle across equivalent implementations.Proposed test-hardening pattern
- expectThat(result.size).isEqualTo(2) - expectThat(result[0]).containsExactly(indexer1, indexer3, indexer5, indexer6) - expectThat(result[1]).containsExactly(indexer2, indexer4) + expectThat(result.size).isEqualTo(2) + val groupsByNames = result.map { group -> group.map { it.name }.toSet() }.toSet() + expectThat(groupsByNames).isEqualTo( + setOf( + setOf("indexer1", "indexer3", "indexer5", "indexer6"), + setOf("indexer2", "indexer4"), + ) + )Also applies to: 114-116, 215-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/org/vechain/indexer/utils/IndexerOrderUtilsTest.kt` around lines 58 - 61, The test over-constrains topological ordering by asserting exact positions (result[0], result[1], result[2]) and sibling order for indexer1/indexer2/indexer3; instead, keep the size check but relax element-position checks: assert that each group (the lists in result) contains the expected indexers in any order (use containsExactlyInAnyOrder-style semantics) and add only the necessary precedence assertions (e.g., assert that indexerA comes before indexerB by comparing their indices in the flattened result) so dependency precedence is verified without fixing sibling ordering; apply this change for the assertions referencing result, indexer1, indexer2, indexer3 and the similar blocks at the other noted locations.src/main/kotlin/org.vechain.indexer/utils/IndexerOrderUtils.kt (1)
105-109: Consider fail-fast validation inconnectedComponentsfor missing dependencies.
connectedComponentssilently ignores missing dependencies, whiledependencySortthrows. Since this is now a standalone utility, aligning behavior would avoid masking invalid dependency wiring.Proposed change
- if (dep != null && dep in indexerSet) { - parent.putIfAbsent(dep, dep) - union(indexer, dep) - } + if (dep != null) { + require(dep in indexerSet) { + "Dependency ${dep.name} for ${indexer.name} is not part of the provided indexers" + } + parent.putIfAbsent(dep, dep) + union(indexer, dep) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/org.vechain.indexer/utils/IndexerOrderUtils.kt` around lines 105 - 109, connectedComponents currently ignores missing dependencies (it only unions when dep in indexerSet) which masks invalid wiring; update connectedComponents to validate each indexer.dependsOn and fail fast (throw an IllegalArgumentException or similar) when dep is non-null but not present in indexerSet, rather than silently skipping it—locate the block using symbols indexer.dependsOn, indexerSet, parent.putIfAbsent and union and add the presence check that raises the error before calling parent.putIfAbsent/union so behavior matches dependencySort.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/kotlin/org.vechain.indexer/utils/IndexerOrderUtils.kt`:
- Around line 105-109: connectedComponents currently ignores missing
dependencies (it only unions when dep in indexerSet) which masks invalid wiring;
update connectedComponents to validate each indexer.dependsOn and fail fast
(throw an IllegalArgumentException or similar) when dep is non-null but not
present in indexerSet, rather than silently skipping it—locate the block using
symbols indexer.dependsOn, indexerSet, parent.putIfAbsent and union and add the
presence check that raises the error before calling parent.putIfAbsent/union so
behavior matches dependencySort.
In `@src/test/kotlin/org/vechain/indexer/utils/IndexerOrderUtilsTest.kt`:
- Around line 292-304: Update the two negative-path tests to assert the
exception message (or a stable substring) so they only pass for the intended
failure: wrap the call to IndexerOrderUtils.dependencySort(...) in assertThrows
as before but capture the exception and assert its message contains the expected
text (e.g., for the cyclic dependency case in the test named `should reject
circular dependency` assert the IllegalStateException message includes
"circular" or similar, and for `should reject missing dependency` assert the
IllegalArgumentException message includes "missing" or the missing indexer id).
Use the exception instance returned by assertThrows to perform the message
contains check so the tests fail if a different error is thrown.
- Around line 58-61: The test over-constrains topological ordering by asserting
exact positions (result[0], result[1], result[2]) and sibling order for
indexer1/indexer2/indexer3; instead, keep the size check but relax
element-position checks: assert that each group (the lists in result) contains
the expected indexers in any order (use containsExactlyInAnyOrder-style
semantics) and add only the necessary precedence assertions (e.g., assert that
indexerA comes before indexerB by comparing their indices in the flattened
result) so dependency precedence is verified without fixing sibling ordering;
apply this change for the assertions referencing result, indexer1, indexer2,
indexer3 and the similar blocks at the other noted locations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c9e58ba-33dd-4e67-a324-a5665b611fd6
📒 Files selected for processing (2)
src/main/kotlin/org.vechain.indexer/utils/IndexerOrderUtils.ktsrc/test/kotlin/org/vechain/indexer/utils/IndexerOrderUtilsTest.kt
Both logExecutionGroups and logProximityGroups now use info level and the same multi-line format with group/indexer counts and block ranges. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
…based on dependencies
Summary by CodeRabbit
New Features
Chores