ISIS-960: handle exception for subscriber when invoked no-arg action. More robust exception handler strategy.
If a no-arg action is invoked which generates an exception in a subscriber, then the Wicket UI should detect this and re-render the original object with any messages generated. In addition, the exception handler strategy previously would only abort the transcation if a NonRecoverableException or a RecoverableException was thrown. This is too narrow; should abort the transaction for any exception thrown Project: http://git-wip-us.apache.org/repos/asf/isis/repo Commit: http://git-wip-us.apache.org/repos/asf/isis/commit/2aa0df30 Tree: http://git-wip-us.apache.org/repos/asf/isis/tree/2aa0df30 Diff: http://git-wip-us.apache.org/repos/asf/isis/diff/2aa0df30 Branch: refs/heads/master Commit: 2aa0df30a968d6cc03b029dad700e994b23da124 Parents: 2d1b44d Author: Dan Haywood <[email protected]> Authored: Fri Nov 28 15:59:12 2014 +0000 Committer: Dan Haywood <[email protected]> Committed: Fri Nov 28 16:03:18 2014 +0000 ---------------------------------------------------------------------- .../ui/components/actions/ActionPanel.java | 20 ++++++++--- .../eventbus/EventBusServiceDefault.java | 34 ++++++++++-------- .../integration/tests/ToDoItemIntegTest.java | 37 +++++++++----------- 3 files changed, 52 insertions(+), 39 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/isis/blob/2aa0df30/component/viewer/wicket/ui/src/main/java/org/apache/isis/viewer/wicket/ui/components/actions/ActionPanel.java ---------------------------------------------------------------------- diff --git a/component/viewer/wicket/ui/src/main/java/org/apache/isis/viewer/wicket/ui/components/actions/ActionPanel.java b/component/viewer/wicket/ui/src/main/java/org/apache/isis/viewer/wicket/ui/components/actions/ActionPanel.java index e2f85ca..b2959ff 100644 --- a/component/viewer/wicket/ui/src/main/java/org/apache/isis/viewer/wicket/ui/components/actions/ActionPanel.java +++ b/component/viewer/wicket/ui/src/main/java/org/apache/isis/viewer/wicket/ui/components/actions/ActionPanel.java @@ -20,15 +20,12 @@ package org.apache.isis.viewer.wicket.ui.components.actions; import java.util.List; - import com.google.common.base.Throwables; - import org.apache.wicket.ajax.AjaxRequestTarget; import org.apache.wicket.markup.html.WebMarkupContainer; import org.apache.wicket.markup.html.basic.Label; import org.apache.wicket.markup.html.form.Form; import org.apache.wicket.model.Model; - import org.apache.isis.applib.RecoverableException; import org.apache.isis.applib.services.command.Command; import org.apache.isis.applib.services.command.Command.Executor; @@ -104,7 +101,22 @@ public class ActionPanel extends PanelAbstract<ActionModel> implements ActionExe if (actionModel.hasParameters()) { buildGuiForParameters(); } else { - executeActionAndProcessResults(null, null); + + boolean succeeded = executeActionAndProcessResults(null, null); + if(succeeded) { + // nothing to do + } else { + + // render the target entity again + // + // (One way this can occur is if an event subscriber has a defect and throws an exception; in which case + // the EventBus' exception handler will automatically veto. This results in a growl message rather than + // an error page, but is probably 'good enough'). + final ObjectAdapter targetAdapter = actionModel.getTargetAdapter(); + + ActionResultResponse resultResponse = ActionResultResponseType.OBJECT.interpretResult(this.getActionModel(), targetAdapter, null); + resultResponse.getHandlingStrategy().handleResults(this, resultResponse); + } } } http://git-wip-us.apache.org/repos/asf/isis/blob/2aa0df30/core/runtime/src/main/java/org/apache/isis/core/runtime/services/eventbus/EventBusServiceDefault.java ---------------------------------------------------------------------- diff --git a/core/runtime/src/main/java/org/apache/isis/core/runtime/services/eventbus/EventBusServiceDefault.java b/core/runtime/src/main/java/org/apache/isis/core/runtime/services/eventbus/EventBusServiceDefault.java index 9b70ab8..1b3c826 100644 --- a/core/runtime/src/main/java/org/apache/isis/core/runtime/services/eventbus/EventBusServiceDefault.java +++ b/core/runtime/src/main/java/org/apache/isis/core/runtime/services/eventbus/EventBusServiceDefault.java @@ -16,17 +16,12 @@ */ package org.apache.isis.core.runtime.services.eventbus; -import java.util.List; - import javax.enterprise.context.RequestScoped; - -import com.google.common.base.Throwables; import com.google.common.eventbus.EventBus; import com.google.common.eventbus.SubscriberExceptionContext; import com.google.common.eventbus.SubscriberExceptionHandler; - -import org.apache.isis.applib.NonRecoverableException; -import org.apache.isis.applib.RecoverableException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.apache.isis.applib.services.eventbus.AbstractInteractionEvent; import org.apache.isis.applib.services.eventbus.AbstractInteractionEvent.Phase; import org.apache.isis.applib.services.eventbus.EventBusService; @@ -44,6 +39,8 @@ import org.apache.isis.core.runtime.system.transaction.IsisTransactionManager; @Deprecated public class EventBusServiceDefault extends EventBusService { + private static final Logger LOG = LoggerFactory.getLogger(EventBusServiceDefault.class); + /** * {@inheritDoc} * @@ -85,35 +82,44 @@ public class EventBusServiceDefault extends EventBusService { public void handleException(Throwable exception, SubscriberExceptionContext context) { Object event = context.getEvent(); if(!(event instanceof AbstractInteractionEvent)) { + if(LOG.isDebugEnabled()) { + LOG.debug("Ignoring exception '%s' (%s), not a subclass of AbstractInteractionEvent", exception.getMessage(), exception.getClass().getName()); + } return; } final AbstractInteractionEvent<?> interactionEvent = (AbstractInteractionEvent<?>) event; final Phase phase = interactionEvent.getPhase(); switch (phase) { case HIDE: + LOG.warn("Exception '%s' (%s) thrown during HIDE phase, to be safe will veto (hide) the interaction event"); interactionEvent.hide(); break; case DISABLE: + LOG.warn("Exception '%s' (%s) thrown during DISABLE phase, to be safe will veto (disable) the interaction event"); interactionEvent.disable(exception.getMessage()!=null?exception.getMessage(): exception.getClass().getName() + " thrown."); break; case VALIDATE: + LOG.warn("Exception '%s' (%s) thrown during VALIDATE phase, to be safe will veto (invalidate) the interaction event"); interactionEvent.invalidate(exception.getMessage()!=null?exception.getMessage(): exception.getClass().getName() + " thrown."); break; case EXECUTING: + LOG.warn("Exception '%s' (%s) thrown during EXECUTING phase, to be safe will abort the transaction"); + abortTransaction(exception); + break; case EXECUTED: - final List<Throwable> causalChain = Throwables.getCausalChain(exception); - for (Throwable cause : causalChain) { - if(cause instanceof RecoverableException || cause instanceof NonRecoverableException) { - getTransactionManager().getTransaction().setAbortCause(new IsisApplicationException(exception)); - return; - } - } + LOG.warn("Exception '%s' (%s) thrown during EXECUTED phase, to be safe will abort the transaction"); + abortTransaction(exception); break; } } }; } + private void abortTransaction(Throwable exception) { + getTransactionManager().getTransaction().setAbortCause(new IsisApplicationException(exception)); + return; + } + protected IsisTransactionManager getTransactionManager() { return IsisContext.getTransactionManager(); } http://git-wip-us.apache.org/repos/asf/isis/blob/2aa0df30/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemIntegTest.java ---------------------------------------------------------------------- diff --git a/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemIntegTest.java b/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemIntegTest.java index 8e961da..7b289d2 100644 --- a/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemIntegTest.java +++ b/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemIntegTest.java @@ -58,7 +58,6 @@ import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.CoreMatchers.nullValue; import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; public class ToDoItemIntegTest extends AbstractToDoIntegTest { @@ -266,17 +265,16 @@ public class ToDoItemIntegTest extends AbstractToDoIntegTest { } @Test - public void subscriberThrowingOtherExceptionIsIgnored() throws Exception { + public void subscriberVetoesEventWithAnyOtherException() throws Exception { // given toDoItemSubscriptions.subscriberBehaviour(ToDoItemSubscriptions.Behaviour.AnyExecuteVetoWithOtherException); + // then + expectedExceptions.expect(RuntimeException.class); + // when toDoItem.completed(); - - // then - // (no expectedExceptions setup, expect to continue) - assertTrue(true); } } @@ -569,17 +567,16 @@ public class ToDoItemIntegTest extends AbstractToDoIntegTest { } @Test - public void subscriberThrowingOtherExceptionIsIgnored() throws Exception { + public void subscriberVetoesEventWithAnyOtherException() throws Exception { // given toDoItemSubscriptions.subscriberBehaviour(ToDoItemSubscriptions.Behaviour.AnyExecuteVetoWithOtherException); + // then + expectedExceptions.expect(RuntimeException.class); + // when toDoItem.add(otherToDoItem); - - // then - // (no expectedExceptions setup, expect to continue) - assertTrue(true); } } public static class Remove extends ToDoItemIntegTest { @@ -667,17 +664,16 @@ public class ToDoItemIntegTest extends AbstractToDoIntegTest { } @Test - public void subscriberThrowingOtherExceptionIsIgnored() throws Exception { + public void subscriberVetoesEventWithAnyOtherException() throws Exception { // given toDoItemSubscriptions.subscriberBehaviour(ToDoItemSubscriptions.Behaviour.AnyExecuteVetoWithOtherException); + // then + expectedExceptions.expect(RuntimeException.class); + // when toDoItem.remove(otherToDoItem); - - // then - // (no expectedExceptions setup, expect to continue) - assertTrue(true); } } } @@ -908,17 +904,16 @@ public class ToDoItemIntegTest extends AbstractToDoIntegTest { @Test - public void subscriberThrowingOtherExceptionIsIgnored() throws Exception { + public void subscriberVetoesEventWithAnyOtherException() throws Exception { // given toDoItemSubscriptions.subscriberBehaviour(ToDoItemSubscriptions.Behaviour.AnyExecuteVetoWithOtherException); + // then + expectedExceptions.expect(RuntimeException.class); + // when toDoItem.setDescription("Buy bread and butter"); - - // then - // (no expectedExceptions setup, expect to continue) - assertTrue(true); }
