Smarthome: Question: Missing @NonNull in Thing.getThingTypeUID()

[martinvw]

As discussed with @triller-telekom https://github.com/openhab/openhab2-addons/pull/2556#issuecomment-323672397

Because of the call to thing.getThingTypeUID() we do require a null check but I would suppose a thing always has an ThingTypeUID what do you think

I think there is an annotation missing in the Thing class because a thing should always have a UID.

@triller-telekom and me think @NonNull annotation is missing on getThingTypeUID do you agree?

[sjsf]

No, to my understanding Thing Types are optional - i.e. they can (and most of the time will) be used as prototypes to describe a thing, but could be omitted completely when describing Things manually.

[martinvw]

Then our archetype is wrong and should be throwing NPE's as well:

https://github.com/eclipse/smarthome/blob/master/tools/archetype/binding/src/main/resources/archetype-resources/src/main/java/internal/__bindingIdCamelCase__HandlerFactory.java#L46

@maggu2810 @kaikreuzer wdyt?

[sjsf]

Independent of that, it would be the better (in the sense of defensive) style anyway to turn it around, i.e.

        if (THING_TYPE_SAMPLE.equals(thingTypeUID)) {
            return new ${bindingIdCamelCase}Handler(thing);
        }

[maggu2810]

I don't know who is currently using things without a type, but it could be possible (if this is an officially supported feature).
So, the change @sjka presents would be a good one.

[martinvw]

@maggu2810 I agree that the change of @sjka is a good one and should be done but my question started out from the following warning and code snippet:

[WARNING] D:\Java\eclipse\openhab\git\openhab2-addons\addons\binding\org.openhab.binding.rfxcom\src\main\java\org\openhab\binding\rfxcom\internal\RFXComHandlerFactory.java:[67] 
    } else if (supportsThingType(thingTypeUID)) {
                                 ^^^^^^^^^^^^
Null type safety (type annotations): The expression of type 'ThingTypeUID' needs unchecked conversion to conform to '@NonNull ThingTypeUID'

Fragment:

@Override
protected ThingHandler createHandler(Thing thing) {

   ThingTypeUID thingTypeUID = thing.getThingTypeUID();

   if (RFXComBindingConstants.SUPPORTED_BRIDGE_THING_TYPES_UIDS.contains(thingTypeUID)) {
       RFXComBridgeHandler handler = new RFXComBridgeHandler((Bridge) thing);
       registerDeviceDiscoveryService(handler);
       return handler;
   } else if (supportsThingType(thingTypeUID)) {
       return new RFXComHandler(thing);
   }

   return null;
}

https://github.com/openhab/openhab2-addons/blob/master/addons/binding/org.openhab.binding.rfxcom/src/main/java/org/openhab/binding/rfxcom/internal/RFXComHandlerFactory.java

We are not allowed to pass null to supportsThingType but because of the call to thing.getThingTypeUID() we do require a null check. I supposed a thing always has an ThingTypeUID. Since this really common code I think that its a pity to put this burden on all bindings.

[maggu2810]

If

• getThingType is allowed to return null (leave it up to other ones if it is an official feature)
• supportsThingType is not allowed to receive null (makes sense, why should someone ask if null is a supported thing type)
• your binding is not able to create an handler for a non set thing type

I assume you should return null yourself if thing type UID is null.

@Override
protected ThingHandler createHandler(Thing thing) {

   ThingTypeUID thingTypeUID = thing.getThingTypeUID();
   if (thingTypeUID == null) {
       // We don't support things without a specific thing type UID
       return null;
   }

   if (RFXComBindingConstants.SUPPORTED_BRIDGE_THING_TYPES_UIDS.contains(thingTypeUID)) {
       RFXComBridgeHandler handler = new RFXComBridgeHandler((Bridge) thing);
       registerDeviceDiscoveryService(handler);
       return handler;
   } else if (supportsThingType(thingTypeUID)) {
       return new RFXComHandler(thing);
   }

   return null;
}

[martinvw]

So my main question remains:

If

• getThingType is allowed to return null (leave it up to other ones if it is an official feature)

@kaikreuzer wdyt?

[kaikreuzer]

@sjka Is right that it used to be an official feature of the originally designed architecture. Nonetheless, in practise this has never been used/supported by any binding and as such it got out of sight.
I'd be ok to give up on that feature and say that we always require a thing type as I see that anything else will rather cause bugs in many situations and I do not see a use case now that would justify any efforts in this direction.

[martinvw]

Just to be sure, a PR could exist of adding the annotation and making sure that no other errors happen because of the added annotation?

[kaikreuzer]

Give it a try ;-)

[kaikreuzer]

With the new guidelines that @triller-telekom created, I am afraid that the solution to this issue now looks pretty different - feel free to come up with a new PR that updates the Thing interface to use @NonNullByDefault.