From 6e60aba0192442c02991ed3ba75d8a133aa742be Mon Sep 17 00:00:00 2001 From: Martin Ndegwa Date: Thu, 28 Sep 2023 17:56:20 +0300 Subject: [PATCH] Refactor to Optimize processing of the Location Hierarchy (#14) * Perform parallel Processing of Location Hierarchy * Refactor location hieranchy processing for optimization * Optimization PR Clean up * Clean up --- pom.xml | 2 +- .../model/location/LocationHierarchyTree.java | 26 +-- .../smartregister/model/location/Tree.java | 171 ++++++++---------- .../model/location/TreeNode.java | 47 +++-- .../java/org/smartregister/utils/Utils.java | 10 + .../location/LocationHierarchyTreeTest.java | 61 ++++--- .../model/location/utils/TestUtils.java | 45 +++++ 7 files changed, 197 insertions(+), 165 deletions(-) create mode 100644 src/main/java/org/smartregister/utils/Utils.java create mode 100644 src/test/java/org/smartregister/model/location/utils/TestUtils.java diff --git a/pom.xml b/pom.xml index 704f951..7664bc3 100755 --- a/pom.xml +++ b/pom.xml @@ -6,7 +6,7 @@ org.smartregister fhir-common-utils - 0.0.10-SNAPSHOT + 0.0.11-SNAPSHOT diff --git a/src/main/java/org/smartregister/model/location/LocationHierarchyTree.java b/src/main/java/org/smartregister/model/location/LocationHierarchyTree.java index 3bdc36a..48b8d24 100755 --- a/src/main/java/org/smartregister/model/location/LocationHierarchyTree.java +++ b/src/main/java/org/smartregister/model/location/LocationHierarchyTree.java @@ -18,17 +18,19 @@ import ca.uhn.fhir.model.api.annotation.Child; import ca.uhn.fhir.model.api.annotation.DatatypeDef; import ca.uhn.fhir.util.ElementUtil; +import org.apache.commons.lang3.StringUtils; import org.hl7.fhir.instance.model.api.ICompositeType; import org.hl7.fhir.r4.model.Location; import org.hl7.fhir.r4.model.StringType; import org.hl7.fhir.r4.model.Type; -import org.apache.commons.lang3.StringUtils; import java.util.List; +import java.util.logging.Logger; @DatatypeDef(name = "LocationHierarchyTree") public class LocationHierarchyTree extends Type implements ICompositeType { + private static final Logger logger = Logger.getLogger(LocationHierarchyTree.class.getSimpleName()); @Child(name = "locationsHierarchy") private Tree locationsHierarchy; @@ -36,19 +38,17 @@ public LocationHierarchyTree() { this.locationsHierarchy = new Tree(); } - public void addLocation(Location l) { + public void addLocation(Location location) { StringType idString = new StringType(); - idString.setValue(l.getId()); - if (!locationsHierarchy.hasNode(idString.getValue())) { - if (l.getPartOf() == null || StringUtils.isEmpty(l.getPartOf().getReference())) { - locationsHierarchy.addNode(idString.getValue(), l.getName(), l, null); - } else { - // get Parent Location - StringType parentId = new StringType(); - parentId.setValue(l.getPartOf().getReference()); - locationsHierarchy.addNode( - idString.getValue(), l.getName(), l, parentId.getValue()); - } + idString.setValue(location.getId()); + if (location.getPartOf() == null || StringUtils.isEmpty(location.getPartOf().getReference())) { + locationsHierarchy.addNode(idString.getValue(), location.getName(), location, null); + } else { + // get Parent Location + StringType parentId = new StringType(); + parentId.setValue(location.getPartOf().getReference()); + locationsHierarchy.addNode( + idString.getValue(), location.getName(), location, parentId.getValue()); } } diff --git a/src/main/java/org/smartregister/model/location/Tree.java b/src/main/java/org/smartregister/model/location/Tree.java index 0654fd4..420fdb6 100755 --- a/src/main/java/org/smartregister/model/location/Tree.java +++ b/src/main/java/org/smartregister/model/location/Tree.java @@ -23,12 +23,15 @@ import org.hl7.fhir.r4.model.Location; import org.hl7.fhir.r4.model.StringType; import org.hl7.fhir.r4.model.Type; +import org.smartregister.utils.Utils; +import javax.annotation.Nullable; import java.util.ArrayList; import java.util.List; -import java.util.logging.Logger; - -import static org.smartregister.utils.Constants.SLASH_UNDERSCORE; +import java.util.Objects; +import java.util.Optional; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicReference; @DatatypeDef(name = "Tree") public class Tree extends Type implements ICompositeType { @@ -48,8 +51,6 @@ public class Tree extends Type implements ICompositeType { summary = false) private List parentChildren; - private static Logger logger = Logger.getLogger(Tree.class.getSimpleName()); - public SingleTreeNode getTree() { return listOfNodes; } @@ -59,57 +60,37 @@ public Tree() { parentChildren = new ArrayList<>(); } - private void addToParentChildRelation(String parent, String id) { + private void addToParentChildRelation(String parentId, String id) { if (parentChildren == null) { - parentChildren = new ArrayList<>(); - } - List kids = null; - if (parentChildren != null) { - for (int i = 0; i < parentChildren.size(); i++) { - kids = - parentChildren.get(i) != null - && parentChildren.get(i).getIdentifier() != null - && StringUtils.isNotBlank( - parentChildren.get(i).getIdentifier().getValue()) - && parentChildren - .get(i) - .getIdentifier() - .getValue() - .equals(parent) - ? parentChildren.get(i).getChildIdentifiers() - : null; - logger.info("Kids are : " + kids); - if (kids != null) { - break; - } - } + parentChildren = new CopyOnWriteArrayList<>(); } + List kids = getChildrenIdsByParentId(parentId); + if (kids == null) { kids = new ArrayList<>(); } StringType idStringType = new StringType(); - String idString = id; - if (idString.contains(SLASH_UNDERSCORE)) { - idString = idString.substring(0, idString.indexOf(SLASH_UNDERSCORE)); - } + String idString = Utils.cleanIdString(id); idStringType.setValue(idString); StringType parentStringType = new StringType(); - parentStringType.setValue(parent); + parentStringType.setValue(parentId); kids.add(idStringType); - Boolean setParentChildMap = false; - for (int i = 0; i < parentChildren.size(); i++) { - if (parentChildren.get(i) != null - && parentChildren.get(i).getIdentifier() != null - && StringUtils.isNotBlank(parentChildren.get(i).getIdentifier().getValue()) - && parentChildren.get(i).getIdentifier().getValue().equals(parent)) { - parentChildren.get(i).setChildIdentifiers(kids); - setParentChildMap = true; - } - } + AtomicReference setParentChildMap = new AtomicReference<>(false); + + List finalKids = kids; + parentChildren.parallelStream().filter(parentChildrenMap -> parentChildrenMap != null + && parentChildrenMap.getIdentifier() != null + && StringUtils.isNotBlank(parentChildrenMap.getIdentifier().getValue()) + && parentChildrenMap.getIdentifier().getValue().equals(parentId)).forEach(innerParentChildrenMap -> { + + innerParentChildrenMap.setChildIdentifiers(finalKids); + setParentChildMap.set(true); - if (!setParentChildMap) { + }); + + if (!setParentChildMap.get()) { ParentChildrenMap parentChildrenMap = new ParentChildrenMap(); parentChildrenMap.setIdentifier(parentStringType); parentChildrenMap.setChildIdentifiers(kids); @@ -117,90 +98,86 @@ private void addToParentChildRelation(String parent, String id) { } } + private List getChildrenIdsByParentId(String parentId) { + Optional> kidsOptional = parentChildren.parallelStream().filter(parentChildrenMap -> parentChildrenMap != null + && parentChildrenMap.getIdentifier() != null + && StringUtils.isNotBlank(parentChildrenMap.getIdentifier().getValue()) + && parentChildrenMap + .getIdentifier() + .getValue() + .equals(parentId)).map(ParentChildrenMap::getChildIdentifiers).filter(Objects::nonNull).findFirst(); + return kidsOptional.orElse(null); + } + public void addNode(String id, String label, Location node, String parentId) { if (listOfNodes == null) { listOfNodes = new SingleTreeNode(); } - // if node exists we should break since user should write optimized code and also tree can - // not have duplicates - if (hasNode(id)) { - throw new IllegalArgumentException("Node with ID " + id + " already exists in tree"); - } + // We only add node if it doesn't already exist, else log as an exception + TreeNode treenode = getNode(id); - TreeNode treeNode = makeNode(id, label, node, parentId); + if (treenode == null) { + TreeNode treeNode = makeNode(id, null, label, node, parentId); - if (parentId != null) { - addToParentChildRelation(parentId, id); + if (parentId != null) { - TreeNode parentNode = getNode(parentId); + addToParentChildRelation(parentId, id); + TreeNode parentNode = getNode(parentId); - // if parent exists add to it otherwise add as root for now - if (parentNode != null) { - parentNode.addChild(treeNode); - } else { - // if no parent exists add it as root node - String idString = (String) id; - if (idString.contains(SLASH_UNDERSCORE)) { - idString = idString.substring(0, idString.indexOf(SLASH_UNDERSCORE)); + // if parent exists add to it otherwise add as root for now + if (parentNode != null) { + parentNode.addChild(treeNode); + } else { + // if no parent exists add it as root node + SingleTreeNode singleTreeNode = getSingleTreeNode(id, treeNode); + listOfNodes = singleTreeNode; } - SingleTreeNode singleTreeNode = new SingleTreeNode(); - StringType treeNodeId = new StringType(); - treeNodeId.setValue(idString); - singleTreeNode.setTreeNodeId(treeNodeId); - singleTreeNode.setTreeNode(treeNode); + } else { + // if no parent add it as root node + SingleTreeNode singleTreeNode = getSingleTreeNode(id, treeNode); listOfNodes = singleTreeNode; } - } else { - // if no parent add it as root node - String idString = id; - if (idString.contains(SLASH_UNDERSCORE)) { - idString = idString.substring(0, idString.indexOf(SLASH_UNDERSCORE)); - } - SingleTreeNode singleTreeNode = new SingleTreeNode(); - StringType treeNodeId = new StringType(); - treeNodeId.setValue(idString); - singleTreeNode.setTreeNodeId(treeNodeId); - singleTreeNode.setTreeNode(treeNode); - listOfNodes = singleTreeNode; + } else { + throw new IllegalArgumentException("Node with ID " + id + " already exists in tree"); } } - private TreeNode makeNode(String id, String label, Location node, String parentId) { - TreeNode treenode = getNode(id); + private static SingleTreeNode getSingleTreeNode(String id, TreeNode treeNode) { + String idString = id; + idString = Utils.cleanIdString(idString); + SingleTreeNode singleTreeNode = new SingleTreeNode(); + StringType treeNodeId = new StringType(); + treeNodeId.setValue(idString); + singleTreeNode.setTreeNodeId(treeNodeId); + singleTreeNode.setTreeNode(treeNode); + return singleTreeNode; + } + + private TreeNode makeNode(String currentNodeId, TreeNode treenode, String label, Location node, String parentId) { if (treenode == null) { treenode = new TreeNode(); StringType nodeId = new StringType(); - String idString = (String) id; - if (idString.contains(SLASH_UNDERSCORE)) { - idString = idString.substring(0, idString.indexOf(SLASH_UNDERSCORE)); - } - nodeId.setValue((String) idString); + String idString = Utils.cleanIdString(currentNodeId); + nodeId.setValue(idString); treenode.setNodeId(nodeId); StringType labelString = new StringType(); labelString.setValue(label); treenode.setLabel(labelString); treenode.setNode(node); StringType parentIdString = new StringType(); - String parentIdStringVar = parentId; - - if (parentIdStringVar != null && parentIdStringVar.contains(SLASH_UNDERSCORE)) { - parentIdStringVar = - parentIdStringVar.substring(0, parentIdStringVar.indexOf(SLASH_UNDERSCORE)); - } + String parentIdStringVar = Utils.cleanIdString(parentId); parentIdString.setValue(parentIdStringVar); treenode.setParent(parentIdString); } return treenode; } + @Nullable public TreeNode getNode(String id) { // Check if id is any root node - String idString = id; - if (idString != null && idString.contains(SLASH_UNDERSCORE)) { - idString = idString.substring(0, idString.indexOf(SLASH_UNDERSCORE)); - } + String idString = Utils.cleanIdString(id); if (listOfNodes.getTreeNodeId() != null && StringUtils.isNotBlank(listOfNodes.getTreeNodeId().getValue()) @@ -215,10 +192,6 @@ public TreeNode getNode(String id) { return null; } - public boolean hasNode(String id) { - return getNode(id) != null; - } - public SingleTreeNode getListOfNodes() { return listOfNodes; } diff --git a/src/main/java/org/smartregister/model/location/TreeNode.java b/src/main/java/org/smartregister/model/location/TreeNode.java index 4bc3aff..2c5ded3 100755 --- a/src/main/java/org/smartregister/model/location/TreeNode.java +++ b/src/main/java/org/smartregister/model/location/TreeNode.java @@ -20,13 +20,14 @@ import ca.uhn.fhir.util.ElementUtil; import org.apache.commons.lang3.StringUtils; import org.hl7.fhir.instance.model.api.ICompositeType; -import org.hl7.fhir.r4.model.*; +import org.hl7.fhir.r4.model.Location; +import org.hl7.fhir.r4.model.StringType; +import org.hl7.fhir.r4.model.Type; +import org.smartregister.utils.Utils; import java.util.ArrayList; import java.util.List; -import static org.smartregister.utils.Constants.SLASH_UNDERSCORE; - @DatatypeDef(name = "TreeNode") public class TreeNode extends Type implements ICompositeType { @@ -185,30 +186,28 @@ public void addChild(TreeNode node) { children.add(childTreeNode); } - public TreeNode findChild(String id) { - String idString = (String) id; - if (idString.contains(SLASH_UNDERSCORE)) { - idString = idString.substring(0, idString.indexOf(SLASH_UNDERSCORE)); - } - if (children != null && children.size() > 0) { - for (int i = 0; i < children.size(); i++) { - if (children.get(i) != null) { - for (ChildTreeNode child : children) { - if (child != null - && child.getChildren() != null - && child.getChildren().getNodeId() != null - && StringUtils.isNotBlank( - child.getChildren().getNodeId().getValue()) - && child.getChildren().getNodeId().getValue().equals(idString)) { - return child.getChildren(); - } else if (child != null && child != null) { - TreeNode node = child.getChildren().findChild(idString); - if (node != null) return node; - } - } + public TreeNode findChild(String childId) { + + String idString = Utils.cleanIdString(childId); + if (children != null && !children.isEmpty()) { + for (ChildTreeNode child : children) { + if (isChildFound(child, idString)) { + return child.getChildren(); + } else if (child != null && child.getChildren() != null) { + TreeNode node = child.getChildren().findChild(idString); + if (node != null) return node; } } } return null; } + + private static boolean isChildFound(ChildTreeNode child, String idString) { + return child != null + && child.getChildren() != null + && child.getChildren().getNodeId() != null + && StringUtils.isNotBlank( + child.getChildren().getNodeId().getValue()) + && child.getChildren().getNodeId().getValue().equals(idString); + } } diff --git a/src/main/java/org/smartregister/utils/Utils.java b/src/main/java/org/smartregister/utils/Utils.java new file mode 100644 index 0000000..4c86c4c --- /dev/null +++ b/src/main/java/org/smartregister/utils/Utils.java @@ -0,0 +1,10 @@ +package org.smartregister.utils; + +import org.apache.commons.lang3.StringUtils; + +public class Utils { + public static String cleanIdString(String idString) { + return StringUtils.isNotBlank(idString) && idString.contains(Constants.SLASH_UNDERSCORE) ? + idString.substring(0, idString.indexOf(Constants.SLASH_UNDERSCORE)) : idString; + } +} diff --git a/src/test/java/org/smartregister/model/location/LocationHierarchyTreeTest.java b/src/test/java/org/smartregister/model/location/LocationHierarchyTreeTest.java index 3cbedb3..960edf4 100644 --- a/src/test/java/org/smartregister/model/location/LocationHierarchyTreeTest.java +++ b/src/test/java/org/smartregister/model/location/LocationHierarchyTreeTest.java @@ -51,41 +51,15 @@ public void testAddLocationWithoutChildLocations() { @Test public void testBuildTreeFromList() { - Location location1 = new Location(); - location1.setId("Location/1"); - location1.setName("Test Location"); - Reference partOfReference = new Reference(); - partOfReference.setReference(""); - location1.setPartOf(partOfReference); - - Location location2 = new Location(); - location2.setId("Location/2"); - location2.setName("Test Location 2"); - partOfReference = new Reference(); - partOfReference.setReference("Location/1"); - location2.setPartOf(partOfReference); - - Location location3 = new Location(); - location3.setId("Location/3"); - location3.setName("Test Location 3"); - partOfReference = new Reference(); - partOfReference.setReference("Location/2"); - location3.setPartOf(partOfReference); - + List locationList = getLocationList(); LocationHierarchyTree locationHierarchyTree = new LocationHierarchyTree(); - - List locationList = new ArrayList<>(); - locationList.add(location1); - locationList.add(location2); - locationList.add(location3); - locationHierarchyTree.buildTreeFromList(locationList); Tree tree = locationHierarchyTree.getLocationsHierarchy(); assertNotNull(tree); assertNotNull(tree.getTree()); assertEquals("Location/1", tree.getTree().getTreeNodeId().getValue()); assertEquals("Location/1", tree.getTree().getTreeNode().getNodeId().getValue()); - assertEquals("Test Location", tree.getTree().getTreeNode().getLabel().getValue()); + assertEquals("Test Location 1", tree.getTree().getTreeNode().getLabel().getValue()); assertNull(tree.getTree().getTreeNode().getParent().getValue()); assertEquals(1, tree.getTree().getTreeNode().getChildren().size()); @@ -248,4 +222,35 @@ public void testBuildTreeFromList() { .get(0) .getValue()); } + + private static List getLocationList() { + Location location1 = new Location(); + location1.setId("Location/1"); + location1.setName("Test Location 1"); + Reference partOfReference = new Reference(); + partOfReference.setReference(""); + location1.setPartOf(partOfReference); + + Location location2 = new Location(); + location2.setId("Location/2"); + location2.setName("Test Location 2"); + partOfReference = new Reference(); + partOfReference.setReference("Location/1"); + location2.setPartOf(partOfReference); + + Location location3 = new Location(); + location3.setId("Location/3"); + location3.setName("Test Location 3"); + partOfReference = new Reference(); + partOfReference.setReference("Location/2"); + location3.setPartOf(partOfReference); + + List locationList = new ArrayList<>(); + locationList.add(location1); + locationList.add(location2); + locationList.add(location3); + + + return locationList; + } } diff --git a/src/test/java/org/smartregister/model/location/utils/TestUtils.java b/src/test/java/org/smartregister/model/location/utils/TestUtils.java new file mode 100644 index 0000000..6525a6e --- /dev/null +++ b/src/test/java/org/smartregister/model/location/utils/TestUtils.java @@ -0,0 +1,45 @@ +package org.smartregister.model.location.utils; + +import org.hl7.fhir.r4.model.Location; +import org.hl7.fhir.r4.model.Reference; + +import java.util.ArrayList; +import java.util.List; + +public class TestUtils { + + public static final int TOTAL_LOCATIONS = 50000;// Generate 50K locations + public static final int TOTAL_CHILD_LOCATIONS_PER_NODE = 5; //Total sub-locations per location + public static List getTestLocations() { + + List locationList = new ArrayList<>(); + + int parentId = 1; + + for (int i = 1; i < TOTAL_LOCATIONS + 1; i++) { + + if(i == 1){ + parentId = 1; + } else if(i % TOTAL_CHILD_LOCATIONS_PER_NODE == 0 ){ + parentId++; + } + + locationList.add(getLocation("Location/" + i, "Test Location " + i, (i == 1 ? null : "Location/" + parentId))); + } + + return locationList; + } + + private static Location getLocation(String id, String name, String reference) { + + Location location = new Location(); + location.setId(id); + location.setName(name); + + Reference partOfReference = new Reference(); + partOfReference.setReference(reference); + location.setPartOf(partOfReference); + + return location; + } +}