Rules and return codes

[rlkoshak (Rich Koshak)]

tl;dr: Give me a flag to detect when the wrapper is enabled, and I’m happy. I’m cautious about shipping with the wrapper enabled though as it is a breaking change.

@Nadahar

But, are you saying that if you use return in a “wrapped” condition, the return value gets through to OH?

Yes.

@florian-h05

How is it determined what’s the return value of a condition if there is no return statement?

The result of the last line executed becomes the implicit return value. I’ve always thought that was weird, but it’s how it’s always worked since the Experimental Rules Engine was first introduced way back in OH 2.1 or 2.2 (I remember having a discussion with @ysc about ti when I first started experimenting with it).

There is precedence for this as this is how the return for a Rules DSL lambda function is determined too.

But you just have to be careful to make sure that all the last lines of a condition that could occur evaluate to a boolean.

For example, here’s an actual one of mine:

console.loggerName = 'org.openhab.automation.rules_tools.Debounce';
// ~ TODO ~ TODO ~ TODO ~ TODO ~ TODO ~ TODO ~ TODO ~ TODO
// Rework when `event` always exists and check the event.type
if(this.event !== undefined && this.event.itemName !== undefined){
  const item = items.getItem(this.event.itemName);
  if(item.isUninitialized) {
    console.debug('Debounce for Item', this.event.itemName, 'is blocked for state', this.event.itemState);
  }
  !item.isUninitialized;
}
else {
  console.trace('Debounce passed conditions');
  true;
}

That condition works when the wrapper is turned off. I must modify it to the following when the rapper is turned on.

console.loggerName = 'org.openhab.automation.rules_tools.Debounce';
// ~ TODO ~ TODO ~ TODO ~ TODO ~ TODO ~ TODO ~ TODO ~ TODO
// Rework when `event` always exists and check the event.type
if(this.event !== undefined && this.event.itemName !== undefined){
  const item = items.getItem(this.event.itemName);
  if(item.isUninitialized) {
    console.debug('Debounce for Item', this.event.itemName, 'is blocked for state', this.event.itemState);
  }
  return !item.isUninitialized;
}
else {
  console.trace('Debounce passed conditions');
  return true;
}

Don’t get me wrong, I much prefer having an explicit return. But making the wrapper configurable adds all kinds of headaches. You can never know from within the rule what to do.

@florian-h05

I would expect the wrapper to be enabled by default.

As long as the wrapper can be turned off, there will have to be two versions of rule templates, or we need to abandon those who turn it off or turn it on and say “too bad, this script won’t work for you anymore unless you disable the wrapper.” Or abandon the use of rule conditions entirely. I don’t like any of those choices.

@florian-h05

Maybe we can do something with the upgrade tool there, or we detect whether a script is an action or a condition and only apply the wrapper if its an action.

As long as this is user configurable, I don’t see how the upgrade tool can help. That only runs during an upgrade, but the user can enable or disable the wrapper at any time.

Only applying the wrapper if it’s an action would be the least disruptive approach compared to what’s been mentioned thus far. However it would mean that the conditions would only get the raw Java event and have additional restrictions that do not exist for Script Actions. I don’t like that one bit. I think we need a bigger reason than this to justify that.

But my biggest limitation is lack of a reliable way to detect in the script whether the wrapper is enabled or not. Maybe the wrapper can add a variable when it calls the script, or openhab-js utils can have a little function to call to tell us whether the wrapper is enabled or not? Maybe I can access the add-ons settings from a registry or API call? I can work with that.

Would it be enough to see if event.raw !== undefined? Is that always guaranteed to be there when the wrapper is enabled?

It occurs to me that I need that info in my script actions too for my templates because it’s not enough to just check that the openhab-js version > 5.14.0 since the wrapper can be turned off. And to maintain backward compatibility and avoid deprecation warnings I need to get the event variables a different way depending on whether the wrapper is on or not. For example, this is what I currently do in Debounce:

var triggerType = null;
var newState = null;
if(this.event !== undefined) {
  if(helpers.compareVersions(utils.OPENHAB_JS_VERSION, '5.14.0') < 0){
    console.debug('Running on version before 5.14.0');
    triggerType = event.type.toString();
    newState = event.itemState.toString();
    itemCommand = event.itemCommand.toString();
  } 
  else {
    console.debug('Running on 5.14.0 or greater');
    triggerType = event.triggerType;
    newState = event.receivedState;
    itemCommand = event.receivedCommand;
  }
}

Clearly that won’t work since the wrapper can be disabled, but if I have a flag I can test instead of version numbers that overall approach will still work. I’d do the same for my conditions. Having the return statement in the code isn’t a problem. It’s only a problem when OH tries to execute it.

No matter what, some way to determine if the wrapper is enbled from within a script is something I think I really do need beyond just this conditions issue.

With that, my only concern is that shipping with the wrapper enabled would be a breaking change. It’s easy to just turn it off, and we can document that in the release notes so it’s not a huge breaking change. Or we can ship it off by default until OH 6. Or something in between.