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 2 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
16 changes: 11 additions & 5 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<>();
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 Down Expand Up @@ -166,13 +171,12 @@ public Response getActions(@PathParam("thingUID") @Parameter(description = "thin
ThingActions thingActions = thingActionsEntry.getValue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For performance reasons I would suggest to split this:

  • the first part of the loop (l. 174 to 159) which is time-consuming and static (after the methods of a given ThingActions will never change) should be done when the ThingActions are added (and stored in something that allows to remove them when the ThingActions is removed)
  • the second part (checking of action type and condig description) should take place here as it depends on the availability of these in the registries (which is dynamic)

This can be refactored later if you find it too complicated now.

Method[] methods = thingActions.getClass().getDeclaredMethods();
for (Method method : methods) {
RuleAction ruleAction = method.getAnnotation(RuleAction.class);

if (ruleAction == null) {
if (!method.isAnnotationPresent(RuleAction.class)) {
continue;
}

String actionUid = thingActionsEntry.getKey() + "." + method.getName();
String actionUid = annotationActionModuleTypeHelper.getModuleIdFromMethod(thingActionsEntry.getKey(),
thingActions.getClass(), method);
ActionType actionType = (ActionType) moduleTypeRegistry.get(actionUid, locale);
if (actionType == null) {
continue;
Expand Down Expand Up @@ -200,6 +204,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 +273,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
20 changes: 18 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 @@ -57,7 +57,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 +97,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, clazz, method);
Set<String> tags = new HashSet<>(Arrays.asList(ruleAction.tags()));

ModuleInformation mi = new ModuleInformation(uid, actionProvider, method);
Expand All @@ -113,6 +114,21 @@ public Collection<ModuleInformation> parseAnnotations(String name, Object action
return moduleInformation;
}

public String getModuleIdFromMethod(String actionScope, Class<?> clazz, Method method) {
boolean hasOverloads = Arrays.stream(clazz.getDeclaredMethods())
.filter(m -> m.isAnnotationPresent(RuleAction.class)).map(Method::getName)
.filter(name -> name.equals(method.getName())).count() > 1;
String uid = actionScope + "." + method.getName();
if (hasOverloads) {
logger.debug("@RuleAction method {}::{} has overloads. Appending signature hash to UID.",
clazz.getSimpleName(), method.getName());
String signature = Arrays.stream(method.getParameterTypes()).map(Class::getName)
.collect(Collectors.joining(","));
uid += "#" + signature.hashCode();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.hashCode() could create collisions, maybe something like

MessageDigest md5 = MessageDigest.getInstance("MD5");
Arrays.stream(methos,getParameterTypes()).forEach(t -> md5.update(t.getName().getBytes()));
uid += String.format("#%032x", new BigInteger(1, md5.digest()));

BTW: Why don't we do that for each method? That would make the need to check if there are overloads unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. It is not really required, but thinking about it, it will be beneficial if one adds an overload for a method in the future as the id of the existing method won't change.
I will look into the "real" hashing.

}
return uid;
}

private List<Input> getInputsFromAction(Method method) {
List<Input> inputs = new ArrayList<>();

Expand Down