-
Notifications
You must be signed in to change notification settings - Fork 2
ME-31: Support Address hierarchy #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
41b777f
72f1828
2b052da
522d9b0
783de89
1b71054
b486b3c
a0e489a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,199 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public License, | ||
| * v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
| * obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||
| * the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||
| * | ||
| * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
| * graphic logo is a trademark of OpenMRS Inc. | ||
| */ | ||
| package org.openmrs.module.metadataexport.domain.addresshierarchy; | ||
|
|
||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.openmrs.OpenmrsObject; | ||
| import org.openmrs.annotation.OpenmrsProfile; | ||
| import org.openmrs.api.context.Context; | ||
| import org.openmrs.layout.address.AddressSupport; | ||
| import org.openmrs.layout.address.AddressTemplate; | ||
| import org.openmrs.module.addresshierarchy.AddressField; | ||
| import org.openmrs.module.addresshierarchy.AddressHierarchyEntry; | ||
| import org.openmrs.module.addresshierarchy.AddressHierarchyLevel; | ||
| import org.openmrs.module.addresshierarchy.config.AddressComponent; | ||
| import org.openmrs.module.addresshierarchy.config.AddressConfiguration; | ||
| import org.openmrs.module.addresshierarchy.config.AddressConfigurationLoader; | ||
| import org.openmrs.module.addresshierarchy.config.AddressHierarchyFile; | ||
| import org.openmrs.module.addresshierarchy.service.AddressHierarchyService; | ||
| import org.openmrs.module.initializer.Domain; | ||
| import org.openmrs.module.metadataexport.export.DomainExporter; | ||
| import org.openmrs.module.metadataexport.export.ExportContext; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Files; | ||
| import java.util.ArrayDeque; | ||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
| import java.util.Deque; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| @Slf4j | ||
| @Component | ||
| @OpenmrsProfile(modules = { "addresshierarchy:2.17.0" }) | ||
| public class AddressHierarchyDomainExporter implements DomainExporter<AddressHierarchyEntry> { | ||
|
|
||
| public static final String CONFIG_FILE_NAME = "addressConfiguration.xml"; | ||
|
|
||
| public static final String ENTRIES_FILE_NAME = "addresshierarchy.csv"; | ||
|
|
||
| public static final String ENTRY_DELIMITER = ","; | ||
|
|
||
| public static final String IDENTIFIER_DELIMITER = "^"; | ||
|
|
||
| private static final int DEFAULT_SIZE_MAPPING = 40; | ||
|
|
||
| @Override | ||
| public Domain getDomain() { | ||
| return Domain.ADDRESS_HIERARCHY; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean handles(OpenmrsObject instance) { | ||
| return instance instanceof AddressHierarchyEntry; | ||
| } | ||
|
|
||
| @Override | ||
| public Collection<AddressHierarchyEntry> getAllInstances() { | ||
| AddressHierarchyService service = Context.getService(AddressHierarchyService.class); | ||
| List<AddressHierarchyEntry> all = new ArrayList<>(); | ||
| for (AddressHierarchyLevel level : service.getOrderedAddressHierarchyLevels()) { | ||
| all.addAll(service.getAddressHierarchyEntriesByLevel(level)); | ||
| } | ||
| return all; | ||
| } | ||
|
|
||
| @Override | ||
| public Collection<? extends OpenmrsObject> getDependencies(AddressHierarchyEntry instance) { | ||
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| @Override | ||
| public void export(Collection<AddressHierarchyEntry> instances, ExportContext context) throws IOException { | ||
| List<AddressHierarchyLevel> levels = Context.getService(AddressHierarchyService.class) | ||
| .getOrderedAddressHierarchyLevels(); | ||
| if (levels.isEmpty()) { | ||
| log.warn("Address Hierarchy: no hierarchy levels are configured, nothing to export"); | ||
| return; | ||
| } | ||
|
|
||
| File domainDir = new File(new File(context.getOutputDir(), "configuration"), getDomain().getName()); | ||
| domainDir.mkdirs(); | ||
|
|
||
| AddressTemplate template = AddressSupport.getInstance().getDefaultLayoutTemplate(); | ||
| String configXml = buildAddressConfigurationXml(levels, template); | ||
| Files.write(new File(domainDir, CONFIG_FILE_NAME).toPath(), configXml.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| String entriesCsv = buildEntriesCsv(instances); | ||
| Files.write(new File(domainDir, ENTRIES_FILE_NAME).toPath(), entriesCsv.getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
|
|
||
| /** | ||
| * Rebuilds the {@code addressConfiguration.xml} content from the ordered hierarchy levels and the | ||
| * live address template, delegating the actual serialization to the addresshierarchy module so the | ||
| * output stays in lockstep with what it parses on import. | ||
| */ | ||
| String buildAddressConfigurationXml(List<AddressHierarchyLevel> levels, AddressTemplate template) { | ||
| Map<String, String> sizeMappings = template == null ? Collections.emptyMap() : template.getSizeMappings(); | ||
| Map<String, String> elementDefaults = template == null ? Collections.emptyMap() : template.getElementDefaults(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is not an exotic state: it is every database whose address template GP has not been rewritten by an Extending the guard you already have for a null template to the two maps individually (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
the guard only checked template == null, but getSizeMappings() . getElementDefaults() can themselves return null. ill update this PR @wikumChamith |
||
|
|
||
| AddressConfiguration configuration = new AddressConfiguration(); | ||
| for (AddressHierarchyLevel level : levels) { | ||
| AddressField field = level.getAddressField(); | ||
| String token = field == null ? null : field.getName(); | ||
|
|
||
| AddressComponent component = new AddressComponent(); | ||
| component.setField(field); | ||
| component.setNameMapping(level.getName()); | ||
| component.setSizeMapping(parseSize(sizeMappings.get(token))); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't the NPE be on levels without an address field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and it lands on this line rather than on Levels with no address field are reachable rather than theoretical: Worth knowing before picking the fix: a plain null check on the lookup would get you past the export but leave a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
already gurded |
||
| component.setElementDefault(elementDefaults.get(token)); | ||
| component.setRequiredInHierarchy(Boolean.TRUE.equals(level.getRequired())); | ||
| configuration.addAddressComponent(component); | ||
| } | ||
|
|
||
| if (template != null && template.getLineByLineFormat() != null) { | ||
| configuration.setLineByLineFormat(new ArrayList<>(template.getLineByLineFormat())); | ||
| } | ||
|
|
||
| AddressHierarchyFile file = new AddressHierarchyFile(); | ||
| file.setFilename(ENTRIES_FILE_NAME); | ||
| file.setEntryDelimiter(ENTRY_DELIMITER); | ||
| file.setIdentifierDelimiter(IDENTIFIER_DELIMITER); | ||
| configuration.setAddressHierarchyFile(file); | ||
|
|
||
| return AddressConfigurationLoader.writeToString(configuration); | ||
| } | ||
|
|
||
| /** | ||
| * Builds the headerless entries CSV from the entries alone (no extra queries): a leaf is any entry | ||
| * that is not some other entry's parent, and each leaf's row is its root-to-leaf path walked | ||
| * through {@link AddressHierarchyEntry#getParent()}. Rows are sorted for deterministic output. | ||
| */ | ||
| String buildEntriesCsv(Collection<AddressHierarchyEntry> instances) { | ||
| Set<Integer> parentIds = new HashSet<>(); | ||
| for (AddressHierarchyEntry entry : instances) { | ||
| AddressHierarchyEntry parent = entry.getParent(); | ||
| if (parent != null && parent.getId() != null) { | ||
| parentIds.add(parent.getId()); | ||
| } | ||
| } | ||
|
|
||
| List<String> rows = new ArrayList<>(); | ||
| for (AddressHierarchyEntry entry : instances) { | ||
| if (entry.getId() != null && parentIds.contains(entry.getId())) { | ||
| continue; // not a leaf: it is covered by its descendants' rows | ||
| } | ||
| rows.add(buildRow(entry)); | ||
| } | ||
| Collections.sort(rows); | ||
|
|
||
| StringBuilder csv = new StringBuilder(); | ||
| for (String row : rows) { | ||
| csv.append(row).append('\n'); | ||
| } | ||
| return csv.toString(); | ||
| } | ||
|
|
||
| private static String buildRow(AddressHierarchyEntry leaf) { | ||
| Deque<String> cells = new ArrayDeque<>(); | ||
| for (AddressHierarchyEntry current = leaf; current != null; current = current.getParent()) { | ||
| cells.addFirst(buildCell(current)); | ||
| } | ||
| return String.join(ENTRY_DELIMITER, cells); | ||
| } | ||
|
|
||
| private static String buildCell(AddressHierarchyEntry entry) { | ||
| String name = entry.getName() == null ? "" : entry.getName(); | ||
| if (StringUtils.isNotEmpty(entry.getUserGeneratedId())) { | ||
| return name + IDENTIFIER_DELIMITER + entry.getUserGeneratedId(); | ||
| } | ||
| return name; | ||
| } | ||
|
|
||
| private static int parseSize(String size) { | ||
| if (StringUtils.isBlank(size)) { | ||
| return DEFAULT_SIZE_MAPPING; | ||
| } | ||
| try { | ||
| return Integer.parseInt(size.trim()); | ||
| } | ||
| catch (NumberFormatException e) { | ||
| return DEFAULT_SIZE_MAPPING; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public License, | ||
| * v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
| * obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||
| * the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||
| * | ||
| * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
| * graphic logo is a trademark of OpenMRS Inc. | ||
| */ | ||
| package org.openmrs.module.metadataexport.domain.addresshierarchy; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.openmrs.layout.address.AddressTemplate; | ||
| import org.openmrs.module.addresshierarchy.AddressField; | ||
| import org.openmrs.module.addresshierarchy.AddressHierarchyEntry; | ||
| import org.openmrs.module.addresshierarchy.AddressHierarchyLevel; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| class AddressHierarchyDomainExporterTest { | ||
|
|
||
| private final AddressHierarchyDomainExporter exporter = new AddressHierarchyDomainExporter(); | ||
|
|
||
| private static AddressHierarchyEntry entry(int id, String name, AddressHierarchyEntry parent) { | ||
| AddressHierarchyEntry entry = new AddressHierarchyEntry(); | ||
| entry.setId(id); | ||
| entry.setName(name); | ||
| entry.setParent(parent); | ||
| return entry; | ||
| } | ||
|
|
||
| private static AddressHierarchyLevel level(AddressField field, String name, boolean required) { | ||
| AddressHierarchyLevel level = new AddressHierarchyLevel(); | ||
| level.setAddressField(field); | ||
| level.setName(name); | ||
| level.setRequired(required); | ||
| return level; | ||
| } | ||
|
|
||
| @Test | ||
| void buildEntriesCsv_emitsOneSortedRowPerLeafPath() { | ||
| AddressHierarchyEntry country = entry(1, "Cambodia", null); | ||
| AddressHierarchyEntry province = entry(2, "Banteay Meanchey", country); | ||
| AddressHierarchyEntry district = entry(3, "Mongkol Borei", province); | ||
| AddressHierarchyEntry otherProvince = entry(4, "Kampong Cham", country); | ||
|
|
||
| String csv = exporter.buildEntriesCsv(Arrays.asList(country, province, district, otherProvince)); | ||
|
|
||
| // country and the first province are interior nodes (parents), so only the two leaves are rows, | ||
| // each a full root-to-leaf path, sorted for deterministic output. | ||
| assertEquals("Cambodia,Banteay Meanchey,Mongkol Borei\n" + "Cambodia,Kampong Cham\n", csv); | ||
| } | ||
|
|
||
| @Test | ||
| void buildEntriesCsv_appendsUserGeneratedIdWithIdentifierDelimiter() { | ||
| AddressHierarchyEntry country = entry(1, "Cambodia", null); | ||
| AddressHierarchyEntry province = entry(2, "Banteay Meanchey", country); | ||
| province.setUserGeneratedId("BM"); | ||
|
|
||
| String csv = exporter.buildEntriesCsv(Arrays.asList(country, province)); | ||
|
|
||
| assertEquals("Cambodia,Banteay Meanchey^BM\n", csv); | ||
| } | ||
|
|
||
| @Test | ||
| void buildAddressConfigurationXml_reproducesComponentsFileAndFormat() { | ||
| Map<String, String> sizeMappings = new HashMap<>(); | ||
| sizeMappings.put(AddressField.COUNTRY.getName(), "40"); | ||
| sizeMappings.put(AddressField.STATE_PROVINCE.getName(), "40"); | ||
| Map<String, String> elementDefaults = new HashMap<>(); | ||
| elementDefaults.put(AddressField.COUNTRY.getName(), "addresshierarchy.cambodia"); | ||
|
|
||
| AddressTemplate template = new AddressTemplate("addressTemplate"); | ||
| template.setSizeMappings(sizeMappings); | ||
| template.setElementDefaults(elementDefaults); | ||
| template.setLineByLineFormat(Arrays.asList("stateProvince", "country")); | ||
|
|
||
| String xml = exporter | ||
| .buildAddressConfigurationXml(Arrays.asList(level(AddressField.COUNTRY, "Location.country", true), | ||
| level(AddressField.STATE_PROVINCE, "Location.province", true)), template); | ||
|
|
||
| assertTrue(xml.contains("<field>COUNTRY</field>"), xml); | ||
| assertTrue(xml.contains("<nameMapping>Location.country</nameMapping>"), xml); | ||
| assertTrue(xml.contains("<sizeMapping>40</sizeMapping>"), xml); | ||
| assertTrue(xml.contains("<elementDefault>addresshierarchy.cambodia</elementDefault>"), xml); | ||
| assertTrue(xml.contains("<requiredInHierarchy>true</requiredInHierarchy>"), xml); | ||
| assertTrue(xml.contains("<field>STATE_PROVINCE</field>"), xml); | ||
| assertTrue(xml.contains("<string>country</string>"), xml); | ||
| assertTrue(xml.contains("<filename>addresshierarchy.csv</filename>"), xml); | ||
| assertTrue(xml.contains("<entryDelimiter>,</entryDelimiter>"), xml); | ||
| assertTrue(xml.contains("<identifierDelimiter>^</identifierDelimiter>"), xml); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These check that the generated string looks right, not that the addresshierarchy module can read it back, which is the property that actually matters for an exporter whose only job is producing input for that module's loader. AddressConfiguration parsed = AddressConfigurationLoader.readFromString(xml);
AddressComponent first = parsed.getAddressComponents().get(0);
assertEquals(AddressField.COUNTRY, first.getField());
assertEquals(40, first.getSizeMapping());
assertEquals("addresshierarchy.csv", parsed.getAddressHierarchyFile().getFilename());Your call whether it is worth the churn. The reason I would take it is that it keeps holding when an addresshierarchy bump renames a field or changes an XStream alias, where ten |
||
| } | ||
|
|
||
| @Test | ||
| void buildAddressConfigurationXml_defaultsSizeWhenTemplateMissing() { | ||
| String xml = exporter | ||
| .buildAddressConfigurationXml(Arrays.asList(level(AddressField.COUNTRY, "Location.country", false)), null); | ||
|
|
||
| assertTrue(xml.contains("<sizeMapping>40</sizeMapping>"), xml); | ||
| assertTrue(xml.contains("<requiredInHierarchy>false</requiredInHierarchy>"), xml); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bare
^will not survive the round trip. The addresshierarchy module hands this delimiter straight toString.split, so it is a Java regex, and^there is the zero-width start anchor rather than a literal caret. I ran it on Java 17:"Cambodia^00".split("^")returns a single element,["Cambodia^00"], soAddressHierarchyImportUtil.splitIntoNameAndUserGeneratedId(2.17.0) never separates the id out.If merged as-is, re-importing an export taken from any database that populates
user_generated_idrenames every entry it touches: what should come back asCambodiawith id00becomes an entry literally namedCambodia^00, withuser_generated_idleft null. Nothing throws on either side, so neither the export nor the Iniz load reports anything wrong.It is the module's own default, which is presumably where it came from, but the sibling default is broken the same way (
"a|b".split("|")gives["a", "|", "b"]), so those two defaults look like they have simply never been exercised. The configs in the wild all avoid the bare caret:openmrs/openmrs-content-referenceapplication-demo,mekomsolutions/openmrs-config-haitiandmekomsolutions/ozone-distro-cambodiaall pair<entryDelimiter>,</entryDelimiter>with<identifierDelimiter>%</identifierDelimiter>(the demo's ownaddresshierarchy.csvnext to that file readsCambodia%00,Banteay Meanchey%01,Mongkol Borei%0101,...), andPIH/openmrs-config-zlescapes rather than swaps,\^and\|. I would go with%, since it matches the reference application demo and sits naturally alongside the comma you already chose for entries.buildEntriesCsv_appendsUserGeneratedIdWithIdentifierDelimiterpins the current character, so it needs the same edit. Asserting that a produced cell splits back into two parts onIDENTIFIER_DELIMITERwould keep a future change to it honest.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. good catch. this file use "% this " https://github.com/openmrs/openmrs-content-referenceapplication-demo/blob/main/configuration/backend_configuration/addresshierarchy/addresshierarchy.csv