refactor: migrate from ostermillerutils 1.4.3 to Apache Commons CSV 1.12.0#2248
refactor: migrate from ostermillerutils 1.4.3 to Apache Commons CSV 1.12.0#2248github-actions[bot] wants to merge 2 commits intomaintenancefrom
Conversation
….12.0
- Replace 21-year-old unmaintained ostermillerutils library
- Migrate all CSV parsing/writing to Apache Commons CSV
- Updated 6 Java files and 2 JSP files
- Key changes:
* CSVParser/ExcelCSVParser → Commons CSV with CSVFormat
* CSVPrinter methods: write() → print(), writeln() → println()
* Tab delimiter: changeDelimiter('\t') → CSVFormat.withDelimiter('\t')
* CSV parsing: Static parse() → instance-based with CSVRecord iteration
- Remove ostermillerutils dependency from pom.xml
- Remove ostermillerutils GPL license from 3RDPARTYLICENSES
Fixes #2153
Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR removes the 21-year-old ostermillerutils dependency and replaces all CSV handling with Apache Commons CSV across the codebase. The dependency is updated in pom.xml, and CSV parsing/writing implementations are migrated in 5 Java action/utility classes, 2 JSP files, and the license archive is updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.java
Fixed
Show fixed
Hide fixed
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.java
Fixed
Show fixed
Hide fixed
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Confidence score: 4/5
- Potential file-handle leak in
src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.javawhere FileReader/CSVParser aren’t closed could impact repeated uploads over time - Score reflects a single medium-severity resource-management issue; otherwise the change looks safe to merge
- Pay close attention to
src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.java- ensure readers/parsers are closed with try-with-resources.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.java">
<violation number="1" location="src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.java:189">
P2: FileReader/CSVParser are not closed, which can leak file handles during repeated uploads. Wrap the parse in a try-with-resources to ensure the reader/parser are closed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ca/openosp/openo/form/eCARES/EcaresForm2Action.java (1)
136-148:⚠️ Potential issue | 🟠 MajorMissing security privilege check in
export()method.The
export()method accesses patient form data but doesn't include asecurityInfoManager.hasPrivilege()check before processing. Other methods in this class (likefetch()) properly check privileges. As per coding guidelines, security checks must be the first operation in action methods.🔒 Proposed fix to add security check
public void export() { LoggedInInfo loggedInInfo = LoggedInInfo.getLoggedInInfoFromSession(request); Integer demographicNo = demographicNumberToInteger(request); + + if (!securityInfoManager.hasPrivilege(loggedInInfo, "_form", SecurityInfoManager.READ, demographicNo)) { + throw new SecurityException("missing required sec object (_form)"); + } + Integer formId = Integer.parseInt(request.getParameter(Constants.Cares.FormField.formId.name()));
🤖 Fix all issues with AI agents
In
`@src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.java`:
- Around line 187-197: The CSVParser.parse(...) and FileReader created for
validatedFile are not closed; wrap the file parsing in a try-with-resources that
opens the FileReader and obtains a CSVParser (e.g., try (FileReader fr = new
FileReader(validatedFile); CSVParser parser = CSVParser.parse(fr,
CSVFormat.EXCEL)) { List<CSVRecord> records = parser.getRecords(); ... } ) so
both resources are closed automatically; update the code that builds the
String[][] data from CSVRecord to use the records list obtained from the parser
inside that try block (refer to CSVParser.parse, validatedFile, CSVRecord and
the data array).
In `@src/main/webapp/report/ClinicalReports.jsp`:
- Line 684: The CSV output is being HTML-encoded causing characters like '&' to
be escaped; in the csv generation call replace
csvp.print(Encode.forHtmlContent(commonRow(heading,demoHash, demoObj))) with a
direct print of the raw CSV string (i.e., csvp.print(commonRow(heading,
demoHash, demoObj))) so the CSV contains unencoded values; ensure any
surrounding CSV-generation helpers (commonRow) already return plain text and
remove any other uses of Encode.forHtmlContent for CSV output.
🧹 Nitpick comments (7)
src/main/java/ca/openosp/openo/report/pageUtil/RptDownloadCSVServlet.java (1)
600-601: Same deprecated API usage; apply builder pattern here as well.This is the same issue as in
formReport(). UseCSVFormat.Builderinstead of the deprecatedwithDelimiter()method.♻️ Suggested fix
- CSVFormat format = CSVFormat.DEFAULT.withDelimiter('\t'); - CSVPrinter csvp = new CSVPrinter(swr, format); + CSVFormat format = CSVFormat.Builder.create(CSVFormat.DEFAULT) + .setDelimiter('\t') + .build(); + CSVPrinter csvp = new CSVPrinter(swr, format);src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.java (1)
53-57: Remove duplicate import.
java.util.Listis imported twice (line 30 and line 57).🧹 Remove duplicate import
import org.apache.commons.csv.CSVFormat; import org.apache.commons.csv.CSVParser; import org.apache.commons.csv.CSVPrinter; import org.apache.commons.csv.CSVRecord; -import java.util.List;src/main/java/ca/openosp/openo/report/reportByTemplate/SQLReporter.java (2)
80-85: Consider flushing CSVPrinter before reading from StringWriter.The CSVPrinter buffers data internally. While it works because
StringWriterdoesn't require flushing, explicitly flushing ensures all data is written before callingswr.toString().♻️ Suggested improvement
CSVPrinter csvp = new CSVPrinter(swr, CSVFormat.DEFAULT); String[][] data = UtilMisc.getArrayFromResultSet(rs); for (String[] row : data) { csvp.printRecord((Object[]) row); } + csvp.flush(); csv = swr.toString();
126-131: Same flush consideration applies here.Apply the same flush pattern in
generateSequencedReport()for consistency.♻️ Suggested improvement
CSVPrinter csvp = new CSVPrinter(swr, CSVFormat.DEFAULT); String[][] data = UtilMisc.getArrayFromResultSet(rs); for (String[] row : data) { csvp.printRecord((Object[]) row); } + csvp.flush(); csv = swr.toString();src/main/java/ca/openosp/openo/report/reportByTemplate/actions/GenerateOutFiles2Action.java (1)
85-95: Consider using try-with-resources for cleaner resource management.While
StringReaderdoesn't hold external resources, using try-with-resources is a good practice and makes the code more consistent with other resource-handling patterns in the codebase.♻️ Suggested improvement
- // Parse CSV using Apache Commons CSV - List<CSVRecord> records = CSVParser.parse(new StringReader(csv), CSVFormat.DEFAULT).getRecords(); - String[][] data = new String[records.size()][]; - for (int i = 0; i < records.size(); i++) { - CSVRecord record = records.get(i); - data[i] = new String[record.size()]; - for (int j = 0; j < record.size(); j++) { - data[i][j] = record.get(j); + // Parse CSV using Apache Commons CSV + String[][] data; + try (CSVParser parser = CSVParser.parse(new StringReader(csv), CSVFormat.DEFAULT)) { + List<CSVRecord> records = parser.getRecords(); + data = new String[records.size()][]; + for (int i = 0; i < records.size(); i++) { + CSVRecord record = records.get(i); + data[i] = new String[record.size()]; + for (int j = 0; j < record.size(); j++) { + data[i][j] = record.get(j); + } } }src/main/webapp/report/ClinicalExport.jsp (1)
61-72: CSV migration is correct; consider try-with-resources for consistency.The parsing logic correctly migrates to Apache Commons CSV. The data source is session-stored CSV content, so there's no path traversal concern. For consistency with best practices, consider wrapping in try-with-resources.
♻️ Optional improvement for resource management
- // Parse CSV using Apache Commons CSV - List<CSVRecord> records = CSVParser.parse(new StringReader(csv), CSVFormat.DEFAULT).getRecords(); - String[][] data = new String[records.size()][]; - for (int i = 0; i < records.size(); i++) { - CSVRecord record = records.get(i); - data[i] = new String[record.size()]; - for (int j = 0; j < record.size(); j++) { - data[i][j] = record.get(j); + // Parse CSV using Apache Commons CSV + String[][] data; + try (CSVParser parser = CSVParser.parse(new StringReader(csv), CSVFormat.DEFAULT)) { + List<CSVRecord> records = parser.getRecords(); + data = new String[records.size()][]; + for (int i = 0; i < records.size(); i++) { + CSVRecord record = records.get(i); + data[i] = new String[record.size()]; + for (int j = 0; j < record.size(); j++) { + data[i][j] = record.get(j); + } } }src/main/webapp/report/ClinicalReports.jsp (1)
644-645: Close theCSVPrinterto ensure output is flushed.
CSVPrinterimplementsCloseableand should be explicitly closed to ensure all buffered content is flushed to the underlying writer. Consider using try-with-resources or explicitly callingclose()before retrievingswr.toString().🔧 Proposed fix using explicit close
StringWriter swr = new StringWriter(); CSVPrinter csvp = new CSVPrinter(swr, CSVFormat.DEFAULT); %>Add before line 703 (before
session.setAttribute):csvp.close();
|
|
||
| // Parse CSV using Apache Commons CSV | ||
| List<CSVRecord> records = CSVParser.parse(new FileReader(validatedFile), CSVFormat.EXCEL).getRecords(); | ||
| String[][] data = new String[records.size()][]; | ||
| for (int i = 0; i < records.size(); i++) { | ||
| CSVRecord record = records.get(i); | ||
| data[i] = new String[record.size()]; | ||
| for (int j = 0; j < record.size(); j++) { | ||
| data[i][j] = record.get(j); | ||
| } | ||
| } |
There was a problem hiding this comment.
Resource leak: CSVParser and FileReader are not closed.
The CSVParser.parse() call creates resources that need to be properly closed. Using try-with-resources ensures both the FileReader and CSVParser are closed even if an exception occurs. This also addresses the CodeQL path expression warning by keeping the file access within a controlled scope.
🔒 Proposed fix using try-with-resources
// Re-validate at point of use for static analysis visibility
File validatedFile = PathValidationUtils.validateUpload(file);
- // Parse CSV using Apache Commons CSV
- List<CSVRecord> records = CSVParser.parse(new FileReader(validatedFile), CSVFormat.EXCEL).getRecords();
- String[][] data = new String[records.size()][];
- for (int i = 0; i < records.size(); i++) {
- CSVRecord record = records.get(i);
- data[i] = new String[record.size()];
- for (int j = 0; j < record.size(); j++) {
- data[i][j] = record.get(j);
+ // Parse CSV using Apache Commons CSV with proper resource management
+ String[][] data;
+ try (FileReader reader = new FileReader(validatedFile);
+ CSVParser parser = new CSVParser(reader, CSVFormat.EXCEL)) {
+ List<CSVRecord> records = parser.getRecords();
+ data = new String[records.size()][];
+ for (int i = 0; i < records.size(); i++) {
+ CSVRecord record = records.get(i);
+ data[i] = new String[record.size()];
+ for (int j = 0; j < record.size(); j++) {
+ data[i][j] = record.get(j);
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Parse CSV using Apache Commons CSV | |
| List<CSVRecord> records = CSVParser.parse(new FileReader(validatedFile), CSVFormat.EXCEL).getRecords(); | |
| String[][] data = new String[records.size()][]; | |
| for (int i = 0; i < records.size(); i++) { | |
| CSVRecord record = records.get(i); | |
| data[i] = new String[record.size()]; | |
| for (int j = 0; j < record.size(); j++) { | |
| data[i][j] = record.get(j); | |
| } | |
| } | |
| // Re-validate at point of use for static analysis visibility | |
| File validatedFile = PathValidationUtils.validateUpload(file); | |
| // Parse CSV using Apache Commons CSV with proper resource management | |
| String[][] data; | |
| try (FileReader reader = new FileReader(validatedFile); | |
| CSVParser parser = new CSVParser(reader, CSVFormat.EXCEL)) { | |
| List<CSVRecord> records = parser.getRecords(); | |
| data = new String[records.size()][]; | |
| for (int i = 0; i < records.size(); i++) { | |
| CSVRecord record = records.get(i); | |
| data[i] = new String[record.size()]; | |
| for (int j = 0; j < record.size(); j++) { | |
| data[i][j] = record.get(j); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 189-189: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
🤖 Prompt for AI Agents
In
`@src/main/java/ca/openosp/openo/dxresearch/pageUtil/dxResearchLoadAssociations2Action.java`
around lines 187 - 197, The CSVParser.parse(...) and FileReader created for
validatedFile are not closed; wrap the file parsing in a try-with-resources that
opens the FileReader and obtains a CSVParser (e.g., try (FileReader fr = new
FileReader(validatedFile); CSVParser parser = CSVParser.parse(fr,
CSVFormat.EXCEL)) { List<CSVRecord> records = parser.getRecords(); ... } ) so
both resources are closed automatically; update the code that builds the
String[][] data from CSVRecord to use the records list obtained from the parser
inside that try block (refer to CSVParser.parse, validatedFile, CSVRecord and
the data array).
|
|
||
| <%for(String heading:headings){ | ||
| csvp.write(Encode.forHtmlContent(commonRow(heading,demoHash, demoObj))); | ||
| csvp.print(Encode.forHtmlContent(commonRow(heading,demoHash, demoObj))); |
There was a problem hiding this comment.
Remove HTML encoding from CSV output — this corrupts the data.
Encode.forHtmlContent() converts characters like & to &, which is correct for HTML output (line 686) but corrupts CSV data. The CSV should contain raw, unencoded values.
🐛 Proposed fix
- csvp.print(Encode.forHtmlContent(commonRow(heading,demoHash, demoObj)));
+ csvp.print(commonRow(heading,demoHash, demoObj));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| csvp.print(Encode.forHtmlContent(commonRow(heading,demoHash, demoObj))); | |
| csvp.print(commonRow(heading,demoHash, demoObj)); |
🤖 Prompt for AI Agents
In `@src/main/webapp/report/ClinicalReports.jsp` at line 684, The CSV output is
being HTML-encoded causing characters like '&' to be escaped; in the csv
generation call replace
csvp.print(Encode.forHtmlContent(commonRow(heading,demoHash, demoObj))) with a
direct print of the raw CSV string (i.e., csvp.print(commonRow(heading,
demoHash, demoObj))) so the CSV contains unencoded values; ensure any
surrounding CSV-generation helpers (commonRow) already return plain text and
remove any other uses of Encode.forHtmlContent for CSV output.
|
@claude review |
|
Claude finished @yingbull's task in 2m 19s —— View job Code Review Complete ✅I've completed a thorough review of the ostermillerutils → Apache Commons CSV migration. Below are my findings: ✅ Overall AssessmentThe migration from ostermillerutils 1.4.3 to Apache Commons CSV 1.12.0 is well-executed and addresses a critical security concern (21-year-old unmaintained library). However, there are 3 issues that must be fixed before merging, plus several recommendations for improvement. 🚨 Critical Issues (Must Fix)1. Resource Leak in
|
|
@claude review |
|
Claude finished @yingbull's task in 2m 17s —— View job Code Review Complete ✅I've completed a thorough review of the ostermillerutils → Apache Commons CSV migration. Below are my findings: ✅ Overall AssessmentThe migration from ostermillerutils 1.4.3 to Apache Commons CSV 1.12.0 is well-executed and addresses a critical security concern (21-year-old unmaintained library). However, there are 3 issues that must be fixed before merging, plus several recommendations for improvement. 🚨 Critical Issues (Must Fix)1. Resource Leak in
|
|
@claude add a commit to make these recommended corrections to this branch. |
|
Claude finished @yingbull's task in 2m 48s —— View job Code Review Corrections Applied ✅Successfully implemented all recommended corrections from the code review. Changes Made:Critical Fixes (HIGH priority):
Recommended Improvements (MEDIUM priority):
Code Quality Improvements (LOW priority):
Summary:All 8 corrections have been successfully implemented across 7 files:
The changes maintain functional equivalence while improving:
Commit: |
Address critical issues and recommended improvements from code review: Critical Fixes (HIGH priority): - Fix resource leak in dxResearchLoadAssociations2Action.java by wrapping FileReader and CSVParser in try-with-resources block - Remove HTML encoding from CSV output in ClinicalReports.jsp to prevent data corruption (CSV should contain raw data, not HTML entities) - Remove duplicate import of java.util.List in dxResearchLoadAssociations2Action.java Recommended Improvements (MEDIUM priority): - Replace deprecated CSVFormat.withDelimiter() with builder pattern in RptDownloadCSVServlet.java (two occurrences) - Add missing security privilege check in EcaresForm2Action.export() to ensure PHI access control Code Quality Improvements (LOW priority): - Add explicit csvp.flush() calls before StringWriter.toString() in SQLReporter.java for defensive programming - Wrap CSV parsing in try-with-resources in GenerateOutFiles2Action.java for consistency - Wrap CSV parsing in try-with-resources in ClinicalExport.jsp for consistency All changes maintain functional equivalence while improving security, resource management, and API usage patterns. Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
|
|
||
| // Parse CSV using Apache Commons CSV with proper resource management | ||
| String[][] data; | ||
| try (FileReader reader = new FileReader(validatedFile); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
In general, to fix uncontrolled-path issues you must ensure that any file you open is constrained to a known safe location or is otherwise validated. Here, validatedFile is used to construct a FileReader, so PathValidationUtils.validateUpload must enforce that the sourceFile is in an allowed upload/temp directory, not just return it unconditionally. This centralizes validation and makes its effects visible to CodeQL.
The best targeted fix without changing existing behavior is:
-
Enhance
PathValidationUtils.validateUpload(File sourceFile)so it:- Rejects
null. - Ensures the file exists and is a regular file.
- Ensures the file resides in one of a small allow‑listed temp directories (for example, system temp directory and Tomcat work dirs) using canonical paths to defeat
..traversal. - Logs and throws
SecurityExceptionif validation fails. - Returns the same
Fileinstance when checks pass (no new paths created), so callers likedxResearchLoadAssociations2Action.uploadFilecontinue to work the same way, except that unsafe paths are now rejected.
- Rejects
-
Implement a reusable containment check in
PathValidationUtils(if not already present in code you’ve shown) using canonical paths and a lazily-initializedallowedTempDirectoriesset, so thatvalidateUploadcan call it. This also makes the behavior consistent with the comment indxResearchLoadAssociations2Action.isValidUploadedFilethat already mentions a temp-directory validation helper. -
Update
dxResearchLoadAssociations2Action.uploadFileto rely solely onPathValidationUtils.validateUpload(file)(and remove or de-duplicateisValidUploadedFile), or at least ensurevalidateUploaditself is sufficient for CodeQL. Since we must minimize changes, the safest approach is to keepisValidUploadedFile(for early user feedback) and just strengthenvalidateUpload; no change is strictly required indxResearchLoadAssociations2Actionbeyond that.
All required changes are confined to the snippets you’ve shown, within PathValidationUtils.validateUpload(File sourceFile) in src/main/java/ca/openosp/openo/utility/PathValidationUtils.java. No new external libraries are needed; we only rely on standard java.io.File and the existing MiscUtils logger.
|
@claude review |
|
Claude finished @yingbull's task in 1m 17s —— View job Code Review Complete ✅I've completed a thorough review of the ostermillerutils → Apache Commons CSV migration. Below are my findings: ✅ Overall AssessmentExcellent work! The recent commit (f1d9548) successfully addressed all critical issues from previous reviews. The migration from ostermillerutils 1.4.3 to Apache Commons CSV 1.12.0 is now ready for merge from a code quality perspective. 🎯 Verification ResultsI verified that commit f1d9548 ("fix: apply code review corrections to CSV migration") properly addressed all 8 issues identified in previous reviews: ✅ Critical Issues - ALL FIXED
✅ Medium Priority - ALL FIXED
✅ Code Quality Improvements - ALL APPLIED
🔍 Additional Findings✅ What's Done Well
|
….12.0
- Replace 21-year-old unmaintained ostermillerutils library
- Migrate all CSV parsing/writing to Apache Commons CSV
- Updated 6 Java files and 2 JSP files
- Key changes:
* CSVParser/ExcelCSVParser -> Commons CSV with CSVFormat
* CSVPrinter methods: write() -> print(), writeln() -> println()
* Tab delimiter: changeDelimiter('\t') -> CSVFormat.withDelimiter('\t')
* CSV parsing: Static parse() -> instance-based with CSVRecord iteration
- Remove ostermillerutils dependency from pom.xml
- Remove ostermillerutils GPL license from 3RDPARTYLICENSES
Fixes #2153
(cherry picked from commit e6514a8)
Upstream-PR: openo-beta/Open-O#2248
Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
Address critical issues and recommended improvements from code review: Critical Fixes (HIGH priority): - Fix resource leak in dxResearchLoadAssociations2Action.java by wrapping FileReader and CSVParser in try-with-resources block - Remove HTML encoding from CSV output in ClinicalReports.jsp to prevent data corruption (CSV should contain raw data, not HTML entities) - Remove duplicate import of java.util.List in dxResearchLoadAssociations2Action.java Recommended Improvements (MEDIUM priority): - Replace deprecated CSVFormat.withDelimiter() with builder pattern in RptDownloadCSVServlet.java (two occurrences) - Add missing security privilege check in EcaresForm2Action.export() to ensure PHI access control Code Quality Improvements (LOW priority): - Add explicit csvp.flush() calls before StringWriter.toString() in SQLReporter.java for defensive programming - Wrap CSV parsing in try-with-resources in GenerateOutFiles2Action.java for consistency - Wrap CSV parsing in try-with-resources in ClinicalExport.jsp for consistency All changes maintain functional equivalence while improving security, resource management, and API usage patterns. (cherry picked from commit f1d9548) Upstream-PR: openo-beta/Open-O#2248 Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
….12.0 (#339) * refactor: migrate from ostermillerutils 1.4.3 to Apache Commons CSV 1.12.0 - Replace 21-year-old unmaintained ostermillerutils library - Migrate all CSV parsing/writing to Apache Commons CSV - Updated 6 Java files and 2 JSP files - Key changes: * CSVParser/ExcelCSVParser -> Commons CSV with CSVFormat * CSVPrinter methods: write() -> print(), writeln() -> println() * Tab delimiter: changeDelimiter('\t') -> CSVFormat.withDelimiter('\t') * CSV parsing: Static parse() -> instance-based with CSVRecord iteration - Remove ostermillerutils dependency from pom.xml - Remove ostermillerutils GPL license from 3RDPARTYLICENSES Fixes #2153 (cherry picked from commit e6514a8) Upstream-PR: openo-beta/Open-O#2248 Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com> * fix: apply code review corrections to CSV migration Address critical issues and recommended improvements from code review: Critical Fixes (HIGH priority): - Fix resource leak in dxResearchLoadAssociations2Action.java by wrapping FileReader and CSVParser in try-with-resources block - Remove HTML encoding from CSV output in ClinicalReports.jsp to prevent data corruption (CSV should contain raw data, not HTML entities) - Remove duplicate import of java.util.List in dxResearchLoadAssociations2Action.java Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I believe the "uncontrolled data used in path expression" issue flagged by Copilot still needs to be fixed. This should get another look when someone has a chance. |
4 participants
Summary
This PR migrates the OpenO EMR codebase from the 21-year-old, unmaintained ostermillerutils 1.4.3 library to the actively maintained Apache Commons CSV 1.12.0.
Changes
Files Modified (9 files)
Java Files (6):
EcaresForm2Action.java- eCARES form CSV exportSQLReporter.java- SQL query result CSV exportdxResearchLoadAssociations2Action.java- DX research association CSV import/exportGenerateOutFiles2Action.java- Report CSV to Excel conversionRptDownloadCSVServlet.java- Report CSV download with tab delimiterJSP Files (2):
ClinicalExport.jsp- Clinical report CSV/Excel exportClinicalReports.jsp- Clinical report CSV generationConfiguration:
pom.xml- Added Commons CSV 1.12.0, removed ostermillerutils 1.4.3docs/archive/3RDPARTYLICENSES- Removed ostermillerutils GPL licenseMigration Details
API Changes:
ExcelCSVPrinter→CSVPrinterwithCSVFormat.EXCELExcelCSVParser→CSVParserwithCSVFormat.EXCELCSVPrinter.write()→CSVPrinter.print()CSVPrinter.writeln()→CSVPrinter.println()CSVPrinter.changeDelimiter('\t')→CSVFormat.withDelimiter('\t')CSVParser.parse()→ Instance-based parsing withCSVRecorditerationWhy This Change?
Testing
Manual Testing Required:
Note: Comprehensive integration tests are deferred to follow-up work due to the 4-5 day estimated complexity. This PR focuses on the core migration with functional equivalence.
Related Issues
Fixes #2153
Part of #2139 (Category 1: Critical - End-of-Life Libraries)
References
Generated with Claude Code
Summary by cubic
Migrated all CSV parsing and writing from ostermillerutils 1.4.3 to Apache Commons CSV 1.12.0. Removes the GPL-licensed, unmaintained library, aligns with #2153 to retire EOL dependencies, and addresses minor security and resource issues.
Refactors
Dependencies
Written for commit f1d9548. Summary will update on new commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.