fix: enhance metadata handling and extraction process, improve test c…#47
Open
fix: enhance metadata handling and extraction process, improve test c…#47
Conversation
📝 WalkthroughWalkthroughAdds metadata-origin tracking (live/snapshot/cache) through MetadataSelection/CatalogData and related sinks, makes snapshot-cache writes conditional on live metadata, removes build-tools fallback in QuarkusApiClient, introduces cooperative cancellation into SafeZipExtractor and delegates extraction cancellation to ProjectArchiveService, and adjusts package-name recompute logic for overridden coordinates. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProjectArchiveService
participant SafeZipExtractor
participant Filesystem
Caller->>ProjectArchiveService: downloadAndExtract(url, output, overwrite, cancelled)
ProjectArchiveService->>ProjectArchiveService: pre-check cancelled
ProjectArchiveService->>SafeZipExtractor: extract(zipFile, staging, overwrite, cancelled)
SafeZipExtractor->>SafeZipExtractor: throwIfCancelled() before reading central dir
SafeZipExtractor->>Filesystem: read ZIP entries
loop per-entry
SafeZipExtractor->>SafeZipExtractor: throwIfCancelled() at entry boundary
SafeZipExtractor->>Filesystem: read entry bytes
SafeZipExtractor->>Filesystem: write to staging
SafeZipExtractor->>SafeZipExtractor: throwIfCancelled() inside read/write loop
end
SafeZipExtractor->>SafeZipExtractor: throwIfCancelled() after staging before apply
SafeZipExtractor->>Filesystem: apply staging -> output (respect overwrite policy)
SafeZipExtractor-->>ProjectArchiveService: ExtractionResult / CancellationException
ProjectArchiveService-->>Caller: return or propagate CancellationException
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/dev/ayagmar/quarkusforge/application/DefaultStartupStateService.java (1)
41-45: Make recomputation intent explicit by usingnulldirectly.This branch currently relies on
defaults.packageName()stayingnullto trigger downstream recomputation. Usingnullhere directly is clearer and avoids accidental behavior drift if defaults change later.Suggested change
- String packageName = - coordinatesOverridden && !hasText(safeRequestedPrefill.packageName()) - ? defaults.packageName() - : preferRequested( - safeRequestedPrefill.packageName(), storedPrefill, CliPrefill::packageName, defaults); + String packageName = + coordinatesOverridden && !hasText(safeRequestedPrefill.packageName()) + ? null + : preferRequested( + safeRequestedPrefill.packageName(), storedPrefill, CliPrefill::packageName, defaults);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ayagmar/quarkusforge/application/DefaultStartupStateService.java` around lines 41 - 45, The branch that sets packageName should explicitly use null when you want downstream recomputation rather than relying on defaults.packageName() being null: in DefaultStartupStateService where packageName is assigned (the expression involving coordinatesOverridden, hasText, safeRequestedPrefill.packageName(), and preferRequested), replace the use of defaults.packageName() with a direct null to make the recomputation intent explicit and avoid accidental behavior if defaults change.src/main/java/dev/ayagmar/quarkusforge/api/QuarkusApiClient.java (1)
97-103: Keep live streams when only build-tool extraction fails.Removing the recovery here means any
/q/openapi404 or contract error now failsfetchMetadata()wholesale.CatalogDataService.loadMetadataSelection()then falls back to the bundled snapshot, so a build-tool-only problem also drops the fresh/api/streamsresult. Consider degrading only the build-tools portion instead of replacing the entire metadata set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ayagmar/quarkusforge/api/QuarkusApiClient.java` around lines 97 - 103, The current combination of streamsMetadataFuture and buildToolsFuture causes fetchMetadata() to fail entirely when the build-tools extraction (the chain starting with sendWithRetry(() -> transport.sendStringAsync(openApiRequest), 1) thenApply(this::assertSuccessful).thenApply(ApiStringResponse::body).thenApply(ApiPayloadParser::parseBuildToolsFromOpenApiPayload)) throws (e.g., /q/openapi 404), so CatalogDataService.loadMetadataSelection() loses fresh /api/streams results; change the buildToolsFuture to recover failures locally (e.g., use exceptionally/handle on the future returned by sendWithRetry / ApiPayloadParser.parseBuildToolsFromOpenApiPayload) to return a safe default (empty List<String> or a preserved previous build-tools set) and log the error, and then keep the streamsMetadataFuture.thenCombine(buildToolsFuture, QuarkusApiClient::toMetadata) unchanged so stream metadata remains available when only build-tool extraction fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/ayagmar/quarkusforge/api/CatalogDataService.java`:
- Around line 58-60: Spotless formatting failed on the if-block around
metadataSelection.liveMetadata() — re-run the project formatter and commit the
formatted result so the wrap around the snapshotCache.write(...) and subsequent
if (!writeOutcome.written()) lines matches the project style; specifically run
the repository's Spotless apply task (e.g., mvn spotless:apply or gradle
spotlessApply), reformat the block containing metadataSelection.liveMetadata(),
snapshotCache.write(...), CacheWriteOutcome writeOutcome and
writeOutcome.written(), then add the formatted file to the commit.
In `@src/main/java/dev/ayagmar/quarkusforge/archive/SafeZipExtractor.java`:
- Around line 45-53: The extract(...) method in SafeZipExtractor should fail
fast when the provided BooleanSupplier cancelled is already set: after the
existing Objects.requireNonNull checks (inside the extract method) call
cancelled.getAsBoolean() and immediately return an appropriate cancelled
ExtractionResult (e.g., ExtractionResult.CANCELLED or a new cancelled result)
instead of proceeding to parse the central directory or create directories;
update any code paths that assume work has started to use this early-return so
callers like ProjectArchiveService don't trigger work after EXTRACTING_ARCHIVE
flips the flag.
In `@src/test/java/dev/ayagmar/quarkusforge/api/CatalogDataServiceTest.java`:
- Line 146: The failing Spotless check is due to line wrapping on the
CachedCatalogSnapshot creation; reformat the expression using the project's
formatter so it meets style rules (e.g., break into multiple lines if required)
around CatalogSnapshotCache(cacheFile).read().orElseThrow() or otherwise adjust
whitespace to satisfy Spotless; then re-run the formatter/Spotless before
committing so the CI passes.
---
Nitpick comments:
In `@src/main/java/dev/ayagmar/quarkusforge/api/QuarkusApiClient.java`:
- Around line 97-103: The current combination of streamsMetadataFuture and
buildToolsFuture causes fetchMetadata() to fail entirely when the build-tools
extraction (the chain starting with sendWithRetry(() ->
transport.sendStringAsync(openApiRequest), 1)
thenApply(this::assertSuccessful).thenApply(ApiStringResponse::body).thenApply(ApiPayloadParser::parseBuildToolsFromOpenApiPayload))
throws (e.g., /q/openapi 404), so CatalogDataService.loadMetadataSelection()
loses fresh /api/streams results; change the buildToolsFuture to recover
failures locally (e.g., use exceptionally/handle on the future returned by
sendWithRetry / ApiPayloadParser.parseBuildToolsFromOpenApiPayload) to return a
safe default (empty List<String> or a preserved previous build-tools set) and
log the error, and then keep the
streamsMetadataFuture.thenCombine(buildToolsFuture,
QuarkusApiClient::toMetadata) unchanged so stream metadata remains available
when only build-tool extraction fails.
In
`@src/main/java/dev/ayagmar/quarkusforge/application/DefaultStartupStateService.java`:
- Around line 41-45: The branch that sets packageName should explicitly use null
when you want downstream recomputation rather than relying on
defaults.packageName() being null: in DefaultStartupStateService where
packageName is assigned (the expression involving coordinatesOverridden,
hasText, safeRequestedPrefill.packageName(), and preferRequested), replace the
use of defaults.packageName() with a direct null to make the recomputation
intent explicit and avoid accidental behavior if defaults change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75e22c38-7353-4c1b-a76e-e6370d998e4c
📒 Files selected for processing (10)
src/main/java/dev/ayagmar/quarkusforge/api/CatalogDataService.javasrc/main/java/dev/ayagmar/quarkusforge/api/MetadataSelection.javasrc/main/java/dev/ayagmar/quarkusforge/api/QuarkusApiClient.javasrc/main/java/dev/ayagmar/quarkusforge/application/DefaultStartupStateService.javasrc/main/java/dev/ayagmar/quarkusforge/archive/ProjectArchiveService.javasrc/main/java/dev/ayagmar/quarkusforge/archive/SafeZipExtractor.javasrc/test/java/dev/ayagmar/quarkusforge/api/CatalogDataServiceTest.javasrc/test/java/dev/ayagmar/quarkusforge/api/QuarkusApiClientTest.javasrc/test/java/dev/ayagmar/quarkusforge/application/DefaultStartupStateServiceTest.javasrc/test/java/dev/ayagmar/quarkusforge/archive/ProjectArchiveServiceTest.java
Comment on lines
+45
to
+53
| public ExtractionResult extract( | ||
| Path zipFile, | ||
| Path outputDirectory, | ||
| OverwritePolicy overwritePolicy, | ||
| BooleanSupplier cancelled) { | ||
| Objects.requireNonNull(zipFile); | ||
| Objects.requireNonNull(outputDirectory); | ||
| Objects.requireNonNull(overwritePolicy); | ||
| Objects.requireNonNull(cancelled); |
There was a problem hiding this comment.
Fail fast when cancelled is already set.
ProjectArchiveService.java, Lines 111-114 can call this overload after the EXTRACTING_ARCHIVE callback flips the flag, but the first actual check here isn't until Line 160. In that case you still parse the central directory and may create parent/staging directories before aborting.
💡 Minimal fix
Objects.requireNonNull(zipFile);
Objects.requireNonNull(outputDirectory);
Objects.requireNonNull(overwritePolicy);
Objects.requireNonNull(cancelled);
+ throwIfCancelled(cancelled);
Map<String, ZipEntryMetadata> metadataByEntry = ZipCentralDirectoryReader.read(zipFile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/dev/ayagmar/quarkusforge/archive/SafeZipExtractor.java` around
lines 45 - 53, The extract(...) method in SafeZipExtractor should fail fast when
the provided BooleanSupplier cancelled is already set: after the existing
Objects.requireNonNull checks (inside the extract method) call
cancelled.getAsBoolean() and immediately return an appropriate cancelled
ExtractionResult (e.g., ExtractionResult.CANCELLED or a new cancelled result)
instead of proceeding to parse the central directory or create directories;
update any code paths that assume work has started to use this early-return so
callers like ProjectArchiveService don't trigger work after EXTRACTING_ARCHIVE
flips the flag.
src/test/java/dev/ayagmar/quarkusforge/api/CatalogDataServiceTest.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/dev/ayagmar/quarkusforge/ui/ExtensionCatalogLoadResult.java (1)
70-74: Consider: Minor duplication of label logic with CatalogData.The
metadataSourceLabel()implementation is identical toCatalogData.metadataSourceLabel(). If this pattern spreads further, consider extracting to a shared utility. For now, the duplication is acceptable given the clear layer boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ayagmar/quarkusforge/ui/ExtensionCatalogLoadResult.java` around lines 70 - 74, The metadataSourceLabel logic in ExtensionCatalogLoadResult.metadataSourceLabel() duplicates CatalogData.metadataSourceLabel(); to eliminate duplication, extract a shared static helper like MetadataLabelUtil.metadataSourceLabel(MetadataSource source, boolean stale) that returns source.label() + " [stale]" when source==MetadataSource.CACHE && stale, otherwise source.label(), then update ExtensionCatalogLoadResult.metadataSourceLabel() and CatalogData.metadataSourceLabel() to delegate to MetadataLabelUtil.metadataSourceLabel(metadataSource, stale).src/main/java/dev/ayagmar/quarkusforge/headless/HeadlessOutputPrinter.java (1)
55-65: NormalizemetadataDetailbefore printing for consistent one-line output.At Line 64, detail is emitted raw after a blank-check. Normalizing once (same as
metadataSource) keeps output stable if callers pass padded/newline text.Diff suggestion
- String effectiveMetadataSourceLabel = - metadataSourceLabel == null ? "" : metadataSourceLabel.strip(); - boolean hasMetadataDetail = metadataDetail != null && !metadataDetail.isBlank(); + String effectiveMetadataSourceLabel = + metadataSourceLabel == null ? "" : metadataSourceLabel.strip(); + String effectiveMetadataDetail = metadataDetail == null ? "" : metadataDetail.strip(); + boolean hasMetadataDetail = !effectiveMetadataDetail.isBlank(); @@ - System.out.println(" - metadataDetail: " + metadataDetail); + System.out.println(" - metadataDetail: " + effectiveMetadataDetail);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ayagmar/quarkusforge/headless/HeadlessOutputPrinter.java` around lines 55 - 65, In HeadlessOutputPrinter, normalize metadataDetail before checking and printing: create a normalized metadataDetail value (e.g., metadataDetail == null ? "" : metadataDetail.strip()), use that normalized value for the hasMetadataDetail check and for the printed " - metadataDetail: ..." line so the output is consistent and one-line even when callers pass padded/newline text; keep existing logic that compares effectiveMetadataSourceLabel to effectiveCatalogSourceLabel unchanged.src/test/java/dev/ayagmar/quarkusforge/headless/HeadlessOutputPrinterTest.java (1)
121-126: Tighten the redundant-source test withmetadataDetailomission assertion.At Line 125, also assert
metadataDetailis absent to fully cover the suppression branch when both metadata fields should be omitted.Diff suggestion
- assertThat(output).contains("catalogSource: live").doesNotContain("metadataSource"); + assertThat(output) + .contains("catalogSource: live") + .doesNotContain("metadataSource") + .doesNotContain("metadataDetail");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/ayagmar/quarkusforge/headless/HeadlessOutputPrinterTest.java` around lines 121 - 126, The test should also assert that "metadataDetail" is not present to fully cover the suppression branch: after calling HeadlessOutputPrinter.printDryRunSummary(request, List.of("io.quarkus:quarkus-rest"), "live", "live", ""); and computing String output = stdout.toString(StandardCharsets.UTF_8); add an additional assertion on output (using assertThat(output)) to verify it doesNotContain("metadataDetail") alongside the existing doesNotContain("metadataSource") check so the test ensures both metadata fields are omitted.
🤖 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/java/dev/ayagmar/quarkusforge/headless/HeadlessOutputPrinter.java`:
- Around line 55-65: In HeadlessOutputPrinter, normalize metadataDetail before
checking and printing: create a normalized metadataDetail value (e.g.,
metadataDetail == null ? "" : metadataDetail.strip()), use that normalized value
for the hasMetadataDetail check and for the printed " - metadataDetail: ..."
line so the output is consistent and one-line even when callers pass
padded/newline text; keep existing logic that compares
effectiveMetadataSourceLabel to effectiveCatalogSourceLabel unchanged.
In `@src/main/java/dev/ayagmar/quarkusforge/ui/ExtensionCatalogLoadResult.java`:
- Around line 70-74: The metadataSourceLabel logic in
ExtensionCatalogLoadResult.metadataSourceLabel() duplicates
CatalogData.metadataSourceLabel(); to eliminate duplication, extract a shared
static helper like MetadataLabelUtil.metadataSourceLabel(MetadataSource source,
boolean stale) that returns source.label() + " [stale]" when
source==MetadataSource.CACHE && stale, otherwise source.label(), then update
ExtensionCatalogLoadResult.metadataSourceLabel() and
CatalogData.metadataSourceLabel() to delegate to
MetadataLabelUtil.metadataSourceLabel(metadataSource, stale).
In
`@src/test/java/dev/ayagmar/quarkusforge/headless/HeadlessOutputPrinterTest.java`:
- Around line 121-126: The test should also assert that "metadataDetail" is not
present to fully cover the suppression branch: after calling
HeadlessOutputPrinter.printDryRunSummary(request,
List.of("io.quarkus:quarkus-rest"), "live", "live", ""); and computing String
output = stdout.toString(StandardCharsets.UTF_8); add an additional assertion on
output (using assertThat(output)) to verify it doesNotContain("metadataDetail")
alongside the existing doesNotContain("metadataSource") check so the test
ensures both metadata fields are omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48f180f6-eb70-419d-8ba8-60a191796325
📒 Files selected for processing (17)
src/main/java/dev/ayagmar/quarkusforge/api/CatalogData.javasrc/main/java/dev/ayagmar/quarkusforge/api/CatalogDataService.javasrc/main/java/dev/ayagmar/quarkusforge/api/MetadataSource.javasrc/main/java/dev/ayagmar/quarkusforge/application/DefaultStartupStateService.javasrc/main/java/dev/ayagmar/quarkusforge/archive/SafeZipExtractor.javasrc/main/java/dev/ayagmar/quarkusforge/headless/HeadlessGenerationService.javasrc/main/java/dev/ayagmar/quarkusforge/headless/HeadlessOutputPrinter.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/CatalogLoadDiagnostics.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/TuiBootstrapService.javasrc/main/java/dev/ayagmar/quarkusforge/ui/ExtensionCatalogLoadResult.javasrc/test/java/dev/ayagmar/quarkusforge/api/CatalogDataServiceTest.javasrc/test/java/dev/ayagmar/quarkusforge/api/CatalogDataTest.javasrc/test/java/dev/ayagmar/quarkusforge/archive/SafeZipExtractorTest.javasrc/test/java/dev/ayagmar/quarkusforge/headless/HeadlessCatalogClientTest.javasrc/test/java/dev/ayagmar/quarkusforge/headless/HeadlessOutputPrinterTest.javasrc/test/java/dev/ayagmar/quarkusforge/runtime/CatalogLoadDiagnosticsTest.javasrc/test/java/dev/ayagmar/quarkusforge/ui/ExtensionCatalogLoadResultTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/dev/ayagmar/quarkusforge/api/MetadataSource.java
- src/test/java/dev/ayagmar/quarkusforge/headless/HeadlessCatalogClientTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/dev/ayagmar/quarkusforge/archive/SafeZipExtractor.java
- src/main/java/dev/ayagmar/quarkusforge/api/CatalogDataService.java
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 by CodeRabbit
New Features
Improvements
Tests