Feature 13771 epipulse measles export #13785
Conversation
📝 WalkthroughWalkthroughAdds Measles (MEAS) EpiPulse export: new MEAS fields and CSV helpers in DTOs, laboratory/enum mappings, SQL CTE builder, configuration lookup, orchestrator and strategy framework, Measles/Pertussis export and CSV strategies, and wiring in service/facade/timer to start MEAS exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Facade as EpipulseDiseaseExportFacadeEjb
participant Orchestrator as EpipulseCsvExportOrchestrator
participant ConfigSvc as EpipulseConfigurationLookupService
participant Service as EpipulseDiseaseExportService
participant Strategy as AbstractEpipulseDiseaseExportStrategy
participant SqlBuilder as EpipulseSqlCteBuilder
participant DB as Database/EntityManager
participant Mapper as EpipulseCommonDtoMapper
participant CsvStrat as CsvExportStrategy
participant File as FileWriter
Caller->>Facade: startMeaslesExport(uuid)
Facade->>Orchestrator: orchestrateExport(uuid, exportFn, measlesStrategy)
Orchestrator->>ConfigSvc: lookupConfiguration(exportDto, country, subject)
ConfigSvc-->>Orchestrator: EpipulseConfigurationContext
Orchestrator->>Service: claim & load exportDto
Service->>Strategy: export(exportDto, serverLocale, serverName)
Strategy->>SqlBuilder: buildDiseaseExportQuery()
SqlBuilder-->>Strategy: SQL with CTEs
Strategy->>DB: execute SQL
DB-->>Strategy: result set
Strategy->>Mapper: map common fields
Strategy->>Strategy: map measles-specific fields & calc max counts
Strategy-->>Service: EpipulseDiseaseExportResult
Service-->>Orchestrator: result
Orchestrator->>CsvStrat: buildColumnNames(exportResult)
CsvStrat-->>Orchestrator: headers
Orchestrator->>File: write header
loop per DTO
Orchestrator->>CsvStrat: writeEntryRow(dto, line, exportResult)
CsvStrat-->>Orchestrator: populated line
Orchestrator->>File: write row
end
Orchestrator->>File: close & finalize
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportEntryDto.java (1)
50-50: Thread-safety issue:SimpleDateFormatis not thread-safe.
SimpleDateFormatis mutable and not thread-safe. As a static field used byformatDateForCsv(), concurrent invocations in a multi-threaded environment (e.g., parallel export processing) can cause corrupted date strings or parsing errors.🔒 Recommended fix: Use thread-safe DateTimeFormatter
- private static final SimpleDateFormat DB_DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd"); + private static final java.time.format.DateTimeFormatter DB_DATE_FORMAT = + java.time.format.DateTimeFormatter.ofPattern("yyyy-MM-dd"); public static String formatDateForCsv(Date date) { - if (date != null) { - return DB_DATE_FORMAT.format(date); + return DB_DATE_FORMAT.format(date.toInstant().atZone(java.time.ZoneId.systemDefault()).toLocalDate()); } else { return ""; } }Alternatively, create a new instance per call or use
ThreadLocal<SimpleDateFormat>.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseExportTimerEjb.java (2)
33-33: Comment is incorrect: 60000ms is 60 seconds, not 5 seconds.The comment states "5 seconds" but the constant value
60000milliseconds equals 60 seconds (1 minute).📝 Fix the comment
- private static final long EXPORT_DELAY_MS = 60000; // 5 seconds + private static final long EXPORT_DELAY_MS = 60000; // 60 seconds (1 minute)
77-78: Malformed error logging: placeholders are not substituted.The
{}placeholders are not being replaced with actual values because they're part of a string that's being concatenated withe.getMessage()rather than passed as SLF4J arguments. Additionally, the exception stack trace is lost.🔧 Fix the logging statement
} catch (Exception e) { - logger.error("Error during scheduled export for UUID: {} of type: {}: " + e.getMessage()); + logger.error("Error during scheduled export for UUID: {} of type: {}", uuid, subjectCodeStr, e); } finally {sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportService.java (1)
49-49: Remove unusedDB_DATE_FORMATfield.The
DB_DATE_FORMATfield at line 49 is no longer used after the export logic was moved to the strategy classes. All date handling in the remaining methods uses eitherDateHelper.convertDateToDbFormat()or the database'snow()function. Remove this field and the now-unusedSimpleDateFormatimport at line 20.
🤖 Fix all issues with AI agents
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCommonDtoMapper.java:
- Around line 175-185: The loop in EpipulseCommonDtoMapper that computes
maxPathogenTests and maxImmunizations calls entry.getPathogenTests().size() and
entry.getImmunizations().size() directly and can throw NPE if those getters
return null; fix by null-checking the collections (or treating null as empty)
before taking size, e.g. compute int pathogenTestCount =
(entry.getPathogenTests() != null) ? entry.getPathogenTests().size() : 0 and
similarly for immunizationCount, keeping the rest of the max update logic for
maxPathogenTests and maxImmunizations unchanged.
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseConfigurationLookupService.java:
- Around line 110-127: The method lookupServerCountryNutsCode can NPE when
countryName is null; add a null guard at the start of
lookupServerCountryNutsCode (e.g., if (countryName == null) return null or throw
IllegalArgumentException) before calling countryName.toLowerCase(), and ensure
the subsequent em.createNativeQuery(...).setParameter("countryName", ...) is
only invoked with a non-null value so the query and result handling remain
unchanged.
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java:
- Around line 133-137: The loop reuses the String[] exportLine, causing stale
values to leak when writeEntryRow writes fewer columns than a previous row; fix
by creating a fresh String[] exportLine = new String[columnNames.size()] inside
the for-loop (for each EpipulseDiseaseExportEntryDto in
exportResult.getExportEntryList()) or, alternatively, ensure
csvStrategy.writeEntryRow clears/unsets all indices up to columnNames.size()
before writing; update usage around csvStrategy.writeEntryRow(...) and
writer.writeNext(...) so each written row has no leftover data.
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportFacadeEjb.java:
- Around line 41-47: The EpipulseDiseaseExportFacadeEjb implementation adds
startMeaslesExport(String uuid) but the EpipulseDiseaseExportFacade interface
lacks that declaration; update the interface (EpipulseDiseaseExportFacade) to
declare public void startMeaslesExport(String uuid); so the interface matches
the implementation (similar to the existing startPertussisExport(String uuid))
and recompile to ensure no contract mismatch.
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/AbstractEpipulseDiseaseExportStrategy.java:
- Around line 117-120: The catch block in AbstractEpipulseDiseaseExportStrategy
currently logs only e.getMessage() so the stack trace is lost; update the
logger.error call inside the catch(Exception e) in that class/method to pass the
exception object as the last parameter (e.g., logger.error("Error while
exporting case based " + exportDto.getSubjectCode() + ":", e)) or use
parameterized logging to include both the message and the Throwable so the full
stack trace is recorded.
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java:
- Around line 86-90: The loop in MeaslesExportStrategy uses
SampleMaterial.valueOf(specimenType.trim()) which will throw
IllegalArgumentException on unexpected DB values; add a safe parser helper
(e.g., safeParseSampleMaterial or parseSampleMaterialOrNull) modeled after
parseSymptomState that catches IllegalArgumentException and returns null or
Optional, then use it in the loops (both the specimenTypesVirusRaw block and the
similar block at 106-110) to skip or log invalid values before calling
EpipulseLaboratoryMapper.mapSampleMaterialToEpipulseCode so exports don't fail
on bad enum strings.
- Around line 92-96: The enum parsing in MeaslesExportStrategy is brittle and
can throw on corrupt data; add a reusable safe-parse helper (e.g., a private
static <E extends Enum<E>> Optional<E> safeValueOf(Class<E> enumClass, String
name) or a method returning null) that catches
IllegalArgumentException/NullPointerException and returns empty/ null, then
replace direct calls to PathogenTestResultType.valueOf(...),
ClusterType.valueOf(...), and CaseImportedStatus.valueOf(...) (and subsequent
uses like EpipulseLaboratoryMapper.mapTestResultToEpipulseCode(...) and
dto.setResultOfVirusDetection(...)) with calls to the helper and only map/set
when a value is present; apply this change consistently for the occurrences
around the existing parsing sites in MeaslesExportStrategy.
🧹 Nitpick comments (7)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseSqlCteBuilder.java (1)
176-192: Minor formatting inconsistency in CTE indentation.
buildImmunizationsCte()starts with"case_all_immunizations"(no leading spaces), while other CTEs likebuildPreviousHospitalizationsCte()use" case_all_prev_hsp_from_latest"with leading spaces. This affects SQL readability when debugging but not functionality.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseConfigurationLookupService.java (2)
41-41: Remove unused logger or add debug logging.The
loggerfield is declared but never used in this class. Consider either removing it or adding appropriate debug/trace logging for the configuration lookups.♻️ Suggested fix: Remove unused logger
- private final Logger logger = LoggerFactory.getLogger(getClass()); -
138-159: Consider including the disease name in the error message for better debugging.The error message "Subject code is empty" doesn't indicate which disease failed the lookup, making debugging harder.
♻️ Suggested improvement
if (StringUtils.isBlank(subjectCode)) { - throw new IllegalStateException("Subject code is empty"); + throw new IllegalStateException("Subject code lookup failed for disease: " + subjectCodeEnum.name()); }sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCommonDtoMapper.java (2)
42-42: Unused logger field.The
loggerfield is declared but never used in this utility class. Consider removing it to avoid dead code.♻️ Suggested fix
- private static final Logger logger = LoggerFactory.getLogger(EpipulseCommonDtoMapper.class);
69-157: The index-based mapping approach is fragile.Using pre-increment (
++index) for sequential column access is error-prone and tightly couples this code to SQL column ordering. Any change in SQL column order will silently cause data corruption. Consider using named column access or defining constants for column indices to improve maintainability.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (2)
93-94: Use parameterized logging instead of string concatenation.String concatenation in log statements evaluates the expression even when the log level is disabled, which wastes CPU cycles and can be a performance concern in high-volume scenarios.
♻️ Suggested fix
- logger.error("EpipulseExport with uuid " + uuid + " not found"); + logger.error("EpipulseExport with uuid {} not found", uuid); return;- logger.error("EpipulseExport with uuid " + uuid + " is not in status PENDING"); + logger.error("EpipulseExport with uuid {} is not in status PENDING", uuid); return;- logger.error("Error during export with uuid " + uuid + ": " + e.getMessage(), e); + logger.error("Error during export with uuid {}: {}", uuid, e.getMessage(), e);Also applies to: 98-99, 142-142
122-124: Consider using try-with-resources for the CSVWriter.While the finally block correctly closes the writer, using try-with-resources would be more idiomatic and ensure proper resource cleanup even if an exception occurs during construction.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportEntryDto.javasormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportResult.javasormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.javasormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseSubjectCode.javasormas-api/src/main/java/de/symeda/sormas/api/epipulse/referencevalue/EpipulseDiseaseRef.javasormas-api/src/main/resources/enum.propertiessormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCommonDtoMapper.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseConfigurationLookupService.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportFacadeEjb.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportService.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseExportTimerEjb.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseSqlCteBuilder.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/AbstractEpipulseDiseaseExportStrategy.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/CsvExportStrategy.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesCsvExportStrategy.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/PertussisCsvExportStrategy.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/PertussisExportStrategy.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/util/EpipulseConfigurationContext.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-24T12:06:40.329Z
Learnt from: roldy
Repo: SORMAS-Foundation/SORMAS-Project PR: 13784
File: sormas-api/src/main/resources/enum_fr-FR.properties:375-385
Timestamp: 2025-12-24T12:06:40.329Z
Learning: In SORMAS, locale-specific enum property files like enum_en.properties can be empty (containing only headers) because they use enum.properties as a fallback base file. This is the intended architecture for localization in the sormas-api/src/main/resources/ directory.
Applied to files:
sormas-api/src/main/resources/enum.properties
📚 Learning: 2025-12-24T12:07:19.832Z
Learnt from: roldy
Repo: SORMAS-Foundation/SORMAS-Project PR: 13784
File: sormas-api/src/main/resources/enum_haw-US.properties:287-297
Timestamp: 2025-12-24T12:07:19.832Z
Learning: In SORMAS, enum_en.properties is intentionally empty (contains only license header) and uses enum.properties as the fallback base file for English locale. This is a standard Java ResourceBundle pattern and should not be flagged as missing translations.
Applied to files:
sormas-api/src/main/resources/enum.properties
🧬 Code graph analysis (5)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesCsvExportStrategy.java (1)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportResult.java (1)
EpipulseDiseaseExportResult(20-96)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (9)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseConfigurationLookupService.java (1)
Stateless(37-160)sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportService.java (1)
Stateless(43-147)sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseSqlCteBuilder.java (1)
Stateless(26-306)sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesCsvExportStrategy.java (1)
Stateless(36-212)sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java (1)
Stateless(42-394)sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/PertussisCsvExportStrategy.java (1)
Stateless(31-114)sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/PertussisExportStrategy.java (1)
Stateless(30-65)sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportEntryDto.java (1)
EpipulseDiseaseExportEntryDto(44-1046)sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportResult.java (1)
EpipulseDiseaseExportResult(20-96)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/CsvExportStrategy.java (2)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportEntryDto.java (1)
EpipulseDiseaseExportEntryDto(44-1046)sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportResult.java (1)
EpipulseDiseaseExportResult(20-96)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCommonDtoMapper.java (2)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportEntryDto.java (1)
EpipulseDiseaseExportEntryDto(44-1046)sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportResult.java (1)
EpipulseDiseaseExportResult(20-96)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/PertussisExportStrategy.java (1)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportResult.java (1)
EpipulseDiseaseExportResult(20-96)
⏰ 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). (4)
- GitHub Check: SORMAS CI
- GitHub Check: android app test (27)
- GitHub Check: android app test (26)
- GitHub Check: android app test (28)
🔇 Additional comments (22)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportResult.java (1)
26-31: LGTM!The new max count fields for MEAS repeatable fields are well-structured and follow the existing pattern. The naming is clear and consistent with related fields in the export strategies.
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java (1)
252-277: LGTM!The symptom-to-complication-code mapping is well-implemented with proper null safety and clear documentation. The note about PNEU not being available is helpful for future maintainers.
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseSqlCteBuilder.java (1)
74-105: LGTM!The conditional inclusion of Measles-specific fields via the
includeMeaslesFieldsparameter is a clean approach for supporting multiple disease exports with shared query logic.sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportEntryDto.java (2)
450-460: LGTM!The MEAS case correctly shares the same age-month logic with PERT. Using case fall-through for shared behavior is appropriate here.
778-794: LGTM!Good implementation of the complication diagnosis CSV getter with the special handling to output "NONE" for the first slot when no complications are present, matching EpiPulse requirements.
sormas-api/src/main/resources/enum.properties (1)
2862-2867: LGTM!The enum property entries are correctly formatted and follow the established naming conventions. The localization setup with
enum.propertiesas the fallback base file is properly leveraged.sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseSubjectCode.java (1)
26-27: LGTM!The MEAS enum constant is correctly configured with the same pattern as PERT, linking to
Disease.MEASLESfor disease-based export functionality.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseExportTimerEjb.java (1)
70-72: LGTM!The MEAS case correctly follows the established pattern from PERT, delegating to the appropriate facade method for Measles export processing.
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/referencevalue/EpipulseDiseaseRef.java (1)
23-24: LGTM!The new
MEASenum constant follows the established pattern fromPERTand correctly associates withEpipulseSubjectCode.MEAS. The existinggetBySubjectCodemethod will properly handle the new constant.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/util/EpipulseConfigurationContext.java (1)
25-59: LGTM!Well-designed immutable DTO with proper
Serializableimplementation. The final fields and lack of setters ensure immutability. The Javadoc documentation is comprehensive. The nullableserverCountryNutsCodeis consistent with the lookup service behavior.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/CsvExportStrategy.java (1)
27-46: LGTM!Clean strategy interface that properly separates concerns. The
buildColumnNamesmethod enables dynamic header generation, andwriteEntryRowreturning the final index allows the caller to validate array population. The Javadoc clearly documents the contract.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/PertussisCsvExportStrategy.java (2)
36-72: LGTM!The column header construction is well-organized with 17 fixed columns, followed by dynamic
PathogenDetectionMethodcolumns based onmaxPathogenTests, conditionalDateOfLastVaccination, and finallyVaccinationStatus. The logic correctly handles edge cases where max values are 0.
74-113: LGTM!The row writing logic correctly mirrors the column header structure. The
++indexpattern ensures proper array indexing, and the conditional blocks for repeatable fields match the header generation logic. Returning the final index enables the orchestrator to validate complete row population.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesCsvExportStrategy.java (2)
40-117: LGTM!The header construction correctly handles the complex Measles CSV structure with 19 fixed columns, 5 types of repeatable fields, and vaccination columns. The ordering and conditional logic for repeatable fields are consistent with the EpiPulse specification.
119-211: LGTM!The row writing logic correctly mirrors the header structure. All 19 initial fixed columns, 5 repeatable field sections, and vaccination columns are written in the exact order defined by
buildColumnNames. The implementation handles all max value conditions consistently.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseConfigurationLookupService.java (1)
61-69: LGTM!The
lookupConfigurationmethod cleanly orchestrates the three configuration lookups and returns an immutable context object. The exception declarations in the method signature properly document the failure modes.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/PertussisExportStrategy.java (1)
30-65: LGTM!Clean implementation of the Template Method pattern for Pertussis exports. The no-op methods are appropriately documented, and the strategy correctly uses only common CTEs without disease-specific extensions.
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/AbstractEpipulseDiseaseExportStrategy.java (1)
55-66: LGTM - Well-designed abstract strategy with proper dependency injection.The Template Method pattern is correctly implemented with clear separation of common logic (configuration lookup, query execution, mapping) and disease-specific hooks (query building, field mapping, max count calculation). The Javadoc clearly documents the intended usage pattern.
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportService.java (1)
54-68: LGTM - Clean delegation to strategy pattern.The refactoring correctly delegates export logic to the injected strategy beans while preserving the public API. This aligns well with the new strategy-based architecture introduced in this PR.
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java (3)
213-337: SQL CTE construction is well-structured.The Measles-specific CTEs are logically organized and correctly aggregate data for laboratory, serology, cluster, exposure, and complication information. The use of STRING_AGG, COALESCE, and subqueries with LIMIT 1 follows appropriate patterns for this type of data extraction.
383-393: Good defensive parsing implementation.The
parseSymptomStatemethod correctly handles invalid enum values by catchingIllegalArgumentExceptionand logging a warning. This pattern should be applied consistently to other enum parsing in this class.
178-209: LGTM - Max count calculation with null safety.The
calculateDiseaseSpecificMaxCountsmethod correctly checks for null before accessing list sizes, preventing NPE. This is the correct pattern that should also be applied toEpipulseCommonDtoMapper.calculateCommonMaxCounts().
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java
Outdated
Show resolved
Hide resolved
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCommonDtoMapper.java
Show resolved
Hide resolved
...kend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseConfigurationLookupService.java
Show resolved
Hide resolved
...s-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java
Outdated
Show resolved
Hide resolved
...-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportFacadeEjb.java
Show resolved
Hide resolved
...n/java/de/symeda/sormas/backend/epipulse/strategy/AbstractEpipulseDiseaseExportStrategy.java
Show resolved
Hide resolved
...-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java
Outdated
Show resolved
Hide resolved
...-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java
Show resolved
Hide resolved
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13785 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
@sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java:
- Around line 34-74: The Javadoc says the method may return null for unmappable
values but the switch default returns "OTH"; change the default branch in
mapSampleMaterialToEpipulseCode(SampleMaterial) to return null instead of "OTH"
so behavior matches the Javadoc (keep the explicit OTHER case mapping to "OTH"
as-is).
- Around line 295-305: The deriveClinicalCriteriaStatus method currently
collapses UNKNOWN (and null) into false; change it to handle null and
YesNoUnknown.UNKNOWN explicitly by returning null for those cases, return
Boolean.TRUE only when clinicalConfirmation == YesNoUnknown.YES, and return
Boolean.FALSE only when clinicalConfirmation == YesNoUnknown.NO so UNKNOWN/null
are omitted as EpiPulse expects; update the method
deriveClinicalCriteriaStatus(YesNoUnknown clinicalConfirmation) accordingly.
- Around line 109-169: The current normalization accepts any letter+digits
(e.g., "Z99") and converts it to MEASV_..., which is invalid for EpiPulse;
update normalizeGenotypeForEpipulse (and use in
extractGenotypeFromDelimitedFormat) to validate against the actual allowed
measles genotype set/pattern instead of the generic "^[A-Z]\\d*$" rule —
implement either a whitelist Set<String> of valid codes (e.g.,
"A","B1","B2","B3","C1","C2","D1"..."D11","E","F","G1".."G3","H1","H2") or a
tight regex that enforces those letter+number ranges, and only return
"MEASV_"+code when the code is in that allowed set; otherwise return null.
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java:
- Around line 97-105: Replace the non-atomic check-then-update in
EpipulseCsvExportOrchestrator with an atomic "claim" update: add a new service
method on diseaseExportService (e.g., claimExportForProcessing or
updateStatusIfPending) that executes an UPDATE setting status = IN_PROGRESS
WHERE uuid = ? AND status = PENDING and returns true only if one row was
affected; then in EpipulseCsvExportOrchestrator call that claim method instead
of first checking epipulseExport.getStatus() and only proceed (and set
shouldUpdateStatus) when the claim returns true, otherwise log and return.
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java:
- Around line 312-330: The current buildExposureLocationsCte uses an inner JOIN
between exposures and location which drops exposures with null location_id;
change the JOIN in buildExposureLocationsCte from "JOIN location l ON
e.location_id = l.id" to a LEFT JOIN so exposures with no location are
preserved, and adjust any references to l.* in the SELECT/CASE to handle nulls
(keep existing COALESCE-like logic or add null-safe checks) so the STRING_AGG
still includes "unknown/other" entries for those records.
- Around line 218-235: The aggregation for specimen_types_virus in
buildSampleDataCte is currently pulling samplematerial from all non-null samples
(s2) and must be limited to samples that are actually used for virus-detection
tests; change the LEFT JOIN for s2 (and analogous joins/aggregations in the
other blocks noted around 237-265 and 267-298) to only include samples that have
a related pathogentest row with virus-specific testtype values (e.g., the same
pattern used for s3/pt_sero), so specimen_types_virus is computed by joining
samples to pathogentest and filtering pt.testtype IN (...) for virus tests
before STRING_AGG.
🧹 Nitpick comments (8)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseConfigurationLookupService.java (4)
41-41: Unused logger field.The logger is declared but never used anywhere in this service. Either remove it or add logging for configuration lookups (e.g., debug logs for lookup results, info logs for context creation).
♻️ Proposed fix: remove unused logger
-import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import de.symeda.sormas.api.epipulse.EpipulseExportDto;public class EpipulseConfigurationLookupService { - private final Logger logger = LoggerFactory.getLogger(getClass()); - @PersistenceContext(unitName = ModelConstants.PERSISTENCE_UNIT_NAME)
61-68: Add null validation for input parameters.If
exportDtoorexportDto.getSubjectCode()is null, aNullPointerExceptionwill occur. Similarly, ifserverCountryLocaleis null, the error message will be confusing ("Invalid server country code: null"). Consider adding explicit null checks with meaningful error messages.♻️ Proposed fix
public EpipulseConfigurationContext lookupConfiguration(EpipulseExportDto exportDto, String serverCountryLocale, String serverCountryName) throws IllegalArgumentException, IllegalStateException { + if (exportDto == null || exportDto.getSubjectCode() == null) { + throw new IllegalArgumentException("Export DTO and subject code must not be null"); + } + if (StringUtils.isBlank(serverCountryLocale)) { + throw new IllegalArgumentException("Server country locale must not be blank"); + } + String reportingCountry = lookupReportingCountry(serverCountryLocale); String serverCountryNutsCode = lookupServerCountryNutsCode(serverCountryName); String subjectCode = lookupSubjectCode(exportDto.getSubjectCode());
88-94: Unnecessary@SuppressWarnings("unchecked").The cast from
ObjecttoStringis a checked cast, not an unchecked generic cast. The compiler warning this annotation suppresses doesn't apply here. The same applies to the similar annotations inlookupServerCountryNutsCodeandlookupSubjectCode.♻️ Proposed fix
- @SuppressWarnings("unchecked") String reportingCountry = (String) em.createNativeQuery(reportingCountryQuery)
158-160: Improve error message to include the queried disease.The current message "Subject code is empty" doesn't indicate which disease lookup failed, making debugging harder.
♻️ Proposed fix
if (StringUtils.isBlank(subjectCode)) { - throw new IllegalStateException("Subject code is empty"); + throw new IllegalStateException("Subject code lookup failed for disease: " + subjectCodeEnum.name()); }sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (2)
115-115: Use Path API for portable file path construction.String concatenation with "/" may cause issues on Windows. Use
Paths.get()for platform-independent path construction.♻️ Suggested fix
- exportFilePath = generatedFilesPath + "/" + exportFileName; + exportFilePath = Paths.get(generatedFilesPath, exportFileName).toString();
93-93: Use parameterized logging instead of string concatenation.Some log statements use string concatenation while others (line 158) correctly use SLF4J parameterized logging. For consistency and performance, prefer placeholders throughout.
♻️ Suggested fix
- logger.error("EpipulseExport with uuid " + uuid + " not found"); + logger.error("EpipulseExport with uuid {} not found", uuid);- logger.error("EpipulseExport with uuid " + uuid + " is not in status PENDING"); + logger.error("EpipulseExport with uuid {} is not in status PENDING", uuid);- logger.error("Error during export with uuid " + uuid + ": " + e.getMessage(), e); + logger.error("Error during export with uuid {}: {}", uuid, e.getMessage(), e);- logger.error("CRITICAL: Failed to update export status for uuid " + uuid + ": " + e.getMessage(), e); + logger.error("CRITICAL: Failed to update export status for uuid {}: {}", uuid, e.getMessage(), e);Also applies to: 98-98, 142-142, 170-170
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java (1)
388-472: Parsing helpers: consider deduplicating into a single genericparseEnumhelper.
The repeatedvalueOf+ warn pattern is consistent; extracting a generic helper would reduce drift and keep logging consistent.sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java (1)
28-33: Make this a non-instantiable utility class.
Prevents accidental construction and signals intended usage.Proposed refactor
-public class EpipulseLaboratoryMapper { +public final class EpipulseLaboratoryMapper { + + private EpipulseLaboratoryMapper() { + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportFacade.javasormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCommonDtoMapper.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseConfigurationLookupService.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/AbstractEpipulseDiseaseExportStrategy.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java
🚧 Files skipped from review as they are similar to previous changes (2)
- sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/AbstractEpipulseDiseaseExportStrategy.java
- sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCommonDtoMapper.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-24T12:07:28.919Z
Learnt from: roldy
Repo: SORMAS-Foundation/SORMAS-Project PR: 13784
File: sormas-api/src/main/resources/enum_haw-US.properties:287-297
Timestamp: 2025-12-24T12:07:28.919Z
Learning: In SORMAS, enum_en.properties is intentionally empty (contains only license header) and uses enum.properties as the fallback base file for English locale. This is a standard Java ResourceBundle pattern and should not be flagged as missing translations.
Applied to files:
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java
🧬 Code graph analysis (1)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (3)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportFacadeEjb.java (2)
LocalBean(49-53)Stateless(26-54)sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportService.java (1)
Stateless(43-147)sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportResult.java (1)
EpipulseDiseaseExportResult(20-96)
⏰ 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). (5)
- GitHub Check: android app test (27)
- GitHub Check: android app test (26)
- GitHub Check: android app test (28)
- GitHub Check: SORMAS CI
- GitHub Check: Lint Code Base
🔇 Additional comments (8)
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseConfigurationLookupService.java (1)
110-131: LGTM!The null check for
countryNameand the case-insensitive lookup usinglower()are appropriate. Returning null when not found aligns with the documented behavior since the NUTS code is optional.sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (3)
44-64: LGTM!The class structure, EJB annotations, and dependency injections are appropriate for a stateless orchestrator service.
176-196: LGTM!Clean functional interface design that enables pluggable disease-specific export strategies. The Javadoc is complete and the signature is appropriate.
118-119: No null check needed —getExportEntryList()is guaranteed non-null.The
mapResultsToEntryDtos()method always returns a non-nullArrayList(line 200 in AbstractEpipulseDiseaseExportStrategy), which is then assigned to the result before the method returns. If any exception occurs beforesetExportEntryList()is called, the exception is thrown and the result is never returned. Therefore,getExportEntryList()cannot be null when the execution completes successfully.Likely an incorrect or invalid review comment.
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java (3)
183-214: Max-count computation is straightforward and safe.
Nice to keep these per-field maxima encapsulated at the strategy level.
46-73: CTE composition is correct. The code properly concatenates common CTEs (withWITHkeyword from the first one) and Measles-specific CTEs (prefixed with comma inbuildSampleDataCte()) into a valid WITH clause that ends withbuildComplicationsDataCte()before the SELECT statement.
75-181: Review comment is based on misunderstanding of index semantics; proposed fix would introduce the error it claims to prevent.The
startIndexparameter correctly represents the last index used by common field mapping (index 27). The current codeint index = startIndex; row[++index]correctly reads disease-specific fields starting from index 28. The proposed fixint index = startIndex - 1would incorrectly read from index 27, re-reading the vaccinations field already mapped byEpipulseCommonDtoMapper.mapCommonFields. The implementation is correct and matches the documented contract inAbstractEpipulseDiseaseExportStrategy(line 207-210, which clearly showsmapCommonFieldsreturns the last index used, and that value is passed asstartIndex).The documentation comment in
AbstractEpipulseDiseaseExportStrategyline 145 ("the index where disease-specific fields start (typically 27)") is slightly misleading—it should state that it's the last common field index, not the first disease-specific index—but the implementation throughout is correct.Likely an incorrect or invalid review comment.
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseDiseaseExportFacade.java (1)
20-26: All implementers and callers are properly updated for the new method.Verification confirms that
EpipulseDiseaseExportFacadeEjbimplements the interface and includes thestartMeaslesExport(String uuid)method, and it is correctly called inEpipulseExportTimerEjb. No compilation or deployment issues will occur.
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java
Show resolved
Hide resolved
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java
Show resolved
Hide resolved
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java
Show resolved
Hide resolved
...s-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java
Outdated
Show resolved
Hide resolved
...-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java
Show resolved
Hide resolved
...-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java
Show resolved
Hide resolved
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13785 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java:
- Around line 76-173: The CSV writer is created with
CSVUtils.createCSVWriter(new OutputStreamWriter(new
FileOutputStream(exportFilePath), ...)) inside orchestrateExport which can throw
and leak the FileOutputStream; change the creation to use try-with-resources so
the FileOutputStream and OutputStreamWriter (and CSVWriter) are opened in a
resource block (or explicitly create and assign a
FileOutputStream/OutputStreamWriter to local variables and close them in
finally) before calling CSVUtils.createCSVWriter, ensuring all streams are
closed if CSVUtils.createCSVWriter throws; update references to writer,
FileOutputStream and OutputStreamWriter within orchestrateExport accordingly so
the finally block only closes resources that were successfully opened.
🧹 Nitpick comments (4)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java (2)
28-32: Add private constructor to prevent instantiation.This is a utility class with only static methods. Adding a private constructor prevents unintended instantiation and signals the class's intent.
Suggested fix
public class EpipulseLaboratoryMapper { + + private EpipulseLaboratoryMapper() { + // utility class + }
156-169: Pre-compile the regex Pattern as a static field.
Pattern.compileis called on every invocation. Pre-compiling as astatic finalfield improves performance and follows Java best practices for regex usage.Suggested fix
public class EpipulseLaboratoryMapper { + + private static final java.util.regex.Pattern GENOTYPE_DELIMITED_PATTERN = + java.util.regex.Pattern.compile("(?:MEV|MEASLES?|GENOTYPE|MV)[-_/\\s]+([A-Z]\\d*)"); // ... other code ... private static String extractGenotypeFromDelimitedFormat(String normalized) { - String pattern = "(?:MEV|MEASLES?|GENOTYPE|MV)[-_/\\s]+([A-Z]\\d*)"; - java.util.regex.Pattern p = java.util.regex.Pattern.compile(pattern); - java.util.regex.Matcher m = p.matcher(normalized); + java.util.regex.Matcher m = GENOTYPE_DELIMITED_PATTERN.matcher(normalized); if (m.find()) { return m.group(1); } return null; }sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java (2)
243-271: Consider performance impact of correlated subqueries.The
buildVirusDetectionDataCteuses two correlated subqueries (lines 247-255 for virus detection result, lines 256-263 for genotype) that scan samples and pathogen tests independently. WhileLIMIT 1mitigates the cost, this pattern repeats the same table scans multiple times per case.♻️ Consider consolidating subqueries
If performance becomes an issue, consider using window functions to retrieve the first virus detection result and genotype in a single scan:
virus_detection_data AS ( SELECT DISTINCT ON (c.id) c.id as case_id, pt.testdatetime as lab_result_date, pt.testresult as virus_detection_result, COALESCE(pt.typingid, pt.genotyperesult) as genotype_raw FROM filtered_cases c LEFT JOIN samples s ON s.associatedcase_id = c.id AND s.deleted = false LEFT JOIN pathogentest pt ON pt.sample_id = s.id AND pt.testtype IN ('PCR_RT_PCR', 'CULTURE', 'ISOLATION', 'DIRECT_FLUORESCENT_ANTIBODY', 'INDIRECT_FLUORESCENT_ANTIBODY') AND pt.testresultverified = true ORDER BY c.id, pt.testdatetime ASC )Note: This is only a suggestion if performance profiling indicates these subqueries are a bottleneck.
126-134: Cluster setting limited to single entry despite list structure.The cluster type mapping creates a list but only adds a single cluster setting code (lines 130-132). While the DTO field
clusterSettingis aList<String>and max counts are tracked (lines 195-196), the current implementation doesn't support multiple cluster types per case.If multiple cluster settings per case are expected in the future, consider updating the SQL CTE (
buildEpidataClusterCte) to aggregate multiple cluster types usingSTRING_AGG, similar to the approach used for specimen types. For now, the single-cluster implementation appears intentional and matches the data model.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportService.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-24T12:07:28.919Z
Learnt from: roldy
Repo: SORMAS-Foundation/SORMAS-Project PR: 13784
File: sormas-api/src/main/resources/enum_haw-US.properties:287-297
Timestamp: 2025-12-24T12:07:28.919Z
Learning: In SORMAS, enum_en.properties is intentionally empty (contains only license header) and uses enum.properties as the fallback base file for English locale. This is a standard Java ResourceBundle pattern and should not be flagged as missing translations.
Applied to files:
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java
⏰ 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). (4)
- GitHub Check: android app test (26)
- GitHub Check: android app test (28)
- GitHub Check: android app test (27)
- GitHub Check: SORMAS CI
🔇 Additional comments (16)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java (5)
50-74: LGTM!The mapping logic is correct with proper null-safety. The switch handles all relevant specimen types with a safe default fallback.
89-107: LGTM!Consistent handling of null and unmapped test results by returning "NOTEST" aligns with EpiPulse semantics.
188-213: LGTM!Clean mapping with proper null-safety and exhaustive handling of known cluster types.
228-245: LGTM!Correct mapping of case imported status values with appropriate null handling.
268-308: LGTM!Both methods handle their respective mappings correctly. The comment documenting that an empty list results in "NONE" in CSV is helpful for maintainability.
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/strategy/MeaslesExportStrategy.java (6)
218-241: Verify STRING_AGG delimiter consistency with downstream parsing.The
buildSampleDataCteuses comma (,) as the delimiter forSTRING_AGGwhen aggregating specimen types (lines 222-223). Ensure this matches the parsing logic inparseSpecimenTypes(line 400), which also splits by comma.The CTE construction is correct and uses proper LEFT JOINs with deleted checks. The distinct specimen materials are aggregated by test type category (virus detection vs. serology).
273-289: LGTM: Correct handling of fourfold antibody titer increase.The IgG serology CTE correctly maps
fourfoldincreaseantibodytiter = 'YES'to'POSITIVE'as the primary indicator (line 277), overriding the standard test result. This aligns with Measles-specific clinical criteria.
76-181: Field mapping logic is correct with proper index tracking.The
mapDiseaseSpecificFieldsmethod correctly uses pre-increment (++index) to map 20 Measles-specific fields from the SQL result row (indices 28-47). The parsing includes appropriate null checks and delegates to mapper utilities for code conversion.
409-479: Consistent parsing pattern with appropriate error handling.All parsing helper methods follow a consistent pattern: validate input, attempt
valueOfparsing, catchIllegalArgumentException, log a warning, and returnnullon failure. This graceful degradation ensures that malformed data doesn't halt the export process.The warning logs provide visibility into data quality issues without failing the export, which is appropriate for a batch data export scenario.
183-214: Max count calculation correctly iterates all entries.The
calculateDiseaseSpecificMaxCountsmethod properly iterates through all entries to determine the maximum count for each repeatable field (complicationDiagnosis, clusterSetting, placeOfInfection, typeOfSpecimenCollected, typeOfSpecimenSerology). These max values drive the CSV column layout in the corresponding CSV export strategy.
318-337: Location precedence and ordering require verification against EpiPulse specifications.The code implementation shows explicit precedence (country name → city → details → 'Unknown') with ordering by most recent exposure first (startdate DESC). However, this implementation cannot be verified against EpiPulse reporting requirements in the codebase. Confirm with product or requirements documentation that:
- Country name should take precedence over city and location details
- Most recent exposures should appear first in the place of infection field
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseDiseaseExportService.java (2)
54-68: Strategy pattern successfully centralizes disease-specific export logic.The refactoring to inject disease-specific export strategies (lines 54-58) and delegate export operations (lines 60-68) successfully extracts the complex SQL and mapping logic into dedicated strategy classes. This improves maintainability and makes it easier to add support for additional diseases in the future.
70-101: Optimistic locking implementation is correct with proper transaction isolation.The
tryClaimExportForProcessingmethod correctly implements optimistic locking for concurrent export processing:
@TransactionAttribute(REQUIRES_NEW)ensures atomic status transition in an isolated transaction (line 70)- The UPDATE with WHERE clause checks current status (line 82), preventing race conditions
em.flush()forces immediate execution before checking the result (line 85)em.clear()in finally block prevents stale entity cache (line 99)The pattern ensures that only one process can claim a PENDING export, which is critical for preventing duplicate processing in a distributed environment.
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (3)
153-161: File size calculation correctly positioned after writer close.The file size is calculated only after the CSV writer is closed and only when the export status is COMPLETED (lines 153-161). This ensures accurate file size measurement and prevents reading incomplete files.
88-104: Early validation prevents status updates for invalid exports.The orchestration correctly validates the export existence (lines 90-95) and attempts to claim processing (lines 98-102) before setting
shouldUpdateStatus = true(line 104). This ensures that invalid or already-claimed exports don't trigger spurious status updates in the finally block.The early return pattern (lines 94, 101) combined with the
shouldUpdateStatusguard (line 164) provides proper status lifecycle management.
178-195: Functional interface cleanly abstracts disease-specific export logic.The
ExportFunctionfunctional interface provides a clean abstraction for disease-specific export operations, allowing the orchestrator to remain agnostic of the underlying disease export implementation. The SQLException propagation in the signature (line 194) is appropriate for database-driven exports.
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java
Outdated
Show resolved
Hide resolved
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13785 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java:
- Around line 89-92: The mapTestResultToEpipulseCode method currently returns
"NOTEST" for null input which is inconsistent with other mappers; change its
null handling to return null (like mapSampleMaterialToEpipulseCode,
mapClusterTypeToEpipulseCode, and mapCaseImportedStatusToEpipulseCode) so
callers that already null-check remain correct and behavior is consistent;
update the method body in EpipulseLaboratoryMapper.mapTestResultToEpipulseCode
to return null when testResult is null and adjust or add a brief Javadoc comment
if needed to document this behavior.
In
@sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java:
- Around line 76-189: The orchestrateExport method currently closes
FileOutputStream (fos), OutputStreamWriter (osw), and CSVWriter (writer)
manually in a verbose finally block; replace that manual resource management by
nesting these three AutoCloseable resources in a try-with-resources when
creating fos, osw, and writer (use CSVUtils.createCSVWriter(osw, ...) to obtain
the CSVWriter) so they are closed automatically in reverse order, remove the
explicit close logic for writer/osw/fos from the finally block, and keep the
exportStatus, totalRecords, exportFileName, exportFilePath and
shouldUpdateStatus logic unchanged but move the file-size calculation and the
call to diseaseExportService.updateStatusForBackgroundProcess outside/after the
try-with-resources so they run after resources are closed.
🧹 Nitpick comments (4)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java (3)
32-32: Consider adding a private constructor to prevent instantiation.This is a utility class with only static methods. Adding a private constructor prevents accidental instantiation and clearly signals the class's intent.
🔧 Proposed fix
public class EpipulseLaboratoryMapper { + + private EpipulseLaboratoryMapper() { + // Utility class - prevent instantiation + } /**
119-150: Extract duplicate regex pattern to a constant.The measles genotype validation pattern appears twice (line 137 and line 163). Extract it to a private static final field to improve maintainability and prevent potential inconsistencies.
♻️ Proposed refactor
public class EpipulseLaboratoryMapper { + + private static final String MEASLES_GENOTYPE_PATTERN = "^(A|B[1-3]|C[1-2]|D(1[0-1]|[1-9])|E|F|G[1-3]|H[1-2])$"; private EpipulseLaboratoryMapper() { // Utility class - prevent instantiation }Then update both usages:
public static String normalizeGenotypeForEpipulse(String genotypeText) { // ... existing code ... // Try to parse formats like "A", "B1", "D10", etc. and add MEASV_ prefix // Measles genotypes are limited (e.g., A, B1-3, C1-2, D1-11, E, F, G1-3, H1-2) - if (normalized.matches("^(A|B[1-3]|C[1-2]|D(1[0-1]|[1-9])|E|F|G[1-3]|H[1-2])$")) { + if (normalized.matches(MEASLES_GENOTYPE_PATTERN)) { return "MEASV_" + normalized; } // ... rest of method ... } private static boolean isValidMeaslesGenotype(String genotype) { if (genotype == null) { return false; } - return genotype.matches("^(A|B[1-3]|C[1-2]|D(1[0-1]|[1-9])|E|F|G[1-3]|H[1-2])$"); + return genotype.matches(MEASLES_GENOTYPE_PATTERN); }
174-187: Compile regex pattern once for better performance.The pattern is compiled on every method invocation (line 179), which can be inefficient during batch exports processing many cases. Extract it to a static final field to compile once.
⚡ Proposed performance improvement
Add the pattern as a static field:
public class EpipulseLaboratoryMapper { private static final String MEASLES_GENOTYPE_PATTERN = "^(A|B[1-3]|C[1-2]|D(1[0-1]|[1-9])|E|F|G[1-3]|H[1-2])$"; + private static final java.util.regex.Pattern DELIMITED_GENOTYPE_PATTERN = + java.util.regex.Pattern.compile("(?:MEV|MEASLES?|GENOTYPE|MV)[-_/\\s]+([A-Z]\\d*)");Then update the method:
private static String extractGenotypeFromDelimitedFormat(String normalized) { // Match patterns like "PREFIX-A", "PREFIX/B1", "PREFIX_D10", "PREFIX A" // where PREFIX is some non-numeric text // This captures the genotype part after common delimiters - String pattern = "(?:MEV|MEASLES?|GENOTYPE|MV)[-_/\\s]+([A-Z]\\d*)"; - java.util.regex.Pattern p = java.util.regex.Pattern.compile(pattern); - java.util.regex.Matcher m = p.matcher(normalized); + java.util.regex.Matcher m = DELIMITED_GENOTYPE_PATTERN.matcher(normalized); if (m.find()) { return m.group(1); }sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (1)
95-95: Use parameterized logging consistently.Several log statements use string concatenation (lines 95, 143, 150, 157, 164, 185) instead of parameterized logging, while others correctly use parameterized logging (lines 102, 173, 175). Parameterized logging is more efficient and avoids unnecessary string concatenation when the log level is disabled.
♻️ Proposed fix for consistent parameterized logging
-logger.error("EpipulseExport with uuid " + uuid + " not found"); +logger.error("EpipulseExport with uuid {} not found", uuid); -logger.error("Error during export with uuid " + uuid + ": " + e.getMessage(), e); +logger.error("Error during export with uuid {}: {}", uuid, e.getMessage(), e); -logger.error("CRITICAL: Failed to close CSVWriter for uuid " + uuid + ": " + e.getMessage(), e); +logger.error("CRITICAL: Failed to close CSVWriter for uuid {}: {}", uuid, e.getMessage(), e); -logger.error("CRITICAL: Failed to close OutputStreamWriter for uuid " + uuid + ": " + e.getMessage(), e); +logger.error("CRITICAL: Failed to close OutputStreamWriter for uuid {}: {}", uuid, e.getMessage(), e); -logger.error("CRITICAL: Failed to close FileOutputStream for uuid " + uuid + ": " + e.getMessage(), e); +logger.error("CRITICAL: Failed to close FileOutputStream for uuid {}: {}", uuid, e.getMessage(), e); -logger.error("CRITICAL: Failed to update export status for uuid " + uuid + ": " + e.getMessage(), e); +logger.error("CRITICAL: Failed to update export status for uuid {}: {}", uuid, e.getMessage(), e);Also applies to: 143-143, 150-150, 157-157, 164-164, 185-185
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java
⏰ 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). (4)
- GitHub Check: SORMAS CI
- GitHub Check: android app test (28)
- GitHub Check: android app test (27)
- GitHub Check: android app test (26)
🔇 Additional comments (6)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java (5)
50-74: LGTM!The mapping logic is clear and well-documented. Null handling and fallback cases are appropriate.
206-231: LGTM!The cluster type mapping is comprehensive and correctly handles all documented EpiPulse cluster setting codes.
246-263: LGTM!The import status mapping comprehensively covers all documented EpiPulse importation status codes.
286-311: LGTM!The complication mapping correctly handles multiple symptom states and appropriately documents the unavailable PNEU (pneumonia) mapping. The empty list convention for "NONE" is clearly noted.
321-327: LGTM!The clinical criteria derivation correctly handles all YesNoUnknown states with appropriate null handling.
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (1)
182-183: The service method already properly handles null parameters with explicit validation.The
updateStatusForBackgroundProcessmethod inEpipulseDiseaseExportServicecontains explicit null validation: when the export status isCOMPLETED, it validates thattotalRecords,exportFileName, andexportFileSizeBytesare non-null, throwing anIllegalArgumentExceptionif they are missing. For non-COMPLETEDstatuses (e.g.,FAILED), null values are acceptable and file metadata is not persisted.The orchestrator appropriately ensures that when
COMPLETEDstatus is passed, all three parameters have been set:exportFileNameis assigned at line 115,totalRecordsat line 120, andexportFileSizeBytesis only calculated when status equalsCOMPLETED(line 169). If any exception occurs during export, the status remainsFAILED, making null values acceptable per the service method's design.No changes are required; the implementation correctly handles the null parameter scenario.
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java
Show resolved
Hide resolved
...s-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java (2)
119-150: Extract duplicate regex pattern to a constant.The measles genotype validation pattern
^(A|B[1-3]|C[1-2]|D(1[0-1]|[1-9])|E|F|G[1-3]|H[1-2])$is duplicated at lines 137 and 163, making maintenance error-prone if the pattern needs updating.♻️ Proposed refactor to extract pattern constant
+ private static final String MEASLES_GENOTYPE_PATTERN = "^(A|B[1-3]|C[1-2]|D(1[0-1]|[1-9])|E|F|G[1-3]|H[1-2])$"; + public static String normalizeGenotypeForEpipulse(String genotypeText) { if (genotypeText == null || genotypeText.trim().isEmpty()) { return null; } String normalized = genotypeText.trim().toUpperCase(); // If already in MEASV_ format, validate the suffix and return if valid if (normalized.startsWith("MEASV_")) { String suffix = normalized.substring(6); // Extract part after "MEASV_" if (isValidMeaslesGenotype(suffix)) { return normalized; } return null; } // Try to parse formats like "A", "B1", "D10", etc. and add MEASV_ prefix // Measles genotypes are limited (e.g., A, B1-3, C1-2, D1-11, E, F, G1-3, H1-2) - if (normalized.matches("^(A|B[1-3]|C[1-2]|D(1[0-1]|[1-9])|E|F|G[1-3]|H[1-2])$")) { + if (normalized.matches(MEASLES_GENOTYPE_PATTERN)) { return "MEASV_" + normalized; } // Try to extract genotype from common formats with delimiters // Matches patterns like "MeV-A", "Genotype-B1", "MV/A", "MEASLES-D4" String extracted = extractGenotypeFromDelimitedFormat(normalized); if (isValidMeaslesGenotype(extracted)) { return "MEASV_" + extracted; } // Return null for ambiguous or unparseable inputs return null; } private static boolean isValidMeaslesGenotype(String genotype) { if (genotype == null) { return false; } - return genotype.matches("^(A|B[1-3]|C[1-2]|D(1[0-1]|[1-9])|E|F|G[1-3]|H[1-2])$"); + return genotype.matches(MEASLES_GENOTYPE_PATTERN); }Also applies to: 159-164
174-187: Pre-compile the regex Pattern for better performance.The Pattern at lines 178-179 is compiled on every invocation of
extractGenotypeFromDelimitedFormat, which is inefficient if this method is called frequently during batch exports.♻️ Proposed refactor to pre-compile Pattern
+ private static final java.util.regex.Pattern GENOTYPE_EXTRACTION_PATTERN = + java.util.regex.Pattern.compile("(?:MEV|MEASLES?|GENOTYPE|MV)[-_/\\s]+([A-Z]\\d*)"); + private static String extractGenotypeFromDelimitedFormat(String normalized) { // Match patterns like "PREFIX-A", "PREFIX/B1", "PREFIX_D10", "PREFIX A" // where PREFIX is some non-numeric text // This captures the genotype part after common delimiters - String pattern = "(?:MEV|MEASLES?|GENOTYPE|MV)[-_/\\s]+([A-Z]\\d*)"; - java.util.regex.Pattern p = java.util.regex.Pattern.compile(pattern); - java.util.regex.Matcher m = p.matcher(normalized); + java.util.regex.Matcher m = GENOTYPE_EXTRACTION_PATTERN.matcher(normalized); if (m.find()) { return m.group(1); } return null; }sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (1)
92-92: Use parameterized logging consistently.Lines 92, 141, and 160 use string concatenation in log statements, while other log calls in the file correctly use parameterized logging (e.g., line 99, 148). Parameterized logging is more efficient and prevents unnecessary string construction when the log level is disabled.
♻️ Proposed fixes for parameterized logging
- logger.error("EpipulseExport with uuid " + uuid + " not found"); + logger.error("EpipulseExport with uuid {} not found", uuid);- logger.error("Error during export with uuid " + uuid + ": " + e.getMessage(), e); + logger.error("Error during export with uuid {}", uuid, e);- logger.error("CRITICAL: Failed to update export status for uuid " + uuid + ": " + e.getMessage(), e); + logger.error("CRITICAL: Failed to update export status for uuid {}", uuid, e);Also applies to: 141-141, 160-160
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.javasormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java
⏰ 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). (5)
- GitHub Check: Lint Code Base
- GitHub Check: android app test (26)
- GitHub Check: android app test (28)
- GitHub Check: android app test (27)
- GitHub Check: SORMAS CI
🔇 Additional comments (12)
sormas-api/src/main/java/de/symeda/sormas/api/epipulse/EpipulseLaboratoryMapper.java (6)
28-74: LGTM! Sample material mapping is well-documented and correctly implemented.The mapping logic correctly handles null inputs and provides appropriate many-to-one mappings between SORMAS and EpiPulse taxonomies. The extensive JavaDoc documenting EpiPulse reference values is helpful for maintainability.
76-107: LGTM! Test result mapping is logically sound.The mapping correctly consolidates PENDING and NOT_DONE into "NOTEST", which aligns with EpiPulse's simpler taxonomy for test states.
189-231: LGTM! Cluster type mapping is comprehensive and correct.All ClusterType enum values are appropriately mapped to their EpiPulse equivalents with proper null handling.
233-263: LGTM! Imported status mapping is correctly implemented.The mapping accurately reflects the relationship between SORMAS case importation states and EpiPulse codes, with appropriate null handling.
265-311: LGTM! Complication mapping logic is correct with safe implicit null handling.The method correctly maps only confirmed symptoms (SymptomState.YES) to complication codes. The implicit null handling is safe since
null == SymptomState.YESevaluates to false, though explicit null checks could improve readability if desired.
313-326: LGTM! Clinical criteria derivation is logically sound.The method correctly handles the three-state logic (YES/NO/UNKNOWN) by mapping UNKNOWN and null inputs to null, maintaining semantic clarity in the export data.
sormas-backend/src/main/java/de/symeda/sormas/backend/epipulse/EpipulseCsvExportOrchestrator.java (6)
44-64: LGTM!The class declaration and dependency injection follow standard Java EE patterns. The use of
@Statelesswith@LocalBeanis appropriate for this orchestration service, and the injected EJBs provide the necessary collaborators for the export workflow.
119-136: LGTM!The CSV writing logic correctly uses try-with-resources to ensure proper cleanup of file handles and streams. The strategy pattern effectively decouples CSV structure definition (headers and row construction) from the orchestration flow, making the code extensible for different disease exports.
139-163: LGTM!The error handling and cleanup logic is well-structured:
- Generic exception catch ensures any failure during export is handled gracefully and updates the export status to FAILED.
- File size is calculated only for completed exports, avoiding misleading metrics for partial files.
- The finally block guards status updates behind
shouldUpdateStatus, preventing status overwrites for exports that weren't successfully claimed.- Individual try-catch blocks within finally ensure that failures in file size calculation don't prevent status updates, and vice versa.
166-186: LGTM!The
ExportFunctionfunctional interface is well-designed with a clear contract. The@FunctionalInterfaceannotation ensures compile-time validation, and the method signature provides the necessary context (exportDto, countryCode, countryName) for disease-specific implementations while returning structured results for CSV generation.
111-113: No action needed. The filename generated bygenerateDownloadFileNameis constructed entirely from safe, controlled sources: enum names, dates (digits only after formatting), Long IDs, and timestamps. Path traversal vulnerabilities are not possible with this implementation.Likely an incorrect or invalid review comment.
76-104: The atomic claim mechanism is properly implemented.The
tryClaimExportForProcessingmethod uses a database-level atomic UPDATE with a WHERE clause that checks both the UUID and the expected status (PENDING). This ensures that only one process can successfully transition the export to IN_PROGRESS, and the method correctly returnstrueonly if the update affected exactly one row. TheorchestrateExportpattern is sound: it validates the export exists, attempts an atomic claim, and uses the boolean result to determine whether to proceed with processing. No changes needed.
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13785 |
1 similar comment
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13785 |
Fixes #13771
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.