null is not an accepted type so convert it to an accepted type #3521

martinvw · 2017-05-25T17:49:32Z

null is not an accepted type so convert it to an accepted type

Overcomes NPE's in a places where the state is broadcasted to

martinvw

Some context

martinvw · 2017-05-25T17:50:25Z

...core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/items/ItemUtil.java

 if (item != null && !isAccepted(item, state)) {
 for (Class<? extends State> acceptedType : item.getAcceptedDataTypes()) {
 State convertedState = state.as(acceptedType);
 if (convertedState != null) {
 LoggerFactory.getLogger(ItemUtil.class).debug("Converting {} '{}' to {} '{}' for item '{}'",
- state.getClass().getSimpleName(), state.toString(),
- convertedState.getClass().getSimpleName(), convertedState.toString(), item.getName());
+ state.getClass().getSimpleName(), state, convertedState.getClass().getSimpleName(),


You should not call toString on logging arguments, just pass them on as an object they will be converted by logging framework if the log level applies

martinvw · 2017-05-25T17:52:04Z

...core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/items/ItemUtil.java

+ if (state == null) {
+ return UnDefType.NULL;
+ }
+
 if (item != null && !isAccepted(item, state)) {
 for (Class<? extends State> acceptedType : item.getAcceptedDataTypes()) {
 State convertedState = state.as(acceptedType);
 if (convertedState != null) {
 LoggerFactory.getLogger(ItemUtil.class).debug("Converting {} '{}' to {} '{}' for item '{}'",


imho this is a real strange solution to overcome a static logger. Now you are going to re-fetch the logger from the factory each time you pass by this debug (!!!) statement which is really often for this specific line. The coding guidelines leave room to have static loggers if required.

@kaikreuzer what is your view on this specific solution

static logger would definitely be ok for me here

martinvw · 2017-05-25T17:52:19Z

...core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/items/ItemUtil.java

@@ -75,14 +75,18 @@ public static void assertValidItemName(String itemName) throws IllegalArgumentEx
 }
 }

- public static State convertToAcceptedState(final State state, final Item item) {
+ public static State convertToAcceptedState(State state, Item item) {
+ if (state == null) {


This is the core of the actual change :-)

I wonder how null actually gets in here. IMHO we should fix the root cause of it first. Still, it's valid to make it fail-safe here.

@martinvw I think we should not silently swallow this situation, but log an error (incl. stacktrace) nonetheless to highlight that there is some bug somewhere in the code as we should never get here.

I added a log statement

martinvw · 2017-05-25T17:52:33Z

...core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/items/ItemUtil.java

- if (itemName != null && !itemName.isEmpty()) {
- return itemName.matches("[a-zA-Z0-9_]*");
- }
+ return StringUtils.isNotEmpty(itemName) && itemName.matches("[a-zA-Z0-9_]*");


Sorry could not leave it like that :-)

Btw, do we think that itemName of 0 characters is valid....?

👍 and no, AFAIK it needs to be a least one character long

and no, AFAIK it needs to be a least one character long

@kaikreuzer agree? Should it be a separate PR?

Certainly agree. Yes, to be clean, you can do it in a separate PR.

I just see that we just did a isNotEmpty check so only the regex looked misleading but nothing will change :-D

martinvw · 2017-05-25T17:56:04Z

@kaikreuzer imho this should solve the problem which was mentioned in the issue #3520 how were you able to reproduce it?

And on the community forum: https://community.openhab.org/t/java-lang-nullpointerexception-every-few-seconds/28944

sjsf · 2017-05-26T09:46:57Z

The main question remains: What is the root-cause? Items should initially have a UnDefType.NULL state, so something recently must have changed which breaks with this assumption. Any clues?

sjsf · 2017-05-26T09:49:26Z

GroupItems without a function maybe? Then it could have been #3411

kaikreuzer · 2017-05-26T09:50:28Z

I think this is a likely a correct assumption...

martinvw · 2017-05-26T09:56:24Z

If I follow up the call-stack, I arrive here:
ItemUtil.convertToAcceptedState
called from
ThingManager line 128 thingHandlerCallback.stateUpdated
called from
BaseThingHandler.updateState

Why can't it be a binding which just call's:

updateState(channelUID, null);

The main question remains: What is the root-cause? Items should initially have a UnDefType.NULL state, so something recently must have changed which breaks with this assumption. Any clues?

Items do initially have the state UnDefType.NULL but if we accept the state null, at one moment in time the oldState will become null as well, or did I misunderstand you?

sjsf · 2017-05-26T10:09:44Z

Why can't it be a binding which just call's:
updateState(channelUID, null);

Because then (as bad as it is...) it would have failed with an NPE in
ItemUtil line 94, if I'm not mistaken.

martinvw · 2017-05-26T10:17:09Z

Well spotted, yes I agree on that, so my change most likely doesn't fix the issue

martinvw · 2017-05-26T10:19:34Z

I have my sister visiting, but I'll add changes later to fix setting of the state of e group.

kaikreuzer · 2017-05-26T11:21:23Z

...core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/items/ItemUtil.java

- if (itemName != null && !itemName.isEmpty()) {
- return itemName.matches("[a-zA-Z0-9_]*");
- }
+ return StringUtils.isNotEmpty(itemName) && itemName.matches("[a-zA-Z0-9_]*");



hey, you missed to remove this empty line :-)

martinvw · 2017-05-26T11:24:52Z

I don't see it coming from the mentioned PR either (no call to setState which might contain null), what do you think about this one:
GroupItem.java#L329

martinvw · 2017-05-26T11:27:33Z

I don't see it coming from the mentioned PR either (no call to setState which might contain null), what do you think about this one: GroupItem.java#L329

Then I'm most likely missing something because my branch contains no function which can return null either. Can you point out the line where you think its actually assigning null to the oldState?

Maybe equality can, but then all items must have state null :-S

martinvw · 2017-05-26T12:24:24Z

Another likely suspect, #3438

https://github.com/eclipse/smarthome/blob/master/bundles/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/types/State.java#L29

IMHO we should also add logging for this case, because it means there is a conversion requested which is not possible. For example requesting conversion of an OpenClosedType to a HSBType.

martinvw · 2017-05-26T18:52:07Z

@triller-telekom can you take a look at NPE in combination with the change and line to which I referred, do you think this might be the cause of this? Thanks!

kaikreuzer · 2017-05-26T19:26:10Z

You are right, the issue is definitely in 6939315#diff-543ecd419447346e6a177db8638e3ad0R74 - the as() method is allowed to return null and if you feed in an UNDEF at that place, the state will (now) be set to null internally. This worked in the past as UNDEF wasn't a Convertable state and thus was set directly.

martinvw · 2017-05-26T19:39:20Z

So do you mean an UNDEF is converted to null, should we change the default implementation in that case?

Or should UnDefType override that method with return this;

Fixes #3520 Signed-off-by: Martin van Wingerden <martinvw@mtin.nl>

kaikreuzer · 2017-05-26T20:12:38Z

Not sure how we best get back the behavior we had with the "Convertibles" - I'd leave it to @SJKA & @triller-telekom to decide as they both worked on that.

kaikreuzer

thanks - so let's merge this as a first fix, but have @triller-telekom come up with a follow-up PR that fixes the underlying issue.

martinvw commented May 25, 2017

View reviewed changes

kaikreuzer reviewed May 26, 2017

View reviewed changes

null is not an accepted type so convert it to an accepted type

40dc87d

Fixes #3520 Signed-off-by: Martin van Wingerden <martinvw@mtin.nl>

kaikreuzer approved these changes May 26, 2017

View reviewed changes

martinvw mentioned this pull request May 26, 2017

[avmfritz] Fixed NPE if temperature, powermeter or switch model is null openhab/openhab-addons#1887

Merged

maggu2810 approved these changes May 27, 2017

View reviewed changes

kaikreuzer merged commit cd69eba into eclipse-archived:master May 27, 2017

martinvw deleted the feature/3520-convert-null-before-broadcasting-it branch May 28, 2017 07:14

kaikreuzer modified the milestone: 0.9.0 Jun 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

null is not an accepted type so convert it to an accepted type #3521

null is not an accepted type so convert it to an accepted type #3521

martinvw commented May 25, 2017

martinvw left a comment

martinvw May 25, 2017

sjsf May 26, 2017

martinvw May 25, 2017

martinvw May 26, 2017

kaikreuzer May 26, 2017

martinvw May 25, 2017

sjsf May 26, 2017

kaikreuzer May 26, 2017

martinvw May 26, 2017

martinvw May 25, 2017

martinvw May 25, 2017

sjsf May 26, 2017

martinvw May 26, 2017

kaikreuzer May 26, 2017

martinvw May 26, 2017

martinvw commented May 25, 2017

sjsf commented May 26, 2017

sjsf commented May 26, 2017

kaikreuzer commented May 26, 2017

martinvw commented May 26, 2017 •

edited

Loading

sjsf commented May 26, 2017

martinvw commented May 26, 2017

martinvw commented May 26, 2017

kaikreuzer May 26, 2017

martinvw commented May 26, 2017

martinvw commented May 26, 2017 •

edited

Loading

martinvw commented May 26, 2017 •

edited

Loading

martinvw commented May 26, 2017

kaikreuzer commented May 26, 2017

martinvw commented May 26, 2017

kaikreuzer commented May 26, 2017

kaikreuzer left a comment

null is not an accepted type so convert it to an accepted type #3521

null is not an accepted type so convert it to an accepted type #3521

Conversation

martinvw commented May 25, 2017

martinvw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvw commented May 25, 2017

sjsf commented May 26, 2017

sjsf commented May 26, 2017

kaikreuzer commented May 26, 2017

martinvw commented May 26, 2017 • edited Loading

sjsf commented May 26, 2017

martinvw commented May 26, 2017

martinvw commented May 26, 2017

Choose a reason for hiding this comment

martinvw commented May 26, 2017

martinvw commented May 26, 2017 • edited Loading

martinvw commented May 26, 2017 • edited Loading

martinvw commented May 26, 2017

kaikreuzer commented May 26, 2017

martinvw commented May 26, 2017

kaikreuzer commented May 26, 2017

kaikreuzer left a comment

Choose a reason for hiding this comment

martinvw commented May 26, 2017 •

edited

Loading

martinvw commented May 26, 2017 •

edited

Loading

martinvw commented May 26, 2017 •

edited

Loading