DSL Items Parser: Fix incorrect parsing of keywords encountered in the wrong context

[jimtng]

When specifying tag names that match one of the valid item types, e.g. Switch, the parser incorrectly treated it as a new start of an item definition.

Fix #4927

Consider backporting to 4.3.x This is a bigger change than I originally thought, it shouldn't be backported

Longer explanation:

The standard way xtext parser / grammar deals with this is to prefix conflicting literals such as Switch, AND, Group, etc with a caret ^. So even without this PR, this would work:

Switch Test1 [^Switch]

This PR makes this possible:

Switch Test1 [Switch]

See Copilot review for more details.

Also "fixed": removed space in the generated DSL item format, from:

Group:Switch:OR (ON,OFF) Name

to

Group:Switch:OR(ON,OFF) Name // remove space before (

Copilot Summary

Pull Request Overview

This PR fixes incorrect parsing of DSL items when keywords matching valid item types (like "Switch", "Group", etc.) are used in other contexts such as tag names. Previously, the parser would incorrectly treat these as new item definitions.

• Simplifies the grammar by removing separate ModelGroupItem and ModelNormalItem types in favor of a unified ModelItem approach
• Updates validation logic to properly validate item types, group functions, and dimensions with improved error messages
• Refactors code generation to remove unnecessary space in group function syntax (e.g., Group:Switch:OR(ON,OFF) instead of Group:Switch:OR (ON,OFF))

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
bundles/org.openhab.core.model.item/src/org/openhab/core/model/Items.xtext	Unified grammar to support flexible item type parsing with up to 4 colon-separated segments
bundles/org.openhab.core.model.item/src/org/openhab/core/model/item/internal/GenericItemProvider.java	Major refactor to handle unified item creation logic with improved group item parsing
bundles/org.openhab.core.model.item/src/org/openhab/core/model/validation/ItemsValidator.xtend	Enhanced validation with comprehensive item type, group function, and dimension checking
bundles/org.openhab.core/src/main/java/org/openhab/core/items/GroupFunction.java	Added constants for valid group functions to centralize function validation
bundles/org.openhab.core/src/main/java/org/openhab/core/library/CoreItemFactory.java	Exposed valid item types as a public constant for validation use
itests/org.openhab.core.model.item.tests/src/main/java/org/openhab/core/model/item/internal/GenericItemProviderTest.java	Added comprehensive tests for keyword parsing edge cases and group type syntax

[andrewfg]

@jimtng many thanks for picking this up. It seems that the tests, although running, are now failing, but you are obviously on the right track. I have no knowledge of xtext so can’t really help any more with the details..

[jimtng]

This passed now. However I've updated the original post with some further explanation

[andrewfg]

@jimtng I think the ^ prefix is a horrible hack, and IMHO not a solution. The parser should break on new lines, and if the word Switch is at the start of the line it should be interpreted as an item type; and if anywhere else it should be interpreted as a string (label or tag name). I think a simple regex could easily identify the two cases.

[lolodomo]

Note that if this PR is not merged, the code generating DSL format for item has to be fixed to add the character `^ when expected when generating tags.
https://github.com/openhab/openhab-core/blob/main/bundles/org.openhab.core.model.item/src/org/openhab/core/model/item/internal/fileconverter/DslItemFileConverter.java#L146

[andrewfg]

^ and we would probably need for safety sake to fix all item getTag(s) and setTag(s) methods to eliminate any stray carets. As I say, I think it is a horrible hack, and I strongly argue to solve this properly. We all know the proper syntax since years, and we should not allow a compromise in order to avoid a fix.

[andrewfg]

If the current parser really cannot distinguish between Switch at the start of a line and Switch anywhere else, the we should implement a regex pre-parser to do that in an inter- phase between loading the file and passing the stream to the existing parser.

[lolodomo]

But when the user writes ^Switch, I guess that the XText parser automatically removes the ^. So the resulting tag is Switch as we expect.

[andrewfg]

Maybe. Who knows. My argument is to make the code accept the syntax that every user knows since OH v1.0 and not introduce some queer kludge to avoid fixing a bug in the code that will confuse everybody.

[lolodomo]

Apparently noone was annoyed by this "bug" since several years ;)

I just checked how I myself solved this problem in my own file. I did not use ^ character at all but all my tags are between double quotes like for example: [ "Switch" ]

IMHO, as string are accepted in addition to ID by the parser, it is fine and there is nothing to fix. If the user wants to use a tag that is a special keyword, he just has to surround it with double-quotes.

[lolodomo]

But @jimtng proposal is also an improvement as it allows a syntax that was not possible before, so why not.
I see that more as an enhancement than as a bug fix.

[andrewfg]

Can either of you explain why the parser is not capable to detect the difference between Switch at the beginning of a line and Switch elsewhere? It cannot be so difficult.

[lolodomo]

I also checked for an item what is the generated code from Main UI and everything is OK, this is not a string that is used but the escape character:

Switch bypassed "ignorée" <switch> [^Switch] { channel="xxxxxx" }

So nothing to change/fix in the DSL generator.

[lolodomo]

I would suggest to not start a big change with risk while this is something that everyone using .items already knows how to handle. Maybe something could be added in our documentation to help someone who could encounter that error for the first time.

[andrewfg]

this is something that everyone using .items already knows how to handle

I don't agree.

[jimtng]

For tags, including generating DSL syntax, I'd suggest just using quoted strings instead of caret or bare.

I tried finding a syntax or solution but haven't found one that's simple/straight forward.

[jimtng]

I also checked for an item what is the generated code from Main UI and everything is OK, this is not a string that is used but the escape character:

Switch bypassed "ignorée" <switch> [^Switch] { channel="xxxxxx" }

So nothing to change/fix in the DSL generator.

indeed, that's good to know!

[jimtng]

I tried creating an item in the UI:

• Name: Switch
• Tag: Switch

That worked and generated the correct DSL: String ^Switch "Switch" [^Switch] -> DSL Parser is happy to load it

Then I set the icon to Switch, DSL generated this: String ^Switch "Switch" <Switch> [^Switch] -> DSL Parser is not happy.
It needs to be manually adjusted to String ^Switch1 "Switch" <^Switch> [^Switch] to make DSL parser happy

[jimtng]

Marking this PR as Draft. I'm not happy with the current solution.

[lolodomo]

@jimtng : Kai just merged PR #4793 (and that is a very good news for our pending stuff in UI) and that means you will have to adjust your PR carefully.
I will check it again after you solve the conflicts.

[lolodomo]

Build failed, you need to apply spotless in /org/openhab/core/model/item

[kaikreuzer]

Thanks!

[openhab-bot]

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

https://community.openhab.org/t/rules-dsl-in-5-1-is-now-unloading-rules-if-it-has-unreachable-expressions/168382/2