Fix the recursive group membership check (#4088)

Allow a group to be a member of its direct parent and also its parent's ancestors without raising an error. Looping membership is still detected and prevented as before, thus Stack Overflow is still avoided. Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
openhab · Feb 12, 2024 · 0efaf23 · 0efaf23
1 parent d806771
commit 0efaf23
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 8 deletions.
diff --git a/....io.rest.core/src/main/java/org/openhab/core/io/rest/core/item/EnrichedItemDTOMapper.java b/....io.rest.core/src/main/java/org/openhab/core/io/rest/core/item/EnrichedItemDTOMapper.java
@@ -105,10 +105,10 @@ private static EnrichedItemDTO map(Item item, ItemDTO itemDTO, boolean drillDown
                 for (Item member : groupItem.getMembers()) {
                     if (parents.contains(member)) {
                         LOGGER.error(
-                                "Recursive group membership found: {} is both, a direct or indirect parent and a child of {}.",
+                                "Recursive group membership found: {} is a member of {}, but it is also one of its ancestors.",
                                 member.getName(), groupItem.getName());
                     } else if (itemFilter == null || itemFilter.test(member)) {
-                        members.add(mapRecursive(member, itemFilter, uriBuilder, locale, parents));
+                        members.add(mapRecursive(member, itemFilter, uriBuilder, locale, new ArrayList<>(parents)));
                     }
                 }
                 memberDTOs = members.toArray(new EnrichedItemDTO[0]);

diff --git a/...rest.core/src/test/java/org/openhab/core/io/rest/core/item/EnrichedItemDTOMapperTest.java b/...rest.core/src/test/java/org/openhab/core/io/rest/core/item/EnrichedItemDTOMapperTest.java
@@ -95,7 +95,7 @@ public void testDirectRecursiveMembershipDoesNotThrowStackOverflowException() {
         assertDoesNotThrow(() -> EnrichedItemDTOMapper.map(groupItem1, true, null, null, null));
 
         assertLogMessage(EnrichedItemDTOMapper.class, LogLevel.ERROR,
-                "Recursive group membership found: group1 is both, a direct or indirect parent and a child of group2.");
+                "Recursive group membership found: group1 is a member of group2, but it is also one of its ancestors.");
     }
 
     @Test
@@ -111,7 +111,7 @@ public void testIndirectRecursiveMembershipDoesNotThrowStackOverflowException()
         assertDoesNotThrow(() -> EnrichedItemDTOMapper.map(groupItem1, true, null, null, null));
 
         assertLogMessage(EnrichedItemDTOMapper.class, LogLevel.ERROR,
-                "Recursive group membership found: group1 is both, a direct or indirect parent and a child of group3.");
+                "Recursive group membership found: group1 is a member of group3, but it is also one of its ancestors.");
     }
 
     @Test
@@ -128,4 +128,19 @@ public void testDuplicateMembershipOfPlainItemsDoesNotTriggerWarning() {
 
         assertNoLogMessage(EnrichedItemDTOMapper.class);
     }
+
+    @Test
+    public void testDuplicateMembershipOfGroupItemsDoesNotTriggerWarning() {
+        GroupItem groupItem1 = new GroupItem("group1");
+        GroupItem groupItem2 = new GroupItem("group2");
+        GroupItem groupItem3 = new GroupItem("group3");
+
+        groupItem1.addMember(groupItem2);
+        groupItem1.addMember(groupItem3);
+        groupItem2.addMember(groupItem3);
+
+        EnrichedItemDTOMapper.map(groupItem1, true, null, null, null);
+
+        assertNoLogMessage(EnrichedItemDTOMapper.class);
+    }
 }
diff --git a/...emantics/src/main/java/org/openhab/core/semantics/internal/SemanticsMetadataProvider.java b/...emantics/src/main/java/org/openhab/core/semantics/internal/SemanticsMetadataProvider.java
@@ -143,10 +143,10 @@ private void processItem(Item item, List<String> parentItems) {
             for (Item memberItem : groupItem.getMembers()) {
                 if (parentItems.contains(memberItem.getName())) {
                     logger.error(
-                            "Recursive group membership found: {} is both, a direct or indirect parent and a child of {}.",
+                            "Recursive group membership found: {} is a member of {}, but it is also one of its ancestors.",
                             memberItem.getName(), groupItem.getName());
                 } else {
-                    processItem(memberItem, parentItems);
+                    processItem(memberItem, new ArrayList<>(parentItems));
                 }
             }
         }

diff --git a/...tics/src/test/java/org/openhab/core/semantics/internal/SemanticsMetadataProviderTest.java b/...tics/src/test/java/org/openhab/core/semantics/internal/SemanticsMetadataProviderTest.java
@@ -250,7 +250,7 @@ public void testRecursiveGroupMembershipDoesNotResultInStackOverflowError() {
         assertDoesNotThrow(() -> semanticsMetadataProvider.added(groupItem1));
 
         assertLogMessage(SemanticsMetadataProvider.class, LogLevel.ERROR,
-                "Recursive group membership found: group1 is both, a direct or indirect parent and a child of group2.");
+                "Recursive group membership found: group1 is a member of group2, but it is also one of its ancestors.");
     }
 
     @Test
@@ -266,7 +266,7 @@ public void testIndirectRecursiveMembershipDoesNotThrowStackOverflowError() {
         assertDoesNotThrow(() -> semanticsMetadataProvider.added(groupItem1));
 
         assertLogMessage(SemanticsMetadataProvider.class, LogLevel.ERROR,
-                "Recursive group membership found: group1 is both, a direct or indirect parent and a child of group3.");
+                "Recursive group membership found: group1 is a member of group3, but it is also one of its ancestors.");
     }
 
     @Test
@@ -284,6 +284,21 @@ public void testDuplicateMembershipOfPlainItemsDoesNotTriggerWarning() {
         assertNoLogMessage(SemanticsMetadataProvider.class);
     }
 
+    @Test
+    public void testDuplicateMembershipOfGroupItemsDoesNotTriggerWarning() {
+        GroupItem groupItem1 = new GroupItem("group1");
+        GroupItem groupItem2 = new GroupItem("group2");
+        GroupItem groupItem3 = new GroupItem("group3");
+
+        groupItem1.addMember(groupItem2);
+        groupItem1.addMember(groupItem3);
+        groupItem2.addMember(groupItem3);
+
+        semanticsMetadataProvider.added(groupItem1);
+
+        assertNoLogMessage(SemanticsMetadataProvider.class);
+    }
+
     private @Nullable Metadata getMetadata(Item item) {
         return semanticsMetadataProvider.getAll().stream() //
                 .filter(metadata -> metadata.getUID().getItemName().equals(item.getName())) //