Generated DSL syntax for Things file invalid

[jlaur]

When discovering this Thing:

clicking it and selecting "Copy DSL Definition", the following output is generated:

Thing danfossairunit:airunit:"-1062731542" [host="ccm.local"]

Putting this into a .things file will result in the following error:

2025-03-17 21:09:46.503 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'danfoss2.things'
2025-03-17 21:09:46.508 [WARN ] [el.core.internal.ModelRepositoryImpl] - Configuration model 'danfoss2.things' has errors, therefore ignoring it: [1,30]: missing RULE_ID at '"-1062731542"'

After manually removing the quotes:

Thing danfossairunit:airunit:-1062731542 [host="ccm.local"]

this is the result:

2025-03-17 21:11:44.748 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'danfoss2.things'
2025-03-17 21:11:44.752 [WARN ] [el.core.internal.ModelRepositoryImpl] - Configuration model 'danfoss2.things' has errors, therefore ignoring it: [1,30]: extraneous input '-' expecting RULE_ID

It looks like DSL doesn't accept this syntax, although it does appear to be a valid ThingUID:

openhab-core/bundles/org.openhab.core/src/main/java/org/openhab/core/common/AbstractUID.java

Line 30 in 2eec58a

private static final Pattern SEGMENT_PATTERN = Pattern.compile("[\\w-]*");

openhab-core/bundles/org.openhab.core/src/main/java/org/openhab/core/common/AbstractUID.java

Lines 99 to 109 in 2eec58a

	public static boolean isValid(@Nullable String segment) {
	return segment != null && SEGMENT_PATTERN.matcher(segment).matches();
	}
	protected void validateSegment(String segment, int index, int length) {
	if (!isValid(segment)) {
	throw new IllegalArgumentException(String.format(
	"ID segment '%s' contains invalid characters. Each segment of the ID must match the pattern %s.",
	segment, SEGMENT_PATTERN));
	}
	}

So either the UID validation is not strict enough, or the DSL validation is too strict?

Related to #4569, but also has UI impact here where none of the buttons can be clicked:

Expected Behavior

Either validateSegment should throw:

openhab-core/bundles/org.openhab.core/src/main/java/org/openhab/core/common/AbstractUID.java

Lines 99 to 109 in 2eec58a

	public static boolean isValid(@Nullable String segment) {
	return segment != null && SEGMENT_PATTERN.matcher(segment).matches();
	}
	protected void validateSegment(String segment, int index, int length) {
	if (!isValid(segment)) {
	throw new IllegalArgumentException(String.format(
	"ID segment '%s' contains invalid characters. Each segment of the ID must match the pattern %s.",
	segment, SEGMENT_PATTERN));
	}
	}

Or it should be possible to provide this ThingUID in a .things file.

Current Behavior

This constructor:

openhab-core/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/ThingUID.java

Lines 40 to 48 in 2eec58a

	/**
	* Instantiates a new thing UID.
	*
	* @param thingTypeUID the thing type
	* @param id the id
	*/
	public ThingUID(ThingTypeUID thingTypeUID, String id) {
	super(thingTypeUID.getBindingId(), thingTypeUID.getId(), id);
	}

accepts:

new ThingTypeUID("danfossairunit", "airunit"), "-123"

Possible Solution

TBD.

Steps to Reproduce (for Bugs)

See above.

In 4.3 the UI allowed to add this ThingUID as a managed Thing:

Also it was accepted when manually creating a Thing:

whereas in 5.0:

This is related to openhab/openhab-webui#3059

Context

I'm working on the Danfoss Air Unit binding, and found this old oddness:

https://github.com/openhab/openhab-addons/blob/379b244b1f73bf7dde89990e2a3b9f00265fb04b/bundles/org.openhab.binding.danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/discovery/DanfossAirUnitDiscoveryService.java#L142

I can easily fix this in case the outcome of this issue is that the ThingUID validation is made more strict.

Your Environment

• Version used: 5.0 snapshot 4547

[jlaur]

FYI @lolodomo, @jimtng

[jimtng]

IMO, the core SEGMENT_PATTERN needs to be updated to prevent this. However, doing so would break existing installations that already had Things with it. Best we could do is using upgradetool + breaking notice. However, since DSL hadn't allowed it, it shouldn't affect anyone with file-based config.

[jimtng]

@florian-h05 FYI

[mherwege]

I believe I am to blame for this. I based it on what is supported in DSL.

However, looking further back, this PR #1517 did change it for DSL. I do think it is dangerous to try to change it back in DSL though. And for consistency, it is probably better to not allow - at the start of the ID. It is also not allowed for item names.

[lolodomo]

Regarding the quotes around the segment id when generating DSL, this is something strange I discovered and I applied a post fix to remove them. But my "replace"uses a regex compliant with an expected segment id in DSL and that means something without a leading -.
I could change my regex but it will not help as it will generate an invalid DSL

I could add a check of segment IDs and log a warning if it starts with minus when generating DSL
It introduced a case DSL generation could fail because Main UI accepted a less restrictive syntax.
I was not expecting that to happen.

[lolodomo]

I could log a warning and remove the leading - from the id to be compliant with DSL syntax.

[jlaur]

IMO, the core SEGMENT_PATTERN needs to be updated to prevent this.

I agree. Otherwise we have inconsistency between managed and file-based Things. And worse, we now also have inconsistency between existing managed Things having such naming and the new stricter UI logic.

However, doing so would break existing installations that already had Things with it. Best we could do is using upgradetool + breaking notice. However, since DSL hadn't allowed it, it shouldn't affect anyone with file-based config.

Correct. Yeah, upgradetool would be a good idea, since otherwise I assume that jsondb loading will fail entirely when trying to deserialize into ThingUID? Since the upgrade would have to rename ThingUIDs it could break rules referencing them, so I agree the breaking notice is also important.

Additionally we would need to ensure that no bindings will try to create such invalid ThingUIDs, since it would throw RuntimeExceptions which in most cases will be unhandled. I have only seen it for the Danfoss Air binding, for which I already have a fix prepared in openhab/openhab-addons#18414. I can help looking for similar cases across the codebase.

And for consistency, it is probably better to not allow - at the start of the ID. It is also not allowed for item names.

I agree, it would be the cleanest solution.

I could log a warning and remove the leading - from the id to be compliant with DSL syntax.

If the ThingUID validation is hardened, I guess this would not be relevant?

[lolodomo]

I could log a warning and remove the leading - from the id to be compliant with DSL syntax.

If the ThingUID validation is hardened, I guess this would not be relevant?

Exactly