Rollershutter potential nil error on operations

[boc-tothefuture]

@pacive - I was looking at the rollershutter code and it is different than the others in that I don't think it handles the case where the rollershutter state is UNDEF or NULL and I don't see a test for either of those conditions.

state can return nil (and does for UNDEF or NULL) so all of your checks should be done with the safe navigation operator for up?/down? and position.

[pacive]

That is probably because I didn't create a wrapper for the type, just the item. For the up?/down?/position methods this can be handled with the safe navigation operator, but what would be the best way to handle <=> if state is nil? Should it raise an exception? Seems like we should avoid raising errors into user-space as much as possible.

Edit: Isn't this a problem in NumberItem as well? E.g in the numeric_compare method:

def numeric_compare(other)
  @number_item.state.to_big_decimal.to_d <=> other.to_d
end

[boc-tothefuture]

Edit: Isn't this a problem in NumberItem as well? E.g in the numeric_compare method:

def numeric_compare(other)
  @number_item.state.to_big_decimal.to_d <=> other.to_d
end

Yes, yes it is.

[boc-tothefuture]

It should return nil on <=> with no state I would think.

[pacive]

Hmm, it seems the methods in Comparable raises an ArgumentError when <=> returns nil, which is correct since it means the comparison failed. The question is where this should be caught?

[jimtng]

I was converting a rule into jruby. Excerpt below:

gBatteries.select { |item| item < LOW_BATTERY_THRESHOLD }

When a member of gBatteries group has an UNDEF state (out of reach, or perhaps its battery is flat), the above code would trigger an error, and my entire rule didn't work.

Ideally I would like the comparison item < LOW_BATTERY_THRESHOLD to return true in the case of item.state is undefined. However... what if someone wrote

gBatteries.select { |item| !(item > LOW_BATTERY_THRESHOLD) }

So the <=> comparison for when self is UNDEF should be -1, and it will therefore fulfil both cases above.

Alternatively, if undef still throws an exception during comparison, I could write it as:

gBatteries.select { |item| !item.state? || item < LOW_BATTERY_THRESHOLD }

It would give me items with undef state and non-undef items whose value is below the threshold.

But alas:

gBatteries.select { |item| !item.state? || item < LOW_BATTERY_THRESHOLD }
          .sort

calling .sort on UNDEF/NULL state caused a java NPE, and string interpolation "#{item}" gives me #<OpenHAB::DSL::Items::NumberItem:0x55c7ea8b> instead of the state (NULL/UNDEF). So there are a few issues that needed fixing there.

In the sort scenario, I would like to see: UNDEF < NULL < other states. WDYT?

[jimtng]

string interpolation "#{item}" gives me #<OpenHAB::DSL::Items::NumberItem:0x55c7ea8b> instead of the state (NULL/UNDEF).

I created #172 to address this

[boc-tothefuture]

@jimtng not sure if this should be a separate issue?

For your specific use case, I am curious if this works?

gBatteries.reject{ |item| item.state&. > LOW_BATTERY_THRESHOLD}

Wouldn't fix your sorting issue.

We would want this to work, but probably doesn't until we monkeypatch UNDEF/NULL case equals ===

gBatteries.grep(UNDEF) + gBatteries.grep_v(UNDEF).select {|item| item < LOW_BATTERY_THRESHOLD).sort

Can you test that? If it doesn't work I will open an issue on UNDEF and NULL needing monkey patch for case equals.

In the sort scenario, I would like to see: UNDEF < NULL < other states. WDYT?

That is a tough one. Is UNDEF or NULL less than an open contact? They are almost not comparable. To me they are closer to the nil type, if you try and sort an array with some nil values in it, your will get an argument error.

Normally, this is handled by calling 'compact' on the enumerable before sorting. You could make the argument that we should on the group case change compact to also filter out UNDEF and NULL states here.

[jimtng]

For your specific use case, I am curious if this works?

gBatteries.reject{ |item| item.state&. > LOW_BATTERY_THRESHOLD}

Resulted in an NPE

21:04:28.971 [ERROR] [ript.internal.ScriptEngineManagerImpl] - Error during evaluation of script 'file:/openhab/conf/automation/jsr223/ruby/personal/battery.rb': org.jruby.embed.EvalFailedException: java.lang.NullPointerException

gBatteries.grep(UNDEF) + gBatteries.grep_v(UNDEF).select {|item| item < LOW_BATTERY_THRESHOLD).sort

21:06:03.908 [ERROR] [ript.internal.ScriptEngineManagerImpl] - Error during evaluation of script 'file:/openhab/conf/automation/jsr223/ruby/personal/battery.rb': org.jruby.embed.EvalFailedException: (NoMethodError) undefined method `to_big_decimal' for nil:NilClass
Did you mean?  BigDecimal

Sorry if this is off topic, shall I open a separate issue?

[pacive]

Might need to change to:

gBatteries.grep(UnDefType) + gBatteries.grep_v(UndefType).select {|item| item < LOW_BATTERY_THRESHOLD).sort

to also handle NULL. Does that work?

[jimtng]

gBatteries.grep(UnDefType) + gBatteries.grep_v(UnDefType).select { |item| item < LOW_BATTERY_THRESHOLD }.sort

01:35:08.141 [ERROR] [ript.internal.ScriptEngineManagerImpl] - Error during evaluation of script 'file:/openhab/conf/automation/jsr223/ruby/personal/test.rb': org.jruby.embed.EvalFailedException: (NoMethodError) undefined method `to_big_decimal' for nil:NilClass
Did you mean?  BigDecimal

It's OK, I'm not asking for help in how to solve this. I brought it up merely as an example of case when dealing with nil / null / undef.

[pacive]

It's a good example, and we have to make it possible (preferably easy) to handle situations like this. Perhaps there's no other way but to force the users to check for NULL/UNDEF, like in the other rule languages.

[github-actions[bot]]

:tada: This issue has been resolved in version 3.1.1 :tada:

The release is available on:

• 3.1.1
• GitHub release

Your semantic-release bot :package::rocket: