From 63c1c73ef85445f94f0728ee924cd7a919942c37 Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Mon, 23 Feb 2026 17:12:10 +0100 Subject: [PATCH 01/16] fix(health-report): fix CLI args, multi-check support, and report-diff comparison logic --- .../dspace/app/healthreport/HealthReport.java | 85 +++-- .../HealthReportScriptConfiguration.java | 14 +- .../org/dspace/app/reportdiff/ReportDiff.java | 303 ++++++++++++++++-- .../ReportDiffScriptConfiguration.java | 7 +- .../main/resources/report-diff-fields.json | 96 +++--- .../java/org/dspace/scripts/ReportDiffIT.java | 72 ++++- dspace/config/launcher.xml | 8 +- 7 files changed, 456 insertions(+), 129 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java index 36bb15c1056f..d93a88b0b976 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java @@ -13,8 +13,10 @@ import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.text.SimpleDateFormat; +import java.util.ArrayList; import java.util.Date; import java.util.LinkedHashMap; +import java.util.List; import java.util.Locale; import java.util.Map; import javax.mail.MessagingException; @@ -59,9 +61,9 @@ public class HealthReport extends DSpaceRunnable checks = Report.checks(); /** - * `-i`: Info, show help information. + * `-h`: Help, show help information. */ - private boolean info = false; + private boolean help = false; /** * `-e`: Email, send report to specified email address. @@ -69,9 +71,10 @@ public class HealthReport extends DSpaceRunnable specificChecks = new ArrayList<>(); /** * `-f`: For, specify the last N days to consider. @@ -80,9 +83,9 @@ public class HealthReport extends DSpaceRunnable= getNumberOfChecks()) { - specificCheck = -1; + String[] checkOptions = commandLine.getOptionValues('c'); + for (String checkOption : checkOptions) { + try { + int checkIndex = Integer.parseInt(checkOption); + if (checkIndex < 0 || checkIndex >= getNumberOfChecks()) { + handler.logError("Invalid value for check: " + checkOption + + ". Must be an integer from 0 to " + (getNumberOfChecks() - 1) + "."); + throw new ParseException("Invalid check index: " + checkOption); + } + specificChecks.add(checkIndex); + } catch (NumberFormatException e) { + handler.logError("Invalid value for check: '" + checkOption + + "'. It has to be an integer number from 0 to " + (getNumberOfChecks() - 1) + "."); + throw new ParseException("Invalid check value: " + checkOption); } - } catch (NumberFormatException e) { - log.info("Invalid value for check. It has to be a number from the displayed range."); - return; } } - // `-f`: For, specify the last N days to consider. + // `-f`: For, specify the last N days to consider. Must be a positive integer. if (commandLine.hasOption('f')) { String daysOption = commandLine.getOptionValue('f'); try { forLastNDays = Integer.parseInt(daysOption); + if (forLastNDays <= 0) { + handler.logError("Invalid value for -f: " + daysOption + + ". Must be a positive integer (greater than 0)."); + throw new ParseException("Invalid -f value: " + daysOption); + } } catch (NumberFormatException e) { - log.info("Invalid value for last N days. Argument f has to be a number."); - return; + handler.logError("Invalid value for -f: '" + daysOption + + "'. Must be a positive integer."); + throw new ParseException("Invalid -f value: " + daysOption); } } - // `-o`: Output, specify a file to save the report. - if (commandLine.hasOption('o')) { - fileName = commandLine.getOptionValue('o'); + // `-r`: Report, specify a file to save the report. + if (commandLine.hasOption('r')) { + reportFile = commandLine.getOptionValue('r'); } } @Override public void internalRun() throws Exception { - if (info) { + if (help) { printHelp(); return; } @@ -157,7 +173,7 @@ public void internalRun() throws Exception { JSONArray checksArray = new JSONArray(); for (Map.Entry check_entry : Report.checks().entrySet()) { ++position; - if (specificCheck != -1 && specificCheck != position) { + if (!specificChecks.isEmpty() && !specificChecks.contains(position)) { continue; } @@ -198,9 +214,9 @@ public void internalRun() throws Exception { context.commit(); // save output to file - if (fileName != null) { + if (reportFile != null) { InputStream inputStream = toInputStream(sbReport.toString(), StandardCharsets.UTF_8); - handler.writeFilestream(context, fileName, inputStream, "export"); + handler.writeFilestream(context, reportFile, inputStream, "export"); context.restoreAuthSystemState(); @@ -226,14 +242,15 @@ public void internalRun() throws Exception { @Override public void printHelp() { - handler.logInfo("\n\nINFORMATION\nThis process creates a health report of your DSpace.\n" + + handler.logInfo("\n\nHELP\nThis process creates a health report of your DSpace.\n" + "You can choose from these available options:\n" + - " -i, --info Show help information\n" + + " -h, --help Show help information\n" + " -e, --email Send report to specified email address\n" + - " -c, --check Perform only specific check by index (0-" + (getNumberOfChecks() - 1) + ")\n" + - " -f, --for Specify the last N days to consider\n" + - " -o, --output Specify a file to save the report\n\n" + - "If you want to execute only one check using -c, use check index:\n" + checksNamesToString() + "\n" + " -c, --check Perform specific check(s) by index (0-" + (getNumberOfChecks() - 1) + + "). Can be used multiple times, e.g. -c 0 -c 3 -c 4\n" + + " -f, --for Specify the last N days to consider (positive integer)\n" + + " -r, --report Specify a file to save the report\n\n" + + "Available checks:\n" + checksNamesToString() + "\n" ); } diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java index 771cc70aadb9..bd8245f34363 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java @@ -32,20 +32,22 @@ public void setDspaceRunnableClass(Class dspaceRunnableClass) { public Options getOptions() { if (options == null) { Options options = new Options(); - options.addOption("i", "info", false, + options.addOption("h", "help", false, "Show help information."); options.addOption("e", "email", true, "Send report to this email address."); options.getOption("e").setType(String.class); options.addOption("c", "check", true, - String.format("Perform only specific check (use index from 0 to %d, " + - "otherwise perform default checks).", HealthReport.getNumberOfChecks() - 1)); + String.format("Perform specific check(s) by index (0 to %d). " + + "Can be used multiple times, e.g. -c 0 -c 3 -c 4.", + HealthReport.getNumberOfChecks() - 1)); options.getOption("c").setType(String.class); + options.getOption("c").setArgs(Integer.MAX_VALUE); options.addOption("f", "for", true, - "Report for last N days. Used only in general information for now."); + "Report for last N days (positive integer). Used only in general information for now."); options.getOption("f").setType(String.class); - options.addOption("o", "output", true, - "Save report to the file."); + options.addOption("r", "report", true, + "Specify the report file to store the output."); super.options = options; } diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java index eaaf7bee6035..215fed799f80 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java @@ -61,9 +61,9 @@ public class ReportDiff extends DSpaceRunnable { private EPersonService ePersonService; /** - * `-i`: Info, show help information. + * `-h`: Help, show help information. */ - private boolean info = false; + private boolean help = false; /** * `-d`: Dates, show all dates that the report was generated for a specific check type. @@ -158,9 +158,9 @@ public ReportDiffScriptConfiguration getScriptConfiguration() { public void setup() throws ParseException { ePersonService = EPersonServiceFactory.getInstance().getEPersonService(); reportResultService = ContentServiceFactory.getInstance().getReportResultService(); - // `-i`: Info, show help information. - if (commandLine.hasOption('i')) { - info = true; + // `-h`: Help, show help information. + if (commandLine.hasOption('h')) { + help = true; return; } @@ -201,7 +201,7 @@ public void setup() throws ParseException { @Override public void internalRun() throws Exception { // If the user requested help information, we will display it. - if (info) { + if (help) { printHelp(); return; } @@ -364,6 +364,7 @@ private void displayReportDates() { * Compare two reports based on the specified `from` and `to` dates. * If the reports are not found, log an appropriate message. * If the reports are found, generate a comparison report showing the differences. + * The comparison is based on the intersection of check names present in both reports. * * @param context the application context */ @@ -371,13 +372,8 @@ private void compareReports(Context context) { try { context.setCurrentUser(ePersonService.find(context, getEpersonIdentifier())); - ReportResult fromReport = specificCheck != -1 - ? reportResultService.findByLastModifiedAndCheckType(context, from, specificCheck) - : reportResultService.findByLastModified(context, from); - - ReportResult toReport = specificCheck != -1 - ? reportResultService.findByLastModifiedAndCheckType(context, to, specificCheck) - : reportResultService.findByLastModified(context, to); + ReportResult fromReport = reportResultService.findByLastModified(context, from); + ReportResult toReport = reportResultService.findByLastModified(context, to); if (fromReport == null || toReport == null) { handler.logInfo("No reports found for specified dates."); @@ -406,9 +402,121 @@ private void compareReports(Context context) { } } + /** + * Holds the result of normalizing two reports to their intersection, + * including information about checks that were skipped (present in one report only). + */ + private static class NormalizationResult { + final String normalizedFromJson; + final String normalizedToJson; + /** Check names that exist only in the "from" report. */ + final List onlyInFrom; + /** Check names that exist only in the "to" report. */ + final List onlyInTo; + + NormalizationResult(String normalizedFromJson, String normalizedToJson, + List onlyInFrom, List onlyInTo) { + this.normalizedFromJson = normalizedFromJson; + this.normalizedToJson = normalizedToJson; + this.onlyInFrom = onlyInFrom; + this.onlyInTo = onlyInTo; + } + } + + /** + * Normalize two report JSON strings so that they only contain checks + * that are present (by name) in both reports. This allows correct comparison + * when reports were created with different check selections. + * + * If the `-c` option was specified, additionally filters to only include + * checks matching the specified check index (by name from the configured check list). + * + * @param fromJson the JSON string of the "from" report + * @param toJson the JSON string of the "to" report + * @return a {@link NormalizationResult} containing normalized JSON and skipped check info + * @throws IOException if JSON parsing fails + */ + private NormalizationResult normalizeReportsToIntersection(String fromJson, String toJson) throws IOException { + JsonNode fromRoot = mapper.readTree(fromJson); + JsonNode toRoot = mapper.readTree(toJson); + + JsonNode fromChecks = fromRoot.get("checks"); + JsonNode toChecks = toRoot.get("checks"); + + if (fromChecks == null || toChecks == null || !fromChecks.isArray() || !toChecks.isArray()) { + return new NormalizationResult(fromJson, toJson, + new ArrayList<>(), new ArrayList<>()); + } + + // Build maps of check name -> check node for both reports + Map fromCheckMap = new LinkedHashMap<>(); + for (JsonNode check : fromChecks) { + JsonNode nameNode = check.get("name"); + if (nameNode != null) { + fromCheckMap.put(nameNode.asText(), check); + } + } + + Map toCheckMap = new LinkedHashMap<>(); + for (JsonNode check : toChecks) { + JsonNode nameNode = check.get("name"); + if (nameNode != null) { + toCheckMap.put(nameNode.asText(), check); + } + } + + // Compute intersection of check names + List commonNames = new ArrayList<>(fromCheckMap.keySet()); + commonNames.retainAll(toCheckMap.keySet()); + + // If specificCheck is set, further filter to only that check name + if (specificCheck != -1) { + String targetCheckName = HealthReport.getCheckName(specificCheck); + if (targetCheckName != null) { + commonNames.retainAll(java.util.Collections.singletonList(targetCheckName)); + } + } + + if (commonNames.isEmpty()) { + handler.logInfo("No common checks found between the two reports for comparison."); + } + + // Determine checks that are only in one report + List onlyInFrom = new ArrayList<>(fromCheckMap.keySet()); + onlyInFrom.removeAll(toCheckMap.keySet()); + List onlyInTo = new ArrayList<>(toCheckMap.keySet()); + onlyInTo.removeAll(fromCheckMap.keySet()); + + // Build normalized JSON with only the common checks (in the same order) + com.fasterxml.jackson.databind.node.ObjectNode normalizedFrom = + mapper.createObjectNode(); + com.fasterxml.jackson.databind.node.ArrayNode normalizedFromChecks = + mapper.createArrayNode(); + for (String name : commonNames) { + normalizedFromChecks.add(fromCheckMap.get(name)); + } + normalizedFrom.set("checks", normalizedFromChecks); + + com.fasterxml.jackson.databind.node.ObjectNode normalizedTo = + mapper.createObjectNode(); + com.fasterxml.jackson.databind.node.ArrayNode normalizedToChecks = + mapper.createArrayNode(); + for (String name : commonNames) { + normalizedToChecks.add(toCheckMap.get(name)); + } + normalizedTo.set("checks", normalizedToChecks); + + return new NormalizationResult( + mapper.writeValueAsString(normalizedFrom), + mapper.writeValueAsString(normalizedTo), + onlyInFrom, onlyInTo); + } + /** * Generate a comparison report between two ReportResult objects. * The report includes the type, last modified dates, and the differences in JSON format. + * When comparing reports with different check selections, only the intersection + * of common check names is compared. * * @param fromReport the "from" report * @param toReport the "to" report @@ -423,6 +531,11 @@ private String generateReportComparison(ReportResult fromReport, ReportResult to return "One of the reports has no value. Cannot compare."; } + // Normalize both reports to contain only intersection of check names + NormalizationResult normalized = normalizeReportsToIntersection(fromJson, toJson); + String normalizedFromJson = normalized.normalizedFromJson; + String normalizedToJson = normalized.normalizedToJson; + StringBuilder sb = new StringBuilder(); // Header @@ -444,17 +557,37 @@ private String generateReportComparison(ReportResult fromReport, ReportResult to sb.append("Report Period: ").append(timePeriod).append("\n\n"); // Enhanced Key Changes Table - String keyChangesTable = generateEnhancedKeyChangesTable(fromJson, toJson, + String keyChangesTable = generateEnhancedKeyChangesTable(normalizedFromJson, normalizedToJson, fromReport.getLastModified(), toReport.getLastModified()); sb.append(keyChangesTable); // Detailed Change Log sb.append("Section 2: Detailed Change Log\n\n"); sb.append("Changes Summary\n"); - String detailedSummary = generateDetailedSummary(fromJson, toJson); + String detailedSummary = generateDetailedSummary(normalizedFromJson, normalizedToJson); sb.append(detailedSummary).append("\n"); - sb.append(generateDiff(fromJson, toJson)); + sb.append(generateDiff(normalizedFromJson, normalizedToJson)); + + // Section 3: Skipped Checks (not present in both reports) + if (!normalized.onlyInFrom.isEmpty() || !normalized.onlyInTo.isEmpty()) { + sb.append("\nSection 3: Skipped Checks\n\n"); + sb.append("The following checks could not be compared because they were not present in both reports.\n\n"); + if (!normalized.onlyInFrom.isEmpty()) { + sb.append("Only in 'From' report (").append(fromReport.getLastModified()).append("):\n"); + for (String name : normalized.onlyInFrom) { + sb.append(" - ").append(name).append("\n"); + } + sb.append("\n"); + } + if (!normalized.onlyInTo.isEmpty()) { + sb.append("Only in 'To' report (").append(toReport.getLastModified()).append("):\n"); + for (String name : normalized.onlyInTo) { + sb.append(" - ").append(name).append("\n"); + } + sb.append("\n"); + } + } return sb.toString(); } @@ -589,8 +722,135 @@ private String formatBytes(long bytes) { return (bytes / (1024 * 1024 * 1024)) + " GB"; } + /** + * Resolve a field path with attribute selectors to a JSON value. + *

+ * Supports XPath-like selector syntax for matching array elements by a named field: + * 

+     *   /checks/[name=General Information]/report/publishedItems
+     *
+ * The segment {@code [name=General Information]} means: find the element in the {@code checks} + * array whose {@code "name"} field equals {@code "General Information"}. + *

+ * Regular path segments (e.g. {@code /report/collectionsSizesInfo/totalSize}) are resolved + * as standard JSON object field traversal. Numeric segments (e.g. {@code /0}) are resolved + * as array indices. + * + * @param rootNode the root JSON node to resolve against + * @param fieldPath the selector path, e.g. + * {@code /checks/[name=Item summary]/report/communitiesCount} + * @return the resolved {@link JsonNode}, or {@code null} if not found + */ + private JsonNode resolveFieldPath(JsonNode rootNode, String fieldPath) { + if (fieldPath == null || rootNode == null) { + return null; + } + + // Remove leading slash and split into segments + String path = fieldPath.startsWith("/") ? fieldPath.substring(1) : fieldPath; + // Split carefully: we need to handle segments like [name=General Information] + // which contain spaces but no slashes + List segments = splitPathSegments(path); + + JsonNode current = rootNode; + for (String segment : segments) { + if (current == null) { + return null; + } + + if (segment.startsWith("[") && segment.endsWith("]")) { + // Attribute selector, e.g. [name=General Information] + // The previous segment should have navigated us to an array node + if (!current.isArray()) { + return null; + } + String selectorContent = segment.substring(1, segment.length() - 1); + int eqIndex = selectorContent.indexOf('='); + if (eqIndex < 0) { + return null; + } + String attrName = selectorContent.substring(0, eqIndex).trim(); + String attrValue = selectorContent.substring(eqIndex + 1).trim(); + + // Find matching element in the array + JsonNode matched = null; + for (JsonNode element : current) { + JsonNode attrNode = element.get(attrName); + if (attrNode != null && attrValue.equals(attrNode.asText())) { + matched = element; + break; + } + } + current = matched; + } else if (current.isArray() && segment.matches("\\d+")) { + // Numeric index into array + int index = Integer.parseInt(segment); + current = (index >= 0 && index < current.size()) ? current.get(index) : null; + } else { + // Regular object field + current = current.get(segment); + } + } + + return current; + } + + /** + * Split a path string into segments, keeping bracket selectors as single segments. + * For example, {@code "checks/[name=General Information]/report/directoryStats/0/size_bytes"} + * becomes: {@code ["checks", "[name=General Information]", "report", "directoryStats", "0", "size_bytes"]}. + * + * @param path the path to split (without leading slash) + * @return list of path segments + */ + private List splitPathSegments(String path) { + List segments = new ArrayList<>(); + int i = 0; + while (i < path.length()) { + if (path.charAt(i) == '[') { + // Find matching closing bracket + int closeBracket = path.indexOf(']', i); + if (closeBracket < 0) { + closeBracket = path.length() - 1; + } + segments.add(path.substring(i, closeBracket + 1)); + i = closeBracket + 1; + // Skip following slash if present + if (i < path.length() && path.charAt(i) == '/') { + i++; + } + } else { + // Regular segment - find next slash or bracket + int nextSlash = path.indexOf('/', i); + int nextBracket = path.indexOf('[', i); + int end; + if (nextSlash < 0 && nextBracket < 0) { + end = path.length(); + } else if (nextSlash < 0) { + end = nextBracket; + } else if (nextBracket < 0) { + end = nextSlash; + } else { + end = Math.min(nextSlash, nextBracket); + } + String segment = path.substring(i, end); + if (!segment.isEmpty()) { + segments.add(segment); + } + i = end; + // Skip slash separator + if (i < path.length() && path.charAt(i) == '/') { + i++; + } + } + } + return segments; + } + /** * Generate enhanced key changes table with dynamic sizing and configurable field names. + * Uses selector-based field resolution that works regardless of check ordering or selection. + * Field paths use XPath-like syntax, e.g. {@code /checks/[name=Item summary]/report/publishedItems}. * * @param oldJson the old JSON report * @param newJson the new JSON report @@ -610,8 +870,15 @@ private String generateEnhancedKeyChangesTable(String oldJson, String newJson, List changes = new ArrayList<>(); for (String fieldPath : fieldOrder) { - JsonNode oldValue = getValueFromPath(oldNode, fieldPath); - JsonNode newValue = getValueFromPath(newNode, fieldPath); + JsonNode oldValue = resolveFieldPath(oldNode, fieldPath); + JsonNode newValue = resolveFieldPath(newNode, fieldPath); + + // Skip fields that don't exist in either report (check not present in both) + boolean oldMissing = oldValue == null || oldValue.isMissingNode(); + boolean newMissing = newValue == null || newValue.isMissingNode(); + if (oldMissing && newMissing) { + continue; + } if (!Objects.equals(getDisplayValue(oldValue), getDisplayValue(newValue))) { String displayName = fieldMappings.getOrDefault(fieldPath, fieldPath); diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java index da1933590bb2..4c89469d7b16 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java @@ -33,14 +33,15 @@ public void setDspaceRunnableClass(Class dspaceRunnableClass) { public Options getOptions() { if (options == null) { Options options = new Options(); - options.addOption("i", "info", false, + options.addOption("h", "help", false, "Show help information."); options.addOption("e", "email", true, "Send report to this email address."); options.getOption("e").setType(String.class); options.addOption("c", "check", true, - String.format("Perform only specific check (use index from 0 to %d, " + - "otherwise perform default checks).", HealthReport.getNumberOfChecks() - 1)); + String.format("Filter comparison to a specific check by index (0 to %d). " + + "Only the specified check will be compared from both reports.", + HealthReport.getNumberOfChecks() - 1)); options.getOption("c").setType(String.class); options.addOption("d", "dates", false, "Show all report dates"); diff --git a/dspace-api/src/main/resources/report-diff-fields.json b/dspace-api/src/main/resources/report-diff-fields.json index a1e1f687aefc..51f668f79d96 100644 --- a/dspace-api/src/main/resources/report-diff-fields.json +++ b/dspace-api/src/main/resources/report-diff-fields.json @@ -1,54 +1,54 @@ { "fieldMappings": { - "/checks/0/report/directoryStats/0/size_bytes": "Assetstore Size (bytes)", - "/checks/0/report/directoryStats/1/size_bytes": "Log Directory Size (bytes)", - "/checks/1/report/communitiesCount": "Communities", - "/checks/1/report/collectionsCount": "Collections", - "/checks/1/report/collectionsSizesInfo/totalSize": "Total Content Size", - "/checks/1/report/itemsCount": "Items", - "/checks/1/report/publishedItems": "Published Items", - "/checks/1/report/notPublishedItems": "Unpublished Items", - "/checks/1/report/withdrawnItems": "Withdrawn Items", - "/checks/1/report/workspaceItemsCount": "Workspace Items", - "/checks/1/report/waitingForApproval": "Workflow Items", - "/checks/1/report/bitstreamsCount": "Bitstreams", - "/checks/1/report/bundlesCount": "Bundles", - "/checks/1/report/collectionsSizesInfo/orphanBitstreamsCount": "Orphaned Bitstreams", - "/checks/1/report/collectionsSizesInfo/deletedBitstreams": "Deleted Bitstreams", - "/checks/1/report/metadataValuesCount": "Metadata Values", - "/checks/1/report/handlesCount": "Handles", - "/checks/1/report/ePersonsCount": "Users", - "/checks/1/report/groupsCount": "Groups", - "/checks/2/report/selfRegistered": "Self Registered Users", - "/checks/2/report/subscribers": "Subscribers", - "/checks/2/report/subscribedCollections": "Subscribed Collections", - "/checks/2/report/emptyGroups": "Empty Groups", - "/checks/3/report/licenses": "Licenses" + "/checks/[name=General Information]/report/directoryStats/0/size_bytes": "Assetstore Size (bytes)", + "/checks/[name=General Information]/report/directoryStats/1/size_bytes": "Log Directory Size (bytes)", + "/checks/[name=Item summary]/report/communitiesCount": "Communities", + "/checks/[name=Item summary]/report/collectionsCount": "Collections", + "/checks/[name=Item summary]/report/collectionsSizesInfo/totalSize": "Total Content Size", + "/checks/[name=Item summary]/report/itemsCount": "Items", + "/checks/[name=Item summary]/report/publishedItems": "Published Items", + "/checks/[name=Item summary]/report/notPublishedItems": "Unpublished Items", + "/checks/[name=Item summary]/report/withdrawnItems": "Withdrawn Items", + "/checks/[name=Item summary]/report/workspaceItemsCount": "Workspace Items", + "/checks/[name=Item summary]/report/waitingForApproval": "Workflow Items", + "/checks/[name=Item summary]/report/bitstreamsCount": "Bitstreams", + "/checks/[name=Item summary]/report/bundlesCount": "Bundles", + "/checks/[name=Item summary]/report/collectionsSizesInfo/orphanBitstreamsCount": "Orphaned Bitstreams", + "/checks/[name=Item summary]/report/collectionsSizesInfo/deletedBitstreams": "Deleted Bitstreams", + "/checks/[name=Item summary]/report/metadataValuesCount": "Metadata Values", + "/checks/[name=Item summary]/report/handlesCount": "Handles", + "/checks/[name=Item summary]/report/ePersonsCount": "Users", + "/checks/[name=Item summary]/report/groupsCount": "Groups", + "/checks/[name=User summary]/report/selfRegistered": "Self Registered Users", + "/checks/[name=User summary]/report/subscribers": "Subscribers", + "/checks/[name=User summary]/report/subscribedCollections": "Subscribed Collections", + "/checks/[name=User summary]/report/emptyGroups": "Empty Groups", + "/checks/[name=License summary]/report/licenses": "Licenses" }, "fieldOrder": [ - "/checks/0/report/directoryStats/0/size_bytes", - "/checks/0/report/directoryStats/1/size_bytes", - "/checks/1/report/communitiesCount", - "/checks/1/report/collectionsCount", - "/checks/1/report/collectionsSizesInfo/totalSize", - "/checks/1/report/itemsCount", - "/checks/1/report/publishedItems", - "/checks/1/report/notPublishedItems", - "/checks/1/report/withdrawnItems", - "/checks/1/report/workspaceItemsCount", - "/checks/1/report/waitingForApproval", - "/checks/1/report/bitstreamsCount", - "/checks/1/report/bundlesCount", - "/checks/1/report/collectionsSizesInfo/orphanBitstreamsCount", - "/checks/1/report/collectionsSizesInfo/deletedBitstreams", - "/checks/1/report/metadataValuesCount", - "/checks/1/report/handlesCount", - "/checks/1/report/ePersonsCount", - "/checks/1/report/groupsCount", - "/checks/2/report/selfRegistered", - "/checks/2/report/subscribers", - "/checks/2/report/subscribedCollections", - "/checks/2/report/emptyGroups", - "/checks/3/report/licenses" + "/checks/[name=General Information]/report/directoryStats/0/size_bytes", + "/checks/[name=General Information]/report/directoryStats/1/size_bytes", + "/checks/[name=Item summary]/report/communitiesCount", + "/checks/[name=Item summary]/report/collectionsCount", + "/checks/[name=Item summary]/report/collectionsSizesInfo/totalSize", + "/checks/[name=Item summary]/report/itemsCount", + "/checks/[name=Item summary]/report/publishedItems", + "/checks/[name=Item summary]/report/notPublishedItems", + "/checks/[name=Item summary]/report/withdrawnItems", + "/checks/[name=Item summary]/report/workspaceItemsCount", + "/checks/[name=Item summary]/report/waitingForApproval", + "/checks/[name=Item summary]/report/bitstreamsCount", + "/checks/[name=Item summary]/report/bundlesCount", + "/checks/[name=Item summary]/report/collectionsSizesInfo/orphanBitstreamsCount", + "/checks/[name=Item summary]/report/collectionsSizesInfo/deletedBitstreams", + "/checks/[name=Item summary]/report/metadataValuesCount", + "/checks/[name=Item summary]/report/handlesCount", + "/checks/[name=Item summary]/report/ePersonsCount", + "/checks/[name=Item summary]/report/groupsCount", + "/checks/[name=User summary]/report/selfRegistered", + "/checks/[name=User summary]/report/subscribers", + "/checks/[name=User summary]/report/subscribedCollections", + "/checks/[name=User summary]/report/emptyGroups", + "/checks/[name=License summary]/report/licenses" ] } \ No newline at end of file diff --git a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java index ce3e8b168be8..77a2293ba7d9 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java @@ -111,7 +111,7 @@ private String formatDate(Date date) { @Test public void testHelpInformation() throws Exception { TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-i" }; + String[] args = new String[] { "report-diff", "-h" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -193,9 +193,8 @@ public void testCompareSpecificCheck() throws Exception { ReportResult report1 = reportResultService.create(context); report1.setType("healthcheck"); - report1.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value1\"}},{\"name\":\"Check2\"" + + report1.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{\"key\":\"value1\"}},{\"name\":\"Item summary\"" + ",\"report\":{\"key\":\"other\"}}]}"); - report1.setArgs("-c: 0"); reportResultService.update(context, report1); // Force commit and flush to ensure timestamp is set context.commit(); @@ -205,9 +204,8 @@ public void testCompareSpecificCheck() throws Exception { ReportResult report2 = reportResultService.create(context); report2.setType("healthcheck"); - report2.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value2\"}},{\"name\":\"Check2\"" + + report2.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{\"key\":\"value2\"}},{\"name\":\"Item summary\"" + ",\"report\":{\"key\":\"other\"}}]}"); - report2.setArgs("-c: 0"); reportResultService.update(context, report2); context.commit(); context.restoreAuthSystemState(); @@ -215,6 +213,7 @@ public void testCompareSpecificCheck() throws Exception { report1 = reportResultService.find(context, report1.getID()); report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + // -c 0 filters comparison to only General Information check String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), "-t", formatDate(report2.getLastModified()), "-c", "0" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -370,7 +369,6 @@ public void testReportDiff() throws Exception { report1.setType("healthcheck"); report1.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value1\"}},{\"name\":\"Check2\"" + ",\"report\":{\"key\":\"other\"}}]}"); - report1.setArgs("-c: 0"); reportResultService.update(context, report1); // Force commit and flush to ensure timestamp is set @@ -383,7 +381,6 @@ public void testReportDiff() throws Exception { report2.setType("healthcheck"); report2.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value2\"}},{\"name\":\"Check2\"" + ",\"report\":{\"key\":\"other\"}}]}"); - report2.setArgs("-c: 0"); reportResultService.update(context, report2); context.commit(); context.restoreAuthSystemState(); @@ -436,10 +433,10 @@ public void testShowDatesLimit() throws Exception { public void testProfessionalReportFormat() throws Exception { context.turnOffAuthorisationSystem(); - // Create first report with sample health data + // Create first report with sample health data using real check name ReportResult report1 = reportResultService.create(context); report1.setType("healthcheck"); - report1.setValue("{\"checks\":[{\"name\":\"HealthCheck\",\"report\":{" + + report1.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{" + "\"publishedItems\":0," + "\"ePersonsCount\":1," + "\"communitiesCount\":0," + @@ -456,7 +453,7 @@ public void testProfessionalReportFormat() throws Exception { // Create second report with changes ReportResult report2 = reportResultService.create(context); report2.setType("healthcheck"); - report2.setValue("{\"checks\":[{\"name\":\"HealthCheck\",\"report\":{" + + report2.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{" + "\"publishedItems\":2," + "\"ePersonsCount\":1721," + "\"communitiesCount\":9," + @@ -584,10 +581,10 @@ public void testCalculateTimePeriod() throws Exception { public void testEnhancedKeyChangesTable() throws Exception { context.turnOffAuthorisationSystem(); - // Create first report with sample health data + // Create first report with sample health data using real check names ReportResult report1 = reportResultService.create(context); report1.setType("healthcheck"); - report1.setValue("{\"checks\":[{\"name\":\"Info summary\",\"report\":{}}," + + report1.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{}}," + "{\"name\":\"Item summary\",\"report\":{" + "\"publishedItems\":10," + "\"ePersonsCount\":5," + @@ -604,7 +601,7 @@ public void testEnhancedKeyChangesTable() throws Exception { // Create second report with changes ReportResult report2 = reportResultService.create(context); report2.setType("healthcheck"); - report2.setValue("{\"checks\":[{\"name\":\"Info summary\",\"report\":{}}," + + report2.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{}}," + "{\"name\":\"Item summary\",\"report\":{" + "\"publishedItems\":25," + "\"ePersonsCount\":8," + @@ -709,4 +706,53 @@ public void testSizeDifferenceFormatting() throws Exception { assertThat("Size differences should show actual size change (9 KB).'", hasSizeDifference, org.hamcrest.Matchers.is(true)); } + + @Test + public void testSkippedChecksSection() throws Exception { + context.turnOffAuthorisationSystem(); + + // Create report1 with checks A and B + ReportResult report1 = reportResultService.create(context); + report1.setType("healthcheck"); + report1.setValue("{\"checks\":[" + + "{\"name\":\"General Information\",\"report\":{\"key\":\"val1\"}}," + + "{\"name\":\"Only In From\",\"report\":{\"key\":\"fromOnly\"}}" + + "]}"); + reportResultService.update(context, report1); + context.commit(); + + Thread.sleep(1000); + + // Create report2 with checks A and C (B missing, C new) + ReportResult report2 = reportResultService.create(context); + report2.setType("healthcheck"); + report2.setValue("{\"checks\":[" + + "{\"name\":\"General Information\",\"report\":{\"key\":\"val2\"}}," + + "{\"name\":\"Only In To\",\"report\":{\"key\":\"toOnly\"}}" + + "]}"); + reportResultService.update(context, report2); + context.commit(); + context.restoreAuthSystemState(); + + report1 = reportResultService.find(context, report1.getID()); + report2 = reportResultService.find(context, report2.getID()); + + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), + "-t", formatDate(report2.getLastModified()) }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + List infoMessages = handler.getInfoMessages(); + + // Should show the skipped checks section + assertThat(infoMessages, hasItem(containsString("Skipped Checks"))); + assertThat(infoMessages, hasItem(containsString("not present in both reports"))); + assertThat(infoMessages, hasItem(containsString("Only In From"))); + assertThat(infoMessages, hasItem(containsString("Only In To"))); + + // The common check "General Information" should be compared normally + assertThat("Should contain diff for common check", + hasDiffOperation(infoMessages, "REPLACE", CHECK_KEY_PATH), + org.hamcrest.Matchers.is(true)); + } } diff --git a/dspace/config/launcher.xml b/dspace/config/launcher.xml index 3853b4e2fabd..dba2f2f73e1b 100644 --- a/dspace/config/launcher.xml +++ b/dspace/config/launcher.xml @@ -7,13 +7,7 @@ org.dspace.storage.bitstore.BitStoreMigrate - - healthcheck - Create health check report - - org.dspace.health.Report - - + checker Run the checksum checker From 1d714c16f0229bd8a158e8339e51d69657c7c870 Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 08:02:38 +0100 Subject: [PATCH 02/16] Fix ReportDiff setup and date validation --- .../dspace/app/healthreport/HealthReport.java | 7 +-- .../org/dspace/app/reportdiff/ReportDiff.java | 61 ++++++------------- 2 files changed, 21 insertions(+), 47 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java index d93a88b0b976..4eba9f6b2d97 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java @@ -259,13 +259,12 @@ public void printHelp() { * This method is used to print the options used for the report. */ private String printCommandlineOptions() { - // Return key-value pairs of options StringBuilder options = new StringBuilder(); for (Option option : commandLine.getOptions()) { String key = option.getOpt(); - String value = commandLine.getOptionValue(key); - if (value != null) { - options.append(String.format(" -%s: %s\n", key, value)); + String[] values = commandLine.getOptionValues(key); + if (values != null) { + options.append(String.format(" -%s: %s\n", key, String.join(", ", values))); } else { options.append(String.format(" -%s\n", key)); } diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java index 215fed799f80..72d232040dd5 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java @@ -135,6 +135,11 @@ private void loadFieldConfiguration() { fieldOrder.add(fieldNode.asText()); } } + } else { + log.warn("Report diff fields configuration '{}' not found on the classpath. " + + "Field mappings will be empty.", REPORT_DIFF_FIELDS); + fieldMappings = new LinkedHashMap<>(); + fieldOrder = new ArrayList<>(); } } catch (IOException e) { log.error("Error loading report diff fields configuration '{}': {}. Using empty configuration.", @@ -269,18 +274,25 @@ private Date parseDateOption(String optionValue) { } /** - * Validate the date range specified by `from` and `to`. - * If the dates are invalid, log an error and return false. - * If both dates are set, ensure that `to` is not before `from`. + * Validate the date range specified by {@code from} and {@code to}. + * Logs a specific error if either date is missing (not provided and not resolvable from the DB) + * or if {@code to} precedes {@code from}. * - * @return true if the date range is valid, false otherwise + * @return true if both dates are set and {@code to} is not before {@code from}, false otherwise */ private boolean validateDateRange() { if (to != null && from != null && to.before(from)) { handler.logError("The 'to' date cannot be before the 'from' date."); return false; - } else if (Objects.isNull(from) || Objects.isNull(to)) { - handler.logError("Both 'from' and 'to' dates must be specified when using a specific check."); + } + if (Objects.isNull(from)) { + handler.logError("The 'from' date could not be determined. " + + "Specify it with -f or ensure at least 2 reports exist in the database."); + return false; + } + if (Objects.isNull(to)) { + handler.logError("The 'to' date could not be determined. " + + "Specify it with -t or ensure at least 1 report exists in the database."); return false; } return true; @@ -939,43 +951,6 @@ private String generateEnhancedKeyChangesTable(String oldJson, String newJson, return table.toString(); } - /** - * Get value from JSON node using path notation (JSON Pointer style). - */ - private JsonNode getValueFromPath(JsonNode node, String path) { - try { - // Use Jackson's JSON Pointer functionality for paths like /checks/0/report/publishedItems - if (path.startsWith("/")) { - return node.at(path); - } - - // Fallback for simple dot notation paths - return getValueFromSimplePath(node, path); - } catch (Exception e) { - return null; - } - } - - /** - * Get value from simple dot-notation path. - */ - private JsonNode getValueFromSimplePath(JsonNode node, String path) { - if (path.isEmpty()) { - return node; - } - String[] parts = path.split("\\."); - JsonNode current = node; - - for (String part : parts) { - if (current == null || !current.has(part)) { - return null; - } - current = current.get(part); - } - - return current; - } - /** * Simple data class for table rows. */ From 2d36bbfae822a97a2979169c7f3260a9eb2b7a07 Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 08:06:33 +0100 Subject: [PATCH 03/16] fix failed integration test --- .../src/test/java/org/dspace/scripts/ReportDiffIT.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java index 77a2293ba7d9..3b3699d5c1a1 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java @@ -659,12 +659,14 @@ public void testSizeDifferenceFormatting() throws Exception { // Create reports with size differences from "0 bytes" to "9 KB" String fromReportJson = "{ \"checks\": [" + - " { \"report\": { \"totalSize\": \"0 bytes\" } }" + + " { \"name\": \"Item summary\", \"report\": {" + + " \"collectionsSizesInfo\": { \"totalSize\": \"0 bytes\" } } }" + "]}"; String toReportJson = "{ \"checks\": [" + - " { \"report\": { \"totalSize\": \"9 KB\" } }" + + " { \"name\": \"Item summary\", \"report\": {" + + " \"collectionsSizesInfo\": { \"totalSize\": \"9 KB\" } } }" + "]}"; ReportResult fromReport = reportResultService.create(context); From aece190f5e3cfb55e98e9d0a29bdb77bc7be0719 Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 08:25:45 +0100 Subject: [PATCH 04/16] checkstyle --- .../src/test/java/org/dspace/scripts/ReportDiffIT.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java index 3b3699d5c1a1..1fbe025a6ae2 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java @@ -193,8 +193,8 @@ public void testCompareSpecificCheck() throws Exception { ReportResult report1 = reportResultService.create(context); report1.setType("healthcheck"); - report1.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{\"key\":\"value1\"}},{\"name\":\"Item summary\"" + - ",\"report\":{\"key\":\"other\"}}]}"); + report1.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{\"key\":\"value1\"}}," + + "{\"name\":\"Item summary\",\"report\":{\"key\":\"other\"}}]}"); reportResultService.update(context, report1); // Force commit and flush to ensure timestamp is set context.commit(); @@ -204,8 +204,8 @@ public void testCompareSpecificCheck() throws Exception { ReportResult report2 = reportResultService.create(context); report2.setType("healthcheck"); - report2.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{\"key\":\"value2\"}},{\"name\":\"Item summary\"" + - ",\"report\":{\"key\":\"other\"}}]}"); + report2.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{\"key\":\"value2\"}}," + + "{\"name\":\"Item summary\",\"report\":{\"key\":\"other\"}}]}"); reportResultService.update(context, report2); context.commit(); context.restoreAuthSystemState(); From f7de663bbc3aa8f7d2d9d86bbc2b4a6280f8696f Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 10:00:49 +0100 Subject: [PATCH 05/16] added tests, used -c 1 2 instead of -c 1 -c 2 --- .../dspace/app/healthreport/HealthReport.java | 2 +- .../HealthReportScriptConfiguration.java | 2 +- .../org/dspace/scripts/HealthReportIT.java | 116 ++++++++++++++++++ .../java/org/dspace/scripts/ReportDiffIT.java | 71 +++++++++++ 4 files changed, 189 insertions(+), 2 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java index 4eba9f6b2d97..52d346b5d0ee 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java @@ -247,7 +247,7 @@ public void printHelp() { " -h, --help Show help information\n" + " -e, --email Send report to specified email address\n" + " -c, --check Perform specific check(s) by index (0-" + (getNumberOfChecks() - 1) + - "). Can be used multiple times, e.g. -c 0 -c 3 -c 4\n" + + "). Accepts multiple space-separated values, e.g. -c 0 3 4\n" + " -f, --for Specify the last N days to consider (positive integer)\n" + " -r, --report Specify a file to save the report\n\n" + "Available checks:\n" + checksNamesToString() + "\n" diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java index bd8245f34363..9f184a2e2b72 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java @@ -39,7 +39,7 @@ public Options getOptions() { options.getOption("e").setType(String.class); options.addOption("c", "check", true, String.format("Perform specific check(s) by index (0 to %d). " + - "Can be used multiple times, e.g. -c 0 -c 3 -c 4.", + "Accepts multiple space-separated values, e.g. -c 0 3 4.", HealthReport.getNumberOfChecks() - 1)); options.getOption("c").setType(String.class); options.getOption("c").setArgs(Integer.MAX_VALUE); diff --git a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java index e77a907ef731..7605c3d17856 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java @@ -14,13 +14,16 @@ import static org.hamcrest.Matchers.hasSize; import java.io.ByteArrayInputStream; +import java.io.File; import java.io.InputStream; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.util.HashSet; import java.util.List; import java.util.Set; import org.dspace.AbstractIntegrationTestWithDatabase; +import org.dspace.app.healthreport.HealthReport; import org.dspace.app.launcher.ScriptLauncher; import org.dspace.app.scripts.handler.impl.TestDSpaceRunnableHandler; import org.dspace.builder.CollectionBuilder; @@ -139,4 +142,117 @@ public void testLicenseCheck() throws Exception { assertThat(messages, hasItem(containsString("UUIDs of items without license bundle:"))); assertThat(messages, hasItem(containsString("PUB"))); } + + /** + * Verifies that -h/--help prints help text and does not run any checks. + * use -h instead of -i. + */ + @Test + public void testHelpOption() throws Exception { + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "health-report", "-h" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getErrorMessages(), empty()); + List messages = handler.getInfoMessages(); + assertThat(messages, hasItem(containsString("HELP"))); + assertThat(messages, hasItem(containsString("Available checks:"))); + } + + /** + * Verifies that multiple -c values run only the specified checks. + * Covers Issue #1328 item 2: support multiple check selection (e.g. -c 0 -c 3). + */ + @Test + public void testMultipleChecks() throws Exception { + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + // Run only check 0 (General Information) and check 3 (License summary): space-separated + String[] args = new String[] { "health-report", "-c", "0", "3" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getErrorMessages(), empty()); + List messages = handler.getInfoMessages(); + assertThat(messages, hasItem(containsString("HEALTH REPORT:"))); + assertThat(messages, hasItem(containsString("General Information"))); + assertThat(messages, hasItem(containsString("License summary"))); + // Item summary (check index 1) should NOT be present + boolean hasItemSummary = messages.stream().anyMatch(m -> m.contains("Item summary:")); + assertThat("Only selected checks should run", hasItemSummary, org.hamcrest.Matchers.is(false)); + } + + /** + * Verifies that an out-of-range -c value causes a script error. + * Covers Issue #1328 item 3: validate -c values. + */ + @Test + public void testInvalidCheckOutOfRange() throws Exception { + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + int maxCheck = HealthReport.getNumberOfChecks() - 1; + String[] args = new String[] { "health-report", "-c", String.valueOf(maxCheck + 1) }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getErrorMessages(), + hasItem(containsString("Must be an integer from 0 to " + maxCheck))); + } + + /** + * Verifies that a non-integer -c value causes a script error. + * Covers Issue #1328 item 3: validate -c values. + */ + @Test + public void testInvalidCheckNonInteger() throws Exception { + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "health-report", "-c", "abc" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getErrorMessages(), + hasItem(containsString("It has to be an integer number from 0 to"))); + } + + /** + * Verifies that a non-positive -f value (zero) causes a script error. + * Covers Issue #1328 item 3: validate -f must be positive integer (greater than 0). + */ + @Test + public void testInvalidForDaysZero() throws Exception { + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "health-report", "-f", "0" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getErrorMessages(), + hasItem(containsString("Must be a positive integer (greater than 0)"))); + } + + /** + * Verifies that a non-integer -f value causes a script error. + * Covers Issue #1328 item 3: validate -f must be integer. + */ + @Test + public void testInvalidForDaysNonInteger() throws Exception { + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "health-report", "-f", "notanumber" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getErrorMessages(), + hasItem(containsString("Must be a positive integer"))); + } + + /** + * Verifies that -r/--report saves report output to the specified file. + * -o/--output renamed to -r/--report. + */ + @Test + public void testReportFileSaved() throws Exception { + File tempFile = File.createTempFile("health-report-test-", ".txt"); + tempFile.deleteOnExit(); + + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "health-report", "-r", tempFile.getAbsolutePath() }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getErrorMessages(), empty()); + assertThat("Report file must exist after -r option", tempFile.exists(), org.hamcrest.Matchers.is(true)); + String content = Files.readString(tempFile.toPath()); + assertThat("Report file must contain health report header", content, containsString("HEALTH REPORT:")); + } } \ No newline at end of file diff --git a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java index 1fbe025a6ae2..38f63edfde64 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java @@ -757,4 +757,75 @@ public void testSkippedChecksSection() throws Exception { hasDiffOperation(infoMessages, "REPLACE", CHECK_KEY_PATH), org.hamcrest.Matchers.is(true)); } + + /** + * Verifies Issue #1334: when one report has more checks than the other, the diff + * compares only the intersection and does NOT show null values for fields in + * the common check. + */ + @Test + public void testIntersectionComparisonNoNullsForCommonChecks() throws Exception { + context.turnOffAuthorisationSystem(); + + // report1 has 5 checks (all) + String allChecksJson = "{\"checks\":[" + + "{\"name\":\"General Information\",\"report\":{\"directoryStats\":[" + + " {\"size_bytes\":1000}," + + " {\"size_bytes\":2000}" + + "]}}," + + "{\"name\":\"Item summary\",\"report\":{\"itemsCount\":100,\"publishedItems\":80}}," + + "{\"name\":\"User summary\",\"report\":{\"selfRegistered\":5}}," + + "{\"name\":\"License summary\",\"report\":{\"licenses\":3}}," + + "{\"name\":\"Embargo check\",\"report\":{}}" + + "]}"; + + // report2 has only 1 check (Item summary) + String singleCheckJson = "{\"checks\":[" + + "{\"name\":\"Item summary\",\"report\":{\"itemsCount\":120,\"publishedItems\":100}}" + + "]}"; + + ReportResult report1 = reportResultService.create(context); + report1.setType("healthcheck"); + report1.setValue(allChecksJson); + reportResultService.update(context, report1); + context.commit(); + + Thread.sleep(1000); + + ReportResult report2 = reportResultService.create(context); + report2.setType("healthcheck"); + report2.setValue(singleCheckJson); + reportResultService.update(context, report2); + context.commit(); + context.restoreAuthSystemState(); + + report1 = reportResultService.find(context, report1.getID()); + report2 = reportResultService.find(context, report2.getID()); + + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), + "-t", formatDate(report2.getLastModified()) }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + List infoMessages = handler.getInfoMessages(); + + // The diff output must not contain null values for the common "Item summary" check + boolean hasNullInKeyChanges = infoMessages.stream() + .anyMatch(msg -> msg.contains("| null") || msg.contains("null |")); + assertThat("Key Changes table must not show null for fields present in both reports", + hasNullInKeyChanges, org.hamcrest.Matchers.is(false)); + + // The checks only present in report1 (General Information, User summary, License summary, Embargo check) + // should appear in the Skipped Checks section + assertThat(infoMessages, hasItem(containsString("Skipped Checks"))); + assertThat(infoMessages, hasItem(containsString("General Information"))); + assertThat(infoMessages, hasItem(containsString("User summary"))); + assertThat(infoMessages, hasItem(containsString("License summary"))); + assertThat(infoMessages, hasItem(containsString("Embargo check"))); + + // Item summary - the common check - must show actual diff (itemsCount 100 -> 120) + assertThat("Should contain diff for common Item summary check", + hasDiffOperation(infoMessages, "REPLACE", "/checks/0/report/itemsCount"), + org.hamcrest.Matchers.is(true)); + } } From 1b891a2cb3c18bea25fa3826823ecc6e50cad317 Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 10:08:18 +0100 Subject: [PATCH 06/16] used UNLIMITED_VALUES unstead of MAX_VALUE --- .../app/healthreport/HealthReportScriptConfiguration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java index 9f184a2e2b72..257c7e06f72f 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java @@ -42,7 +42,7 @@ public Options getOptions() { "Accepts multiple space-separated values, e.g. -c 0 3 4.", HealthReport.getNumberOfChecks() - 1)); options.getOption("c").setType(String.class); - options.getOption("c").setArgs(Integer.MAX_VALUE); + options.getOption("c").setArgs(org.apache.commons.cli.Option.UNLIMITED_VALUES); options.addOption("f", "for", true, "Report for last N days (positive integer). Used only in general information for now."); options.getOption("f").setType(String.class); From 32457afe4d31adb5133ac81e3bcade10ceddc18f Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 10:09:22 +0100 Subject: [PATCH 07/16] fix doc --- .../src/main/java/org/dspace/app/healthreport/HealthReport.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java index 52d346b5d0ee..5a86c46d470a 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java @@ -109,7 +109,7 @@ public void setup() throws ParseException { } // `-c`: Check, perform only specific checks by index (0-`getNumberOfChecks()`). - // Supports multiple values e.g. -c 0 -c 3 -c 4 + // Supports multiple values e.g. -c 0 3 4 if (commandLine.hasOption('c')) { String[] checkOptions = commandLine.getOptionValues('c'); for (String checkOption : checkOptions) { From ae824e8db71715dcc85f314270d54a5945c9c617 Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 10:18:42 +0100 Subject: [PATCH 08/16] fix doc --- .../src/test/java/org/dspace/scripts/HealthReportIT.java | 8 +++----- .../src/test/java/org/dspace/scripts/ReportDiffIT.java | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java index 7605c3d17856..d99f443fddc0 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java @@ -161,7 +161,7 @@ public void testHelpOption() throws Exception { /** * Verifies that multiple -c values run only the specified checks. - * Covers Issue #1328 item 2: support multiple check selection (e.g. -c 0 -c 3). + * Support multiple check selection (e.g. -c 0 -c 3). */ @Test public void testMultipleChecks() throws Exception { @@ -182,7 +182,6 @@ public void testMultipleChecks() throws Exception { /** * Verifies that an out-of-range -c value causes a script error. - * Covers Issue #1328 item 3: validate -c values. */ @Test public void testInvalidCheckOutOfRange() throws Exception { @@ -197,7 +196,6 @@ public void testInvalidCheckOutOfRange() throws Exception { /** * Verifies that a non-integer -c value causes a script error. - * Covers Issue #1328 item 3: validate -c values. */ @Test public void testInvalidCheckNonInteger() throws Exception { @@ -211,7 +209,7 @@ public void testInvalidCheckNonInteger() throws Exception { /** * Verifies that a non-positive -f value (zero) causes a script error. - * Covers Issue #1328 item 3: validate -f must be positive integer (greater than 0). + * Validate -f must be positive integer (greater than 0). */ @Test public void testInvalidForDaysZero() throws Exception { @@ -225,7 +223,7 @@ public void testInvalidForDaysZero() throws Exception { /** * Verifies that a non-integer -f value causes a script error. - * Covers Issue #1328 item 3: validate -f must be integer. + * Validate -f must be integer. */ @Test public void testInvalidForDaysNonInteger() throws Exception { diff --git a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java index 38f63edfde64..764866bb0fd7 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java @@ -759,7 +759,7 @@ public void testSkippedChecksSection() throws Exception { } /** - * Verifies Issue #1334: when one report has more checks than the other, the diff + * When one report has more checks than the other, the diff * compares only the intersection and does NOT show null values for fields in * the common check. */ From f7972098d25f1f3dc5e42def1e3503703746ad69 Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 10:58:36 +0100 Subject: [PATCH 09/16] improved doc --- .../src/main/java/org/dspace/app/reportdiff/ReportDiff.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java index 72d232040dd5..ff4e24be4148 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java @@ -812,6 +812,10 @@ private JsonNode resolveFieldPath(JsonNode rootNode, String fieldPath) { * For example, {@code "checks/[name=General Information]/report/directoryStats/0/size_bytes"} * becomes: {@code ["checks", "[name=General Information]", "report", "directoryStats", "0", "size_bytes"]}. * + *

Limitation: The parser finds the first {@code ]} after an opening {@code [}, so check + * names that themselves contain bracket characters (e.g., {@code [name=Check [beta]]}) are not + * supported and will produce incorrect segments. Check names must not contain {@code [} or {@code ]}. + * * @param path the path to split (without leading slash) * @return list of path segments */ From a5385b42e1806b06cfc2d585103233f16519f3e1 Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 12:02:23 +0100 Subject: [PATCH 10/16] used multilist for -c , removed unused method --- .../org/dspace/app/reportdiff/ReportDiff.java | 40 +++++++++++++------ .../content/ReportResultServiceImpl.java | 6 --- .../dspace/content/dao/ReportResultDAO.java | 11 ----- .../content/dao/impl/ReportResultDAOImpl.java | 14 ------- .../content/service/ReportResultService.java | 10 ----- 5 files changed, 28 insertions(+), 53 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java index ff4e24be4148..66b63258cfb3 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java @@ -77,9 +77,10 @@ public class ReportDiff extends DSpaceRunnable { private long limit = -1; /** - * `-c`: Check, perform only specific check by index (0-`getNumberOfChecks()`). + * `-c`: Check, perform only specific checks by index (0-`getNumberOfChecks()`). + * Supports multiple values. */ - private int specificCheck = -1; + private List specificChecks = new ArrayList<>(); /** * `-f`: From, specify the start date for the report. @@ -169,12 +170,17 @@ public void setup() throws ParseException { return; } - // `-c`: Check, perform only specific check by index (0-`getNumberOfChecks()`). + // `-c`: Check, perform only specific checks by index (0-`getNumberOfChecks()`). + // Supports multiple values e.g. -c 0 3 4 if (commandLine.hasOption('c')) { - specificCheck = parseCheckOption(commandLine.getOptionValue('c')); - if (specificCheck == -1) { - // Error already logged in parseCheckOption - return; + String[] checkOptions = commandLine.getOptionValues('c'); + for (String checkOption : checkOptions) { + int parsedCheck = parseCheckOption(checkOption); + if (parsedCheck == -1) { + // Error already logged in parseCheckOption + return; + } + specificChecks.add(parsedCheck); } } @@ -481,12 +487,16 @@ private NormalizationResult normalizeReportsToIntersection(String fromJson, Stri List commonNames = new ArrayList<>(fromCheckMap.keySet()); commonNames.retainAll(toCheckMap.keySet()); - // If specificCheck is set, further filter to only that check name - if (specificCheck != -1) { - String targetCheckName = HealthReport.getCheckName(specificCheck); - if (targetCheckName != null) { - commonNames.retainAll(java.util.Collections.singletonList(targetCheckName)); + // If specificChecks are set, further filter to only those check names + if (!specificChecks.isEmpty()) { + List targetCheckNames = new ArrayList<>(); + for (int checkIndex : specificChecks) { + String targetCheckName = HealthReport.getCheckName(checkIndex); + if (targetCheckName != null) { + targetCheckNames.add(targetCheckName); + } } + commonNames.retainAll(targetCheckNames); } if (commonNames.isEmpty()) { @@ -499,6 +509,12 @@ private NormalizationResult normalizeReportsToIntersection(String fromJson, Stri List onlyInTo = new ArrayList<>(toCheckMap.keySet()); onlyInTo.removeAll(fromCheckMap.keySet()); + // When specific checks are requested, do not report other checks as skipped + if (!specificChecks.isEmpty()) { + onlyInFrom.clear(); + onlyInTo.clear(); + } + // Build normalized JSON with only the common checks (in the same order) com.fasterxml.jackson.databind.node.ObjectNode normalizedFrom = mapper.createObjectNode(); diff --git a/dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java b/dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java index 4bfa18b668ba..15722dfee6c1 100644 --- a/dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java @@ -52,12 +52,6 @@ public ReportResult findByLastModified(Context context, Date lastModified) throw return reportResultDAO.findByLastModified(context, lastModified); } - @Override - public ReportResult findByLastModifiedAndCheckType(Context context, Date lastModified, int checkType) - throws SQLException { - return reportResultDAO.findByLastModifiedAndCheckType(context, lastModified, checkType); - } - @Override public void delete(Context context, ReportResult reportResult) throws SQLException { reportResultDAO.delete(context, reportResult); diff --git a/dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java b/dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java index 8f2aeabb2e7c..6cf00a632728 100644 --- a/dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java +++ b/dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java @@ -30,16 +30,5 @@ public interface ReportResultDAO extends GenericDAO { * @throws SQLException if a database error occurs */ ReportResult findByLastModified(Context context, Date lastModified) throws SQLException; - - /** - * Find a ReportResult by its last modified date and check type. - * - * @param context the DSpace context - * @param lastModified the exact last modified date to search for - * @param checkType the check type index to filter by (searches within args field) - * @return the ReportResult matching both criteria, or null if not found - * @throws SQLException if a database error occurs - */ - ReportResult findByLastModifiedAndCheckType(Context context, Date lastModified, int checkType) throws SQLException; } diff --git a/dspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.java b/dspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.java index a85d53f5f4be..f8706565da76 100644 --- a/dspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.java @@ -34,18 +34,4 @@ public ReportResult findByLastModified(Context context, Date lastModified) throw return singleResult(query); } - - @Override - public ReportResult findByLastModifiedAndCheckType(Context context, Date lastModified, int checkType) - throws SQLException { - // Use string matching for checkType in args (args contains command line options like "-c: 0") - Query query = createQuery(context, "SELECT r FROM ReportResult r WHERE r.lastModified = :lastModified " + - "AND r.args LIKE :argsPattern"); - - query.setParameter("lastModified", lastModified); - query.setParameter("argsPattern", "%-c: " + checkType + "%"); - query.setHint("org.hibernate.cacheable", Boolean.TRUE); - - return singleResult(query); - } } diff --git a/dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java b/dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java index aa52ec31de99..fabfaeb997a7 100644 --- a/dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java +++ b/dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java @@ -70,16 +70,6 @@ public interface ReportResultService { */ ReportResult findByLastModified(Context context, Date lastModified) throws SQLException; - /** - * Find a ReportResult by last modified date and check type. - * - * @param context the DSpace context - * @param lastModified the exact last modified date to search for - * @param checkType the check type index to filter by - * @return the matching ReportResult, or null if not found - * @throws SQLException if a database error occurs - */ - ReportResult findByLastModifiedAndCheckType(Context context, Date lastModified, int checkType) throws SQLException; /** * Deletes the specified ReportResult instance in the given context. From c47bef0fdd4e762bfff73dcb3e7e4388d9cbb2d0 Mon Sep 17 00:00:00 2001 From: Paurikova2 Date: Wed, 25 Feb 2026 12:21:56 +0100 Subject: [PATCH 11/16] improved doc --- .../src/test/java/org/dspace/scripts/HealthReportIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java index d99f443fddc0..939ad6b41442 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java @@ -160,8 +160,8 @@ public void testHelpOption() throws Exception { } /** - * Verifies that multiple -c values run only the specified checks. - * Support multiple check selection (e.g. -c 0 -c 3). + * Verifies that multiple values for a single -c option run only the specified checks. + * Supports multiple check selection (e.g. -c 0 3). */ @Test public void testMultipleChecks() throws Exception { From 887fce0501c69bad2b3f16bdc328b87d746949fd Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Tue, 12 May 2026 10:47:28 +0200 Subject: [PATCH 12/16] WIP updated health report and report diff --- .../dspace/app/healthreport/HealthReport.java | 16 +- .../org/dspace/app/reportdiff/ReportDiff.java | 221 +++++++++++++----- .../ReportDiffScriptConfiguration.java | 6 +- .../org/dspace/scripts/HealthReportIT.java | 18 ++ .../java/org/dspace/scripts/ReportDiffIT.java | 156 +++++-------- 5 files changed, 264 insertions(+), 153 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java index 5a86c46d470a..451513f8cee8 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java @@ -16,9 +16,11 @@ import java.util.ArrayList; import java.util.Date; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Set; import javax.mail.MessagingException; import org.apache.commons.cli.Option; @@ -217,6 +219,7 @@ public void internalRun() throws Exception { if (reportFile != null) { InputStream inputStream = toInputStream(sbReport.toString(), StandardCharsets.UTF_8); handler.writeFilestream(context, reportFile, inputStream, "export"); + context.commit(); context.restoreAuthSystemState(); @@ -260,11 +263,20 @@ public void printHelp() { */ private String printCommandlineOptions() { StringBuilder options = new StringBuilder(); + Set processedOptions = new LinkedHashSet<>(); + for (Option option : commandLine.getOptions()) { String key = option.getOpt(); + if (key == null || processedOptions.contains(key)) { + continue; + } + processedOptions.add(key); + String[] values = commandLine.getOptionValues(key); - if (values != null) { - options.append(String.format(" -%s: %s\n", key, String.join(", ", values))); + if (values != null && values.length > 0) { + for (String value : values) { + options.append(String.format(" -%s: %s\n", key, value)); + } } else { options.append(String.format(" -%s\n", key)); } diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java index 66b63258cfb3..87d5d71f631e 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java @@ -10,7 +10,6 @@ import java.io.IOException; import java.io.InputStream; import java.sql.SQLException; -import java.time.LocalDateTime; import java.time.ZoneId; import java.time.format.DateTimeFormatter; import java.util.ArrayList; @@ -22,6 +21,8 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.mail.MessagingException; import com.fasterxml.jackson.databind.JsonNode; @@ -83,14 +84,14 @@ public class ReportDiff extends DSpaceRunnable { private List specificChecks = new ArrayList<>(); /** - * `-f`: From, specify the start date for the report. + * `-f`: From, specify source report ID. */ - private Date from = null; + private Integer fromReportId = null; /** - * `-t`: Till, specify the end date for the report. + * `-t`: Till, specify target report ID. */ - private Date to = null; + private Integer toReportId = null; /** * `-e`: Email, send report to specified email address. @@ -102,6 +103,8 @@ public class ReportDiff extends DSpaceRunnable { private static final String REPORT_DIFF_FIELDS = "report-diff-fields.json"; private static final String FIELD_MAPPINGS_KEY = "fieldMappings"; private static final String FIELD_ORDER_KEY = "fieldOrder"; + private static final Pattern SHORT_ARG_WITH_VALUE = Pattern.compile("^-([a-zA-Z]):\\s*(.*)$"); + private static final Pattern SHORT_ARG_WITHOUT_VALUE = Pattern.compile("^-([a-zA-Z])$"); // Field configuration cache private static Map fieldMappings = null; @@ -198,10 +201,17 @@ public void setup() throws ParseException { } - // `-f`: From, specify the start date for the report. - from = parseDateOption(commandLine.getOptionValue('f')); - // `-t`: To, specify the end date for the report. - to = parseDateOption(commandLine.getOptionValue('t')); + // `-f`: From, specify source report ID. + fromReportId = parseReportIdOption(commandLine.getOptionValue('f')); + if (commandLine.hasOption('f') && fromReportId == null) { + return; + } + + // `-t`: To, specify target report ID. + toReportId = parseReportIdOption(commandLine.getOptionValue('t')); + if (commandLine.hasOption('t') && toReportId == null) { + return; + } if (commandLine.hasOption('e')) { emails = commandLine.getOptionValues('e'); @@ -223,10 +233,9 @@ public void internalRun() throws Exception { return; } try (Context context = new Context()) { - defaultDate(context); + defaultReportIds(context); - // If the user specified a specific check, we need to ensure that both `from` and `to` dates are set. - if (!validateDateRange()) { + if (!validateReportIdSelection()) { return; } @@ -259,61 +268,60 @@ private int parseCheckOption(String checkOption) { } /** - * Parse the date option and return a Date object. + * Parse report ID option and return an Integer. * If the option is invalid, log an error and return null. - * The date format is expected to be "yyyy-MM-dd HH:mm:ss.SSS". * * @param optionValue the date option value - * @return the parsed Date or null if invalid + * @return the parsed report ID or null if invalid */ - private Date parseDateOption(String optionValue) { + private Integer parseReportIdOption(String optionValue) { if (optionValue == null) { return null; } try { - LocalDateTime ldt = LocalDateTime.parse(optionValue, FORMATTER); - return Date.from(ldt.atZone(ZoneId.systemDefault()).toInstant()); + return Integer.parseInt(optionValue); } catch (Exception e) { - handler.logError("Cannot create a Date from the input: " + optionValue); + handler.logError("Cannot parse report ID from input: " + optionValue); return null; } } /** - * Validate the date range specified by {@code from} and {@code to}. - * Logs a specific error if either date is missing (not provided and not resolvable from the DB) - * or if {@code to} precedes {@code from}. + * Validate report ID selection for comparison. + * If one ID is set, both must be set. * - * @return true if both dates are set and {@code to} is not before {@code from}, false otherwise + * @return true if the report IDs are valid, false otherwise */ - private boolean validateDateRange() { - if (to != null && from != null && to.before(from)) { - handler.logError("The 'to' date cannot be before the 'from' date."); + private boolean validateReportIdSelection() { + if ((Objects.isNull(fromReportId) && Objects.nonNull(toReportId)) + || (Objects.nonNull(fromReportId) && Objects.isNull(toReportId))) { + handler.logError("Both 'from' and 'to' report IDs must be specified."); return false; } - if (Objects.isNull(from)) { - handler.logError("The 'from' date could not be determined. " + - "Specify it with -f or ensure at least 2 reports exist in the database."); + + if (Objects.nonNull(fromReportId) && fromReportId <= 0) { + handler.logError("The 'from' report ID must be a positive integer."); return false; } - if (Objects.isNull(to)) { - handler.logError("The 'to' date could not be determined. " + - "Specify it with -t or ensure at least 1 report exists in the database."); + + if (Objects.nonNull(toReportId) && toReportId <= 0) { + handler.logError("The 'to' report ID must be a positive integer."); return false; } + return true; } /** - * Sets default values for the `from` and `to` dates if they are not already specified. + * Sets default values for the source and target report IDs if not already specified. * * @param context the application context used for fetching reports and logging */ - private void defaultDate(Context context) { - if (Objects.nonNull(from) && Objects.nonNull(to)) { + private void defaultReportIds(Context context) { + if (Objects.nonNull(fromReportId) && Objects.nonNull(toReportId)) { return; } - handler.logInfo("No dates specified, using the last two dates from the database."); + handler.logInfo("No report IDs specified, using the last two reports from the database."); try { List allReports = reportResultService.findAll(context); @@ -324,11 +332,11 @@ private void defaultDate(Context context) { int size = allReports.size(); - if (Objects.isNull(to) && size > 0) { - to = allReports.get(size - 1).getLastModified(); + if (Objects.isNull(toReportId) && size > 0) { + toReportId = allReports.get(size - 1).getID(); } - if (Objects.isNull(from) && size > 1) { - from = allReports.get(size - 2).getLastModified(); + if (Objects.isNull(fromReportId) && size > 1) { + fromReportId = allReports.get(size - 2).getID(); } } catch (SQLException e) { throw new RuntimeException(e); @@ -336,10 +344,10 @@ private void defaultDate(Context context) { } /** - * Display all report dates for the specified check type. + * Display available reports with their IDs and timestamps. * If no reports are found, log an appropriate message. - * Display the last 20 report dates for each type, sorted by date. - * In the format "Report Type: \n - | \n", + * Display the last 20 report entries for each type, sorted by date. + * In the format "Report Type: \n - ID: | | \n", */ private void displayReportDates() { try (Context context = new Context()) { @@ -355,10 +363,10 @@ private void displayReportDates() { .toInstant() .atZone(ZoneId.systemDefault()).toLocalDateTime()); reportDatesMap.computeIfAbsent(report.getType(), k -> new ArrayList<>()) - .add(new DateWithArgs(formattedDate, report.getArgs())); + .add(new DateWithArgs(report.getID(), formattedDate, report.getArgs())); } - StringBuilder sb = new StringBuilder("Report Dates Summary:\n"); + StringBuilder sb = new StringBuilder("Available Reports Summary:\n"); reportDatesMap.forEach((type, dates) -> { sb.append("Report Type: ").append(type).append("\n"); dates.stream() @@ -366,9 +374,11 @@ private void displayReportDates() { .limit(20) .forEach(dwa -> sb .append(" - ") + .append("ID: ").append(dwa.getId()) + .append(" | ") .append(dwa.getDate()) .append(" | ") - .append(dwa.getArgs() != null ? dwa.getArgs().strip() : "") + .append(formatReportArgsForDisplay(dwa.getArgs())) .append("\n")); }); @@ -378,6 +388,85 @@ private void displayReportDates() { } } + private String formatReportArgsForDisplay(String args) { + if (args == null || args.isBlank()) { + return ""; + } + + List formattedEntries = new ArrayList<>(); + String[] lines = args.split("\\r?\\n"); + for (String line : lines) { + String trimmed = line.trim(); + if (trimmed.isEmpty()) { + continue; + } + + Matcher withValue = SHORT_ARG_WITH_VALUE.matcher(trimmed); + if (withValue.matches()) { + String shortOpt = withValue.group(1); + String value = withValue.group(2); + String longOpt = resolveHealthReportLongOption(shortOpt); + + if ("c".equals(shortOpt)) { + value = appendCheckName(value); + } + + if (longOpt != null) { + formattedEntries.add("--" + longOpt + ": " + value); + } else { + formattedEntries.add(trimmed); + } + continue; + } + + Matcher withoutValue = SHORT_ARG_WITHOUT_VALUE.matcher(trimmed); + if (withoutValue.matches()) { + String shortOpt = withoutValue.group(1); + String longOpt = resolveHealthReportLongOption(shortOpt); + if (longOpt != null) { + formattedEntries.add("--" + longOpt); + } else { + formattedEntries.add(trimmed); + } + continue; + } + + formattedEntries.add(trimmed); + } + + return String.join(", ", formattedEntries); + } + + private String resolveHealthReportLongOption(String shortOpt) { + switch (shortOpt) { + case "h": + return "help"; + case "e": + return "email"; + case "c": + return "check"; + case "f": + return "for"; + case "r": + return "report"; + default: + return null; + } + } + + private String appendCheckName(String value) { + try { + int checkIndex = Integer.parseInt(value.trim()); + String checkName = HealthReport.getCheckName(checkIndex); + if (checkName != null) { + return checkIndex + " (" + checkName + ")"; + } + return value; + } catch (NumberFormatException e) { + return value; + } + } + /** * Compare two reports based on the specified `from` and `to` dates. * If the reports are not found, log an appropriate message. @@ -390,11 +479,11 @@ private void compareReports(Context context) { try { context.setCurrentUser(ePersonService.find(context, getEpersonIdentifier())); - ReportResult fromReport = reportResultService.findByLastModified(context, from); - ReportResult toReport = reportResultService.findByLastModified(context, to); + ReportResult fromReport = reportResultService.find(context, fromReportId); + ReportResult toReport = reportResultService.find(context, toReportId); if (fromReport == null || toReport == null) { - handler.logInfo("No reports found for specified dates."); + handler.logInfo("No reports found for specified report IDs."); return; } @@ -577,8 +666,10 @@ private String generateReportComparison(ReportResult fromReport, ReportResult to // Report metadata sb.append("Report Type: ").append(toReport.getType()).append("\n"); - sb.append("From: ").append(fromReport.getLastModified()).append("\n"); - sb.append("To: ").append(toReport.getLastModified()).append("\n"); + sb.append("Source Report: ID ").append(fromReport.getID()) + .append(" at ").append(fromReport.getLastModified()).append("\n"); + sb.append("Target Report: ID ").append(toReport.getID()) + .append(" at ").append(toReport.getLastModified()).append("\n"); // Calculate time period String timePeriod = calculateTimePeriod(fromReport.getLastModified(), toReport.getLastModified()); @@ -674,6 +765,20 @@ private String getDisplayValue(JsonNode node) { * @return difference string */ private String calculateDifference(JsonNode oldValue, JsonNode newValue) { + if (oldValue == null || newValue == null + || oldValue.isMissingNode() || newValue.isMissingNode() + || oldValue.isNull() || newValue.isNull()) { + if ((oldValue == null || oldValue.isMissingNode() || oldValue.isNull()) + && !(newValue == null || newValue.isMissingNode() || newValue.isNull())) { + return "Added"; + } + if ((newValue == null || newValue.isMissingNode() || newValue.isNull()) + && !(oldValue == null || oldValue.isMissingNode() || oldValue.isNull())) { + return "Removed"; + } + return "Changed"; + } + if (oldValue.isNumber() && newValue.isNumber()) { long oldNum = oldValue.asLong(); long newNum = newValue.asLong(); @@ -1050,10 +1155,10 @@ private String generateDetailedSummary(String oldJson, String newJson) throws IO public void printHelp() { handler.printHelp(getScriptConfiguration().getOptions(), getScriptConfiguration().getName()); handler.logInfo("This script compares two health reports and shows the differences between them."); - handler.logInfo("You can specify the 'from' and 'to' dates to compare reports from specific dates."); - handler.logInfo("If you want to see all available report dates, use the '-d' option."); + handler.logInfo("You can specify the 'from' and 'to' report IDs to compare specific reports."); + handler.logInfo("If you want to see all available reports with IDs, use the '-d' option."); handler.logInfo("If you want to compare a specific check, use the '-c' option with the check index, " + - "in this case you must also specify the `from` and `to` dates."); + "in this case you must also specify the `from` and `to` report IDs."); handler.logInfo("If you want to send the report to a specified email address, use '-e'."); } @@ -1228,14 +1333,20 @@ private static String nodeToEscapedString(JsonNode node) { * Used for displaying report dates with their arguments. */ class DateWithArgs { + private final Integer id; private final String date; private final String args; - public DateWithArgs(String date, String args) { + public DateWithArgs(Integer id, String date, String args) { + this.id = id; this.date = date; this.args = args; } + public Integer getId() { + return id; + } + public String getDate() { return date; } diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java index 4c89469d7b16..52001c463d5e 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java @@ -44,16 +44,16 @@ public Options getOptions() { HealthReport.getNumberOfChecks() - 1)); options.getOption("c").setType(String.class); - options.addOption("d", "dates", false, "Show all report dates"); + options.addOption("d", "dates", false, "Show available reports (ID, timestamp, args)"); options.addOption("l", "limit", true, "Limit the number of entries (use only with -d). If omitted, all entries are shown."); options.getOption("l").setType(String.class); - options.addOption("f", "from", true,"Report from specific date [YYYY-MM-DD HH:mm:ss.SSS]."); + options.addOption("f", "from", true,"Source report ID (integer)."); options.getOption("f").setType(String.class); - options.addOption("t", "to", true,"Report to specific date [YYYY-MM-DD HH:mm:ss.SSS]."); + options.addOption("t", "to", true,"Target report ID (integer)."); options.getOption("t").setType(String.class); super.options = options; diff --git a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java index 939ad6b41442..73836ba86954 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java @@ -34,11 +34,13 @@ import org.dspace.content.Collection; import org.dspace.content.Community; import org.dspace.content.Item; +import org.dspace.content.ReportResult; import org.dspace.content.clarin.ClarinLicense; import org.dspace.content.clarin.ClarinLicenseLabel; import org.dspace.content.clarin.ClarinLicenseResourceMapping; import org.dspace.content.factory.ClarinServiceFactory; import org.dspace.content.factory.ContentServiceFactory; +import org.dspace.content.service.ReportResultService; import org.dspace.content.service.BitstreamService; import org.dspace.content.service.BundleService; import org.dspace.content.service.clarin.ClarinLicenseLabelService; @@ -253,4 +255,20 @@ public void testReportFileSaved() throws Exception { String content = Files.readString(tempFile.toPath()); assertThat("Report file must contain health report header", content, containsString("HEALTH REPORT:")); } + @Test + public void testStoredArgsContainAllCheckOptions() throws Exception { + ReportResultService reportResultService = ContentServiceFactory.getInstance().getReportResultService(); + + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "health-report", "-c", "2", "-c", "3" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + context.reloadEntity(eperson); + List 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")); + } } \ No newline at end of file diff --git a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java index 764866bb0fd7..82eb61d522cd 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java @@ -126,6 +126,7 @@ public void testShowDates() throws Exception { ReportResult report1 = reportResultService.create(context); report1.setType("healthcheck"); report1.setValue("{\"checks\":[]}"); + report1.setArgs("-c: 0\n-c: 0\n-r: reportout.txt\n-f: 3\n"); reportResultService.update(context, report1); // Force commit and flush to ensure timestamp is set context.commit(); @@ -136,6 +137,7 @@ public void testShowDates() throws Exception { ReportResult report2 = reportResultService.create(context); report2.setType("healthcheck"); report2.setValue("{\"checks\":[]}"); + report2.setArgs("-c: 2, 3\n-r: reportout.csv\n"); reportResultService.update(context, report2); context.commit(); context.restoreAuthSystemState(); @@ -147,10 +149,16 @@ public void testShowDates() throws Exception { ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); - assertThat(infoMessages, hasItem(containsString("Report Dates Summary:"))); + assertThat(infoMessages, hasItem(containsString("Available Reports Summary:"))); assertThat(infoMessages, hasItem(containsString("Report Type: healthcheck"))); + assertThat(infoMessages, hasItem(containsString("ID: " + report1.getID()))); + assertThat(infoMessages, hasItem(containsString("ID: " + report2.getID()))); assertThat(infoMessages, hasItem(containsString(formatDate(report1.getLastModified())))); assertThat(infoMessages, hasItem(containsString(formatDate(report2.getLastModified())))); + assertThat(infoMessages, hasItem(containsString("--check: 0 (General Information)"))); + assertThat(infoMessages, hasItem(containsString("--report: reportout.txt"))); + assertThat(infoMessages, hasItem(containsString("--for: 3"))); + assertThat(infoMessages, hasItem(containsString("--check: 2, 3"))); } @Test @@ -177,8 +185,8 @@ public void testCompareReports() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()) }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -214,8 +222,8 @@ public void testCompareSpecificCheck() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); // -c 0 filters comparison to only General Information check - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()), "-c", "0" }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()), "-c", "0" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -228,8 +236,7 @@ public void testCompareSpecificCheck() throws Exception { @Test public void testInvalidCheckIndex() throws Exception { TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", "2023-01-01 00:00:00.000", - "-t", "2023-01-02 00:00:00.000", "-c", "999" }; + String[] args = new String[] { "report-diff", "-f", "1", "-t", "2", "-c", "999" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List errorMessages = handler.getErrorMessages(); @@ -238,35 +245,33 @@ public void testInvalidCheckIndex() throws Exception { } @Test - public void testInvalidDateFormat() throws Exception { + public void testInvalidReportIdFormat() throws Exception { TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", "invalid-date", "-t", "2023-01-02 00:00:00.000" }; + String[] args = new String[] { "report-diff", "-f", "invalid-id", "-t", "2" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List errorMessages = handler.getErrorMessages(); - assertThat(errorMessages, hasItem(containsString("Cannot create a Date from the input: invalid-date"))); + assertThat(errorMessages, hasItem(containsString("Cannot parse report ID from input: invalid-id"))); } @Test - public void testNoReportsForDates() throws Exception { + public void testNoReportsForIds() throws Exception { TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", "2022-01-01 00:00:00.000", - "-t", "2022-01-02 00:00:00.000" }; + String[] args = new String[] { "report-diff", "-f", "999999", "-t", "999998" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); - assertThat(infoMessages, hasItem(containsString("No reports found for specified dates."))); + assertThat(infoMessages, hasItem(containsString("No reports found for specified report IDs."))); } @Test - public void testToBeforeFrom() throws Exception { + public void testNonPositiveReportId() throws Exception { TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", "2023-01-02 00:00:00.000", - "-t", "2023-01-01 00:00:00.000" }; + String[] args = new String[] { "report-diff", "-f", "-1", "-t", "1" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List errorMessages = handler.getErrorMessages(); - assertThat(errorMessages, hasItem(containsString("The 'to' date cannot be before the 'from' date."))); + assertThat(errorMessages, hasItem(containsString("The 'from' report ID must be a positive integer."))); } @Test @@ -291,8 +296,8 @@ public void testReportWithMissingValue() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()) }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -322,8 +327,8 @@ public void testNoDifferences() throws Exception { report1 = reportResultService.find(context, report1.getID()); report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()) }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -357,8 +362,8 @@ public void testNoEnteredDate() throws Exception { ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); - assertThat(infoMessages, hasItem(containsString("No dates specified, " + - "using the last two dates from the database."))); + assertThat(infoMessages, hasItem(containsString("No report IDs specified, " + + "using the last two reports from the database."))); } @Test @@ -423,10 +428,10 @@ public void testShowDatesLimit() throws Exception { ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); - assertThat(infoMessages, hasItem(containsString("Report Dates Summary:"))); + assertThat(infoMessages, hasItem(containsString("Available Reports Summary:"))); assertThat(infoMessages, hasItem(containsString("Report Type: healthcheck"))); - assertThat(infoMessages, not(hasItem(containsString(formatDate(report1.getLastModified()))))); - assertThat(infoMessages, hasItem(containsString(formatDate(report2.getLastModified())))); + assertThat(infoMessages, not(hasItem(containsString("ID: " + report1.getID())))); + assertThat(infoMessages, hasItem(containsString("ID: " + report2.getID()))); } @Test @@ -470,8 +475,8 @@ public void testProfessionalReportFormat() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()) }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -526,8 +531,8 @@ public void testReportFormatWithNoChanges() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()) }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -563,8 +568,8 @@ public void testCalculateTimePeriod() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()) }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -618,8 +623,8 @@ public void testEnhancedKeyChangesTable() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()) }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -659,14 +664,16 @@ public void testSizeDifferenceFormatting() throws Exception { // Create reports with size differences from "0 bytes" to "9 KB" String fromReportJson = "{ \"checks\": [" + - " { \"name\": \"Item summary\", \"report\": {" + - " \"collectionsSizesInfo\": { \"totalSize\": \"0 bytes\" } } }" + + " { \"name\": \"Item summary\", \"report\": { " + + " \"collectionsSizesInfo\": { \"totalSize\": \"0 bytes\" }" + + " } }" + "]}"; String toReportJson = "{ \"checks\": [" + - " { \"name\": \"Item summary\", \"report\": {" + - " \"collectionsSizesInfo\": { \"totalSize\": \"9 KB\" } } }" + + " { \"name\": \"Item summary\", \"report\": { " + + " \"collectionsSizesInfo\": { \"totalSize\": \"9 KB\" }" + + " } }" + "]}"; ReportResult fromReport = reportResultService.create(context); @@ -693,19 +700,19 @@ public void testSizeDifferenceFormatting() throws Exception { String[] args = new String[] { "report-diff", - "-f", formatDate(fromReport.getLastModified()), - "-t", formatDate(toReport.getLastModified()) + "-f", String.valueOf(fromReport.getID()), + "-t", String.valueOf(toReport.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), testHandler, kernelImpl); List infoMessages = testHandler.getInfoMessages(); - // Verify that size differences show actual byte differences instead of "Changed" + // Verify that size differences show actual size delta instead of "Changed" boolean hasSizeDifference = infoMessages.stream() - .anyMatch(msg -> msg.contains("totalSize") && msg.contains("9 KB")); + .anyMatch(msg -> msg.contains("Total Content Size") && msg.contains("+9 KB")); - assertThat("Size differences should show actual size change (9 KB).'", + assertThat("Size differences should show actual size change (+9 KB).", hasSizeDifference, org.hamcrest.Matchers.is(true)); } @@ -740,8 +747,8 @@ public void testSkippedChecksSection() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()) }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -758,35 +765,13 @@ public void testSkippedChecksSection() throws Exception { org.hamcrest.Matchers.is(true)); } - /** - * When one report has more checks than the other, the diff - * compares only the intersection and does NOT show null values for fields in - * the common check. - */ @Test - public void testIntersectionComparisonNoNullsForCommonChecks() throws Exception { + public void testCompareReportsWithMissingMappedFieldDoesNotFail() throws Exception { context.turnOffAuthorisationSystem(); - // report1 has 5 checks (all) - String allChecksJson = "{\"checks\":[" + - "{\"name\":\"General Information\",\"report\":{\"directoryStats\":[" + - " {\"size_bytes\":1000}," + - " {\"size_bytes\":2000}" + - "]}}," + - "{\"name\":\"Item summary\",\"report\":{\"itemsCount\":100,\"publishedItems\":80}}," + - "{\"name\":\"User summary\",\"report\":{\"selfRegistered\":5}}," + - "{\"name\":\"License summary\",\"report\":{\"licenses\":3}}," + - "{\"name\":\"Embargo check\",\"report\":{}}" + - "]}"; - - // report2 has only 1 check (Item summary) - String singleCheckJson = "{\"checks\":[" + - "{\"name\":\"Item summary\",\"report\":{\"itemsCount\":120,\"publishedItems\":100}}" + - "]}"; - ReportResult report1 = reportResultService.create(context); report1.setType("healthcheck"); - report1.setValue(allChecksJson); + report1.setValue("{\"checks\":[{\"name\":\"Item summary\",\"report\":{}}]}"); reportResultService.update(context, report1); context.commit(); @@ -794,7 +779,9 @@ public void testIntersectionComparisonNoNullsForCommonChecks() throws Exception ReportResult report2 = reportResultService.create(context); report2.setType("healthcheck"); - report2.setValue(singleCheckJson); + report2.setValue("{\"checks\":[{\"name\":\"Item summary\",\"report\":{" + + "\"publishedItems\":2" + + "}}]}"); reportResultService.update(context, report2); context.commit(); context.restoreAuthSystemState(); @@ -803,29 +790,12 @@ public void testIntersectionComparisonNoNullsForCommonChecks() throws Exception report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", formatDate(report1.getLastModified()), - "-t", formatDate(report2.getLastModified()) }; + String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); - List infoMessages = handler.getInfoMessages(); - - // The diff output must not contain null values for the common "Item summary" check - boolean hasNullInKeyChanges = infoMessages.stream() - .anyMatch(msg -> msg.contains("| null") || msg.contains("null |")); - assertThat("Key Changes table must not show null for fields present in both reports", - hasNullInKeyChanges, org.hamcrest.Matchers.is(false)); - - // The checks only present in report1 (General Information, User summary, License summary, Embargo check) - // should appear in the Skipped Checks section - assertThat(infoMessages, hasItem(containsString("Skipped Checks"))); - assertThat(infoMessages, hasItem(containsString("General Information"))); - assertThat(infoMessages, hasItem(containsString("User summary"))); - assertThat(infoMessages, hasItem(containsString("License summary"))); - assertThat(infoMessages, hasItem(containsString("Embargo check"))); - - // Item summary - the common check - must show actual diff (itemsCount 100 -> 120) - assertThat("Should contain diff for common Item summary check", - hasDiffOperation(infoMessages, "REPLACE", "/checks/0/report/itemsCount"), - org.hamcrest.Matchers.is(true)); + assertThat("Script should not fail with missing mapped field", handler.getErrorMessages(), empty()); + assertThat(handler.getInfoMessages(), hasItem(containsString("Repository Health Report Diff"))); + assertThat(handler.getInfoMessages(), hasItem(containsString("Published Items"))); } } From c17703836419d753674d881cc226fd2789ed7adb Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Wed, 20 May 2026 15:49:21 +0200 Subject: [PATCH 13/16] Complete update of health-report and report-diff --- .../dspace/app/healthreport/HealthReport.java | 4 +- .../org/dspace/app/reportdiff/ReportDiff.java | 253 ++++++++++++------ .../ReportDiffScriptConfiguration.java | 17 +- .../main/resources/report-diff-fields.json | 4 +- .../java/org/dspace/scripts/ReportDiffIT.java | 238 ++++++++++++++-- .../rest/repository/ScriptRestRepository.java | 10 +- 6 files changed, 406 insertions(+), 120 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java index 451513f8cee8..f18c148b3016 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java @@ -107,7 +107,6 @@ public void setup() throws ParseException { // `-e`: Email, send report to specified email address. if (commandLine.hasOption('e')) { emails = commandLine.getOptionValues('e'); - handler.logInfo("\nReport sent to this email address: " + String.join(", ", emails)); } // `-c`: Check, perform only specific checks by index (0-`getNumberOfChecks()`). @@ -234,8 +233,11 @@ public void internalRun() throws Exception { } e.addArgument(sbReport.toString()); e.send(); + handler.logInfo("Report sent to: " + String.join(", ", emails)); } catch (IOException | MessagingException e) { log.error("Error sending email:", e); + handler.logError("Error sending email to " + String.join(", ", emails) + + ": " + e.getMessage()); } } diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java index 87d5d71f631e..af0103a10908 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java @@ -58,8 +58,8 @@ public class ReportDiff extends DSpaceRunnable { private static final ObjectMapper mapper = new ObjectMapper(); - private ReportResultService reportResultService; - private EPersonService ePersonService; + private ReportResultService reportResultService = ContentServiceFactory.getInstance().getReportResultService(); + private EPersonService ePersonService = EPersonServiceFactory.getInstance().getEPersonService(); /** * `-h`: Help, show help information. @@ -67,15 +67,15 @@ public class ReportDiff extends DSpaceRunnable { private boolean help = false; /** - * `-d`: Dates, show all dates that the report was generated for a specific check type. + * `-l`: List all stored reports with IDs, timestamps and arguments. */ - private boolean showDates = false; + private boolean showList = false; /** - * `-l`: Limits the number of report entries (dates) displayed when using the --date option. + * `-m`: Maximum number of report entries displayed when using --list. * Default is -1 (no limit). */ - private long limit = -1; + private long maxEntries = -1; /** * `-c`: Check, perform only specific checks by index (0-`getNumberOfChecks()`). @@ -84,14 +84,14 @@ public class ReportDiff extends DSpaceRunnable { private List specificChecks = new ArrayList<>(); /** - * `-f`: From, specify source report ID. + * `-s`: Source, specify source report ID. */ - private Integer fromReportId = null; + private Integer sourceReportId = null; /** - * `-t`: Till, specify target report ID. + * `-t`: Till, specify target report ID. */ - private Integer toReportId = null; + private Integer targetReportId = null; /** * `-e`: Email, send report to specified email address. @@ -166,7 +166,6 @@ public ReportDiffScriptConfiguration getScriptConfiguration() { @Override public void setup() throws ParseException { ePersonService = EPersonServiceFactory.getInstance().getEPersonService(); - reportResultService = ContentServiceFactory.getInstance().getReportResultService(); // `-h`: Help, show help information. if (commandLine.hasOption('h')) { help = true; @@ -180,42 +179,61 @@ public void setup() throws ParseException { for (String checkOption : checkOptions) { int parsedCheck = parseCheckOption(checkOption); if (parsedCheck == -1) { - // Error already logged in parseCheckOption - return; + handler.logWarning("Invalid value for -c: '" + checkOption + + "'. All checks will be compared."); + specificChecks.clear(); + break; } specificChecks.add(parsedCheck); } } - // `-d`: Dates, show all dates that the report was generated for a specific check type. - if (commandLine.hasOption('d')) { - showDates = true; - try { - if (commandLine.hasOption("l")) { - limit = Long.parseLong(commandLine.getOptionValue("l")); + // `-l`: List all available reports with IDs/timestamps/args. + if (commandLine.hasOption('l')) { + showList = true; + if (commandLine.hasOption('m')) { + String mValue = commandLine.getOptionValue('m'); + try { + long parsedMax = Long.parseLong(mValue); + if (parsedMax <= 0) { + handler.logWarning("Invalid value for -m: '" + mValue + + "'. Must be a positive integer. All entries will be shown."); + maxEntries = -1; + } else { + maxEntries = parsedMax; + } + } catch (NumberFormatException e) { + handler.logWarning("Invalid value for -m: '" + mValue + + "'. Must be a positive integer. All entries will be shown."); + maxEntries = -1; } - } catch (NumberFormatException e) { - handler.logError("Invalid value for -l. Must be a valid number."); - return; } } - // `-f`: From, specify source report ID. - fromReportId = parseReportIdOption(commandLine.getOptionValue('f')); - if (commandLine.hasOption('f') && fromReportId == null) { - return; + // `-s`: Source, specify source report ID. + if (commandLine.hasOption('s')) { + String sValue = commandLine.getOptionValue('s'); + sourceReportId = parseReportIdOption(sValue); + if (sourceReportId == null) { + handler.logWarning("Invalid value for -s: '" + sValue + + "'. The last two reports from the database will be compared instead."); + } } - // `-t`: To, specify target report ID. - toReportId = parseReportIdOption(commandLine.getOptionValue('t')); - if (commandLine.hasOption('t') && toReportId == null) { - return; + // `-t`: Target, specify target report ID. + if (commandLine.hasOption('t')) { + String tValue = commandLine.getOptionValue('t'); + targetReportId = parseReportIdOption(tValue); + if (targetReportId == null) { + handler.logWarning("Invalid value for -t: '" + tValue + + "'. The last two reports from the database will be compared instead."); + } } if (commandLine.hasOption('e')) { emails = commandLine.getOptionValues('e'); - handler.logInfo("\nReport sent to this email address: " + String.join(", ", emails)); + handler.logInfo("\nReport will be sent to: " + String.join(", ", emails)); } } @@ -228,11 +246,20 @@ public void internalRun() throws Exception { } // If the user requested to see all report dates, we will display them. - if (showDates) { + if (showList) { displayReportDates(); return; } try (Context context = new Context()) { + // If only one of -s/-t is specified, warn the user and fall back to the + // last two reports for comparison. + if ((sourceReportId == null) ^ (targetReportId == null)) { + handler.logWarning("Only one of '-s'/'-t' was specified. " + + "The last two reports from the database will be compared instead."); + sourceReportId = null; + targetReportId = null; + } + defaultReportIds(context); if (!validateReportIdSelection()) { @@ -256,13 +283,10 @@ private int parseCheckOption(String checkOption) { try { int index = Integer.parseInt(checkOption); if (index < 0 || index >= HealthReport.getNumberOfChecks()) { - handler.logError("Invalid value for check. Must be between 0 and " + - (HealthReport.getNumberOfChecks() - 1) + ". Using all checks."); return -1; } return index; } catch (NumberFormatException e) { - handler.logError("Invalid value for check. It must be a NUMBER from the displayed range."); return -1; } } @@ -281,7 +305,6 @@ private Integer parseReportIdOption(String optionValue) { try { return Integer.parseInt(optionValue); } catch (Exception e) { - handler.logError("Cannot parse report ID from input: " + optionValue); return null; } } @@ -293,19 +316,19 @@ private Integer parseReportIdOption(String optionValue) { * @return true if the report IDs are valid, false otherwise */ private boolean validateReportIdSelection() { - if ((Objects.isNull(fromReportId) && Objects.nonNull(toReportId)) - || (Objects.nonNull(fromReportId) && Objects.isNull(toReportId))) { - handler.logError("Both 'from' and 'to' report IDs must be specified."); + 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(fromReportId) && fromReportId <= 0) { - handler.logError("The 'from' report ID must be a positive integer."); + if (Objects.nonNull(sourceReportId) && sourceReportId <= 0) { + handler.logError("The 'source' report ID must be a positive integer."); return false; } - if (Objects.nonNull(toReportId) && toReportId <= 0) { - handler.logError("The 'to' report ID must be a positive integer."); + if (Objects.nonNull(targetReportId) && targetReportId <= 0) { + handler.logError("The 'target' report ID must be a positive integer."); return false; } @@ -318,7 +341,7 @@ private boolean validateReportIdSelection() { * @param context the application context used for fetching reports and logging */ private void defaultReportIds(Context context) { - if (Objects.nonNull(fromReportId) && Objects.nonNull(toReportId)) { + if (Objects.nonNull(sourceReportId) && Objects.nonNull(targetReportId)) { return; } handler.logInfo("No report IDs specified, using the last two reports from the database."); @@ -332,11 +355,11 @@ private void defaultReportIds(Context context) { int size = allReports.size(); - if (Objects.isNull(toReportId) && size > 0) { - toReportId = allReports.get(size - 1).getID(); + if (Objects.isNull(targetReportId) && size > 0) { + targetReportId = allReports.get(size - 1).getID(); } - if (Objects.isNull(fromReportId) && size > 1) { - fromReportId = allReports.get(size - 2).getID(); + if (Objects.isNull(sourceReportId) && size > 1) { + sourceReportId = allReports.get(size - 2).getID(); } } catch (SQLException e) { throw new RuntimeException(e); @@ -353,8 +376,8 @@ private void displayReportDates() { try (Context context = new Context()) { context.setCurrentUser(ePersonService.find(context, getEpersonIdentifier())); List allReports = reportResultService.findAll(context); - // Determine how many reports to process, respecting the `limit` if it's within valid range - long limitCount = (limit > 0 && limit < allReports.size()) ? limit : allReports.size(); + // 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> 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 @@ -479,8 +502,8 @@ private void compareReports(Context context) { try { context.setCurrentUser(ePersonService.find(context, getEpersonIdentifier())); - ReportResult fromReport = reportResultService.find(context, fromReportId); - ReportResult toReport = reportResultService.find(context, toReportId); + ReportResult fromReport = reportResultService.find(context, sourceReportId); + ReportResult toReport = reportResultService.find(context, targetReportId); if (fromReport == null || toReport == null) { handler.logInfo("No reports found for specified report IDs."); @@ -677,30 +700,23 @@ private String generateReportComparison(ReportResult fromReport, ReportResult to // Enhanced Key Changes Table String keyChangesTable = generateEnhancedKeyChangesTable(normalizedFromJson, normalizedToJson, - fromReport.getLastModified(), toReport.getLastModified()); + fromReport.getID(), toReport.getID()); sb.append(keyChangesTable); - // Detailed Change Log - sb.append("Section 2: Detailed Change Log\n\n"); - sb.append("Changes Summary\n"); - String detailedSummary = generateDetailedSummary(normalizedFromJson, normalizedToJson); - sb.append(detailedSummary).append("\n"); - - sb.append(generateDiff(normalizedFromJson, normalizedToJson)); - - // Section 3: Skipped Checks (not present in both reports) + // Section 2: Skipped Checks (not present in both reports) if (!normalized.onlyInFrom.isEmpty() || !normalized.onlyInTo.isEmpty()) { - sb.append("\nSection 3: Skipped Checks\n\n"); - sb.append("The following checks could not be compared because they were not present in both reports.\n\n"); + sb.append("Section 2: Skipped Checks\n\n"); + sb.append("The following checks could not be compared because they were not present in " + + "both reports.\n\n"); if (!normalized.onlyInFrom.isEmpty()) { - sb.append("Only in 'From' report (").append(fromReport.getLastModified()).append("):\n"); + sb.append("Only in source report (ID ").append(fromReport.getID()).append("):\n"); for (String name : normalized.onlyInFrom) { sb.append(" - ").append(name).append("\n"); } sb.append("\n"); } if (!normalized.onlyInTo.isEmpty()) { - sb.append("Only in 'To' report (").append(toReport.getLastModified()).append("):\n"); + sb.append("Only in target report (ID ").append(toReport.getID()).append("):\n"); for (String name : normalized.onlyInTo) { sb.append(" - ").append(name).append("\n"); } @@ -708,6 +724,14 @@ private String generateReportComparison(ReportResult fromReport, ReportResult to } } + // Section 3: Detailed Change Log + sb.append("Section 3: Detailed Change Log\n\n"); + sb.append("Changes Summary\n"); + String detailedSummary = generateDetailedSummary(normalizedFromJson, normalizedToJson); + sb.append(detailedSummary).append("\n"); + + sb.append(generateDiff(normalizedFromJson, normalizedToJson)); + return sb.toString(); } @@ -855,6 +879,53 @@ private String formatBytes(long bytes) { return (bytes / (1024 * 1024 * 1024)) + " GB"; } + /** + * Format an unsigned byte count with two-decimal precision for KB/MB/GB; used for value + * columns of byte-typed fields in the Key Changes table. + * + * @param bytes byte count (negatives are treated as their absolute value) + * @return formatted string, e.g. {@code 65.32 MB} + */ + private String formatBytesHuman(long bytes) { + long abs = Math.abs(bytes); + if (abs < 1024L) { + return abs + " B"; + } + if (abs < 1024L * 1024) { + return String.format(java.util.Locale.ROOT, "%.2f KB", abs / 1024.0); + } + if (abs < 1024L * 1024 * 1024) { + return String.format(java.util.Locale.ROOT, "%.2f MB", abs / (1024.0 * 1024)); + } + return String.format(java.util.Locale.ROOT, "%.2f GB", abs / (1024.0 * 1024 * 1024)); + } + + /** + * Format a (possibly negative) byte delta into a signed, human-readable string used in the + * Difference column for byte-typed fields. Produces values such as {@code -5.71 KB} or + * {@code +123 B} so administrators don't have to read raw byte counts. + * + * @param bytes signed byte delta + * @return formatted signed string + */ + private String formatSignedBytesHuman(long bytes) { + if (bytes == 0) { + return "0 B"; + } + String sign = bytes > 0 ? "+" : "-"; + long abs = Math.abs(bytes); + if (abs < 1024L) { + return sign + abs + " B"; + } + if (abs < 1024L * 1024) { + return String.format(java.util.Locale.ROOT, "%s%.2f KB", sign, abs / 1024.0); + } + if (abs < 1024L * 1024 * 1024) { + return String.format(java.util.Locale.ROOT, "%s%.2f MB", sign, abs / (1024.0 * 1024)); + } + return String.format(java.util.Locale.ROOT, "%s%.2f GB", sign, abs / (1024.0 * 1024 * 1024)); + } + /** * Resolve a field path with attribute selectors to a JSON value. *

@@ -997,7 +1068,8 @@ private List splitPathSegments(String path) { * @throws IOException if JSON parsing fails */ private String generateEnhancedKeyChangesTable(String oldJson, String newJson, - Date fromDate, Date toDate) throws IOException { + Integer sourceReportId, Integer targetReportId) + throws IOException { loadFieldConfiguration(); JsonNode oldNode = mapper.readTree(oldJson); @@ -1017,12 +1089,27 @@ private String generateEnhancedKeyChangesTable(String oldJson, String newJson, continue; } - if (!Objects.equals(getDisplayValue(oldValue), getDisplayValue(newValue))) { - String displayName = fieldMappings.getOrDefault(fieldPath, fieldPath); - String oldDisplay = getDisplayValue(oldValue); - String newDisplay = getDisplayValue(newValue); - String difference = calculateDifference(oldValue, newValue); + // For byte-typed fields (paths ending with size_bytes) render values and diff in + // human-readable units so administrators get e.g. -5.71 KB instead of -5850. + boolean isByteField = fieldPath.endsWith("size_bytes"); + String oldDisplay; + String newDisplay; + String difference; + if (isByteField && oldValue != null && newValue != null + && oldValue.isNumber() && newValue.isNumber()) { + long oldBytes = oldValue.asLong(); + long newBytes = newValue.asLong(); + oldDisplay = formatBytesHuman(oldBytes); + newDisplay = formatBytesHuman(newBytes); + difference = formatSignedBytesHuman(newBytes - oldBytes); + } else { + oldDisplay = getDisplayValue(oldValue); + newDisplay = getDisplayValue(newValue); + difference = calculateDifference(oldValue, newValue); + } + if (!Objects.equals(oldDisplay, newDisplay)) { + String displayName = fieldMappings.getOrDefault(fieldPath, fieldPath); changes.add(new TableRow(displayName, oldDisplay, newDisplay, difference)); } } @@ -1032,16 +1119,16 @@ private String generateEnhancedKeyChangesTable(String oldJson, String newJson, "No significant changes detected between reports.\n\n"; } - // Format dates for column headers using thread-safe DateTimeFormatter - String fromDateStr = fromDate.toInstant().atZone(java.time.ZoneId.systemDefault()).format(FORMATTER); - String toDateStr = toDate.toInstant().atZone(java.time.ZoneId.systemDefault()).format(FORMATTER); + // Compact ID-only column headers; full timestamps appear in the Executive Summary above. + String fromHeader = "Source: ID " + sourceReportId; + String toHeader = "Target: ID " + targetReportId; // Calculate dynamic column widths including header content int fieldWidth = Math.max("Field".length(), changes.stream().mapToInt(r -> r.field.length()).max().orElse(25)); - int oldWidth = Math.max(fromDateStr.length(), + int oldWidth = Math.max(fromHeader.length(), changes.stream().mapToInt(r -> r.oldValue.length()).max().orElse(15)); - int newWidth = Math.max(toDateStr.length(), + int newWidth = Math.max(toHeader.length(), changes.stream().mapToInt(r -> r.newValue.length()).max().orElse(15)); int diffWidth = Math.max("Difference".length(), changes.stream().mapToInt(r -> r.difference.length()).max().orElse(12)); @@ -1056,8 +1143,8 @@ private String generateEnhancedKeyChangesTable(String oldJson, String newJson, // Header with separator table.append(separator).append("\n"); table.append("| ").append(padRight("Field", fieldWidth)) - .append(" | ").append(padRight(fromDateStr, oldWidth)) - .append(" | ").append(padRight(toDateStr, newWidth)) + .append(" | ").append(padRight(fromHeader, oldWidth)) + .append(" | ").append(padRight(toHeader, newWidth)) .append(" | ").append(padRight("Difference", diffWidth)) .append(" |\n"); table.append(separator).append("\n"); @@ -1155,10 +1242,12 @@ private String generateDetailedSummary(String oldJson, String newJson) throws IO public void printHelp() { handler.printHelp(getScriptConfiguration().getOptions(), getScriptConfiguration().getName()); handler.logInfo("This script compares two health reports and shows the differences between them."); - handler.logInfo("You can specify the 'from' and 'to' report IDs to compare specific reports."); - handler.logInfo("If you want to see all available reports with IDs, use the '-d' option."); + handler.logInfo("Use '-s/--source' and '-t/--target' with report IDs to pick the source" + + " and target report."); + handler.logInfo("Use '-l/--list' to list all available reports with their IDs and timestamps."); + handler.logInfo("Use '-m/--max' together with '--list' to limit how many entries are shown."); handler.logInfo("If you want to compare a specific check, use the '-c' option with the check index, " + - "in this case you must also specify the `from` and `to` report IDs."); + "in this case you must also specify the source and target report IDs."); handler.logInfo("If you want to send the report to a specified email address, use '-e'."); } diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java index 52001c463d5e..fafce1bc49a0 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java @@ -44,16 +44,19 @@ public Options getOptions() { HealthReport.getNumberOfChecks() - 1)); options.getOption("c").setType(String.class); - options.addOption("d", "dates", false, "Show available reports (ID, timestamp, args)"); + options.addOption("l", "list", false, + "List available reports (ID, timestamp, args). Use to find report IDs."); - options.addOption("l", "limit", true, - "Limit the number of entries (use only with -d). If omitted, all entries are shown."); - options.getOption("l").setType(String.class); + options.addOption("m", "max", true, + "Limit the number of entries (use only with -l). If omitted, all entries are shown."); + options.getOption("m").setType(String.class); - options.addOption("f", "from", true,"Source report ID (integer)."); - options.getOption("f").setType(String.class); + options.addOption("s", "source", true, + "Source report ID to compare from."); + options.getOption("s").setType(String.class); - options.addOption("t", "to", true,"Target report ID (integer)."); + options.addOption("t", "target", true, + "Target report ID to compare against."); options.getOption("t").setType(String.class); super.options = options; diff --git a/dspace-api/src/main/resources/report-diff-fields.json b/dspace-api/src/main/resources/report-diff-fields.json index 51f668f79d96..23891359a95e 100644 --- a/dspace-api/src/main/resources/report-diff-fields.json +++ b/dspace-api/src/main/resources/report-diff-fields.json @@ -1,7 +1,7 @@ { "fieldMappings": { - "/checks/[name=General Information]/report/directoryStats/0/size_bytes": "Assetstore Size (bytes)", - "/checks/[name=General Information]/report/directoryStats/1/size_bytes": "Log Directory Size (bytes)", + "/checks/[name=General Information]/report/directoryStats/0/size_bytes": "Assetstore Size", + "/checks/[name=General Information]/report/directoryStats/1/size_bytes": "Log Directory Size", "/checks/[name=Item summary]/report/communitiesCount": "Communities", "/checks/[name=Item summary]/report/collectionsCount": "Collections", "/checks/[name=Item summary]/report/collectionsSizesInfo/totalSize": "Total Content Size", diff --git a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java index 82eb61d522cd..627423046bb9 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java @@ -20,7 +20,6 @@ import java.util.regex.Pattern; import org.dspace.AbstractIntegrationTestWithDatabase; -import org.dspace.app.healthreport.HealthReport; import org.dspace.app.launcher.ScriptLauncher; import org.dspace.app.scripts.handler.impl.TestDSpaceRunnableHandler; import org.dspace.content.ReportResult; @@ -145,7 +144,7 @@ public void testShowDates() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-d" }; + String[] args = new String[] { "report-diff", "-l" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -185,7 +184,7 @@ public void testCompareReports() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -222,7 +221,7 @@ public void testCompareSpecificCheck() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); // -c 0 filters comparison to only General Information check - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()), "-c", "0" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -236,28 +235,32 @@ public void testCompareSpecificCheck() throws Exception { @Test public void testInvalidCheckIndex() throws Exception { TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", "1", "-t", "2", "-c", "999" }; + String[] args = new String[] { "report-diff", "-s", "1", "-t", "2", "-c", "999" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); - List errorMessages = handler.getErrorMessages(); - assertThat(errorMessages, hasItem("Invalid value for check. Must be between 0 and " + - (HealthReport.getNumberOfChecks() - 1) + ". Using all checks.")); + // Invalid -c is now a warning + fallback (all checks compared), not a hard error. + List warningMessages = handler.getWarningMessages(); + assertThat(warningMessages, hasItem(containsString("Invalid value for -c: '999'"))); + assertThat(warningMessages, hasItem(containsString("All checks will be compared."))); } @Test public void testInvalidReportIdFormat() throws Exception { TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", "invalid-id", "-t", "2" }; + String[] args = new String[] { "report-diff", "-s", "invalid-id", "-t", "2" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); - List errorMessages = handler.getErrorMessages(); - assertThat(errorMessages, hasItem(containsString("Cannot parse report ID from input: invalid-id"))); + // Invalid -s value is now a warning + fallback (last two reports), not a hard error. + List warningMessages = handler.getWarningMessages(); + assertThat(warningMessages, hasItem(containsString("Invalid value for -s: 'invalid-id'"))); + assertThat(warningMessages, hasItem(containsString( + "The last two reports from the database will be compared instead."))); } @Test public void testNoReportsForIds() throws Exception { TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", "999999", "-t", "999998" }; + String[] args = new String[] { "report-diff", "-s", "999999", "-t", "999998" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -267,11 +270,11 @@ public void testNoReportsForIds() throws Exception { @Test public void testNonPositiveReportId() throws Exception { TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", "-1", "-t", "1" }; + String[] args = new String[] { "report-diff", "-s", "-1", "-t", "1" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List errorMessages = handler.getErrorMessages(); - assertThat(errorMessages, hasItem(containsString("The 'from' report ID must be a positive integer."))); + assertThat(errorMessages, hasItem(containsString("The 'source' report ID must be a positive integer."))); } @Test @@ -296,7 +299,7 @@ public void testReportWithMissingValue() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -327,7 +330,7 @@ public void testNoDifferences() throws Exception { report1 = reportResultService.find(context, report1.getID()); report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -424,7 +427,7 @@ public void testShowDatesLimit() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-d", "-l", "1" }; + String[] args = new String[] { "report-diff", "-l", "-m", "1" }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -475,7 +478,7 @@ public void testProfessionalReportFormat() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -493,11 +496,11 @@ public void testProfessionalReportFormat() throws Exception { assertThat(infoMessages, hasItem(containsString("Key Changes"))); assertThat(infoMessages, hasItem(containsString("| Field"))); assertThat(infoMessages, hasItem(containsString("| Difference"))); - assertThat(infoMessages, hasItem(containsString("Assetstore Size (bytes)"))); - assertThat(infoMessages, hasItem(containsString("Log Directory Size (bytes)"))); + assertThat(infoMessages, hasItem(containsString("Assetstore Size"))); + assertThat(infoMessages, hasItem(containsString("Log Directory Size"))); // Test detailed change log section - assertThat(infoMessages, hasItem(containsString("Section 2: Detailed Change Log"))); + assertThat(infoMessages, hasItem(containsString("Section 3: Detailed Change Log"))); assertThat(infoMessages, hasItem(containsString("Changes Summary"))); assertThat(infoMessages, hasItem(containsString("Total operations:"))); assertThat(infoMessages, hasItem(containsString("Fields modified:"))); @@ -531,7 +534,7 @@ public void testReportFormatWithNoChanges() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -568,7 +571,7 @@ public void testCalculateTimePeriod() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -623,7 +626,7 @@ public void testEnhancedKeyChangesTable() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -700,7 +703,7 @@ public void testSizeDifferenceFormatting() throws Exception { String[] args = new String[] { "report-diff", - "-f", String.valueOf(fromReport.getID()), + "-s", String.valueOf(fromReport.getID()), "-t", String.valueOf(toReport.getID()) }; @@ -747,7 +750,7 @@ public void testSkippedChecksSection() throws Exception { report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -790,7 +793,7 @@ public void testCompareReportsWithMissingMappedFieldDoesNotFail() throws Excepti report2 = reportResultService.find(context, report2.getID()); TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "report-diff", "-f", String.valueOf(report1.getID()), + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()), "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); @@ -798,4 +801,185 @@ public void testCompareReportsWithMissingMappedFieldDoesNotFail() throws Excepti assertThat(handler.getInfoMessages(), hasItem(containsString("Repository Health Report Diff"))); assertThat(handler.getInfoMessages(), hasItem(containsString("Published Items"))); } + + /** + * When only -s is provided (no -t), the script should warn the user and fall back + * to comparing the last two reports in the database. + */ + @Test + public void testSourceWithoutTargetWarnsAndFallsBack() throws Exception { + context.turnOffAuthorisationSystem(); + + ReportResult report1 = reportResultService.create(context); + report1.setType("healthcheck"); + report1.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value1\"}}]}"); + reportResultService.update(context, report1); + context.commit(); + Thread.sleep(1000); + + ReportResult report2 = reportResultService.create(context); + report2.setType("healthcheck"); + report2.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value2\"}}]}"); + reportResultService.update(context, report2); + context.commit(); + context.restoreAuthSystemState(); + + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()) }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getWarningMessages(), hasItem(containsString( + "Only one of '-s'/'-t' was specified."))); + assertThat(handler.getInfoMessages(), hasItem(containsString( + "No report IDs specified, using the last two reports from the database."))); + assertThat(handler.getErrorMessages(), empty()); + } + + /** + * When only -t is provided (no -s), the script should warn the user and fall back + * to comparing the last two reports in the database. + */ + @Test + public void testTargetWithoutSourceWarnsAndFallsBack() throws Exception { + context.turnOffAuthorisationSystem(); + + ReportResult report1 = reportResultService.create(context); + report1.setType("healthcheck"); + report1.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value1\"}}]}"); + reportResultService.update(context, report1); + context.commit(); + Thread.sleep(1000); + + ReportResult report2 = reportResultService.create(context); + report2.setType("healthcheck"); + report2.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value2\"}}]}"); + reportResultService.update(context, report2); + context.commit(); + context.restoreAuthSystemState(); + + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "report-diff", "-t", String.valueOf(report2.getID()) }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getWarningMessages(), hasItem(containsString( + "Only one of '-s'/'-t' was specified."))); + assertThat(handler.getInfoMessages(), hasItem(containsString( + "No report IDs specified, using the last two reports from the database."))); + assertThat(handler.getErrorMessages(), empty()); + } + + /** + * When -s has a non-numeric value, the script should warn and fall back to the + * last two reports for comparison. + */ + @Test + public void testInvalidSourceValueWarnsAndFallsBack() throws Exception { + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "report-diff", "-s", "abc", "-t", "2" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getWarningMessages(), hasItem(containsString("Invalid value for -s: 'abc'"))); + assertThat(handler.getWarningMessages(), hasItem(containsString( + "The last two reports from the database will be compared instead."))); + // Should also fall back via the XOR branch since -s became null while -t is set. + assertThat(handler.getWarningMessages(), hasItem(containsString( + "Only one of '-s'/'-t' was specified."))); + } + + /** + * When -c is an out-of-range index, the script should warn and compare all checks + * instead of filtering to one. + */ + @Test + public void testInvalidCheckIndexWarnsAndComparesAll() throws Exception { + context.turnOffAuthorisationSystem(); + + ReportResult report1 = reportResultService.create(context); + report1.setType("healthcheck"); + report1.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{\"key\":\"v1\"}}]}"); + reportResultService.update(context, report1); + context.commit(); + Thread.sleep(1000); + + ReportResult report2 = reportResultService.create(context); + report2.setType("healthcheck"); + report2.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{\"key\":\"v2\"}}]}"); + reportResultService.update(context, report2); + context.commit(); + context.restoreAuthSystemState(); + + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "report-diff", + "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()), + "-c", "999" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getWarningMessages(), hasItem(containsString("Invalid value for -c: '999'"))); + assertThat(handler.getWarningMessages(), hasItem(containsString("All checks will be compared."))); + // Comparison still runs and produces the diff for the common check. + assertThat(handler.getInfoMessages(), hasItem(containsString("Repository Health Report Diff"))); + assertThat(handler.getErrorMessages(), empty()); + } + + /** + * When -c is a non-numeric value, the script should warn and compare all checks. + */ + @Test + public void testNonNumericCheckIndexWarnsAndComparesAll() throws Exception { + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "report-diff", "-s", "1", "-t", "2", "-c", "abc" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getWarningMessages(), hasItem(containsString("Invalid value for -c: 'abc'"))); + assertThat(handler.getWarningMessages(), hasItem(containsString("All checks will be compared."))); + } + + /** + * When -m has a non-numeric value alongside -l, the script should warn and show + * all available entries (no limit). + */ + @Test + public void testInvalidMaxValueWarnsAndShowsAll() throws Exception { + context.turnOffAuthorisationSystem(); + + ReportResult report1 = reportResultService.create(context); + report1.setType("healthcheck"); + report1.setValue("{\"checks\":[]}"); + reportResultService.update(context, report1); + context.commit(); + Thread.sleep(1000); + + ReportResult report2 = reportResultService.create(context); + report2.setType("healthcheck"); + report2.setValue("{\"checks\":[]}"); + reportResultService.update(context, report2); + context.commit(); + context.restoreAuthSystemState(); + + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "report-diff", "-l", "-m", "abc" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getWarningMessages(), hasItem(containsString("Invalid value for -m: 'abc'"))); + assertThat(handler.getWarningMessages(), hasItem(containsString("All entries will be shown."))); + // Both reports must be present in the listing since the limit was discarded. + assertThat(handler.getInfoMessages(), hasItem(containsString("ID: " + report1.getID()))); + assertThat(handler.getInfoMessages(), hasItem(containsString("ID: " + report2.getID()))); + assertThat(handler.getErrorMessages(), empty()); + } + + /** + * When -m has a non-positive value, same fallback as non-numeric: warn and show all. + */ + @Test + public void testNonPositiveMaxValueWarnsAndShowsAll() throws Exception { + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "report-diff", "-l", "-m", "0" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + assertThat(handler.getWarningMessages(), hasItem(containsString("Invalid value for -m: '0'"))); + assertThat(handler.getWarningMessages(), hasItem(containsString("All entries will be shown."))); + assertThat(handler.getErrorMessages(), empty()); + } } diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ScriptRestRepository.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ScriptRestRepository.java index 1eea06a4ee8d..c0e86e0353c3 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ScriptRestRepository.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ScriptRestRepository.java @@ -32,6 +32,7 @@ import org.dspace.core.Context; import org.dspace.scripts.DSpaceCommandLineParameter; import org.dspace.scripts.DSpaceRunnable; +import org.dspace.scripts.DSpaceRunnable.StepResult; import org.dspace.scripts.configuration.ScriptConfiguration; import org.dspace.scripts.service.ScriptService; import org.springframework.beans.factory.annotation.Autowired; @@ -151,7 +152,14 @@ private void runDSpaceScript(List files, Context context, ScriptC throws IOException, SQLException, AuthorizeException, InstantiationException, IllegalAccessException { DSpaceRunnable dSpaceRunnable = scriptService.createDSpaceRunnableForScriptConfiguration(scriptToExecute); try { - dSpaceRunnable.initialize(args.toArray(new String[0]), restDSpaceRunnableHandler, context.getCurrentUser()); + StepResult initResult = dSpaceRunnable.initialize( + args.toArray(new String[0]), restDSpaceRunnableHandler, context.getCurrentUser()); + // -h/--help returns Exit: skip run() and just mark the process started + completed. + if (initResult == StepResult.Exit) { + restDSpaceRunnableHandler.start(); + restDSpaceRunnableHandler.handleCompletion(); + return; + } if (files != null && !files.isEmpty()) { checkFileNames(dSpaceRunnable, files); processFiles(context, restDSpaceRunnableHandler, files); From 903c321240757a94ca891130da800f194e82ef51 Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Wed, 20 May 2026 16:49:33 +0200 Subject: [PATCH 14/16] Fixed checkstyle --- dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java index 73836ba86954..f291f58241e6 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java @@ -40,9 +40,9 @@ import org.dspace.content.clarin.ClarinLicenseResourceMapping; import org.dspace.content.factory.ClarinServiceFactory; import org.dspace.content.factory.ContentServiceFactory; -import org.dspace.content.service.ReportResultService; import org.dspace.content.service.BitstreamService; import org.dspace.content.service.BundleService; +import org.dspace.content.service.ReportResultService; import org.dspace.content.service.clarin.ClarinLicenseLabelService; import org.dspace.content.service.clarin.ClarinLicenseResourceMappingService; import org.dspace.content.service.clarin.ClarinLicenseService; From b34dd8ab9fdbbbc8da09be556a79d1dff2c18ddc Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Wed, 20 May 2026 18:01:07 +0200 Subject: [PATCH 15/16] Sorting reports and updating tests, plus enable multiple -c in report-diff --- .../org/dspace/app/reportdiff/ReportDiff.java | 11 +++++++ .../ReportDiffScriptConfiguration.java | 13 +++++--- .../org/dspace/scripts/HealthReportIT.java | 33 ++++++++++--------- 3 files changed, 36 insertions(+), 21 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java index af0103a10908..5c06068cf1b5 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java @@ -262,6 +262,11 @@ public void internalRun() throws Exception { defaultReportIds(context); + if (sourceReportId == null || targetReportId == null) { + handler.logInfo("Need at least 2 reports in the database to perform a comparison. Aborting."); + return; + } + if (!validateReportIdSelection()) { return; } @@ -353,6 +358,9 @@ private void defaultReportIds(Context context) { return; } + // findAll() does not guarantee ordering; sort by lastModified ascending so the + // newest reports are at the end of the list. + allReports.sort(Comparator.comparing(ReportResult::getLastModified)); int size = allReports.size(); if (Objects.isNull(targetReportId) && size > 0) { @@ -376,6 +384,9 @@ private void displayReportDates() { try (Context context = new Context()) { context.setCurrentUser(ePersonService.find(context, getEpersonIdentifier())); List allReports = reportResultService.findAll(context); + // findAll() does not guarantee ordering; sort by lastModified ascending so the + // newest reports are at the end of the list. + allReports.sort(Comparator.comparing(ReportResult::getLastModified)); // 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> reportDatesMap = new HashMap<>(); diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java index fafce1bc49a0..5067b0496a5a 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java @@ -7,6 +7,7 @@ */ package org.dspace.app.reportdiff; +import org.apache.commons.cli.Option; import org.apache.commons.cli.Options; import org.dspace.app.healthreport.HealthReport; import org.dspace.scripts.configuration.ScriptConfiguration; @@ -38,11 +39,13 @@ public Options getOptions() { options.addOption("e", "email", true, "Send report to this email address."); options.getOption("e").setType(String.class); - options.addOption("c", "check", true, - String.format("Filter comparison to a specific check by index (0 to %d). " + - "Only the specified check will be compared from both reports.", - HealthReport.getNumberOfChecks() - 1)); - options.getOption("c").setType(String.class); + 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(); + options.addOption(checkOption); options.addOption("l", "list", false, "List available reports (ID, timestamp, args). Use to find report IDs."); diff --git a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java index f291f58241e6..c260971fef09 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java @@ -255,20 +255,21 @@ public void testReportFileSaved() throws Exception { String content = Files.readString(tempFile.toPath()); assertThat("Report file must contain health report header", content, containsString("HEALTH REPORT:")); } - @Test - public void testStoredArgsContainAllCheckOptions() throws Exception { - ReportResultService reportResultService = ContentServiceFactory.getInstance().getReportResultService(); - - TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "health-report", "-c", "2", "-c", "3" }; - ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); - - context.reloadEntity(eperson); - List 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")); - } + + @Test + public void testStoredArgsContainAllCheckOptions() throws Exception { + ReportResultService reportResultService = ContentServiceFactory.getInstance().getReportResultService(); + + TestDSpaceRunnableHandler handler = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "health-report", "-c", "2", "-c", "3" }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + context.reloadEntity(eperson); + List 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")); + } } \ No newline at end of file From 929945827c0490669497d94e9965181b6c6064c6 Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Thu, 28 May 2026 16:38:33 +0200 Subject: [PATCH 16/16] Improved docs, output and info and logic --- .../dspace/app/healthreport/HealthReport.java | 17 +++-- .../HealthReportScriptConfiguration.java | 15 ++-- .../org/dspace/app/reportdiff/ReportDiff.java | 72 ++++++++++++++----- .../org/dspace/scripts/HealthReportIT.java | 8 ++- .../java/org/dspace/scripts/ReportDiffIT.java | 21 +++--- 5 files changed, 91 insertions(+), 42 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java index f18c148b3016..8d9a62c94d4a 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java @@ -166,7 +166,6 @@ public void internalRun() throws Exception { ReportInfo ri = new ReportInfo(this.forLastNDays); StringBuilder sbReport = new StringBuilder(); - sbReport.append("\n\nHEALTH REPORT:\n"); int position = -1; JSONObject root = new JSONObject(); @@ -214,9 +213,12 @@ public void internalRun() throws Exception { reportResultService.update(context, reportResult); context.commit(); + // Prepend the header with the persisted report ID so users can refer to it later + String finalReport = "\n\nHEALTH REPORT " + reportResult.getID() + ":\n" + sbReport.toString(); + // save output to file if (reportFile != null) { - InputStream inputStream = toInputStream(sbReport.toString(), StandardCharsets.UTF_8); + InputStream inputStream = toInputStream(finalReport, StandardCharsets.UTF_8); handler.writeFilestream(context, reportFile, inputStream, "export"); context.commit(); @@ -231,7 +233,7 @@ public void internalRun() throws Exception { for (String recipient : emails) { e.addRecipient(recipient); } - e.addArgument(sbReport.toString()); + e.addArgument(finalReport); e.send(); handler.logInfo("Report sent to: " + String.join(", ", emails)); } catch (IOException | MessagingException e) { @@ -241,19 +243,22 @@ public void internalRun() throws Exception { } } - handler.logInfo(sbReport.toString()); + handler.logInfo(finalReport); } } @Override public void printHelp() { + int configuredForLastNDays = configurationService.getIntProperty("healthcheck.last_n_days"); handler.logInfo("\n\nHELP\nThis process creates a health report of your DSpace.\n" + "You can choose from these available options:\n" + " -h, --help Show help information\n" + " -e, --email Send report to specified email address\n" + " -c, --check Perform specific check(s) by index (0-" + (getNumberOfChecks() - 1) + - "). Accepts multiple space-separated values, e.g. -c 0 3 4\n" + - " -f, --for Specify the last N days to consider (positive integer)\n" + + "). Repeat the flag (e.g. -c 1 -c 3) to run multiple checks. " + + "Default: All checks\n" + + " -f, --for Specify the last N days to consider (positive integer). " + + "Default: " + configuredForLastNDays + "\n" + " -r, --report Specify a file to save the report\n\n" + "Available checks:\n" + checksNamesToString() + "\n" ); diff --git a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java index 257c7e06f72f..613b18977a5d 100644 --- a/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java @@ -7,6 +7,7 @@ */ package org.dspace.app.healthreport; +import org.apache.commons.cli.Option; import org.apache.commons.cli.Options; import org.dspace.scripts.configuration.ScriptConfiguration; @@ -37,12 +38,14 @@ public Options getOptions() { options.addOption("e", "email", true, "Send report to this email address."); options.getOption("e").setType(String.class); - options.addOption("c", "check", true, - String.format("Perform specific check(s) by index (0 to %d). " + - "Accepts multiple space-separated values, e.g. -c 0 3 4.", - HealthReport.getNumberOfChecks() - 1)); - options.getOption("c").setType(String.class); - options.getOption("c").setArgs(org.apache.commons.cli.Option.UNLIMITED_VALUES); + Option checkOption = Option.builder("c").longOpt("check").hasArgs() + .desc(String.format("Perform specific check(s) by index (0 to %d). " + + "Repeat the flag (e.g. -c 1 -c 3) to run multiple checks. " + + "Default: All checks.", + HealthReport.getNumberOfChecks() - 1)) + .type(String.class) + .build(); + options.addOption(checkOption); options.addOption("f", "for", true, "Report for last N days (positive integer). Used only in general information for now."); options.getOption("f").setType(String.class); diff --git a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java index 5c06068cf1b5..2094af9e8f48 100644 --- a/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java +++ b/dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java @@ -251,15 +251,8 @@ public void internalRun() throws Exception { return; } try (Context context = new Context()) { - // If only one of -s/-t is specified, warn the user and fall back to the - // last two reports for comparison. - if ((sourceReportId == null) ^ (targetReportId == null)) { - handler.logWarning("Only one of '-s'/'-t' was specified. " - + "The last two reports from the database will be compared instead."); - sourceReportId = null; - targetReportId = null; - } - + // If only one of -s/-t was specified, fill in the missing one with the latest + // stored report (handled inside defaultReportIds). defaultReportIds(context); if (sourceReportId == null || targetReportId == null) { @@ -349,7 +342,13 @@ private void defaultReportIds(Context context) { if (Objects.nonNull(sourceReportId) && Objects.nonNull(targetReportId)) { return; } - handler.logInfo("No report IDs specified, using the last two reports from the database."); + boolean bothMissing = Objects.isNull(sourceReportId) && Objects.isNull(targetReportId); + if (bothMissing) { + handler.logInfo("No report IDs specified, using the last two reports from the database."); + } else { + handler.logInfo("Only one of '-s'/'-t' was specified; the missing one will be set to the " + + "latest report from the database."); + } try { List allReports = reportResultService.findAll(context); @@ -384,6 +383,10 @@ private void displayReportDates() { try (Context context = new Context()) { context.setCurrentUser(ePersonService.find(context, getEpersonIdentifier())); List allReports = reportResultService.findAll(context); + if (allReports == null || allReports.isEmpty()) { + handler.logInfo("No reports found in the database."); + return; + } // findAll() does not guarantee ordering; sort by lastModified ascending so the // newest reports are at the end of the list. allReports.sort(Comparator.comparing(ReportResult::getLastModified)); @@ -516,8 +519,12 @@ private void compareReports(Context context) { ReportResult fromReport = reportResultService.find(context, sourceReportId); ReportResult toReport = reportResultService.find(context, targetReportId); - if (fromReport == null || toReport == null) { - handler.logInfo("No reports found for specified report IDs."); + if (fromReport == null) { + handler.logInfo("No report found for report ID: " + sourceReportId); + return; + } + if (toReport == null) { + handler.logInfo("No report found for report ID: " + targetReportId); return; } @@ -554,13 +561,17 @@ private static class NormalizationResult { final List onlyInFrom; /** Check names that exist only in the "to" report. */ final List onlyInTo; + /** True when the two reports share at least one check eligible for comparison. */ + final boolean hasCommonChecks; NormalizationResult(String normalizedFromJson, String normalizedToJson, - List onlyInFrom, List onlyInTo) { + List onlyInFrom, List onlyInTo, + boolean hasCommonChecks) { this.normalizedFromJson = normalizedFromJson; this.normalizedToJson = normalizedToJson; this.onlyInFrom = onlyInFrom; this.onlyInTo = onlyInTo; + this.hasCommonChecks = hasCommonChecks; } } @@ -586,7 +597,7 @@ private NormalizationResult normalizeReportsToIntersection(String fromJson, Stri if (fromChecks == null || toChecks == null || !fromChecks.isArray() || !toChecks.isArray()) { return new NormalizationResult(fromJson, toJson, - new ArrayList<>(), new ArrayList<>()); + new ArrayList<>(), new ArrayList<>(), true); } // Build maps of check name -> check node for both reports @@ -660,7 +671,8 @@ private NormalizationResult normalizeReportsToIntersection(String fromJson, Stri return new NormalizationResult( mapper.writeValueAsString(normalizedFrom), mapper.writeValueAsString(normalizedTo), - onlyInFrom, onlyInTo); + onlyInFrom, onlyInTo, + !commonNames.isEmpty()); } /** @@ -709,6 +721,32 @@ private String generateReportComparison(ReportResult fromReport, ReportResult to String timePeriod = calculateTimePeriod(fromReport.getLastModified(), toReport.getLastModified()); sb.append("Report Period: ").append(timePeriod).append("\n\n"); + // When there are no checks in common between the two reports there is nothing to diff. + // In that case, only show the executive summary and the list of skipped checks so the + // user can immediately see why the comparison was not performed. + if (!normalized.hasCommonChecks) { + if (!normalized.onlyInFrom.isEmpty() || !normalized.onlyInTo.isEmpty()) { + sb.append("Section 2: Skipped Checks\n\n"); + sb.append("The following checks could not be compared because they were not present in " + + "both reports.\n\n"); + if (!normalized.onlyInFrom.isEmpty()) { + sb.append("Only in source report (ID ").append(fromReport.getID()).append("):\n"); + for (String name : normalized.onlyInFrom) { + sb.append(" - ").append(name).append("\n"); + } + sb.append("\n"); + } + if (!normalized.onlyInTo.isEmpty()) { + sb.append("Only in target report (ID ").append(toReport.getID()).append("):\n"); + for (String name : normalized.onlyInTo) { + sb.append(" - ").append(name).append("\n"); + } + sb.append("\n"); + } + } + return sb.toString(); + } + // Enhanced Key Changes Table String keyChangesTable = generateEnhancedKeyChangesTable(normalizedFromJson, normalizedToJson, fromReport.getID(), toReport.getID()); @@ -1073,8 +1111,8 @@ private List splitPathSegments(String path) { * * @param oldJson the old JSON report * @param newJson the new JSON report - * @param fromDate the date of the old report - * @param toDate the date of the new report + * @param sourceReportId the ID of the source (older) report, used in column headers + * @param targetReportId the ID of the target (newer) report, used in column headers * @return formatted table string * @throws IOException if JSON parsing fails */ diff --git a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java index c260971fef09..68142e18fcc4 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java @@ -73,7 +73,7 @@ public void testDefaultHealthcheckRun() throws Exception { List messages = testDSpaceRunnableHandler.getInfoMessages(); assertThat(messages, hasSize(1)); - assertThat(messages, hasItem(containsString("HEALTH REPORT:"))); + assertThat(messages, hasItem(containsString("HEALTH REPORT "))); } @Test @@ -174,7 +174,7 @@ public void testMultipleChecks() throws Exception { assertThat(handler.getErrorMessages(), empty()); List messages = handler.getInfoMessages(); - assertThat(messages, hasItem(containsString("HEALTH REPORT:"))); + assertThat(messages, hasItem(containsString("HEALTH REPORT "))); assertThat(messages, hasItem(containsString("General Information"))); assertThat(messages, hasItem(containsString("License summary"))); // Item summary (check index 1) should NOT be present @@ -253,7 +253,7 @@ public void testReportFileSaved() throws Exception { assertThat(handler.getErrorMessages(), empty()); assertThat("Report file must exist after -r option", tempFile.exists(), org.hamcrest.Matchers.is(true)); String content = Files.readString(tempFile.toPath()); - assertThat("Report file must contain health report header", content, containsString("HEALTH REPORT:")); + assertThat("Report file must contain health report header", content, containsString("HEALTH REPORT ")); } @Test @@ -266,6 +266,8 @@ public void testStoredArgsContainAllCheckOptions() throws Exception { context.reloadEntity(eperson); List allReports = reportResultService.findAll(context); + // findAll() does not guarantee ordering; sort by lastModified so the newest report is last. + allReports.sort(java.util.Comparator.comparing(ReportResult::getLastModified)); ReportResult latest = allReports.get(allReports.size() - 1); assertThat(handler.getErrorMessages(), empty()); diff --git a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java index 627423046bb9..1ed169db22fb 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java @@ -264,7 +264,7 @@ public void testNoReportsForIds() throws Exception { ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); - assertThat(infoMessages, hasItem(containsString("No reports found for specified report IDs."))); + assertThat(infoMessages, hasItem(containsString("No report found for report ID:"))); } @Test @@ -828,10 +828,9 @@ public void testSourceWithoutTargetWarnsAndFallsBack() throws Exception { String[] args = new String[] { "report-diff", "-s", String.valueOf(report1.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); - assertThat(handler.getWarningMessages(), hasItem(containsString( - "Only one of '-s'/'-t' was specified."))); + // When only -s is supplied, the missing -t is now auto-filled with the latest report. assertThat(handler.getInfoMessages(), hasItem(containsString( - "No report IDs specified, using the last two reports from the database."))); + "Only one of '-s'/'-t' was specified"))); assertThat(handler.getErrorMessages(), empty()); } @@ -861,10 +860,11 @@ public void testTargetWithoutSourceWarnsAndFallsBack() throws Exception { String[] args = new String[] { "report-diff", "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); - assertThat(handler.getWarningMessages(), hasItem(containsString( - "Only one of '-s'/'-t' was specified."))); + // When only -t is supplied, the missing -s is now auto-filled with the next latest + // report (instead of falling back to the last two reports). The script logs a dedicated + // info message announcing that and the comparison still runs without errors. assertThat(handler.getInfoMessages(), hasItem(containsString( - "No report IDs specified, using the last two reports from the database."))); + "Only one of '-s'/'-t' was specified"))); assertThat(handler.getErrorMessages(), empty()); } @@ -881,9 +881,10 @@ public void testInvalidSourceValueWarnsAndFallsBack() throws Exception { assertThat(handler.getWarningMessages(), hasItem(containsString("Invalid value for -s: 'abc'"))); assertThat(handler.getWarningMessages(), hasItem(containsString( "The last two reports from the database will be compared instead."))); - // Should also fall back via the XOR branch since -s became null while -t is set. - assertThat(handler.getWarningMessages(), hasItem(containsString( - "Only one of '-s'/'-t' was specified."))); + // Source becomes null after the invalid -s parse, so the missing-source branch in + // defaultReportIds now logs an info message instead of the legacy XOR warning. + assertThat(handler.getInfoMessages(), hasItem(containsString( + "Only one of '-s'/'-t' was specified"))); } /**