ME-32: Support IDGEN Domains (identifier sources & auto generation options) - #37
ME-32: Support IDGEN Domains (identifier sources & auto generation options)#37wikumChamith wants to merge 2 commits into
Conversation
…tions)
Adds exporters for the idgen module's two Initializer domains:
- **idgen** — identifier sources, split into `idgen_sequential` / `idgen_remote` / `idgen_pool`
CSVs with `_order:` headers so pools load after the sources they reference (Initializer infers
the source type from which columns are present, so types can't share a file). Remote-source
passwords are exported as `property:` placeholders, never plaintext.
- **autogenerationoptions** — referenced identifier types, sources and locations are pulled in
via cross-domain closure.
|
@claude review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (pool.getSource() == null) { | ||
| log.warn("Idgen: identifier pool {} has no backing source; Iniz requires one on import", pool.getUuid()); | ||
| } else { | ||
| line.put(IdentifierSourceLineExporter.HEADER_POOL_IDENTIFIER_SOURCE, pool.getSource().getUuid()); | ||
| } |
There was a problem hiding this comment.
This branch logs that Iniz will reject the row, then writes the row anyway, and I think it has to skip the pool instead. A pool exported without a pool identifier source cell fails on import: IdentifierPoolLineProcessor reads that column with line.get(header, true) and throws, either "No identifier source found with uuid null", or an unknown-column error when no pool in the file carries a source. In Iniz's default non-throwing mode BaseFileLoader.loadUnsafe logs that error and still writes the file checksum, so the pool is simply missing on the target and a restart won't retry it; with doThrow on, the idgen load aborts. idgen_id_pool.source is nullable in idgen's schema, so this is reachable.
The same gap sits one level out in IdentifierSourceDomainExporter: a pool whose backing source is one of the custom subclasses getAllInstances() skips is still exported, and the uuid written on line 38 then names a source that is in none of the exported files, which fails on import the same way.
AutoGenerationOptionDomainExporter.exportable() already applies this guard on the other side, dropping options whose source is a type we skip (the test there calls it a dangling reference). I'd mirror it for pools, skipping with a warning when the backing source is null or isn't a type handles() accepts, so the export loses one pool rather than producing a pool file that can't be imported. Leaving the column out isn't enough on its own, since Iniz reads it with get(header, true).
|
@claude review |
| if (source != null && !(source instanceof SequentialIdentifierGenerator) | ||
| && !(source instanceof RemoteIdentifierSource) && !(source instanceof IdentifierPool)) { |
There was a problem hiding this comment.
Now that IdentifierSourceDomainExporter.exportable() drops pools as well as custom types, this check no longer matches what the idgen domain will actually write. A pool with no backing source (the case the new guard was added for) is skipped there, but source instanceof IdentifierPool still passes here, so the option is exported.
If merged as-is, a server with an auto generation option on such a pool gets an autogenerationoptions.csv row whose Identifier Source uuid appears in none of the exported idgen files. On import, AutoGenerationOptionLineProcessor.fill resolves it through getIdentifierSourceByUuid, gets null, and calls setSource(null); source is not-null="true" in idgen's IdentifierSource.hbm.xml, so the save fails. With Iniz's default non-throwing mode the error is logged and BaseFileLoader still writes the file checksum, so the option is quietly missing on the target and a restart won't retry it.
Asking the idgen exporter whether it will really export the source, instead of repeating a type test here, would stop the two sides drifting again. It would also make the README's "auto generation options pointing at them" line true for the pool cases it now lists.
| if (StringUtils.isBlank(remote.getUser())) { | ||
| log.warn("Idgen: remote identifier source {} has no user; Iniz requires one on import", remote.getUuid()); | ||
| } |
There was a problem hiding this comment.
This is the warn-then-write-anyway shape that just came out of IdentifierPoolLineExporter, and it has the same outcome. user is nullable in idgen (not-null="false" in IdentifierSource.hbm.xml, and RemoteIdentifierSourceValidator only checks url), but Iniz reads it through RemoteIdentifierSourceLineProcessor.getRequiredProperty, which is CsvLine.get("user", true) plus a null check.
If merged as-is, exporting a remote source with no user writes a row with an empty user cell. CsvParser.fetchNextLine turns blank cells into nulls, so that row throws "user is required" on import; and if no remote source in the file has a user, the column is absent altogether and get(header, true) throws out of getColumn instead. Either way the source never lands on the target, and in non-throwing mode Iniz writes the checksum anyway, so it stays missing.
I would skip these in IdentifierSourceDomainExporter.exportable() with a warning, the same as a pool with no backing source. That does depend on the auto generation option guard being fixed first, or an option pointing at the newly skipped source becomes the dangling reference instead.
| if (!exportable(real)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
partition() runs after selection, so a pool dropped here has already been through Selector and is sitting in the ExportManifest. BuildManifest.of turns that manifest into the resolvedItems of the package.json at the zip root, so a REST-triggered build lists the pool as exported while idgen_pool.csv has no row for it.
Nothing fails, so I would not hold the PR for it, but this is the invariant handlesOnlyTheSourceTypesInizCanRepresent spells out ("handles() must agree with partition(), or selection puts sources in the manifest that export drops"), and that test still passes because it only covers the type rule. Moving the whole exportable() check into handles() would have Selector skip the pool outright and keep the manifest honest.
| if (BooleanUtils.isTrue(option.getRetired())) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
AutoGenerationOption has no retired column. idgen maps only id, uuid, identifier_type, location, source, manual_entry_enabled and automatic_generation_enabled (IdentifierSource.hbm.xml, and the liquibase changesets add nothing else), so a persisted option always reports the in-memory BaseOpenmrsMetadata default and this branch never fires. exportableFiltersRetiredOptions passes only because it retires an option that never came from the database.
Small thing, but together with the "retired options are filtered out by the domain exporter instead" note in AutoGenerationOptionLineExporter it reads as though retirement were supported for this domain, which AutoGenerationOptionsCsvParser.setRetired explicitly refuses on the import side.
2 participants
Adds exporters for the idgen module's two Initializer domains:
idgen_sequential/idgen_remote/idgen_poolCSVs with_order:headers so pools load after the sources they reference (Initializer infers the source type from which columns are present, so types can't share a file). Remote-source passwords are exported asproperty:placeholders, never plaintext.Description of what I changed
Issue I worked on
see https://openmrs.atlassian.net/browse/ME-32
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