Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bundles/org.openhab.core.model.script/bnd.bnd
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Export-Package: org.openhab.core.model.script,\
org.openhab.core.model.script.validation
Import-Package: \
org.openhab.core.audio,\
org.openhab.core.automation,\
org.openhab.core.automation.module.script.action,\
org.openhab.core.automation.module.script.rulesupport.shared,\
org.openhab.core.common.registry,\
Expand Down
5 changes: 5 additions & 0 deletions bundles/org.openhab.core.model.script/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@
<artifactId>org.openhab.core.io.net</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.openhab.core.bundles</groupId>
<artifactId>org.openhab.core.automation</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.openhab.core.bundles</groupId>
<artifactId>org.openhab.core.automation.module.script</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicReference;

import org.openhab.core.automation.RuleManager;
import org.openhab.core.automation.RuleRegistry;
import org.openhab.core.events.EventPublisher;
import org.openhab.core.items.ItemRegistry;
import org.openhab.core.items.MetadataRegistry;
import org.openhab.core.model.core.ModelRepository;
import org.openhab.core.model.script.engine.ScriptEngine;
import org.openhab.core.model.script.engine.action.ActionService;
Expand Down Expand Up @@ -50,6 +53,9 @@ public class ScriptServiceUtil {
private final ThingRegistry thingRegistry;
private final EventPublisher eventPublisher;
private final ModelRepository modelRepository;
private final MetadataRegistry metadataRegistry;
private final RuleRegistry ruleRegistry;
private final RuleManager ruleManager;
private final Scheduler scheduler;

private final AtomicReference<ScriptEngine> scriptEngine = new AtomicReference<>();
Expand All @@ -59,11 +65,15 @@ public class ScriptServiceUtil {
@Activate
public ScriptServiceUtil(final @Reference ItemRegistry itemRegistry, final @Reference ThingRegistry thingRegistry,
final @Reference EventPublisher eventPublisher, final @Reference ModelRepository modelRepository,
final @Reference Scheduler scheduler) {
final @Reference MetadataRegistry metadataRegistry, final @Reference RuleRegistry ruleRegistry,
final @Reference RuleManager ruleManager, final @Reference Scheduler scheduler) {
this.itemRegistry = itemRegistry;
this.thingRegistry = thingRegistry;
this.eventPublisher = eventPublisher;
this.modelRepository = modelRepository;
this.metadataRegistry = metadataRegistry;
this.ruleRegistry = ruleRegistry;
this.ruleManager = ruleManager;
this.scheduler = scheduler;

if (instance != null) {
Expand Down Expand Up @@ -91,6 +101,10 @@ public ItemRegistry getItemRegistryInstance() {
return itemRegistry;
}

public static ThingRegistry getThingRegistry() {
return getInstance().thingRegistry;
}

public ThingRegistry getThingRegistryInstance() {
return thingRegistry;
}
Expand All @@ -107,6 +121,30 @@ public ModelRepository getModelRepositoryInstance() {
return modelRepository;
}

public static MetadataRegistry getMetadataRegistry() {
return getInstance().metadataRegistry;
}

public MetadataRegistry getMetadataRegistryInstance() {
return metadataRegistry;
}

public static RuleRegistry getRuleRegistry() {
return getInstance().ruleRegistry;
}

public RuleRegistry getRuleRegistryInstance() {
return ruleRegistry;
}

public static RuleManager getRuleManager() {
return getInstance().ruleManager;
}

public RuleManager getRuleManagerInstance() {
return ruleManager;
}

public static Scheduler getScheduler() {
return getInstance().scheduler;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
/*
* Copyright (c) 2010-2026 Contributors to the openHAB project
*
* See the NOTICE file(s) distributed with this work for additional
* information.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*/
package org.openhab.core.model.script.actions;

import java.util.Map;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.core.automation.RuleManager;
import org.openhab.core.automation.RuleRegistry;
import org.openhab.core.items.ItemRegistry;
import org.openhab.core.items.MetadataRegistry;
import org.openhab.core.model.script.ScriptServiceUtil;
import org.openhab.core.model.script.engine.action.ActionDoc;
import org.openhab.core.thing.ThingRegistry;
import org.osgi.framework.Bundle;
import org.osgi.framework.BundleContext;
import org.osgi.framework.FrameworkUtil;
import org.osgi.framework.ServiceReference;

/**
* {@link System} provides DSL access to things like OSGi instances, system registries and the ability to run other
* rules.
*
* @author Ravi Nadahar - Initial contribution
*/
@NonNullByDefault
public class System {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"System" is a pretty generic and almost meaningless name. Would something like "Registries" be feasible? Everything here either exposes or lets users interact with one of the registries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name can be anything, but it does handle more than registries. It also provides OSGi instances and some rule manipulation methods. I chose system because I couldn't really find anything "suitable".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also considered names like "OSGi", "Core", "OpenHAB", but they can all be considered "wrong" or misunderstood. The users don't actually have to see this name though. Look at ScriptExecution that creates timers for example. My guess is that nobody knows/cares, because they don't have to qualify the method with the class name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about "Services" or "Utility"? They don't mean much more than "System", but it's hard to find something that means much and also fits the methods.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about "Helper"?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at ScriptExecution that creates timers for example. My guess is that nobody knows/cares, because they don't have to qualify the method with the class name.

All of the JSR223 languages have to deal with those class names. It's really only Rules DSL that doesn't. But since this is all for the benefit of Rules DSL in the first place maybe it doesn't matter.

that means much and also fits the methods.

Which brings up something I decided not to mention. Would it make more sense to split this into separate classes, one for rules, one for metadata, one for things and one for Items? Then the naming becomes obvious and clear.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be done, but I'm not sure if there's much to gain by doing it. It would mean more classes that had to be added to the "implicit DSL context", but whether that has any practical consequences I don't know. Probably not.


/**
* Retrieve an OSGi instance of the specified {@link Class}, if it exists.
*
* @param <T> the class type.
* @param clazz the class of the instance to get.
* @return The instance or {@code null} if the instance wasn't found.
*/
@ActionDoc(text = "acquire an OSGi instance")
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;
}

/**
* Run the rule with the specified UID.
*
* @param ruleUID the UID of the rule to run.
* @return A copy of the rule context, including possible return values.
* @throws IllegalArgumentException If a rule with the specified UID doesn't exist.
*/
@ActionDoc(text = "run the rule with the specified UID")
public static Map<String, Object> runRule(String ruleUID) {
RuleManager ruleManager = ScriptServiceUtil.getRuleManager();
if (ruleManager.getStatus(ruleUID) == null) {
throw new IllegalArgumentException("Rule with UID '" + ruleUID + "' doesn't exist");
}
return ruleManager.runNow(ruleUID);
}

/**
* Run the rule with the specified UID with the specified context.
*
* @param ruleUID the UID of the rule to run.
* @param context the {@link Map} of {@link String} and {@link Object} pairs that constitutes the context.
* @return A copy of the rule context, including possible return values.
* @throws IllegalArgumentException If a rule with the specified UID doesn't exist.
*/
@ActionDoc(text = "run the rule with the specified UID and context")
public static Map<String, Object> runRule(String ruleUID, Map<String, Object> context) {
RuleManager ruleManager = ScriptServiceUtil.getRuleManager();
if (ruleManager.getStatus(ruleUID) == null) {
throw new IllegalArgumentException("Rule with UID '" + ruleUID + "' doesn't exist");
}
return ruleManager.runNow(ruleUID, false, context);
Comment thread
Nadahar marked this conversation as resolved.
Outdated
}

/**
* Run the rule with the specified UID with the specified context, while optionally taking conditions into
* account.
*
* @param ruleUID the UID of the rule to run.
* @param considerConditions {@code true} to not run the rule if its conditions don't qualify.
* @param context the {@link Map} of {@link String} and {@link Object} pairs that constitutes the context.
* @return A copy of the rule context, including possible return values.
* @throws IllegalArgumentException If a rule with the specified UID doesn't exist.
*/
@ActionDoc(text = "run the rule with the specified UID, condition evaluation setting and context")
public static Map<String, Object> runRule(String ruleUID, boolean considerConditions, Map<String, Object> context) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems weird to make considerConditions the second argument given there is a version of the method that includes the context but doesn't take the condition boolean. That is how it appears in JS. I don't know the other languages.

As an end user who knows nothing of the RuleManager, I would expect the actions to be:

  • runRule(ruleUID)
  • runRule(ruleUID, context)
  • runRule(ruleUID, context, considerConditions)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the parameter order of the underlying methods. I'd say that it's not a big deal either way, but that perhaps it would be useful with a runRule(ruleUID, considerConditions) as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it comes to JS, that's an entirely different thing. It allows that you only specify some parameters, the rest are undefined (?). This means that you must try to guesstimate what parameters people are most likely to drop, and then list those last.

Here, there is no such thing, you must have an exact signature match, so the order doesn't have any significance, unless you use varargs that can't have anything following them (in a sense, in JS, "everything" are varargs of Object). Because collections can often optionally be specified as varargs, you often put them last - even when a varargs method doesn't exist. I could have made a version here where context could be Entry<String, Object>.... In that case, context would have to be last, and my guess is that because that's a thing people get used to, it is somehow intuitive to put the collection last anyway. I didn't do this here because it's awkward to have to create Entry instances for each "context pair" you wish to include, so you might as well provide a Map.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really wanted to go down the road of type-unsafety, I could also let the last parameter be Object..., and then "expect" them to come in pairs where the first was a string and the second some object. The method could then "build" the map and throw an exception if the parameters didn't meet expectations. It might be the most convenient way one could supply context, since you could just supply strings directly in the call, but it "feels" hacky and something you'd do in JS or similar, not in a typed language.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that perhaps it would be useful with a runRule(ruleUID, considerConditions) as well?

I'm certainly not against it.

so the order doesn't have any significance,

I just brought up the JS as an example of following a pattern. Given runRule(ruleUID) and runRule(ruleUID, context), the expected next method signature would be runRule(ruleUID, context, considerConditions). Each new signature adds a new argument to the end of the list of arguments. Placing the considerConditions as the second argument violates that expectation.

It's not about technical stuff. It's about making decisions that allow users who are not as familiar with the source code to correctly guess what is correct.

Of course, if you add the newly proposed method there is no more pattern and the order doesn't matter as much.

RuleManager ruleManager = ScriptServiceUtil.getRuleManager();
if (ruleManager.getStatus(ruleUID) == null) {
throw new IllegalArgumentException("Rule with UID '" + ruleUID + "' doesn't exist");
}
return ruleManager.runNow(ruleUID, considerConditions, context);
}

/**
* Check whether the specified rule is enabled.
*
* @param ruleUID the UID of the rule to check.
* @return {@code true} if the rule is enabled, {@code false} otherwise.
* @throws IllegalArgumentException If a rule with the specified UID doesn't exist.
*/
@ActionDoc(text = "check whether the specified rule rule is enabled")
public static boolean isRuleEnabled(String ruleUID) {
RuleManager ruleManager = ScriptServiceUtil.getRuleManager();
Boolean result = ruleManager.isEnabled(ruleUID);
if (result == null) {
throw new IllegalArgumentException("Rule with UID '" + ruleUID + "' doesn't exist");
}
return result.booleanValue();
}

/**
* Set whether the specified rule is enabled.
*
* @param ruleUID the UID of the rule to enable or disable.
* @param enabled {@code true} to enable the rule, {@code false} to disable the rule.
* @throws IllegalArgumentException If a rule with the specified UID doesn't exist.
*/
@ActionDoc(text = "set whether the specified rule is enabled")
public static void setRuleEnabled(String ruleUID, boolean enabled) {
ScriptServiceUtil.getRuleManager().setEnabled(ruleUID, enabled);
}

/**
* @return The {@link ThingRegistry}.
*/
@ActionDoc(text = "get the thing registry")
public static ThingRegistry getThingRegistry() {
return ScriptServiceUtil.getThingRegistry();
}

/**
* @return The {@link MetadataRegistry}.
*/
@ActionDoc(text = "get the metadata registry")
public static MetadataRegistry getMetadataRegistry() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even once you gain access to the MetadataRegistry, creating and adding or modifying is a pain because it requires creating a bunch of custom classes. Maybe not for this PR but eventually adding some helper methods like are here for rules would be beneficial.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could add more "helper tools" if somebody just give me a hint of what to make (a reference to something else that does something similar)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an example, JS provides a metadata on the class it uses to expose the ItemRegistry which lets one access an Items metadata. See https://openhab.github.io/openhab-js/items.metadata.html.

In addition it provides access to those methods on the Item itself. The methods include:

  • addMetadata(itemOrName, namespace, value, configurationopt, persistopt)
  • getMetadata(itemOrName, namespaceopt)
  • removeMetadata(itemOrName, namespaceopt)
  • replaceMetadata(itemOrName, namespace, value, configurationopt)

itemOrName can be an instance of an Item or the name of an Item. value is a String; empty String is allowed and I think null becomes the empty String. namespace is a String of course. configurationopt is an optional Object though I'd expect it to be a Map in Rules DSL.

persistopt is a new field I'm not familiar with.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those methods are quite similar to those I've already created. I could certainly add variants that accepts Item instead of "itemName", if that's easier to provide (if you already have the Item, I guess it's easy to just pass that on).

When in comes to persistopt, that is related to "stuff created by a JS script" and not really relevant here. There's a separate metadata provider that doesn't persist, so that the metadata only exists until the file is unloaded. So, I'm guessing that this option determines whether to store them there or in the "managed metadata provider".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that they have called one method replaceMetadata when the underlying method is called updateMetadata. They are the exact same thing, but I prefer to stick to what's underneath, so that it's easier for people to see how it all ties together. I went with "update" instead of "replace".

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. I could certainly add variants that accepts Item instead of "itemName"

I think that is not as useful in DSL as it is in JS.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already done that, but it's done as "extensions", so it becomes:

  • item.addMetadata(namespace, value [, context etc.])
  • item.getMetadata(namespace)
  • etc.

Given that Items are magically available in DSL, this should make it very easy to manipulate Item metadata.

return ScriptServiceUtil.getMetadataRegistry();
}

/**
* @return The {@link ItemRegistry}.
*/
@ActionDoc(text = "get the item registry")
public static ItemRegistry getItemRegistry() {
return ScriptServiceUtil.getItemRegistry();
}

/**
* @return The {@link RuleRegistry}.
*/
@ActionDoc(text = "get the rule registry")
public static RuleRegistry getRuleRegistry() {
return ScriptServiceUtil.getRuleRegistry();
}

/**
* @return The {@link RuleManager}.
*/
@ActionDoc(text = "get the rule manager")
public static RuleManager getRuleManager() {
return ScriptServiceUtil.getRuleManager();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.openhab.core.model.script.actions.Log;
import org.openhab.core.model.script.actions.Ping;
import org.openhab.core.model.script.actions.ScriptExecution;
import org.openhab.core.model.script.actions.System;
import org.openhab.core.model.script.actions.Transformation;
import org.openhab.core.model.script.engine.IActionServiceProvider;
import org.openhab.core.model.script.engine.IThingActionsProvider;
Expand Down Expand Up @@ -76,6 +77,7 @@ protected List<Class<?>> getExtensionClasses() {
result.add(Ping.class);
result.add(Transformation.class);
result.add(ScriptExecution.class);
result.add(System.class);
result.add(URLEncoder.class);

result.addAll(getActionClasses());
Expand All @@ -92,6 +94,7 @@ protected List<Class<?>> getStaticImportClasses() {
result.add(Ping.class);
result.add(Transformation.class);
result.add(ScriptExecution.class);
result.add(System.class);
result.add(URLEncoder.class);
result.add(CoreUtil.class);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.openhab.core.library.unit.SIUnits;
import org.openhab.core.library.unit.Units;
import org.openhab.core.model.script.ScriptServiceUtil;
import org.openhab.core.service.StartLevelService;
import org.openhab.core.test.java.JavaOSGiTest;
import org.openhab.core.types.State;

Expand All @@ -68,6 +69,7 @@ public class ScriptEngineOSGiTest extends JavaOSGiTest {
private @NonNullByDefault({}) ItemRegistry itemRegistry;
private @NonNullByDefault({}) ScriptEngine scriptEngine;
private @Mock @NonNullByDefault({}) UnitProvider unitProviderMock;
private @Mock @NonNullByDefault({}) StartLevelService startLevelService;

@BeforeEach
public void setup() {
Expand All @@ -81,6 +83,9 @@ public void setup() {

registerService(eventPublisher);

when(startLevelService.getStartLevel()).thenReturn(40);
registerService(startLevelService, StartLevelService.class.getName());

itemRegistry = getService(ItemRegistry.class);
assertNotNull(itemRegistry);

Expand Down
Loading