Health report, report diff fixes#1254
Merged
Merged
Conversation
…f comparison logic
There was a problem hiding this comment.
Pull request overview
This PR updates the Health Report and Report Diff CLI tooling in CLARIN-DSpace to improve comparison correctness when reports contain different check selections and to align CLI option handling/help output.
Changes:
- Remove the legacy
healthcheckCLI command fromlauncher.xmland rely on the Spring script-service basedhealth-report. - Update
report-diffto normalize report JSON to the intersection of check names, add a “Skipped Checks” section, and make key-field mappings independent of check ordering via selector-based paths. - Standardize help flags to
-h/--helpand update tests/fixtures accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
dspace/config/launcher.xml |
Removes legacy healthcheck command entry. |
dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java |
Updates help flag usage and adds coverage for skipped checks / name-based comparisons. |
dspace-api/src/main/resources/report-diff-fields.json |
Switches from index-based paths to selector-by-check-name paths. |
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java |
Changes -i to -h and clarifies -c semantics for filtering comparisons. |
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java |
Implements intersection normalization, skipped-check reporting, and selector-based field resolution. |
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java |
Changes help flag to -h, supports multiple -c values, and renames output option to -r. |
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java |
Implements multi-check -c parsing, help renaming, and output option rename. |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java:385
displayReportDates()also assumes the newest report is at the end offindAll()(reverses index viaallReports.size() - 1 - i). SincefindAll()is unordered, the listing and max-entry truncation can be inconsistent. Prefer querying reports already ordered bylastModified/idDESC and applying the limit at the query level.
List<ReportResult> allReports = reportResultService.findAll(context);
// Determine how many reports to process, respecting maxEntries if it's within valid range
long limitCount = (maxEntries > 0 && maxEntries < allReports.size()) ? maxEntries : allReports.size();
Map<String, List<DateWithArgs>> reportDatesMap = new HashMap<>();
for (long i = 0; i < limitCount; i++) {
// the newest report is at the end of the list, so we reverse the index
ReportResult report = allReports.get(allReports.size() - 1 - (int) i);
String formattedDate = FORMATTER.format(report.getLastModified()
dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java:272
- This test picks the "latest"
ReportResultviafindAll(context).get(size - 1), butReportResultDAOinheritsAbstractHibernateDAO.findAll()which has no ORDER BY. This can make the test flaky if the DB returns rows in a different order or other reports exist. Sort bylastModified/idexplicitly (or add a dedicated query for the most recent report) before asserting onlatest.getArgs().
context.reloadEntity(eperson);
List<ReportResult> allReports = reportResultService.findAll(context);
ReportResult latest = allReports.get(allReports.size() - 1);
assertThat(handler.getErrorMessages(), empty());
assertThat(latest.getArgs(), containsString("-c: 2"));
assertThat(latest.getArgs(), containsString("-c: 3"));
Collaborator
|
Regarding incompatible reports, comparing the report-diff, I got the following results (example):
I think, all the information started from Section 3: is not needed to show. The report-diff, for incompatible reports, may be shorter: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR modernizes DSpace health reporting and comparison scripts: HealthReport supports repeatable ChangesHealthReport CLI and Report Persistence
ReportDiff CLI Modernization and Comparison Logic
ReportResult Service API Cleanup
Configuration and Integration Updates
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (1)
342-386:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLine length exceeds 120 characters in multiple places.
Lines 369-370 and 377-378 exceed the 120-character maximum. Consider breaking these into separate lines or extracting the message strings.
📏 Suggested reformat for lines 369-370
if (Objects.isNull(sourceReportId)) { - handler.logInfo("Only '-t' was specified; '-s' will be set to the latest report from the " - + "database."); + handler.logInfo("Only '-t' was specified; '-s' will be set to the latest report " + + "from the database.");As per coding guidelines, maintain maximum 120 character line length in Java code.
🤖 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 `@dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java` around lines 342 - 386, In defaultReportIds, several handler.logInfo calls (the messages for "Only '-t' was specified; '-s' will be set..." and "Only '-s' was specified; '-t' will be set...") exceed the 120-char limit; shorten them by splitting the long literal into two concatenated string literals or assign the message to a local String variable and break it across lines (or use String.format) before passing it to handler.logInfo so each source line stays under 120 characters while keeping the same message semantics and using the existing symbols sourceReportId, targetReportId, handler.logInfo, and defaultReportIds.Source: Coding guidelines
♻️ Duplicate comments (2)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (1)
519-526:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate Javadoc to reflect ID-based comparison.
The Javadoc at line 520 still refers to "specified
fromandtodates," but the method now usessourceReportIdandtargetReportId(IDs, not dates). Update the description to match the current implementation.📝 Suggested Javadoc update
/** - * Compare two reports based on the specified `from` and `to` dates. + * Compare two reports based on the specified source and target report IDs. * If the reports are not found, log an appropriate message. * If the reports are found, generate a comparison report showing the differences.🤖 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 `@dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java` around lines 519 - 526, Update the Javadoc on the compareReports method in class ReportDiff to reflect that comparison is performed using report IDs rather than date ranges: change the description to say it compares two reports identified by sourceReportId and targetReportId, explains behavior when reports are missing and that comparison uses the intersection of check names present in both reports, and update the `@param` tags to describe sourceReportId and targetReportId (IDs of the reports) and keep the existing `@param` context description unchanged.dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java (1)
42-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLine length exceeds 120 characters.
Lines 43-44 exceed the 120-character maximum specified in the coding guidelines. Consider breaking the description string into multiple concatenated parts or reformatting the builder chain.
📏 Suggested reformat
- Option checkOption = Option.builder("c").longOpt("check").hasArgs() - .desc(String.format("Filter comparison to one or more specific checks by index (0 to %d). " + - "Repeat the flag (e.g. -c 1 -c 3) to compare multiple checks from both reports.", - HealthReport.getNumberOfChecks() - 1)) - .type(String.class) - .build(); + Option checkOption = Option.builder("c") + .longOpt("check") + .hasArgs() + .desc(String.format( + "Filter comparison to one or more specific checks by index (0 to %d). " + + "Repeat the flag (e.g. -c 1 -c 3) to compare multiple checks.", + HealthReport.getNumberOfChecks() - 1)) + .type(String.class) + .build();As per coding guidelines, maintain maximum 120 character line length in Java code.
🤖 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 `@dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java` around lines 42 - 46, The description string for the Option checkOption (created via Option.builder("c").longOpt("check").hasArgs()) exceeds 120 characters; split the desc(...) argument into multiple concatenated string literals or separate method call segments so no source line is longer than 120 chars (e.g., break after a sentence or before the String.format call parameters), keeping the same call chain and still using HealthReport.getNumberOfChecks() - 1 to compute the upper index.Source: Coding guidelines
🧹 Nitpick comments (5)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java (3)
156-248: ⚡ Quick winMissing Javadoc for public method.
The
internalRun()method is public (inherited as public from DSpaceRunnable) but lacks Javadoc documentation. As per coding guidelines, all public methods in Java files must have Javadoc comments.📝 Add Javadoc
+ /** + * Execute the health report generation. + * Runs selected checks, persists results, writes output file (if requested), and sends email (if requested). + * `@throws` Exception if an error occurs during report generation + */ `@Override` public void internalRun() throws Exception {🤖 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 `@dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java` around lines 156 - 248, Add a Javadoc comment for the public method internalRun() in class HealthReport: document purpose, thrown exceptions, important parameters/state (e.g., use of Context, reportFile, emails) and side-effects (persists ReportResult, writes file, sends email), and include `@throws` Exception to match the signature; place the comment immediately above the internalRun() method declaration so it satisfies the project's public-method Javadoc requirement.Source: Coding guidelines
250-265: ⚡ Quick winMissing Javadoc for public method.
The
printHelp()method is public (inherited as public from DSpaceRunnable) but lacks Javadoc documentation. As per coding guidelines, all public methods in Java files must have Javadoc comments.📝 Add Javadoc
+ /** + * Print help information for the health report script. + * Displays available options, defaults, and the list of available checks. + */ `@Override` public void printHelp() {🤖 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 `@dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java` around lines 250 - 265, Add a Javadoc comment for the public method printHelp() in class HealthReport: describe that it prints help/usage information for the health report CLI, mention it overrides DSpaceRunnable.printHelp(), note that it reads the configured "healthcheck.last_n_days" and lists available checks, and include `@Override` and an `@since` or `@see` tag if project conventions require it; place the Javadoc immediately above the public void printHelp() declaration so the method is properly documented.Source: Coding guidelines
271-292: ⚡ Quick winMissing Javadoc for private helper method.
While the coding guideline focuses on public methods, adding Javadoc to
printCommandlineOptions()would improve maintainability, especially given its role in persisting report arguments.📝 Add Javadoc
+ /** + * Format command line options as a readable string. + * Deduplicates repeated option keys and includes all values for multi-value options. + * This method is used to record the options used when generating the report. + * `@return` formatted string of command line options + */ private String printCommandlineOptions() {🤖 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 `@dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java` around lines 271 - 292, Add a Javadoc comment to the private helper method printCommandlineOptions() describing its purpose (formatting and returning the currently parsed command-line options for inclusion in the health report), its behavior (iterates over CommandLine options, handles multi-valued options and de-duplicates by option key), the fact that it has no parameters, and what it returns (a formatted String of options); attach this Javadoc directly above the printCommandlineOptions() method signature to improve maintainability and clarity.dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java (1)
33-58: ⚡ Quick winMissing Javadoc for public method.
The
getOptions()method is public but lacks Javadoc documentation. As per coding guidelines, all public methods in Java files must have Javadoc comments.📝 Add Javadoc
+ /** + * Get the CLI options for the health report script. + * `@return` Options configured options for the script + */ `@Override` public Options getOptions() {🤖 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 `@dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java` around lines 33 - 58, Add a Javadoc block above the public method getOptions() in class HealthReportScriptConfiguration describing its purpose (builds and returns CLI Options for the health report script), the fact it caches the Options instance, the return type (Options), and any behavior notes (which flags are added, e.g. -h/--help, -e/--email, -c/--check, -f/--for, -r/--report); include `@return` describing the returned Options and `@since/`@author tags if your project requires them.Source: Coding guidelines
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (1)
317-322: 💤 Low valueDead code: XOR validation is unreachable after
defaultReportIds().The check at lines 318-322 validates that both
sourceReportIdandtargetReportIdare set (neither is null while the other isn't). However, this method is only called afterdefaultReportIds()has already executed (line 256), which fills any missing ID. AfterdefaultReportIds(), either both IDs are set or both remain null (if insufficient reports exist in the DB), so the XOR condition will never be true.Consider removing lines 318-322 or moving this validation earlier in the flow (before
defaultReportIds()) if you want to enforce that users must supply both IDs explicitly.♻️ Option 1: Remove dead XOR check
private boolean validateReportIdSelection() { - if ((Objects.isNull(sourceReportId) && Objects.nonNull(targetReportId)) - || (Objects.nonNull(sourceReportId) && Objects.isNull(targetReportId))) { - handler.logError("Both 'source' and 'target' report IDs must be specified."); - return false; - } - if (Objects.nonNull(sourceReportId) && sourceReportId <= 0) {🤖 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 `@dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java` around lines 317 - 322, The XOR null-check in validateReportIdSelection() (the conditional that logs via handler.logError when exactly one of sourceReportId/targetReportId is null) is dead after defaultReportIds() populates missing IDs; either remove that XOR branch from validateReportIdSelection() or call validateReportIdSelection() before defaultReportIds() so the XOR check can catch user-supplied-only cases. Update the call site around defaultReportIds()/validateReportIdSelection() accordingly and keep the handler.logError usage (or remove it if you choose deletion) to ensure behavior remains consistent.
🤖 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.
Outside diff comments:
In `@dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java`:
- Around line 342-386: In defaultReportIds, several handler.logInfo calls (the
messages for "Only '-t' was specified; '-s' will be set..." and "Only '-s' was
specified; '-t' will be set...") exceed the 120-char limit; shorten them by
splitting the long literal into two concatenated string literals or assign the
message to a local String variable and break it across lines (or use
String.format) before passing it to handler.logInfo so each source line stays
under 120 characters while keeping the same message semantics and using the
existing symbols sourceReportId, targetReportId, handler.logInfo, and
defaultReportIds.
---
Duplicate comments:
In `@dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java`:
- Around line 519-526: Update the Javadoc on the compareReports method in class
ReportDiff to reflect that comparison is performed using report IDs rather than
date ranges: change the description to say it compares two reports identified by
sourceReportId and targetReportId, explains behavior when reports are missing
and that comparison uses the intersection of check names present in both
reports, and update the `@param` tags to describe sourceReportId and
targetReportId (IDs of the reports) and keep the existing `@param` context
description unchanged.
In
`@dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java`:
- Around line 42-46: The description string for the Option checkOption (created
via Option.builder("c").longOpt("check").hasArgs()) exceeds 120 characters;
split the desc(...) argument into multiple concatenated string literals or
separate method call segments so no source line is longer than 120 chars (e.g.,
break after a sentence or before the String.format call parameters), keeping the
same call chain and still using HealthReport.getNumberOfChecks() - 1 to compute
the upper index.
---
Nitpick comments:
In `@dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java`:
- Around line 156-248: Add a Javadoc comment for the public method internalRun()
in class HealthReport: document purpose, thrown exceptions, important
parameters/state (e.g., use of Context, reportFile, emails) and side-effects
(persists ReportResult, writes file, sends email), and include `@throws` Exception
to match the signature; place the comment immediately above the internalRun()
method declaration so it satisfies the project's public-method Javadoc
requirement.
- Around line 250-265: Add a Javadoc comment for the public method printHelp()
in class HealthReport: describe that it prints help/usage information for the
health report CLI, mention it overrides DSpaceRunnable.printHelp(), note that it
reads the configured "healthcheck.last_n_days" and lists available checks, and
include `@Override` and an `@since` or `@see` tag if project conventions require it;
place the Javadoc immediately above the public void printHelp() declaration so
the method is properly documented.
- Around line 271-292: Add a Javadoc comment to the private helper method
printCommandlineOptions() describing its purpose (formatting and returning the
currently parsed command-line options for inclusion in the health report), its
behavior (iterates over CommandLine options, handles multi-valued options and
de-duplicates by option key), the fact that it has no parameters, and what it
returns (a formatted String of options); attach this Javadoc directly above the
printCommandlineOptions() method signature to improve maintainability and
clarity.
In
`@dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java`:
- Around line 33-58: Add a Javadoc block above the public method getOptions() in
class HealthReportScriptConfiguration describing its purpose (builds and returns
CLI Options for the health report script), the fact it caches the Options
instance, the return type (Options), and any behavior notes (which flags are
added, e.g. -h/--help, -e/--email, -c/--check, -f/--for, -r/--report); include
`@return` describing the returned Options and `@since/`@author tags if your project
requires them.
In `@dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java`:
- Around line 317-322: The XOR null-check in validateReportIdSelection() (the
conditional that logs via handler.logError when exactly one of
sourceReportId/targetReportId is null) is dead after defaultReportIds()
populates missing IDs; either remove that XOR branch from
validateReportIdSelection() or call validateReportIdSelection() before
defaultReportIds() so the XOR check can catch user-supplied-only cases. Update
the call site around defaultReportIds()/validateReportIdSelection() accordingly
and keep the handler.logError usage (or remove it if you choose deletion) to
ensure behavior remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34707414-4957-4bfc-8053-9a5cda1c39df
📒 Files selected for processing (13)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.javadspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.javadspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.javadspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.javadspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.javadspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.javadspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.javadspace-api/src/main/java/org/dspace/content/service/ReportResultService.javadspace-api/src/main/resources/report-diff-fields.jsondspace-api/src/test/java/org/dspace/scripts/HealthReportIT.javadspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.javadspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ScriptRestRepository.javadspace/config/launcher.xml
💤 Files with no reviewable changes (4)
- dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java
- dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java
- dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java
- dspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.java
milanmajchrak
requested changes
Jun 10, 2026
| try (Context context = new Context()) { | ||
| defaultDate(context); | ||
| // If at least one of -s/-t is missing, fill missing values from latest reports. | ||
| if (Objects.isNull(sourceReportId) || Objects.isNull(targetReportId)) { |
Collaborator
There was a problem hiding this comment.
Minor comment:
May be shorter version:
if (sourceReportId == null || tergetReportId == null) {
...
)
…ecks section, validate report IDs before defaulting Co-Authored-By: Claude Fable 5 <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
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.
Problem description
Followed issues:
https://github.com/dataquest-dev/dspace-customers/issues/430
#1250
Manual Testing (if applicable)
Copilot review
Summary by CodeRabbit
New Features
Improvements
Tests
Refactor
Chores