Migrate to OpenMRS Core 3.0.0-SNAPSHOT#702
Conversation
Key changes: - Upgrade Java from 8 to 21 - javax.servlet -> jakarta.servlet namespace migration - org.apache.commons.lang -> org.apache.commons.lang3 - Jackson 1.x (org.codehaus.jackson) -> Jackson 2.x (com.fasterxml.jackson) - Hibernate Criteria API -> JPA Criteria API in RestHelperServiceImpl - Spring 6: CommonsMultipartResolver -> StandardServletMultipartResolver - Spring 6: PathPatternParser compatibility (remove setPathMatcher after init) - Hibernate 6: Cache API updates, Query API changes, strict lazy loading - Remove legacyui-omod dependency (AdminSection extends Extension directly) - Fix Core 3.0 API removals (Field.getAnswers, setStatus(int,String), etc.) - Update version patterns in resources/search handlers for Core 3.0 - Fix test datasets for Core 3.0 schema changes (NOT NULL constraints) - Add -parameters compiler flag for Spring 6 parameter name resolution Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update GitHub Actions and Bamboo CI to only build with Java 21, matching the minimum Java version required by Core 3.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@wikumChamith i switched to the CLI which behaved in the correct way (iterated until all errors were fixed and tests passed). What do you think about its output? 😊 |
Restore AdminSection to extend AdministrationSectionExt from legacyui so that the <openmrs:extensionPoint> tag properly recognizes and renders the admin section links. Add legacyui-omod 2.1.0-SNAPSHOT as an optional provided dependency. Also fix person display test expectations to remove 'Mr.' prefix which is no longer included in Core 3.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dkayiwa, there are some test errors. Otherwise, this looks great :) |
|
Yes iam trying to find out why these errors are only on ci but not locally. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| assertFalse(sessionFactory.getCache().containsEntity(PERSON_NAME_CLASS, ID_2)); | ||
| assertFalse(sessionFactory.getCache().containsEntity(PERSON_NAME_CLASS, ID_8)); | ||
| //All persistent collections containing the names should have been discarded | ||
| assertNull(sessionFactory.getStatistics().getSecondLevelCacheStatistics(Person.class.getName() + ".names") |
There was a problem hiding this comment.
Hibernate 6 changed the SecondLevelCacheStatistics API. The getEntries() method that returned a map of cached collection entries no longer exists in Hibernate 6's statistics API. There's no direct equivalent to inspect individual collection region entries, so those assertions couldn't be ported. The tests still verify the core behavior — that entity cache entries and query cache entries are cleared — just not the collection-level cache entries.
| return delegate.toString(); | ||
| } | ||
|
|
||
| @PropertyGetter("concept") |
There was a problem hiding this comment.
Hibernate 6 is stricter about lazy loading. When the REST framework serializes a ConceptStateConversion, it accesses delegate.getConcept() which returns a lazy-loaded Concept proxy. In Hibernate 5 this proxy could initialize transparently, but in Hibernate 6 accessing the proxy's properties (like concept.names) outside the original Hibernate session throws a LazyInitializationException.
The @PropertyGetter("concept") re-fetches the Concept via ConceptService within an active session, ensuring it's fully initialized before serialization.
The Hibernate.initialize(delegate.getConcept()) approach avoids the extra database round-trip of fetching by UUID. However, if the LazyInitializationException is triggered by the concept's nested lazy collections (like names, descriptions), then Hibernate.initialize() on the concept alone may not be sufficient — it only initializes the top-level proxy, not its child collections. The re-fetch via ConceptService works because that service method loads the concept fully within its own transaction.
|
|
||
| @Override | ||
| public String getURI() { | ||
| return "/drugreferencemap"; |
There was a problem hiding this comment.
Spring 6's PathPatternParser (which replaces the old AntPathMatcher) is stricter about URL patterns. A leading slash in the URI returned by getURI() results in a double slash when the test framework prepends the base path (e.g., /rest/v1/ + /drugreferencemap → /rest/v1//drugreferencemap). PathPatternParser rejects double slashes, causing the request to not match any handler. Removing the leading slash produces the correct path: /rest/v1/drugreferencemap.
legacyui-omod is now a provided dependency, so LongFreeTextTextareaHandler is available on the test classpath. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Deduplicate the flush/clear/re-fetch pattern into a createWorkflowState() helper method. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Core 3.0 no longer bundles pre-configured name templates, so the tests now register short and long templates programmatically via NameSupport.setLayoutTemplates() before each test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 5 name templates (spain, latinamerica, givenfamily, short, long) are defined in legacyui's webModuleApplicationContext.xml which is on the test classpath since legacyui-omod is a provided dependency. No manual template registration is needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| credentials | ||
| .setActivationKey("b071c88d6d877922e35af2e6a90dd57d37ac61143a03bb986c5f353566f3972a86ce9b2604c31a22dfa467922dcfd54fa7d18b0a7c7648d94ca3d97a88ea2fd0:" | ||
| + tokenTime); | ||
| dao.updateLoginCredential(credentials); |
There was a problem hiding this comment.
In Core 3.0, updateLoginCredential() has a StackWalker security check that restricts which classes can call it — it only allows HibernateUserDAO, UserDAOTest, and UserServiceTest. Our test class PasswordResetController2_2Test is not on that allow-list, so calling updateLoginCredential() would throw a DAOException("Illegal attempt to change user password from unknown caller").
setUserActivationKey() does the same saveOrUpdate but without the caller restriction, since it's specifically designed for setting activation keys (which is exactly what the test does).
The change was necessary because Core 3.0 added a StackWalker-based security check to updateLoginCredential() that only allows calls from HibernateUserDAO,
UserDAOTest, and UserServiceTest. Our test class isn't on that allow-list and would get a DAOException("Illegal attempt to change user password from unknown
caller"). setUserActivationKey() performs the same saveOrUpdate without the caller restriction, and is semantically more correct since the test is specifically
setting an activation key, not updating a password.
| Field field = Context.getFormService().getFieldByUuid(RestTestConstants1_8.FIELD_UUID); | ||
| FieldAnswer fieldAnswer = new FieldAnswer(); | ||
| fieldAnswer.setConcept(Context.getConceptService().getConceptByUuid(RestTestConstants1_8.CONCEPT_UUID)); | ||
| field.addAnswer(fieldAnswer); |
There was a problem hiding this comment.
field.addAnswer(fieldAnswer) → fieldAnswer.setField(field): Core 3.0 removed Field.getAnswers() and Field.addAnswer() (PR #5309, TRUNK-5797). The addAnswer() method no longer exists on the Field class. Instead, since the FieldAnswer entity still has a @manytoone reference back to Field, we set the relationship from the answer side via fieldAnswer.setField(field).
| */ | ||
| @Override | ||
| public String getDisplayProperty() { | ||
| return "Null Field - YES"; |
There was a problem hiding this comment.
"Null Field - YES" → "Some concept - YES": The display format is " - ". Previously, fieldAnswer.getField() returned null because the relationship was set via field.addAnswer() which populated the field's collection but didn't set the back-reference on the FieldAnswer. Now that we explicitly call fieldAnswer.setField(field), the field is properly set, so the display shows the field's actual name ("Some concept") instead of "Null Field".
Restore the legacyui-omod version management in the parent POM (updated to 2.1.0-SNAPSHOT for Core 3.0 compatibility) instead of hardcoding the version in omod/pom.xml. The omod dependency keeps its original position and just adds optional=true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All callers now use getHibernateSession() which returns a native Hibernate Session directly. The old getSession() with its Hibernate 3 fallback via reflection is no longer needed with Core 3.0 (Hibernate 6). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Return directly like the other three occurrences. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both branches did the same thing since Jakarta Servlet 6.0 removed setStatus(int, String). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| public static void setResponseStatus(Throwable ex, HttpServletResponse response) { | ||
| ResponseStatus ann = ex.getClass().getAnnotation(ResponseStatus.class); | ||
| if (ann != null) { | ||
| if (StringUtils.isNotBlank(ann.reason())) { |
There was a problem hiding this comment.
The HttpServletResponse.setStatus(int, String) method (status code + reason phrase) was removed in Jakarta Servlet 6.0 (which Core 3.0 uses). Only setStatus(int) remains. The two-argument version was deprecated since Servlet 2.1 because HTTP/2 doesn't support custom reason phrases, and Jakarta finally removed it.
| <bean id="stringHttpMessageConverter" class="org.springframework.http.converter.StringHttpMessageConverter" /> | ||
|
|
||
| <bean id="multipartResolver" | ||
| class="org.springframework.web.multipart.commons.CommonsMultipartResolver"> |
There was a problem hiding this comment.
Spring 6 removed CommonsMultipartResolver (which depended on Apache Commons FileUpload). The replacement is StandardServletMultipartResolver, which uses the Servlet 3.0+ built-in multipart support (jakarta.servlet.http.Part).
The maxUploadSize property was removed because StandardServletMultipartResolver doesn't have it — upload size limits are configured in the servlet container's multipart config (e.g., via @MultipartConfig annotation or in web.xml) rather than on the resolver bean.
| @Test | ||
| public void SearchConfig_shouldFailIfGivenIdIsNull() throws Exception { | ||
|
|
||
| expectedException.expect(IllegalArgumentException.class); |
There was a problem hiding this comment.
The SearchConfig constructor uses Validate.notEmpty() from Apache Commons Lang. The migration from commons-lang to commons-lang3 changed the exception type: commons-lang's Validate.notEmpty(null) throws IllegalArgumentException, while commons-lang3's Validate.notEmpty(null) throws NullPointerException (it delegates to Validate.notNull() first for null values).
| @Controller("webservices.rest.formResourceController") | ||
| @RequestMapping(value = "/rest/" + RestConstants.VERSION_1 + "/form/{uuid}/resource/{resourceUuid}/value") | ||
| public class FormResourceController1_9 extends MainResourceController { | ||
| public class FormResourceController1_9 extends BaseRestController { |
There was a problem hiding this comment.
This controller is a special-purpose endpoint for form resource binary values — it defines its own specific GET and POST handlers and has no use for MainResourceController's generic CRUD methods.
MainResourceController has @RequestMapping("/rest/v1") with handler methods that use {uuid} path variables (e.g. update(String uuid, ...)). FormResourceController1_9 has its own @RequestMapping("/rest/v1/form/{uuid}/resource/{resourceUuid}/value"). When it extends MainResourceController, Spring 6's PathPatternParser sees the inherited handler methods' {uuid} combined with the controller's {uuid} path and rejects the duplicate capture variable — something Spring 5's AntPathMatcher tolerated.
|
|
||
| public List<T> getAllLayoutTemplates() { | ||
| List<T> templates = source.getLayoutTemplates(); | ||
| if (templates == null) { |
There was a problem hiding this comment.
In Core 2.x, NameSupport/AddressSupport always had templates pre-configured (via Spring XML in openmrs-web or legacyui), so getLayoutTemplates() never returned null. In Core 3.0, if legacyui isn't installed, no templates are configured and getLayoutTemplates() returns null — which would cause a NullPointerException on the next line (templates.size()). The null check returns an empty list instead, making the module work gracefully without legacyui.
| List<FieldAnswer> fieldAnswers = new ArrayList<FieldAnswer>(); | ||
| if (parent.getAnswers() != null) { | ||
| fieldAnswers.addAll(parent.getAnswers()); | ||
| } |
There was a problem hiding this comment.
Core 3.0 removed the Field.getAnswers() method (PR #5309, TRUNK-5797) — the Java collection property on the Field class was removed. The FieldAnswer entity and the field_answer database table still exist, and FieldAnswer still has a @ManyToOne back-reference to Field.
So instead of navigating from the parent side (field.getAnswers()), we now query from the child side using RestHelperService.getObjectsByFields() to find all FieldAnswer objects where field equals the parent — achieving the same result via a database query instead of a collection accessor.
| * We're using it instead of {@link AdministrationSectionExt} for a few reasons: | ||
| * - {@link AdministrationSectionExt} class doesn't support setters for its fields | ||
| * so we cannot map their values, for example as we do with message keys. | ||
| * - Using {@link AdministrationSectionExt} as a Resource requires all Resource classes |
There was a problem hiding this comment.
This rationale is no longer accurate — legacyui-omod is now declared as an true provided dependency, so downstream modules are not forced to include it.
| @Before | ||
| public void setUp() { | ||
| Concept concept = Context.getConceptService().getConceptByUuid(RestTestConstants1_8.CONCEPT2_UUID); | ||
| ProgramWorkflowState state = createWorkflowState(RestTestConstants1_8.CONCEPT2_UUID); |
There was a problem hiding this comment.
The core issue is that Hibernate 6 is stricter about transient references after cascade saves.
In the original code:
- A new ProgramWorkflowState is created, added to a workflow, and saved via saveProgram() (cascade)
- After flushSession(), the state is persisted but the in-memory concept and state objects may hold stale references
- When these are then used to create a ConceptStateConversion, Hibernate 6 rejects them as transient/detached references
The fix extracts the duplicated state-creation logic into a createWorkflowState() helper that does a flush/clear/re-fetch cycle:
- flushSession() — ensures the state is written to the database
- clearSession() — detaches all objects from the persistence context
- Re-fetches the workflow and finds the state from it — returns a clean, managed entity
This gives us properly managed Hibernate entities to use when creating the ConceptStateConversion. The helper also eliminates code duplication — the same state-creation pattern was repeated three times (setUp(), shouldCreateStateConversion(), shouldPurgeStateConversion()).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
||
| HandlerExecutionChain handlerExecutionChain = null; | ||
| for (RequestMappingHandlerMapping handlerMapping : handlerMappings) { | ||
| handlerMapping.setPathMatcher(pathMatcher); |
There was a problem hiding this comment.
Spring 6's RequestMappingHandlerMapping uses PathPatternParser by default instead of PathMatcher. When PathPatternParser is active, calling setPathMatcher() has no effect (the pattern parser takes precedence). Since we configured setPatternParser(null) elsewhere to fall back to AntPathMatcher for compatibility, the path matcher is already set at bean initialization time, making this per-request setPathMatcher() call unnecessary.
|
|
||
| @Override | ||
| public NameTemplate newObject() { | ||
| Context.getAdministrationService().setGlobalProperty(GLOBAL_PROPERTY_LAYOUT_NAME_FORMAT, PERSON_NAME_FORMAT_SHORT); |
There was a problem hiding this comment.
The layout.name.format global property needs to be set before calling getDefaultLayoutTemplate(). getDefaultLayoutTemplate() delegates to getDefaultLayoutFormat(), which reads this GP to determine which template is the default (e.g., "short"). In Core 2.x, the NameSupport bean had its defaultLayoutFormat field set to "short" via Spring XML (in openmrs-web or legacyui), providing a fallback when the GP was absent. In Core 3.0's test environment, the NameSupport bean's defaultLayoutFormat field isn't set (the webModuleApplicationContext.xml from legacyui isn't loaded by the test framework), so without the GP, getDefaultLayoutFormat() returns null and getDefaultLayoutTemplate() returns
null — causing the test to fail.
|
|
||
| resource.setConvertedProperties(obs, propertyMap, resource.getUpdatableProperties(), false); | ||
| org.springframework.util.Assert.isTrue(((Double) new ObsResource2_1().getValue(obs)) == 10.0); | ||
| org.springframework.util.Assert.isTrue(((Double) new ObsResource2_1().getValue(obs)) == 10.0, "Expected obs value to be 10.0"); |
There was a problem hiding this comment.
Spring 6 removed the single-argument Assert.isTrue(boolean) method. Only Assert.isTrue(boolean, String) remains, requiring a message parameter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Key changes:
Description of what I changed
Issue I worked on
see https://issues.openmrs.org/browse/RESTWS-
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master