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

[expiry] extend ExpiryManager to allow ignoring state updates #2739

Merged
merged 8 commits into from
Feb 24, 2022
Merged

[expiry] extend ExpiryManager to allow ignoring state updates #2739

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3c75ae2
[expiry] extend ExpiryManager to allow ignoring state updates
J-N-K Feb 6, 2022
f597110
improve test stability
J-N-K Feb 7, 2022
1a84c89
another try
J-N-K Feb 7, 2022
bf43121
fix
J-N-K Feb 7, 2022
9d7962d
address review comment
J-N-K Feb 23, 2022
fb5dff9
rename mocks in new tests
J-N-K Feb 23, 2022
f7e4a72
address review comments
J-N-K Feb 24, 2022
c70c5c9
address review comments
J-N-K Feb 24, 2022
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
51 changes: 32 additions & 19 deletions bundles/org.openhab.core/src/main/java/org/openhab/core/internal/items/ExpireManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.openhab.core.items.events.GroupItemStateChangedEvent;
import org.openhab.core.items.events.ItemCommandEvent;
import org.openhab.core.items.events.ItemEventFactory;
import org.openhab.core.items.events.ItemStateChangedEvent;
import org.openhab.core.items.events.ItemStateEvent;
import org.openhab.core.library.types.StringType;
import org.openhab.core.types.Command;
Expand Down Expand Up @@ -72,15 +73,15 @@ public class ExpireManager implements EventSubscriber, RegistryChangeListener<It
protected static final String METADATA_NAMESPACE = "expire";
protected static final String PROPERTY_ENABLED = "enabled";

private static final Set<String> SUBSCRIBED_EVENT_TYPES = Set.of(ItemStateEvent.TYPE, ItemCommandEvent.TYPE,
GroupItemStateChangedEvent.TYPE);
private static final Set<String> SUBSCRIBED_EVENT_TYPES = Set.of(ItemStateEvent.TYPE, ItemStateChangedEvent.TYPE,
ItemCommandEvent.TYPE, GroupItemStateChangedEvent.TYPE);

private final Logger logger = LoggerFactory.getLogger(ExpireManager.class);

private Map<String, @Nullable Optional<ExpireConfig>> itemExpireConfig = new ConcurrentHashMap<>();
private Map<String, Instant> itemExpireMap = new ConcurrentHashMap<>();
private final Map<String, @Nullable Optional<ExpireConfig>> itemExpireConfig = new ConcurrentHashMap<>();
private final Map<String, Instant> itemExpireMap = new ConcurrentHashMap<>();

private ScheduledExecutorService threadPool = ThreadPoolManager
private final ScheduledExecutorService threadPool = ThreadPoolManager
.getScheduledPool(ThreadPoolManager.THREAD_POOL_NAME_COMMON);

private final EventPublisher eventPublisher;
Expand Down Expand Up @@ -140,10 +141,13 @@ protected void deactivate() {
itemExpireMap.clear();
}

private void processEvent(String itemName, Type type) {
logger.trace("Received '{}' for item {}", type, itemName);
private void processEvent(String itemName, Type type, boolean isStateUpdate) {
logger.trace("Received '{}' for item {}, state update = {}", type, itemName, isStateUpdate);
ExpireConfig expireConfig = getExpireConfig(itemName);
if (expireConfig != null) {
if (isStateUpdate && expireConfig.ignoreStateUpdates) {
return;
}
Command expireCommand = expireConfig.expireCommand;
State expireState = expireConfig.expireState;

Expand All @@ -166,7 +170,7 @@ private void processEvent(String itemName, Type type) {
private @Nullable ExpireConfig getExpireConfig(String itemName) {
Optional<ExpireConfig> itemConfig = itemExpireConfig.get(itemName);
if (itemConfig != null) {
return itemConfig.isPresent() ? itemConfig.get() : null;
return itemConfig.orElse(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return itemConfig.get() may be enough. You should remove the Nullable annotation on the itemExpireConfig map value. As it is a ConcurrentHashMap, null values are not allowed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mistake on my side, you do need the code you put in, but you can still remove the @nullable annotation from the declaration.

} else {
Metadata metadata = metadataRegistry.get(new MetadataKey(METADATA_NAMESPACE, itemName));
if (metadata != null) {
Expand Down Expand Up @@ -240,13 +244,13 @@ public void receive(Event event) {
}
if (event instanceof ItemStateEvent) {
ItemStateEvent isEvent = (ItemStateEvent) event;
processEvent(isEvent.getItemName(), isEvent.getItemState());
processEvent(isEvent.getItemName(), isEvent.getItemState(), true);
} else if (event instanceof ItemCommandEvent) {
ItemCommandEvent icEvent = (ItemCommandEvent) event;
processEvent(icEvent.getItemName(), icEvent.getItemCommand());
} else if (event instanceof GroupItemStateChangedEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason to remove this? I can see a use case for the expire functionality on a group (e.g. to turn off all lights in a group if they stay on for too long).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GroupItemStateChangedEvent is a sub-class of ItemStateChangedEvent, so every GroupItemStateChangedEvent will also fulfill this condition.

GroupItemStateChangedEvent gcEvent = (GroupItemStateChangedEvent) event;
processEvent(gcEvent.getItemName(), gcEvent.getItemState());
processEvent(icEvent.getItemName(), icEvent.getItemCommand(), false);
} else if (event instanceof ItemStateChangedEvent) {
ItemStateChangedEvent icEvent = (ItemStateChangedEvent) event;
processEvent(icEvent.getItemName(), icEvent.getItemState(), false);
}
}

Expand Down Expand Up @@ -296,19 +300,18 @@ static class ExpireConfig {
protected static final Pattern DURATION_PATTERN = Pattern
.compile("(?:([0-9]+)H)?\\s*(?:([0-9]+)M)?\\s*(?:([0-9]+)S)?", Pattern.CASE_INSENSITIVE);

@Nullable
final Command expireCommand;
@Nullable
final State expireState;
final @Nullable Command expireCommand;
final @Nullable State expireState;
final String durationString;
final Duration duration;
final boolean ignoreStateUpdates;

/**
* Construct an ExpireConfig from the config string.
*
* Valid syntax:
*
* {@code &lt;duration&gt;[,[state=|command=|]&lt;stateOrCommand&gt;]}<br>
* {@code &lt;duration&gt;[,(state=|command=|)&lt;stateOrCommand&gt;][,ignoreStateUpdates]}<br>
* if neither state= or command= is present, assume state
*
* @param item the item to which we are bound
Expand All @@ -325,6 +328,16 @@ public ExpireConfig(Item item, String configString) throws IllegalArgumentExcept
? configString.substring(commaPos + 1).trim()
: null;

if (stateOrCommand != null && stateOrCommand.endsWith(",ignoreStateUpdates")) {
stateOrCommand = stateOrCommand.substring(0, stateOrCommand.lastIndexOf(","));
ignoreStateUpdates = true;
} else if ("ignoreStateUpdates".equals(stateOrCommand)) {
stateOrCommand = null;
ignoreStateUpdates = true;
} else {
ignoreStateUpdates = false;
}

if ((stateOrCommand != null) && (stateOrCommand.length() > 0)) {
if (stateOrCommand.startsWith(COMMAND_PREFIX)) {
String commandString = stateOrCommand.substring(COMMAND_PREFIX.length());
Expand Down Expand Up @@ -384,7 +397,7 @@ private Duration parseDuration(String durationString) throws IllegalArgumentExce
@Override
public String toString() {
return "duration='" + durationString + "', s=" + duration.toSeconds() + ", state='" + expireState
+ "', command='" + expireCommand + "'";
+ "', command='" + expireCommand + "', ignoreStateUpdates=" + ignoreStateUpdates;
}
}
}
77 changes: 72 additions & 5 deletions ...les/org.openhab.core/src/test/java/org/openhab/core/internal/items/ExpireManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.openhab.core.library.items.NumberItem;
import org.openhab.core.library.items.StringItem;
import org.openhab.core.library.items.SwitchItem;
import org.openhab.core.library.types.DecimalType;
import org.openhab.core.library.types.OnOffType;
import org.openhab.core.library.types.QuantityType;
import org.openhab.core.library.types.StringType;
Expand Down Expand Up @@ -128,6 +129,55 @@ void testCancelExpiry() throws InterruptedException, ItemNotFoundException {
verify(eventPublisherMock, never()).post(any());
}

@Test
void testIgnoreStateUpdateExtendsExpiryOnStateChange() throws InterruptedException, ItemNotFoundException {
Item testItem = new NumberItem(ITEMNAME);
when(itemRegistryMock.getItem(ITEMNAME)).thenReturn(testItem);
when(metadataRegistryMock.get(METADATA_KEY)).thenReturn(config("2s,ignoreStateUpdates"));

Event event = ItemEventFactory.createCommandEvent(ITEMNAME, new DecimalType(1));
expireManager.receive(event);
Thread.sleep(1500L);
event = ItemEventFactory.createStateChangedEvent(ITEMNAME, new DecimalType(2), new DecimalType(1));
expireManager.receive(event);
Thread.sleep(1500L);
verify(eventPublisherMock, never()).post(any());
Thread.sleep(2000L);
verify(eventPublisherMock, times(1)).post(any());
}

@Test
void testIgnoreStateUpdateExtendsExpiryOnCommand() throws InterruptedException, ItemNotFoundException {
Item testItem = new NumberItem(ITEMNAME);
when(itemRegistryMock.getItem(ITEMNAME)).thenReturn(testItem);
when(metadataRegistryMock.get(METADATA_KEY)).thenReturn(config("2s,ignoreStateUpdates"));

Event event = ItemEventFactory.createCommandEvent(ITEMNAME, new DecimalType(1));
expireManager.receive(event);
Thread.sleep(1500L);
event = ItemEventFactory.createCommandEvent(ITEMNAME, new DecimalType(1));
expireManager.receive(event);
Thread.sleep(1500L);
verify(eventPublisherMock, never()).post(any());
Thread.sleep(2000L);
verify(eventPublisherMock, times(1)).post(any());
}

@Test
void testIgnoreStateUpdateDoesNotExtendExpiryOnStateUpdate() throws InterruptedException, ItemNotFoundException {
Item testItem = new NumberItem(ITEMNAME);
when(itemRegistryMock.getItem(ITEMNAME)).thenReturn(testItem);
when(metadataRegistryMock.get(METADATA_KEY)).thenReturn(config("2s,ignoreStateUpdates"));

Event event = ItemEventFactory.createCommandEvent(ITEMNAME, new DecimalType(1));
expireManager.receive(event);
Thread.sleep(1500L);
event = ItemEventFactory.createStateEvent(ITEMNAME, new DecimalType(1));
expireManager.receive(event);
Thread.sleep(1500L);
verify(eventPublisherMock, times(1)).post(any());
}

@Test
void testMetadataChange() throws InterruptedException, ItemNotFoundException {
Metadata md = new Metadata(METADATA_KEY, "1s", null);
Expand Down Expand Up @@ -158,27 +208,44 @@ void testExpireConfig() {
ExpireConfig cfg = new ExpireManager.ExpireConfig(testItem, "1s");
assertEquals(Duration.ofSeconds(1), cfg.duration);
assertEquals(UnDefType.UNDEF, cfg.expireState);
assertEquals(null, cfg.expireCommand);
assertNull(cfg.expireCommand);
assertFalse(cfg.ignoreStateUpdates);

cfg = new ExpireManager.ExpireConfig(testItem, "5h 3m 2s");
assertEquals(Duration.ofHours(5).plusMinutes(3).plusSeconds(2), cfg.duration);
assertEquals(UnDefType.UNDEF, cfg.expireState);
assertEquals(null, cfg.expireCommand);
assertNull(cfg.expireCommand);
assertFalse(cfg.ignoreStateUpdates);

cfg = new ExpireManager.ExpireConfig(testItem, "1h,OFF");
assertEquals(Duration.ofHours(1), cfg.duration);
assertEquals(OnOffType.OFF, cfg.expireState);
assertEquals(null, cfg.expireCommand);
assertNull(cfg.expireCommand);
assertFalse(cfg.ignoreStateUpdates);

cfg = new ExpireManager.ExpireConfig(testItem, "1h,state=OFF");
assertEquals(Duration.ofHours(1), cfg.duration);
assertEquals(OnOffType.OFF, cfg.expireState);
assertEquals(null, cfg.expireCommand);
assertNull(cfg.expireCommand);
assertFalse(cfg.ignoreStateUpdates);

cfg = new ExpireManager.ExpireConfig(testItem, "1h,command=OFF");
assertEquals(Duration.ofHours(1), cfg.duration);
assertEquals(null, cfg.expireState);
assertNull(cfg.expireState);
assertEquals(OnOffType.OFF, cfg.expireCommand);
assertFalse(cfg.ignoreStateUpdates);

cfg = new ExpireManager.ExpireConfig(testItem, "1h,command=OFF,ignoreStateUpdates");
assertEquals(Duration.ofHours(1), cfg.duration);
assertNull(cfg.expireState);
assertEquals(OnOffType.OFF, cfg.expireCommand);
assertTrue(cfg.ignoreStateUpdates);

cfg = new ExpireManager.ExpireConfig(testItem, "5h 3m 2s,ignoreStateUpdates");
assertEquals(Duration.ofHours(5).plusMinutes(3).plusSeconds(2), cfg.duration);
assertEquals(UnDefType.UNDEF, cfg.expireState);
assertNull(cfg.expireCommand);
assertTrue(cfg.ignoreStateUpdates);

try {
cfg = new ExpireManager.ExpireConfig(testItem, "1h,command=OPEN");
Expand Down