Yaml/DSL Thing provider didn't check for existence of a Thing before adding one

[jimtng]

The Yaml Thing provider doesn't check if a Thing with the same ID already exists in the Thing Registry (i.e. registered by another provider) before adding one to its "provider".

Steps:

• Create a Thing in Main UI, e.g. astro:moon:test1
• Create a yaml file and create the same UID (thus duplicating it)

version: 2
things:
  astro:moon:test1:
    config:
      geolocation: "1,2"

Save this file. This is the step where the problem occurred. It "thinks" that it had registered this UID when in fact it failed (or did create a duplicate uid?)

• Edit the yaml file, and change the uid to astro:moon:test2
  At this point it tried to remove the previous "test1" but failed with the following error

You'll see

Provider 'YamlThingProvider' is not allowed to remove element 'ThingImpl' with key 'astro:moon:test1' from the registry because it was added by provider 'ManagedThingProvider'.

It should've logged a warning and refuse to add an existing thing id in the first place.

Originally posted by @jimtng in #4793 (comment)

[jimtng]

@lolodomo, continuing here:

have to check the checks in thing registry but I agree it would be better to have a message when first adding a thing that already existed through the UI managed provider.
It is the load of the updated YAML file that triggers the thing removal in the registry.

Is it different when you use DSL thing ? I assume it is not.

I just tried, the DSL suffered from the same problem. So I guess this isn't a specific issue to the Yaml Thing provider.

[lolodomo]

Can you tell us if it is different when you use a DSL configuration file instead of a YAML file?

[lolodomo]

So I guess this isn't a specific issue to the Yaml Thing provider.

Exactly so please adjust your issue title

This is like that probably since more than 10 years ;)

[lolodomo]

The problem is not at thing provider level but at thing registry level.

[lolodomo]

Each provider provides its things to the registry independtly of each other. This is the thing registry which then handles data provided by all providers.

So you can have things with same UID provided by the UI provider, the DSL provider and now the YAML provider. The thing registry will keep only one and should log warnings.

[lolodomo]

The log is generated by the abstract registry so for any registry:

openhab-core/bundles/org.openhab.core/src/main/java/org/openhab/core/common/registry/AbstractRegistry.java

Line 252 in e9700f8

logger.error(

This log makes sense.

There is also a log when adding something that already exists but it is at DEBUG level:

openhab-core/bundles/org.openhab.core/src/main/java/org/openhab/core/common/registry/AbstractRegistry.java

Line 195 in e9700f8

logger.debug(

So maybe we should increase level of this one (and check that the provider is different) and reduce level of the previous?

[jimtng]

if there are multiple Things, won't there be multiple instances of ThingHandler (created by the binding) for the same UID? I think we need to prevent that from happening

[lolodomo]

There will be a thing handler only for things in the thing registry and you can't have duplicates in the registry.

[jimtng]

There will be a thing handler only for things in the thing registry and you can't have duplicates in the registry.

that's good. So the only problem is to warn users that their Thing in Yaml / DSL (or any other sources for that matter) isn't applied / loaded. Right now they wouldn't know that a duplicate exists during load time.

[lolodomo]

Right now they wouldn't know that a duplicate exists during load time.

Yes because only DEBUG log.

[lolodomo]

I removed my previous message as I made a mismatch between thing handler and thing handler factory..
The providers don't instantiate thing handlers, they just create things that can be finally added to the registry or ignored by the registry if the thing with same UID already exist in the registry.