ScriptFileWatcher fixes for entire directories (#3185)

* handle entire directories being moved in and out of a watched script path
* ensure sorted scripts when ScriptFileWatcher restarts or new directories are added

Signed-off-by: Cody Cutrer <cody@cutrer.us>

bundles/org.openhab.core.automation.module.script.rulesupport/src/main/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcher.java

@@ -24,9 +24,11 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
 import java.nio.file.WatchEvent;
 import java.nio.file.WatchEvent.Kind;
+import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.TreeSet;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
@@ -138,41 +140,52 @@ public abstract class AbstractScriptFileWatcher extends AbstractWatchService imp
         if (rootDirectory.exists()) {
             File[] files = rootDirectory.listFiles();
             if (files != null) {
+                Collection<ScriptFileReference> resources = new TreeSet<>();
                 for (File f : files) {
                     if (!f.isHidden()) {
-                        importResources(f);
+                        resources.addAll(collectResources(f));
                     }
                 }
+                resources.forEach(this::importFileWhenReady);
             }
         }
     }
 
     /**
-     * Imports resources from the specified file or directory.
+     * Collects all resources from the specified file or directory,
+     * possibly including subdirectories.
+     *
+     * The results will be sorted.
      *
      * @param file the file or directory to import resources from
      */
-    private void importResources(File file) {
+    private Collection<ScriptFileReference> collectResources(File file) {
-        if (file.exists()) {
+        Collection<ScriptFileReference> resources = new TreeSet<>();
-            File[] files = file.listFiles();
+        if (!file.exists()) {
-            if (files != null) {
+            return resources;
-                if (watchSubDirectories()) {
+        }
+
+        if (file.isDirectory()) {
+            if (watchSubDirectories()) {
+                File[] files = file.listFiles();
+                if (files != null) {
                     for (File f : files) {
                         if (!f.isHidden()) {
-                            importResources(f);
+                            resources.addAll(collectResources(f));
                         }
                     }
                 }
-            } else {
+            }
-                try {
+        } else {
-                    URL url = file.toURI().toURL();
+            try {
-                    importFileWhenReady(new ScriptFileReference(url));
+                URL url = file.toURI().toURL();
-                } catch (MalformedURLException e) {
+                resources.add(new ScriptFileReference(url));
-                    // can't happen for the 'file' protocol handler with a correctly formatted URI
+            } catch (MalformedURLException e) {
-                    logger.debug("Can't create a URL", e);
+                // can't happen for the 'file' protocol handler with a correctly formatted URI
-                }
+                logger.debug("Can't create a URL", e);
             }
         }
+        return resources;
     }
 
     @Override
@@ -190,13 +203,24 @@ public abstract class AbstractScriptFileWatcher extends AbstractWatchService imp
         File file = path.toFile();
         if (!file.isHidden()) {
             try {
-                URL fileUrl = file.toURI().toURL();
                 if (ENTRY_DELETE.equals(kind)) {
-                    removeFile(new ScriptFileReference(fileUrl));
+                    if (file.isDirectory()) {
+                        if (watchSubDirectories()) {
+                            synchronized (this) {
+                                String prefix = file.toString() + File.separator;
+                                var toRemove = loaded.stream()
+                                        .filter(f -> f.getScriptFileURL().getFile().startsWith(prefix))
+                                        .collect(Collectors.toList());
+                                toRemove.forEach(this::removeFile);
+                            }
+                        }
+                    } else {
+                        removeFile(new ScriptFileReference(file.toURI().toURL()));
+                    }
                 }
 
                 if (file.canRead() && (ENTRY_CREATE.equals(kind) || ENTRY_MODIFY.equals(kind))) {
-                    importFileWhenReady(new ScriptFileReference(fileUrl));
+                    collectResources(file).forEach(this::importFileWhenReady);
                 }
             } catch (MalformedURLException e) {
                 logger.error("malformed", e);
@@ -289,9 +313,7 @@ public abstract class AbstractScriptFileWatcher extends AbstractWatchService imp
             pending.removeAll(newlySupported);
         }
 
-        for (ScriptFileReference ref : newlySupported) {
+        newlySupported.forEach(this::importFileWhenReady);
-            importFileWhenReady(ref);
-        }
     }
 
     private synchronized void onStartLevelChanged(int newLevel) {

bundles/org.openhab.core.automation.module.script.rulesupport/src/test/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcherTest.java

@@ -363,4 +363,69 @@ class AbstractScriptFileWatcherTest {
         verify(scriptEngineManagerMock, timeout(10000).times(2)).createScriptEngine("js",
                 p.toFile().toURI().toString());
     }
+
+    @Test
+    public void testDirectoryAdded() {
+        when(scriptEngineManagerMock.isSupported("js")).thenReturn(true);
+        ScriptEngineContainer scriptEngineContainer = mock(ScriptEngineContainer.class);
+        when(scriptEngineContainer.getScriptEngine()).thenReturn(mock(ScriptEngine.class));
+        when(scriptEngineManagerMock.createScriptEngine(anyString(), anyString())).thenReturn(scriptEngineContainer);
+
+        updateStartLevel(100);
+
+        Path p1 = getFile("dir/script.js");
+        Path p2 = getFile("dir/script2.js");
+        Path d = p1.getParent();
+
+        scriptFileWatcher.processWatchEvent(null, ENTRY_CREATE, d);
+
+        verify(scriptEngineManagerMock, timeout(10000)).createScriptEngine("js", p1.toFile().toURI().toString());
+        verify(scriptEngineManagerMock, timeout(10000)).createScriptEngine("js", p2.toFile().toURI().toString());
+    }
+
+    @Test
+    public void testSortsAllFilesInNewDirectory() {
+        when(scriptEngineManagerMock.isSupported("js")).thenReturn(true);
+        ScriptEngineContainer scriptEngineContainer = mock(ScriptEngineContainer.class);
+        when(scriptEngineContainer.getScriptEngine()).thenReturn(mock(ScriptEngine.class));
+        when(scriptEngineManagerMock.createScriptEngine(anyString(), anyString())).thenReturn(scriptEngineContainer);
+
+        updateStartLevel(100);
+
+        Path p20 = getFile("dir/script.sl20.js");
+        Path p10 = getFile("dir/script2.sl10.js");
+        Path d = p10.getParent();
+
+        scriptFileWatcher.processWatchEvent(null, ENTRY_CREATE, d);
+
+        InOrder inOrder = inOrder(scriptEngineManagerMock);
+
+        inOrder.verify(scriptEngineManagerMock, timeout(10000).times(1)).createScriptEngine("js",
+                p10.toFile().toURI().toString());
+        inOrder.verify(scriptEngineManagerMock, timeout(10000).times(1)).createScriptEngine("js",
+                p20.toFile().toURI().toString());
+    }
+
+    @Test
+    public void testDirectoryRemoved() {
+        when(scriptEngineManagerMock.isSupported("js")).thenReturn(true);
+        ScriptEngineContainer scriptEngineContainer = mock(ScriptEngineContainer.class);
+        when(scriptEngineContainer.getScriptEngine()).thenReturn(mock(ScriptEngine.class));
+        when(scriptEngineManagerMock.createScriptEngine(anyString(), anyString())).thenReturn(scriptEngineContainer);
+
+        updateStartLevel(100);
+
+        Path p1 = getFile("dir/script.js");
+        Path p2 = getFile("dir/script2.js");
+        Path d = p1.getParent();
+
+        scriptFileWatcher.processWatchEvent(null, ENTRY_CREATE, p1);
+        scriptFileWatcher.processWatchEvent(null, ENTRY_CREATE, p2);
+        scriptFileWatcher.processWatchEvent(null, ENTRY_DELETE, d);
+
+        verify(scriptEngineManagerMock, timeout(10000)).createScriptEngine("js", p1.toFile().toURI().toString());
+        verify(scriptEngineManagerMock, timeout(10000)).createScriptEngine("js", p2.toFile().toURI().toString());
+        verify(scriptEngineManagerMock, timeout(10000)).removeEngine(p1.toFile().toURI().toString());
+        verify(scriptEngineManagerMock, timeout(10000)).removeEngine(p2.toFile().toURI().toString());
+    }
 }