[automation] Log warning for equals condition for DecimalTypes and QuantityTypes (#2653)

* Allow equals comparison for DecimalTypes and QuantityTypes

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

bundles/org.openhab.core.automation/src/main/java/org/openhab/core/automation/internal/module/handler/ItemStateConditionHandler.java

@@ -23,6 +23,7 @@ import org.openhab.core.items.ItemNotFoundException;
 import org.openhab.core.items.ItemRegistry;
 import org.openhab.core.library.types.DecimalType;
 import org.openhab.core.library.types.QuantityType;
+import org.openhab.core.library.unit.Units;
 import org.openhab.core.types.State;
 import org.openhab.core.types.TypeParser;
 import org.slf4j.Logger;
@@ -77,7 +78,6 @@ public class ItemStateConditionHandler extends BaseConditionModuleHandler {
         itemRegistry = null;
     }
 
-    @SuppressWarnings({ "rawtypes", "unchecked", "null" })
     @Override
     public boolean isSatisfied(Map<String, Object> inputs) {
         String itemName = (String) module.getConfiguration().get(ITEM_NAME);
@@ -93,122 +93,106 @@ public class ItemStateConditionHandler extends BaseConditionModuleHandler {
             return false;
         }
         try {
-            Item item = itemRegistry.getItem(itemName);
+            logger.debug("ItemStateCondition '{}' checking if {} {} {}", module.getId(), itemName, operator, state);
-            State compareState = TypeParser.parseState(item.getAcceptedDataTypes(), state);
-            State itemState = item.getState();
-            logger.debug("ItemStateCondition '{}' checking if {} (State={}) {} {}", module.getId(), itemName, itemState,
-                    operator, compareState);
             switch (operator) {
                 case "=":
-                    return itemState.equals(compareState);
+                    return equalsToItemState(itemName, state);
                 case "!=":
-                    return !itemState.equals(compareState);
+                    return !equalsToItemState(itemName, state);
                 case "<":
-                    if (itemState instanceof QuantityType) {
+                    return !greaterThanOrEqualsToItemState(itemName, state);
-                        QuantityType qtState = (QuantityType) itemState;
-                        if (compareState instanceof DecimalType) {
-                            // allow compareState without unit -> implicitly assume its the same as the one from the
-                            // state, but warn the user
-                            logger.warn(
-                                    "Received a QuantityType state '{}' with unit for item {}, but the condition is defined as a plain number without unit ({}), please consider adding a unit to the condition.",
-                                    qtState, itemName, state);
-                            return qtState.compareTo(new QuantityType<>(((DecimalType) compareState).toBigDecimal(),
-                                    qtState.getUnit())) < 0;
-                        } else if (compareState instanceof QuantityType) {
-                            return qtState.compareTo((QuantityType) compareState) < 0;
-                        } else {
-                            logger.warn(
-                                    "Condition '{}' cannot be compared to the incompatible state '{}' from item {}.",
-                                    state, qtState, itemName);
-                        }
-                    } else if (itemState instanceof DecimalType && null != compareState) {
-                        DecimalType decimalState = compareState.as(DecimalType.class);
-                        if (null != decimalState) {
-                            return ((DecimalType) itemState).compareTo(decimalState) < 0;
-                        }
-                    }
-                    break;
                 case "<=":
                 case "=<":
-                    if (itemState instanceof QuantityType) {
+                    return lessThanOrEqualsToItemState(itemName, state);
-                        QuantityType qtState = (QuantityType) itemState;
-                        if (compareState instanceof DecimalType) {
-                            // allow compareState without unit -> implicitly assume its the same as the one from the
-                            // state, but warn the user
-                            logger.warn(
-                                    "Received a QuantityType state '{}' with unit for item {}, but the condition is defined as a plain number without unit ({}), please consider adding a unit to the condition.",
-                                    qtState, itemName, state);
-                            return qtState.compareTo(new QuantityType<>(((DecimalType) compareState).toBigDecimal(),
-                                    qtState.getUnit())) <= 0;
-                        } else if (compareState instanceof QuantityType) {
-                            return qtState.compareTo((QuantityType) compareState) <= 0;
-                        } else {
-                            logger.warn(
-                                    "Condition '{}' cannot be compared to the incompatible state '{}' from item {}.",
-                                    state, qtState, itemName);
-                        }
-                    } else if (itemState instanceof DecimalType && null != compareState) {
-                        DecimalType decimalState = compareState.as(DecimalType.class);
-                        if (null != decimalState) {
-                            return ((DecimalType) itemState).compareTo(decimalState) <= 0;
-                        }
-                    }
-                    break;
                 case ">":
-                    if (itemState instanceof QuantityType) {
+                    return !lessThanOrEqualsToItemState(itemName, state);
-                        QuantityType qtState = (QuantityType) itemState;
-                        if (compareState instanceof DecimalType) {
-                            // allow compareState without unit -> implicitly assume its the same as the one from the
-                            // state, but warn the user
-                            logger.warn(
-                                    "Received a QuantityType state '{}' with unit  for item {}, but the condition is defined as a plain number without unit ({}), please consider adding a unit to the condition.",
-                                    qtState, itemName, state);
-                            return qtState.compareTo(new QuantityType<>(((DecimalType) compareState).toBigDecimal(),
-                                    qtState.getUnit())) > 0;
-                        } else if (compareState instanceof QuantityType) {
-                            return qtState.compareTo((QuantityType) compareState) > 0;
-                        } else {
-                            logger.warn(
-                                    "Condition '{}' cannot be compared to the incompatible state '{}' from item {}.",
-                                    state, qtState, itemName);
-                        }
-                    } else if (itemState instanceof DecimalType && null != compareState) {
-                        DecimalType decimalState = compareState.as(DecimalType.class);
-                        if (null != decimalState) {
-                            return ((DecimalType) itemState).compareTo(decimalState) > 0;
-                        }
-                    }
-                    break;
                 case ">=":
                 case "=>":
-                    if (itemState instanceof QuantityType) {
+                    return greaterThanOrEqualsToItemState(itemName, state);
-                        QuantityType qtState = (QuantityType) itemState;
-                        if (compareState instanceof DecimalType) {
-                            // allow compareState without unit -> implicitly assume its the same as the one from the
-                            // state, but warn the user
-                            logger.warn(
-                                    "Received a QuantityType state '{}' with unit for item {}, but the condition is defined as a plain number without unit ({}), please consider adding a unit to the condition.",
-                                    qtState, itemName, state);
-                            return qtState.compareTo(new QuantityType<>(((DecimalType) compareState).toBigDecimal(),
-                                    qtState.getUnit())) >= 0;
-                        } else if (compareState instanceof QuantityType) {
-                            return qtState.compareTo((QuantityType) compareState) >= 0;
-                        } else {
-                            logger.warn(
-                                    "Condition '{}' cannot be compared to the incompatible state '{}' from item {}.",
-                                    state, qtState, itemName);
-                        }
-                    } else if (itemState instanceof DecimalType && null != compareState) {
-                        DecimalType decimalState = compareState.as(DecimalType.class);
-                        if (null != decimalState) {
-                            return ((DecimalType) itemState).compareTo(decimalState) >= 0;
-                        }
-                    }
-                    break;
             }
         } catch (ItemNotFoundException e) {
             logger.error("Item with name {} not found in ItemRegistry.", itemName);
         }
         return false;
     }
+
+    @SuppressWarnings({ "rawtypes", "unchecked", "null" })
+    private boolean lessThanOrEqualsToItemState(String itemName, String state) throws ItemNotFoundException {
+        Item item = itemRegistry.getItem(itemName);
+        State compareState = TypeParser.parseState(item.getAcceptedDataTypes(), state);
+        State itemState = item.getState();
+        if (itemState instanceof QuantityType) {
+            QuantityType qtState = (QuantityType) itemState;
+            if (compareState instanceof DecimalType) {
+                // allow compareState without unit -> implicitly assume its the same as the one from the
+                // state, but warn the user
+                if (!Units.ONE.equals(qtState.getUnit())) {
+                    logger.warn(
+                            "Received a QuantityType state '{}' with unit for item {}, but the condition is defined as a plain number without unit ({}), please consider adding a unit to the condition.",
+                            qtState, itemName, state);
+                }
+                return qtState.compareTo(
+                        new QuantityType<>(((DecimalType) compareState).toBigDecimal(), qtState.getUnit())) <= 0;
+            } else if (compareState instanceof QuantityType) {
+                return qtState.compareTo((QuantityType) compareState) <= 0;
+            }
+        } else if (itemState instanceof DecimalType && null != compareState) {
+            DecimalType decimalState = compareState.as(DecimalType.class);
+            if (null != decimalState) {
+                return ((DecimalType) itemState).compareTo(decimalState) <= 0;
+            }
+        }
+        return false;
+    }
+
+    @SuppressWarnings({ "rawtypes", "unchecked", "null" })
+    private boolean greaterThanOrEqualsToItemState(String itemName, String state) throws ItemNotFoundException {
+        Item item = itemRegistry.getItem(itemName);
+        State compareState = TypeParser.parseState(item.getAcceptedDataTypes(), state);
+        State itemState = item.getState();
+        if (itemState instanceof QuantityType) {
+            QuantityType qtState = (QuantityType) itemState;
+            if (compareState instanceof DecimalType) {
+                // allow compareState without unit -> implicitly assume its the same as the one from the
+                // state, but warn the user
+                if (!Units.ONE.equals(qtState.getUnit())) {
+                    logger.warn(
+                            "Received a QuantityType state '{}' with unit for item {}, but the condition is defined as a plain number without unit ({}), please consider adding a unit to the condition.",
+                            qtState, itemName, state);
+                }
+                return qtState.compareTo(
+                        new QuantityType<>(((DecimalType) compareState).toBigDecimal(), qtState.getUnit())) >= 0;
+            } else if (compareState instanceof QuantityType) {
+                return qtState.compareTo((QuantityType) compareState) >= 0;
+            }
+        } else if (itemState instanceof DecimalType && null != compareState) {
+            DecimalType decimalState = compareState.as(DecimalType.class);
+            if (null != decimalState) {
+                return ((DecimalType) itemState).compareTo(decimalState) >= 0;
+            }
+        }
+        return false;
+    }
+
+    @SuppressWarnings("null")
+    private boolean equalsToItemState(String itemName, String state) throws ItemNotFoundException {
+        Item item = itemRegistry.getItem(itemName);
+        State compareState = TypeParser.parseState(item.getAcceptedDataTypes(), state);
+        State itemState = item.getState();
+        if (itemState instanceof QuantityType && compareState instanceof DecimalType) {
+            QuantityType<?> qtState = (QuantityType<?>) itemState;
+            if (Units.ONE.equals(qtState.getUnit())) {
+                // allow compareStates without unit if the unit of the state equals to ONE
+                return itemState
+                        .equals(new QuantityType<>(((DecimalType) compareState).toBigDecimal(), qtState.getUnit()));
+            } else {
+                // log a warning if the unit of the state differs from ONE
+                logger.warn(
+                        "Received a QuantityType state '{}' with unit for item {}, but the condition is defined as a plain number without unit ({}), comparison will fail unless a unit is added to the condition.",
+                        itemState, itemName, state);
+                return false;
+            }
+        }
+        return itemState.equals(compareState);
+    }
 }

bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/internal/module/handler/ItemStateConditionHandlerTest.java

@@ -35,6 +35,7 @@ import org.openhab.core.items.ItemRegistry;
 import org.openhab.core.library.items.NumberItem;
 import org.openhab.core.library.types.DecimalType;
 import org.openhab.core.library.types.QuantityType;
+import org.openhab.core.library.unit.ImperialUnits;
 import org.openhab.core.library.unit.SIUnits;
 import org.openhab.core.types.State;
 
@@ -45,9 +46,9 @@ import org.openhab.core.types.State;
  */
 @ExtendWith(MockitoExtension.class)
 @MockitoSettings(strictness = Strictness.WARN)
+@NonNullByDefault
 public class ItemStateConditionHandlerTest {
 
-    @NonNullByDefault
     public static class ParameterSet {
         public final String comparisonState;
         public final State itemState;
@@ -66,10 +67,13 @@ public class ItemStateConditionHandlerTest {
                 { new ParameterSet("5", new DecimalType(5), true) }, //
                 { new ParameterSet("5 °C", new DecimalType(23), false) }, //
                 { new ParameterSet("5 °C", new DecimalType(5), false) }, //
+                { new ParameterSet("0", new QuantityType<>(), true) }, //
                 { new ParameterSet("5", new QuantityType<>(23, SIUnits.CELSIUS), false) }, //
                 { new ParameterSet("5", new QuantityType<>(5, SIUnits.CELSIUS), false) }, //
                 { new ParameterSet("5 °C", new QuantityType<>(23, SIUnits.CELSIUS), false) }, //
-                { new ParameterSet("5 °C", new QuantityType<>(5, SIUnits.CELSIUS), true) } });
+                { new ParameterSet("5 °C", new QuantityType<>(5, SIUnits.CELSIUS), true) }, //
+                { new ParameterSet("0 °C", new QuantityType<>(32, ImperialUnits.FAHRENHEIT), true) }, //
+                { new ParameterSet("32 °F", new QuantityType<>(0, SIUnits.CELSIUS), true) } });
     }
 
     public static Collection<Object[]> greaterThanParameters() {
@@ -78,6 +82,7 @@ public class ItemStateConditionHandlerTest {
                 { new ParameterSet("5", new DecimalType(5), false) }, //
                 { new ParameterSet("5 °C", new DecimalType(23), true) }, //
                 { new ParameterSet("5 °C", new DecimalType(5), false) }, //
+                { new ParameterSet("0", new QuantityType<>(), false) }, //
                 { new ParameterSet("5", new QuantityType<>(23, SIUnits.CELSIUS), true) }, //
                 { new ParameterSet("5", new QuantityType<>(5, SIUnits.CELSIUS), false) }, //
                 { new ParameterSet("5 °C", new QuantityType<>(23, SIUnits.CELSIUS), true) }, //
@@ -92,12 +97,15 @@ public class ItemStateConditionHandlerTest {
                 { new ParameterSet("5 °C", new DecimalType(23), true) }, //
                 { new ParameterSet("5 °C", new DecimalType(5), true) }, //
                 { new ParameterSet("5 °C", new DecimalType(4), false) }, //
+                { new ParameterSet("0", new QuantityType<>(), true) }, //
                 { new ParameterSet("5", new QuantityType<>(23, SIUnits.CELSIUS), true) }, //
                 { new ParameterSet("5", new QuantityType<>(5, SIUnits.CELSIUS), true) }, //
                 { new ParameterSet("5", new QuantityType<>(4, SIUnits.CELSIUS), false) }, //
                 { new ParameterSet("5 °C", new QuantityType<>(23, SIUnits.CELSIUS), true) }, //
                 { new ParameterSet("5 °C", new QuantityType<>(5, SIUnits.CELSIUS), true) }, //
-                { new ParameterSet("5 °C", new QuantityType<>(4, SIUnits.CELSIUS), false) } });
+                { new ParameterSet("5 °C", new QuantityType<>(4, SIUnits.CELSIUS), false) }, //
+                { new ParameterSet("0 °C", new QuantityType<>(32, ImperialUnits.FAHRENHEIT), true) }, //
+                { new ParameterSet("32 °F", new QuantityType<>(0, SIUnits.CELSIUS), true) } });
     }
 
     public static Collection<Object[]> lessThanParameters() {
@@ -106,6 +114,7 @@ public class ItemStateConditionHandlerTest {
                 { new ParameterSet("5", new DecimalType(4), true) }, //
                 { new ParameterSet("5 °C", new DecimalType(23), false) }, //
                 { new ParameterSet("5 °C", new DecimalType(4), true) }, //
+                { new ParameterSet("0", new QuantityType<>(), false) }, //
                 { new ParameterSet("5", new QuantityType<>(23, SIUnits.CELSIUS), false) }, //
                 { new ParameterSet("5", new QuantityType<>(4, SIUnits.CELSIUS), true) }, //
                 { new ParameterSet("5 °C", new QuantityType<>(23, SIUnits.CELSIUS), false) }, //
@@ -120,19 +129,22 @@ public class ItemStateConditionHandlerTest {
                 { new ParameterSet("5 °C", new DecimalType(23), false) }, //
                 { new ParameterSet("5 °C", new DecimalType(5), true) }, //
                 { new ParameterSet("5 °C", new DecimalType(4), true) }, //
+                { new ParameterSet("0", new QuantityType<>(), true) }, //
                 { new ParameterSet("5", new QuantityType<>(23, SIUnits.CELSIUS), false) }, //
                 { new ParameterSet("5", new QuantityType<>(5, SIUnits.CELSIUS), true) }, //
                 { new ParameterSet("5", new QuantityType<>(4, SIUnits.CELSIUS), true) }, //
                 { new ParameterSet("5 °C", new QuantityType<>(23, SIUnits.CELSIUS), false) }, //
                 { new ParameterSet("5 °C", new QuantityType<>(5, SIUnits.CELSIUS), true) }, //
-                { new ParameterSet("5 °C", new QuantityType<>(4, SIUnits.CELSIUS), true) } });
+                { new ParameterSet("5 °C", new QuantityType<>(4, SIUnits.CELSIUS), true) }, //
+                { new ParameterSet("0 °C", new QuantityType<>(32, ImperialUnits.FAHRENHEIT), true) }, //
+                { new ParameterSet("32 °F", new QuantityType<>(0, SIUnits.CELSIUS), true) } });
     }
 
     private static final String ITEM_NAME = "myItem";
 
     private final NumberItem item = new NumberItem(ITEM_NAME);
 
-    private @Mock ItemRegistry mockItemRegistry;
+    private @NonNullByDefault({}) @Mock ItemRegistry mockItemRegistry;
 
     @BeforeEach
     public void setup() throws ItemNotFoundException {