Nullness fixes

[maggu2810]

The commits are separated, so rebase without squash should be okay.

[kaikreuzer]

Travis does not seem to like the changes to ItemRegistryDelegate...

[kaikreuzer]

Don't we still have the issue that Rule.getUID() is still allowed to return null and thus is @nullable?

[maggu2810]

Hm, you are right, the current concept allows rules without an UID.
But does it make sense to state that we have an interface Identifiable that provides an UID and allow an implementing class to return null on that method (so "I am itentifiable but don't give you an UID)?
Shouldn't we throw an IllegalStateException on the Rule.getUID() method if there is no UID assigned? As soon as someone wants to call getUID because it relies that the reference is an Identifiable it should also provide an UID. If the Rule has no UID assigned, the method should not be called at all (the usage of that reference should know about the fact that there is no UID yet assigned and should not call that method).

[kaikreuzer]

Note that I brought this topic up here already:

I think we have introduced a bug when adding the Identifiable interface: I would claim that Identifiable.getUID() should return a non-null value. Adding such an annotation gets us into trouble with the Rule class, though as we allow the creation of rules without any UID (which is added only by the rule registry upon add). I think we should have a closer look at how this can best be resolved - for the moment, I didn't add the annotation on getUID().

Throwing an IllegalStateException might be a solution, but @danchom should be involved in that refactoring.

[maggu2810]

Great, I identified that I replied to your comment 😉

[danchom]

Shouldn't we throw an IllegalStateException on the Rule.getUID() method if there is no UID assigned?

For me no, because it will be hard for users to know for already used strings which are used by the RuleEngine as ruleUIDs. At the moment the RuleEngine grantee unique ids for added rules.
Actuality the Rule is Identifiable after the object is added to RuleRegistry. Before that it is just a POJO object which carries rule's information and there is no guarantee that it is a valid rule object.

[adimova]

@kaikreuzer, @maggu2810 we need to agree on a decision about the @NonNull UID and @NonNullByDefault for the getUID() method in the Identifiable interface. The possible solutions are:

1. Null to be possible for UID - this is the current situation when Null is a valid UID.

• Advantage: no changes :-)
• Disadvantage: all "not added rules" are equal

2. System UID that will be recognized and replaced by the Registry after adding the Rule - this is similar to the current situation, but without default constructors, and if the constructors are called with Null, the system UID (constant) will be used instead.

• Advantage: @NonNullByDefault can be used for the getUID() method.
• Disadvantage: all "not added rules" are equal

3. Add a generateUID() method in Registry. It can be added in:
   a) Rule Registry, or
   b) Abstract Registry

• Advantage: @NonNullByDefault can be used for the getUID() method.
• Disadvantage: The IDs generated by the generateUID() will be unique at the time of generation. When you try to add the Rule in the Registry, the ID may have already been taken for another rule, and adding will fail. In the add method, the Registry does not know if the rule UID was generated by the generateUID() method, or set explicitly by the user.

4. The user must provide a correct UID for the rule

• Advantage: the Rule registry acts as all other Registries.
• Disadvantage: we make it more difficult for the user

All resolutions have negatives and we should discuss which is the most effective one.

[maggu2810]

Didn't we already agree that the getUID() of an Identifiable should return an identifiable ID and so not null already?

About your point 3: Couldn't this (the UID generation) be make clever that a collision is such a rare case that it will "never" be happen (see e.g. https://en.wikipedia.org/wiki/Universally_unique_identifier#Collisions)?

[adimova]

Didn't we already agree that the getUID() of an Identifiable should return an identifiable ID and so not null already?

@maggu2810 I agree with this, but I can't get this decision alone, I should convince our architects. I am actually asking by their request.

About your point 3: Couldn't this (the UID generation) be make clever that a collision is such a rare case that it will "never" be happen?

It is not about "when the same UUID is generated more than once", it is about that the same UUID could be generated form more than one generators, each of them with its own way to generate a UUID for its rules:

• scripting
• file provider
• bundle provider
• by the user it self
• third party app

I am actually voting for point 4 but let me say again I can't get this decision by my self

[kaikreuzer]

I agree that option 4 is probably the best. This would at least also make it consistent with how we deal with it for Things, where a UID is also generated (by the UI) if a thing is manually created.

To reduce the impact, we could imho simply generate a java.util.UUID in the Rule constructor, if no UID is passed from the client. This way we would ensure to always have a UID while all client code can stay as it is. Note that the ScriptedRuleSupport already uses with UUIDs, so it would be in sync with this part as well. The only downside would be that the UIDs are not "nice" compared to the current "rule_x", but as they are an internal identifier not really meant for any end user, this is imho acceptable.

[adimova]

See #4438 to confirm is it what we agreed here

[maggu2810]

Travis does not seem to like the changes to ItemRegistryDelegate...

Crazy 😉 Will have a look at.

[maggu2810]

Travis does not seem to like the changes to ItemRegistryDelegate...

Crazy 😉 Will have a look at.

Ah, it is about the missing external annotations.
The key and the value of an entry set is @Nullable or @NonNull dependent on the type that is used for the key and the value (the entry set only contains entries of a map that contains only types that are allowed in the map). As long as there is not the correct external annotation also an entry set could return nullables also if it has been declared that there are no null entries (key or value).
I will remove that commit and move it to my other branch.

[htreu]

Imho it really is a pity that we have to "work around" the annotations now just because the compiler is not able to recognise StringUtils.isBlank as an implicit null check. I just had a similar situation in a builder pattern (see https://github.com/eclipse/smarthome/pull/4301/files#diff-8f975ae88094089585192875908ba08bR79) which I could solve by doing some API changes. In the end it turned out these changes where a turn for the better.

Just to clarify: I thought @NonNullByDefault should guarantee the call to brokerConnectionConfig.get(NAME_PROPERTY) to always return a non null value? So why is the null check (or StringUtils.isBlank) required at all?

[maggu2810]

Imho it really is a pity that we have to "work around" the annotations now just because the compiler is not able to recognise StringUtils.isBlank as an implicit null check.

You are right, the Eclipse Nullness Checker should be improved and the EEAs.
The Checker Framework can handle such situations already.
There I could use astub files (similar to EEA) and state that if the method returns true, the reference used in the argument is non-null.

class StringUtils {
    static @EnsuresNonNull("#1") boolean isBlank(@Nullable String str);
}

[htreu]

another question about the non null behaviour of maps: With @NonNullByDefault we force the compiler to interpret a call to brokerConnectionConfig.get("url") to always return a non null value. How can this be guaranteed at all? I get the impression that any call to a map is interpreted as non null value, making additional null checks obsolete. Thats a dangerous behaviour:

Object value = myMap.get("key");
if (value == null) {
   throw ....
}

in this example the body of the if clause is reported as dead code. I guess for Maps the default should be Map<@NoNull KEY_TYPE, @Nullable VALUE_TYPE>.

wdyt?

[maggu2810]

another question about the non null behaviour of maps: With @NonNullByDefault we force the compiler to interpret a call to brokerConnectionConfig.get("url") to always return a non null value.

No, you define the default for your classes or packages but not for the map itself.
A map's get method could always return null e.g. your ask for a value of a non-existing key.

I guess for Maps the default should be Map<@NoNull KEY_TYPE, @Nullable VALUE_TYPE>.

No, this is not correct. If you look at the Map API null is allowed for the key if the implementation allows null keys (stated in the JavaDoc).

Also @NonNull K is not correct at all, because of the upper / lower bounds of generic types. In the generic type declaration you need to use K extends @NonNull Object.

The checker framework has a good documentation about that. I realized it a few days ago.

[maggu2810]

This PR contains three commits and it seems at least the Identifiable blocks the other ones.
Let's clean it up.

• fix nullness of MQTT service
  => could be done on the go in MqttService: Remove all deprecated methods, rename close to stop. #4173
• Identifiable should make use of '@NonNullByDefault' …
  => we are waiting for a PR of @adimova first Nullness fixes #4290 (comment)
• fix nullness annotations for automation commands utils …
  => moved to fix nullness annotations for automation commands utils #4319

[maggu2810]

Rebased