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..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 @@ -13,10 +13,14 @@ 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.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; @@ -59,9 +63,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 +73,10 @@ public class HealthReport extends DSpaceRunnable specificChecks = new ArrayList<>(); /** * `-f`: For, specify the last N days to consider. @@ -80,9 +85,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; } @@ -149,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(); @@ -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; } @@ -197,10 +213,14 @@ 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 (fileName != null) { - InputStream inputStream = toInputStream(sbReport.toString(), StandardCharsets.UTF_8); - handler.writeFilestream(context, fileName, inputStream, "export"); + if (reportFile != null) { + InputStream inputStream = toInputStream(finalReport, StandardCharsets.UTF_8); + handler.writeFilestream(context, reportFile, inputStream, "export"); + context.commit(); context.restoreAuthSystemState(); @@ -213,27 +233,34 @@ 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) { log.error("Error sending email:", e); + handler.logError("Error sending email to " + String.join(", ", emails) + + ": " + e.getMessage()); } } - handler.logInfo(sbReport.toString()); + handler.logInfo(finalReport); } } @Override public void printHelp() { - handler.logInfo("\n\nINFORMATION\nThis process creates a health report of your DSpace.\n" + + 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" + - " -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) + + "). 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" ); } @@ -242,13 +269,21 @@ 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(); + Set processedOptions = new LinkedHashSet<>(); + 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)); + if (key == null || processedOptions.contains(key)) { + continue; + } + processedOptions.add(key); + + String[] values = commandLine.getOptionValues(key); + 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/healthreport/HealthReportScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/healthreport/HealthReportScriptConfiguration.java index 771cc70aadb9..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; @@ -32,20 +33,24 @@ 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)); - options.getOption("c").setType(String.class); + 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. 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..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 @@ -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; @@ -57,39 +58,40 @@ 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(); /** - * `-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. + * `-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 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. + * `-s`: Source, specify source report ID. */ - private Date from = null; + private Integer sourceReportId = null; /** - * `-t`: Till, specify the end date for the report. + * `-t`: Till, specify target report ID. */ - private Date to = null; + private Integer targetReportId = null; /** * `-e`: Email, send report to specified email address. @@ -101,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; @@ -135,6 +139,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.", @@ -157,65 +166,101 @@ public ReportDiffScriptConfiguration getScriptConfiguration() { @Override 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; } - // `-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) { + 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 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')); + // `-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`: 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)); } } @Override public void internalRun() throws Exception { // If the user requested help information, we will display it. - if (info) { + if (help) { printHelp(); return; } // If the user requested to see all report dates, we will display them. - if (showDates) { + if (showList) { displayReportDates(); return; } try (Context context = new Context()) { - defaultDate(context); + // 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) { + handler.logInfo("Need at least 2 reports in the database to perform a comparison. Aborting."); + return; + } - // If the user specified a specific check, we need to ensure that both `from` and `to` dates are set. - if (!validateDateRange()) { + if (!validateReportIdSelection()) { return; } @@ -236,66 +281,74 @@ 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; } } /** - * 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); return null; } } /** - * 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 report ID selection for comparison. + * If one ID is set, both must be set. * - * @return true if the date range is valid, 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(sourceReportId) && Objects.nonNull(targetReportId)) + || (Objects.nonNull(sourceReportId) && Objects.isNull(targetReportId))) { + handler.logError("Both 'source' and 'target' report IDs must be specified."); + return false; + } + + if (Objects.nonNull(sourceReportId) && sourceReportId <= 0) { + handler.logError("The 'source' report ID must be a positive integer."); 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.nonNull(targetReportId) && targetReportId <= 0) { + handler.logError("The 'target' 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(sourceReportId) && Objects.nonNull(targetReportId)) { return; } - handler.logInfo("No dates specified, using the last two dates 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); @@ -304,13 +357,16 @@ private void defaultDate(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(to) && size > 0) { - to = allReports.get(size - 1).getLastModified(); + if (Objects.isNull(targetReportId) && size > 0) { + targetReportId = allReports.get(size - 1).getID(); } - if (Objects.isNull(from) && size > 1) { - from = allReports.get(size - 2).getLastModified(); + if (Objects.isNull(sourceReportId) && size > 1) { + sourceReportId = allReports.get(size - 2).getID(); } } catch (SQLException e) { throw new RuntimeException(e); @@ -318,17 +374,24 @@ 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()) { 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(); + 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)); + // 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 @@ -337,10 +400,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() @@ -348,9 +411,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")); }); @@ -360,10 +425,90 @@ 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. * 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,16 +516,15 @@ 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.find(context, sourceReportId); + ReportResult toReport = reportResultService.find(context, targetReportId); - if (fromReport == null || toReport == null) { - handler.logInfo("No reports found for specified dates."); + 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; } @@ -406,9 +550,136 @@ 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; + /** 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, + boolean hasCommonChecks) { + this.normalizedFromJson = normalizedFromJson; + this.normalizedToJson = normalizedToJson; + this.onlyInFrom = onlyInFrom; + this.onlyInTo = onlyInTo; + this.hasCommonChecks = hasCommonChecks; + } + } + + /** + * 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<>(), true); + } + + // 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 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()) { + 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()); + + // 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(); + 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, + !commonNames.isEmpty()); + } + /** * 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 +694,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 @@ -436,25 +712,74 @@ 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()); 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(fromJson, toJson, - fromReport.getLastModified(), toReport.getLastModified()); + String keyChangesTable = generateEnhancedKeyChangesTable(normalizedFromJson, normalizedToJson, + fromReport.getID(), toReport.getID()); sb.append(keyChangesTable); - // Detailed Change Log - sb.append("Section 2: Detailed Change Log\n\n"); + // Section 2: Skipped Checks (not present in both reports) + 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"); + } + } + + // Section 3: Detailed Change Log + sb.append("Section 3: 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)); return sb.toString(); } @@ -513,6 +838,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(); @@ -589,18 +928,197 @@ 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. + *

+ * 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"]}. + * + *

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 + */ + 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 - * @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 */ 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); @@ -610,15 +1128,37 @@ 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); - 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)); } } @@ -628,16 +1168,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)); @@ -652,8 +1192,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"); @@ -672,43 +1212,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. */ @@ -788,10 +1291,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' dates to compare reports from specific dates."); - handler.logInfo("If you want to see all available report dates, 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` dates."); + "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'."); } @@ -966,14 +1471,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 da1933590bb2..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; @@ -33,26 +34,32 @@ 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)); - 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("d", "dates", false, "Show all report dates"); + 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,"Report from specific date [YYYY-MM-DD HH:mm:ss.SSS]."); - 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,"Report to specific date [YYYY-MM-DD HH:mm:ss.SSS]."); + 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/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. diff --git a/dspace-api/src/main/resources/report-diff-fields.json b/dspace-api/src/main/resources/report-diff-fields.json index a1e1f687aefc..23891359a95e 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", + "/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", + "/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/HealthReportIT.java b/dspace-api/src/test/java/org/dspace/scripts/HealthReportIT.java index e77a907ef731..68142e18fcc4 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; @@ -31,6 +34,7 @@ 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; @@ -38,6 +42,7 @@ import org.dspace.content.factory.ContentServiceFactory; 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; @@ -68,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 @@ -139,4 +144,134 @@ 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 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 { + 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. + */ + @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. + */ + @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. + * 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. + * 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 ")); + } + + @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); + // 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()); + 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 ce3e8b168be8..1ed169db22fb 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; @@ -111,7 +110,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(); @@ -126,6 +125,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 +136,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(); @@ -143,14 +144,20 @@ 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(); - 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 +184,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", "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -193,9 +200,8 @@ public void testCompareSpecificCheck() throws Exception { ReportResult report1 = reportResultService.create(context); report1.setType("healthcheck"); - report1.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value1\"}},{\"name\":\"Check2\"" + - ",\"report\":{\"key\":\"other\"}}]}"); - report1.setArgs("-c: 0"); + 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(); @@ -205,9 +211,8 @@ public void testCompareSpecificCheck() throws Exception { ReportResult report2 = reportResultService.create(context); report2.setType("healthcheck"); - report2.setValue("{\"checks\":[{\"name\":\"Check1\",\"report\":{\"key\":\"value2\"}},{\"name\":\"Check2\"" + - ",\"report\":{\"key\":\"other\"}}]}"); - report2.setArgs("-c: 0"); + report2.setValue("{\"checks\":[{\"name\":\"General Information\",\"report\":{\"key\":\"value2\"}}," + + "{\"name\":\"Item summary\",\"report\":{\"key\":\"other\"}}]}"); reportResultService.update(context, report2); context.commit(); context.restoreAuthSystemState(); @@ -215,8 +220,9 @@ public void testCompareSpecificCheck() 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()), "-c", "0" }; + // -c 0 filters comparison to only General Information check + 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); List infoMessages = handler.getInfoMessages(); @@ -229,45 +235,46 @@ 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", "-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 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", "-s", "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"))); + // 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 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", "-s", "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 report found for report ID:"))); } @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", "-s", "-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 'source' report ID must be a positive integer."))); } @Test @@ -292,8 +299,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", "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -323,8 +330,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", "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -358,8 +365,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 @@ -370,7 +377,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 +389,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(); @@ -422,24 +427,24 @@ 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(); - 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 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 +461,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," + @@ -473,8 +478,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", "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -491,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:"))); @@ -529,8 +534,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", "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -566,8 +571,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", "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -584,10 +589,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 +609,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," + @@ -621,8 +626,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", "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); List infoMessages = handler.getInfoMessages(); @@ -662,12 +667,16 @@ 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); @@ -694,19 +703,284 @@ public void testSizeDifferenceFormatting() throws Exception { String[] args = new String[] { "report-diff", - "-f", formatDate(fromReport.getLastModified()), - "-t", formatDate(toReport.getLastModified()) + "-s", 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)); } + + @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", "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; + 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)); + } + + @Test + public void testCompareReportsWithMissingMappedFieldDoesNotFail() throws Exception { + context.turnOffAuthorisationSystem(); + + ReportResult report1 = reportResultService.create(context); + report1.setType("healthcheck"); + report1.setValue("{\"checks\":[{\"name\":\"Item summary\",\"report\":{}}]}"); + reportResultService.update(context, report1); + context.commit(); + + Thread.sleep(1000); + + ReportResult report2 = reportResultService.create(context); + report2.setType("healthcheck"); + report2.setValue("{\"checks\":[{\"name\":\"Item summary\",\"report\":{" + + "\"publishedItems\":2" + + "}}]}"); + 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", "-s", String.valueOf(report1.getID()), + "-t", String.valueOf(report2.getID()) }; + ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), handler, kernelImpl); + + 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"))); + } + + /** + * 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); + + // When only -s is supplied, the missing -t is now auto-filled with the latest report. + assertThat(handler.getInfoMessages(), hasItem(containsString( + "Only one of '-s'/'-t' was specified"))); + 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); + + // 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( + "Only one of '-s'/'-t' was specified"))); + 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."))); + // 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"))); + } + + /** + * 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); 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