Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 7 commits into from
Nov 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f0f54f
[automation] Handle overloaded `@RuleAction`s
florian-h05 Nov 7, 2024
33c9a96
[automation] ThingActionsResource: Provide action visibility
florian-h05 Nov 7, 2024
cb77453
[automation] Always append signature hash & Use MD5 for hashing
florian-h05 Nov 7, 2024
ca6bdfb
[automation] Improve ThingActionsResource code for better performance
florian-h05 Nov 7, 2024
33ced98
Add classname to signature hash
florian-h05 Nov 9, 2024
41e60af
Revert "Add classname to signature hash"
florian-h05 Nov 10, 2024
86df5e0
Attempt to stabilise tests
florian-h05 Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 26 additions & 17 deletions ...on.rest/src/main/java/org/openhab/core/automation/rest/internal/ThingActionsResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand All @@ -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<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);
}
}

Expand All @@ -124,7 +140,7 @@ public void removeThingActions(ThingActions thingActions) {
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()) {
Expand Down Expand Up @@ -156,24 +172,15 @@ public Response getActions(@PathParam("thingUID") @Parameter(description = "thin
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()) {
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<String, List<String>> thingActionsEntry : thingActionsMap.entrySet()) {
for (String actionUID : thingActionsEntry.getValue()) {
ActionType actionType = (ActionType) moduleTypeRegistry.get(actionUID, locale);
if (actionType == null) {
continue;
}
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -268,6 +276,7 @@ private static class ThingActionDTO {

public @Nullable String label;
public @Nullable String description;
public @Nullable Visibility visibility;

public List<Input> inputs = new ArrayList<>();

Expand Down
22 changes: 20 additions & 2 deletions ...in/java/org/openhab/core/automation/module/provider/AnnotationActionModuleTypeHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -96,7 +100,7 @@ public Collection<ModuleInformation> parseAnnotations(String name, Object action
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);
Expand All @@ -113,6 +117,20 @@ public Collection<ModuleInformation> 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<Input> getInputsFromAction(Method method) {
List<Input> inputs = new ArrayList<>();

Expand Down
30 changes: 26 additions & 4 deletions ...nhab/core/automation/internal/module/provider/AnnotationActionModuleTypeProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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";

Expand Down Expand Up @@ -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<String, Object> 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<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) {
Expand Down
22 changes: 21 additions & 1 deletion .../org/openhab/core/automation/thingsupport/AnnotatedThingActionModuleTypeProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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";

Expand Down