[OH 4] Channel ID contains invalid characters

[rkoshak]

This is related to OH 4 SNAPSHOT build #3292.

After upgrading to the latest snapshot today I'm getting errors on the Zigbee binding.

  │2023-01-24 09:41:46.714 [ERROR] [ng.zigbee.handler.ZigBeeThingHandler] - 282C02BFFFE726CD: Exception creating channels                                                  │
  │java.lang.IllegalArgumentException: UID segment 'zigbee:device:zg_coordinator:282c02bfffe726cd:282C02BFFFE726CD_1_batterylevel' contains invalid characters. The last se│
  │gment of the channel UID must match the pattern '[\w-]*|[\w-]*#[\w-]*'.                                                                                                 │
  │        at org.openhab.core.thing.ChannelUID.validateSegment(ChannelUID.java:136) ~[bundleFile:?]                                                                       │
  │        at org.openhab.core.common.AbstractUID.<init>(AbstractUID.java:76) ~[bundleFile:?]                                                                              │
  │        at org.openhab.core.thing.UID.<init>(UID.java:66) ~[bundleFile:?]                                                                                               │
  │        at org.openhab.core.thing.ChannelUID.<init>(ChannelUID.java:59) ~[bundleFile:?]                                                                                 │
  │        at org.openhab.core.thing.internal.ThingImpl.getChannel(ThingImpl.java:125) ~[?:?]                                                                              │
  │        at org.openhab.binding.zigbee.handler.ZigBeeThingHandler.doNodeInitialisation(ZigBeeThingHandler.java:377) [bundleFile:?]                                       │
  │        at org.openhab.binding.zigbee.handler.ZigBeeThingHandler$1.call(ZigBeeThingHandler.java:244) [bundleFile:?]                                                     │
  │        at org.openhab.binding.zigbee.handler.ZigBeeThingHandler$1.call(ZigBeeThingHandler.java:1) [bundleFile:?]                                                       │
  │        at java.util.concurrent.FutureTask.run(FutureTask.java:264) [?:?]                                                                                               │  Ca│        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) [?:?]                                         │  00│        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) [?:?]                                                                        │3120│        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [?:?]                                                                        │
20│        at java.lang.Thread.run(Thread.java:833) [?:?]  

Something recently changed which appears to now be enforcing that regex in the Channel IDs. In this particular case it doesn't like _ in the channel ID.

Looking at the code where the error is generated there hasn't been any meaningful changes to the code in years. At the same time, these UIDs created by the Zigbee binding have also been working in my OH for years without complaint.

My initial thought was to go file an issue on the binding, but it occurs to me that forcing the recreation of a bunch of Things to upgrade to OH 4 (I do not know whether the Zigbee binding is the only one impacted) is going to be a pretty big job to impose on the end users. So I wanted to open an issue here to float the idea of changing the regex to also allow underscore in addition to dash.

Unless there is some side effect or concern I'm unaware of this seems to be the smallest impact way to address the problem across the board.

[cdjackson]

Just to note that underscores have always been allowed in the past. The zwave thing IDs all use userscores since the database creates the thing id based on manufacturer, name, version - separated with underscores - eg actec_zerodim2pol_00_000. This would be a massive impact if it was no longer supported so changing the core to allow this, as suggested here by Rich, seems the best option.

Also to note that the xsd uses the following -:

<xs:simpleType name="idRestrictionPattern">
<xs:restriction base="xs:string">
<xs:pattern value="[A-Za-z0-9\-_]+"/>
</xs:restriction>
</xs:simpleType>

[rkoshak]

I'm exploring more with @mhilbush on the forum. The error might be misleading or something else may be going on. We both have other Things with Channel UIDs that include underscores that are working just fine.

I also went in and manually edited my JSONDB files to change the underscores to dashes for my Zigbee Things and I'm still getting the error.

Has there been a recent change in the Zigbee binding that is now calling ChannelUID.validateSegment() where it wasn't being called before? Also, the ID generated in the error appears to be generated from a UID that is dynamically constructed in the binding, not read from the JSONDB. To play around this I manually edited the JSONDB to change the _ to - for my Zigbee Thing UIDs. The string 1_voltage no longer exists anywhere in my JSONDB files but the error persists with

UID segment 'zigbee:device:zg_coordinator:000d6f00053ac32d:000D6F00053AC32D_1_voltage' contains invalid characters.

Something merged in the past week or so? Given the behavior I'm not convinced any more that the error being reported is the actual root error.

But I agree, the regex should be fixed in ChannelUID to match and IMO the XSD and code should be using the exact same regex. The XSD is actually more constrained than what's in the ChannelUID except for the inclusion of \ (when would one want to use \ I wonder) and _.

Hmmm, I wonder if the error is that the full Channel UID is being passed to the validator but the validator is only expecting the last segment? The name implies it wouldn't want the full UID and it's actually chocking on the :.

[openhab-bot]

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

https://community.openhab.org/t/openhab-4-0-snapshot-discussion/142322/108

[cdjackson]

There haven't been any significant changes to the zigbee binding for some time so I don't think changes to the binding should be causing this.

https://github.com/openhab/org.openhab.binding.zigbee/pulls?q=is%3Apr+is%3Aclosed

[mhilbush]

Well, there is this change. But I'm not immediately seeing how it could result in the error you're getting.

1c34321

[mhilbush]

But getChannel is in your stack trace, and that was definitely changed as part of that commit.

[mhilbush]

I guess we should wait for @J-N-K to peel this back further...

[J-N-K]

Well, there is indeed a change. Before null was returned when the string representation of the UID (i.e. binding:type:thing:channelId was passed to getChannel(String channelId) because the comparison of channel.getUID().getId() with that string always false. Now a ChannelUID is created from the ThingUID and the passed string. This fails because the full UID is not a valid channelId-part.

We can log a warning and return null instead of throwing an exception, but to return a useful value (i.e. the requested channel) the code in the binding needs to be adjusted (either bei only passing the id-part or even better using the ChannelUID directly instead of calling .toString on it).

[rkoshak]

This fails because the full UID is not a valid channelId-part.

But even if the valid channelId-part were passed it would still fail the check because of the _.

So as I see it there are three problems here.

1. It sounds like binding needs to be updated to use ChannelID correctly.
2. The REGEX should include _ in ChannelID. Tons of bindings include _ in the Channel ID and the regex would fail any time a binding uses this validate function even though, until now, they've been allowed. And even if they've not been allowed, it will be a huge pain for both add-on developers and end users to go back and fix where as adding that one character to the REGEX can solve it for everyone. Or if the REGEX already does allow it, the exception message needs to be changed to show the actual REGEX used.
3. The REGEX in the XSD should match the REGEX of the ChannelID since it's the XSD. The XSD is apparently what the add-on developer use as guidance for what's allowed or not so the two should match.

[J-N-K]

The regex is fine. \w includes a-zA-Z_0-9 and we add -.

See https://docs.oracle.com/javase/tutorial/essential/regex/pre_char_classes.html

[rkoshak]

That's what I get for going by memory. So we have two problems, 1 and 3.

[J-N-K]

The XSD has the same characters. The backslash is needed because - needs to be escaped if it is not the last character.

[rkoshak]

Even if they are logically the same it seems weird to define it two different ways in the two places. My concern is not that they are not equivalent but that the representations are different (though not realizing that the \ was an escape shows how little sleep I've had the past few days).

Given they define the allowed format for this same ID field it seems inconsistent and a little troubling to represent the REGEX two different ways, even if they are equivalent.

But if the developers don't care, this isn't something that end users have to see and mentally work around so it's no big deal I guess.

So do I need to file an issue on the add-on and close this one?

[J-N-K]

Not a core isse.