Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fp7T1pGYEMrMVp1M7AW9Fg
| <emrapiModuleVersion>2.0.0</emrapiModuleVersion> | ||
| <htmlformentryVersion>4.0.0</htmlformentryVersion> | ||
| <idgenVersion>4.6.0</idgenVersion> | ||
| <idgenVersion>6.0.0-SNAPSHOT</idgenVersion> |
There was a problem hiding this comment.
This can't merge until four upstream PRs land: CI's own command (mvn --batch-mode --update-snapshots clean package) fails on this branch before anything compiles. With a clean local repository, under Maven 3.10.0 as on the runner:
[ERROR] Failed to read artifact descriptor for org.openmrs.module:metadatamapping-api:jar:3.0.0-SNAPSHOT
[ERROR] Caused by: The following artifacts could not be resolved: org.openmrs.module:metadatamapping-api:pom:3.0.0-SNAPSHOT (absent)
Four of the SNAPSHOTs set here aren't Platform 3.0 builds in any repository CI reads:
metadatamapping-api3.0.0-SNAPSHOT andstockmanagement-api4.0.0-SNAPSHOT aren't published anywhere (404 on mavenrepo.openmrs.org and on Mekom's Nexus). They come from Upgrade to support OpenMRS Platform 3.0 openmrs/openmrs-module-metadatamapping#70 and Upgrade to support OpenMRS Platform 3.0 openmrs/openmrs-module-stockmanagement#49, both still open.idgen-api6.0.0-SNAPSHOT resolves to idgen master's Platform 2.8 build (20260911.215946-12: Java 8 classes,parent="patientServiceTarget"). With it and the published cohort jar below, the integration test context fails to start:No bean named 'patientServiceTarget' available, from idgen's moduleApplicationContext.xml. Upgrade to support OpenMRS Platform 3.0 openmrs/openmrs-module-idgen#140 builds 7.0.0-SNAPSHOT now, so this property has to follow it.cohort-api4.0.0-SNAPSHOT resolves to cohort master's Platform 2.7 build (Java 8, javax) until Upgrade to support OpenMRS Platform 3.0 openmrs/openmrs-module-cohort#44 merges. That PR keeps the version, so nothing changes here for cohort.
CI hasn't run on this PR yet (both runs are waiting for approval), so nothing has flagged this. If it merges as is, main's Build and Publish job fails at dependency resolution and nothing gets published. So the merge order is those four PRs, then this one with the bump below. That's on top of the two the description already lists: openmrs/openmrs-core#6639 (the two program workflow ITs still error on today's core snapshot, build 435) and openmrs/openmrs-module-fhir2#633.
| <idgenVersion>6.0.0-SNAPSHOT</idgenVersion> | |
| <idgenVersion>7.0.0-SNAPSHOT</idgenVersion> |
| try { | ||
| DatabaseUpdater.executeChangelog(relativePath, (ChangeSetExecutorCallback) null); | ||
| // Changesets write with SQL, behind the back of Platform 3.0's global property cache | ||
| Context.getRegisteredComponent("globalPropertyCache", GlobalPropertyCache.class).clear(); |
There was a problem hiding this comment.
Worth moving this clear into a finally after the catch. Liquibase commits each changeset on its own, so when a later changeset fails, the global properties written by the earlier ones are already in the database, but the exception skips this line. In the default continue_on_error mode Initializer logs the error and carries on, and the cache keeps serving the old values for up to the global-properties lifespan (an hour), including to the domains loaded after this one.
I checked it by adding a failing third changeset to api-2.5's test changelog: load_shouldExecuteNewChangeSet then fails with expected: <true> but was: <null> on test_changes_1, and passes with the clear in a finally.
| billableService.setServiceStatus(StringUtils.isNotBlank(serviceStatus) ? BillableServiceStatus.valueOf(serviceStatus | ||
| .toUpperCase()) : BillableServiceStatus.ENABLED); |
There was a problem hiding this comment.
This hunk doesn't survive a build: mvn package on this branch puts the file back to main's exact bytes, so it can be dropped. The same build also rewrites four of the api-2.7 tests this PR touches (BillableServicesLoaderIntegrationTest, CashierItemPriceLineProcessorTest, ConceptReferenceRangeLoaderIntegrationTest and LocationsLoader_2_7_IntegrationTest), so building the branch leaves a dirty tree. Committing what the build produces for those files takes care of both.
| billableService.setServiceStatus(StringUtils.isNotBlank(serviceStatus) ? BillableServiceStatus.valueOf(serviceStatus | |
| .toUpperCase()) : BillableServiceStatus.ENABLED); | |
| billableService.setServiceStatus( | |
| StringUtils.isNotBlank(serviceStatus) ? BillableServiceStatus.valueOf(serviceStatus.toUpperCase()) | |
| : BillableServiceStatus.ENABLED); |
| <activator>org.openmrs.module.initializer.InitializerActivator</activator> | ||
|
|
||
| <require_version>${openmrsPlatformVersion}</require_version> | ||
| <require_version>3.0.0</require_version> |
There was a problem hiding this comment.
I'd keep ${openmrsPlatformVersion} here, as main had. Core treats the two spellings the same: ModuleUtil.checkRequiredVersion, the check ModuleFactory runs before it starts a module, accepts both 3.0.0-SNAPSHOT and 3.0.0 on cores 3.0.0, 3.0.1 and 3.1.0 and refuses both on 2.8.3 and 2.9.0 (run against today's build-435 jar). The literal only costs something at the next bump: whoever raises openmrsPlatformVersion for a newer core API also has to remember this line, or an older core starts the module and it breaks at runtime instead of being refused up front.
| <require_version>3.0.0</require_version> | |
| <require_version>${openmrsPlatformVersion}</require_version> |
| <groupId>org.openmrs.module</groupId> | ||
| <artifactId>initializer</artifactId> | ||
| <version>2.13.0-SNAPSHOT</version> | ||
| <version>3.0.0-SNAPSHOT</version> |
There was a problem hiding this comment.
README.md needs a pass for this bump. It isn't in the diff and still describes the 2.x line:
- "Runtime compatibility" says OpenMRS Core 2.1.1 is required, so someone on Platform 2.x who follows "Build the master branch" gets a module that won't start. Core 3.0.0 and Java 21, plus a pointer to 2.12.x for Platform 2.x, would cover it.
- "Releases notes" still opens with an empty
version 2.13.0. Add support for Concept Map Types domain #336, O3-5820: Fix OrderType parent silently dropped on import for concrete order types #331 and O3-5658: Add support for Procedure Types domain in the Initializer module #323 each added their line there, so this one should turn it into the 3.0.0 entry. - readme/validator.md still says to build with
mvn clean package -P validator, which now fails before building anything: validator/pom.xml pinsopenmrsPlatformVersionto 2.3.6, so the BOM import it inherits asks foropenmrs-bom:2.3.6, which doesn't exist. Until the validator is ported, a line there saying it only exists on the 2.x line would do.
None of this blocks the merge.
| package org.openmrs.module.initializer.api; | ||
|
|
||
| import org.apache.commons.lang.BooleanUtils; | ||
| import org.apache.commons.lang3.BooleanUtils; |
There was a problem hiding this comment.
Moving BooleanUtils to lang3 changes what getBool accepts: 1 now reads as true, where lang 2.6 read it as false. It's the only spelling that changed among the ones I ran through both libraries (true/false, yes/no, on/off, y/n, t/f, 1/0). getVoidOrRetire was already on lang3, so this makes the two agree, which seems right, but a 1 under required or allow decimals in an existing CSV now loads differently, so it's worth a line in the 3.0.0 release notes.
There was a problem hiding this comment.
The new release note has Void/Retire backwards, as did my comment above: the 2.x omod bundles commons-lang3 3.1, which the module classloader loads ahead of core's copy, and 3.1 reads 1 as false. The real getVoidOrRetire gives false for 1 with the 2.12.1 jars and true at this head, so a 1 there now retires or voids the row. Something like:
* Most boolean CSV columns, `Void/Retire` included, and the `initializer.skip.checksums` and `initializer.row.checksums.enabled` properties now read `1` as true. Initializer 2.x read it as false.
- Build against idgen 7.0.0-SNAPSHOT, the version openmrs/openmrs-module-idgen#140 now builds. The published 6.0.0-SNAPSHOT is idgen master's Platform 2.8 build. - LiquibaseLoader2_5 clears the global property cache in a finally block. The changesets that ran before a failing one stay committed, so their global properties were served stale for up to an hour. LiquibaseLoader25IntegrationTest covers it with a changelog whose second changeset fails. - require_version goes back to ${openmrsPlatformVersion}. - Commit what the pinned formatter produces for the api-2.7 files it rewrote on every build, which puts BillableServicesLineProcessor back to main's version. - README: OpenMRS Core 3.0.0 and Java 21, the 3.0.0 release notes, and a note that the validator stays on the 2.x line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Initializer 2.x bundles commons-lang3 3.1, which the module classloader loads ahead of core's copy, and 3.1 reads "1" as false. So Void/Retire, the other lang3 boolean columns and the checksum runtime properties all read 1 as false on 2.x and read it as true on 3.0. Void/Retire was not already reading it as true. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| <dependency> | ||
| <groupId>com.opencsv</groupId> | ||
| <artifactId>opencsv</artifactId> | ||
| <version>4.5</version> |
There was a problem hiding this comment.
Worth moving this to 5.11, even though the JDK 21 build passes. Core 3.0's Liquibase 4.32 depends on opencsv 5.11, and this 4.5 wins on every test classpath here, so the tests run core's Liquibase against an opencsv that's missing classes it needs:
- On JDK 25,
DeleteDomainChecksumsChangesetIntegrationTestand two of theLiquibaseLoader25IntegrationTesttests fail. Liquibase's scan of its change types throwsNoClassDefFoundError: com/opencsv/exceptions/CsvMalformedLineException. - On JDK 21, Liquibase logs "Cannot load service" for
LoadDataChangeandLoadUpdateDataChangeand carries on without them, which is why the build passes there. A changeset usingloadDatathen fails withUnknown change type 'loadData'.
A running server isn't affected: OpenmrsClassLoader defines no classes itself, so Liquibase comes from the war and links against the war's 5.11.
I'd use 5.11 with provided scope, as commons-codec does above, which also takes 4.5 out of the omod. The only code change is in CsvParser: in 5.x readNext() also declares CsvValidationException, which only a registered validator throws, so the three calls just need to wrap it. With that change the build passes on JDK 21 and 25, a loadData changeset runs, and all 82 test CSVs parse to the same rows under 4.5 and 5.11.
Core 3.0's Liquibase 4.32 depends on opencsv 5.11, and the 4.5 that initializer-api declared won on every test classpath. Liquibase then could not load LoadDataChange or LoadUpdateDataChange. On JDK 21 it logged "Cannot load service" and ran without them, so a loadData changeset failed with "Unknown change type 'loadData'". On JDK 25 the same NoClassDefFoundError failed three tests. A server was not affected, since Liquibase loads opencsv from the war there. opencsv is now 5.11 and provided, like commons-codec, so the omod no longer bundles 4.5. In 5.x readNext() also declares CsvValidationException, which only a registered validator throws, so CsvParser wraps it in an IOException. All 83 CSV files in the repository parse to the same rows with 4.5 and 5.11. LiquibaseLoader25IntegrationTest covers a loadData changeset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| @BeforeEach | ||
| public void setup() throws Exception { | ||
| loader.load(); | ||
| } |
There was a problem hiding this comment.
This needs a cleanup step before it merges, or the api module's CI result depends on the order surefire finds the test classes in. The Liquibase run below commits the test's connection, so the concepts that loader.load() creates here are never rolled back. On main that was harmless, because JUnit 4's BaseContextSensitiveTest calls deleteAllData() in its @AfterClass. The jupiter base class only closes the session in @AfterAll, so those concepts stay in H2 for every later class in the JVM. ConceptsLoaderIntegrationTest, ConceptSetsLoaderIntegrationTest and ConceptSourcesLoaderIntegrationTest then fail in setup, because testdata/test-concepts.xml inserts uuid d803e973-1010-4415-8659-c011dec707c0 a second time.
If it merges like this, CI goes red whenever the runner lists this class before at least one of those three. That's what happened when I built the head inside a Linux container (temurin 21, against core master + openmrs/openmrs-core#6639 and the four module PRs). This class ran first and the three failed, 7 errors in all. On a Mac it passes only because this class happens to run last. -Dtest=DeleteDomainChecksumsChangesetIntegrationTest,ConceptSourcesLoaderIntegrationTest -Dsurefire.runOrder=reversealphabetical reproduces it on any machine, and the same pair passes on main.
Core's javadoc on getConnection() says a test that commits has to clean up after itself with deleteAllData(). With this added, the Linux build and the reversed order both pass:
@AfterEach
public void deleteCommittedData() {
// the changeset commits the test's transaction, so the concepts loaded in setup outlive the rollback
deleteAllData();
}The patientflags ITs load the same dataset, so they'd run into this as well once they're re-enabled.
| @BeforeEach | ||
| public void setUp() { | ||
| PowerMockito.mockStatic(Context.class); | ||
| contextMock = Mockito.mockStatic(Context.class); |
There was a problem hiding this comment.
Could you add openmrs/openmrs-core#6672 under "Depends on"? On the published 3.0.0-SNAPSHOT (build 435, which is what CI resolves), Context can't initialize if this line loads it before anything else in the test JVM has started log4j, which is openmrs/openmrs-core#6670. This class then errors on all 27 tests when it runs alone, and LocationAttributeLineProcessorTest (3 tests) and ConceptAttributeLineProcessorTest (1) do the same. Every later class in that JVM that uses Context then fails with NoClassDefFoundError: Could not initialize class org.openmrs.api.context.Context, as -Dtest=UtilsTest,AmpathFormsLoaderIntegrationTest,DeleteDomainChecksumsChangesetIntegrationTest -Dsurefire.runOrder=alphabetical shows. Neither the Mac order nor the Linux container's order put one of these three first, so a full build hasn't hit it yet, but on CI it comes down to the runner's file order.
On build 434 all three pass alone, and they also pass on a69b14a84b with that PR's reorder (userContextHolder declared before log).
There was a problem hiding this comment.
Could you change "which fixes" to something like "which addresses" in the new "Depends on" line? GitHub reads which fixes openmrs/openmrs-core#6670 as a closing keyword. The Development box here now says "Successfully merging this pull request may close these issues" and lists that issue, so if this merges before openmrs/openmrs-core#6672, it could close the core issue while the bug is still on core master.
| strategy: | ||
| matrix: | ||
| java: [ '8', '11'] | ||
| java: [ '21' ] |
There was a problem hiding this comment.
This matrix should build on 25 as well as 21. #231 set it up to build every PR on both Java 8 and 11, the versions Initializer supported then, and on Platform 3.0 the same pair is 21 and 25: core's own CI builds on both, and so does the shared module workflow in openmrs-contrib-gha-workflows. The opencsv problem from the last round only failed the build on 25, so a 21-only matrix would have let it through. This head already passes on 25 here. It doesn't block anything, and it's one line:
| java: [ '21' ] | |
| java: [ '21', '25' ] |
DeleteDomainChecksumsChangesetIntegrationTest runs a Liquibase changeset on the test's connection and commits it, so the concepts its setup loads were never rolled back. On 2.x that was harmless, because JUnit 4's BaseContextSensitiveTest deleted all data after each class, but the jupiter base classes only close the session in @afterall. The concepts then stayed for the rest of the run, and testdata/test-concepts.xml failed on a duplicate concept uuid in ConceptsLoaderIntegrationTest, ConceptSetsLoaderIntegrationTest and ConceptSourcesLoaderIntegrationTest whenever they ran after it. Whether they did depends on surefire's file order: the build passed on macOS, where this class runs last, and failed with 7 errors in a Linux container, where it runs first. The test now calls deleteAllData() after each test, as core's javadoc on getConnection() asks of a test that commits. Pull request builds now run on JDK 25 as well as 21, the way mekomsolutions#231 set them up for 8 and 11. Core's own CI builds on both, and the opencsv problem fixed in the previous commit only failed on 25. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| <!-- So that the classpath gets core's versions of these rather than opencsv's --> | ||
| <exclusion> | ||
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-lang3</artifactId> | ||
| </exclusion> |
There was a problem hiding this comment.
Without its own copy of commons-lang3, Initializer can read the same CSV differently depending on what else is installed, so I'd bundle it in the omod again before this merges. Its class loader looks in its own jars, then walks its required and aware-of modules (in hash order, not config.xml order), and only then the webapp, since core's ModuleClassLoader has probeParentLoaderLast true by default. fhir2 6.x bundles commons-lang3 3.9, which reads "1" as false, as did the 3.1 that Initializer 2.x shipped.
I loaded a person attribute types CSV with Searchable set to 1 through this omod, on the published core war (build 435, Tomcat 11, MySQL 8):
- With webservices.rest and Initializer, the row was saved as searchable, with
BooleanUtilsloaded from core's commons-lang3-3.21.0. - After adding fhir2, the same row was saved as not searchable, with
BooleanUtilsfrom fhir2's commons-lang3-3.9. - With all 29 O3 modules it was searchable, from webservices.rest's copy of 3.21.0, because a module that requires rest was reached before fhir2.
A row with true was saved as searchable in all three runs.
So the O3 RefApp isn't affected today, and a server that gets 3.9 reads 1 as false just as 2.x did. But nothing logs which copy won, and the 3.0.0 release note I suggested in the CsvLine thread ("now read 1 as true") is false on those servers.
Adding this after the opencsv dependency puts commons-lang3-3.21.0 in the omod (the BOM's version, the same jar core has). That does shadow core's copy, which the comments above avoid on purpose, but without it the copy that wins isn't necessarily core's either. The build passes on JDK 21 with the test counts in the description, and on rest + fhir2 + Initializer the 1 row is then saved as searchable, with BooleanUtils from Initializer's own copy:
<!-- Bundled: a module's own jars come before its aware-of modules', and fhir2 bundles lang3 3.9 -->
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
</dependency>Initializer 2.x bundled commons-lang3. This branch stopped bundling it by excluding it from opencsv, expecting core's copy to be used, but that is not what a module gets: core's ModuleClassLoader looks in the module's own jars, then in its required and aware-of modules, and only then in the webapp. fhir2 6.x bundles commons-lang3 3.9, which reads "1" as false. On core 3.0, with webservices.rest, fhir2 and Initializer installed, a person attribute types row with Searchable set to 1 was saved as not searchable, and with fhir2 removed it was saved as searchable. Which copy wins depends on the modules installed, and nothing logs it. commons-lang3 is now bundled at the version openmrs-bom manages, the same 3.21.0 jar core ships, so Initializer always uses its own copy. On core build 437 with webservices.rest and fhir2, that row is now saved as searchable, and the 3.0.0 release note on reading 1 as true holds whatever else is installed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cohort's core-3.x branch moved to 5.0.0-SNAPSHOT (openmrs-module-cohort#47). The 4.0.0-SNAPSHOT now published is the 2.x line, which Hibernate 7 rejects with "cohort_attribute refers to an unmapped class CohortM", so every test needing the Spring context errored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Updates initializer so it builds against, and runs on, openmrs-core master (Platform 3.0.0-SNAPSHOT: Java 21, Jakarta EE, Spring 7, Hibernate 7). This is part of upgrading the O3 reference application modules to Platform 3. Same approach as the platform 3 upgrades of openmrs-module-calculation (#19), legacyui and webservices.rest.
Changes
openmrs-bomimported.require_versionstays${openmrsPlatformVersion}, so it is 3.0.0-SNAPSHOT for now.fhir2-api-2.xartifacts are dropped) and stockmanagement 4.0.0-SNAPSHOT.maven-parent-openmrs-module1.1.1 forces junit 4.11, mockito 1.9.5 and PowerMock. Those are overridden, and PowerMock is kept off the test classpath because its MockMaker breaks Mockito 5.openmrs-toolsis pinned to 2.1.1, so the 3.x formatter doesn't reformat the code base.provided, since core ships both. opencsv moves to 5.11, the version core's Liquibase 4.32 depends on: with 4.5 on the test classpath Liquibase could not load itsloadDatachanges (and failed outright on JDK 25).1as false, so without its own copy Initializer read1differently depending on what else was installed.HibernateInitializerDAOmoves from Criteria to HQL.CsvParserwraps theCsvValidationExceptionthat opencsv 5'sreadNext()declares, which only a registered validator throws.CriteriaBuilder.InitializerLogConfigurator2_4andConceptReferenceRangeLoadernow go up to9.*."FR"as a language-less locale, somessages_fr_FR.propertieswas being loaded as_FR.getLocaleFromFileBaseNamenow skips language-less candidates.global_propertyvia SQL behind core 3.0's new GP cache (TRUNK-6707).LiquibaseLoader2_5now clears that cache after running the changelog, also when a changeset fails, since the ones that ran before it stay committed.MockedStaticin place of PowerMock and the jupiter base classes.LiquibaseLoader25IntegrationTestcovers a changelog whose second changeset fails after the first one wrote a global property, and aloadDatachangeset.DeleteDomainChecksumsChangesetIntegrationTestdeletes the data its Liquibase run commits. The jupiter base classes don't clear the database between test classes the way JUnit 4's did, so without that the concepts it loads collided withtestdata/test-concepts.xmlin the concept loader ITs whenever they ran after it.Platform3ModuleContextLoaderkeeps beans of optional modules with no 3.0 release out of the test context.ProgramWorkflowsLoaderIntegrationTestandProgramWorkflowStatesLoaderIntegrationTestrun and pass against a core that includes Do not evict the program being saved in ProgramValidator openmrs/openmrs-core#6639. On current core master they hit that NPE, which fires when a program created earlier in the same session is saved again.-P validatorprofile is still on Platform 2.3.6 and is not ported.readme/validator.mdsays the validator stays on the 2.x line for now.Depends on
core-3.xbranch, which the shared build workflow doesn't deploy from, so the cohort-api 4.0.0-SNAPSHOT on the repository is master's Platform 2.x build (republished on 2026-10-11). With it, Hibernate can't map cohort's entities, so the api module's test context fails to start (72 of its 182 tests error).jakarta.annotation-api, and Spring ignoresInitializerMessageSource's@PostConstruct. The message cache then stays empty and every message code renders as itself (the home page title showsindex.title).UtilsTest,LocationAttributeLineProcessorTestandConceptAttributeLineProcessorTestcan't initializeContextwhen one of them is the first test class in the run to load it, and every later test class that usesContextthen fails too. Whether that happens depends on surefire's file order. On their own, all three fail on the published build 435 and pass with Initialize Context's user context holder before its logger openmrs/openmrs-core#6672.Verification
core-3.xhead:mvn clean packagepasses in every module, with api 182 tests, 2.2: 2, 2.3: 2, 2.5: 4, 2.7: 17 and 2.8: 13. That holds on JDK 21 and 25, with Maven 3.10.0 (the runner's version), and in a Linux container, where surefire runs the test classes in a different order than on macOS.loadDatachangeset.Searchableset to1is saved as searchable. Before commons-lang3 was bundled it was saved as not searchable there, because Initializer picked up fhir2's lang3 3.9.mvn jetty:run, MySQL 8): every module starts, logging in through the UI works, and core message codes resolve (home page title "OpenMRS - Home"), with Do not bundle jakarta.annotation-api in the omod openmrs/openmrs-module-fhir2#633 deployed.🤖 Generated with Claude Code
https://claude.ai/code/session_01Fp7T1pGYEMrMVp1M7AW9Fg