YAML things provider: create things even if binding is not yet installed (#4753)

Makes it consistent with managed thing provider.

Removes the OSGi reference between YamlThingProvider and YamlModelRepository and as consequence removes the circular reference.

Signed-off-by: Laurent Garnier <[email protected]>

Author: lolodomo

bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/YamlModelRepository.java

@@ -21,7 +21,7 @@
  * The {@link YamlModelRepository} defines methods to update elements in a YAML model.
  *
  * @author Jan N. Klug - Initial contribution
- * @author Laurent Garnier - Added methods refreshModelElements and generateSyntaxFromElements
+ * @author Laurent Garnier - Added method generateSyntaxFromElements
  */
 @NonNullByDefault
 public interface YamlModelRepository {
@@ -31,14 +31,6 @@ public interface YamlModelRepository {
 
     void updateElementInModel(String modelName, YamlElement element);
 
-    /**
-     * Triggers the refresh of a certain type of elements in a given model.
-     *
-     * @param modelName the model name
-     * @param elementName the type of elements to refresh
-     */
-    void refreshModelElements(String modelName, String elementName);
-
     /**
      * Generate the YAML syntax from a provided list of elements.
      *

bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/YamlModelRepositoryImpl.java

@@ -77,7 +77,7 @@
  * @author Jan N. Klug - Refactored for multiple types per file and add modifying possibility
  * @author Laurent Garnier - Introduce version 2 using map instead of table
  * @author Laurent Garnier - Added basic version management
- * @author Laurent Garnier - Added methods refreshModelElements and generateSyntaxFromElements + new parameters
+ * @author Laurent Garnier - Added method generateSyntaxFromElements + new parameters
  *         for method isValid
  */
 @NonNullByDefault
@@ -564,35 +564,6 @@ public void updateElementInModel(String modelName, YamlElement element) {
         writeModel(modelName);
     }
 
-    @Override
-    @SuppressWarnings({ "rawtypes", "unchecked" })
-    public void refreshModelElements(String modelName, String elementName) {
-        logger.info("Refreshing {} from YAML model {}", elementName, modelName);
-        YamlModelWrapper model = modelCache.get(modelName);
-        if (model == null) {
-            logger.warn("Failed to refresh model {} because it is not known.", modelName);
-            return;
-        }
-
-        List<JsonNode> modelNodes = model.getNodesV1().get(elementName);
-        JsonNode modelMapNode = model.getNodes().get(elementName);
-        if (modelNodes == null && modelMapNode == null) {
-            logger.warn("Failed to refresh model {} because type {} is not known in the model.", modelName,
-                    elementName);
-            return;
-        }
-
-        getElementListeners(elementName, model.getVersion()).forEach(listener -> {
-            Class<? extends YamlElement> elementClass = listener.getElementClass();
-
-            List elements = parseJsonNodes(modelNodes != null ? modelNodes : List.of(), modelMapNode, elementClass,
-                    null, null);
-            if (!elements.isEmpty()) {
-                listener.updatedModel(modelName, elements);
-            }
-        });
-    }
-
     private void writeModel(String modelName) {
         YamlModelWrapper model = modelCache.get(modelName);
         if (model == null) {

bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/things/YamlThingProvider.java

@@ -32,9 +32,7 @@
 import org.openhab.core.config.core.ConfigUtil;
 import org.openhab.core.config.core.Configuration;
 import org.openhab.core.i18n.LocaleProvider;
-import org.openhab.core.model.yaml.YamlElementName;
 import org.openhab.core.model.yaml.YamlModelListener;
-import org.openhab.core.model.yaml.YamlModelRepository;
 import org.openhab.core.service.ReadyMarker;
 import org.openhab.core.service.ReadyService;
 import org.openhab.core.service.StartLevelService;
@@ -84,7 +82,6 @@ public class YamlThingProvider extends AbstractProvider<Thing>
 
     private final Logger logger = LoggerFactory.getLogger(YamlThingProvider.class);
 
-    private final YamlModelRepository modelRepository;
     private final BundleResolver bundleResolver;
     private final ThingTypeRegistry thingTypeRegistry;
     private final ChannelTypeRegistry channelTypeRegistry;
@@ -104,32 +101,8 @@ public void run() {
             logger.debug("Starting lazy retry thread");
             while (!queue.isEmpty()) {
                 for (QueueContent qc : queue) {
-                    logger.trace("Retry creating thing {}", qc.thingUID);
-                    Thing newThing = qc.thingHandlerFactory.createThing(qc.thingTypeUID, qc.configuration, qc.thingUID,
-                            qc.bridgeUID);
-                    if (newThing != null) {
-                        logger.debug("Successfully loaded thing \'{}\' during retry", qc.thingUID);
-                        Thing oldThing = null;
-                        for (Map.Entry<String, Collection<Thing>> entry : thingsMap.entrySet()) {
-                            oldThing = entry.getValue().stream().filter(t -> t.getUID().equals(newThing.getUID()))
-                                    .findFirst().orElse(null);
-                            if (oldThing != null) {
-                                mergeThing(newThing, oldThing);
-                                Collection<Thing> thingsForModel = Objects
-                                        .requireNonNull(thingsMap.get(entry.getKey()));
-                                thingsForModel.remove(oldThing);
-                                thingsForModel.add(newThing);
-                                logger.debug("Refreshing thing \'{}\' after successful retry", newThing.getUID());
-                                if (!ThingHelper.equals(oldThing, newThing)) {
-                                    notifyListenersAboutUpdatedElement(oldThing, newThing);
-                                }
-                                break;
-                            }
-                        }
-                        if (oldThing == null) {
-                            logger.debug("Refreshing thing \'{}\' after retry failed because thing is not found",
-                                    newThing.getUID());
-                        }
+                    if (retryCreateThing(qc.thingHandlerFactory, qc.thingTypeUID, qc.configuration, qc.thingUID,
+                            qc.bridgeUID)) {
                         queue.remove(qc);
                     }
                 }
@@ -153,12 +126,11 @@ private record QueueContent(ThingHandlerFactory thingHandlerFactory, ThingTypeUI
     }
 
     @Activate
-    public YamlThingProvider(final @Reference YamlModelRepository modelRepository,
-            final @Reference BundleResolver bundleResolver, final @Reference ThingTypeRegistry thingTypeRegistry,
+    public YamlThingProvider(final @Reference BundleResolver bundleResolver,
+            final @Reference ThingTypeRegistry thingTypeRegistry,
             final @Reference ChannelTypeRegistry channelTypeRegistry,
             final @Reference ConfigDescriptionRegistry configDescriptionRegistry,
             final @Reference LocaleProvider localeProvider) {
-        this.modelRepository = modelRepository;
         this.bundleResolver = bundleResolver;
         this.thingTypeRegistry = thingTypeRegistry;
         this.channelTypeRegistry = channelTypeRegistry;
@@ -242,6 +214,7 @@ public void removedModel(String modelName, Collection<YamlThingDTO> elements) {
 
     @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC)
     protected void addThingHandlerFactory(final ThingHandlerFactory thingHandlerFactory) {
+        logger.debug("addThingHandlerFactory {}", thingHandlerFactory.getClass().getSimpleName());
         thingHandlerFactories.add(thingHandlerFactory);
         thingHandlerFactoryAdded(thingHandlerFactory);
     }
@@ -278,16 +251,73 @@ public void onReadyMarkerRemoved(ReadyMarker readyMarker) {
         loadedXmlThingTypes.remove(readyMarker.getIdentifier());
     }
 
-    private void thingHandlerFactoryAdded(ThingHandlerFactory thingHandlerFactory) {
-        String bundleName = getBundleName(thingHandlerFactory);
-        if (bundleName != null && loadedXmlThingTypes.contains(bundleName)) {
-            logger.debug("Refreshing models due to new thing handler factory {}",
-                    thingHandlerFactory.getClass().getSimpleName());
-            thingsMap.keySet().forEach(modelName -> {
-                modelRepository.refreshModelElements(modelName,
-                        getElementClass().getAnnotation(YamlElementName.class).value());
-            });
+    private void thingHandlerFactoryAdded(ThingHandlerFactory handlerFactory) {
+        logger.debug("thingHandlerFactoryAdded {} isThingHandlerFactoryReady={}",
+                handlerFactory.getClass().getSimpleName(), isThingHandlerFactoryReady(handlerFactory));
+        if (isThingHandlerFactoryReady(handlerFactory)) {
+            if (!thingsMap.isEmpty()) {
+                logger.debug("Refreshing models due to new thing handler factory {}",
+                        handlerFactory.getClass().getSimpleName());
+                thingsMap.keySet().forEach(modelName -> {
+                    List<Thing> things = thingsMap.getOrDefault(modelName, List.of()).stream()
+                            .filter(th -> handlerFactory.supportsThingType(th.getThingTypeUID())).toList();
+                    if (!things.isEmpty()) {
+                        logger.info("Refreshing YAML model {} ({} things with {})", modelName, things.size(),
+                                handlerFactory.getClass().getSimpleName());
+                        things.forEach(thing -> {
+                            if (!retryCreateThing(handlerFactory, thing.getThingTypeUID(), thing.getConfiguration(),
+                                    thing.getUID(), thing.getBridgeUID())) {
+                                // Possible cause: Asynchronous loading of the XML files
+                                // Add the data to the queue in order to retry it later
+                                logger.debug(
+                                        "ThingHandlerFactory \'{}\' claimed it can handle \'{}\' type but actually did not. Queued for later refresh.",
+                                        handlerFactory.getClass().getSimpleName(), thing.getThingTypeUID());
+                                queueRetryThingCreation(handlerFactory, thing.getThingTypeUID(),
+                                        thing.getConfiguration(), thing.getUID(), thing.getBridgeUID());
+                            }
+                        });
+                    } else {
+                        logger.debug("No refresh needed from YAML model {}", modelName);
+                    }
+                });
+            } else {
+                logger.debug("No things yet loaded; no need to trigger a refresh due to new thing handler factory");
+            }
+        }
+    }
+
+    private boolean retryCreateThing(ThingHandlerFactory handlerFactory, ThingTypeUID thingTypeUID,
+            Configuration configuration, ThingUID thingUID, @Nullable ThingUID bridgeUID) {
+        logger.trace("Retry creating thing {}", thingUID);
+        Thing newThing = handlerFactory.createThing(thingTypeUID, configuration, thingUID, bridgeUID);
+        if (newThing != null) {
+            logger.debug("Successfully loaded thing \'{}\' during retry", thingUID);
+            Thing oldThing = null;
+            for (Collection<Thing> modelThings : thingsMap.values()) {
+                oldThing = modelThings.stream().filter(t -> t.getUID().equals(newThing.getUID())).findFirst()
+                        .orElse(null);
+                if (oldThing != null) {
+                    mergeThing(newThing, oldThing);
+                    modelThings.remove(oldThing);
+                    modelThings.add(newThing);
+                    logger.debug("Refreshing thing \'{}\' after successful retry", newThing.getUID());
+                    if (!ThingHelper.equals(oldThing, newThing)) {
+                        notifyListenersAboutUpdatedElement(oldThing, newThing);
+                    }
+                    break;
+                }
+            }
+            if (oldThing == null) {
+                logger.debug("Refreshing thing \'{}\' after retry failed because thing is not found",
+                        newThing.getUID());
+            }
         }
+        return newThing != null;
+    }
+
+    private boolean isThingHandlerFactoryReady(ThingHandlerFactory thingHandlerFactory) {
+        String bundleName = getBundleName(thingHandlerFactory);
+        return bundleName != null && loadedXmlThingTypes.contains(bundleName);
     }
 
     private @Nullable String getBundleName(ThingHandlerFactory thingHandlerFactory) {
@@ -300,20 +330,6 @@ private void thingHandlerFactoryAdded(ThingHandlerFactory thingHandlerFactory) {
         String[] segments = thingUID.getAsString().split(AbstractUID.SEPARATOR);
         ThingTypeUID thingTypeUID = new ThingTypeUID(thingUID.getBindingId(), segments[1]);
 
-        ThingHandlerFactory handlerFactory = thingHandlerFactories.stream()
-                .filter(thf -> thf.supportsThingType(thingTypeUID)).findFirst().orElse(null);
-        if (handlerFactory == null) {
-            if (modelLoaded) {
-                logger.info("No ThingHandlerFactory found for thing {} (thing-type is {}). Deferring initialization.",
-                        thingUID, thingTypeUID);
-            }
-            return null;
-        }
-        String bundleName = getBundleName(handlerFactory);
-        if (bundleName == null || !loadedXmlThingTypes.contains(bundleName)) {
-            return null;
-        }
-
         ThingType thingType = thingTypeRegistry.getThingType(thingTypeUID, localeProvider.getLocale());
         ThingUID bridgeUID = thingDto.bridge != null ? new ThingUID(thingDto.bridge) : null;
         Configuration configuration = new Configuration(thingDto.config);
@@ -333,22 +349,22 @@ private void thingHandlerFactoryAdded(ThingHandlerFactory thingHandlerFactory) {
 
         Thing thing = thingBuilder.build();
 
-        Thing thingFromHandler = handlerFactory.createThing(thingTypeUID, configuration, thingUID, bridgeUID);
-        if (thingFromHandler != null) {
-            mergeThing(thingFromHandler, thing);
-            logger.debug("Successfully loaded thing \'{}\'", thingUID);
-        } else {
-            // Possible cause: Asynchronous loading of the XML files
-            // Add the data to the queue in order to retry it later
-            logger.debug(
-                    "ThingHandlerFactory \'{}\' claimed it can handle \'{}\' type but actually did not. Queued for later refresh.",
-                    handlerFactory.getClass().getSimpleName(), thingTypeUID);
-            queue.add(new QueueContent(handlerFactory, thingTypeUID, configuration, thingUID, bridgeUID));
-            Thread thread = lazyRetryThread;
-            if (thread == null || !thread.isAlive()) {
-                thread = new Thread(lazyRetryRunnable);
-                lazyRetryThread = thread;
-                thread.start();
+        Thing thingFromHandler = null;
+        ThingHandlerFactory handlerFactory = thingHandlerFactories.stream()
+                .filter(thf -> isThingHandlerFactoryReady(thf) && thf.supportsThingType(thingTypeUID)).findFirst()
+                .orElse(null);
+        if (handlerFactory != null) {
+            thingFromHandler = handlerFactory.createThing(thingTypeUID, configuration, thingUID, bridgeUID);
+            if (thingFromHandler != null) {
+                mergeThing(thingFromHandler, thing);
+                logger.debug("Successfully loaded thing \'{}\'", thingUID);
+            } else {
+                // Possible cause: Asynchronous loading of the XML files
+                // Add the data to the queue in order to retry it later
+                logger.debug(
+                        "ThingHandlerFactory \'{}\' claimed it can handle \'{}\' type but actually did not. Queued for later refresh.",
+                        handlerFactory.getClass().getSimpleName(), thingTypeUID);
+                queueRetryThingCreation(handlerFactory, thingTypeUID, configuration, thingUID, bridgeUID);
             }
         }
 
@@ -383,7 +399,7 @@ private List<Channel> createChannels(ThingTypeUID thingTypeUID, ThingUID thingUI
                                 configDescriptionRegistry.getConfigDescription(descUriO));
                     }
                 } else {
-                    logger.warn("Channel type {} could not be found.", channelTypeUID);
+                    logger.warn("Channel type {} could not be found for thing '{}'.", channelTypeUID, thingUID);
                 }
             }
 
@@ -415,7 +431,12 @@ private List<Channel> createChannels(ThingTypeUID thingTypeUID, ThingUID thingUI
     }
 
     private void mergeThing(Thing target, Thing source) {
-        target.setLabel(source.getLabel());
+        String label = source.getLabel();
+        if (label == null) {
+            ThingType thingType = thingTypeRegistry.getThingType(target.getThingTypeUID(), localeProvider.getLocale());
+            label = thingType != null ? thingType.getLabel() : null;
+        }
+        target.setLabel(label);
         target.setLocation(source.getLocation());
         target.setBridgeUID(source.getBridgeUID());
 
@@ -439,4 +460,15 @@ private void mergeThing(Thing target, Thing source) {
         // add the channels only defined in source list to the target list
         ThingHelper.addChannelsToThing(target, channelsToAdd);
     }
+
+    private void queueRetryThingCreation(ThingHandlerFactory handlerFactory, ThingTypeUID thingTypeUID,
+            Configuration configuration, ThingUID thingUID, @Nullable ThingUID bridgeUID) {
+        queue.add(new QueueContent(handlerFactory, thingTypeUID, configuration, thingUID, bridgeUID));
+        Thread thread = lazyRetryThread;
+        if (thread == null || !thread.isAlive()) {
+            thread = new Thread(lazyRetryRunnable);
+            lazyRetryThread = thread;
+            thread.start();
+        }
+    }
 }