Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 15, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fae6d1e
Allow equals comparison for DecimalTypes and QuantityTypes
cweitkamp Dec 29, 2021
59dc89b
Reverted cahnges partially; Incorporated comments from review;
cweitkamp Dec 30, 2021
2410eac
Incorporated comments from review
cweitkamp Dec 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 88 additions & 104 deletions ...n/java/org/openhab/core/automation/internal/module/handler/ItemStateConditionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
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;
Expand Down Expand Up @@ -77,7 +78,6 @@ public void dispose() {
itemRegistry = null;
}

@SuppressWarnings({ "rawtypes", "unchecked", "null" })
@Override
public boolean isSatisfied(Map<String, Object> inputs) {
String itemName = (String) module.getConfiguration().get(ITEM_NAME);
Expand All @@ -93,122 +93,106 @@ public boolean isSatisfied(Map<String, Object> inputs) {
return false;
}
try {
Item item = itemRegistry.getItem(itemName);
State compareState = TypeParser.parseState(item.getAcceptedDataTypes(), state);
State itemState = item.getState();
logger.debug("ItemStateCondition '{}' checking if {} (State={}) {} {}", module.getId(), itemName, itemState,
operator, compareState);
logger.debug("ItemStateCondition '{}' checking if {} {} {}", module.getId(), itemName, operator, state);
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) {
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;
return !greaterThanOrEqualsToItemState(itemName, state);
case "<=":
case "=<":
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
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;
return lessThanOrEqualsToItemState(itemName, state);
case ">":
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
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;
return !lessThanOrEqualsToItemState(itemName, state);
case ">=":
case "=>":
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
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;
return greaterThanOrEqualsToItemState(itemName, state);
}
} 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);
}
}
22 changes: 17 additions & 5 deletions ...va/org/openhab/core/automation/internal/module/handler/ItemStateConditionHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
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;

Expand All @@ -45,9 +46,9 @@
*/
@ExtendWith(MockitoExtension.class)
@MockitoSettings(strictness = Strictness.WARN)
@NonNullByDefault
public class ItemStateConditionHandlerTest {

@NonNullByDefault
public static class ParameterSet {
public final String comparisonState;
public final State itemState;
Expand All @@ -66,10 +67,13 @@ public static Collection<Object[]> equalsParameters() {
{ 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() {
Expand All @@ -78,6 +82,7 @@ public static Collection<Object[]> greaterThanParameters() {
{ 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) }, //
Expand All @@ -92,12 +97,15 @@ public static Collection<Object[]> greaterThanOrEqualsParameters() {
{ 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() {
Expand All @@ -106,6 +114,7 @@ public static Collection<Object[]> lessThanParameters() {
{ 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) }, //
Expand All @@ -120,19 +129,22 @@ public static Collection<Object[]> lessThanOrEqualsParameters() {
{ 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 {
Expand Down