JSR223 Scripting: SimpleRule does not set rule UID

[steve-bate]

@smerschjohann The SimpleRule class does not set the rule UID and this causes issues when the rule event publishers try to use the UID to generate event messages. In my local environment, I solved the problem by adding a SimpleRule constructor that initialized the UID. I also added a constructor to allow the user to specify a prefix for the UID (to make it more user-friendly when logging).

[kaikreuzer]

@steve-bate Can you elaborate why not setting a rule UID causes issues?

Note that this is the normal case: A newly added rule should not have a UID set, but it is the rule engine, which assigns a unique id to the rule.

[steve-bate]

@kai Given the Jython rule below...

scriptExtension.importPreset("RuleSimple")
scriptExtension.importPreset("RuleSupport")

class MyRule(SimpleRule):
    def __init__(self):
        self.triggers = [
             Trigger("MyTrigger", "core.ItemStateUpdateTrigger", 
                    Configuration({ "itemName": "TestString1"}))
        ]
        
    def execute(self, module, input):
        events.postUpdate("TestString2", "some data")

automationManager.addRule(MyRule())

I see the following exception information in the log when the rule is reloaded (after an edit)...

2017-06-05 05:56:44.814 [WARN ] [.c.c.registry.AbstractRegistry:159  ] - Could not remove element: nulljava.lang.NullPointerException: null
	at java.lang.String.replace(String.java:2240)
	at org.eclipse.smarthome.automation.events.RuleEventFactory.buildTopic(RuleEventFactory.java:167)
	at org.eclipse.smarthome.automation.events.RuleEventFactory.buildTopic(RuleEventFactory.java:171)
	at org.eclipse.smarthome.automation.events.RuleEventFactory.createRuleRemovedEvent(RuleEventFactory.java:146)
	at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.postRuleRemovedEvent(RuleRegistryImpl.java:303)
	at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.notifyListenersAboutRemovedElement(RuleRegistryImpl.java:365)
	at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.notifyListenersAboutRemovedElement(RuleRegistryImpl.java:1)
	at org.eclipse.smarthome.core.common.registry.AbstractRegistry.removed(AbstractRegistry.java:157)
	at org.eclipse.smarthome.automation.module.script.rulesupport.shared.ScriptedRuleProvider.removeRule(ScriptedRuleProvider.java:59)
	at org.eclipse.smarthome.automation.module.script.rulesupport.shared.ScriptedRuleProvider.removeRule(ScriptedRuleProvider.java:54)
	at org.eclipse.smarthome.automation.module.script.rulesupport.shared.RuleSupportRuleRegistryDelegate.removeAllAddedByScript(RuleSupportRuleRegistryDelegate.java:118)

The uid member has protected scope in the org.eclipse.smarthome.automation.Rule class (SimpleRule is a subclass) and there no public accessor. It seems like the only way that uid can be set is by a subclass or provided in the Rule constructor.

I see the ScriptedAutomationManager copies added rules in the addRule method. It doesn't appear to assign a UID if one is not present in the copied rule.

[smerschjohann]

I keep track of the added rules using their UID. If the scriptfile is modified or deleted later on, those rules are removed from the rule engine. If looking at the source of the RuleEngine, I don't see any way to figure the ruleID out, if it is not assigned by my code beforehand.

I don't even see where the RuleEngine itself assigns a ruleID. If looking at addRule of RuleEngine, the Rule-object must already have an id assigned. @kaikreuzer I think you were mislead in this case? I think we have to generate a unique ID and change that line in the ScriptedAutomationManager again.

[danchom]

If the scriptfile is modified or deleted later on, those rules are removed from the rule engine.

I'm not sure if this is right behaviour here. The rules are imported instead of provided, this means when the rules are added into RuleEngine they stay there, until the rules are explicitly removed by the user (through the RuleRegistry interface). This is the behaiviour of RuleResourceBundleImporter. This is done because the rules are acctually stored into managed rule provider which permit to save changes on a rule done by the user.

I don't even see where the RuleEngine itself assigns a ruleID. If looking at addRule of RuleEngine, the Rule-object must already have an id assigned.

The Rule registry assigns a rule uid when the rule is missing. This is done in the RuleRegistry.add(rule) method.

[kaikreuzer]

The rules are imported instead of provided

I think we are all confused by the misleading class names (and the fact that there isn't a clear separation of concerns) - a class diagram as a dev documentation would definitely help here...

The ScriptedAutomationManager actually does NOT import rules, but it passes them to RuleSupportRuleRegistryDelegate, which internally holds a ScriptedRuleProvider which registers as a normal provider in the rule registry - so it is ok that this provider also removes rules again at any time.
For a rule provider, it indeed does not make sense to have uids being assigned by the rule engine (I wonder why this is currently possible at all...). So yes, the provider should come up with uids for its rules.
@smerschjohann What is the reason for having the RuleSupportRuleRegistryDelegate? Why can't the rules be added to the ScriptedRuleProvider directly? (Especially, since the method RuleSupportRuleRegistryDelegate.addPermanent() does not seem to be used anywhere.) It would make the architecture much cleaner and more straight-forward.

[danchom]

so it is ok that this provider also removes rules again at any time

Ok I agree, I didn't know that this is a separate rule provider.

For a rule provider, it indeed does not make sense to have uids being assigned by the rule engine (I wonder why this is currently possible at all...).

I agree that uid is better to be set by the provider, but in case of mistake or wrong rule provider the RuleEngine must assign them, otherwise these rules can't be added.

[kaikreuzer]

otherwise these rules can't be added.

It can be discussed whether we want to consider them as valid rules at all, because otherwise they should not be added (or better say "provided") anyhow.

[smerschjohann]

@kaikreuzer The RuleSupportRuleRegistryDelegate can also be used directly by the scripts but still allow the removal of all rules which where added using that script if it is unloaded see removeAllAddedByScript.

So the resolution as I understood is that the ScriptedRuleProvider requires all rules that are about to be added to have a UID, am I right?

[kaikreuzer]

The RuleSupportRuleRegistryDelegate can also be used directly by the scripts

"providing a rule" and "adding a rule to the registry" are two very different concepts and I think that should be treated that way in scripts as well. So how about being able to add (or maybe better call it "register"?) rules to the ScriptedRuleProvider within scripts directly?

So the resolution as I understood is that the ScriptedRuleProvider requires all rules that are about to be added to have a UID, am I right?

Let me reformulate it: The ScriptedRuleProvider should provide a set of rules, which all have UIDs, yes.

[steve-bate]

Any update on this issue?

[smerschjohann]

I'm sorry, I am a bit busy these days.

"providing a rule" and "adding a rule to the registry" are two very different concepts and I think that should be treated that way in scripts as well. So how about being able to add (or maybe better call it "register"?) rules to the ScriptedRuleProvider within scripts directly?

If you want to add a rule permanantly to the RuleRegistry you could already do that by using: ruleRegistry.addPermanent(). We could discuss about the wording here.

Let me reformulate it: The ScriptedRuleProvider should provide a set of rules, which all have UIDs, yes.

I open a PR for that.

[kaikreuzer]

If you want to add a rule permanantly to the RuleRegistry you could already do that

Right, but I was talking about "being able to add (or maybe better call it "register"?) rules to the ScriptedRuleProvider" - which is what we are currently doing when defining rules in scripts. I therefore think that this code should be refactored to have a clean architecture.