Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
*/
package org.openhab.core.model.rule.runtime.internal;

import static org.openhab.core.model.core.ModelCoreConstants.isIsolatedModel;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Iterator;
Expand Down Expand Up @@ -76,6 +78,7 @@
* No rule conditions are used as this concept does not exist for DSL rules.
*
* @author Kai Kreuzer - Initial contribution
* @author Laurent Garnier - Add optional rule UID + rules stored in a map per model
*/
@NonNullByDefault
@Component(immediate = true, service = { DSLRuleProvider.class, RuleProvider.class, DSLScriptContextProvider.class })
Expand All @@ -86,7 +89,7 @@ public class DSLRuleProvider

private final Logger logger = LoggerFactory.getLogger(DSLRuleProvider.class);
private final Collection<ProviderChangeListener<Rule>> listeners = new ArrayList<>();
private final Map<String, Rule> rules = new ConcurrentHashMap<>();
private final Map<String, Collection<Rule>> rulesMap = new ConcurrentHashMap<>();
private final Map<String, IEvaluationContext> contexts = new ConcurrentHashMap<>();
private final Map<String, XExpression> xExpressions = new ConcurrentHashMap<>();
private final ReadyMarker marker = new ReadyMarker("rules", "dslprovider");
Expand All @@ -110,7 +113,7 @@ protected void activate() {
@Deactivate
protected void deactivate() {
modelRepository.removeModelRepositoryChangeListener(this);
rules.clear();
rulesMap.clear();
contexts.clear();
xExpressions.clear();
}
Expand All @@ -122,7 +125,9 @@ public void addProviderChangeListener(ProviderChangeListener<Rule> listener) {

@Override
public Collection<Rule> getAll() {
return rules.values();
// Ignore isolated models
return rulesMap.keySet().stream().filter(name -> !isIsolatedModel(name))
.map(name -> rulesMap.getOrDefault(name, List.of())).flatMap(list -> list.stream()).toList();
}

@Override
Expand All @@ -134,69 +139,62 @@ public void removeProviderChangeListener(ProviderChangeListener<Rule> listener)
public void modelChanged(String modelFileName, EventType type) {
String ruleModelType = modelFileName.substring(modelFileName.lastIndexOf(".") + 1);
if ("rules".equalsIgnoreCase(ruleModelType)) {
String ruleModelName = modelFileName.substring(0, modelFileName.lastIndexOf("."));
List<ModelRulePair> modelRules = new ArrayList<>();
switch (type) {
case MODIFIED:
removeRuleModel(modelFileName, false);
case ADDED:
String ruleModelName = modelFileName.substring(0, modelFileName.lastIndexOf("."));
List<Rule> rules = new ArrayList<>();
List<ModelRulePair> modelRules = new ArrayList<>();
EObject model = modelRepository.getModel(modelFileName);
if (model instanceof RuleModel ruleModel) {
int index = 1;
for (org.openhab.core.model.rule.rules.Rule rule : ruleModel.getRules()) {
Rule newRule = toRule(ruleModelName, rule, index);
rules.put(newRule.getUID(), newRule);
rules.add(newRule);
xExpressions.put(ruleModelName + "-" + index, rule.getScript());
modelRules.add(new ModelRulePair(newRule, null));
index++;
}
rulesMap.put(modelFileName, rules);
handleVarDeclarations(ruleModelName, ruleModel);
}
break;
case MODIFIED:
removeRuleModel(ruleModelName);
EObject modifiedModel = modelRepository.getModel(modelFileName);
if (modifiedModel instanceof RuleModel ruleModel) {
int index = 1;
for (org.openhab.core.model.rule.rules.Rule rule : ruleModel.getRules()) {
Rule newRule = toRule(ruleModelName, rule, index);
Rule oldRule = rules.remove(ruleModelName);
rules.put(newRule.getUID(), newRule);
xExpressions.put(ruleModelName + "-" + index, rule.getScript());
modelRules.add(new ModelRulePair(newRule, oldRule));
index++;
if (!isIsolatedModel(modelFileName)) {
notifyProviderChangeListeners(modelRules);
}
handleVarDeclarations(ruleModelName, ruleModel);
}
break;
case REMOVED:
removeRuleModel(ruleModelName);
removeRuleModel(modelFileName, false);
break;
default:
logger.debug("Unknown event type.");
}
notifyProviderChangeListeners(modelRules);
} else if ("script".equals(ruleModelType)) {
List<ModelRulePair> modelRules = new ArrayList<>();
switch (type) {
case MODIFIED:
case ADDED:
List<ModelRulePair> modelRules = new ArrayList<>();
EObject model = modelRepository.getModel(modelFileName);
if (model instanceof Script script) {
Rule oldRule = rules.remove(modelFileName);
Collection<Rule> oldRules = rulesMap.getOrDefault(modelFileName, List.of());
Rule oldRule = null;
if (oldRules.size() == 1) {
oldRule = oldRules.iterator().next();
}
Rule newRule = toRule(modelFileName, script);
rules.put(newRule.getUID(), newRule);
rulesMap.put(modelFileName, List.of(newRule));
modelRules.add(new ModelRulePair(newRule, oldRule));
if (!isIsolatedModel(modelFileName)) {
notifyProviderChangeListeners(modelRules);
}
}
break;
case REMOVED:
Rule oldRule = rules.remove(modelFileName);
if (oldRule != null) {
listeners.forEach(listener -> listener.removed(this, oldRule));
}
removeRuleModel(modelFileName, true);
break;
default:
logger.debug("Unknown event type.");
}
notifyProviderChangeListeners(modelRules);
}
}

Expand All @@ -215,21 +213,24 @@ private void handleVarDeclarations(String modelName, RuleModel ruleModel) {
contexts.put(modelName, context);
}

private void removeRuleModel(String modelName) {
Iterator<Entry<String, Rule>> it = rules.entrySet().iterator();
while (it.hasNext()) {
Entry<String, Rule> entry = it.next();
if (belongsToModel(entry.getKey(), modelName)) {
listeners.forEach(listener -> listener.removed(this, entry.getValue()));
private void removeRuleModel(String modelFileName, boolean script) {
if (!isIsolatedModel(modelFileName)) {
rulesMap.getOrDefault(modelFileName, List.of()).forEach(r -> {
listeners.forEach(listener -> listener.removed(this, r));
});
}
rulesMap.remove(modelFileName);

it.remove();
}
if (script) {
return;
}
Iterator<Entry<String, XExpression>> it2 = xExpressions.entrySet().iterator();
while (it2.hasNext()) {
Entry<String, XExpression> entry = it2.next();

String modelName = modelFileName.substring(0, modelFileName.lastIndexOf("."));
Iterator<Entry<String, XExpression>> it = xExpressions.entrySet().iterator();
while (it.hasNext()) {
Entry<String, XExpression> entry = it.next();
if (belongsToModel(entry.getKey(), modelName)) {
it2.remove();
it.remove();
}
}
contexts.remove(modelName);
Expand Down Expand Up @@ -258,7 +259,10 @@ private Rule toRule(String modelName, Script script) {

private Rule toRule(String modelName, org.openhab.core.model.rule.rules.Rule rule, int index) {
String name = rule.getName();
String uid = modelName + "-" + index;
String uid = rule.getUid();
if (uid == null) {
uid = modelName + "-" + index;
Comment on lines +335 to +337
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

User-provided rule UIDs from the DSL are accepted verbatim. If a UID contains '/', it will be embedded into event topics like openhab/rules/{ruleID}/..., which breaks RuleEventFactory#getRuleId (expects exactly 4 topic segments) and can cause event parsing failures. Consider validating rule.getUid() (e.g., reject or fall back to the auto-generated UID when it contains '/' or other topic-delimiter characters) and log a clear warning when the configured UID is invalid.

Suggested change
String uid = rule.getUid();
if (uid == null || uid.isBlank()) {
uid = modelName + "-" + index;
String autoUid = modelName + "-" + index;
String uid = rule.getUid();
if (uid == null || uid.isBlank()) {
uid = autoUid;
} else if (uid.indexOf('/') >= 0) {
logger.warn(
"Ignoring invalid UID '{}' for rule '{}' in model '{}': character '/' is not allowed in rule UIDs. Using auto-generated UID '{}'.",
uid, name, modelName, autoUid);
uid = autoUid;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is exactly the kind of thing I was thinking of when I referred to problematic characters that we don't yet know about. I'd say that it "proves" that #5467 is necessary, and that / must be excluded (this is assuming that copilot's claim is true, but I think I've seen code doing things like that).

}
Comment on lines 334 to +338
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

User-provided rule UIDs from the DSL are accepted verbatim (only blank/null falls back to auto-generated). Because rule UIDs are embedded directly into event topics (e.g., openhab/rules/{ruleID}/...), a UID containing '/' will produce an invalid topic that RuleEventFactory cannot parse (it expects exactly 4 topic segments). Add validation/sanitization for configured UIDs (at minimum reject '/' and fall back to the auto-generated UID, ideally with a warning log) to prevent runtime event failures.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is a separate PR, #5467, that deals with this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

User-provided rule UIDs from the DSL are accepted verbatim (only blank/null falls back to auto-generated). Because rule UIDs are embedded directly into event topics (e.g., openhab/rules/{ruleID}/...), a UID containing '/' will produce an invalid topic that RuleEventFactory cannot parse (it expects exactly 4 topic segments). Add validation/sanitization for configured UIDs (at minimum reject '/' and fall back to the auto-generated UID, ideally with a warning log) to prevent runtime event failures.

I think this comment from copilot is the best reason to reject my wild idea. It does correctly point out the UID should be validated more.

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.

@Nadahar : maybe it is good to exclude the / in that PR, even if there is a more general discussion in parallel in another PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@lolodomo I have already modified #5467 so that both / and \ are excluded in all "suggested patterns". I just wished we could agree on something and move that PR forward.


// Create Triggers
triggerId = 0;
Expand Down Expand Up @@ -431,19 +435,24 @@ private String removeIndentation(String script) {
public void onReadyMarkerAdded(ReadyMarker readyMarker) {
for (String ruleFileName : modelRepository.getAllModelNamesOfType("rules")) {
EObject model = modelRepository.getModel(ruleFileName);
String ruleModelName = ruleFileName.substring(0, ruleFileName.indexOf("."));
if (model instanceof RuleModel ruleModel) {
String ruleModelName = ruleFileName.substring(0, ruleFileName.indexOf("."));
int index = 1;
List<Rule> rules = new ArrayList<>();
List<ModelRulePair> modelRules = new ArrayList<>();
for (org.openhab.core.model.rule.rules.Rule rule : ruleModel.getRules()) {
Rule newRule = toRule(ruleModelName, rule, index);
rules.add(newRule);
xExpressions.put(ruleModelName + "-" + index, rule.getScript());
modelRules.add(new ModelRulePair(newRule, null));
index++;
}
rulesMap.put(ruleFileName, rules);
handleVarDeclarations(ruleModelName, ruleModel);

notifyProviderChangeListeners(modelRules);
if (!isIsolatedModel(ruleFileName)) {
notifyProviderChangeListeners(modelRules);
}
}
}
modelRepository.addModelRepositoryChangeListener(this);
Expand All @@ -454,11 +463,8 @@ private void notifyProviderChangeListeners(List<ModelRulePair> modelRules) {
modelRules.forEach(rulePair -> {
Rule oldRule = rulePair.oldRule();
if (oldRule != null) {
rules.remove(oldRule.getUID());
rules.put(rulePair.newRule().getUID(), rulePair.newRule());
listeners.forEach(listener -> listener.updated(this, oldRule, rulePair.newRule()));
} else {
rules.put(rulePair.newRule().getUID(), rulePair.newRule());
listeners.forEach(listener -> listener.added(this, rulePair.newRule()));
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ VariableDeclaration:
;

Rule:
'rule' name=(STRING|ID) ('[' tags+=(ID|STRING) (',' tags+=(ID|STRING))* ']')?
'rule' name=(STRING|ID) (uid=ID)? ('[' tags+=(ID|STRING) (',' tags+=(ID|STRING))* ']')?
'when' eventtrigger+=EventTrigger ('or' eventtrigger+=EventTrigger)*
'then' script=XExpressionInClosure
'end'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public void testSimpleRules() {
Collection<Rule> rules = dslRuleProvider.getAll();
assertThat(rules.size(), is(0));

String model = "rule RuleNumberOne [ Test, \"my test\" ]\n" + //
String model = "rule RuleNumberOne rule1 [ Test, \"my test\" ]\n" + //
"when\n" + //
" System started\n" + //
"then\n" + //
Expand All @@ -117,7 +117,7 @@ public void testSimpleRules() {

Rule firstRule = it.next();

assertThat(firstRule.getUID(), is("dslruletest-1"));
assertThat(firstRule.getUID(), is("rule1"));
assertThat(firstRule.getName(), is("RuleNumberOne"));
assertThat(firstRule.getTags(), hasSize(2));
assertThat(firstRule.getTags(), hasItem("Test"));
Expand Down
Loading