AbstractScriptModuleHandler: Remove prefixes from ctx keys when setting execution context

[florian-h05]

This aligns the keys of the injected ctx HashMap with the keys used when injecting the single entries of that HashMap.

It will allow JS Scripting to provide event information in a native JS object in UI-based scripts similarly to how it's done for file-based scripts.

See openhab/openhab-js#443.

[florian-h05]

@openhab/jruby-maintainers @openhab/python-maintainers FYI, if this gets merged you might need to adjust your library code.

[HolgerHees]

@openhab/jruby-maintainers @openhab/python-maintainers FYI, if this gets merged you might need to adjust your library code.

It is changed in the python helper libs

The needed changes in my binding will follow as part of my upcoming pull request for the python next version.

[jimtng]

As I understand it, this PR:

• Retains the "ctx" context key
• Strips off the key's prefix up to and including the dot, e.g. "1234.abc" -> "abc"
• Also populate the top level context with the contents of ctx.

So given "context" of { "123.abc" -> "value1" },

before:

{
   "ruleUID" => ruleuid,
   "ctx" => { "123.abc" => "value1", "ruleUID" => ruleuid },
}

after:

{
   "ctx" => { "abc" => "value1", "ruleUID" => ruleuid },
   "ruleUID" => same as before,
   "abc" => "value1"
}

If I have understood this correctly, I believe no changes are required for JRuby, because:

• It relies on "ctx" which remains available
• It extracts the last part of the key name after .. When there's no ., it simply returns the same value, so it works regardless of the dot.

[HolgerHees]

If I have understood this correctly, I believe no changes are required for JRuby, because:

I thought a second time about it and you are right, as I just depend on ruleUID on topLevel, I can revert my mentioned change... Means no change is needed from my side.

[holgerfriedrich]

LGTM, thanks.
Good to see the maintainers of the consuming libraries already involved.

[florian-h05]

Also populate the top level context with the contents of ctx

This has also been done before.

The only change is that the keys of the ctx map don’t have the numeric prefix anymore.

[Nadahar]

I'm trying to figure out how to make this information available in DSL, and I don't understand the logic here. Why was the prefixes removed? What were the prefixes? Were they module IDs, so that you could know which trigger/condition it originated from? If so, doesn't this change mean that entries can be overwritten of several conditions of the same type is used?

As far as I can understand, all the properties was already available in "stripped form" in the "root map". The prefixes were only retained in the ctx map, probably so that it should be possible to tell them apart.

Why does removing the prefixes "help" JS scripting, when the same keys were already available in "stripped form"? What value is it in having everything stored twice, not that the added information has been stripped?

[Nadahar]

As I suspected, the prefixes are the module IDs, and they are added just a few calls up the chain from where they are removed again by this PR:

openhab-core/bundles/org.openhab.core.automation/src/main/java/org/openhab/core/automation/internal/RuleEngineImpl.java

Lines 1251 to 1259 in 2169fe0

	private void updateContext(String ruleUID, String moduleUID, @Nullable Map<String, ?> outputs) {
	Map<String, @Nullable Object> context = getContext(ruleUID, null);
	if (outputs != null) {
	for (Map.Entry<String, ?> entry : outputs.entrySet()) {
	String key = moduleUID + OUTPUT_SEPARATOR + entry.getKey();
	context.put(key, entry.getValue());
	}
	}
	}

By removing them, you risk overwriting elements with the same key name from different modules, which means that information is just "lost". I don't know exactly how you can get the context to contain properties from several modules - it seems like conditions don't have outputs, only triggers and actions do. There can't be more than one trigger that actually triggers, but you can have many actions that could overwrite each other, which is probably why the prefixes was added in the first place.

[Nadahar]

The logic with the prefixes comes from the initial design of the automation engine: eclipse-archived/smarthome#317

Unfortunately, Eclipse has terminated the forum where this was explained...

I was able to find this post, which explains it pretty well: https://community.openhab.org/t/experimental-next-gen-rules-engine-documentation-4-of-writing-scripts/55963/6

In short, the module IDs play an important role, and this PR leads to loss of information. I can't quite see why the JS scripting add-on can't just remove the prefix before constructing the JS objects, which would suffer from the same "overwriting of information", but at east it would be limited to the JS add-on instead of removing the information for everybody. If I remember correctly, JS also still keeps the "raw event object" around, so that the information could actually be extracted if there was a need.

[Nadahar]

This is also broken now, which is an interesting aspect of the former structure:

The core.RunRuleAction (“run rules”) action forwards the context of the current rule to the rules it runs! This means, if all modules have unique IDs, you may be able to chain rules and still access outputs of modules of the calling rules (for instance, determine what was the oldState in the trigger of the original rule in a rule that was run from it). This was very interesting for my “flows builder” because it relies heavily on the “run rules” action to build flows into rules chained together.

[florian-h05]

Instead of reverting this change, I’d rather have both „behaviours“. Keep the prefixed context entries but also add the current context entries without a prefix.
If you just revert this, you might break scripts/rules that access the raw event object and depend on not having the prefixes. Also, IMO it would be against DRY to add the prefixes here so that most helper libraries strip them again.

[Nadahar]

Instead of reverting this change, I’d rather have both „behaviours“. Keep the prefixed context entries but also add the current context entries without a prefix.
If you just revert this, you might break scripts/rules that access the raw event object and depend on not having the prefixes.

I think you're missing something here, doing this actually corrupts the data. It caters to a special case where there only is data from one module available, and is detrimental when that's not the case.

Here is some debugging I just made from a branch where I've reverted this PR:

14:30:00.691 [ERROR] (OH-rule-managedDSL-1) [time.internal.engine.DSLScriptEngine] - Engine scope bindings: [result=652.6580191713 W/m², oh.extension-accessor=org.openhab.core.automation.module.script.internal.ScriptExtensionManager@6199f82c, oh.engine-identifier=f457d7b1-8375-49d9-affa-975d34f9a550, ctx={4.event=Timer 4 triggered., 7.result=13.22564492738843 °, ruleUID=managedDSL, 8.result=652.6580191713 W/m²}, oh.module-type-id=script.ScriptAction, ruleUID=managedDSL, event=Timer 4 triggered.]
14:30:00.693 [ERROR] (OH-rule-managedDSL-1) [org.openhab.core.model.script.ctx   ] - Ctx: {4.event=Timer 4 triggered., 7.result=13.22564492738843 °, ruleUID=managedDSL, 8.result=652.6580191713 W/m²}
14:30:00.694 [ERROR] (OH-rule-managedDSL-1) [penhab.core.model.script.eventObject] - eventObject: Timer 4 triggered.
14:30:00.694 [ERROR] (OH-rule-managedDSL-1) [nhab.core.model.script.modulesInputs] - modulesInputs: {4={event=Timer 4 triggered.}, 7={result=13.22564492738843 °}, 8={result=652.6580191713 W/m²}}

This is from my upcoming PR, so in addition to ctx, I extract eventObject by just taking the last Event that is encountered. It will work with the same convenience as in JS today, where it will pick "the event" when there's just one, and be basically useless if there are more than one. modelsInputs is a new map I've constructed that gathers outputs per module, so that it's easy to look up outputs from a particular module.

The module IDs aren't "random", they are dictated by the rule. The reason they are just "meaningless numbers" is because MainUI doesn't allow specifying them. The intention is that they should be meaningful and refer to the module in question, e.g. sunElevation. The whole structure makes more sense if module names were editable. They can be changed from YAML to something meaningful despite MainUI now exposing them.

This PR broke access to the event for all scripts across all languages from those that update frequently. Reverting it is the only way to prevent breaking the scripts of those that aren't as quick to upgrade. So, it's hard to say what's "the most breaking", but I'd say that leaving the unprefixed entries just causes chaos and confusion.

I made openhab/openhab-js#527 as a suggestion for how to handle this for JS before making any reversal. I would suggest to "mix" the entries from the collapsed map in there, if you're worried about breaking. It won't be much breaking anyway, because it will only change what's in the raw entry (as my suggestion is now). The eventObject will still point to a random Event, which will be the "correct one" when there's only one.

Also, IMO it would be against DRY to add the prefixes here so that most helper libraries strip them again.

That's not a valid application of the principle, this isn't some useless operation, it's a way to avoid name conflicts between modules and is necessary to not overwrite information. Helper libraries shouldn't strip them again, but what they do really isn't my business.

[florian-h05]

In what way does it overwrite data if I keep the context entries with module id prefixes and add entries without the prefix for current context values?
The prefixed values are all still available, and the unprefixed ones can be the latest ones.

[Nadahar]

In what way does it overwrite data if I keep the context entries with module id prefixes and add entries without the prefix for current context values?
The prefixed values are all still available, and the unprefixed ones can be the latest ones.

There are unprefixed entries in ctx too, those that don't stem from a module. If a module happens to use the same key as one of those already there, the module entry will overwrite the unprefixed entry.

Also, there's a misunderstanding here about what "version" will be used. The entries will keep overwriting each other, so the last one processed is what will "be" the entry. But, the iteration order is random for HashMaps (and most other Java maps), so it won't even be the same object between different executions. It's completely unreliable and not useful at for anything at all, as soon as more than one entry exists for a given key.

[Nadahar]

@florian-h05 I don't quite understand your resistance to doing this in the add-on. In my view, that's the right way to do it, because then we don't "corrupt" the data in core, and each add-on is free to handle it however they'd like.

Have you even looked at openhab/openhab-js#527? It's very easy to "merge" the collapsed ctx with the "full ctx" in the same code, it's probably as simple as using putAll(). That way, JS scripting users wouldn't get anything broken, they could reference them either way. The one downside is that "root entries" in ctx could be overwritten by module entries, but I guess the risk is relatively low.

Since, according to comments in this PR, the Ruby and Python add-ons didn't have to change for this PR, they won't have to change if it's reversed either. They already strip the keys and "collapse" the map. So, nothing will break there either.

This isn't yet available in DSL, this is what I'm working on and why I've come across this issue, so there's no breaking there either.

That leaves the potential breaking to Groovy and Jython users that have used these entries after this PR was merged. If so, they already have dealt with it breaking once, and I'm sure they can deal with it "going back". But, I suspect that the number of users this would affect is pretty close to zero.

[florian-h05]

With openhab/openhab-js#527 merged, I'll publish a new openhab-js release soon, so you can revert this PR here.