diff --git a/bundles/org.openhab.core.semantics/pom.xml b/bundles/org.openhab.core.semantics/pom.xml index e01133e4df9..6388ba0e3a9 100644 --- a/bundles/org.openhab.core.semantics/pom.xml +++ b/bundles/org.openhab.core.semantics/pom.xml @@ -20,6 +20,12 @@ org.openhab.core ${project.version} + + org.openhab.core.bundles + org.openhab.core.test + ${project.version} + test + org.ow2.asm asm diff --git a/bundles/org.openhab.core.semantics/src/main/java/org/openhab/core/semantics/internal/SemanticsMetadataProvider.java b/bundles/org.openhab.core.semantics/src/main/java/org/openhab/core/semantics/internal/SemanticsMetadataProvider.java index 535fc3f4a3e..863594d15b8 100644 --- a/bundles/org.openhab.core.semantics/src/main/java/org/openhab/core/semantics/internal/SemanticsMetadataProvider.java +++ b/bundles/org.openhab.core.semantics/src/main/java/org/openhab/core/semantics/internal/SemanticsMetadataProvider.java @@ -12,8 +12,8 @@ */ package org.openhab.core.semantics.internal; +import java.util.ArrayList; import java.util.Collection; -import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -40,6 +40,8 @@ import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Deactivate; import org.osgi.service.component.annotations.Reference; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * This {@link MetadataProvider} collects semantic information about items and provides them as metadata under the @@ -56,6 +58,8 @@ public class SemanticsMetadataProvider extends AbstractProvider implements ItemRegistryChangeListener, MetadataProvider { + private final Logger logger = LoggerFactory.getLogger(SemanticsMetadataProvider.class); + // the namespace to use for the metadata public static final String NAMESPACE = "semantics"; @@ -65,12 +69,7 @@ public class SemanticsMetadataProvider extends AbstractProvider private final Map>, String> propertyRelations = new HashMap<>(); // local cache of the created metadata as a map from itemName->Metadata - private final Map semantics = new TreeMap<>(new Comparator() { - @Override - public int compare(String s1, String s2) { - return s1.compareTo(s2); - } - }); + private final Map semantics = new TreeMap<>(String::compareTo); private final ItemRegistry itemRegistry; @@ -106,6 +105,10 @@ public Collection getAll() { * @param item the item to update the metadata for */ private void processItem(Item item) { + processItem(item, new ArrayList<>()); + } + + private void processItem(Item item, List parentItems) { MetadataKey key = new MetadataKey(NAMESPACE, item.getName()); Map configuration = new HashMap<>(); Class type = SemanticTags.getSemanticType(item); @@ -127,8 +130,15 @@ private void processItem(Item item) { } if (item instanceof GroupItem groupItem) { + parentItems.add(item.getName()); for (Item memberItem : groupItem.getMembers()) { - processItem(memberItem); + if (parentItems.contains(memberItem.getName())) { + logger.error( + "Recursive group membership found: {} is both, a direct or indirect parent and a child of {}.", + memberItem.getName(), groupItem.getName()); + } else { + processItem(memberItem, parentItems); + } } } } diff --git a/bundles/org.openhab.core.semantics/src/test/java/org/openhab/core/semantics/internal/SemanticsMetadataProviderTest.java b/bundles/org.openhab.core.semantics/src/test/java/org/openhab/core/semantics/internal/SemanticsMetadataProviderTest.java index a8de24199fc..0c67b9ae283 100644 --- a/bundles/org.openhab.core.semantics/src/test/java/org/openhab/core/semantics/internal/SemanticsMetadataProviderTest.java +++ b/bundles/org.openhab.core.semantics/src/test/java/org/openhab/core/semantics/internal/SemanticsMetadataProviderTest.java @@ -36,10 +36,12 @@ import org.openhab.core.items.Item; import org.openhab.core.items.ItemRegistry; import org.openhab.core.items.Metadata; +import org.openhab.core.library.items.NumberItem; import org.openhab.core.library.items.SwitchItem; import org.openhab.core.semantics.ManagedSemanticTagProvider; import org.openhab.core.semantics.SemanticTagRegistry; import org.openhab.core.semantics.model.DefaultSemanticTagProvider; +import org.openhab.core.test.java.JavaTest; /** * @author Simon Lamon - Initial contribution @@ -47,7 +49,7 @@ @NonNullByDefault @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) -public class SemanticsMetadataProviderTest { +public class SemanticsMetadataProviderTest extends JavaTest { private static final String ITEM_NAME = "switchItem"; @@ -61,6 +63,8 @@ public class SemanticsMetadataProviderTest { @BeforeEach public void beforeEach() throws Exception { + setupInterceptedLogger(SemanticsMetadataProvider.class, LogLevel.DEBUG); + when(managedSemanticTagProviderMock.getAll()).thenReturn(List.of()); SemanticTagRegistry semanticTagRegistry = new SemanticTagRegistryImpl(new DefaultSemanticTagProvider(), managedSemanticTagProviderMock); @@ -235,6 +239,51 @@ public void testGroupItemRemovedAfterMemberItemAdded() { assertNull(metadata.getConfiguration().get("hasLocation")); } + @Test + public void testRecursiveGroupMembershipDoesNotResultInStackOverflowError() { + GroupItem groupItem1 = new GroupItem("group1"); + GroupItem groupItem2 = new GroupItem("group2"); + + groupItem1.addMember(groupItem2); + groupItem2.addMember(groupItem1); + + 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."); + } + + @Test + public void testIndirectRecursiveMembershipDoesNotThrowStackOverflowError() { + GroupItem groupItem1 = new GroupItem("group1"); + GroupItem groupItem2 = new GroupItem("group2"); + GroupItem groupItem3 = new GroupItem("group3"); + + groupItem1.addMember(groupItem2); + groupItem2.addMember(groupItem3); + groupItem3.addMember(groupItem1); + + 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."); + } + + @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); + + semanticsMetadataProvider.added(groupItem1); + + assertNoLogMessage(SemanticsMetadataProvider.class); + } + private @Nullable Metadata getMetadata(Item item) { return semanticsMetadataProvider.getAll().stream() // .filter(metadata -> metadata.getUID().getItemName().equals(item.getName())) //