From bd7e0141ea9c1f92d0183bc7b02f8e6bfacbd443 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Sat, 29 Oct 2016 08:48:53 +0100 Subject: [PATCH] Fix interceptor handling - Modify so that interceptor `preStateChange` is not called for transient states which would falsely try to i.e. persist choice and other states with persist recipe. - Added some tests to verify that `preStateChange` is called only once. - Relates to #266 --- .../support/AbstractStateMachine.java | 3 +- .../support/StateMachineUtils.java | 29 ++++ .../support/StateChangeInterceptorTests.java | 131 ++++++++++++++++++ .../PersistStateMachineHandlerTests.java | 51 +++++++ 4 files changed, 213 insertions(+), 1 deletion(-) diff --git a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java index 21e071de..7567ee67 100644 --- a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java +++ b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java @@ -766,7 +766,8 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo } private void switchToState(State state, Message message, Transition transition, StateMachine stateMachine) { - if (!isInitialTransition(transition) && !callPreStateChangeInterceptors(state, message, transition, stateMachine)) { + if (!isInitialTransition(transition) && !StateMachineUtils.isTransientPseudoState(state) + && !callPreStateChangeInterceptors(state, message, transition, stateMachine)) { return; } diff --git a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/StateMachineUtils.java b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/StateMachineUtils.java index 3e669d0e..c2a2f409 100644 --- a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/StateMachineUtils.java +++ b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/StateMachineUtils.java @@ -95,6 +95,35 @@ public abstract class StateMachineUtils { } } + /** + * Checks if state is a transient pseudo state, meaning it is a pseudostate + * and its kind indicates that machine will never stay on this state after + * run to completion has finished. + * + * @param the type of state + * @param the type of event + * @param state the state + * @return true, if is normal pseudo state + */ + public static boolean isTransientPseudoState(State state) { + if (state == null) { + return false; + } + PseudoState pseudoState = state.getPseudoState(); + if (pseudoState == null) { + return false; + } + PseudoStateKind kind = pseudoState.getKind(); + if (kind == PseudoStateKind.CHOICE || kind == PseudoStateKind.JUNCTION || kind == PseudoStateKind.ENTRY + || kind == PseudoStateKind.EXIT || kind == PseudoStateKind.HISTORY_DEEP + || kind == PseudoStateKind.HISTORY_SHALLOW || kind == PseudoStateKind.FORK + || kind == PseudoStateKind.JOIN) { + return true; + } else { + return false; + } + } + public static boolean isPseudoState(State state, PseudoStateKind kind) { if (state != null) { PseudoState pseudoState = state.getPseudoState(); diff --git a/spring-statemachine-core/src/test/java/org/springframework/statemachine/support/StateChangeInterceptorTests.java b/spring-statemachine-core/src/test/java/org/springframework/statemachine/support/StateChangeInterceptorTests.java index c851eff7..79dde986 100644 --- a/spring-statemachine-core/src/test/java/org/springframework/statemachine/support/StateChangeInterceptorTests.java +++ b/spring-statemachine-core/src/test/java/org/springframework/statemachine/support/StateChangeInterceptorTests.java @@ -89,6 +89,81 @@ public class StateChangeInterceptorTests extends AbstractStateMachineTests { assertThat((Integer)machine.getExtendedState().getVariables().get("foo"), is(1)); } + @Test + public void testIntercept2() throws InterruptedException { + context.register(Config2.class); + context.refresh(); + @SuppressWarnings("unchecked") + StateMachine machine = context.getBean(StateMachineSystemConstants.DEFAULT_ID_STATEMACHINE, StateMachine.class); + TestListener listener = new TestListener(); + machine.addStateListener(listener); + TestStateChangeInterceptor interceptor = new TestStateChangeInterceptor(); + + machine.getStateMachineAccessor().doWithRegion(new StateMachineFunction>() { + + @Override + public void apply(StateMachineAccess function) { + function.addStateMachineInterceptor(interceptor); + } + }); + + machine.start(); + assertThat(listener.stateChangedLatch.await(2, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateChangedCount, is(1)); + assertThat(machine.getState().getIds(), containsInAnyOrder(States.S0)); + + interceptor.reset(1); + listener.reset(1); + machine.sendEvent(Events.A); + assertThat(listener.stateChangedLatch.await(2, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateChangedCount, is(1)); + assertThat(machine.getState().getIds(), containsInAnyOrder(States.S1)); + assertThat(interceptor.preStateChangeLatch.await(2, TimeUnit.SECONDS), is(true)); + assertThat(interceptor.preStateChangeCount, is(1)); + + interceptor.reset(1); + listener.reset(1); + machine.sendEvent(Events.B); + assertThat(listener.stateChangedLatch.await(2, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateChangedCount, is(1)); + assertThat(machine.getState().getIds(), containsInAnyOrder(States.S2)); + assertThat(interceptor.preStateChangeLatch.await(2, TimeUnit.SECONDS), is(true)); + assertThat(interceptor.preStateChangeCount, is(1)); + } + + @Test + public void testIntercept3() throws InterruptedException { + context.register(Config3.class); + context.refresh(); + @SuppressWarnings("unchecked") + StateMachine machine = context.getBean(StateMachineSystemConstants.DEFAULT_ID_STATEMACHINE, StateMachine.class); + TestListener listener = new TestListener(); + machine.addStateListener(listener); + TestStateChangeInterceptor interceptor = new TestStateChangeInterceptor(); + + machine.getStateMachineAccessor().doWithRegion(new StateMachineFunction>() { + + @Override + public void apply(StateMachineAccess function) { + function.addStateMachineInterceptor(interceptor); + } + }); + + machine.start(); + assertThat(listener.stateChangedLatch.await(2, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateChangedCount, is(1)); + assertThat(machine.getState().getIds(), containsInAnyOrder(States.S0)); + + interceptor.reset(1); + listener.reset(1); + machine.sendEvent(Events.A); + assertThat(listener.stateChangedLatch.await(2, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateChangedCount, is(1)); + assertThat(machine.getState().getIds(), containsInAnyOrder(States.S2)); + assertThat(interceptor.preStateChangeLatch.await(2, TimeUnit.SECONDS), is(true)); + assertThat(interceptor.preStateChangeCount, is(1)); + } + @Configuration @EnableStateMachine static class Config1 extends EnumStateMachineConfigurerAdapter { @@ -210,6 +285,62 @@ public class StateChangeInterceptorTests extends AbstractStateMachineTests { } + @Configuration + @EnableStateMachine + static class Config2 extends EnumStateMachineConfigurerAdapter { + + @Override + public void configure(StateMachineStateConfigurer states) + throws Exception { + states + .withStates() + .initial(States.S0) + .state(States.S1) + .state(States.S2); + } + + @Override + public void configure(StateMachineTransitionConfigurer transitions) + throws Exception { + transitions + .withExternal() + .source(States.S0).target(States.S1) + .event(Events.A) + .and() + .withExternal() + .source(States.S1).target(States.S2) + .event(Events.B); + } + } + + @Configuration + @EnableStateMachine + static class Config3 extends EnumStateMachineConfigurerAdapter { + + @Override + public void configure(StateMachineStateConfigurer states) + throws Exception { + states + .withStates() + .initial(States.S0) + .choice(States.S1) + .state(States.S2); + } + + @Override + public void configure(StateMachineTransitionConfigurer transitions) + throws Exception { + transitions + .withExternal() + .source(States.S0).target(States.S1) + .event(Events.A) + .and() + .withChoice() + .source(States.S1) + .last(States.S2); + } + } + public static enum States { S0, S1, S11, S12, S2, S21, S211, S212 } diff --git a/spring-statemachine-recipes/src/test/java/org/springframework/statemachine/recipes/PersistStateMachineHandlerTests.java b/spring-statemachine-recipes/src/test/java/org/springframework/statemachine/recipes/PersistStateMachineHandlerTests.java index aeee68be..3d6ce63f 100644 --- a/spring-statemachine-recipes/src/test/java/org/springframework/statemachine/recipes/PersistStateMachineHandlerTests.java +++ b/spring-statemachine-recipes/src/test/java/org/springframework/statemachine/recipes/PersistStateMachineHandlerTests.java @@ -19,6 +19,8 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertThat; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -71,13 +73,36 @@ public class PersistStateMachineHandlerTests { assertThat(stateMachine.getState().getIds(), containsInAnyOrder("S1")); } + @Test + public void testChoice() throws Exception { + StateMachine stateMachine = buildTestStateMachine2(); + + PersistStateMachineHandler handler = new PersistStateMachineHandler(stateMachine); + handler.afterPropertiesSet(); + handler.start(); + + TestPersistStateChangeListener listener = new TestPersistStateChangeListener(); + handler.addPersistStateChangeListener(listener); + + Message event = MessageBuilder.withPayload("E1").build(); + + boolean accepted = handler.handleEventWithState(event, "SI"); + assertThat(accepted, is(true)); + assertThat(listener.latch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.states.size(), is(1)); + assertThat(listener.states.get(0).getId(), is("S2")); + assertThat(stateMachine.getState().getIds(), containsInAnyOrder("S2")); + } + private static class TestPersistStateChangeListener implements PersistStateChangeListener { CountDownLatch latch = new CountDownLatch(1); + List> states = new ArrayList<>(); @Override public void onPersist(State state, Message message, Transition transition, StateMachine stateMachine) { + states.add(state); latch.countDown(); } @@ -108,4 +133,30 @@ public class PersistStateMachineHandlerTests { return builder.build(); } + private static StateMachine buildTestStateMachine2() + throws Exception { + StateMachineBuilder.Builder builder = StateMachineBuilder.builder(); + + builder.configureConfiguration() + .withConfiguration() + .taskExecutor(new SyncTaskExecutor()) + .autoStartup(true); + + builder.configureStates() + .withStates() + .initial("SI") + .choice("S1") + .state("S2") + .state("S3"); + + builder.configureTransitions() + .withExternal() + .source("SI").target("S1").event("E1") + .and() + .withChoice() + .source("S1") + .last("S2"); + + return builder.build(); + } }