Fix syntax for time in trigger/condition in DSL rule by lolodomo · Pull Request #5591 · openhab/openhab-core

lolodomo · 2026-05-23T05:32:37Z

Avoid conflict with switch-case statement

Time as "7:0" is now also supported.

Avoid conflict with switch-case statement Time as "7:0" is now also supported. Fix openhab#5586 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2026-05-23T05:35:48Z

With that rule:

import java.time.Duration

rule "Test rule" uid = "test" [ Test, "my test" ]
when
  Time is 9:00 or
  Time is midnight or
  Time is noon or
  Item DemoSwitch changed from OFF to ON or
  System started
but only if
  Time is between 7:0 and 22:30 and
  Day is weekend and
  Day offset 7 is not holiday and
  1000 ms between executions
then
  val duration = Duration.between(now, now.plusMinutes(1)).toSeconds()
  logInfo("Rule","duration=" + duration)
  val value = 10
  var String message
  switch (value) {
    case 9: {
      message = "Case value 9"
    }
    case 10: {
      message = "Case value 10"
    }
  }
  logInfo("Rule","message=" + message)
end

the rule is loaded and the result is:

07:32:01.075 [INFO ] [org.openhab.core.model.script.Rule  ] - duration=60
07:32:01.079 [INFO ] [org.openhab.core.model.script.Rule  ] - message=Case value 10

And the rule looks like that in Main UI:

openhab-bot · 2026-05-23T07:03:43Z

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

https://community.openhab.org/t/dsl-rule-load-failure-on-latest-snapshot/169331/14

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

mherwege · 2026-05-23T08:57:03Z

@Nadahar Any concerns left with this solution?

Let's wait to see if Copilot has anything. For me LGTM.

Copilot

Pull request overview

This PR updates the openHAB DSL Rules grammar and runtime handling of Time literals so that time expressions in triggers/conditions no longer conflict with the switch/case syntax in rule scripts, and it broadens accepted time formats (e.g., allowing 7:0).

Changes:

Replaces the lexer-level TIME terminal with a parser-level Time rule (INT ':' INT) to prevent tokenization conflicts with case 10:.
Adds validation for TimerTrigger and TimeOfDayCondition times (hour/minute range checking) and classifies invalid times as load-blocking model errors.
Normalizes non-zero-padded times in DSLRuleProvider to HH:mm when mapping to automation module configuration.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
bundles/org.openhab.core.model.rule/src/org/openhab/core/model/rule/validation/RulesValidator.java	Adds validation checks for trigger/condition time values and improves line-referenced diagnostics.
bundles/org.openhab.core.model.rule/src/org/openhab/core/model/rule/Rules.xtext	Changes time syntax handling to avoid lexer conflicts with `switch/case` constructs.
bundles/org.openhab.core.model.rule.runtime/src/org/openhab/core/model/rule/runtime/internal/DSLRuleProvider.java	Normalizes parsed time strings to `HH:mm` before configuring time-based triggers/conditions.
bundles/org.openhab.core.model.core/src/main/java/org/openhab/core/model/core/internal/ModelRepositoryImpl.java	Treats rule validation diagnostics with issue code `time` as fatal (like `uid`) so invalid times prevent model load.

Comments suppressed due to low confidence (1)

bundles/org.openhab.core.model.rule/src/org/openhab/core/model/rule/validation/RulesValidator.java:95

Similarly to the trigger time check, the condition time validation messages don’t state what format/range is required. Consider expanding these messages so users immediately know the expected syntax and valid hour/minute bounds.

        if (!isValidTime(start)) {
            error(buildMsgWithLineNb("start time '" + start + "' in condition is invalid", timeOfDayCondition,
                    RulesPackage.Literals.TIME_OF_DAY_CONDITION__START),
                    RulesPackage.Literals.TIME_OF_DAY_CONDITION
                            .getEStructuralFeature(RulesPackage.TIME_OF_DAY_CONDITION__START),
                    "time");
        }
        if (!isValidTime(end)) {
            error(buildMsgWithLineNb("end time '" + end + "' in condition is invalid", timeOfDayCondition,
                    RulesPackage.Literals.TIME_OF_DAY_CONDITION__END),
                    RulesPackage.Literals.TIME_OF_DAY_CONDITION
                            .getEStructuralFeature(RulesPackage.TIME_OF_DAY_CONDITION__END),
                    "time");
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mherwege · 2026-05-23T09:24:23Z

I don't see anything major in Copilot's feedback.

lolodomo · 2026-05-23T10:03:40Z

I don't see anything major in Copilot's feedback.

I will make the adjustments.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

Nadahar · 2026-05-23T11:37:21Z

+        String[] splittedTime = time.split(":", 2);
+        int hour = Integer.parseInt(splittedTime[0]);
+        int minute = Integer.parseInt(splittedTime[1]);
+        return "%02d:%02d".formatted(hour, minute);


Suggested change

return "%02d:%02d".formatted(hour, minute);

return String.format(Locale.ROOT, "%02d:%02d", hour == 24 ? 0 : hour, minute);

.formatted() is not locale-safe, and should not be used. Use String.format(Locale.ROOT, ... ) instead. Locale must probably be imported as well (unless it's in use somewhere else in the class).

I am surprised because this method is used in many places in our code base.
In our context, what problem do you expect?

I know that the method is used a lot, and I don't understand why. I also don't understand why the checks I made in the SAT plugin doesn't generate warnings for it, I must have missed the signature in some way. But, it should not be used at all, because it uses the JVM default locale, which is completely unpredictable and can change at any time.

I doubt there is much consequence here, since I'm guessing that integers (%d) is formatted the same in any locale, but to me, using this "unsafe method" is a "red flag" to begin with. Internally it is identical to String.format() that has been around for ages, except that it doesn't include an overload where you can specify the locale. It was added as syntactic sugar in Java 15 for reasons I don't understand, but since they omitted the locale, it just shouldn't be used.

If I ever get around to figure out why it doesn't trigger a SAT warning, it will, so not using it will avoid a future warning.

I doubt there is much consequence here, since I'm guessing that integers (%d) is formatted the same in any locale

Yes so the change is not necessary

But why not do it correctly anyway? It's no big deal no, but it's still a "bug" in principle. There's no way to know what formatting rules locales have or will have in the future, and when you don't know what locale you use to format, you don't know the result.

Nadahar · 2026-05-23T11:42:48Z

+     */
+    private String formatTime(String time) {
+        String[] splittedTime = time.split(":", 2);
+        int hour = Integer.parseInt(splittedTime[0]);


Wouldn't it be better to check the length of the array after splitting, and then throwing IllegalArgumentException or something if the length is wrong (too few or too many :s). Like the code is now, it will throw ArrayOutOfBoundsException if the array is too short.

I did not do it because we know that the input is valid when we run this code. If not correct, the loading of rule fails and this code is not reached.

Nadahar · 2026-05-23T11:43:56Z

+    private String formatTime(String time) {
+        String[] splittedTime = time.split(":", 2);
+        int hour = Integer.parseInt(splittedTime[0]);
+        int minute = Integer.parseInt(splittedTime[1]);


Similarly here - wouldn't it be better to catch the NumberFormatException and rethrow as e.g. IllegalArgumentException?

I did not do it because we know that the input is valid when we run this code. If not correct, the loading of rule fails and this code is not reached.

Ok - I don't like to make such assumptions, what if the validation code is changed in the future, and nobody is aware that this code relies on it for protection? It would be very "cheap" to add the "protection" here as well, and avoid the potential future "problem".

That said, it will throw some RuntimeException in any case - so this is only a "concern" if we think IllegalArgumentException is somehow "better"/more consistent than the others.

Nadahar · 2026-05-23T12:00:46Z

+        try {
+            int hour = Integer.parseInt(splittedTime[0]);
+            int minute = Integer.parseInt(splittedTime[1]);
+            return hour >= 0 && hour <= 23 && minute >= 0 && minute <= 59;


Suggested change

return hour >= 0 && hour <= 23 && minute >= 0 && minute <= 59;

return hour >= 0 && hour <= 24 && minute >= 0 && minute <= 59;

I checked, and as I thought, 24 is a permitted value and means the same as 0, so why not handle it in case somebody choose to use it? It should be "translated" to 0 where the zero-padding takes place.

You mean 24:55 should be accepted?!

In some cultures / countries "24 hour” is used. But I think it is over-engineering here. Users have to stick to 0..23 hours and that’s it. If somebody can write DSL Rules, that person should also understand what “0 hour” means.

You mean 24:55 should be accepted?!

Yes, I believe that it's written that way some places. I've also made it "automatically translate" to 00 in the provider in my suggestion. It costs so little extra to do things like this, and affected users just gets an easier life.

Adding 24 adds confusion in my opinion.
I should mention in the error message that range is 0 to 24 which is not clear at all.
I believe it is better to remain in range 00:00 - 23:59.
@mherwege : WDYT?

Agree. Stick with 23.

I've never understood this sentiment, that it's better to enforce "what you yourself is used to" than to be flexible and allow people to use what is familiar to them. But. It's in no way a big deal here, such flexibility would need to be system-wide to have much value.

Still, the resistance to "tolerance itself" has always been something I don't understand, if I can make it work better for more people with minimal cost, I just do it.

Nadahar · 2026-05-23T12:12:57Z

Any concerns left with this solution?

Not that I'm aware of. My testing showed that most of the things I was concerned with didn't seem to be a problem. I've left a few minor comments as I'm always "concerned" about code not being robust enough, and in particular "inaccuracies" involving timezones, charsets and locales (don't quite know why, it just irks me because their "misuse" is so widespread).

mherwege

LGTM

Nadahar · 2026-05-23T19:31:24Z

I assume that this means that I can remove the "fix" I made for the same issue in #5572: f2c0a5d ?

Fix syntax for time in trigger/condition in DSL rule

9bb9d4b

Avoid conflict with switch-case statement Time as "7:0" is now also supported. Fix openhab#5586 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo requested a review from a team as a code owner May 23, 2026 05:32

dilyanpalauzov reviewed May 23, 2026

View reviewed changes

Comment thread ...s/org.openhab.core.model.rule/src/org/openhab/core/model/rule/validation/RulesValidator.java Outdated

Optimize method buildMsgWithLineNb

442e2d6

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

mherwege requested a review from Copilot May 23, 2026 08:53

Copilot started reviewing on behalf of mherwege May 23, 2026 08:54 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

Consider Copilot review comments

8827460

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

mherwege requested changes May 23, 2026

View reviewed changes

mherwege added the bug An unexpected problem or unintended behavior of the Core label May 23, 2026

mherwege added this to the 5.2 milestone May 23, 2026

Consider human comments

f6b1752

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo force-pushed the fix_rule_TIME branch from 41e72b5 to f6b1752 Compare May 23, 2026 10:45

Nadahar reviewed May 23, 2026

View reviewed changes

mherwege approved these changes May 23, 2026

View reviewed changes

mherwege merged commit 2169fe0 into openhab:main May 23, 2026
5 checks passed

lolodomo deleted the fix_rule_TIME branch May 23, 2026 19:34

	return "%02d:%02d".formatted(hour, minute);
	return String.format(Locale.ROOT, "%02d:%02d", hour == 24 ? 0 : hour, minute);

	return hour >= 0 && hour <= 23 && minute >= 0 && minute <= 59;
	return hour >= 0 && hour <= 24 && minute >= 0 && minute <= 59;

Uh oh!

Conversation

lolodomo commented May 23, 2026

Uh oh!

lolodomo commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openhab-bot commented May 23, 2026

Uh oh!

Uh oh!

mherwege commented May 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mherwege commented May 23, 2026

Uh oh!

lolodomo commented May 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nadahar May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolodomo May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolodomo May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolodomo May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nadahar May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nadahar commented May 23, 2026

Uh oh!

mherwege left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Nadahar commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

lolodomo commented May 23, 2026 •

edited

Loading

Nadahar May 23, 2026 •

edited

Loading

lolodomo May 23, 2026 •

edited

Loading

lolodomo May 23, 2026 •

edited

Loading

lolodomo May 23, 2026 •

edited

Loading

Nadahar May 23, 2026 •

edited

Loading

Nadahar commented May 23, 2026 •

edited

Loading