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

[florian-h05]

Supersedes #4438.

This appends a method signature hash to @RuleAction methods which have overloades to ensure module IDs are unique.
It also returns the visibility for Thing actions from the REST API.

[florian-h05]

@J-N-K This implements an alternative solution that ensures unique, predictable module IDs for Thing actions by hashing the method signature. Can you please have a look so we can get this into M3 and test it before M3?

[lsiepel]

This was wat I meant with #4438 (comment)
👍

[J-N-K]

.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.

[florian-h05]

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.

[J-N-K]

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.

[florian-h05]

@J-N-K I implemented hashing for all methods, now the tests are failing and I can't find the reason.

[J-N-K]

I guess the test fails because the action UID now contains the hash and the test is against private static final String TEST_ACTION_TYPE_ID = "binding.test.testMethod"

[florian-h05]

I modified the test to contain the hash in the TEST_ACTION_TYPE_ID.

[J-N-K]

I checked out your branch and modified the test:

assertEquals(TEST_ACTION_TYPE_ID, types.toArray()[0]);

[ERROR] Failures: 
[ERROR]   AnnotatedThingActionModuleTypeProviderTest.testMultiServiceAnnotationActions:116 expected: <test.testMethod> but was: <test.testMethod#26aa5ea09027ad2b11b752652e808c6a>

so the hash is missing in the expected value.

[florian-h05]

@J-N-K Fixed this test - did not notice at first that there are two different tests.

[lolodomo]

Is the class considered in the hash ?
Because it would be possible to have actions coming from different classes with same method signature. Even if I have not seen this case in bindings yet.

[florian-h05]

Is the class considered in the hash ?

No, but we have the method name and the action scope inside the name, so I hope that’s enough.

[J-N-K]

Is the class considered in the hash ? Because it would be possible to have actions coming from different classes with same method signature. Even if I have not seen this case in bindings yet.

Within the same scope there should never be methods with the same signature. Is there any binding where this is the case?

[lsiepel]

e methods wi

For the compiler the scope would be class level, for this annotation/PR it would be binding level? So i guess it is a matter of time before a collision occurs, better safe now to add the class name to the hashing.

[florian-h05]

I added the classname to the hash. I am unavailable this evening, so feel free to revert it if you want to merge tonight.

[J-N-K]

For openHAB the scope is not the class but the defined scope (@ActionScope). Having more than one class per scope will fail in all scripting add-ons because the ThingActionService only allows one ThingActions class per scope. So collisions between methods of different classes cannot happen.

Adding it doesn't hurt though. I'm finde with that.

What has to be investigated though is why the tests seem to be unstable, this time Java profile-j21 failed, before the Java 21 build failed.

[lolodomo]

Having more than one class per scope will fail in all scripting add-ons

Several bindings are doing that.
But probably with different method signatures.

[J-N-K]

Then these bindings should be fixed.

[florian-h05]

Adding it doesn't hurt though. I'm finde with that.

As more than one class per scope will fail in all scripting add-ons and should not be the case, I will revert because not having the class name keeps the code a bit simpler.

[florian-h05]

@J-N-K I have been able to stabilise the tests, can you please have a look at this PR so we can get it into M3?
Not having it in M3 would break all UI-based rule usage of Thing actions with the next milestone.

[holgerfriedrich]

@florian-h05 Sorry, just technical questions:

• Should we still use MD5? Today I would assume SHA-256 is standard.... (I know, the data to be protected is nothing critical, but I would assume that performance is not too different on such short strings)
• Does it make sense to cache the instance you get from the MD5.getInstance call? I have no experience here, but it might go deep into the crypto providers to get the instance.

Sorry adding noise.

[J-N-K]

SHA1/MD5: probably doesn't matter much, but according according to various sources SHA-256 has about 40% of the hash rate compared to MD5. SHA-256 also has a longer hash, so added overhead during REST calls (probably doesn't matter as well). Regarding hash quality and collision rate MD5 is probably already over-kill, but it's a algorithm that is easily available in Java.

The instance should not be cached, it is not thread-safe and different ThingActions can be added in parallel.

[florian-h05]

Should we still use MD5? Today I would assume SHA-256 is standard....

SHA256 hash is twice longer (256 bit vs 128 bit) and a bit slower.
While SHA256 is more secure, MD5 is still in use for environments where no high level of security is needed, e.g. checksum for file integrity.

Security doesn't matter in that case, that bit of speed difference is nice though.

Does it make sense to cache the instance you get from the MD5.getInstance call?

Given the small amount of data to be hashed I don't think we need that, and I don't know how that should be done.

[J-N-K]

LGTM, just re-running the CI builds to be sure.