Ufal/Regulart reports for health check - #980
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds persistent storage and diffing for health reports: a new ReportResult JPA entity, DAO, service and wiring; health checks now produce JSON alongside text and HealthReport persists results; new ReportDiff runnable compares stored JSON reports (CLI/email) with tests, migrations and templates. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant HealthReport
participant Check
participant ReportResultService
participant DB
Admin->>HealthReport: run()
HealthReport->>Check: execute each check (collect text + JSON)
Check-->>HealthReport: return text and JSON
HealthReport->>ReportResultService: create/update ReportResult(type,args,value,executor)
ReportResultService->>DB: persist ReportResult
HealthReport-->>Admin: write file / send email
sequenceDiagram
participant User
participant ReportDiff
participant ReportResultService
participant DB
participant zjsonpatch
User->>ReportDiff: run(dates,check,email?)
ReportDiff->>ReportResultService: query reports (by date/check)
ReportResultService->>DB: fetch report rows
ReportDiff->>zjsonpatch: compute JSON Patch(oldValue,newValue)
zjsonpatch-->>ReportDiff: patch ops
ReportDiff-->>User: formatted diff (stdout/email)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java (1)
148-206:⚠️ Potential issueFix context resource management.
The
Contextobject is not properly managed - it's only completed whenfileNameis set, and there's no guarantee it will be closed if exceptions occur. Use try-with-resources or ensure proper cleanup in all paths.- Context context = new Context(); - context.setCurrentUser(ePersonService.find(context, this.getEpersonIdentifier())); - - ReportInfo ri = new ReportInfo(this.forLastNDays); - - StringBuilder sbReport = new StringBuilder(); - sbReport.append("\n\nHEALTH REPORT:\n"); - - int position = -1; - JSONObject root = new JSONObject(); - // Create the array - JSONArray checksArray = new JSONArray(); + try (Context context = new Context()) { + context.setCurrentUser(ePersonService.find(context, this.getEpersonIdentifier())); + + ReportInfo ri = new ReportInfo(this.forLastNDays); + + StringBuilder sbReport = new StringBuilder(); + sbReport.append("\n\nHEALTH REPORT:\n"); + + int position = -1; + JSONObject root = new JSONObject(); + // Create the array + JSONArray checksArray = new JSONArray();And ensure the rest of the method is wrapped in the try block, removing the manual
context.complete()call since try-with-resources will handle it.
🧹 Nitpick comments (7)
dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/postgres/V7.6_2025.06.03__Create_table_report_result.sql (1)
14-20: Bind sequence to table column.Although JPA can explicitly invoke the sequence, adding a default next-value on the column improves clarity and supports direct SQL inserts:
CREATE SEQUENCE report_result_id_seq START WITH 1 INCREMENT BY 1 NO MAXVALUE NO MINVALUE CACHE 1; + +ALTER TABLE report_result + ALTER COLUMN report_result_id + SET DEFAULT nextval('report_result_id_seq');dspace-api/src/main/java/org/dspace/health/Check.java (2)
25-26: Reset JSON state between runs.
reportJson_is initialized once and never cleared inreport(). If the sameCheckinstance is reused, previous JSON content may persist. Consider resettingreportJson_at the start ofreport(ReportInfo)to avoid stale data.
67-73: Add JavaDoc for JSON accessors.The new
getReportJson()andsetReportJson()methods lack documentation. Please add JavaDoc to clarify their purpose and the expected structure of the JSON report.dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java (1)
29-31: Consider validation for the parameterless create method.The method creates a new
ReportResult()without any initialization. Consider setting default values or validating the state before persistence.@Override public ReportResult create(Context context) throws SQLException { - return reportResultDAO.create(context, new ReportResult()); + ReportResult reportResult = new ReportResult(); + reportResult.setLastModified(new Date()); + return reportResultDAO.create(context, reportResult); }dspace-api/src/main/java/org/dspace/content/ReportResult.java (1)
57-59: Consider using @CreationTimestamp or @UpdateTimestamp.The manual timestamp initialization may not handle updates correctly. Consider using Hibernate's temporal annotations for automatic timestamp management.
+import org.hibernate.annotations.CreationTimestamp; +import org.hibernate.annotations.UpdateTimestamp; @Column(name = "last_modified", columnDefinition = "timestamp with time zone") -@Temporal(TemporalType.TIMESTAMP) -private Date lastModified = new Date(); +@UpdateTimestamp +@Temporal(TemporalType.TIMESTAMP) +private Date lastModified;dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java (1)
16-19: Remove unused imports.The imports
java.util.Arraysandjava.util.Listare not used in this file.-import java.util.Arrays; import java.util.Date; import java.util.LinkedHashMap; -import java.util.List; import java.util.Locale;dspace-api/src/main/java/org/dspace/health/InfoCheck.java (1)
33-40: Improve date formatting consistency.The code uses different date formats -
Date.toString()for the generated field andSimpleDateFormatfor the fromTill field. Consider using consistent formatting for better JSON parsing.- String generatedStr = new Date().toString(); + String generatedStr = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(new Date()); sb.append("Generated: ").append(generatedStr).append("\n"); root.put("generated", generatedStr);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java(6 hunks)dspace-api/src/main/java/org/dspace/content/ReportResult.java(1 hunks)dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java(1 hunks)dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java(1 hunks)dspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.java(1 hunks)dspace-api/src/main/java/org/dspace/content/factory/ContentServiceFactory.java(2 hunks)dspace-api/src/main/java/org/dspace/content/factory/ContentServiceFactoryImpl.java(3 hunks)dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java(1 hunks)dspace-api/src/main/java/org/dspace/health/Check.java(3 hunks)dspace-api/src/main/java/org/dspace/health/InfoCheck.java(2 hunks)dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/h2/V7.6_2025.06.03__Create_table_report_result.sql(1 hunks)dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/postgres/V7.6_2025.06.03__Create_table_report_result.sql(1 hunks)dspace/config/hibernate.cfg.xml(1 hunks)dspace/config/spring/api/core-dao-services.xml(1 hunks)dspace/config/spring/api/core-services.xml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java (1)
dspace-api/src/main/java/org/dspace/core/Context.java (1)
Context(52-1026)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java (1)
dspace-api/src/main/java/org/dspace/content/factory/ContentServiceFactory.java (1)
ContentServiceFactory(50-179)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: dspace-dependencies / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (17)
dspace/config/spring/api/core-services.xml (1)
95-95: LGTM! Properly configured service bean.The Spring bean declaration for
ReportResultServiceImplfollows the established pattern and is correctly placed among other content service implementations.dspace/config/spring/api/core-dao-services.xml (1)
58-58: LGTM! Properly configured DAO bean.The Spring bean declaration for
ReportResultDAOImplfollows the established pattern and is appropriately placed among other content DAO implementations.dspace/config/hibernate.cfg.xml (1)
82-82: LGTM! Properly configured entity mapping.The Hibernate mapping for
ReportResultfollows the established pattern and is correctly placed among other content entity mappings.dspace-api/src/main/java/org/dspace/content/factory/ContentServiceFactory.java (2)
35-35: LGTM! Import correctly placed.The import statement for
ReportResultServiceis correctly placed in alphabetical order with other service imports.
136-137:Details
✅ Verification successful
Verify implementation exists in ContentServiceFactoryImpl.
The abstract method declaration follows the established pattern and naming convention. Please ensure that the corresponding implementation is added to
ContentServiceFactoryImpl.
🏁 Script executed:
#!/bin/bash # Description: Verify that ContentServiceFactoryImpl implements the new getReportResultService method # Search for ContentServiceFactoryImpl and check if it implements getReportResultService rg -A 10 "class.*ContentServiceFactoryImpl" --type java echo "---" rg "getReportResultService" --type javaLength of output: 1999
Implementation Verified in ContentServiceFactoryImpl
The abstract method
getReportResultService()is already implemented in
- dspace-api/src/main/java/org/dspace/content/factory/ContentServiceFactoryImpl.java
No further changes are required.
dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java (1)
1-20: New DAO interface aligns with project conventions.The
ReportResultDAOcorrectly extendsGenericDAO<ReportResult>and follows the existing license header and package structure. No additional methods are required at this stage.dspace-api/src/main/java/org/dspace/content/factory/ContentServiceFactoryImpl.java (3)
32-32: Import added correctly.The
ReportResultServiceimport is placed alongside other service imports, matching the existing structure.
101-103: Field injection matches existing pattern.Injecting
ReportResultServicevia@Autowired(required = true)is consistent with other service beans in this class.
189-193: Getter exposes the new service.The
getReportResultService()method properly returns the injected service and is annotated with@Override.dspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.java (1)
1-23: DAO implementation follows established pattern.
ReportResultDAOImplcorrectly extendsAbstractHibernateDAO<ReportResult>and implementsReportResultDAOwithout additional methods. This matches other DAO implementations in the codebase.dspace-api/src/main/java/org/dspace/health/Check.java (1)
11-11: Verify JSON library dependency.The new
org.json.JSONObjectimport requires thatorg.jsonis on the classpath. Please confirm the project’s POM or build file includes the JSON library.dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java (1)
24-26: LGTM! Clean service implementation.The service implementation follows DSpace patterns correctly with proper DAO injection and delegation. The @Autowired annotation and method implementations are appropriate.
dspace-api/src/main/java/org/dspace/content/ReportResult.java (2)
50-52: LGTM! Proper lazy loading configuration.The
@ManyToOne(fetch = FetchType.LAZY)annotation with proper join column mapping follows JPA best practices for performance optimization.
39-42:Details
✅ Verification successful
Verify sequence creation in migration scripts.
The entity uses
GenerationType.SEQUENCEwithreport_result_id_seq, but the H2 migration script usesAUTO_INCREMENT. Ensure corresponding sequence creation exists for PostgreSQL.Run the following script to verify sequence creation in PostgreSQL migrations:
🏁 Script executed:
#!/bin/bash # Description: Check for sequence creation in PostgreSQL migration files fd -t f -e sql . | xargs grep -l "report_result_id_seq" | head -5Length of output: 190
🏁 Script executed:
#!/bin/bash # Inspect sequence creation in the PostgreSQL migration for report_result grep -n -C3 "report_result_id_seq" dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/postgres/V7.6_2025.06.03__Create_table_report_result.sqlLength of output: 363
PostgreSQL sequence for report_result_id_seq is present in the migration script
Verified that
report_result_id_seqis created in the PostgreSQL migration:
- File:
dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/postgres/V7.6_2025.06.03__Create_table_report_result.sql
Lines 14–17:CREATE SEQUENCE report_result_id_seq START WITH 1 INCREMENT BY 1 NO MAXVALUENo further action needed.
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java (2)
55-55: LGTM!The
reportResultServicefield is properly initialized and used for persisting health report results.
175-185:Details
❌ Incorrect review comment
Verify null safety for JSON report retrieval.
The code assumes
check.getReportJson()always returns a valid JSONObject. Consider adding null checks to prevent potential NPEs if a check doesn't properly implement JSON reporting.
🏁 Script executed:
#!/bin/bash # Description: Verify that all Check implementations properly implement getReportJson() method # Search for all classes that extend Check ast-grep --pattern 'class $_ extends Check { $$$ }' # Also search for getReportJson implementations rg -A 5 "getReportJson\(\)" --type javaLength of output: 1286
#!/bin/bash # Display the start of Check.java to inspect reportJson_ declaration and class nature sed -n '1,200p' dspace-api/src/main/java/org/dspace/health/Check.java
🏁 Script executed:
#!/bin/bash # Inspect Check.java for reportJson_ initialization and constructor behavior sed -n '1,200p' dspace-api/src/main/java/org/dspace/health/Check.javaLength of output: 1853
Ignore unnecessary null check for getReportJson()
getReportJson()always returns a non-nullJSONObject(it’s initialized in Check.java), so adding a null check here isn’t needed.
No changes required.Likely an incorrect or invalid review comment.
dspace-api/src/main/java/org/dspace/health/InfoCheck.java (1)
51-92: LGTM!The directory statistics JSON generation is well-structured with proper error handling for all edge cases.
…that `context.complete` will be called every time.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java (1)
57-59: Previous timestamp concern has been addressed.The update method now correctly sets the
lastModifiedtimestamp before saving, which addresses the concern raised in previous reviews about automatic timestamp updates.
🧹 Nitpick comments (8)
dspace/config/spring/api/scripts.xml (1)
105-105: Replace placeholder description with proper text.The description "Heeeeey" is clearly placeholder text and should be replaced with a professional description before deployment.
Apply this diff to provide a proper description:
- <property name="description" value="Heeeeey"/> + <property name="description" value="Generate a diff between the last two stored report results"/>dspace/config/spring/rest/scripts.xml (1)
88-88: Replace placeholder description with proper text.The description "Heeeeey" is clearly placeholder text and should be replaced with a professional description before deployment, consistent with the API configuration.
Apply this diff to provide a proper description:
- <property name="description" value="Heeeeey"/> + <property name="description" value="Generate a diff between the last two stored report results"/>dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java (3)
13-15: Fix incorrect class documentation.The JavaDoc comment refers to "HealthReport" but this class is for "ReportDiff" functionality.
Apply this diff to correct the documentation:
-/** - * This class represents a HealthReport that is used in the CLI. - * @author Matus Kasak (dspace at dataquest.sk) - */ +/** + * This class represents a ReportDiff script configuration that is used in the CLI. + * @author Matus Kasak (dspace at dataquest.sk) + */
19-19: Fix field naming inconsistency.The field name
dspaceRunnableclassshould follow camelCase convention to match the getter/setter method names.Apply this diff to fix the naming:
- private Class<T> dspaceRunnableclass; + private Class<T> dspaceRunnableClass;And update the getter method:
@Override public Class<T> getDspaceRunnableClass() { - return dspaceRunnableclass; + return dspaceRunnableClass; }And update the setter method:
@Override public void setDspaceRunnableClass(Class<T> dspaceRunnableClass) { - this.dspaceRunnableclass = dspaceRunnableClass; + this.dspaceRunnableClass = dspaceRunnableClass; }
37-49: Remove or implement commented CLI options.The extensive commented code should either be removed if not needed or properly implemented. Commented code in production can be confusing and may reference methods that don't exist.
If these options are not needed, remove the commented code:
-// 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).", ReportDiff.getNumberOfChecks() - 1)); -// options.getOption("c").setType(String.class); -// options.addOption("f", "for", true, -// "Report for last N days. Used only in general information for now."); -// options.getOption("f").setType(String.class); -// options.addOption("o", "output", true, -// "Save report to the file.");If they are needed, implement them properly and verify that referenced methods like
ReportDiff.getNumberOfChecks()exist.dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (3)
10-54: Remove unused importsSeveral imports are not used in the current implementation and should be removed to improve code clarity.
Remove these unused imports:
-import org.apache.commons.lang3.builder.Diff; -import org.dspace.core.Email; -import org.dspace.core.I18nUtil; -import org.dspace.health.Check; -import org.dspace.health.Report; -import org.dspace.health.ReportInfo; -import org.json.JSONArray; -import org.json.JSONObject; -import javax.mail.MessagingException; -import java.io.InputStream; -import java.nio.charset.StandardCharsets; -import java.text.SimpleDateFormat; -import java.util.ArrayList; -import java.util.Date; -import java.util.HashSet; -import java.util.Iterator; -import java.util.LinkedHashMap; -import java.util.Locale; -import java.util.Map; -import java.util.Set; -import java.util.TreeSet; -import static org.apache.commons.io.IOUtils.toInputStream;
146-146: Remove or implement commented codeThe commented line suggests command-line options should be displayed but the method doesn't exist.
Either implement
printCommandlineOptions()or remove this line:-// sbReport.append(printCommandlineOptions());
174-262: Consider adding unit tests for generateDiff methodThe
generateDiffmethod is well-structured and static, making it ideal for unit testing. Consider adding tests to verify the diff generation for various scenarios.Since this is a public static method that performs complex JSON diff operations, comprehensive unit tests would help ensure correctness and prevent regressions. Test cases should cover:
- Empty/identical JSONs
- Simple value changes
- Array modifications
- Nested object changes
- All operation types (add, remove, replace, move, copy, test)
Would you like me to generate unit test cases for this method?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
dspace-api/pom.xml(1 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java(1 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java(1 hunks)dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java(1 hunks)dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java(1 hunks)dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/postgres/V7.6_2025.06.03__Create_table_report_result.sql(1 hunks)dspace/config/spring/api/scripts.xml(1 hunks)dspace/config/spring/rest/scripts.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- dspace-api/pom.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/postgres/V7.6_2025.06.03__Create_table_report_result.sql
- dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java (1)
dspace-api/src/main/java/org/dspace/scripts/configuration/ScriptConfiguration.java (1)
ScriptConfiguration(28-126)
dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java (1)
dspace-api/src/main/java/org/dspace/core/Context.java (1)
Context(52-1026)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
- GitHub Check: dspace-dependencies / docker-build (linux/amd64, ubuntu-latest, true)
🔇 Additional comments (1)
dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java (1)
26-61: Well-structured service implementation.The service implementation follows DSpace conventions correctly:
- Proper dependency injection of DAO
- Standard CRUD operations with Context parameter
- Appropriate delegation to DAO methods
- Explicit timestamp management in update method
The implementation is consistent with other DSpace service classes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java (1)
240-253: Handle null values in command line optionsBoolean flags like
-ireturn null fromgetOptionValue(), resulting in "null" being printed.Apply the previously suggested fix:
private String printCommandlineOptions() { // Return key-value pairs of options StringBuilder options = new StringBuilder(); for (Option option : commandLine.getOptions()) { String key = option.getOpt(); String value = commandLine.getOptionValue(key); - options.append(String.format(" -%s: %s\n", key, value)); + if (value != null) { + options.append(String.format(" -%s: %s\n", key, value)); + } else { + options.append(String.format(" -%s\n", key)); + } } return options.toString(); }dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java (1)
65-70: Previous review comment has been properly addressed.The update method now correctly sets the
lastModifiedtimestamp before persisting the entity, which resolves the concern raised in the previous review.
🧹 Nitpick comments (3)
dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java (1)
24-26: Add Javadoc for interface methodsDocument the purpose and parameters of these methods for better API clarity.
+ /** + * Find a ReportResult by its last modified date. + * + * @param context the DSpace context + * @param lastModified the exact last modified date to search for + * @return the ReportResult with the given last modified date, or null if not found + * @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;dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java (1)
54-58: Add missing Javadoc for public methodsThese public interface methods lack documentation.
+ /** + * Find all ReportResult instances. + * + * @param context the DSpace context + * @return list of all ReportResult instances + * @throws SQLException if a database error occurs + */ List<ReportResult> findAll(Context context) throws SQLException; + /** + * Find a ReportResult by its last modified date. + * + * @param context the DSpace context + * @param lastModified the exact last modified date to search for + * @return the ReportResult with the given date, or null if not found + * @throws SQLException if a database error occurs + */ 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;dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (1)
396-412: Consider making DateWithArgs a static inner classFor better code organization, consider making this a private static inner class of
ReportDiffsince it's only used internally.Move this class inside
ReportDiff:-} - -class DateWithArgs { + private static class DateWithArgs { private final String date; private final String args; public DateWithArgs(String date, String args) { this.date = date; this.args = args; } public String getDate() { return date; } public String getArgs() { return args; } } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java(6 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java(1 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java(1 hunks)dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java(1 hunks)dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java(1 hunks)dspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.java(1 hunks)dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java
🧰 Additional context used
🧬 Code Graph Analysis (4)
dspace-api/src/main/java/org/dspace/content/dao/ReportResultDAO.java (1)
dspace-api/src/main/java/org/dspace/core/Context.java (1)
Context(52-1026)
dspace-api/src/main/java/org/dspace/content/service/ReportResultService.java (1)
dspace-api/src/main/java/org/dspace/core/Context.java (1)
Context(52-1026)
dspace-api/src/main/java/org/dspace/content/dao/impl/ReportResultDAOImpl.java (1)
dspace-api/src/main/java/org/dspace/core/Context.java (1)
Context(52-1026)
dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java (1)
dspace-api/src/main/java/org/dspace/core/Context.java (1)
Context(52-1026)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: dspace-dependencies / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (3)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java (1)
146-147: Good context management with try-finallyThe refactoring to use a single context with proper cleanup in a finally block is a solid improvement for resource management.
Also applies to: 222-224
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (1)
228-229: Verify DAO implementation fix for checkType queriesThese method calls depend on
findByLastModifiedAndCheckTypewhich has an implementation issue with the LIKE query. Ensure the DAO is fixed before using this functionality.Once the DAO implementation is corrected, verify that the check type filtering works as expected by testing with actual report data.
dspace-api/src/main/java/org/dspace/content/ReportResultServiceImpl.java (1)
31-33:Details
✅ Verification successful
Verify that the default ReportResult constructor initializes all required fields.
The method creates an empty ReportResult instance. Ensure that the entity's default constructor properly initializes all required fields to avoid potential null pointer exceptions or constraint violations.
🏁 Script executed:
#!/bin/bash # Description: Check ReportResult entity for required fields and default constructor initialization # Find and examine the ReportResult entity class fd -e java "ReportResult.java" -x cat {} | grep -A 20 "class ReportResult" # Look for @NotNull or @Column(nullable = false) annotations fd -e java "ReportResult.java" -x grep -B 2 -A 2 "@NotNull\|nullable.*=.*false" {} # Check for constructor definitions fd -e java "ReportResult.java" -x grep -A 10 "public ReportResult()"Length of output: 1011
No required fields in ReportResult; default constructor is sufficient
The
ReportResultentity declares no@NotNullornullable = falseconstraints, and all columns (type, value, executor, args) default to nullable. Callingnew ReportResult()without additional initialization is safe and won’t lead to constraint violations. No changes are needed here.
* JSONificate ItemCheck * CodeRabbit's optimisation * CodeRabbits context closing to prevent potential issue * CodeRabbits hint to jsonificate int values as ints not strings * Revert "CodeRabbits context closing to prevent potential issue" This reverts commit 630e12f. * Refactor to use stringbuilder instead of string and added keywords static final
* JSONificate UserCheck
* Maven checkstyle error fix - Wrong lexicographical order
* Encapsulated the case formatting into method
* Refactoring - name of method, removing last delimeter, optimization of replacing spaces
* Added spacing before {
* refactored formatIds method to not return a value, when doing multiple key operations
* fixed formatIds method and refactored from string to StringBuilder
* added javadoc comment for formatIds method
* JSONificate LicenseCheck * Refactoring edit * readability fix - ids are array of strings, not other objects * Renamed IDs to UUIDs for consistency
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
dspace-api/src/main/java/org/dspace/health/UserCheck.java (1)
78-79: Fix incorrect logic for "Self registered" counter.The condition checks for the presence of a netid and increments the "Self registered" counter, which appears to be backwards. Typically, self-registered users would NOT have a netid, as netids are usually assigned by institutions.
- if (e.getNetid() != null && e.getNetid().length() > 0) { + if (e.getSelfRegistered()) { info.put("Self registered", info.get("Self registered") + 1); }If
getSelfRegistered()method is not available, the logic should be:- if (e.getNetid() != null && e.getNetid().length() > 0) { + if (e.getNetid() == null || e.getNetid().length() == 0) { info.put("Self registered", info.get("Self registered") + 1); }
🧹 Nitpick comments (1)
dspace-api/src/main/java/org/dspace/health/LicenseCheck.java (1)
115-125: Consider consolidating text and JSON generation for better maintainability.The license data processing logic is correct, but the code now processes the same data twice - once for text output and once for JSON output. This creates potential maintenance issues if the logic needs to be updated.
Consider refactoring to build both text and JSON outputs in a single loop:
+JSONArray licensesArray = new JSONArray(); for (Map.Entry<String, Integer> result : licensesCount.entrySet()) { + JSONObject oneLicense = new JSONObject(); + sb.append(String.format("%-20s: %d\n", result.getKey(), result.getValue())); + oneLicense.put("type", result.getKey()); + oneLicense.put("count", result.getValue()); + + licensesArray.put(oneLicense); } +root.put("licenses", licensesArray);This would eliminate the duplicate iteration and keep related logic together.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dspace-api/src/main/java/org/dspace/health/LicenseCheck.java(3 hunks)dspace-api/src/main/java/org/dspace/health/UserCheck.java(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dspace-dependencies / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests
🔇 Additional comments (11)
dspace-api/src/main/java/org/dspace/health/LicenseCheck.java (4)
31-32: LGTM! JSON imports are correctly added.The JSON library imports are appropriate for the new JSON output functionality.
50-50: LGTM! JSON root object initialization is correct.The JSON root object is properly initialized at the beginning of the method.
128-144: LGTM! Problem items JSON generation is well-structured.The JSON generation for problem items correctly mirrors the text output structure, with proper handling of:
- Problem item types and counts
- UUID arrays for each problem type
- Proper JSON object nesting
The code maintains consistency with the existing text output format.
148-148: LGTM! JSON report is properly set.The JSON report is correctly set using the inherited method, completing the dual output functionality.
dspace-api/src/main/java/org/dspace/health/UserCheck.java (7)
15-15: Good addition of JSON support.The imports for JSON handling and CaseFormat utility are appropriate for the dual reporting functionality being added.
Also applies to: 26-27
38-39: Good use of constants to avoid magic strings.Extracting frequently used strings into constants improves maintainability and reduces the risk of typos.
44-45: Clean dual reporting approach.The addition of
JSONObjectalongsideStringBuildermaintains backward compatibility while enabling structured data output.
97-98: Good camelCase conversion for JSON consistency.The conversion to camelCase ensures consistent JSON key formatting, which is important for API consumers.
108-116: Well-structured JSON array building.The parallel construction of JSON arrays and text output maintains consistency between the two reporting formats.
175-179: Good utility method for camelCase conversion.The method correctly handles the conversion from human-readable strings to camelCase JSON keys using Guava's CaseFormat utility.
142-143: Proper integration with the reporting framework.The call to
setReportJson(root)properly integrates the JSON report with the framework before returning the text report.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java (6)
73-73: Replace Thread.sleep with more reliable synchronization.This Thread.sleep usage was flagged in previous reviews and makes tests potentially flaky. Consider mocking timestamp generation or using explicit timestamp setting if the service supports it.
102-102: Replace Thread.sleep with more reliable synchronization.Same issue as previous comment - Thread.sleep makes tests flaky and should be replaced with more reliable timestamp differentiation.
208-208: Replace Thread.sleep with more reliable synchronization.Another instance of the Thread.sleep issue flagged in previous reviews.
235-235: Replace Thread.sleep with more reliable synchronization.Same Thread.sleep issue as previous comments.
262-262: Replace Thread.sleep with more reliable synchronization.Another Thread.sleep instance that should be addressed per previous review comments.
291-291: Replace Thread.sleep with more reliable synchronization.Final Thread.sleep instance that needs to be addressed for test reliability.
🧹 Nitpick comments (1)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (1)
200-225: Consider improving exception handling and single report scenario.Two concerns:
- When there's only one report,
fromremains null, which may cause issues downstream- Wrapping SQLException in RuntimeException loses the checked exception semantics and may not be the best approach
Consider handling the single report case explicitly:
if (Objects.isNull(from) && size > 1) { from = allReports.get(size - 2).getLastModified(); +} else if (Objects.isNull(from) && size == 1) { + handler.logInfo("Only one report found. Cannot compare without at least two reports."); + return; }Also consider whether SQLException should be handled differently rather than wrapped in RuntimeException.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java(1 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java(1 hunks)dspace-api/src/test/data/dspaceFolder/config/local.cfg(1 hunks)dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java(1 hunks)scripts/fast-build/update-solr-configsets.bat(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- dspace-api/src/test/data/dspaceFolder/config/local.cfg
- scripts/fast-build/update-solr-configsets.bat
🚧 Files skipped from review as they are similar to previous changes (1)
- dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java
🔇 Additional comments (9)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (7)
1-76: LGTM! Clean class structure and proper initialization.The class structure follows DSpace patterns well with proper service injection, static ObjectMapper for JSON processing efficiency, and clear field declarations for command-line options.
78-108: LGTM! Well-structured setup method.The setup method properly handles command-line parsing with appropriate early returns and delegation to helper methods for validation.
110-134: LGTM! Clean execution flow with proper resource management.The method uses appropriate early returns for different execution paths and proper try-with-resources for Context management.
137-193: LGTM! Well-designed helper methods with proper validation.The parsing and validation methods handle edge cases appropriately, provide clear error messages, and return sensible defaults on failure.
227-265: LGTM! Well-structured method for displaying report dates.The method efficiently groups reports by type, formats dates consistently, and applies sensible limits (20 reports) with proper sorting.
267-323: LGTM! Solid report comparison logic with proper null handling.The methods correctly handle the conditional logic for specific checks and provide appropriate null checking with clear user feedback.
325-521: LGTM! Comprehensive JSON diff implementation with proper help documentation.The JSON diff generation is thorough, handling all patch operations correctly. The help method provides clear usage information, and the helper classes are well-designed.
dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java (2)
1-52: LGTM! Proper test class structure and setup.The test class properly extends the database integration test base and includes a helpful date formatting utility for consistent test assertions.
154-197: LGTM! Comprehensive error handling test coverage.These tests properly verify various error scenarios including invalid inputs, missing data, and validation failures with appropriate assertion checks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java (4)
20-20: Fix the typo in field name.The field name
dspaceRunnableclassshould follow camelCase convention.Apply this fix:
- private Class<T> dspaceRunnableclass; + private Class<T> dspaceRunnableClass;
24-24: Update getter to use corrected field name.The getter method references the misspelled field name.
Apply this fix:
- return dspaceRunnableclass; + return dspaceRunnableClass;
29-29: Update setter to use corrected field name.The setter method references the misspelled field name.
Apply this fix:
- this.dspaceRunnableclass = dspaceRunnableClass; + this.dspaceRunnableClass = dspaceRunnableClass;
14-17: Correct the class documentation.The class comment incorrectly describes this as representing a HealthReport when it's actually a configuration class for ReportDiff script.
Apply this fix:
/** - * This class represents a HealthReport that is used in the CLI. + * Configuration class for the ReportDiff script that provides CLI options for comparing health reports. * @author Matus Kasak (dspace at dataquest.sk) */dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (1)
89-89: Consider making the DateTimeFormatter static and final.The formatter is used throughout the class and should be shared across instances.
Apply this fix:
- DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSS"); + private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSS");Then update all references from
formattertoFORMATTERthroughout the class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java(1 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java(1 hunks)dspace/config/emails/report_diff(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- dspace/config/emails/report_diff
🔇 Additional comments (15)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java (1)
33-57: LGTM! The CLI options are well-structured.The option definitions correctly support the ReportDiff functionality with appropriate help text, type constraints, and dynamic validation using
HealthReport.getNumberOfChecks().dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (14)
51-58: LGTM! Well-structured class with appropriate dependencies.The class correctly extends
DSpaceRunnable, uses proper dependency injection pattern for services, and follows DSpace conventions.
99-127: LGTM! The setup method properly handles CLI options.The method correctly parses and validates all command-line options with appropriate error handling.
129-153: LGTM! Clear execution flow with proper resource management.The main execution logic follows a clear flow with proper Context resource management using try-with-resources.
163-176: LGTM! Robust input validation for check option.The method properly validates the check index against the available range and handles parsing errors gracefully.
186-197: LGTM! Proper date parsing with error handling.The date parsing method correctly handles the expected format and provides clear error messages for invalid input.
206-212: LGTM! Logical date range validation.The validation correctly ensures the 'to' date is not before the 'from' date.
219-244: LGTM! Smart default date handling.The method intelligently sets defaults using the last two reports when dates aren't specified, with proper error handling for database operations.
252-284: LGTM! Well-structured report date display.The method efficiently groups reports by type, sorts dates, limits output to the most recent 20, and formats the output clearly.
293-330: LGTM! Comprehensive report comparison with email support.The method handles both specific check and general report comparison scenarios, includes proper error handling, and successfully integrates email functionality.
341-358: LGTM! Clear report comparison output format.The method generates a well-formatted comparison report with all necessary metadata and proper null handling.
361-369: LGTM! Comprehensive help documentation.The help method now provides clear, informative guidance on script usage, addressing the previous review comment.
381-422: LGTM! Robust JSON diff implementation.The method correctly uses the zjsonpatch library, handles empty diffs, and provides comprehensive coverage of all JSON Patch operations.
432-534: LGTM! Well-structured helper methods for diff operations.All the helper methods (
appendReplace,appendAdd,appendRemove, etc.) follow consistent formatting patterns and properly handle JSON node operations with appropriate escaping.
541-557: LGTM! Simple and effective helper class.The
DateWithArgsclass serves its purpose well as a data container with clear, immutable design.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (2)
114-119: Improve error handling in check option validationWhen
parseCheckOptionreturns -1 (indicating an error), the method continues execution instead of returning early. This could lead to unexpected behavior sincespecificCheckis set to -1 but the script continues.Consider returning early when validation fails:
if (commandLine.hasOption('c')) { specificCheck = parseCheckOption(commandLine.getOptionValue('c')); if (specificCheck == -1) { + // Error already logged in parseCheckOption return; } }
122-124: Remove redundant assignmentThe
showDatesvariable is being set tocommandLine.hasOption('d')when we're already inside a condition that checkscommandLine.hasOption('d'), making this assignment alwaystrue.Apply this fix:
if (commandLine.hasOption('d')) { - showDates = commandLine.hasOption('d'); + showDates = true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java(6 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java(1 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java(1 hunks)dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java
- dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java
🔇 Additional comments (7)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (3)
382-390: Well-implemented help methodThe help method provides comprehensive and useful information about the script's functionality and usage patterns. This addresses the previous review comment about placeholder help text.
402-443: Excellent JSON diff implementationThe
generateDiffmethod provides a clean, human-readable representation of JSON changes using the RFC 6902 JSON Patch standard. The implementation properly handles all patch operations and provides clear formatting.
562-578: Simple and effective helper classThe
DateWithArgsclass provides a clean way to associate report dates with their arguments for display purposes. The implementation is straightforward and appropriate.dspace-api/src/test/java/org/dspace/scripts/ReportDiffIT.java (4)
64-91: Good test data management approachUsing hard-coded timestamps (1000, 2000) instead of Thread.sleep provides more reliable and faster tests while ensuring different timestamps for comparison scenarios.
122-153: Comprehensive specific check testingThe test properly verifies that when a specific check is requested, only that check's differences are included in the output while other checks are excluded.
254-279: Good default behavior testingThis test verifies that when no dates are specified, the system correctly uses the last two reports from the database, which is important default functionality.
312-339: Effective limit functionality testingThe test properly verifies that the limit parameter works correctly by showing only the most recent report when limit is set to 1, excluding the older report.
There was a problem hiding this comment.
Pull Request Overview
This PR implements health report diff functionality for DSpace, enabling comparison between stored health reports and email notifications. The system now persists health report results with metadata and provides tools to compare reports and identify changes over time.
Key changes:
- Enhanced health report persistence: Health reports now generate JSON output and are automatically stored in a new database table with executor and command-line metadata
- Report comparison tool: New
report-diffscript provides diff functionality with flexible filtering by date range and specific checks - Database schema: Added
report_resulttable to store historical health report data and results
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/fast-build/update-solr-configsets.bat | Updated batch script to use Windows-native commands instead of Unix commands |
| dspace/config/spring/*.xml | Registered new report-diff script configuration and services |
| dspace/config/hibernate.cfg.xml | Added ReportResult entity mapping |
| dspace/config/emails/report_diff | Added email template for report diff notifications |
| SQL migration files | Created report_result table schema for PostgreSQL and H2 |
| Health check classes | Enhanced to generate structured JSON output alongside text reports |
| ReportDiff* classes | New script implementation for comparing stored health reports |
| ReportResult* classes | New entity, service, and DAO layers for managing report results |
| HealthReport.java | Enhanced to persist results and generate JSON output |
Comments suppressed due to low confidence (1)
scripts/fast-build/update-solr-configsets.bat:3
- The variable reference is missing a percent sign. 'dspace_solr%server' should be '%dspace_solr%server' to properly reference the environment variable.
:: Remove old Solr config sets
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (2)
126-133: Handle limit parsing outside the showDates condition.The limit parsing is only executed when the 'd' option is present, but the limit could be used in other contexts. Move the limit parsing outside the showDates condition.
Apply this fix to handle limit parsing independently:
- // `-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")); - } - } catch (NumberFormatException e) { - handler.logError("Invalid value for -l. Must be a valid number."); - return; - } - } + // `-d`: Dates, show all dates that the report was generated for a specific check type. + if (commandLine.hasOption('d')) { + showDates = true; + } + + // `-l`: Limits the number of report entries (dates) displayed. + if (commandLine.hasOption('l')) { + try { + limit = Long.parseLong(commandLine.getOptionValue("l")); + } catch (NumberFormatException e) { + handler.logError("Invalid value for -l. Must be a valid number."); + return; + } + }
181-194: Consider throwing exceptions for better error handling.Currently, the method logs errors and returns -1, which requires calling code to check for errors. This could lead to missed error handling.
Consider replacing the return-based error handling with exceptions:
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; + throw new IllegalArgumentException("Invalid value for check. Must be between 0 and " + + (HealthReport.getNumberOfChecks() - 1) + "."); } return index; } catch (NumberFormatException e) { - handler.logError("Invalid value for check. It must be a NUMBER from the displayed range."); - return -1; + throw new IllegalArgumentException("Invalid value for check. It must be a NUMBER from the displayed range.", e); } }Then update the calling code in setup() to catch the exception and handle it appropriately.
🧹 Nitpick comments (2)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (2)
245-264: Add null safety check for allReports.The code checks if
allReportsis null or empty but still proceeds to access elements without additional safety checks in the size logic.Apply this fix to improve robustness:
try { List<ReportResult> allReports = reportResultService.findAll(context); if (allReports == null || allReports.isEmpty()) { handler.logInfo("No reports found in the database."); return; } + + if (allReports.size() < 2) { + handler.logInfo("Need at least 2 reports for comparison. Found: " + allReports.size()); + return; + } int size = allReports.size(); - if (Objects.isNull(to) && size > 0) { + if (Objects.isNull(to)) { to = allReports.get(size - 1).getLastModified(); } - if (Objects.isNull(from) && size > 1) { + if (Objects.isNull(from)) { from = allReports.get(size - 2).getLastModified(); }
565-581: Consider making DateWithArgs an inner class or record.The
DateWithArgsclass is only used withinReportDiffand could be simplified.Consider these alternatives:
Option 1: Make it a static inner class:
+ /** + * A simple class to hold a date and its associated arguments. + * Used for displaying report dates with their arguments. + */ + static class DateWithArgs { + private final String date; + private final String args; + + public DateWithArgs(String date, String args) { + this.date = date; + this.args = args; + } + + public String getDate() { + return date; + } + + public String getArgs() { + return args; + } + } } - -/** - * A simple class to hold a date and its associated arguments. - * Used for displaying report dates with their arguments. - */ -class DateWithArgs { - private final String date; - private final String args; - - public DateWithArgs(String date, String args) { - this.date = date; - this.args = args; - } - - public String getDate() { - return date; - } - - public String getArgs() { - return args; - } -}Option 2: If using Java 14+, convert to a record:
static record DateWithArgs(String date, String args) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java(1 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiffScriptConfiguration.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (7)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java (1)
HealthReport(49-295)dspace-api/src/main/java/org/dspace/content/factory/ContentServiceFactory.java (1)
ContentServiceFactory(50-179)dspace-api/src/main/java/org/dspace/core/Context.java (1)
Context(52-1026)dspace-api/src/main/java/org/dspace/core/Email.java (1)
dspace-api/src/main/java/org/dspace/core/I18nUtil.java (1)
I18nUtil(37-416)dspace-api/src/main/java/org/dspace/eperson/factory/EPersonServiceFactory.java (1)
EPersonServiceFactory(23-39)dspace-api/src/main/java/org/dspace/scripts/DSpaceRunnable.java (1)
DSpaceRunnable(33-211)
🔇 Additional comments (4)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (4)
384-393: LGTM! Excellent help documentation.The help method now provides comprehensive and meaningful usage information, addressing previous feedback about placeholder text.
405-446: LGTM! Well-designed JSON diff implementation.The
generateDiffmethod is well-structured with proper error handling, clear formatting, and comprehensive coverage of JSON Patch operations. The use of helper methods for each operation type improves maintainability.
456-558: LGTM! Clean helper method implementations.All the helper methods for JSON patch operations are well-implemented with consistent formatting and appropriate use of
nodeToEscapedStringfor safe output.
278-288: Fix potential IndexOutOfBoundsException in date display.The code calculates
limitCountbut then uses it in a way that could cause an IndexOutOfBoundsException when reversing the index.Apply this fix to prevent the exception:
// 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(); + int totalReports = allReports.size(); + long limitCount = (limit > 0 && limit < totalReports) ? limit : totalReports; Map<String, List<DateWithArgs>> reportDatesMap = new HashMap<>(); - for (long i = 0; i < limitCount; i++) { + for (int i = 0; i < limitCount; i++) { // the newest report is at the end of the list, so we reverse the index - ReportResult report = allReports.get(allReports.size() - 1 - (int) i); + ReportResult report = allReports.get(totalReports - 1 - i);Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (6)
177-194: Improve error handling consistency with method contractThe method documentation states it returns -1 on invalid input, but the error logging suggests "Using all checks" which is misleading since the method doesn't actually fall back to using all checks.
The error handling approach was previously discussed. Consider updating the error message to be more accurate:
- handler.logError("Invalid value for check. Must be between 0 and " + - (HealthReport.getNumberOfChecks() - 1) + ". Using all checks."); + handler.logError("Invalid value for check. Must be between 0 and " + + (HealthReport.getNumberOfChecks() - 1) + ".");
346-348: Use consistent logging mechanism for error handlingThe email sending error uses
handler.logError()which is good, but this was previously flagged as inconsistent. This appears to have been addressed correctly.The logging consistency issue from previous reviews has been properly addressed here.
386-394: Help documentation has been properly implementedThe help method now provides meaningful usage information instead of the placeholder text that was previously flagged.
The help documentation issue from previous reviews has been properly resolved with comprehensive usage instructions.
56-56: Move service initialization to setup() method for proper DSpace contextThe ReportResultService is being initialized as a field which can execute before the DSpace context is ready. This follows an anti-pattern that should be corrected.
This issue was already flagged in past reviews but appears to remain unaddressed. Move the service initialization to the
setup()method:- private ReportResultService reportResultService; + private ReportResultService reportResultService;And ensure Line 107 properly initializes it in
setup().
225-233: Fix incorrect validation logic and error messageThe validation logic has two issues: it incorrectly requires both dates when using specific checks, and the error message doesn't match the actual validation being performed.
This validation logic was flagged in previous reviews. The current logic forces both dates to be specified even when they should be optional:
private boolean validateDateRange() { if (to != null && from != null && to.before(from)) { handler.logError("The 'to' date cannot be before the 'from' date."); return false; - } else if (Objects.isNull(from) || Objects.isNull(to)) { - handler.logError("Both 'from' and 'to' dates must be specified when using a specific check."); - return false; + } else if (specificCheck != -1 && (Objects.isNull(from) || Objects.isNull(to))) { + handler.logError("Both 'from' and 'to' dates must be specified when using a specific check."); + return false; } return true; }
246-265: Add bounds checking for array access in defaultDate methodThe method accesses list elements by index without verifying the list has sufficient elements, which could cause IndexOutOfBoundsException.
This issue was previously identified but appears unresolved. Add proper bounds checking:
try { List<ReportResult> allReports = reportResultService.findAll(context); if (allReports == null || allReports.isEmpty()) { handler.logInfo("No reports found in the database."); return; } + + if (allReports.size() < 2) { + handler.logError("Need at least 2 reports to compare. Found: " + allReports.size()); + return; + } int size = allReports.size();
🧹 Nitpick comments (2)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (2)
275-309: Potential performance issue with reverse indexing in loopThe current implementation calculates
allReports.size() - 1 - (int) iin each iteration which is inefficient. Consider using a more direct approach.Optimize the loop by iterating backwards or pre-calculating the indices:
- for (long i = 0; i < limitCount; i++) { - // the newest report is at the end of the list, so we reverse the index - ReportResult report = allReports.get(allReports.size() - 1 - (int) i); + for (int i = allReports.size() - 1; i >= allReports.size() - limitCount; i--) { + ReportResult report = allReports.get(i);
566-582: Consider making DateWithArgs a static nested classThe
DateWithArgsclass is only used withinReportDiffand could benefit from being a static nested class for better encapsulation and clearer relationship.Move the class inside
ReportDiffas a static nested class:+ /** + * A simple class to hold a date and its associated arguments. + * Used for displaying report dates with their arguments. + */ + static class DateWithArgs { + private final String date; + private final String args; + + public DateWithArgs(String date, String args) { + this.date = date; + this.args = args; + } + + public String getDate() { + return date; + } + + public String getArgs() { + return args; + } + } } -/** - * A simple class to hold a date and its associated arguments. - * Used for displaying report dates with their arguments. - */ -class DateWithArgs { - private final String date; - private final String args; - - public DateWithArgs(String date, String args) { - this.date = date; - this.args = args; - } - - public String getDate() { - return date; - } - - public String getArgs() { - return args; - } -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dspace-api/pom.xml(0 hunks)dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java(1 hunks)dspace-api/src/test/data/dspaceFolder/config/local.cfg(1 hunks)
💤 Files with no reviewable changes (1)
- dspace-api/pom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- dspace-api/src/test/data/dspaceFolder/config/local.cfg
🧰 Additional context used
🧬 Code Graph Analysis (1)
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java (6)
dspace-api/src/main/java/org/dspace/app/healthreport/HealthReport.java (1)
HealthReport(49-295)dspace-api/src/main/java/org/dspace/content/factory/ContentServiceFactory.java (1)
ContentServiceFactory(50-179)dspace-api/src/main/java/org/dspace/core/Email.java (1)
dspace-api/src/main/java/org/dspace/core/I18nUtil.java (1)
I18nUtil(37-416)dspace-api/src/main/java/org/dspace/eperson/factory/EPersonServiceFactory.java (1)
EPersonServiceFactory(23-39)dspace-api/src/main/java/org/dspace/scripts/DSpaceRunnable.java (1)
DSpaceRunnable(33-211)
🪛 GitHub Actions: Build
dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java
[error] 28-28: Step failed: mvn --no-transfer-progress -V install -P-assembly -Pcoverage-report $TEST_FLAGS. Compilation error: package com.flipkart.zjsonpatch does not exist.
[error] 410-410: Step failed: mvn --no-transfer-progress -V install -P-assembly -Pcoverage-report $TEST_FLAGS. Compilation error: cannot find symbol 'JsonDiff'. Ensure the zjsonpatch library is on the classpath.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
|
|
||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.flipkart.zjsonpatch.JsonDiff; |
There was a problem hiding this comment.
Critical compilation error: Missing dependency for zjsonpatch library
The import com.flipkart.zjsonpatch.JsonDiff is causing a compilation failure as the zjsonpatch library is not available on the classpath. This dependency needs to be added to the Maven configuration.
The zjsonpatch library dependency is missing from the build configuration. Add the following dependency to the appropriate pom.xml file:
<dependency>
<groupId>com.flipkart.zjsonpatch</groupId>
<artifactId>zjsonpatch</artifactId>
<version>0.4.14</version>
</dependency>🧰 Tools
🪛 GitHub Actions: Build
[error] 28-28: Step failed: mvn --no-transfer-progress -V install -P-assembly -Pcoverage-report $TEST_FLAGS. Compilation error: package com.flipkart.zjsonpatch does not exist.
🤖 Prompt for AI Agents
In dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java around
line 28, the import com.flipkart.zjsonpatch.JsonDiff fails because the
zjsonpatch library is not on the classpath; add the zjsonpatch dependency to the
project Maven configuration by updating the module's pom.xml (or the parent pom
if dependencies are managed centrally) to include the
com.flipkart.zjsonpatch:zjsonpatch dependency with a compatible version (e.g.
0.4.14), then run mvn clean compile to verify the import resolves.
| JsonNode oldNode = mapper.readTree(oldJson); | ||
| JsonNode newNode = mapper.readTree(newJson); | ||
|
|
||
| JsonNode patch = JsonDiff.asJson(oldNode, newNode); |
There was a problem hiding this comment.
Critical compilation error: JsonDiff symbol not found
The JsonDiff.asJson() call is failing to compile due to the missing zjsonpatch dependency, as indicated in the pipeline failure.
This is the same dependency issue as the import. Once the zjsonpatch library is added to the Maven dependencies, this compilation error will be resolved.
🧰 Tools
🪛 GitHub Actions: Build
[error] 410-410: Step failed: mvn --no-transfer-progress -V install -P-assembly -Pcoverage-report $TEST_FLAGS. Compilation error: cannot find symbol 'JsonDiff'. Ensure the zjsonpatch library is on the classpath.
🤖 Prompt for AI Agents
In dspace-api/src/main/java/org/dspace/app/reportdiff/ReportDiff.java around
line 410, the call to JsonDiff.asJson(...) fails to compile because the
zjsonpatch library dependency is missing; add the zjsonpatch Maven dependency
(e.g., groupId com.flipkart.zjsonpatch, artifactId zjsonpatch, and an
appropriate version) to the module's pom.xml, run mvn clean install (or mvn -U
clean package) to refresh dependencies and verify compilation, and ensure any
necessary import for JsonDiff remains or is added after the dependency is
available.
292e6d0 to
380640f
Compare
|
New PR because of the new version of DSpace > #1039 |
Problem description
Summary by CodeRabbit
New Features
Bug Fixes
Chores