[dsl] Model loading improvements (#1514)

* Model loading improvements

Signed-off-by: Kai Kreuzer <kai@openhab.org>

bundles/org.openhab.core.model.core/src/main/java/org/openhab/core/model/core/internal/ModelRepositoryImpl.java

@@ -98,20 +98,22 @@ public class ModelRepositoryImpl implements ModelRepository {
 
     @Override
     public boolean addOrRefreshModel(String name, final InputStream originalInputStream) {
+        logger.info("Loading model '{}'", name);
         Resource resource = null;
-        InputStream inputStream = null;
+        byte[] bytes = null;
-        try {
+        try (InputStream inputStream = originalInputStream) {
-            if (originalInputStream != null) {
+            bytes = inputStream.readAllBytes();
-                byte[] bytes = originalInputStream.readAllBytes();
             String validationResult = validateModel(name, new ByteArrayInputStream(bytes));
             if (validationResult != null) {
-                    logger.warn("Configuration model '{}' has errors, therefore ignoring it: {}", name,
+                logger.warn("Configuration model '{}' has errors, therefore ignoring it: {}", name, validationResult);
-                            validationResult);
                 removeModel(name);
                 return false;
             }
-                inputStream = new ByteArrayInputStream(bytes);
+        } catch (IOException e) {
+            logger.warn("Configuration model '{}' cannot be parsed correctly!", name, e);
+            return false;
         }
+        try (InputStream inputStream = new ByteArrayInputStream(bytes)) {
             resource = getResource(name);
             if (resource == null) {
                 synchronized (resourceSet) {
@@ -123,13 +125,6 @@ public class ModelRepositoryImpl implements ModelRepository {
                         Resource.Factory.Registry.INSTANCE.getExtensionToFactoryMap().remove("*");
                         resource = resourceSet.createResource(URI.createURI(name));
                         if (resource != null) {
-                            logger.info("Loading model '{}'", name);
-                            if (inputStream == null) {
-                                logger.warn(
-                                        "Resource '{}' not found. You have to pass an inputStream to create the resource.",
-                                        name);
-                                return false;
-                            }
                             resource.load(inputStream, resourceOptions);
                             notifyListeners(name, EventType.ADDED);
                             return true;
@@ -141,12 +136,7 @@ public class ModelRepositoryImpl implements ModelRepository {
             } else {
                 synchronized (resourceSet) {
                     resource.unload();
-                    logger.info("Refreshing model '{}'", name);
-                    if (inputStream == null) {
-                        resource.load(resourceOptions);
-                    } else {
                     resource.load(inputStream, resourceOptions);
-                    }
                     notifyListeners(name, EventType.MODIFIED);
                     return true;
                 }
@@ -156,13 +146,6 @@ public class ModelRepositoryImpl implements ModelRepository {
             if (resource != null) {
                 resourceSet.getResources().remove(resource);
             }
-        } finally {
-            if (inputStream != null) {
-                try {
-                    inputStream.close();
-                } catch (IOException e) {
-                }
-            }
         }
         return false;
     }
@@ -189,7 +172,7 @@ public class ModelRepositoryImpl implements ModelRepository {
             List<Resource> resourceListCopy = new ArrayList<>(resourceSet.getResources());
 
             return resourceListCopy.stream().filter(input -> {
-                return input != null && input.getURI().lastSegment().contains(".") && input.isLoaded()
+                return input.getURI().lastSegment().contains(".") && input.isLoaded()
                         && modelType.equalsIgnoreCase(input.getURI().fileExtension());
             }).map(from -> {
                 return from.getURI().path();
@@ -203,8 +186,9 @@ public class ModelRepositoryImpl implements ModelRepository {
             // Make a copy to avoid ConcurrentModificationException
             List<Resource> resourceListCopy = new ArrayList<>(resourceSet.getResources());
             for (Resource resource : resourceListCopy) {
-                if (resource != null && resource.getURI().lastSegment().contains(".") && resource.isLoaded()) {
+                if (resource.getURI().lastSegment().contains(".") && resource.isLoaded()
-                    if (modelType.equalsIgnoreCase(resource.getURI().fileExtension())) {
+                        && modelType.equalsIgnoreCase(resource.getURI().fileExtension())
+                        && !resource.getURI().lastSegment().startsWith("tmp_")) {
                     XtextResource xtextResource = (XtextResource) resource;
                     // It's not sufficient to discard the derived state.
                     // The quick & dirts solution is to reparse the whole resource.
@@ -216,7 +200,6 @@ public class ModelRepositoryImpl implements ModelRepository {
             }
         }
     }
-    }
 
     @Override
     public Set<String> removeAllModelsOfType(final String modelType) {
@@ -225,8 +208,9 @@ public class ModelRepositoryImpl implements ModelRepository {
             // Make a copy to avoid ConcurrentModificationException
             List<Resource> resourceListCopy = new ArrayList<>(resourceSet.getResources());
             for (Resource resource : resourceListCopy) {
-                if (resource != null && resource.getURI().lastSegment().contains(".") && resource.isLoaded()) {
+                if (resource.getURI().lastSegment().contains(".") && resource.isLoaded()
-                    if (modelType.equalsIgnoreCase(resource.getURI().fileExtension())) {
+                        && modelType.equalsIgnoreCase(resource.getURI().fileExtension())
+                        && !resource.getURI().lastSegment().startsWith("tmp_")) {
                     logger.debug("Removing resource '{}'", resource.getURI().lastSegment());
                     ret.add(resource.getURI().lastSegment());
                     resourceSet.getResources().remove(resource);
@@ -234,7 +218,6 @@ public class ModelRepositoryImpl implements ModelRepository {
                 }
             }
         }
-        }
         return ret;
     }
 

bundles/org.openhab.core.model.core/src/main/java/org/openhab/core/model/core/internal/folder/FolderObserver.java

@@ -38,6 +38,8 @@ import org.openhab.core.config.core.ConfigConstants;
 import org.openhab.core.model.core.ModelParser;
 import org.openhab.core.model.core.ModelRepository;
 import org.openhab.core.service.AbstractWatchService;
+import org.openhab.core.service.ReadyMarker;
+import org.openhab.core.service.ReadyService;
 import org.osgi.service.component.ComponentContext;
 import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
@@ -60,6 +62,11 @@ public class FolderObserver extends AbstractWatchService {
 
     /* the model repository is provided as a service */
     private final ModelRepository modelRepository;
+    private static final String READYMARKER_TYPE = "dsl";
+
+    private final ReadyService readyService;
+
+    private boolean activated;
 
     /* map that stores a list of valid file extensions for each folder */
     private final Map<String, String[]> folderFileExtMap = new ConcurrentHashMap<>();
@@ -72,18 +79,21 @@ public class FolderObserver extends AbstractWatchService {
     private final Map<String, File> nameFileMap = new HashMap<>();
 
     @Activate
-    public FolderObserver(final @Reference ModelRepository modelRepo) {
+    public FolderObserver(final @Reference ModelRepository modelRepo, final @Reference ReadyService readyService) {
         super(ConfigConstants.getConfigFolder());
 
         this.modelRepository = modelRepo;
+        this.readyService = readyService;
     }
 
     @Reference(cardinality = ReferenceCardinality.AT_LEAST_ONE, policy = ReferencePolicy.DYNAMIC)
     protected void addModelParser(ModelParser modelParser) {
         parsers.add(modelParser.getExtension());
 
-        // if the component isn't activated yet, ignoredFiles will be empty and thus this method does nothing
+        if (activated) {
             processIgnoredFiles(modelParser.getExtension());
+            readyService.markReady(new ReadyMarker(READYMARKER_TYPE, modelParser.getExtension()));
+        }
     }
 
     protected void removeModelParser(ModelParser modelParser) {
@@ -118,13 +128,14 @@ public class FolderObserver extends AbstractWatchService {
         }
 
         addModelsToRepo();
-
         super.activate();
+        this.activated = true;
     }
 
     @Override
     @Deactivate
     public void deactivate() {
+        this.activated = false;
         super.deactivate();
         deleteModelsFromRepo();
         this.ignoredFiles.clear();
@@ -186,6 +197,9 @@ public class FolderObserver extends AbstractWatchService {
                             }
                         }
                     }
+                    for (String ext : validExtension) {
+                        readyService.markReady(new ReadyMarker(READYMARKER_TYPE, ext));
+                    }
                 }
             }
         }

bundles/org.openhab.core.model.item/src/org/openhab/core/model/item/internal/GenericItemProvider.java

@@ -127,7 +127,7 @@ public class GenericItemProvider extends AbstractProvider<Item>
      *
      * @param factory The {@link ItemFactory} to add.
      */
-    @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC)
+    @Reference(cardinality = ReferenceCardinality.AT_LEAST_ONE, policy = ReferencePolicy.DYNAMIC)
     public void addItemFactory(ItemFactory factory) {
         itemFactorys.add(factory);
         dispatchBindingsPerItemType(null, factory.getSupportedItemTypes());

itests/org.openhab.core.model.core.tests/src/main/java/org/openhab/core/model/core/internal/folder/FolderObserverTest.java

@@ -44,6 +44,7 @@ import org.openhab.core.model.core.ModelParser;
 import org.openhab.core.model.core.ModelRepository;
 import org.openhab.core.model.core.ModelRepositoryChangeListener;
 import org.openhab.core.service.AbstractWatchService;
+import org.openhab.core.service.ReadyService;
 import org.openhab.core.test.java.JavaOSGiTest;
 import org.osgi.service.component.ComponentContext;
 
@@ -89,6 +90,9 @@ public class FolderObserverTest extends JavaOSGiTest {
     @Mock
     private ModelParser modelParser;
 
+    @Mock
+    private ReadyService readyService;
+
     @Mock
     private ComponentContext context;
     private Dictionary<String, Object> configProps;
@@ -124,7 +128,7 @@ public class FolderObserverTest extends JavaOSGiTest {
 
         modelRepo = new ModelRepoDummy();
 
-        folderObserver = new FolderObserver(modelRepo);
+        folderObserver = new FolderObserver(modelRepo, readyService);
         folderObserver.addModelParser(modelParser);
     }
 
@@ -348,7 +352,7 @@ public class FolderObserverTest extends JavaOSGiTest {
                 throw new RuntimeException("intentional failure.");
             }
         };
-        FolderObserver localFolderObserver = new FolderObserver(modelRepo);
+        FolderObserver localFolderObserver = new FolderObserver(modelRepo, readyService);
         localFolderObserver.addModelParser(modelParser);
 
         String validExtension = "java";