Rules and return codes

Development
1

I’m not sure if this is a real problem or if it’s a problem with the way I expet things to work.

I’m seeing a bunch of errors from managed rules that haven’t been touched in forever which have a Condition.

It doesn’t seem to be every time the rule runs, but when I do see the error it complains that the script condition returned null instead of a boolean. I’ve tried changing it to use return instead of just leaving the last executed line as the return value but I still get the error.

I can’t tell if it’s every time or only occasionally. I’m still gathering data but posting now in case others are seeing this and can provide more data.

Example log:

2025-09-23 12:37:08.975 [ERROR] [ernal.handler.ScriptConditionHandler] - Script of rule with UID 'weather_ligs' did not return a boolean value, but 'null'

The script condition:

!items.Outdoors_Weighted_Brightness.isUninitialized;

I’ve also tried

return !items.Outdoors_Weighted_Brightness.isUninitialized;

Edit 1: I think I can confirm that it’s failing for all rule executions where there is a rule condition. It always give this error and it prevents the rule from running. @florian-h05, could this be related to the changes to JS Scripting and the function wrapper?

Edit 2: In at least one case adding return to the condition fixed the problem. Investigation continues…

2

Yes, that’s from this PR. The version range in the title has been reported as a bug in the past, and after looking at it, I agreed that it was in fact a bug. It was only shown after the add-on had been installed, and was intentionally hidden when you browsed the add-ons. As I interpreted it, it was clearly an omission, the range was never meant to be displayed.

You could argue that the information could be useful, however that should then be displayed in a more user-friendly way, consistently. Users can’t be expected to be familiar with the Maven version range syntax. The information is still used to filter which add-ons are shown, which is the purpose of the field.

In which situation is having it there considered useful?

3

I don’t think so, but you can simply try by turning off the wrapper in the add-on settings.

4

Could it be related to this?

https://github.com/openhab/openhab-core/pull/4922/files#diff-64adc40a8d9ec0fa4597ca08419dd4600131685159603fc6d2481dc8ce7b4d7e

5

ping @stefan.hoehn Blockly needs fix see post#6

6

Thanks @mstormi , didn’t see that.

is a normalization between the event Object between file based and managed rules.

@rlkoshak @florian-h05 can you explain exactly what needs to be done or where this is documented, so I know what I need to change in Blockly?

7

I don’t see a possibility how this would change the behaviour of a script in its execution.

8

I should have added a column to the event object table to better „explain“ what needs to be changed … though it is relatively easy to find out when looking at the code that provides the backwards compatibility and logs the deprecation warning and what to use instead:

9

I can confirm that after turning off the wrapper settings (and restarting OH) it works without the return statement.

Based on my tests so far, if the wrapper is turned on return is required in Script Conditions and if the wrapper is turned off return cannot be used. Toggling the setting appears to require a restart of OH, though it might just require a disable/enable of the rules. I haven’t tested that yet.

Unless there is a way to determine from a rule whether the wrapper is turned on or not this could be really-bad for rule templates in particular. Even with that ability it complicates code sharing and debugging problems/helping users as well.

10

It seems that @rlkoshak has confirmed that this PR isn’t related, I just saw that it matched both code location and timeline quite closely, so thought it was worth consideration.

11

It requires that the scripts are resaved.

Apart from the limitation with conditions, I don’t think there are other disadvantages of the wrapper. Effectively file based rules (their callback) are wrapped similarly.

12

The disadvantage is any rule template I publish that uses a condition (and I have several) has to be published twice now, one with the return and one without the return (that’s the only difference) or rewritten to not use conditions at all. Because there is no way to detect if the wrapper is enabled there is no way to make one rule template compatible with both the wrapper being on or off.

That adds complexity because the users who install the rule template now need to know what their wrapper setting is, and they need to know to install the correct version of the template based on that settings. Not to mention doubling the number of rule template postings since it’s still one post per rule template and the maintenance headache maintaining two basically identical versions of the same rule template.

As an aside, @stefan.hoehn, in addition to the changes with event, a return block will need to be added or else anyone using JS with the wrapper enabled will not be able to use Blockly for rule conditions at all (except through the inline script block).

Also, the wrapper needs to ship toggled off by default or else this is a breaking change. Every existing managed rule using a JS Condition will break with the wrapper enabled.

13

I’m not 100% sure, but I don’t think this is the only problem here. I think it also breaks something fundamental in OH: Even though they aren’t usually used, Actions too can return values - and these can be used as inputs for other modules. If I understand it correctly, this wrapping means that scripts can’t return a value in general, and that isn’t good.

Isn’t there a different way to do the wrapping itself, where a potential return value is “forwarded”?

The wrapping, as it is now, is very “basic”:

    @Override
    protected String onScript(String script) {
        if (isUiBasedScript() && configuration.isWrapperEnabled()) {
            logger.debug("Wrapping script for engine '{}' ...", engineIdentifier);

            String eventConversionScript = "";
            if (configuration.isEventConversionEnabled()) {
                eventConversionScript = EVENT_CONVERSION_CODE + System.lineSeparator();
            }

            return "(function() {" + System.lineSeparator() + eventConversionScript + script + System.lineSeparator()
                    + "})()";
        }
        return super.onScript(script);
    }

I’m no JS guru, but can’t you basically abuse/manipulate anything in JS so that you could manage to make the wrapper return something?

14

That’s not really the issue here.

The issue is that when the wrapper is not enabled, a JS script condition might look like this:

!items.Foo.isUninitialized && items.Bar.lastStateUpdate.isBefore(time.toZDT('PT-5S'))

You can not use return or else you get an error.

When the wrapper is enabled, a JS scipt condition might look like this:

return !items.Foo.isUninitialized && items.Bar.lastStateUpdate.isBefore(time.toZDT('PT-5S'))

You must use return or else you get an error.

There is no way to detect whether the wrapper is enabled or not (maybe I can do something with the event Object?) so there is no way to write a JS script condition that will work whether or not the wrapper is enabled. The code must be different depending on that wrapper setting.

15

There probably are things I don’t understand here, but I was thinking of the original error reported from OH where the condition returns null. I interpret that as if the return value never “reaches” the OH Java code. But, are you saying that if you use return in a “wrapped” condition, the return value gets through to OH?

16

It seems like there is an implicit return statement inserted somewhere here then, do any of you know where this is done?

17

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

I would expect the wrapper to be enabled by default.
If we fix the problem with script conditions, there shouldn‘t be any major issues with actions. Yes, they can returna value, but when having 20 lines of code, its very unclear what‘s returned when there is no return statement.
And ultimately, the wrappr allows the use of return, class, function, let and const keywords which allows writing much more powerful and cleaner code.

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.

That‘s wrong. You need to use the return keyword.

That‘s not correct. There was no need for declaring a return value.
Think of it like the following for script conditions:
We eval(‚script condition‘) and check whether the result is true. Without the wrapper, we eval a condition that returns a boolean. With the wrapper we eval a self-invoking function that contains some conditional code, but returns nothing as there is no return used.

It‘s not inserted, it‘s just the above difference in what is evaluated.

18

That’s what I mean by “implicit return”. It looks like the expression without the wrapper is written so that the result of the expression is automatically returned..?

There’s still a lot of complexity to handle here, because even if you assume that everybody from 5.1 have this enabled, you must have two versions of the rule templates, one for > 5.1 and one for >= 5.1.

Ok, I don’t quite follow, but isn’t there a way to handle both situations by making the wrapper “more elaborate”?

What about something as crude as simply erasing the return statement from the script if the wrapper is not enabled before processing? That way, one could use return in the script itself, and it should work either way? It would still mean incompatibility between before 5.1 and not though.

19

Is there a reason the wrapper has to be a global configuration instead of being an option on individual rules? If it could be disabled for specific rules, then the rule template problem shouldn’t be an issue at all assuming that the wrapper config is also contained in the template.

Also, users who experience breaking issues during upgrade (or the upgrade tool itself) could simply deactivate the wrapper on those rules that are incompatible.

20

I basically answered by question below where I asked it, I can imagine how this works …

I the wrapper is only applied for actions, then we can avoid this situation.

That‘s also quite hacky …

There is no way of adding add-on specific config to rules.