Fix "recursive membership detected" for plain items (#2918)

Signed-off-by: Jan N. Klug <github@klug.nrw>

bundles/org.openhab.core.io.rest.core/pom.xml

@@ -41,6 +41,12 @@
       <version>${project.version}</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>ch.qos.logback</groupId>
+      <artifactId>logback-classic</artifactId>
+      <version>1.2.3</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 
 </project>

bundles/org.openhab.core.io.rest.core/src/main/java/org/openhab/core/io/rest/core/item/EnrichedItemDTOMapper.java

@@ -64,16 +64,19 @@ public class EnrichedItemDTOMapper {
         return map(item, itemDTO, drillDown, itemFilter, uriBuilder, locale, new ArrayList<>());
     }
 
-    private static EnrichedItemDTO map(Item item, boolean drillDown, @Nullable Predicate<Item> itemFilter,
+    private static EnrichedItemDTO mapRecursive(Item item, @Nullable Predicate<Item> itemFilter,
             @Nullable UriBuilder uriBuilder, @Nullable Locale locale, List<Item> parents) {
         ItemDTO itemDTO = ItemDTOMapper.map(item);
-        return map(item, itemDTO, drillDown, itemFilter, uriBuilder, locale, parents);
+        return map(item, itemDTO, true, itemFilter, uriBuilder, locale, parents);
     }
 
     private static EnrichedItemDTO map(Item item, ItemDTO itemDTO, boolean drillDown,
             @Nullable Predicate<Item> itemFilter, @Nullable UriBuilder uriBuilder, @Nullable Locale locale,
             List<Item> parents) {
-        parents.add(item);
+        if (item instanceof GroupItem) {
+            // only add as parent item if it is a group, otherwise duplicate memberships trigger false warnings
+            parents.add(item);
+        }
         String state = item.getState().toFullString();
         String transformedState = considerTransformation(state, item, locale);
         if (transformedState != null && transformedState.equals(state)) {
@@ -88,7 +91,7 @@ public class EnrichedItemDTOMapper {
             link = null;
         }
 
-        EnrichedItemDTO enrichedItemDTO = null;
+        EnrichedItemDTO enrichedItemDTO;
 
         if (item instanceof GroupItem) {
             GroupItem groupItem = (GroupItem) item;
@@ -101,10 +104,10 @@ public class EnrichedItemDTOMapper {
                                 "Recursive group membership found: {} is both, a direct or indirect parent and a child of {}.",
                                 member.getName(), groupItem.getName());
                     } else if (itemFilter == null || itemFilter.test(member)) {
-                        members.add(map(member, drillDown, itemFilter, uriBuilder, locale, parents));
+                        members.add(mapRecursive(member, itemFilter, uriBuilder, locale, parents));
                     }
                 }
-                memberDTOs = members.toArray(new EnrichedItemDTO[members.size()]);
+                memberDTOs = members.toArray(new EnrichedItemDTO[0]);
             } else {
                 memberDTOs = new EnrichedItemDTO[0];
             }

bundles/org.openhab.core.io.rest.core/src/test/java/org/openhab/core/io/rest/core/item/EnrichedItemDTOMapperTest.java

@@ -17,11 +17,20 @@ import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.openhab.core.items.GenericItem;
 import org.openhab.core.items.GroupItem;
 import org.openhab.core.library.CoreItemFactory;
+import org.openhab.core.library.items.NumberItem;
 import org.openhab.core.test.java.JavaTest;
+import org.slf4j.LoggerFactory;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
 
 /**
  * @author Kai Kreuzer - Initial contribution
@@ -29,6 +38,23 @@ import org.openhab.core.test.java.JavaTest;
 @NonNullByDefault
 public class EnrichedItemDTOMapperTest extends JavaTest {
 
+    private @NonNullByDefault({}) ListAppender<ILoggingEvent> listAppender;
+
+    @BeforeEach
+    public void setup() {
+        // add logging interceptor
+        Logger logger = (Logger) LoggerFactory.getLogger(EnrichedItemDTOMapper.class);
+        logger.setLevel(Level.ERROR);
+        listAppender = new ListAppender<>();
+        listAppender.start();
+        logger.addAppender(listAppender);
+    }
+
+    @AfterEach
+    public void tearDown() {
+        listAppender.stop();
+    }
+
     @Test
     public void testFiltering() {
         CoreItemFactory itemFactory = new CoreItemFactory();
@@ -84,6 +110,12 @@ public class EnrichedItemDTOMapperTest extends JavaTest {
         groupItem2.addMember(groupItem1);
 
         assertDoesNotThrow(() -> EnrichedItemDTOMapper.map(groupItem1, true, null, null, null));
+
+        assertThat(listAppender.list.size(), is(1));
+        ILoggingEvent loggingEvent = listAppender.list.get(0);
+        assertThat(loggingEvent.getLevel(), is(Level.ERROR));
+        assertThat(loggingEvent.getFormattedMessage(), is(
+                "Recursive group membership found: group1 is both, a direct or indirect parent and a child of group2."));
     }
 
     @Test
@@ -97,5 +129,26 @@ public class EnrichedItemDTOMapperTest extends JavaTest {
         groupItem3.addMember(groupItem1);
 
         assertDoesNotThrow(() -> EnrichedItemDTOMapper.map(groupItem1, true, null, null, null));
+
+        assertThat(listAppender.list.size(), is(1));
+        ILoggingEvent loggingEvent = listAppender.list.get(0);
+        assertThat(loggingEvent.getLevel(), is(Level.ERROR));
+        assertThat(loggingEvent.getFormattedMessage(), is(
+                "Recursive group membership found: group1 is both, a direct or indirect parent and a child of group3."));
+    }
+
+    @Test
+    public void testDuplicateMembershipOfPlainItemsDoesNotTriggerWarning() {
+        GroupItem groupItem1 = new GroupItem("group1");
+        GroupItem groupItem2 = new GroupItem("group2");
+        NumberItem numberItem = new NumberItem("number");
+
+        groupItem1.addMember(groupItem2);
+        groupItem1.addMember(numberItem);
+        groupItem2.addMember(numberItem);
+
+        EnrichedItemDTOMapper.map(groupItem1, true, null, null, null);
+
+        assertThat(listAppender.list.size(), is(0));
     }
 }