From 41fb4ef670d7541e74728a7bcae91af0b73d4d23 Mon Sep 17 00:00:00 2001 From: Simon Kaufmann Date: Mon, 12 Jun 2017 14:33:44 +0200 Subject: [PATCH] improve handling of links related to model-items * change the order of how the GenericItemProvider registers links and items * enable the GenericItemChannelLinkProvider to handle deletions correctly * call GenericItemChannelLinkProvider from within GenericItemProvider only * enforce correct transaction handling in GenericItemChannelLinkProvider fixes #2491 relates to #3607 Signed-off-by: Simon Kaufmann --- .../item/internal/GenericItemProvider.java | 17 +- .../META-INF/MANIFEST.MF | 6 +- ....smarthome.model.thing.tests.launch.launch | 2 +- ...enericItemChannelLinkProviderJavaTest.java | 176 ++++++++++++++++++ .../genericitemchannellinkprovider.xml | 1 - .../GenericItemChannelLinkProvider.java | 75 +++----- 6 files changed, 218 insertions(+), 59 deletions(-) create mode 100644 bundles/model/org.eclipse.smarthome.model.thing.tests/src/org/eclipse/smarthome/model/thing/tests/GenericItemChannelLinkProviderJavaTest.java diff --git a/bundles/model/org.eclipse.smarthome.model.item/src/org/eclipse/smarthome/model/item/internal/GenericItemProvider.java b/bundles/model/org.eclipse.smarthome.model.item/src/org/eclipse/smarthome/model/item/internal/GenericItemProvider.java index ef1e179c9be..d2f1a7cc3ee 100644 --- a/bundles/model/org.eclipse.smarthome.model.item/src/org/eclipse/smarthome/model/item/internal/GenericItemProvider.java +++ b/bundles/model/org.eclipse.smarthome.model.item/src/org/eclipse/smarthome/model/item/internal/GenericItemProvider.java @@ -160,7 +160,7 @@ private Collection getItemsFromModel(String modelName) { return items; } - private void processBindingConfigsFromModel(String modelName) { + private void processBindingConfigsFromModel(String modelName, EventType type) { logger.debug("Processing binding configs for items from model '{}'", modelName); if (modelRepository != null) { @@ -175,10 +175,12 @@ private void processBindingConfigsFromModel(String modelName) { } // create items and read new binding configuration - for (ModelItem modelItem : model.getItems()) { - Item item = createItemFromModelItem(modelItem); - if (item != null) { - internalDispatchBindings(modelName, item, modelItem.getBindings()); + if (!EventType.REMOVED.equals(type)) { + for (ModelItem modelItem : model.getItems()) { + Item item = createItemFromModelItem(modelItem); + if (item != null) { + internalDispatchBindings(modelName, item, modelItem.getBindings()); + } } } @@ -341,15 +343,14 @@ public void modelChanged(String modelName, EventType type) { if (modelName.endsWith("items")) { switch (type) { case ADDED: - processBindingConfigsFromModel(modelName); Collection allNewItems = getItemsFromModel(modelName); itemsMap.put(modelName, allNewItems); for (Item item : allNewItems) { notifyListenersAboutAddedElement(item); } + processBindingConfigsFromModel(modelName, type); break; case MODIFIED: - processBindingConfigsFromModel(modelName); Map oldItems = toItemMap(itemsMap.get(modelName)); Map newItems = toItemMap(getAll()); itemsMap.put(modelName, newItems.values()); @@ -363,6 +364,7 @@ public void modelChanged(String modelName, EventType type) { notifyListenersAboutAddedElement(newItem); } } + processBindingConfigsFromModel(modelName, type); for (Item oldItem : oldItems.values()) { if (!newItems.containsKey(oldItem.getName())) { notifyListenersAboutRemovedElement(oldItem); @@ -370,6 +372,7 @@ public void modelChanged(String modelName, EventType type) { } break; case REMOVED: + processBindingConfigsFromModel(modelName, type); Collection itemsFromModel = getItemsFromModel(modelName); itemsMap.remove(modelName); for (Item item : itemsFromModel) { diff --git a/bundles/model/org.eclipse.smarthome.model.thing.tests/META-INF/MANIFEST.MF b/bundles/model/org.eclipse.smarthome.model.thing.tests/META-INF/MANIFEST.MF index 1e73a4e46ee..859ecf6789f 100644 --- a/bundles/model/org.eclipse.smarthome.model.thing.tests/META-INF/MANIFEST.MF +++ b/bundles/model/org.eclipse.smarthome.model.thing.tests/META-INF/MANIFEST.MF @@ -4,7 +4,8 @@ Bundle-Name: Eclipse SmartHome Thing Model Tests Bundle-Vendor: Eclipse.org/SmartHome Bundle-Version: 0.9.0.qualifier Bundle-SymbolicName: org.eclipse.smarthome.model.thing.tests; singleton:=true -Require-Bundle: org.eclipse.core.runtime +Require-Bundle: org.eclipse.core.runtime, + org.hamcrest Import-Package: groovy.lang, org.apache.log4j, org.codehaus.groovy.reflection, @@ -16,6 +17,7 @@ Import-Package: groovy.lang, org.eclipse.smarthome.core.thing.util, org.eclipse.smarthome.core.types, org.eclipse.smarthome.test, + org.eclipse.smarthome.test.java, org.hamcrest;core=split, org.hamcrest.core, org.junit;version="4.0.0", @@ -24,6 +26,8 @@ Import-Package: groovy.lang, org.junit.runner.notification;version="4.0.0", org.junit.runners;version="4.0.0", org.junit.runners.model;version="4.0.0", + org.mockito, + org.mockito.verification, org.osgi.service.component;version="1.2.2" Bundle-RequiredExecutionEnvironment: JavaSE-1.8 Fragment-Host: org.eclipse.smarthome.model.thing diff --git a/bundles/model/org.eclipse.smarthome.model.thing.tests/org.eclipse.smarthome.model.thing.tests.launch.launch b/bundles/model/org.eclipse.smarthome.model.thing.tests/org.eclipse.smarthome.model.thing.tests.launch.launch index 5fd0e1f65cf..ea9b502dd9b 100644 --- a/bundles/model/org.eclipse.smarthome.model.thing.tests/org.eclipse.smarthome.model.thing.tests.launch.launch +++ b/bundles/model/org.eclipse.smarthome.model.thing.tests/org.eclipse.smarthome.model.thing.tests.launch.launch @@ -34,7 +34,7 @@ - + diff --git a/bundles/model/org.eclipse.smarthome.model.thing.tests/src/org/eclipse/smarthome/model/thing/tests/GenericItemChannelLinkProviderJavaTest.java b/bundles/model/org.eclipse.smarthome.model.thing.tests/src/org/eclipse/smarthome/model/thing/tests/GenericItemChannelLinkProviderJavaTest.java new file mode 100644 index 00000000000..cc1e8efd9e2 --- /dev/null +++ b/bundles/model/org.eclipse.smarthome.model.thing.tests/src/org/eclipse/smarthome/model/thing/tests/GenericItemChannelLinkProviderJavaTest.java @@ -0,0 +1,176 @@ +/** + * Copyright (c) 2014-2017 by the respective copyright holders. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + */ +package org.eclipse.smarthome.model.thing.tests; + +import static org.hamcrest.CoreMatchers.*; +import static org.junit.Assert.assertThat; +import static org.mockito.Matchers.*; +import static org.mockito.Mockito.*; +import static org.mockito.MockitoAnnotations.initMocks; + +import java.io.ByteArrayInputStream; +import java.util.Collection; + +import org.eclipse.smarthome.core.common.registry.ProviderChangeListener; +import org.eclipse.smarthome.core.items.ItemRegistry; +import org.eclipse.smarthome.core.thing.ChannelUID; +import org.eclipse.smarthome.core.thing.Thing; +import org.eclipse.smarthome.core.thing.ThingRegistry; +import org.eclipse.smarthome.core.thing.link.ItemChannelLink; +import org.eclipse.smarthome.core.thing.link.ItemChannelLinkProvider; +import org.eclipse.smarthome.core.thing.link.ItemChannelLinkRegistry; +import org.eclipse.smarthome.model.core.ModelRepository; +import org.eclipse.smarthome.model.thing.internal.GenericItemChannelLinkProvider; +import org.eclipse.smarthome.test.java.JavaOSGiTest; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; + +/** + * + * @author Simon Kaufmann - initial contribution and API + * + */ +public class GenericItemChannelLinkProviderJavaTest extends JavaOSGiTest { + + private final static String THINGS_TESTMODEL_NAME = "test.things"; + private final static String ITEMS_TESTMODEL_NAME = "test.items"; + + private static final String ITEM = "test"; + private static final String CHANNEL = "test:test:test:test"; + private static final String LINK = ITEM + " -> " + CHANNEL; + + private ModelRepository modelRepository; + private ThingRegistry thingRegistry; + private ItemRegistry itemRegistry; + private ItemChannelLinkRegistry itemChannelLinkRegistry; + private ItemChannelLinkProvider itemChannelLinkProvider; + + @Mock + private ProviderChangeListener listener; + + @Before + public void setUp() { + initMocks(this); + + thingRegistry = getService(ThingRegistry.class); + assertThat(thingRegistry, is(notNullValue())); + modelRepository = getService(ModelRepository.class); + assertThat(modelRepository, is(notNullValue())); + itemRegistry = getService(ItemRegistry.class); + assertThat(itemRegistry, is(notNullValue())); + itemChannelLinkRegistry = getService(ItemChannelLinkRegistry.class); + assertThat(itemChannelLinkRegistry, is(notNullValue())); + itemChannelLinkProvider = getService(ItemChannelLinkProvider.class); + assertThat(itemChannelLinkProvider, is(notNullValue())); + modelRepository.removeModel(THINGS_TESTMODEL_NAME); + modelRepository.removeModel(ITEMS_TESTMODEL_NAME); + } + + @After + public void tearDown() { + modelRepository.removeModel(THINGS_TESTMODEL_NAME); + modelRepository.removeModel(ITEMS_TESTMODEL_NAME); + + assertThat(itemRegistry.getItems().size(), is(0)); + assertThat(itemChannelLinkRegistry.getAll().size(), is(0)); + } + + @Test + public void testIntegrationWithGenericItemProvider() throws Exception { + Thread.sleep(2500); // Wait for the ChannelItemProvider to join the game + + Collection things = thingRegistry.getAll(); + assertThat(things.size(), is(0)); + + String thingsModel = + + "Bridge hue:bridge:huebridge [ ipAddress = \"192.168.3.84\", userName = \"19fc3fa6fc870a4280a55f21315631f\" ] {" + + "LCT001 bulb3 [ lightId = \"3\" ]" + "LCT001 bulb4 [ lightId = \"3\" ]" + "}"; + modelRepository.addOrRefreshModel(THINGS_TESTMODEL_NAME, new ByteArrayInputStream(thingsModel.getBytes())); + Collection actualThings = thingRegistry.getAll(); + + assertThat(actualThings.size(), is(3)); + + assertThat(itemRegistry.getItems().size(), is(0)); + assertThat(itemChannelLinkRegistry.getAll().size(), is(0)); + + String itemsModel = "Color Light3Color \"Light3 Color\" { channel=\"hue:LCT001:huebridge:bulb3:color\" }" + + "Group:Switch:MAX TestSwitches"; + + // Initially load the model + modelRepository.addOrRefreshModel(ITEMS_TESTMODEL_NAME, new ByteArrayInputStream(itemsModel.getBytes())); + assertThat(itemRegistry.getItems().size(), is(2)); + assertThat(itemChannelLinkRegistry.getAll().size(), is(1)); + + // Update the model to run into GenericItemChannelLinkProvider's amnesia + modelRepository.addOrRefreshModel(ITEMS_TESTMODEL_NAME, new ByteArrayInputStream(itemsModel.getBytes())); + assertThat(itemRegistry.getItems().size(), is(2)); + assertThat(itemChannelLinkRegistry.getAll().size(), is(1)); + + // Remove the model (-> the link and therefore the item is kept) + modelRepository.removeModel(ITEMS_TESTMODEL_NAME); + assertThat(itemRegistry.getItems().size(), is(0)); + assertThat(itemChannelLinkRegistry.getAll().size(), is(0)); + + // Now add the model again + modelRepository.addOrRefreshModel(ITEMS_TESTMODEL_NAME, new ByteArrayInputStream(itemsModel.getBytes())); + assertThat(itemRegistry.getItems().size(), is(2)); // -> ensure ChannelItemProvider cleans up properly + assertThat(itemChannelLinkRegistry.getAll().size(), is(1)); + } + + @Test(expected = IllegalStateException.class) + public void testTransaction_requiredStart() { + GenericItemChannelLinkProvider provider = new GenericItemChannelLinkProvider(); + provider.stopConfigurationUpdate(ITEMS_TESTMODEL_NAME); + } + + @Test(expected = IllegalStateException.class) + public void testTransaction_requiredProcess() throws Exception { + GenericItemChannelLinkProvider provider = new GenericItemChannelLinkProvider(); + provider.processBindingConfiguration(ITEMS_TESTMODEL_NAME, "Number", ITEM, CHANNEL); + } + + @Test(expected = IllegalStateException.class) + public void testTransaction_duplicate() { + GenericItemChannelLinkProvider provider = new GenericItemChannelLinkProvider(); + provider.startConfigurationUpdate(ITEMS_TESTMODEL_NAME); + provider.startConfigurationUpdate(ITEMS_TESTMODEL_NAME); + } + + @SuppressWarnings("unchecked") + @Test + public void testNoAmnesia() throws Exception { + GenericItemChannelLinkProvider provider = new GenericItemChannelLinkProvider(); + provider.addProviderChangeListener(listener); + + provider.startConfigurationUpdate(ITEMS_TESTMODEL_NAME); + provider.processBindingConfiguration(ITEMS_TESTMODEL_NAME, "Number", ITEM, CHANNEL); + provider.stopConfigurationUpdate(ITEMS_TESTMODEL_NAME); + assertThat(provider.getAll().size(), is(1)); + assertThat(provider.getAll().iterator().next().toString(), is(LINK)); + verify(listener, only()).added(same(provider), eq(new ItemChannelLink(ITEM, new ChannelUID(CHANNEL)))); + + reset(listener); + provider.startConfigurationUpdate(ITEMS_TESTMODEL_NAME); + provider.processBindingConfiguration(ITEMS_TESTMODEL_NAME, "Number", ITEM, CHANNEL); + provider.stopConfigurationUpdate(ITEMS_TESTMODEL_NAME); + assertThat(provider.getAll().size(), is(1)); + assertThat(provider.getAll().iterator().next().toString(), is(LINK)); + verify(listener, only()).updated(same(provider), eq(new ItemChannelLink(ITEM, new ChannelUID(CHANNEL))), + eq(new ItemChannelLink(ITEM, new ChannelUID(CHANNEL)))); + + reset(listener); + provider.startConfigurationUpdate(ITEMS_TESTMODEL_NAME); + provider.stopConfigurationUpdate(ITEMS_TESTMODEL_NAME); + assertThat(provider.getAll().size(), is(0)); + verify(listener, only()).removed(same(provider), eq(new ItemChannelLink(ITEM, new ChannelUID(CHANNEL)))); + } + +} diff --git a/bundles/model/org.eclipse.smarthome.model.thing/OSGI-INF/genericitemchannellinkprovider.xml b/bundles/model/org.eclipse.smarthome.model.thing/OSGI-INF/genericitemchannellinkprovider.xml index ac41992407d..1b50ce97cb9 100644 --- a/bundles/model/org.eclipse.smarthome.model.thing/OSGI-INF/genericitemchannellinkprovider.xml +++ b/bundles/model/org.eclipse.smarthome.model.thing/OSGI-INF/genericitemchannellinkprovider.xml @@ -10,7 +10,6 @@ --> - diff --git a/bundles/model/org.eclipse.smarthome.model.thing/src/org/eclipse/smarthome/model/thing/internal/GenericItemChannelLinkProvider.java b/bundles/model/org.eclipse.smarthome.model.thing/src/org/eclipse/smarthome/model/thing/internal/GenericItemChannelLinkProvider.java index 7d2d9914d3a..912facf463c 100644 --- a/bundles/model/org.eclipse.smarthome.model.thing/src/org/eclipse/smarthome/model/thing/internal/GenericItemChannelLinkProvider.java +++ b/bundles/model/org.eclipse.smarthome.model.thing/src/org/eclipse/smarthome/model/thing/internal/GenericItemChannelLinkProvider.java @@ -8,6 +8,7 @@ package org.eclipse.smarthome.model.thing.internal; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -17,9 +18,6 @@ import org.eclipse.smarthome.core.thing.ChannelUID; import org.eclipse.smarthome.core.thing.link.ItemChannelLink; import org.eclipse.smarthome.core.thing.link.ItemChannelLinkProvider; -import org.eclipse.smarthome.model.core.EventType; -import org.eclipse.smarthome.model.core.ModelRepository; -import org.eclipse.smarthome.model.core.ModelRepositoryChangeListener; import org.eclipse.smarthome.model.item.BindingConfigParseException; import org.eclipse.smarthome.model.item.BindingConfigReader; @@ -33,8 +31,8 @@ * @author Alex Tugarev - Added parsing of multiple Channel UIDs * */ -public class GenericItemChannelLinkProvider extends AbstractProvider implements BindingConfigReader, - ItemChannelLinkProvider, ModelRepositoryChangeListener { +public class GenericItemChannelLinkProvider extends AbstractProvider + implements BindingConfigReader, ItemChannelLinkProvider { /** caches binding configurations. maps itemNames to {@link BindingConfig}s */ protected Map> itemChannelLinkMap = new ConcurrentHashMap<>(); @@ -45,9 +43,6 @@ public class GenericItemChannelLinkProvider extends AbstractProvider> contextMap = new ConcurrentHashMap<>(); - @SuppressWarnings("unused") - private ModelRepository modelRepository = null; - private Set previousItemNames; @Override @@ -63,6 +58,9 @@ public void validateItemType(String itemType, String bindingConfig) throws Bindi @Override public void processBindingConfiguration(String context, String itemType, String itemName, String bindingConfig) throws BindingConfigParseException { + if (previousItemNames == null) { + throw new IllegalStateException("No active update transaction"); + } String[] uids = bindingConfig.split(","); if (uids.length == 0) { throw new BindingConfigParseException( @@ -89,10 +87,8 @@ private void createItemChannelLink(String context, String itemName, String chann contextMap.put(context, itemNames); } itemNames.add(itemName); - if(previousItemNames != null ) { - previousItemNames.remove(itemName); - } - + previousItemNames.remove(itemName); + Set links = itemChannelLinkMap.get(itemName); if (links == null) { itemChannelLinkMap.put(itemName, links = new HashSet<>()); @@ -107,23 +103,31 @@ private void createItemChannelLink(String context, String itemName, String chann @Override public void startConfigurationUpdate(String context) { - previousItemNames = contextMap.get(context); + if (previousItemNames != null) { + throw new IllegalStateException("There already is an active update transaction"); + } + Set previous = contextMap.get(context); + previousItemNames = previous != null ? new HashSet<>(previous) : Collections.emptySet(); } @Override public void stopConfigurationUpdate(String context) { - if (previousItemNames != null) { - for (String itemName : previousItemNames) { - // we remove all binding configurations that were not processed - Set links = itemChannelLinkMap.remove(itemName); - if (links != null) { - for (ItemChannelLink removedItemChannelLink : links) { - notifyListenersAboutRemovedElement(removedItemChannelLink); - } + if (previousItemNames == null) { + throw new IllegalStateException("An update transaction has to be started first"); + } + for (String itemName : previousItemNames) { + // we remove all binding configurations that were not processed + Set links = itemChannelLinkMap.remove(itemName); + if (links != null) { + for (ItemChannelLink removedItemChannelLink : links) { + notifyListenersAboutRemovedElement(removedItemChannelLink); } } - contextMap.remove(context); } + if (contextMap.get(context) != null) { + contextMap.get(context).removeAll(previousItemNames); + } + previousItemNames = null; } @Override @@ -131,31 +135,4 @@ public Collection getAll() { return Lists.newLinkedList(Iterables.concat(itemChannelLinkMap.values())); } - public void setModelRepository(ModelRepository modelRepository) { - this.modelRepository = modelRepository; - modelRepository.addModelRepositoryChangeListener(this); - } - - public void unsetModelRepository(ModelRepository modelRepository) { - modelRepository.removeModelRepositoryChangeListener(this); - this.modelRepository = null; - } - - @Override - public void modelChanged(String modelName, EventType type) { - if (modelName.endsWith("items")) { - switch (type) { - case ADDED: - startConfigurationUpdate(modelName); - break; - case MODIFIED: - startConfigurationUpdate(modelName); - break; - case REMOVED: - startConfigurationUpdate(modelName); - stopConfigurationUpdate(modelName); - break; - } - } - } }