chore: migrate javax.* to Jakarta EE namespace (big bang)#652
Closed
chore: migrate javax.* to Jakarta EE namespace (big bang)#652
Conversation
Bulk migration of the entire CARLOS EMR codebase from javax.* to
jakarta.* namespace, upgrading all frameworks to Jakarta EE 10
compatible versions.
**Dependency upgrades (pom.xml):**
- Spring Framework 5.3.39 → 6.2.17
- Hibernate ORM 5.6.15 → 6.6.40.Final (org.hibernate.orm groupId)
- Apache Struts 6.8.0 → 7.1.1 GA
- Apache CXF 3.6.9 → 4.1.5
- Servlet API 4.0.1 → 6.0.0 (jakarta.servlet)
- JSP API 2.3.3 → 3.1.0 (jakarta.servlet.jsp)
- JSTL 1.2.5 → 3.0.1 (jakarta.servlet.jsp.jstl)
- JAXB API 2.3.1 → 4.0.2, Runtime 2.3.9 → 4.0.5
- Annotations API 1.3.2 → 2.1.1 (jakarta.annotation)
- Inject API 1 → 2.0.1 (jakarta.inject)
- Mail: com.sun.mail → org.eclipse.angus:angus-mail:2.0.3
- jaxws-ri 2.3.7 → 4.0.3, saaj-impl 1.5.3 → 3.0.4
- DisplayTag 2.9.0 → 3.7.0, JavaMelody 1.99.4 → 2.6.0
- CSRFGuard 4.5.0 → 4.5.0-jakarta
**Java imports (~1,629 files):**
- javax.servlet → jakarta.servlet
- javax.persistence → jakarta.persistence
- javax.xml.bind → jakarta.xml.bind
- javax.xml.ws → jakarta.xml.ws, javax.xml.soap → jakarta.xml.soap
- javax.ws.rs → jakarta.ws.rs, javax.jws → jakarta.jws
- javax.annotation.{PostConstruct,PreDestroy,Resource,Nonnull} → jakarta.annotation.*
- javax.inject → jakarta.inject
- javax.mail → jakarta.mail, javax.activation → jakarta.activation
**Struts 7 migration (~410 files):**
- com.opensymphony.xwork2.* → org.apache.struts2.*
- struts.xml DTD: struts-6.0.dtd → struts-7.0.dtd
**JSP JSTL taglib URIs (~681 files):**
- http://java.sun.com/jsp/jstl/core → jakarta.tags.core
- http://java.sun.com/jsp/jstl/fmt → jakarta.tags.fmt
- http://java.sun.com/jsp/jstl/functions → jakarta.tags.functions
**Config files:**
- web.xml: Jakarta EE 6.0 namespace
- persistence.xml: Jakarta Persistence 3.0 namespace
- Dockerfile: Tomcat 9.0.97 → 10.1 (Jakarta Servlet 6.0)
**JDK javax packages preserved (not migrated):**
javax.crypto, javax.net.ssl, javax.security.auth, javax.imageio,
javax.naming, javax.sql, javax.xml.parsers, javax.xml.xpath,
javax.xml.transform, javax.swing, etc.
https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
- Jackson: jackson-module-jaxb-annotations → jackson-module-jakarta-xmlbind-annotations - Jackson: jackson-jaxrs-json-provider → jackson-jakarta-rs-json-provider - applicationContextREST.xml: JaxbAnnotationIntrospector → JakartaXmlBindAnnotationIntrospector - applicationContextREST.xml: JacksonJaxbJsonProvider → JacksonJsonProvider (Jakarta variant) - JasperReports: 6.21.5 → 7.0.3 (Jakarta EE 10 compatible) - Fixed 19 JSPF files with old JSTL taglib URIs (sed missed .jspf extension) - Fixed 4 JSP files with remaining javax references in scriptlets: - renal/*.jsp: javax.xml.bind → jakarta.xml.bind - admin/providerAddRole.jsp: javax.persistence → jakarta.persistence - provider/providerupdatepreference.jsp: javax.persistence → jakarta.persistence https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
Replace deprecated KieHelper (internal API) with standard KieServices/ KieFileSystem/KieBuilder pipeline. Replace 4 individual Drools deps (kie-api, drools-core, drools-compiler, drools-mvel) with single drools-engine aggregator. Remove mvel2 version override (managed by Drools 10). Use UUID-based ReleaseId for thread-safe concurrent DRL compilation. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
Update KieHelper references in CLAUDE.md, DroolsHelperUnitTest JavaDoc, and RuleBaseCreator JavaDoc to reflect the Drools 10 standard KIE API. Fix ReleaseId group ID to use underscore (io.github.carlos_emr) matching the project package convention. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
…tale refs Dispose KieContainer and remove KieModule from global KieRepository after extracting KieBase in DroolsHelper.createKieBaseFromDrl(). Without this, each compilation permanently registers a KieModule in the singleton KieRepository, causing unbounded heap growth in long-running servers. Also update remaining Drools 7.74.1 version references across 11 files (DroolsNumerator 1/2/4/5, PreventionDS, PreventionDSImpl, WorkFlowDSFactory, RuleBaseCreator, and 3 test files) to reflect Drools 10.0.0. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
…ation Plain buildAll() defaults to DrlProject (non-executable model), which is deprecated in Drools 10. Explicitly pass ExecutableModelProject.class to opt into the executable model compiler provided by drools-engine. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
- OscarMySQL5Dialect: extend MySQLDialect (Hibernate 6) instead of removed MySQL5Dialect; remove registerHibernateType calls (handled natively by Hibernate 6) - DroolsHelper: wrap KIE pipeline in try/finally to prevent KieModule leak on exception; catch RuntimeException from executable model compiler and wrap as DroolsCompilationException for callers - CLAUDE.md: update all stale version references (Spring 6.2.17, Hibernate 6.6.40, Struts 7.1.1, Tomcat 10.1, CXF 4.1.5) - Drools docs: fix 3 stale KieHelper references, DRL count (38→39), broken ToC anchor - Fix stale comments in CarlosWebTestBase (Struts 6.x→7.x), DrlCompilationIntegrationTest (Drools 7.x→10.x), copilot-instructions.md, docs/README.md, pom.xml https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
Plan for upgrading JasperReports usage to be fully JR7-compatible: - Add missing extension artifacts (jasperreports-pdf, jasperreports-jdt) - Fix 4 Java files with broken imports/removed APIs - Convert 40 JRXML templates from v6 to v7 format - Delete/recompile 3 stale .jasper binary files https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
…dencies Complete JasperReports 6.x → 7.x migration for Jakarta EE 10 compatibility: **pom.xml**: Add split extension artifacts required by JR7: - jasperreports-pdf (JRPdfExporter moved from core) - jasperreports-jdt (runtime JRXML compilation) - jasperreports-excel-poi (JRXlsxExporter moved from core) **Java API fixes** (4 files): - PdfRecordPrinter: Replace removed JRExporter/JRExporterParameter API with modern SimpleExporterInput/SimpleOutputStreamExporterOutput - FrmBCAR20202Action, FrmRourke2017Record, FrmRourke2020Record: Update JRPdfExporter import (engine.export → pdf package) **JRXML format conversion** (40 files): - Convert all templates from JR6 Digester XML to JR7 Jackson XML format - Element tags (staticText, textField, image, etc.) → <element kind="..."> - Flatten reportElement/textElement/font into element attributes - Rename expressions (textFieldExpression → expression, etc.) - Restructure band sections (remove wrapper for single-band sections) - Preserve CDATA sections in all expressions **Pre-compiled .jasper files** (3 files removed): - Delete JR6-binary .jasper files (incompatible with JR7) - Update subreport references to load .jrxml source at runtime **Converter tool**: Add scripts/convert_jrxml_v6_to_v7.py for future use https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
…ormat The initial JR7 migration only converted .jrxml files. 12 JasperReports templates with .xml extensions (labels, day sheets, demographic forms, BC billing) were still in JR6 Digester format and would fail at runtime. Also updates the converter script to accept .xml files when explicitly specified, and fixes label-appt.xml XML declaration ordering. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
…ibility The Spring modules had explicit version 5.3.39 (javax.* namespace) that overrode the spring-framework-bom 6.2.17 already declared in dependencyManagement. Spring 5.x is incompatible with the Jakarta EE 10 stack (Tomcat 10.1, Hibernate 6, Struts 7, CXF 4.1.5) already in use. All 9 Spring modules updated: spring-core, spring-tx, spring-orm, spring-webmvc, spring-web, spring-aspects, spring-test, spring-aop, spring-context-support. No code changes required — codebase already uses jakarta.* imports. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
- commons-io 2.18.0 → 2.22.0 - jsoup 1.17.2 → 1.22.1 - bcpkix-jdk18on 1.79 → 1.83 Minor version bumps, no API changes required. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
- JAXB namespace: java.sun.com/xml/ns/jaxb → jakarta.ee/xml/ns/jaxb - DatatypeConverter: javax.xml.bind → jakarta.xml.bind - Binding version: 1.0 → 3.0 Aligns the CXF client stub generation tooling with the Jakarta EE 10 stack already used by all production code. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
Replace Apache Commons FileUpload 1.6.0 with the built-in Jakarta Servlet 6.0 Part API (HttpServletRequest.getParts()), eliminating the third-party dependency entirely. Files migrated: - CarlosCsrfGuardFilter: replace ServletFileUpload.isMultipartContent() with a simple content-type check - DocumentMgtUploadServlet: migrate from deprecated DiskFileUpload to Part API, add PathValidationUtils for path traversal prevention - DocumentUploadServlet: migrate from ServletFileUpload/DiskFileItemFactory to Part API (PathValidationUtils already present) - UploadImage: migrate from deprecated DiskFileUpload to Part API, add PathValidationUtils and OWASP encoding for XSS prevention - DocumentTeleplanReportUploadServlet (BC): migrate from deprecated DiskFileUpload to Part API, add PathValidationUtils Also adds multipart-config to web.xml for all 3 registered upload servlets (50 MB file/request limit, 1 MB memory threshold) and removes the commons-fileupload dependency from pom.xml. Removes ~270 lines of dead commented-out legacy multipart parsing code from DocumentMgtUploadServlet and DocumentTeleplanReportUploadServlet. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
Spring 6 auto-detects @PersistenceContext and @PersistenceUnit annotations via CommonAnnotationBeanPostProcessor, making the explicit PersistenceAnnotationBeanPostProcessor bean definition unnecessary. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
Covers five phases: JUnit 4 test migration (blocker), Jakarta EE 11 API version bumps, Spring Framework 7.0.6 upgrade, Tomcat 11 container switch, and full verification. Includes risk assessment and dependency compatibility matrix. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
The 362 legacy JUnit 4 tests should already have been migrated to JUnit 5. Restructured plan to list this as a prerequisite rather than a separate phase, reducing the upgrade to 4 phases: API bumps, Spring 7, Tomcat 11, and verification. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
The JUnit 4 to 5 migration was completed in PR #591 (commit e78a564): - 334 legacy tests migrated to JUnit 5 in src/test-modern/ (450 files) - src/test/java/ cleared of all Java files - 24 deferred tests tracked for future recreation - Plan now shows all prerequisites as met, 4 clean phases remain https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
|
Important Review skippedToo many files! This PR contains 300 files, which is 150 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (300)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import sys | ||
| import os | ||
| import re | ||
| import glob |
| import re | ||
| import glob | ||
| import argparse | ||
| import copy |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| MiscUtils.getLogger().debug(fileheader + " uploaded to " + foldername); | ||
|
|
||
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 2 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
|
|
||
| item.write(savedFile); | ||
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix uncontrolled path usage for uploads you must: (1) normalize and sanitize the user‑provided name to remove path components and reject suspicious patterns, and (2) ensure the final resolved path is strictly contained within a predefined directory that you control and that directory is itself validated.
In this codebase, the best fix with minimal behavior change is to (a) harden PathValidationUtils.validatePath so it validates that allowedDir exists and is a directory, and so it explicitly ensures the constructed file is inside that directory via a canonical containment check, and (b) in DocumentTeleplanReportUploadServlet, ensure the configured DOCUMENT_DIR is validated before being used. The core behavior (accepting uploads into DOCUMENT_DIR under the submitted filename) remains unchanged from the caller’s perspective, but we add strong guards around the directory configuration and the final path.
Concretely:
-
In
PathValidationUtils.validatePath(...):- Before building the
File path, verify thatallowedDiris non‑null, exists, and is a directory; otherwise throwSecurityException. - After constructing
File path = new File(allowedDir, safeName);, call the already‑present containment logic (validateWithinDirectory(path, allowedDir)) to enforce that the canonical path lies withinallowedDir. Although we don’t seevalidateWithinDirectoryin the snippet, it is referenced and presumably usesisWithinDirectory(...). If it doesn’t currently do so, we adjust it in the shown region to callisWithinDirectoryand throw aSecurityExceptionupon failure. - This ensures that even if CodeQL does not model
sanitizeFileName, the final sink is protected by a strong containment check.
- Before building the
-
In
DocumentTeleplanReportUploadServlet.service(...):- After
File documentDir = new File(foldername);, validate thatdocumentDirexists and is a directory. If not, log an error and abort the request (for example, by returning before processing parts). This prevents misconfiguration (such asDOCUMENT_DIRpointing to/or to a non‑directory) from silently causing files to be stored in undesired places or causing errors.
- After
These changes require no new external dependencies; they only use File checks and the existing helper methods.
Suggested changeset
2
src/main/java/io/github/carlos_emr/carlos/billings/ca/bc/MSP/DocumentTeleplanReportUploadServlet.java
| @@ -63,6 +63,10 @@ | ||
| if (forwardTo == null || forwardTo.length() < 1) return; | ||
|
|
||
| File documentDir = new File(foldername); | ||
| if (!documentDir.exists() || !documentDir.isDirectory()) { | ||
| MiscUtils.getLogger().error("Invalid DOCUMENT_DIR configuration for Teleplan upload: {}", foldername); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| for (Part part : request.getParts()) { |
src/main/java/io/github/carlos_emr/carlos/utility/PathValidationUtils.java
Outside changed files
| @@ -64,7 +64,8 @@ | ||
| * <p>Performs the following validations:</p> | ||
| * <ol> | ||
| * <li>Sanitizes the user-provided filename (strips path components, rejects hidden files)</li> | ||
| * <li>Validates the resulting path is within the allowed directory</li> | ||
| * <li>Ensures that the allowed directory exists and is a directory</li> | ||
| * <li>Validates the resulting path is within the allowed directory (canonical containment check)</li> | ||
| * </ol> | ||
| * | ||
| * @param userProvidedFileName the filename provided by the user | ||
| @@ -73,14 +74,22 @@ | ||
| * @throws SecurityException if validation fails | ||
| */ | ||
| public static File validatePath(String userProvidedFileName, File allowedDir) { | ||
| // 1. Sanitize filename | ||
| String safeName = sanitizeFileName(userProvidedFileName); | ||
| if (allowedDir == null) { | ||
| throw new SecurityException("Allowed directory must not be null"); | ||
| } | ||
| if (!allowedDir.exists() || !allowedDir.isDirectory()) { | ||
| logger.error("Allowed directory is invalid: {}", allowedDir); | ||
| throw new SecurityException("Allowed directory is invalid"); | ||
| } | ||
|
|
||
| // 2. Build and validate path | ||
| File path = new File(allowedDir, safeName); | ||
| validateWithinDirectory(path, allowedDir); | ||
| // 1. Sanitize filename | ||
| String safeName = sanitizeFileName(userProvidedFileName); | ||
|
|
||
| return path; | ||
| // 2. Build and validate path | ||
| File path = new File(allowedDir, safeName); | ||
| validateWithinDirectory(path, allowedDir); | ||
|
|
||
| return path; | ||
| } | ||
|
|
||
| /** |
Copilot is powered by AI and may make mistakes. Always verify output.
| upload.setFileSizeMax(52428800); // 50 MB per file | ||
| upload.setSizeMax(52428800); // 50 MB total request size | ||
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
General approach: ensure that any file path derived from user input is fully validated immediately before use, ideally by a function that (a) canonicalizes the path, (b) guarantees it is within a trusted directory, and (c) rejects anything invalid by throwing. We already have PathValidationUtils.validatePath for construction and validateExistingPath / isWithinDirectory logic for containment checks.
Best concrete fix here: after constructing savedFile with validatePath(submittedFilename, documentDir), perform a second explicit validation before calling Files.copy, using validateExistingPath(savedFile, documentDir). Even though validatePath already uses validateWithinDirectory, the extra check right before the sink (1) makes the taint chain more obviously broken to CodeQL and (2) defends against any future refactorings of validatePath. The write then targets the revalidated File, not an unchecked path.
Changes needed:
- File:
src/main/java/io/github/carlos_emr/DocumentUploadServlet.java- In the upload loop (the
elsebranch wherePartobjects are handled), directly before thetry (InputStream in = part.getInputStream()) { ... }block, insert a call toPathValidationUtils.validateExistingPath(savedFile, documentDir);and assign its result to a new local (e.g.validatedSavedFile). - Use
validatedSavedFile.toPath()instead ofsavedFile.toPath()in theFiles.copycall. - Optionally, also use
validatedSavedFilewhen copying to the inbox directory to keep all subsequent usage tied to a validated instance (even though the path is the same, this is clearer and slightly more robust).
- In the upload loop (the
No new imports are needed: PathValidationUtils is already imported, and the new code only calls an existing method in that class.
Suggested changeset
1
src/main/java/io/github/carlos_emr/DocumentUploadServlet.java
| @@ -154,13 +154,16 @@ | ||
|
|
||
| fileheader = savedFile.getName(); | ||
|
|
||
| // Re-validate the resolved path just before use to ensure it is within documentDir | ||
| File validatedSavedFile = PathValidationUtils.validateExistingPath(savedFile, documentDir); | ||
|
|
||
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); | ||
| Files.copy(in, validatedSavedFile.toPath()); | ||
| } | ||
|
|
||
| if (OscarProperties.getInstance().isPropertyActive("moh_file_management_enabled")) { | ||
| File inboxDir = new File(inboxFolder); | ||
| FileUtils.copyFileToDirectory(savedFile, inboxDir); | ||
| FileUtils.copyFileToDirectory(validatedSavedFile, inboxDir); | ||
| } | ||
| } catch (SecurityException e) { | ||
| MiscUtils.getLogger().error("Invalid uploaded filename: " + submittedFilename); |
Copilot is powered by AI and may make mistakes. Always verify output.
|
|
||
| item.write(savedFile); | ||
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix uncontrolled path usage for uploaded filenames, you should (1) treat the original filename as untrusted, (2) reduce it to a single safe name component (no directories), (3) enforce an explicit policy on allowed characters and length, and (4) ensure the final resolved path stays within a configured base directory using canonical-path checks. Any violation should cause the upload to be rejected before performing file I/O.
In this codebase, the best minimal fix is to harden PathValidationUtils so that it becomes a clear, strong sanitizer that CodeQL (and reviewers) can treat as a trust boundary. Concretely:
- Keep using
FilenameUtils.getNameto strip directories. - Extend
sanitizeFileNameto (a) reject any filename containing path separators or".."just in case (even aftergetName), and (b) enforce a conservative allow‑list regex (for example, letters, digits, spaces, dots, underscores, dashes, limited length). - Ensure that
validatePathcannot silently succeed with an invalid directory: ifallowedDiris null or not a directory, throw aSecurityException. - Ensure that
validateWithinDirectoryalways throws viavalidateWithinDirectory(path, allowedDir)when the file is outside the allowed directory, and thatisWithinDirectoryhandles I/O errors by failing closed.
The servlet DocumentMgtUploadServlet already calls PathValidationUtils.validatePath(timestampedName, documentDir) and catches SecurityException, so we do not need to change its behavior: we simply make validatePath stricter. All changes are confined to PathValidationUtils.java (in the code we’ve been shown), specifically around validatePath, sanitizeFileName, and isWithinDirectory. No new external libraries are required beyond the existing Apache Commons IO.
Suggested changeset
1
src/main/java/io/github/carlos_emr/carlos/utility/PathValidationUtils.java
Outside changed files
| @@ -73,14 +73,21 @@ | ||
| * @throws SecurityException if validation fails | ||
| */ | ||
| public static File validatePath(String userProvidedFileName, File allowedDir) { | ||
| // 1. Sanitize filename | ||
| String safeName = sanitizeFileName(userProvidedFileName); | ||
| if (allowedDir == null) { | ||
| throw new SecurityException("Allowed directory is not configured"); | ||
| } | ||
| if (!allowedDir.isDirectory()) { | ||
| throw new SecurityException("Allowed directory is not a directory: " + allowedDir); | ||
| } | ||
|
|
||
| // 2. Build and validate path | ||
| File path = new File(allowedDir, safeName); | ||
| validateWithinDirectory(path, allowedDir); | ||
| // 1. Sanitize filename | ||
| String safeName = sanitizeFileName(userProvidedFileName); | ||
|
|
||
| return path; | ||
| // 2. Build and validate path | ||
| File path = new File(allowedDir, safeName); | ||
| validateWithinDirectory(path, allowedDir); | ||
|
|
||
| return path; | ||
| } | ||
|
|
||
| /** | ||
| @@ -261,7 +266,7 @@ | ||
| throw new SecurityException("Filename is null or empty"); | ||
| } | ||
|
|
||
| // Use Apache Commons IO to extract just the filename | ||
| // Use Apache Commons IO to extract just the filename (strip any path components) | ||
| String baseName = FilenameUtils.getName(fileName); | ||
|
|
||
| // Reject hidden files (starting with .) | ||
| @@ -276,6 +281,23 @@ | ||
| throw new SecurityException("Invalid filename"); | ||
| } | ||
|
|
||
| // Reject any remaining path traversal indicators or separators | ||
| if (baseName.contains("..") || baseName.contains("/") || baseName.contains("\\\\")) { | ||
| logger.warn("Path traversal or separator detected in filename after sanitization: {}", baseName); | ||
| throw new SecurityException("Invalid filename"); | ||
| } | ||
|
|
||
| // Enforce a conservative allow-list: letters, digits, space, dot, underscore, dash | ||
| // and a reasonable length limit to avoid resource abuse. | ||
| if (baseName.length() > 255) { | ||
| logger.warn("Filename too long: {} ({} characters)", baseName, baseName.length()); | ||
| throw new SecurityException("Invalid filename: too long"); | ||
| } | ||
| if (!baseName.matches("[A-Za-z0-9._\\- ]+")) { | ||
| logger.warn("Filename contains disallowed characters: {}", baseName); | ||
| throw new SecurityException("Invalid filename"); | ||
| } | ||
|
|
||
| return baseName; | ||
| } | ||
|
|
||
| @@ -294,6 +316,7 @@ | ||
| return canonicalPath.equals(dirCanonical) || canonicalPath.startsWith(dirCanonical + File.separator); | ||
| } catch (IOException e) { | ||
| logger.error("Error checking if file is within directory", e); | ||
| // Fail closed on I/O errors: treat as not within directory | ||
| return false; | ||
| } | ||
| } |
Copilot is powered by AI and may make mistakes. Always verify output.
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant architectural overhaul, moving the CARLOS EMR system to the Jakarta EE 10 ecosystem. The primary goal was to modernize the application's underlying frameworks and APIs, ensuring compatibility with the latest Java enterprise standards. This extensive migration involved updating numerous third-party libraries, refactoring code to adopt the new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Re-enable session tracking before request cleanup
This class still exposes trackSession()/releaseThreadSessions(), but after the migration the SessionFactory wrapper that called trackSession() was removed, leaving trackSession() with no call sites while DbConnectionFilter still invokes request-end cleanup. That effectively disables the leak-safety mechanism and means lingering sessions opened during request processing are no longer auto-closed by this path.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| upload.setFileSizeMax(52428800); // 50 MB per file | ||
| upload.setSizeMax(52428800); // 50 MB total request size | ||
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); |
There was a problem hiding this comment.
Files.copy(in, savedFile.toPath()) throws FileAlreadyExistsException when a user uploads a file whose name already exists in the destination directory; this code catches IOException and continues, so the upload is silently skipped while the request still forwards normally. This breaks re-upload/update workflows for recurring filenames unless overwrite or explicit conflict handling is added.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request performs a significant upgrade of the application's Java ecosystem to Jakarta EE 10 compatibility, including updating Tomcat to 10.1, Spring Framework to 6.2.17, Struts to 7.1.1, Hibernate to 6.6.40, Drools to 10.0.0, and JasperReports to 7.0.3. This involved migrating numerous javax.* imports to their jakarta.* equivalents across many Java files and updating various dependencies in pom.xml. The file upload servlets (DocumentMgtUploadServlet.java and DocumentUploadServlet.java) were modernized to use the standard Jakarta Servlet Part API, replacing the deprecated Apache Commons FileUpload library. Review comments highlight a security vulnerability in DocumentMgtUploadServlet.java due to missing file size limits for uploads, suggesting the addition of @MultipartConfig annotation. Additionally, a structural improvement opportunity was identified for end_year_statement_report.jrxml and its subreport, recommending moving these resource files from src/main/java to src/main/resources to adhere to Maven standards and improve build/packaging. A performance concern was also raised for the JasperReports subreport, which currently points to the .jrxml source file, leading to runtime compilation overhead; pre-compiling it to a .jasper file is recommended for better performance.
| * @throws IOException if an I/O error occurs | ||
| * @throws ServletException if a servlet error occurs | ||
| */ | ||
| public void service(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { |
Contributor
There was a problem hiding this comment.
The update to use the standard Servlet Part API is a great modernization. However, the servlet does not enforce any file size limits for uploads. This could leave the application vulnerable to Denial of Service (DoS) attacks via large file uploads.
Consider adding the @MultipartConfig annotation to the servlet to enforce reasonable size limits.
Example:
import jakarta.servlet.annotation.MultipartConfig;
@MultipartConfig(
maxFileSize = 1024 * 1024 * 10, // 10 MB
maxRequestSize = 1024 * 1024 * 10 // 10 MB
)
public class DocumentMgtUploadServlet extends HttpServlet { /* ... */ }This would enhance the security of the file upload functionality.
| @@ -1,513 +1,245 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
Contributor
There was a problem hiding this comment.
This JasperReports template file (.jrxml) and its associated subreport are located in the src/main/java source directory. According to Maven standard directory layout, resource files like this should be placed in src/main/resources. The .plan file for the JasperReports migration also indicates that .jasper files were previously in src/main/resources.
Keeping resources in the Java source tree can lead to build and packaging issues. Please consider moving these .jrxml files to src/main/resources/io/github/carlos_emr/carlos/billings/ca/on/reports/.
| <expression><![CDATA[$F{id}]]></expression> | ||
| </parameter> | ||
| <connectionExpression><![CDATA[$P{REPORT_CONNECTION}]]></connectionExpression> | ||
| <expression><![CDATA[$P{SUBREPORT_DIR} + "end_year_statement_subreport.jrxml"]]></expression> |
Contributor
There was a problem hiding this comment.
The subreport expression now points to the .jrxml source file instead of a pre-compiled .jasper file. This means the subreport will be compiled at runtime every time the main report is generated, which can negatively impact performance.
The plan.md file for JasperReports migration, also added in this PR, recommends build-time pre-compilation to avoid this runtime overhead. To improve performance, please consider pre-compiling the subreport to a .jasper file and referencing that here instead.
<expression><![CDATA[$P{SUBREPORT_DIR} + "end_year_statement_subreport.jasper"]]></expression>
Migrate CARLOS EMR from Spring 6.2/Hibernate 6.6/Tomcat 10.1 to Spring 7.0.6/Hibernate 7.2.6/Tomcat 11.0 (Jakarta EE 11 baseline). pom.xml version bumps: - Spring Framework: 6.2.17 → 7.0.6 - Spring Security: 6.3.9 → 7.0.3 - Hibernate ORM: 6.6.40.Final → 7.2.6.Final - JUnit Jupiter: 5.10.1 → 6.0.3 - Servlet API: 6.0.0 → 6.1.0 - JSP API: 3.1.0 → 4.0.0 - JPA API: 3.1.0 → 3.2.0 - Annotation API: 2.1.1 → 3.0.0 - Commons Logging: 1.2 → 1.3.5 - Removed JUnit 4 dependency entirely Spring 7 breaking changes addressed: - org.springframework.orm.hibernate5 → org.springframework.orm.jpa.hibernate (LocalSessionFactoryBean relocated in Spring 7) - HibernateTemplate removed from Spring 7; created test-only replacement at test.base.HibernateTemplate wrapping SessionFactory.getCurrentSession() Hibernate 7 Session API migration (27 DAO files, 65+ call sites): - Session.save() → Session.persist() - Session.update() → Session.merge() - Session.saveOrUpdate() → Session.merge() - Session.delete() → Session.remove() - Fixed persist() void return in CaseManagementNoteDAOImpl Dockerfile: Tomcat 10.1 → 11.0 (Servlet 6.1 support) Test infrastructure: Updated 12 integration tests and 6 XML configs to use relocated Spring packages and new HibernateTemplate. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
…bernate 7 Session.createSQLQuery() was removed in Hibernate 6. Migrate the 2 calls in ProviderDaoIntegrationTest to createNativeQuery() to prevent compile errors. https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
# Conflicts: # src/main/webapp/library/bootstrap/3.0.0/data-tables.jsp # src/main/webapp/oscarReport/demographicSetEdit.jsp # src/test-modern/java/io/github/carlos_emr/carlos/PMmodule/dao/DefaultRoleAccessDAOIntegrationTest.java # src/test-modern/java/io/github/carlos_emr/carlos/PMmodule/dao/FormsDAOIntegrationTest.java # src/test-modern/java/io/github/carlos_emr/carlos/PMmodule/dao/ProgramProviderDAOIntegrationTest.java # src/test-modern/java/io/github/carlos_emr/carlos/PMmodule/dao/SecUserRoleDaoIntegrationTest.java # src/test-modern/java/io/github/carlos_emr/carlos/commn/dao/utils/EntityDataGenerator.java # src/test-modern/resources/META-INF/persistence.xml # src/test/java/io/github/carlos_emr/carlos/commn/dao/ProviderInboxRoutingDaoTest.java # src/test/java/io/github/carlos_emr/carlos/utility/TestClass.java
Contributor
Dependency ReviewThe following issues were found:
Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Vulnerabilitiespom.xml
License Issuespom.xml
OpenSSF ScorecardScorecard details
Scanned Files
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Signed-off-by: Michael Yingbull <michael@maplecreekmedical.ca>
Contributor
There was a problem hiding this comment.
Pull request overview
Bulk migration of CARLOS EMR from javax.* to jakarta.* plus related framework upgrades/migrations (Struts 7, Hibernate 6) and a few targeted refactors (notably multipart upload handling and some DI annotations).
Changes:
- Updated Java/JSP/JAXB/JAX-WS/JAX-RS imports to the Jakarta EE namespaces and moved Struts actions to
org.apache.struts2.*. - Replaced a number of Hibernate
SessionCRUD calls (save,update,delete,saveOrUpdate) withpersist,merge, andremove. - Refactored multipart upload servlets away from Apache Commons FileUpload toward the Jakarta Servlet
PartAPI; devcontainer Tomcat image bumped.
Reviewed changes
Copilot reviewed 297 out of 2554 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteDAOImpl.java | Changes persistence semantics (save→persist, update→merge) and alters saveAndReturn behavior. |
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/IssueDAOImpl.java | Replaces saveOrUpdate/delete with merge/remove. |
| src/main/java/io/github/carlos_emr/DocumentUploadServlet.java | Migrates multipart upload handling to Part + Files.copy and path validation. |
| src/main/java/io/github/carlos_emr/carlos/billings/ca/bc/MSP/DocumentTeleplanReportUploadServlet.java | Migrates multipart upload handling to Part + Files.copy and path validation. |
| src/main/java/io/github/carlos_emr/carlos/app/CarlosCsrfGuardFilter.java | Removes commons-fileupload dependency; adds custom multipart detection. |
| src/main/java/io/github/carlos_emr/carlos/PMmodule/service/ClientManagerImpl.java | Replaces @Required with @Autowired on multiple setters, including a primitive boolean setter. |
| .devcontainer/development/Dockerfile | Updates devcontainer Tomcat base image (now Tomcat 11) and copy stage. |
| src/main/java/io/github/carlos_emr/carlos/billings/ca/on/reports/end_year_statement_subreport.jrxml | Refactors JasperReports jrxml structure. |
You can also share your feedback on Copilot code review. Take the survey.
| } | ||
| return currentSession().save(note); | ||
| currentSession().persist(note); | ||
| return note; |
| @Override | ||
| public void saveIssue(Issue issue) { | ||
| currentSession().saveOrUpdate(issue); | ||
| currentSession().merge(issue); |
Comment on lines
+144
to
148
| for (Part part : request.getParts()) { | ||
| String submittedFilename = part.getSubmittedFileName(); | ||
| if (submittedFilename == null || submittedFilename.isEmpty()) { | ||
| continue; | ||
| } |
Comment on lines
+157
to
+159
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); | ||
| } |
| upload.setFileSizeMax(52428800); // 50 MB per file | ||
| upload.setSizeMax(52428800); // 50 MB total request size | ||
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); |
| @@ -1,4 +1,4 @@ | |||
| FROM tomcat:9.0.97-jdk21-temurin | |||
| FROM tomcat:11.0-jdk21-temurin | |||
|
|
||
| # Copy Tomcat installation from the official Tomcat image | ||
| COPY --from=tomcat:9.0.97 /usr/local/tomcat $CATALINA_HOME | ||
| COPY --from=tomcat:11.0-jdk21-temurin /usr/local/tomcat $CATALINA_HOME |
|
|
||
| # Copy Tomcat installation from the official Tomcat image | ||
| COPY --from=tomcat:9.0.97 /usr/local/tomcat $CATALINA_HOME | ||
| COPY --from=tomcat:11.0-jdk21-temurin /usr/local/tomcat $CATALINA_HOME |
| </summary> | ||
| </jasperReport> | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <jasperReport name="end_year_end_subreport" columnCount="1" printOrder="Vertical" orientation="Portrait" pageWidth="595" pageHeight="842" columnWidth="535" columnSpacing="0" leftMargin="30" rightMargin="30" topMargin="20" bottomMargin="20" whenNoDataType="NoPages" isTitleNewPage="false" isSummaryNewPage="false"> |
Comment on lines
+22
to
+29
| <element kind="textField" x="0" y="0" width="120" height="18" key="textField" blankWhenNull="false" evaluationTime="Now" hyperlinkType="None" hyperlinkTarget="Self"> | ||
| <box /> | ||
| <expression><![CDATA[$F{service_code}]]></expression> | ||
| </element> | ||
| <element kind="textField" x="120" y="0" width="100" height="18" key="textField" blankWhenNull="false" evaluationTime="Now" hyperlinkType="None" hyperlinkTarget="Self"> | ||
| <box /> | ||
| <expression><![CDATA[$F{fee}]]></expression> | ||
| </element> |
Signed-off-by: Michael Yingbull <michael@maplecreekmedical.ca>
Signed-off-by: Michael Yingbull <michael@maplecreekmedical.ca>
Contributor
There was a problem hiding this comment.
Pull request overview
Bulk migration of CARLOS EMR from javax.* to jakarta.* with accompanying framework/runtime upgrades and a few behavioral refactors needed for Jakarta-compatible libraries.
Changes:
- Updated Java/JSP/Servlet/JPA imports to the Jakarta EE namespaces and migrated Struts actions to
org.apache.struts2.*. - Replaced several Hibernate
Sessionpersistence calls (save,update,saveOrUpdate,delete) with Hibernate 6/JPA-aligned APIs (persist,merge,remove). - Refactored multipart upload servlets away from Apache Commons FileUpload to the Jakarta Servlet
PartAPI, and updated devcontainer Tomcat image.
Reviewed changes
Copilot reviewed 297 out of 2585 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/IssueDAOImpl.java | Hibernate 6 migration: replace saveOrUpdate/delete with merge/remove. |
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/ClientImageDAOImpl.java | Hibernate 6 migration: replace saveOrUpdate/delete with merge/remove. |
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteLinkDAOImpl.java | Hibernate 6 migration: replace save/update with persist/merge. |
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteExtDAOImpl.java | Hibernate 6 migration: replace save/update with persist/merge. |
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteDAOImpl.java | Hibernate 6 migration; changed saveAndReturn behavior to return entity instead of save() result. |
| src/main/java/io/github/carlos_emr/carlos/billing/CA/BC/MSP/DocumentTeleplanReportUploadServlet.java | Reworked upload handling to request.getParts() + PathValidationUtils. |
| src/main/java/io/github/carlos_emr/DocumentUploadServlet.java | Reworked upload handling to request.getParts() + PathValidationUtils (removed Commons FileUpload). |
| src/main/java/io/github/carlos_emr/carlos/app/CarlosCsrfGuardFilter.java | Dropped Commons FileUpload dependency by adding a local multipart content-type check. |
| src/main/java/io/github/carlos_emr/carlos/billings/ca/on/reports/end_year_statement_subreport.jrxml | JasperReports template refactor to newer element syntax. |
| .devcontainer/development/Dockerfile | Updated devcontainer base image from Tomcat 9 to Tomcat 11. |
| plan.md | Added follow-up migration plan (Tomcat 11 + Spring 7 + Jakarta EE 11). |
| docs/README.md | Updated documented runtime stack versions after Jakarta migration. |
| CLAUDE.md | Updated documented runtime stack + Struts migration notes after Jakarta migration. |
| .github/copilot-instructions.md | Updated documented runtime stack versions after Jakarta migration. |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+465
to
+466
| currentSession().persist(note); | ||
| return note; |
Comment on lines
+144
to
+145
| for (Part part : request.getParts()) { | ||
| String submittedFilename = part.getSubmittedFileName(); |
Comment on lines
+157
to
+159
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); | ||
| } |
Comment on lines
+166
to
+169
| MiscUtils.getLogger().error("Invalid uploaded filename: " + submittedFilename); | ||
| continue; | ||
| } catch (IOException e) { | ||
| MiscUtils.getLogger().error("Error processing file: " + submittedFilename, e); |
Comment on lines
+183
to
+186
| private static boolean isMultipartContent(HttpServletRequest request) { | ||
| String contentType = request.getContentType(); | ||
| return contentType != null && contentType.toLowerCase().startsWith("multipart/"); | ||
| } |
| @@ -1,4 +1,4 @@ | |||
| FROM tomcat:9.0.97-jdk21-temurin | |||
| FROM tomcat:11.0-jdk21-temurin | |||
|
|
||
| # Copy Tomcat installation from the official Tomcat image | ||
| COPY --from=tomcat:9.0.97 /usr/local/tomcat $CATALINA_HOME | ||
| COPY --from=tomcat:11.0-jdk21-temurin /usr/local/tomcat $CATALINA_HOME |
Comment on lines
+12
to
+19
| <background height="0"> | ||
| </background> | ||
| <title height="0"> | ||
| </title> | ||
| <pageHeader height="0"> | ||
| </pageHeader> | ||
| <columnHeader height="0"> | ||
| </columnHeader> |
Signed-off-by: Michael Yingbull <michael@maplecreekmedical.ca>
Comprehensive fixes for runtime issues discovered during Jakarta EE migration testing. Build fixes: - Add jakarta.transaction-api 2.0.1 for Hibernate 7 - Fix Jackson JAX-RS/JAXB providers to Jakarta namespace (spring_ws.xml) - Add WebServiceContext bean for Spring 7 CXF integration - Fix Hibernate 7: remove @NotFound/@batchsize on @manytoone (Tickler) - Add @entity to HBM-only classes referenced by JPA associations - Configure JPA EMF with packagesToScan + mappingResources for Hibernate 7 - Fix Struts 7 DTD (6.5), fileUpload->actionFileUpload interceptor - Fix Struts 7 namespace: alwaysSelectFullNamespace=false, fallbackToEmptyNamespace=true Servlet 6.1 / Tomcat 11 fixes: - Replace session.getValue() with session.getAttribute() (107 occurrences, 102 JSPs) - Replace pageContext.forward() with RequestDispatcher.include() (20 JSPs) - Add out.clearBuffer() before include for Tomcat 11 buffer handling - Fix LoginFilter to treat context root "/" as index.jsp for exemptions - Fix CsrfGuardScriptInjectionFilter: skip .do wrapper, fix isCommitted/writeToResponse - Fix LogoutBroadcastFilter: remove response wrapper, add grace period - Remove dead Struts 1 TLD files and orphaned tag entries in caisi/oscar TLDs - Migrate encoder-jsp to encoder-jakarta-jsp (Jakarta JSP taglib) - Update OWASP encoder taglib URI to owasp.encoder.jakarta (21 JSPs) - Add csrfguard script to global-head.jspf and encounter layout Hibernate 7 HQL fixes: - Fix Integer vs String comparisons (OscarLogDaoImpl: demographicId != -1) - Fix Boolean vs Integer/String comparisons (AllergyDaoImpl, JointAdmissionDaoImpl, etc.) - Fix unqualified ORDER BY (OscarLogDaoImpl: dateTime -> l.created) - Add SELECT clause to multi-root HQL queries (~15 DAOs) - Fix ScheduleDateDaoImpl cross-join query Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Bulk migration of CARLOS EMR from javax.* to jakarta.*, plus framework/runtime upgrades to support Jakarta EE-based stacks (Struts 7, Hibernate 6, Servlet 6+) and related refactors (notably multipart upload handling and JasperReports templates).
Changes:
- Migrated servlet/JSP/JPA/JAXB/JAX-WS/JAX-RS imports to
jakarta.*across the codebase. - Updated Hibernate usage patterns (
save/update/delete→persist/merge/remove) in multiple DAOs. - Refactored multipart upload handling to use the Jakarta Servlet
PartAPI and updated devcontainer Tomcat image.
Reviewed changes
Copilot reviewed 297 out of 2653 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteDAOImpl.java | Replaced Hibernate save/update calls; adjusted saveAndReturn behavior. |
| src/main/java/io/github/carlos_emr/DocumentUploadServlet.java | Replaced Apache Commons FileUpload with Servlet Part upload flow and path validation. |
| src/main/java/io/github/carlos_emr/carlos/app/CarlosCsrfGuardFilter.java | Removed commons-fileupload dependency; added multipart detection helper. |
| src/main/java/io/github/carlos_emr/carlos/billings/ca/on/reports/end_year_statement_subreport.jrxml | Refactored JasperReports template to a new XML structure. |
| src/main/java/io/github/carlos_emr/carlos/PMmodule/model/ProgramTeam.java | Added JPA annotations (@Entity, @Table). |
| src/main/java/io/github/carlos_emr/carlos/PMmodule/model/ProgramClientStatus.java | Added JPA annotations (@Entity, @Table). |
| src/main/java/io/github/carlos_emr/carlos/billings/ca/on/administration/GstControl2Action.java | Migrated Struts packages and Servlet aware interfaces to Struts 7 locations. |
| .devcontainer/development/Dockerfile | Updated devcontainer Tomcat base image to Tomcat 11. |
You can also share your feedback on Copilot code review. Take the survey.
| } | ||
| return currentSession().save(note); | ||
| currentSession().persist(note); | ||
| return note; |
Comment on lines
+144
to
148
| for (Part part : request.getParts()) { | ||
| String submittedFilename = part.getSubmittedFileName(); | ||
| if (submittedFilename == null || submittedFilename.isEmpty()) { | ||
| continue; | ||
| } |
Comment on lines
+157
to
+159
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); | ||
| } |
Comment on lines
+12
to
+19
| <background height="0"> | ||
| </background> | ||
| <title height="0"> | ||
| </title> | ||
| <pageHeader height="0"> | ||
| </pageHeader> | ||
| <columnHeader height="0"> | ||
| </columnHeader> |
Comment on lines
+183
to
+186
| private static boolean isMultipartContent(HttpServletRequest request) { | ||
| String contentType = request.getContentType(); | ||
| return contentType != null && contentType.toLowerCase().startsWith("multipart/"); | ||
| } |
Comment on lines
+38
to
40
| @Entity | ||
| @Table(name = "program_team") | ||
| public class ProgramTeam implements Serializable { |
Comment on lines
+34
to
36
| @Entity | ||
| @Table(name = "program_clientstatus") | ||
| public class ProgramClientStatus implements Serializable { |
| @@ -1,4 +1,4 @@ | |||
| FROM tomcat:9.0.97-jdk21-temurin | |||
| FROM tomcat:11.0-jdk21-temurin | |||
|
|
||
| # Copy Tomcat installation from the official Tomcat image | ||
| COPY --from=tomcat:9.0.97 /usr/local/tomcat $CATALINA_HOME | ||
| COPY --from=tomcat:11.0-jdk21-temurin /usr/local/tomcat $CATALINA_HOME |
ConsultationRequest.status is String but was compared to Integer 4 in HQL query. Hibernate 7 strictly validates types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comprehensive sweep for remaining migration issues found by exhaustive codebase scanning. Fixes applied proactively before they cause runtime errors. Multi-root HQL without SELECT (5 queries): - BillingDaoImpl: 3 queries (Billing+Billingmaster, Billing+BillingDetail) - BillingmasterDAO: 1 query (Billing+Billingmaster) - DiagnosticCodeDaoImpl: 1 query (DiagnosticCode+CtlDiagCode) Boolean/String type mismatches (13 queries): - ConsentDaoImpl: deleted=0 → deleted=false (3 locations) - DrugDaoImpl: archived=0/1 → archived=false/true - HRMDocumentCommentDao: deleted=0 → deleted=false - ConsentTypeDaoImpl: active=1 → active=true (2 locations) - DemographicPharmacyDaoImpl: status=1 → status='1' (String field) - ConsultResponseDaoImpl: status!=4/5 → status!='4'/'5' (String field) - ConsultRequestDaoImpl: status!=4/5/7 → status!='4'/'5'/'7' (line 254) GROUP BY with full entity SELECT (2 queries): - CtlBillingServiceDaoImpl: replaced invalid GROUP BY with subquery - QuickListDaoImpl: replaced invalid GROUP BY with subquery Servlet 6.1: - TemplateFlowSheetPage.jspf: session.getValue() → session.getAttribute() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Bulk migration of CARLOS EMR from javax.* to jakarta.*, including upgrades/migrations for Struts 7 and Hibernate 6 APIs, plus a few behavioral changes to request/response filtering and file upload handling.
Changes:
- Updated Java/JSP/JAXB/JAX-WS imports to Jakarta EE namespaces and adjusted Struts action imports for Struts 7.
- Replaced deprecated Hibernate Session operations (
save,update,saveOrUpdate,delete) withpersist,merge, andremove. - Refactored selected infrastructure pieces (logout broadcast injection, CSRF multipart detection, servlet upload) and updated the devcontainer Tomcat image.
Reviewed changes
Copilot reviewed 297 out of 2653 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/io/github/carlos_emr/carlos/billings/ca/on/administration/GstControl2Action.java | Struts 7 interface/import migration; request/response injection method changes |
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteDAOImpl.java | Hibernate 6 API updates and HQL boolean literal changes; altered saveAndReturn behavior |
| src/main/java/io/github/carlos_emr/DocumentUploadServlet.java | Replaced Commons FileUpload with Jakarta Part API and changed upload I/O behavior |
| src/main/java/io/github/carlos_emr/carlos/app/CarlosCsrfGuardFilter.java | Removed Commons FileUpload dependency; added multipart content detection helper |
| src/main/resources/**/*.jrxml | JasperReports template refactors to new XML structure |
| .devcontainer/development/Dockerfile | Devcontainer Tomcat image bump and related Docker layering changes |
| docs/README.md, CLAUDE.md, .github/copilot-instructions.md | Documentation updates reflecting the new Jakarta/Spring/Struts/Tomcat baseline |
| (numerous other Java files) | Mechanical javax.* → jakarta.* import migration and Struts package updates |
Comments suppressed due to low confidence (1)
src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteDAOImpl.java:1
saveAndReturnpreviously returned the value fromSession.save(note)(typically the generated identifier). After this change it returns the entity instance instead, which is a behavioral/contract change and is likely to break callers expecting an id. If the intent is to return the generated id under Hibernate 6, persist the entity and returnnote.getId()(optionally afterflush()if required by your identifier strategy), or keep using an API that returns the identifier.
/**
You can also share your feedback on Copilot code review. Take the survey.
| import org.apache.struts2.action.ServletResponseAware; | ||
| import org.apache.struts2.interceptor.parameter.StrutsParameter; | ||
|
|
||
| public class GstControl2Action extends ActionSupport implements ServletRequestAware, ServletResponseAware { |
| private HttpServletResponse response; | ||
| @Override | ||
| public void setServletRequest(HttpServletRequest request) { | ||
| public void withServletRequest(HttpServletRequest request) { |
|
|
||
| @Override | ||
| public void setServletResponse(HttpServletResponse response) { | ||
| public void withServletResponse(HttpServletResponse response) { |
| if (issues.length > 1) { | ||
| List<Long> issueIdList = parseIssueIds(issues); | ||
| String hql = "select cmn from CaseManagementNote cmn join cmn.issues i where i.issue_id in (:issueIds) and cmn.demographic_no = :demoNo and cmn.archived = 0 and cmn.id = (select max(cmn2.id) from CaseManagementNote cmn2 where cmn.uuid = cmn2.uuid) ORDER BY cmn.position, cmn.observation_date desc"; | ||
| String hql = "select cmn from CaseManagementNote cmn join cmn.issues i where i.issue_id in (:issueIds) and cmn.demographic_no = :demoNo and cmn.archived = false and cmn.id = (select max(cmn2.id) from CaseManagementNote cmn2 where cmn.uuid = cmn2.uuid) ORDER BY cmn.position, cmn.observation_date desc"; |
| long id = Long.parseLong(issues[0]); | ||
|
|
||
| String hql = "select cmn from CaseManagementNote cmn join cmn.issues i where i.issue_id = :issueId and cmn.demographic_no = :demoNo and cmn.archived=0 order by cmn.position, cmn.observation_date desc"; | ||
| String hql = "select cmn from CaseManagementNote cmn join cmn.issues i where i.issue_id = :issueId and cmn.demographic_no = :demoNo and cmn.archived=false order by cmn.position, cmn.observation_date desc"; |
|
|
||
| # Copy Tomcat installation from the official Tomcat image | ||
| COPY --from=tomcat:9.0.97 /usr/local/tomcat $CATALINA_HOME | ||
| COPY --from=tomcat:11.0-jdk21-temurin /usr/local/tomcat $CATALINA_HOME |
|
|
||
| # Copy Tomcat installation from the official Tomcat image | ||
| COPY --from=tomcat:9.0.97 /usr/local/tomcat $CATALINA_HOME | ||
| COPY --from=tomcat:11.0-jdk21-temurin /usr/local/tomcat $CATALINA_HOME |
Comment on lines
+183
to
+186
| private static boolean isMultipartContent(HttpServletRequest request) { | ||
| String contentType = request.getContentType(); | ||
| return contentType != null && contentType.toLowerCase().startsWith("multipart/"); | ||
| } |
| <queryString><![CDATA[SELECT bi.`service_code`, bi.`fee` FROM `billing_on_item` bi WHERE ch1_id = $P{invoiceId} ORDER BY bi.service_code ASC]]></queryString> | ||
| <field name="service_code" class="java.lang.String" /> | ||
| <field name="fee" class="java.lang.String" /> | ||
| <background height="0"> |
| </columnHeader> | ||
| <detail> | ||
| <band height="18"> | ||
| <element kind="textField" x="0" y="0" width="120" height="18" key="textField" blankWhenNull="false" evaluationTime="Now" hyperlinkType="None" hyperlinkTarget="Self"> |
Signed-off-by: Michael Yingbull <michael@maplecreekmedical.ca>
- global-head.jspf: keep both csrfguard script and new transitions.css/carlos-ajax.js - SearchDrug3.jsp: use CarlosAjax.updater (handles CSRF automatically) instead of manual CSRF token injection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Bulk migration of CARLOS EMR from javax.* to jakarta.*, alongside major framework/container upgrades to Jakarta-compatible versions (Spring 6, Hibernate 6, Struts 7, Tomcat 10+/11).
Changes:
- Updated Java/JSP/JAX-* imports and Struts package locations to Jakarta/Struts 7 namespaces.
- Migrated Hibernate Session persistence calls away from removed APIs (
saveOrUpdate,update,delete) topersist/merge/remove. - Refactored specific runtime components (multipart upload handling, logout broadcast script injection, report templates, devcontainer Tomcat baseline).
Reviewed changes
Copilot reviewed 297 out of 2655 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/ClientImageDAOImpl.java | Hibernate Session API migration (saveOrUpdate→merge, delete→remove) |
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteLinkDAOImpl.java | Replace save/update with persist/merge |
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteExtDAOImpl.java | Replace save/update with persist/merge |
| src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteDAOImpl.java | HQL boolean literal changes + Session API migration + return-value change in saveAndReturn |
| src/main/java/io/github/carlos_emr/carlos/app/LogoutBroadcastFilter.java | Change response handling/injection strategy for Tomcat 11 compatibility |
| src/main/java/io/github/carlos_emr/carlos/app/CarlosCsrfGuardFilter.java | Remove commons-fileupload dependency; add local multipart detection |
| src/main/java/io/github/carlos_emr/DocumentUploadServlet.java | Switch from Commons FileUpload to Jakarta Servlet Part upload handling |
| src/main/java/io/github/carlos_emr/carlos/billings/ca/on/reports/end_year_statement_subreport.jrxml | JasperReports template refactor/new XML structure |
| src/main/java/io/github/carlos_emr/carlos/billings/ca/on/administration/GstControl2Action.java | Struts 7 package migration + request/response aware method changes |
| .devcontainer/development/Dockerfile | Devcontainer Tomcat baseline bump to 11 |
| plan.md | Forward-looking plan for Tomcat 11 / Spring 7 follow-up work |
| docs/README.md | Docs updated to reflect upgraded stack versions |
| CLAUDE.md | Docs updated to reflect upgraded stack versions + Struts 7 notes |
| .github/copilot-instructions.md | Docs updated to reflect upgraded stack versions + Struts 7 import example |
Comments suppressed due to low confidence (1)
src/main/java/io/github/carlos_emr/carlos/casemgmt/dao/CaseManagementNoteDAOImpl.java:1
saveAndReturnpreviously returned the value fromSession.save(...)(typically the generated identifier). It now returns the entity instance, which is a behavioral change likely to break callers expecting an ID/Serializable. Prefer returning the identifier (e.g.,note.getId()after a flush if needed) or rename/change the method contract to reflect returning the entity.
/**
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+12
to
+29
| <background height="0"> | ||
| </background> | ||
| <title height="0"> | ||
| </title> | ||
| <pageHeader height="0"> | ||
| </pageHeader> | ||
| <columnHeader height="0"> | ||
| </columnHeader> | ||
| <detail> | ||
| <band height="18"> | ||
| <element kind="textField" x="0" y="0" width="120" height="18" key="textField" blankWhenNull="false" evaluationTime="Now" hyperlinkType="None" hyperlinkTarget="Self"> | ||
| <box /> | ||
| <expression><![CDATA[$F{service_code}]]></expression> | ||
| </element> | ||
| <element kind="textField" x="120" y="0" width="100" height="18" key="textField" blankWhenNull="false" evaluationTime="Now" hyperlinkType="None" hyperlinkTarget="Self"> | ||
| <box /> | ||
| <expression><![CDATA[$F{fee}]]></expression> | ||
| </element> |
| @@ -1,4 +1,4 @@ | |||
| FROM tomcat:9.0.97-jdk21-temurin | |||
| FROM tomcat:11.0-jdk21-temurin | |||
|
|
||
| # Copy Tomcat installation from the official Tomcat image | ||
| COPY --from=tomcat:9.0.97 /usr/local/tomcat $CATALINA_HOME | ||
| COPY --from=tomcat:11.0-jdk21-temurin /usr/local/tomcat $CATALINA_HOME |
Comment on lines
+144
to
+159
| for (Part part : request.getParts()) { | ||
| String submittedFilename = part.getSubmittedFileName(); | ||
| if (submittedFilename == null || submittedFilename.isEmpty()) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // Validate the temp directory is within allowed system temp path | ||
| PathValidationUtils.validateExistingPath(uploadTempDir, new File(systemTempDir)); | ||
| factory.setRepository(uploadTempDir); | ||
| try { | ||
| // Use PathValidationUtils to sanitize and validate the destination path | ||
| File documentDir = new File(foldername); | ||
| File savedFile = PathValidationUtils.validatePath(submittedFilename, documentDir); | ||
|
|
||
| } catch (SecurityException e) { | ||
| MiscUtils.getLogger().error("Security validation failed for upload temp directory", e); | ||
| throw new ServletException("Upload configuration error: invalid temp directory path", e); | ||
| } | ||
| fileheader = savedFile.getName(); | ||
|
|
||
| // Create a new file upload handler | ||
| ServletFileUpload upload = new ServletFileUpload(factory); | ||
| upload.setHeaderEncoding("UTF-8"); | ||
|
|
||
| // Set file size limits to prevent DoS attacks (50 MB limit for MOH billing files) | ||
| upload.setFileSizeMax(52428800); // 50 MB per file | ||
| upload.setSizeMax(52428800); // 50 MB total request size | ||
| try (InputStream in = part.getInputStream()) { | ||
| Files.copy(in, savedFile.toPath()); | ||
| } |
Comment on lines
+166
to
+169
| MiscUtils.getLogger().error("Invalid uploaded filename: " + submittedFilename); | ||
| continue; | ||
| } catch (IOException e) { | ||
| MiscUtils.getLogger().error("Error processing file: " + submittedFilename, e); |
Comment on lines
+183
to
+186
| private static boolean isMultipartContent(HttpServletRequest request) { | ||
| String contentType = request.getContentType(); | ||
| return contentType != null && contentType.toLowerCase().startsWith("multipart/"); | ||
| } |
Comment on lines
+173
to
+177
| // Pass through without wrapping - Tomcat 11's RequestDispatcher.forward() | ||
| // is incompatible with response wrappers that suppress flush/close. | ||
| // The script is injected by CsrfGuardScriptInjectionFilter instead, | ||
| // or appended directly after the chain completes. | ||
| chain.doFilter(request, response); |
Comment on lines
+185
to
+188
| HttpServletResponse httpResponse = (HttpServletResponse) response; | ||
|
|
||
| // Only inject for HTML responses | ||
| String contentType = delegatingResponse.getContentType(); | ||
| String contentType = httpResponse.getContentType(); |
Comment on lines
+199
to
+210
| // Don't inject if response is already committed (forward/redirect already sent) | ||
| if (httpResponse.isCommitted()) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| String script = buildScript(httpRequest.getContextPath(), httpRequest.getLocale()); | ||
| httpResponse.getWriter().print(script); | ||
| } catch (IllegalStateException e) { | ||
| // getWriter() fails if getOutputStream() was already called - skip injection | ||
| logger.debug("Cannot inject logout script - output stream already obtained", e); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Bulk migration of the entire CARLOS EMR codebase from javax.* to
jakarta.* namespace, upgrading all frameworks to Jakarta EE 10
compatible versions.
Dependency upgrades (pom.xml):
Java imports (~1,629 files):
Struts 7 migration (~410 files):
JSP JSTL taglib URIs (~681 files):
Config files:
JDK javax packages preserved (not migrated):
javax.crypto, javax.net.ssl, javax.security.auth, javax.imageio,
javax.naming, javax.sql, javax.xml.parsers, javax.xml.xpath,
javax.xml.transform, javax.swing, etc.
https://claude.ai/code/session_01RTbR8gPjRdLaxZpMeJtqdn
Description
This PR includes a bulk migration of the CARLOS EMR codebase from
javax.*tojakarta.*namespace, upgrading all frameworks to Jakarta EE 10 compatible versions.Changes walkthrough 📝
billDaySheet.xml
Refactor JasperReport Template for Bill Day Sheetsrc/main/resources/oscar/oscarReport/pageUtil/billDaySheet.xml
reflabel.xml
Refactor JasperReport Template for Referral Labelsrc/main/resources/org/oscarehr/common/web/reflabel.xml
Textualizer.java
Update Import for Jakarta EE Compatibilitysrc/main/java/io/github/carlos_emr/carlos/util/Textualizer.java
javax.persistence.Columntojakarta.persistence.Column.