Fix DevCenter cards silently emitting zero rows#77
Merged
Conversation
The five dependency-version cards — LibraryUpgrade, GroovyVersionUpgrade, KotlinVersionUpgrade, ScalaVersionUpgrade, ParentPomUpgrade — read dependency versions by running a sibling recipe (DependencyInsight / ParentPomInsight) for the side effect of writing a data-table row, then fishing the row back out via DataTableRowWatcher. DependencyInsight's MarkIndividualDependency sub-visitor adds SearchResult markers, forcing OpenRewrite to schedule cycle 2. On cycle 2, `DataTable#insertRow` silently drops every write that doesn't override `allowWritingInThisCycle`. `DependenciesInUse` / `ParentPomsInUse` don't override it, so the watcher's getRows() snapshot diff is always zero and the cards emit no rows. This worked before the DataTableRowWatcher was retrofitted (#61) to read from the new `DataTableStore` interface — the previous implementation read from the ungated `org.openrewrite.dataTables` context-message bag. Replace the sibling-recipe + watcher pattern with direct reads from LST markers: `MavenResolutionResult.findDependencies(...)` and `ResolvedDependency.findDependencies(...)`. ParentPomUpgrade reads `Pom.getParent()` for immediate-parent matching (drops the recursive ancestor walk that ParentPomInsight did via MavenPomDownloader — DevCenter cards typically want immediate-parent matches anyway). DependencyVulnerabilityCheck uses the new `DependencyVulnerabilityCheckBase#vulnerabilities(Accumulator)` accessor exposed in rewrite-java-security to avoid the same coupling. The DataTableRowWatcher class itself is deleted. Tests were asserting on the `<!--~~(...)~~>--` SearchResult markers from the now-removed sub-recipe runs; those assertions are dropped. Requires rewrite-java-security to publish a release containing the new `DependencyVulnerabilityCheckBase#vulnerabilities(Accumulator)` method before this can compile against the published artifact.
Resolves conflict with #74 (DataTableRowWatcher fix for CsvDataTableStore). This branch's refactor removes all callers of DataTableRowWatcher, so the file is dropped entirely rather than fixed. Adapts the new `libraryUpgradeUnderCsvDataTableStore` regression test in DevCenterTest to the new mechanism: drops the `<!--~~(...)~~>` SearchResult marker assertions (DependencyInsight is no longer invoked) and the DependenciesInUse.csv assertions (LibraryUpgrade no longer populates that table as a side effect). The UpgradesAndMigrations.csv assertion remains and is the right regression test for this bug.
Same pattern as the per-card test fixes earlier on this branch: `UpgradesAndMigrationsTest` and `DevCenterTest#devcenterWithMultipleLibraryUpgradeRecipesHasCorrectData` were asserting on the `<!--~~>-->` / `~~(...)~~` `SearchResult` markers that `ParentPomInsight` and `DependencyInsight.MarkIndividualDependency` previously left on the source — those sub-recipes are no longer invoked after the cycle-gate refactor, so the markers don't get added and the expected-after assertions fail with "Recipe was expected to make a change but made no changes". Convert two-arg `pomXml(before, after, ...)` / `buildGradle(before, after)` to the single-arg `pomXml(before, ...)` / `buildGradle(before)` form; the `UpgradesAndMigrations.Row` data-table assertions remain and are the right test for the bug.
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
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
The five dependency-version DevCenter cards —
LibraryUpgrade,GroovyVersionUpgrade,KotlinVersionUpgrade,ScalaVersionUpgrade,ParentPomUpgrade— currently produce zero rows. The "Move to Spring Boot 4.0" card, for example, shows no data even on repos that clearly use Spring Boot.Root cause
These cards read dependency versions by running a sibling recipe (
DependencyInsight/ParentPomInsight) for the side effect of writing a row to its data table, then capturing the row viaDataTableRowWatcher.DependencyInsight.MarkIndividualDependencyaddsSearchResultmarkers to the LST, which forces OpenRewrite to schedule a cycle 2. On cycle 2,DataTable#insertRowsilently drops every write whoseDataTabledoesn't overrideallowWritingInThisCycle— andDependenciesInUse/ParentPomsInUsedon't. The watcher'sgetRows()snapshot diff comes back zero, and the cards emit nothing.DataTableRowWatcherread from the ungatedorg.openrewrite.dataTablescontext-message bag rather than the cycle-gatedDataTableStore.Fix
Replace the sibling-recipe + watcher pattern with direct reads from LST markers via
MavenResolutionResult.findDependencies(...)andResolvedDependency.findDependencies(...). A shared helperResolvedDependencyVersions.findVersions(SourceFile, groupIdPattern, artifactIdPattern)walks both Maven and Gradle markers; the four version-card recipes route through it.ParentPomUpgradereadsPom.getParent()for immediate-parent matching only — drops the recursive ancestor walk thatParentPomInsightdid viaMavenPomDownloader. DevCenter cards typically want to track the closest parent (e.g. a corporate parent that extends spring-boot-starter-parent); if there's a use case for transitive grand-parent matching, happy to add it back.DependencyVulnerabilityCheckuses the newDependencyVulnerabilityCheckBase#vulnerabilities(Accumulator)accessor (companion PR moderneinc/rewrite-java-security#354) to avoid the same coupling.DataTableRowWatcheris deleted; no remaining consumers.Tests previously asserted on
<!--~~(...)~~>--SearchResultmarkers from the now-removed sub-recipe runs; those assertions are dropped.Test plan
./gradlew :test --tests 'LibraryUpgradeTest' --tests 'GroovyVersionUpgradeTest' --tests 'KotlinVersionUpgradeTest' --tests 'ScalaVersionUpgradeTest' --tests 'ParentPomUpgradeTest'— all pass locallymod git sync moderne ./working-set Default && mod run --recipe=io.moderne.devcenter.DevCenterStarter && mod devcenterproduces non-empty "Move to Spring Boot 4.0" rowsDependency