[transform.map] Refactor service to use TransformationConfigurationRegistry (#12433)

This is the reference implementation for using the new `TransformationConfigurationRegistry`. The localization tests can be omitted as this is covered by tests in the core component.

Signed-off-by: Jan N. Klug <github@klug.nrw>

bundles/org.openhab.transform.map/src/main/java/org/openhab/transform/map/internal/MapTransformationService.java

@@ -12,22 +12,30 @@
  */
 package org.openhab.transform.map.internal;
 
-import java.io.FileReader;
 import java.io.IOException;
+import java.io.StringReader;
 import java.net.URI;
 import java.util.Collection;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Properties;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.openhab.core.common.registry.RegistryChangeListener;
 import org.openhab.core.config.core.ConfigOptionProvider;
 import org.openhab.core.config.core.ParameterOption;
-import org.openhab.core.transform.AbstractFileTransformationService;
+import org.openhab.core.transform.TransformationConfiguration;
+import org.openhab.core.transform.TransformationConfigurationRegistry;
 import org.openhab.core.transform.TransformationException;
 import org.openhab.core.transform.TransformationService;
+import org.osgi.service.component.annotations.Activate;
 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;
 
@@ -37,31 +45,46 @@ import org.slf4j.LoggerFactory;
  *
  * @author Kai Kreuzer - Initial contribution and API
  * @author Gaël L'hopital - Make it localizable
+ * @author Jan N. Klug - Refactored to use {@link TransformationConfigurationRegistry}
  */
 @NonNullByDefault
 @Component(service = { TransformationService.class, ConfigOptionProvider.class }, property = {
         "openhab.transform=MAP" })
-public class MapTransformationService extends AbstractFileTransformationService<Properties>
+public class MapTransformationService
-        implements ConfigOptionProvider {
+        implements TransformationService, ConfigOptionProvider, RegistryChangeListener<TransformationConfiguration> {
-
     private final Logger logger = LoggerFactory.getLogger(MapTransformationService.class);
 
     private static final String PROFILE_CONFIG_URI = "profile:transform:MAP";
     private static final String CONFIG_PARAM_FUNCTION = "function";
-    private static final String[] FILE_NAME_EXTENSIONS = { "map" };
+    private static final Set<String> SUPPORTED_CONFIGURATION_TYPES = Set.of("map");
+
+    private final TransformationConfigurationRegistry transformationConfigurationRegistry;
+    private final Map<String, Properties> cachedTransformations = new ConcurrentHashMap<>();
+
+    @Activate
+    public MapTransformationService(
+            @Reference TransformationConfigurationRegistry transformationConfigurationRegistry) {
+        this.transformationConfigurationRegistry = transformationConfigurationRegistry;
+        transformationConfigurationRegistry.addRegistryChangeListener(this);
+    }
+
+    @Deactivate
+    public void deactivate() {
+        transformationConfigurationRegistry.removeRegistryChangeListener(this);
+    }
 
-    /**
-     * <p>
-     * Transforms the input <code>source</code> by mapping it to another string. It expects the mappings to be read from
-     * a file which is stored under the 'configurations/transform' folder. This file should be in property syntax, i.e.
-     * simple lines with "key=value" pairs. To organize the various transformations one might use subfolders.
-     *
-     * @param properties the list of properties which contains the key value pairs for the mapping.
-     * @param source the input to transform
-     * @return the transformed result or null if the transformation couldn't be completed for any reason.
-     */
     @Override
-    protected @Nullable String internalTransform(Properties properties, String source) throws TransformationException {
+    public @Nullable String transform(String function, String source) throws TransformationException {
+        // always get a configuration from the registry to account for changed system locale
+        TransformationConfiguration transformationConfiguration = transformationConfigurationRegistry.get(function,
+                null);
+
+        if (transformationConfiguration != null) {
+            if (!cachedTransformations.containsKey(transformationConfiguration.getUID())) {
+                importConfiguration(transformationConfiguration);
+            }
+            Properties properties = cachedTransformations.get(function);
+            if (properties != null) {
                 String target = properties.getProperty(source);
 
                 if (target == null) {
@@ -74,28 +97,48 @@ public class MapTransformationService extends AbstractFileTransformationService<
                 logger.debug("Transformation resulted in '{}'", target);
                 return target;
             }
-
-    @Override
-    protected Properties internalLoadTransform(String filename) throws TransformationException {
-        Properties result = new Properties();
-        try (FileReader reader = new FileReader(filename)) {
-            result.load(reader);
-            return result;
-        } catch (IOException e) {
-            throw new TransformationException("An error occurred while opening file.", e);
         }
+        throw new TransformationException("Could not find configuration '" + function + "' or failed to parse it.");
     }
 
     @Override
     public @Nullable Collection<ParameterOption> getParameterOptions(URI uri, String param, @Nullable String context,
             @Nullable Locale locale) {
         if (PROFILE_CONFIG_URI.equals(uri.toString())) {
-            switch (param) {
+            if (CONFIG_PARAM_FUNCTION.equals(param)) {
-                case CONFIG_PARAM_FUNCTION:
+                return transformationConfigurationRegistry.getConfigurations(SUPPORTED_CONFIGURATION_TYPES).stream()
-                    return getFilenames(FILE_NAME_EXTENSIONS).stream().map(f -> new ParameterOption(f, f))
+                        .map(c -> new ParameterOption(c.getUID(), c.getLabel())).collect(Collectors.toList());
-                            .collect(Collectors.toList());
             }
         }
         return null;
     }
+
+    @Override
+    public void added(TransformationConfiguration element) {
+        // do nothing, configurations are added to cache if needed
+    }
+
+    @Override
+    public void removed(TransformationConfiguration element) {
+        cachedTransformations.remove(element.getUID());
+    }
+
+    @Override
+    public void updated(TransformationConfiguration oldElement, TransformationConfiguration element) {
+        if (cachedTransformations.remove(oldElement.getUID()) != null) {
+            // import only if it was present before
+            importConfiguration(element);
+        }
+    }
+
+    private void importConfiguration(@Nullable TransformationConfiguration configuration) {
+        if (configuration != null) {
+            try {
+                Properties properties = new Properties();
+                properties.load(new StringReader(configuration.getContent()));
+                cachedTransformations.put(configuration.getUID(), properties);
+            } catch (IOException ignored) {
+            }
+        }
+    }
 }

bundles/org.openhab.transform.map/src/test/java/org/openhab/transform/map/internal/MapTransformationServiceTest.java

@@ -13,189 +13,146 @@
 package org.openhab.transform.map.internal;
 
 import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
 
-import java.io.File;
-import java.io.FileReader;
-import java.io.FileWriter;
 import java.io.IOException;
-import java.io.PrintStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.Paths;
+import java.util.HashMap;
-import java.util.Comparator;
+import java.util.Map;
-import java.util.Locale;
+import java.util.Objects;
-import java.util.Properties;
-import java.util.concurrent.Callable;
-import java.util.stream.Stream;
 
-import org.junit.jupiter.api.AfterEach;
+import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
+import org.mockito.Mockito;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.mockito.junit.jupiter.MockitoSettings;
 import org.mockito.quality.Strictness;
-import org.osgi.framework.BundleContext;
+import org.mockito.stubbing.Answer;
+import org.openhab.core.test.java.JavaTest;
+import org.openhab.core.transform.TransformationConfiguration;
+import org.openhab.core.transform.TransformationConfigurationRegistry;
+import org.openhab.core.transform.TransformationException;
 
 /**
  * @author Gaël L'hopital - Initial contribution
+ * @author Jan N. Klug - Refactored to use {@link TransformationConfigurationRegistry}
  */
 @ExtendWith(MockitoExtension.class)
 @MockitoSettings(strictness = Strictness.LENIENT)
-public class MapTransformationServiceTest {
+@NonNullByDefault
-
+public class MapTransformationServiceTest extends JavaTest {
     private static final String SOURCE_CLOSED = "CLOSED";
     private static final String SOURCE_UNKNOWN = "UNKNOWN";
-    private static final String EXISTING_FILENAME_DE = "map/doorstatus_de.map";
-    private static final String SHOULD_BE_LOCALIZED_FILENAME = "map/doorstatus.map";
-    private static final String DEFAULTED_FILENAME = "map/doorstatus_defaulted.map";
-    private static final String INEXISTING_FILENAME = "map/de.map";
-    private static final String BASE_FOLDER = "target";
-    private static final String SRC_FOLDER = "conf";
-    private static final String CONFIG_FOLDER = BASE_FOLDER + File.separator + SRC_FOLDER;
-    private static final String USED_FILENAME = CONFIG_FOLDER + File.separator + "transform/" + EXISTING_FILENAME_DE;
 
-    private @Mock BundleContext bundleContext;
+    private static final String NON_DEFAULTED_TRANSFORMATION_DE = "map/doorstatus_de.map";
+    private static final String NON_DEFAULTED_TRANSFORMATION_FR = "map/doorstatus_fr.map";
+    private static final String DEFAULTED_TRANSFORMATION = "map/doorstatus_defaulted.map";
+    private static final String UNKNOWN_TRANSFORMATION = "map/de.map";
 
-    private TestableMapTransformationService processor;
+    private static final String SRC_FOLDER = "conf/transform";
 
-    private class TestableMapTransformationService extends MapTransformationService {
+    @Mock
-        @Override
+    private @NonNullByDefault({}) TransformationConfigurationRegistry transformationConfigurationRegistry;
-        protected String getSourcePath() {
-            return BASE_FOLDER + File.separator + super.getSourcePath();
-        }
 
-        @Override
+    private @NonNullByDefault({}) MapTransformationService processor;
-        protected Locale getLocale() {
+    private final Map<String, TransformationConfiguration> configurationMap = new HashMap<>();
-            return Locale.US;
-        }
-
-        @Override
-        public void activate(BundleContext context) {
-            super.activate(context);
-        }
-
-        @Override
-        public void deactivate() {
-            super.deactivate();
-        }
-    };
 
     @BeforeEach
     public void setUp() throws IOException {
-        processor = new TestableMapTransformationService();
+        configurationMap.clear();
-        processor.activate(bundleContext);
+        Files.walk(Path.of(SRC_FOLDER)).filter(Files::isRegularFile).forEach(file -> {
-        copyDirectory(SRC_FOLDER, CONFIG_FOLDER);
-    }
-
-    @AfterEach
-    public void tearDown() throws IOException {
-        processor.deactivate();
-
-        try (Stream<Path> walk = Files.walk(Path.of(CONFIG_FOLDER))) {
-            walk.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
-        }
-    }
-
-    private void copyDirectory(String from, String to) throws IOException {
-        Files.walk(Paths.get(from)).forEach(fromPath -> {
-            Path toPath = Paths.get(to, fromPath.toString().substring(from.length()));
             try {
-                Files.copy(fromPath, toPath);
+                String content = new String(Files.readAllBytes(file));
-            } catch (IOException e) {
+                String uid = Path.of(SRC_FOLDER).relativize(file).toString();
+                TransformationConfiguration transformationConfiguration = new TransformationConfiguration(uid, uid,
+                        "map", null, content);
+                configurationMap.put(uid, transformationConfiguration);
+            } catch (IOException ignored) {
             }
         });
+
+        Mockito.when(transformationConfigurationRegistry.get(anyString(), eq(null)))
+                .thenAnswer((Answer<TransformationConfiguration>) invocation -> {
+                    Object[] args = invocation.getArguments();
+                    return configurationMap.get((String) args[0]);
+                });
+
+        processor = new MapTransformationService(transformationConfigurationRegistry);
     }
 
     @Test
-    public void testTransformByMap() throws Exception {
+    public void testTransformSucceeds() throws TransformationException {
-        // Test that we find a translation in an existing file
+        String transformedResponse = processor.transform(NON_DEFAULTED_TRANSFORMATION_DE, SOURCE_CLOSED);
-        String transformedResponse = processor.transform(EXISTING_FILENAME_DE, SOURCE_CLOSED);
         assertEquals("zu", transformedResponse);
-
-        Properties properties = new Properties();
-        try (FileReader reader = new FileReader(USED_FILENAME); FileWriter writer = new FileWriter(USED_FILENAME)) {
-            properties.load(reader);
-            properties.setProperty(SOURCE_CLOSED, "changevalue");
-            properties.store(writer, "");
-
-            // This tests that the requested transformation file has been removed from
-            // the cache
-            waitForAssert(new Callable<Void>() {
-                @Override
-                public Void call() throws Exception {
-                    final String transformedResponse = processor.transform(EXISTING_FILENAME_DE, SOURCE_CLOSED);
-                    assertEquals("changevalue", transformedResponse);
-                    return null;
-                }
-            }, 10000, 100);
-
-            properties.setProperty(SOURCE_CLOSED, "zu");
-            properties.store(writer, "");
-
-            waitForAssert(new Callable<Void>() {
-                @Override
-                public Void call() throws Exception {
-                    final String transformedResponse = processor.transform(EXISTING_FILENAME_DE, SOURCE_CLOSED);
-                    assertEquals("zu", transformedResponse);
-                    return null;
-                }
-            }, 10000, 100);
-        } catch (IOException e1) {
-            PrintStream err = System.err;
-            if (err != null) {
-                e1.printStackTrace(err);
-            }
-        }
-
-        // Checks that an unknown input in an existing file give the expected
-        // transformed response that shall be empty string (Issue #1107) if not found in the file
-        transformedResponse = processor.transform(EXISTING_FILENAME_DE, SOURCE_UNKNOWN);
-        assertEquals("", transformedResponse);
-
-        // Test that an inexisting file raises correct exception as expected
-        try {
-            transformedResponse = processor.transform(INEXISTING_FILENAME, SOURCE_CLOSED);
-            fail();
-        } catch (Exception e) {
-            // That's what we expect.
-        }
-
-        // Test that we find a localized version of desired file
-        transformedResponse = processor.transform(SHOULD_BE_LOCALIZED_FILENAME, SOURCE_CLOSED);
-        // as we don't know the real locale at the moment the
-        // test is run, we test that the string has actually been transformed
-        assertNotEquals(SOURCE_CLOSED, transformedResponse);
-        transformedResponse = processor.transform(SHOULD_BE_LOCALIZED_FILENAME, SOURCE_CLOSED);
-        assertNotEquals(SOURCE_CLOSED, transformedResponse);
     }
 
     @Test
-    public void testTransformByMapWithDefault() throws Exception {
+    public void testTransformFailsWithoutDefault() {
-        // Standard behaviour with no default value
+        assertThrows(TransformationException.class,
-        String transformedResponse = processor.transform(SHOULD_BE_LOCALIZED_FILENAME, "toBeDefaulted");
+                () -> processor.transform(NON_DEFAULTED_TRANSFORMATION_DE, SOURCE_UNKNOWN));
-        assertEquals("", transformedResponse);
-        // Modified behaviour with a file containing default value definition
-        transformedResponse = processor.transform(DEFAULTED_FILENAME, "toBeDefaulted");
-        assertEquals("Default Value", transformedResponse);
     }
 
-    protected void waitForAssert(Callable<Void> assertion, int timeout, int sleepTime) throws Exception {
+    @Test
-        int waitingTime = 0;
+    public void testTransformSucceedsWithDefault() throws TransformationException {
-        while (waitingTime < timeout) {
+        assertEquals("Default Value", processor.transform(DEFAULTED_TRANSFORMATION, SOURCE_UNKNOWN));
-            try {
+    }
-                assertion.call();
+
-                return;
+    @Test
-            } catch (AssertionError error) {
+    public void testTransformFailsOnUnknownTransformation() {
-                waitingTime += sleepTime;
+        assertThrows(TransformationException.class, () -> processor.transform(UNKNOWN_TRANSFORMATION, SOURCE_CLOSED));
-                try {
+    }
-                    Thread.sleep(sleepTime);
+
-                } catch (InterruptedException e) {
+    @Test
-                    throw new RuntimeException(e);
+    public void setTransformationConfigurationIsRemoved() throws TransformationException {
-                }
+        assertEquals("zu", processor.transform(NON_DEFAULTED_TRANSFORMATION_DE, SOURCE_CLOSED));
-            }
+
-        }
+        TransformationConfiguration transformationConfiguration = configurationMap
-        assertion.call();
+                .remove(NON_DEFAULTED_TRANSFORMATION_DE);
+        processor.removed(Objects.requireNonNull(transformationConfiguration));
+
+        assertThrows(TransformationException.class,
+                () -> processor.transform(NON_DEFAULTED_TRANSFORMATION_DE, SOURCE_CLOSED));
+    }
+
+    @Test
+    public void setTransformationConfigurationIsNotUpdatedIfOldElementMissing() throws TransformationException {
+        // update configuration
+        TransformationConfiguration transformationConfigurationDE = Objects
+                .requireNonNull(configurationMap.get(NON_DEFAULTED_TRANSFORMATION_DE));
+        TransformationConfiguration transformationConfigurationFR = Objects
+                .requireNonNull(configurationMap.get(NON_DEFAULTED_TRANSFORMATION_FR));
+        TransformationConfiguration transformationConfigurationModified = new TransformationConfiguration(
+                transformationConfigurationDE.getUID(), transformationConfigurationDE.getLabel(),
+                transformationConfigurationDE.getType(), transformationConfigurationDE.getLanguage(),
+                transformationConfigurationFR.getContent());
+        processor.updated(transformationConfigurationDE, transformationConfigurationModified);
+
+        // assert there is no modified cached version
+        assertEquals("zu", processor.transform(NON_DEFAULTED_TRANSFORMATION_DE, SOURCE_CLOSED));
+    }
+
+    @Test
+    public void setTransformationConfigurationIsUpdatedIfOldElementPresent() throws TransformationException {
+        // ensure old transformation is cached
+        processor.transform(NON_DEFAULTED_TRANSFORMATION_DE, SOURCE_CLOSED);
+
+        // update configuration
+        TransformationConfiguration transformationConfigurationDE = Objects
+                .requireNonNull(configurationMap.get(NON_DEFAULTED_TRANSFORMATION_DE));
+        TransformationConfiguration transformationConfigurationFR = Objects
+                .requireNonNull(configurationMap.get(NON_DEFAULTED_TRANSFORMATION_FR));
+        TransformationConfiguration transformationConfigurationModified = new TransformationConfiguration(
+                transformationConfigurationDE.getUID(), transformationConfigurationDE.getLabel(),
+                transformationConfigurationDE.getType(), transformationConfigurationDE.getLanguage(),
+                transformationConfigurationFR.getContent());
+        processor.updated(transformationConfigurationDE, transformationConfigurationModified);
+
+        // ensure modified configuration is applied
+        assertEquals("fermé", processor.transform(NON_DEFAULTED_TRANSFORMATION_DE, SOURCE_CLOSED));
     }
 }