[automation] Ensure unique module IDs for overloaded @RuleAction methods (#4441)

* [automation] Handle overloaded `@RuleAction`s - Append signature hash to avoid duplicate module ids.
* [automation] ThingActionsResource: Provide action visibility

Signed-off-by: Florian Hotze <dev@florianhotze.com>

bundles/org.openhab.core.automation.rest/src/main/java/org/openhab/core/automation/rest/internal/ThingActionsResource.java

@@ -37,9 +37,11 @@ import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.core.auth.Role;
 import org.openhab.core.automation.Action;
+import org.openhab.core.automation.Visibility;
 import org.openhab.core.automation.annotation.RuleAction;
 import org.openhab.core.automation.handler.ActionHandler;
 import org.openhab.core.automation.handler.ModuleHandlerFactory;
+import org.openhab.core.automation.module.provider.AnnotationActionModuleTypeHelper;
 import org.openhab.core.automation.type.ActionType;
 import org.openhab.core.automation.type.Input;
 import org.openhab.core.automation.type.ModuleTypeRegistry;
@@ -97,16 +99,19 @@ public class ThingActionsResource implements RESTResource {
     private final LocaleService localeService;
     private final ModuleTypeRegistry moduleTypeRegistry;
     private final ActionInputsHelper actionInputsHelper;
+    private final AnnotationActionModuleTypeHelper annotationActionModuleTypeHelper;
 
-    Map<ThingUID, Map<String, ThingActions>> thingActionsMap = new ConcurrentHashMap<>();
+    Map<ThingUID, Map<String, List<String>>> thingActionsMap = new ConcurrentHashMap<>();
     private List<ModuleHandlerFactory> moduleHandlerFactories = new ArrayList<>();
 
     @Activate
     public ThingActionsResource(@Reference LocaleService localeService,
-            @Reference ModuleTypeRegistry moduleTypeRegistry, @Reference ActionInputsHelper actionInputsHelper) {
+            @Reference ModuleTypeRegistry moduleTypeRegistry, @Reference ActionInputsHelper actionInputsHelper,
+            @Reference AnnotationActionModuleTypeHelper annotationActionModuleTypeHelper) {
         this.localeService = localeService;
         this.moduleTypeRegistry = moduleTypeRegistry;
         this.actionInputsHelper = actionInputsHelper;
+        this.annotationActionModuleTypeHelper = annotationActionModuleTypeHelper;
     }
 
     @Reference(policy = ReferencePolicy.DYNAMIC, cardinality = ReferenceCardinality.MULTIPLE)
@@ -115,7 +120,18 @@ public class ThingActionsResource implements RESTResource {
         String scope = getScope(thingActions);
         if (handler != null && scope != null) {
             ThingUID thingUID = handler.getThing().getUID();
-            thingActionsMap.computeIfAbsent(thingUID, thingUid -> new ConcurrentHashMap<>()).put(scope, thingActions);
+            Method[] methods = thingActions.getClass().getDeclaredMethods();
+            List<String> actionUIDs = new ArrayList<>();
+            for (Method method : methods) {
+                if (!method.isAnnotationPresent(RuleAction.class)) {
+                    continue;
+                }
+                actionUIDs.add(annotationActionModuleTypeHelper.getModuleIdFromMethod(scope, method));
+            }
+            if (actionUIDs.isEmpty()) {
+                return;
+            }
+            thingActionsMap.computeIfAbsent(thingUID, thingUid -> new ConcurrentHashMap<>()).put(scope, actionUIDs);
         }
     }
 
@@ -124,7 +140,7 @@ public class ThingActionsResource implements RESTResource {
         String scope = getScope(thingActions);
         if (handler != null && scope != null) {
             ThingUID thingUID = handler.getThing().getUID();
-            Map<String, ThingActions> actionMap = thingActionsMap.get(thingUID);
+            Map<String, List<String>> actionMap = thingActionsMap.get(thingUID);
             if (actionMap != null) {
                 actionMap.remove(scope);
                 if (actionMap.isEmpty()) {
@@ -156,24 +172,15 @@ public class ThingActionsResource implements RESTResource {
         ThingUID aThingUID = new ThingUID(thingUID);
 
         List<ThingActionDTO> actions = new ArrayList<>();
-        Map<String, ThingActions> thingActionsMap = this.thingActionsMap.get(aThingUID);
+        Map<String, List<String>> thingActionsMap = this.thingActionsMap.get(aThingUID);
         if (thingActionsMap == null) {
             return Response.status(Response.Status.NOT_FOUND).build();
         }
 
         // inspect ThingActions
-        for (Map.Entry<String, ThingActions> thingActionsEntry : thingActionsMap.entrySet()) {
+        for (Map.Entry<String, List<String>> thingActionsEntry : thingActionsMap.entrySet()) {
-            ThingActions thingActions = thingActionsEntry.getValue();
+            for (String actionUID : thingActionsEntry.getValue()) {
-            Method[] methods = thingActions.getClass().getDeclaredMethods();
+                ActionType actionType = (ActionType) moduleTypeRegistry.get(actionUID, locale);
-            for (Method method : methods) {
-                RuleAction ruleAction = method.getAnnotation(RuleAction.class);
-
-                if (ruleAction == null) {
-                    continue;
-                }
-
-                String actionUid = thingActionsEntry.getKey() + "." + method.getName();
-                ActionType actionType = (ActionType) moduleTypeRegistry.get(actionUid, locale);
                 if (actionType == null) {
                     continue;
                 }
@@ -200,6 +207,7 @@ public class ThingActionsResource implements RESTResource {
                 actionDTO.inputConfigDescriptions = inputParameters == null ? null
                         : ConfigDescriptionDTOMapper.mapParameters(inputParameters);
                 actionDTO.outputs = actionType.getOutputs();
+                actionDTO.visibility = actionType.getVisibility();
                 actions.add(actionDTO);
             }
         }
@@ -268,6 +276,7 @@ public class ThingActionsResource implements RESTResource {
 
         public @Nullable String label;
         public @Nullable String description;
+        public @Nullable Visibility visibility;
 
         public List<Input> inputs = new ArrayList<>();
 

bundles/org.openhab.core.automation/src/main/java/org/openhab/core/automation/module/provider/AnnotationActionModuleTypeHelper.java

@@ -17,6 +17,9 @@ import static org.openhab.core.automation.internal.module.handler.AnnotationActi
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
 import java.lang.reflect.Parameter;
+import java.math.BigInteger;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -57,7 +60,8 @@ import org.slf4j.LoggerFactory;
  * Helper methods for {@link AnnotatedActions} {@link ModuleTypeProvider}
  *
  * @author Stefan Triller - Initial contribution
- * @author Florian Hotze - Added configuration description parameters for thing modules
+ * @author Florian Hotze - Added configuration description parameters for thing modules, Added method signature hash to
+ *         module ID in case of method overloads
  * @author Laurent Garnier - Converted into a an OSGi component
  */
 @NonNullByDefault
@@ -96,7 +100,7 @@ public class AnnotationActionModuleTypeHelper {
                 List<Output> outputs = getOutputsFromAction(method);
 
                 RuleAction ruleAction = method.getAnnotation(RuleAction.class);
-                String uid = name + "." + method.getName();
+                String uid = getModuleIdFromMethod(name, method);
                 Set<String> tags = new HashSet<>(Arrays.asList(ruleAction.tags()));
 
                 ModuleInformation mi = new ModuleInformation(uid, actionProvider, method);
@@ -113,6 +117,20 @@ public class AnnotationActionModuleTypeHelper {
         return moduleInformation;
     }
 
+    public String getModuleIdFromMethod(String actionScope, Method method) {
+        String uid = actionScope + "." + method.getName() + "#";
+        MessageDigest md5 = null;
+        try {
+            md5 = MessageDigest.getInstance("MD5");
+        } catch (NoSuchAlgorithmException e) {
+            throw new RuntimeException(e);
+        }
+        for (Class<?> parameter : method.getParameterTypes()) {
+            md5.update(parameter.getName().getBytes());
+        }
+        return uid + String.format("%032x", new BigInteger(1, md5.digest()));
+    }
+
     private List<Input> getInputsFromAction(Method method) {
         List<Input> inputs = new ArrayList<>();
 

bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/internal/module/provider/AnnotationActionModuleTypeProviderTest.java

@@ -16,6 +16,11 @@ import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.when;
 
+import java.lang.reflect.Method;
+import java.math.BigInteger;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
@@ -33,6 +38,7 @@ import org.openhab.core.automation.AnnotatedActions;
 import org.openhab.core.automation.Visibility;
 import org.openhab.core.automation.annotation.ActionInput;
 import org.openhab.core.automation.annotation.ActionOutput;
+import org.openhab.core.automation.annotation.ActionOutputs;
 import org.openhab.core.automation.annotation.ActionScope;
 import org.openhab.core.automation.annotation.RuleAction;
 import org.openhab.core.automation.module.provider.AnnotationActionModuleTypeHelper;
@@ -55,7 +61,22 @@ import org.openhab.core.test.java.JavaTest;
 @NonNullByDefault
 public class AnnotationActionModuleTypeProviderTest extends JavaTest {
 
-    private static final String TEST_ACTION_TYPE_ID = "binding.test.testMethod";
+    private static final String TEST_ACTION_SIGNATURE_HASH;
+    static {
+        MessageDigest md5 = null;
+        try {
+            md5 = MessageDigest.getInstance("MD5");
+        } catch (NoSuchAlgorithmException e) {
+            throw new RuntimeException(e);
+        }
+        Method method = Arrays.stream(TestActionProvider.class.getDeclaredMethods())
+                .filter(m -> m.getName().equals("testMethod")).findFirst().orElseThrow();
+        for (Class<?> parameter : method.getParameterTypes()) {
+            md5.update(parameter.getName().getBytes());
+        }
+        TEST_ACTION_SIGNATURE_HASH = String.format("%032x", new BigInteger(1, md5.digest()));
+    }
+    private static final String TEST_ACTION_TYPE_ID = "binding.test.testMethod#" + TEST_ACTION_SIGNATURE_HASH;
     private static final String ACTION_LABEL = "Test Label";
     private static final String ACTION_DESCRIPTION = "My Description";
 
@@ -195,9 +216,10 @@ public class AnnotationActionModuleTypeProviderTest extends JavaTest {
 
         @RuleAction(label = ACTION_LABEL, description = ACTION_DESCRIPTION, visibility = Visibility.HIDDEN, tags = {
                 "tag1", "tag2" })
-        public @ActionOutput(name = ACTION_OUTPUT1, type = ACTION_OUTPUT1_TYPE, description = ACTION_OUTPUT1_DESCRIPTION, label = ACTION_OUTPUT1_LABEL, defaultValue = ACTION_OUTPUT1_DEFAULT_VALUE, reference = ACTION_OUTPUT1_REFERENCE, tags = {
+        public @ActionOutputs({
-                "tagOut11",
+                @ActionOutput(name = ACTION_OUTPUT1, type = ACTION_OUTPUT1_TYPE, description = ACTION_OUTPUT1_DESCRIPTION, label = ACTION_OUTPUT1_LABEL, defaultValue = ACTION_OUTPUT1_DEFAULT_VALUE, reference = ACTION_OUTPUT1_REFERENCE, tags = {
-                "tagOut12" }) @ActionOutput(name = ACTION_OUTPUT2, type = ACTION_OUTPUT2_TYPE) Map<String, Object> testMethod(
+                        "tagOut11", "tagOut12" }),
+                @ActionOutput(name = ACTION_OUTPUT2, type = ACTION_OUTPUT2_TYPE) }) Map<String, Object> testMethod(
                         @ActionInput(name = ACTION_INPUT1, label = ACTION_INPUT1_LABEL, defaultValue = ACTION_INPUT1_DEFAULT_VALUE, description = ACTION_INPUT1_DESCRIPTION, reference = ACTION_INPUT1_REFERENCE, required = true, type = "Item", tags = {
                                 "tagIn11", "tagIn12" }) String input1,
                         @ActionInput(name = ACTION_INPUT2) String input2) {

bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/thingsupport/AnnotatedThingActionModuleTypeProviderTest.java

@@ -16,6 +16,11 @@ import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.when;
 
+import java.lang.reflect.Method;
+import java.math.BigInteger;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
@@ -60,7 +65,22 @@ public class AnnotatedThingActionModuleTypeProviderTest extends JavaTest {
 
     private static final ThingTypeUID TEST_THING_TYPE_UID = new ThingTypeUID("binding", "thing-type");
 
-    private static final String TEST_ACTION_TYPE_ID = "test.testMethod";
+    private static final String TEST_ACTION_SIGNATURE_HASH;
+    static {
+        MessageDigest md5 = null;
+        try {
+            md5 = MessageDigest.getInstance("MD5");
+        } catch (NoSuchAlgorithmException e) {
+            throw new RuntimeException(e);
+        }
+        Method method = Arrays.stream(TestThingActionProvider.class.getDeclaredMethods())
+                .filter(m -> m.getName().equals("testMethod")).findFirst().orElseThrow();
+        for (Class<?> parameter : method.getParameterTypes()) {
+            md5.update(parameter.getName().getBytes());
+        }
+        TEST_ACTION_SIGNATURE_HASH = String.format("%032x", new BigInteger(1, md5.digest()));
+    }
+    private static final String TEST_ACTION_TYPE_ID = "test.testMethod#" + TEST_ACTION_SIGNATURE_HASH;
     private static final String ACTION_LABEL = "Test Label";
     private static final String ACTION_DESCRIPTION = "My Description";