[DSL] Add extra commands/abilities to DSL scripts/rules

[Nadahar]

Add access to various system registries, OSGi instances and the ability to run and enable/disable rules to DSL scripts.

This has been considered "impossible" to do, yet I've met no special obstacles, and it seems to work with my (limited) testing. I'm therefore skeptical that there might be something that I've missed, like additional startup issues caused by ScriptServiceUtil referencing additional instances, thus depending on them. But, I've observed no such problems.

Additional context/information about the historical problems with this is appreciated, but as far as I can see at the moment, this works without complications.

[Nadahar]

Also, consider the specific new commands I've implemented as more of a suggestion. This can be tweaked as desired.

@rkoshak Perhaps you have something to add here?

[Nadahar]

The itest fails because of some unresolved reference. I'm trying to figure it out, but if anybody has any hints of how to figure out the problem, it would be appreciated, because I frankly have no idea how to handle the bndrun resolution.

[Nadahar]

I don't think the itest fails over dependencies, but that the startlevel is insufficient:

Components implementing org.openhab.core.model.script.ScriptServiceUtil:
129 [UNSATISFIED_REFERENCE] org.openhab.core.model.script.ScriptServiceUtil in org.openhab.core.model.script
	$006 (org.openhab.core.automation.RuleManager)
		54 [UNSATISFIED_REFERENCE] org.openhab.core.automation.internal.RuleEngineImpl in org.openhab.core.automation
			$004 (org.openhab.core.service.StartLevelService)

If I understand it correctly, RuleEngineImpl can't start if the startlevel is below 40 - and in the test, it is. This requirement now applies to ScriptServiceUtil because I've included a reference to RuleManager (which is implemented by RuleEngineImpl) in the constructor.

So, the question is: Is it actually a problem that ScriptServiceUtil can't start before startlevel 40, or does the test need to be modified to get around this?

[Nadahar]

I've set the startlevel in the test to 40, which is required for the script engine to start, and it seems to resolve the problem with the test. I'm still uncertain as to whether reaching startlevel 40 before the ScriptServiceUtil can start is a problem or not.

Are there scripts being executed before startlevel 40 is reached?

[Nadahar]

Now there's a new test failure, not sure exactly how it is related yet:

TEST org.openhab.core.model.rule.runtime.DSLRuleProviderTest#testSimpleRules() <<< ERROR: Cannot invoke "org.eclipse.emf.ecore.resource.Resource.load(java.io.InputStream, java.util.Map)" because "resource" is null
java.lang.NullPointerException: Cannot invoke "org.eclipse.emf.ecore.resource.Resource.load(java.io.InputStream, java.util.Map)" because "resource" is null

[rkoshak]

Some comments.

[Nadahar]

The current test failure seems to have some complicated timing reason. The following line in ModelRepository.validateModel() returns null, which doesn't seem expected:

        Resource resource = resourceSet.createResource(URI.createURI(PREFIX_TMP_MODEL + name));

This results in an NPE. The question is why it returns null now all of a sudden, and it probably has to do with how the changes in this PR has impacted the startup order of OSGi components. The Javadocs for createResource() says:

Creates a new resource, of the appropriate type, and returns it.

It delegates to the resource factory registry to determine the correct factory, and then it uses that factory to create the resource and adds it to the contents. If there is no registered factory, null will be returned; when running within Eclipse, a default XMI factory will be registered, and this will never return null.

I'm not sure what this mythical factory is and why it suddenly can't be found, but I assume that it has something to do with ScriptServiceUtil now having to wait for startlevel 40 before starting. The details of how this all relates aren't clear to me though.

[Nadahar]

I changed the strategy regarding RuleManager. It requires startlevel 40 to start, which caused all kind of timing-based issues with tests when it became a reference of ScriptServiceUtil. I've thus modified it to be a dynamic reference, so that ScriptServiceUtil can start without RuleManager having been started.

This has resolved the test problems, but comes with the caveat that getRuleManager() might return null. Still, all in all, I think it's the best solution. It shouldn't return null when rules are actually running, since it's the one executing rule actions. So, in reality, it might actually never return null.

[Nadahar]

In case somebody wants to test it for themselves, this bundle should be all that's needed.

org.openhab.core.model.script-5.2.0-SNAPSHOT.jar.txt

[Nadahar]

@rkoshak I've added some additional methods for manipulation of metadata. Is that somewhat what you had in mind? I've also reluctantly implemented the varargs "solution" for the maps, so that it's possible to simply call e.g. addMetadata(namespace, itemName, value, configKey1, configValue1, configKey2, configValue2...) (the same for the runNow() context).

Since these are just suggestions, I haven't bothered to finish the Javadocs, as I'm sure they'll change, which makes the time I spend writing the documentation a waste.

[Nadahar]

@rkoshak I've added even more overloads, so not all the metadata methods accept either item name or Item as the first argument. Doesn't that make it pretty close to what JS provides?

[Nadahar]

@lolodomo There's no a lot of activity here, maybe you're interested in this?

This isn't important for me in any way, but seen in light of all the other discussions about DSL versus other scripting languages and the difference in what you can do, I thought that this might be a welcome contribution to close the gap.

[Nadahar]

Ï've never fully understood the "action" concept in OH. I understand that they are similar to functions/methods, but not what the significance of making things "actions" is. We have for example org.openhab.core.model.script.actions.Things, which doesn't implement ActionService, but exists in package actions. Then, we have org.openhab.core.model.script.internal.engine.action.ThingActionService, which does implement ActionService. The former basically delegates to the latter, so they both provide the exact same functionality.

Why this structure? Is an ActionService available more broadly, to other scripting languages for example? It doesn't seem so, since it's placed in an internal package in the code that makes up DSL.

[Nadahar]

I've reorganized System into different classes. I've also discovered and implemented extensions, so that it's now possible to do e.g.:

var shared = getRule("shared-test-1")
logInfo("test", shared.getUID() + " enabled: " + shared.isEnabled())
shared.setEnabled(true)
logInfo("test", shared.getUID() + " enabled: " + shared.isEnabled())
shared.run()

The "full versions" are also there, so you can call setRuleEnabled(ruleUID, enabled) instead. I've done the same for Items, so you can just do e.g. someItem.addMetadata(namespace, value).

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/rules-and-rule-templates-yaml-integration/168568/222

[dilyanpalauzov]

#5484 adds Import-Package: org.openhab.core.automation to both org.openhab.core.model.rule/bnd.bnd and org.openhab.core.model.script/bnd.bnd. In turn org.openhab.core.automation.RuleManager can be resolved in both DSL Scripts and Textual DSL Rules.

Running afterwards RuleManager.runNow() is demonstrated at openhab/openhab-docs#2701.

[Nadahar]

#5484 adds Import-Package: org.openhab.core.automation to both org.openhab.core.model.rule/bnd.bnd and org.openhab.core.model.script/bnd.bnd. In turn org.openhab.core.automation.RuleManager can be resolved in both DSL Scripts and Textual DSL Rules.

Running afterwards RuleManager.runNow() is demonstrated at openhab/openhab-docs#2701.

Adding org.openhab.core.automation import to model.rule I'm sure is useful, I'm surprised it's not already there. This PR is about the model.script bundle, where it also adds the same import.

That said, the goal of this PR isn't to make it possible to do those things, but to make it reasonably convenient. As long as imports can be resolved, you can always use FrameworkUtil manually like your example does, but I wouldn't call that very user-friendly.

This PR has getInstance() to do exactly the same operation for you, without having to deal with all the "OSGi details":

    public static @Nullable <T> T getInstance(Class<T> clazz) {
        Bundle bundle = FrameworkUtil.getBundle(clazz);
        if (bundle != null) {
            BundleContext bc = bundle.getBundleContext();
            if (bc != null) {
                ServiceReference<T> ref = bc.getServiceReference(clazz);
                if (ref != null) {
                    return bc.getService(ref);
                }
            }
        }
        return null;
    }

So, I don't see these two as "competing" in any way.

[dilyanpalauzov]

After a = BundleContext.getService(…) there must be corresponding BundleContext.ungetService(a) - https://docs.osgi.org/javadoc/osgi.core/8.0.0/org/osgi/framework/BundleContext.html#getService-org.osgi.framework.ServiceReference- - otherwise resources stay allocated unnecessary.

By using the function getInstance() from the previous comment it is not possible to invoke ungetService() on the result of getService().

[dilyanpalauzov]

After a = BundleContext.getService(…) there must be corresponding BundleContext.ungetService(a)

Rectification:

After a = BundleContext.getServiceReference(…) there must be corresponding BundleContext.ungetService(a).

[Nadahar]

I didn't remember that the services were reference counted, so I'll have to make some modifications to accommodate that. But, as I see it, we can't expect script authors to use try/finally to make sure to release the reference. I've looked a bit in core, and it seems like this is "mishandled" other places as well. If you look at JS Scripting, it does the exact same thing:

https://openhab.github.io/openhab-js/osgi.js.html#line38

Since it's unrealistic that script authors can manage the reference counting, I think the best thing to do is to release the reference before returning it. That will work just fine as long as something else keeps a "counted reference" to the instance. In OH, this will almost always be the case, there are other components that reference "everything".

As far as I understand, OSGi uses this reference counting to automatically stop services that aren't used by anything. The fact that a service is referenced doesn't prevent an explicit shutdown, so it's not like "a lock", it just prevents automatic shutdown. Since the script acquiring the reference should never have the only reference, releasing it before returning should work fine in practice. In the very unlikely scenario that this isn't the case, the script will fail when it tries to use the service if OSGi has shut it down in the meanwhile. I think that is acceptable, given that it is extremely unlikely to happen, and it should be better than to "leak references".

[lolodomo]

Very promising contribution.
Just few comments

[lolodomo]

@Nadahar : thank you for that contribution. Why is it still in draft mode ? Does it mean it is not yet finished ?

[dilyanpalauzov]

https://www.openhab.org/docs/configuration/persistence.html#persistence-extensions-in-scripts-and-rules suggests that ChronoUnits.DAYS can be used. But nowhere is stated, that DAYS and ChronoUnit.DAYS are the same thing, explaining that this is implied by implicit imports.

Likewise I find no statement, that both TUESDAY and DayOfWeek.TUESDAY can be used in DSL Rule/Scripts/Transformations and that DayOfWeek.TUESDAY is identical to TUESDAY, because of the implicit imports.

So im my eyes, an excerpt of all implicit imports is documented, but not all of them.

I find also no documentation, stating that "A".logError("B") is identical to logError("A", "B") but it is anyway identical.

So there are plenty of undocumented implicit imports.

[mherwege]

I didn’t say the documentation was complete. But there is more then you implied.
Your log example is by the way not about implicit imports, but about the fact that it is also an extension. The implicit import is documented. Question is if it should actually also be an extension. But that’s another debate.

I seem to remember there was some debate at some point about ChronoUnit.DAYS vs just DAYS. There was some breaking change replacing one by the other. In the end, both were accepted.

[Nadahar]

I find also no documentation, stating that "A".logError("B") is identical to logError("A", "B") but it is anyway identical.

This is because all the actions are also registered as extensions. I've pointed out how I think this is just wrong earlier, but changing it now risks breaking things. I don't think it's documented because "A".logError("B") was never intended to be a thing. It's the result of logError("A", "B") being registered as an extension, where the first parameter is seen as the type to extends, and the others are kept. As a result, with the current system, a lot of strange and unintended extensions exists for String, ZonedDateTime and some other types.

This isn't something I've attempted to "clean up" in this PR, ideally it should probably be, but it will be breaking in some way or another. I contemplated making a new annotation or similar, where each action method would need to be explicitly annotated to also register as an extension. Because some of them are meant to be extensions as well, like the persistence methods.

[mherwege]

As a result, with the current system, a lot of strange and unintended extensions exists for String, ZonedDateTime and some other types.

Exactly. But just because they work doesn't mean we need to explicitly document and promote these. I will already be very happy if we get everything documented that actually makes sense. And that's where I think we are less far off than some of the discussion implies. There are gaps for sure, but there is more documented then just State and Command.

[mherwege]

I seem to remember there was some debate at some point about ChronoUnit.DAYS vs just DAYS. There was some breaking change replacing one by the other. In the end, both were accepted.

Found the reference: #4791

[Nadahar]

@mherwege I've revised the imports - please check.

[Nadahar]

I thought that with such a simple change, I couldn't possibly upset spotless, so I didn't run it. I was wrong 😒

[Nadahar]

The Java25 failure seems to be unrelated to this PR, in a bundle not touched by this PR. I'd say it's most likely flaky tests, although I didn't investigate further when I saw where it was.

[Nadahar]

@mherwege I get some strange errors when trying to run this branch now - has something been merged that somehow makes the code incompatible?

org.openhab.core.model.core [org.openhab.core.model.core.internal.folder.FolderObserver] ERROR : Error handling update of file 'C:\Repos\Java\openHAB\openhab-distro\launch\app\runtime\conf\sitemaps\demo.sitemap': Unable to provision, see the following errors:

1) [Guice/ErrorInjectingConstructor]: WrappedException: ClasspathUriResolutionException: FileNotFoundOnClasspathException: Couldn't find resource on classpath. URI was 'classpath:/org/openhab/core/model/sitemap/Sitemap.xmi'
  at TerminalsGrammarAccess.<init>(TerminalsGrammarAccess.java:36)
  at TerminalsGrammarAccess.class(TerminalsGrammarAccess.java:36)
  at SitemapGrammarAccess.<init>(SitemapGrammarAccess.java:5069)
      \_ for 2nd parameter
  at SitemapGrammarAccess.class(SitemapGrammarAccess.java:5069)
  while locating SitemapGrammarAccess
  at LazyLinker.grammarAccess(LazyLinker.java:300)
      \_ for field grammarAccess
  while locating LazyLinker
  at XtextResource.linker(XtextResource.java:506)
      \_ for field linker
  while locating LazyLinkingResource
  while locating XtextResource

I don't quite understand it, because I've built all of core successfully on this branch, yet I still get these failures during runtime.

I guess I should just rebase it and see if that solves it, but I'm not sure if any review is "pending" here at the moment.

[Nadahar]

It seems to have cleared up after a rebase, so the solution is probably just to push the rebased version.

[Nadahar]

I take the lack of response of a sign that nobody is in the middle of something here, so I'll push the rebased version.

[Nadahar]

I've added two additional commits, one adds access to TimeZone, ZoneId and Locale, the other just moves the new classes into the same package as the existing package lib where NumberExtensions live. I hadn't discovered that, but there's no reason to create a new package to hold basically the same type of classes.