From 0d031a3b78b0ecd4718146db2a7ea8bb0f030c42 Mon Sep 17 00:00:00 2001 From: Florian Hotze Date: Sun, 10 Nov 2024 14:11:24 +0100 Subject: [PATCH] [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 --- .../rest/internal/ThingActionsResource.java | 43 +++++++++++-------- .../AnnotationActionModuleTypeHelper.java | 22 +++++++++- ...nnotationActionModuleTypeProviderTest.java | 30 +++++++++++-- ...atedThingActionModuleTypeProviderTest.java | 22 +++++++++- 4 files changed, 93 insertions(+), 24 deletions(-) diff --git a/bundles/org.openhab.core.automation.rest/src/main/java/org/openhab/core/automation/rest/internal/ThingActionsResource.java b/bundles/org.openhab.core.automation.rest/src/main/java/org/openhab/core/automation/rest/internal/ThingActionsResource.java index 4dba80b5b46..40c25278bc3 100644 --- a/bundles/org.openhab.core.automation.rest/src/main/java/org/openhab/core/automation/rest/internal/ThingActionsResource.java +++ b/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.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> thingActionsMap = new ConcurrentHashMap<>(); + Map>> thingActionsMap = new ConcurrentHashMap<>(); private List 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 void addThingActions(ThingActions thingActions) { 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 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 void removeThingActions(ThingActions thingActions) { String scope = getScope(thingActions); if (handler != null && scope != null) { ThingUID thingUID = handler.getThing().getUID(); - Map actionMap = thingActionsMap.get(thingUID); + Map> actionMap = thingActionsMap.get(thingUID); if (actionMap != null) { actionMap.remove(scope); if (actionMap.isEmpty()) { @@ -156,24 +172,15 @@ public Response getActions(@PathParam("thingUID") @Parameter(description = "thin ThingUID aThingUID = new ThingUID(thingUID); List actions = new ArrayList<>(); - Map thingActionsMap = this.thingActionsMap.get(aThingUID); + Map> thingActionsMap = this.thingActionsMap.get(aThingUID); if (thingActionsMap == null) { return Response.status(Response.Status.NOT_FOUND).build(); } // inspect ThingActions - for (Map.Entry thingActionsEntry : thingActionsMap.entrySet()) { - ThingActions thingActions = thingActionsEntry.getValue(); - Method[] methods = thingActions.getClass().getDeclaredMethods(); - 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); + for (Map.Entry> thingActionsEntry : thingActionsMap.entrySet()) { + for (String actionUID : thingActionsEntry.getValue()) { + ActionType actionType = (ActionType) moduleTypeRegistry.get(actionUID, locale); if (actionType == null) { continue; } @@ -200,6 +207,7 @@ public Response getActions(@PathParam("thingUID") @Parameter(description = "thin actionDTO.inputConfigDescriptions = inputParameters == null ? null : ConfigDescriptionDTOMapper.mapParameters(inputParameters); actionDTO.outputs = actionType.getOutputs(); + actionDTO.visibility = actionType.getVisibility(); actions.add(actionDTO); } } @@ -268,6 +276,7 @@ private static class ThingActionDTO { public @Nullable String label; public @Nullable String description; + public @Nullable Visibility visibility; public List inputs = new ArrayList<>(); diff --git a/bundles/org.openhab.core.automation/src/main/java/org/openhab/core/automation/module/provider/AnnotationActionModuleTypeHelper.java b/bundles/org.openhab.core.automation/src/main/java/org/openhab/core/automation/module/provider/AnnotationActionModuleTypeHelper.java index 5e5c2799db7..bc9919176a3 100644 --- a/bundles/org.openhab.core.automation/src/main/java/org/openhab/core/automation/module/provider/AnnotationActionModuleTypeHelper.java +++ b/bundles/org.openhab.core.automation/src/main/java/org/openhab/core/automation/module/provider/AnnotationActionModuleTypeHelper.java @@ -17,6 +17,9 @@ 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 @@ * 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 Collection parseAnnotations(String name, Object action List outputs = getOutputsFromAction(method); RuleAction ruleAction = method.getAnnotation(RuleAction.class); - String uid = name + "." + method.getName(); + String uid = getModuleIdFromMethod(name, method); Set tags = new HashSet<>(Arrays.asList(ruleAction.tags())); ModuleInformation mi = new ModuleInformation(uid, actionProvider, method); @@ -113,6 +117,20 @@ public Collection parseAnnotations(String name, Object action 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 getInputsFromAction(Method method) { List inputs = new ArrayList<>(); diff --git a/bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/internal/module/provider/AnnotationActionModuleTypeProviderTest.java b/bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/internal/module/provider/AnnotationActionModuleTypeProviderTest.java index e56869f13b0..96f9b8e7b7c 100644 --- a/bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/internal/module/provider/AnnotationActionModuleTypeProviderTest.java +++ b/bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/internal/module/provider/AnnotationActionModuleTypeProviderTest.java @@ -16,6 +16,11 @@ 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.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 @@ @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 @@ private static class TestActionProvider implements AnnotatedActions { @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 = { - "tagOut11", - "tagOut12" }) @ActionOutput(name = ACTION_OUTPUT2, type = ACTION_OUTPUT2_TYPE) Map testMethod( + public @ActionOutputs({ + @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 = { + "tagOut11", "tagOut12" }), + @ActionOutput(name = ACTION_OUTPUT2, type = ACTION_OUTPUT2_TYPE) }) Map 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) { diff --git a/bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/thingsupport/AnnotatedThingActionModuleTypeProviderTest.java b/bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/thingsupport/AnnotatedThingActionModuleTypeProviderTest.java index bed80a27ae8..bd6d3a13b39 100644 --- a/bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/thingsupport/AnnotatedThingActionModuleTypeProviderTest.java +++ b/bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/thingsupport/AnnotatedThingActionModuleTypeProviderTest.java @@ -16,6 +16,11 @@ 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";