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

Jump to bottom

Fix rule startlevel trigger executes during initialization #3717

Merged
merged 2 commits into from
Jul 21, 2023
Merged

Fix rule startlevel trigger executes during initialization #3717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f22c90d
Fix rule startlevel trigger executes during initialization
J-N-K Jul 20, 2023
750bdb6
fix itests
J-N-K Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion ...ab.core.automation/src/main/java/org/openhab/core/automation/internal/RuleEngineImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ public class RuleEngineImpl implements RuleManager, RegistryChangeListener<Modul
* The storage for the disable information
*/
private final Storage<Boolean> disabledRulesStorage;
private final StartLevelService startLevelService;

/**
* Locker which does not permit rule initialization when the rule engine is stopping.
Expand Down Expand Up @@ -254,7 +255,7 @@ public void runNow(String uid, boolean considerConditions, @Nullable Map<String,
@Activate
public RuleEngineImpl(final @Reference ModuleTypeRegistry moduleTypeRegistry,
final @Reference RuleRegistry ruleRegistry, final @Reference StorageService storageService,
final @Reference ReadyService readyService) {
final @Reference ReadyService readyService, final @Reference StartLevelService startLevelService) {
this.disabledRulesStorage = storageService.<Boolean> getStorage(DISABLED_RULE_STORAGE,
this.getClass().getClassLoader());

Expand All @@ -265,6 +266,7 @@ public RuleEngineImpl(final @Reference ModuleTypeRegistry moduleTypeRegistry,

this.ruleRegistry = ruleRegistry;
this.readyService = readyService;
this.startLevelService = startLevelService;

listener = new RegistryChangeListener<>() {
@Override
Expand Down Expand Up @@ -846,6 +848,17 @@ private boolean activateRule(final WrappedRule rule) {
// Register the rule and set idle status.
register(rule);
setStatus(ruleUID, new RuleStatusInfo(RuleStatus.IDLE));

// check if we have to trigger because of the startlevel
List<Trigger> slTriggers = rule.getTriggers().stream().map(WrappedTrigger::unwrap)
.filter(t -> SystemTriggerHandler.STARTLEVEL_MODULE_TYPE_ID.equals(t.getTypeUID())).toList();
if (slTriggers.stream()
.anyMatch(t -> ((BigDecimal) t.getConfiguration().get(SystemTriggerHandler.CFG_STARTLEVEL))
.intValue() <= startLevelService.getStartLevel())) {
runNow(rule.getUID(), true,
Map.of(SystemTriggerHandler.OUT_STARTLEVEL, StartLevelService.STARTLEVEL_RULES));
}

return true;
}

Expand Down
2 changes: 1 addition & 1 deletion ...in/java/org/openhab/core/automation/internal/module/factory/CoreModuleHandlerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public Collection<String> getTypes() {
} else if (ItemCommandTriggerHandler.MODULE_TYPE_ID.equals(moduleTypeUID)) {
return new ItemCommandTriggerHandler(trigger, ruleUID, bundleContext, itemRegistry);
} else if (SystemTriggerHandler.STARTLEVEL_MODULE_TYPE_ID.equals(moduleTypeUID)) {
return new SystemTriggerHandler(trigger, bundleContext, startLevelService);
return new SystemTriggerHandler(trigger, bundleContext);
} else if (ThingStatusTriggerHandler.CHANGE_MODULE_TYPE_ID.equals(moduleTypeUID)
|| ThingStatusTriggerHandler.UPDATE_MODULE_TYPE_ID.equals(moduleTypeUID)) {
return new ThingStatusTriggerHandler(trigger, bundleContext);
Expand Down
10 changes: 1 addition & 9 deletions ...c/main/java/org/openhab/core/automation/internal/module/handler/SystemTriggerHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,13 @@ public class SystemTriggerHandler extends BaseTriggerModuleHandler implements Ev

private final Integer startlevel;
private final Set<String> types;
private final StartLevelService startLevelService;

private boolean triggered = false;

private final ServiceRegistration<?> eventSubscriberRegistration;

public SystemTriggerHandler(Trigger module, BundleContext bundleContext, StartLevelService startLevelService) {
public SystemTriggerHandler(Trigger module, BundleContext bundleContext) {
super(module);
this.startLevelService = startLevelService;
this.startlevel = ((BigDecimal) module.getConfiguration().get(CFG_STARTLEVEL)).intValue();
if (STARTLEVEL_MODULE_TYPE_ID.equals(module.getTypeUID())) {
this.types = Set.of(StartlevelEvent.TYPE);
Expand All @@ -69,12 +67,6 @@ public SystemTriggerHandler(Trigger module, BundleContext bundleContext, StartLe
@Override
public void setCallback(ModuleHandlerCallback callback) {
super.setCallback(callback);

// trigger immediately when start level is already reached
int currentStartLevel = startLevelService.getStartLevel();
if (currentStartLevel > StartLevelService.STARTLEVEL_RULEENGINE && currentStartLevel >= startlevel) {
trigger();
}
}

@Override
Expand Down
47 changes: 4 additions & 43 deletions ...st/java/org/openhab/core/automation/internal/module/handler/SystemTriggerHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,34 +67,17 @@ public void setup() {
public void testDoesNotTriggerIfStartLevelTooLow() {
when(startLevelServiceMock.getStartLevel()).thenReturn(0);

SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock,
startLevelServiceMock);
SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock);
triggerHandler.setCallback(callbackMock);

verifyNoInteractions(callbackMock);
}

@Test
public void testDoesTriggerImmediatelyIfStartLevelHigherOnInit() {
when(startLevelServiceMock.getStartLevel()).thenReturn(100);

SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock,
startLevelServiceMock);
triggerHandler.setCallback(callbackMock);

ArgumentCaptor<Map> captor = ArgumentCaptor.forClass(Map.class);
verify(callbackMock).triggered(eq(triggerMock), captor.capture());

Map<String, Object> configuration = (Map<String, Object>) captor.getValue();
assertThat(configuration.get(SystemTriggerHandler.OUT_STARTLEVEL), is(CFG_STARTLEVEL));
}

@Test
public void testDoesNotTriggerIfStartLevelEventLower() {
when(startLevelServiceMock.getStartLevel()).thenReturn(0);

SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock,
startLevelServiceMock);
SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock);
triggerHandler.setCallback(callbackMock);

Event event = SystemEventFactory.createStartlevelEvent(70);
Expand All @@ -107,8 +90,7 @@ public void testDoesNotTriggerIfStartLevelEventLower() {
public void testDoesTriggerIfStartLevelEventHigher() {
when(startLevelServiceMock.getStartLevel()).thenReturn(0);

SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock,
startLevelServiceMock);
SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock);
triggerHandler.setCallback(callbackMock);

Event event = SystemEventFactory.createStartlevelEvent(100);
Expand All @@ -121,32 +103,11 @@ public void testDoesTriggerIfStartLevelEventHigher() {
assertThat(configuration.get(SystemTriggerHandler.OUT_STARTLEVEL), is(CFG_STARTLEVEL));
}

@Test
public void testDoesNotTriggerAfterInitialTrigger() {
when(startLevelServiceMock.getStartLevel()).thenReturn(100);

SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock,
startLevelServiceMock);
triggerHandler.setCallback(callbackMock);

ArgumentCaptor<Map> captor = ArgumentCaptor.forClass(Map.class);
verify(callbackMock).triggered(eq(triggerMock), captor.capture());

Map<String, Object> configuration = (Map<String, Object>) captor.getValue();
assertThat(configuration.get(SystemTriggerHandler.OUT_STARTLEVEL), is(CFG_STARTLEVEL));

Event event = SystemEventFactory.createStartlevelEvent(100);
triggerHandler.receive(event);

verifyNoMoreInteractions(callbackMock);
}

@Test
public void testDoesNotTriggerAfterEventTrigger() {
when(startLevelServiceMock.getStartLevel()).thenReturn(0);

SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock,
startLevelServiceMock);
SystemTriggerHandler triggerHandler = new SystemTriggerHandler(triggerMock, bundleContextMock);
triggerHandler.setCallback(callbackMock);

Event event = SystemEventFactory.createStartlevelEvent(100);
Expand Down
8 changes: 7 additions & 1 deletion ...main/java/org/openhab/core/automation/integration/test/AutomationIntegrationJsonTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.Collection;
import java.util.Optional;
Expand Down Expand Up @@ -80,6 +81,7 @@ public class AutomationIntegrationJsonTest extends JavaOSGiTest {
private final Logger logger = LoggerFactory.getLogger(AutomationIntegrationJsonTest.class);
private @NonNullByDefault({}) EventPublisher eventPublisher;
private @NonNullByDefault({}) ItemRegistry itemRegistry;
private @NonNullByDefault({}) StartLevelService startLevelService;
private @NonNullByDefault({}) RuleRegistry ruleRegistry;
private @NonNullByDefault({}) RuleManager ruleManager;
private @NonNullByDefault({}) ManagedRuleProvider managedRuleProvider;
Expand All @@ -96,8 +98,12 @@ public void before() {

eventPublisher = getService(EventPublisher.class);
itemRegistry = getService(ItemRegistry.class);
startLevelService = mock(StartLevelService.class);
when(startLevelService.getStartLevel()).thenReturn(100);
registerService(startLevelService, StartLevelService.class.getName());

CoreModuleHandlerFactory coreModuleHandlerFactory = new CoreModuleHandlerFactory(getBundleContext(),
eventPublisher, itemRegistry, mock(TimeZoneProvider.class), mock(StartLevelService.class));
eventPublisher, itemRegistry, mock(TimeZoneProvider.class), startLevelService);
mock(CoreModuleHandlerFactory.class);
registerService(coreModuleHandlerFactory);

Expand Down
8 changes: 7 additions & 1 deletion ...src/main/java/org/openhab/core/automation/integration/test/AutomationIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -98,6 +99,7 @@ public class AutomationIntegrationTest extends JavaOSGiTest {
private final Logger logger = LoggerFactory.getLogger(AutomationIntegrationTest.class);
private @Nullable EventPublisher eventPublisher;
private @Nullable ItemRegistry itemRegistry;
private @NonNullByDefault({}) StartLevelService startLevelService;
private @Nullable RuleRegistry ruleRegistry;
private @Nullable RuleManager ruleEngine;
private @Nullable ManagedRuleProvider managedRuleProvider;
Expand All @@ -113,9 +115,13 @@ public void before() {

eventPublisher = getService(EventPublisher.class);
itemRegistry = getService(ItemRegistry.class);
startLevelService = mock(StartLevelService.class);
when(startLevelService.getStartLevel()).thenReturn(100);
registerService(startLevelService, StartLevelService.class.getName());

CoreModuleHandlerFactory coreModuleHandlerFactory = new CoreModuleHandlerFactory(getBundleContext(),
Objects.requireNonNull(eventPublisher), Objects.requireNonNull(itemRegistry),
mock(TimeZoneProvider.class), mock(StartLevelService.class));
mock(TimeZoneProvider.class), startLevelService);
mock(CoreModuleHandlerFactory.class);
registerService(coreModuleHandlerFactory);

Expand Down
8 changes: 8 additions & 0 deletions ....tests/src/main/java/org/openhab/core/automation/integration/test/RuleSimulationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import static org.hamcrest.CoreMatchers.*;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.time.ZoneId;
import java.time.ZonedDateTime;
Expand Down Expand Up @@ -54,6 +56,7 @@
import org.openhab.core.common.registry.ProviderChangeListener;
import org.openhab.core.config.core.Configuration;
import org.openhab.core.service.ReadyMarker;
import org.openhab.core.service.StartLevelService;
import org.openhab.core.storage.StorageService;
import org.openhab.core.test.java.JavaOSGiTest;
import org.slf4j.Logger;
Expand All @@ -71,11 +74,16 @@ public class RuleSimulationTest extends JavaOSGiTest {

private @Nullable RuleRegistry ruleRegistry;
private @Nullable RuleManager ruleEngine;
private @NonNullByDefault({}) StartLevelService startLevelService;

@BeforeEach
public void before() {
registerVolatileStorageService();

startLevelService = mock(StartLevelService.class);
when(startLevelService.getStartLevel()).thenReturn(100);
registerService(startLevelService, StartLevelService.class.getName());

StorageService storageService = getService(StorageService.class);

ruleRegistry = getService(RuleRegistry.class);
Expand Down
5 changes: 5 additions & 0 deletions ...re.tests/src/main/java/org/openhab/core/automation/internal/module/RunRuleModuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import static java.util.Map.entry;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -69,9 +70,13 @@ public class RunRuleModuleTest extends JavaOSGiTest {

private final Logger logger = LoggerFactory.getLogger(RunRuleModuleTest.class);
private final VolatileStorageService volatileStorageService = new VolatileStorageService();
private @NonNullByDefault({}) StartLevelService startLevelService;

@BeforeEach
public void before() {
startLevelService = mock(StartLevelService.class);
when(startLevelService.getStartLevel()).thenReturn(100);
registerService(startLevelService, StartLevelService.class.getName());
EventPublisher eventPublisher = getService(EventPublisher.class);
ItemRegistry itemRegistry = getService(ItemRegistry.class);
CoreModuleHandlerFactory coreModuleHandlerFactory = new CoreModuleHandlerFactory(getBundleContext(),
Expand Down
5 changes: 5 additions & 0 deletions ...core.tests/src/main/java/org/openhab/core/automation/internal/module/RuntimeRuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import static java.util.Map.entry;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -79,9 +80,13 @@ public class RuntimeRuleTest extends JavaOSGiTest {

private final Logger logger = LoggerFactory.getLogger(RuntimeRuleTest.class);
private final VolatileStorageService volatileStorageService = new VolatileStorageService();
private @NonNullByDefault({}) StartLevelService startLevelService;

@BeforeEach
public void before() {
startLevelService = mock(StartLevelService.class);
when(startLevelService.getStartLevel()).thenReturn(100);
registerService(startLevelService, StartLevelService.class.getName());
EventPublisher eventPublisher = getService(EventPublisher.class);
ItemRegistry itemRegistry = getService(ItemRegistry.class);
CoreModuleHandlerFactory coreModuleHandlerFactory = new CoreModuleHandlerFactory(getBundleContext(),
Expand Down
5 changes: 5 additions & 0 deletions ...ain/java/org/openhab/core/automation/module/timer/internal/BasicConditionHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -78,6 +79,7 @@ public abstract class BasicConditionHandlerTest extends JavaOSGiTest {
private @NonNullByDefault({}) RuleRegistry ruleRegistry;
private @NonNullByDefault({}) RuleManager ruleEngine;
private @Nullable Event itemEvent;
private @NonNullByDefault({}) StartLevelService startLevelService;

/**
* This executes before every test and before the
Expand All @@ -86,6 +88,9 @@ public abstract class BasicConditionHandlerTest extends JavaOSGiTest {
*/
@BeforeEach
public void beforeBase() {
startLevelService = mock(StartLevelService.class);
when(startLevelService.getStartLevel()).thenReturn(100);
registerService(startLevelService, StartLevelService.class.getName());
EventPublisher eventPublisher = getService(EventPublisher.class);
ItemRegistry itemRegistry = getService(ItemRegistry.class);
CoreModuleHandlerFactory coreModuleHandlerFactory = new CoreModuleHandlerFactory(getBundleContext(),
Expand Down
5 changes: 5 additions & 0 deletions ...ests/src/main/java/org/openhab/core/automation/module/timer/internal/RuntimeRuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.Collection;
import java.util.HashMap;
Expand Down Expand Up @@ -75,9 +76,13 @@ public class RuntimeRuleTest extends JavaOSGiTest {
private VolatileStorageService volatileStorageService = new VolatileStorageService();
private @NonNullByDefault({}) RuleRegistry ruleRegistry;
private @NonNullByDefault({}) RuleManager ruleEngine;
private @NonNullByDefault({}) StartLevelService startLevelService;

@BeforeEach
public void before() {
startLevelService = mock(StartLevelService.class);
when(startLevelService.getStartLevel()).thenReturn(100);
registerService(startLevelService, StartLevelService.class.getName());
EventPublisher eventPublisher = getService(EventPublisher.class);
ItemRegistry itemRegistry = getService(ItemRegistry.class);
CoreModuleHandlerFactory coreModuleHandlerFactory = new CoreModuleHandlerFactory(getBundleContext(),
Expand Down
5 changes: 5 additions & 0 deletions ....core.automation.tests/src/main/java/org/openhab/core/automation/event/RuleEventTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -78,12 +79,16 @@ public class RuleEventTest extends JavaOSGiTest {

private @Nullable Event itemEvent = null;
private @Nullable Event ruleRemovedEvent = null;
private @NonNullByDefault({}) StartLevelService startLevelService;

public RuleEventTest() {
}

@BeforeEach
public void before() {
startLevelService = mock(StartLevelService.class);
when(startLevelService.getStartLevel()).thenReturn(100);
registerService(startLevelService, StartLevelService.class.getName());
EventPublisher eventPublisher = getService(EventPublisher.class);
ItemRegistry itemRegistry = getService(ItemRegistry.class);
CoreModuleHandlerFactory coreModuleHandlerFactory = new CoreModuleHandlerFactory(getBundleContext(),
Expand Down