You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@isis.apache.org by da...@apache.org on 2014/10/07 09:40:18 UTC
[5/5] git commit: ISIS-865: further fixes for 'ensureSafeSemantics';
also ensure that integration tests' command are set to be executed as
"USER" (rather than "OTHER").
ISIS-865: further fixes for 'ensureSafeSemantics'; also ensure that integration tests' command are set to be executed as "USER" (rather than "OTHER").
The further fixes for 'ensureSafeSemantics' are mostly comments; the actual fix was in the todo integration tests that were failing because of a missing call to 'nextTransaction()'. There is a bit more 'belt-n-braces' resetting to null:
- of the command's event (at xactn completion)
- of the xactn's changedProperties hash (at xactn completion)
Project: http://git-wip-us.apache.org/repos/asf/isis/repo
Commit: http://git-wip-us.apache.org/repos/asf/isis/commit/52c18eb8
Tree: http://git-wip-us.apache.org/repos/asf/isis/tree/52c18eb8
Diff: http://git-wip-us.apache.org/repos/asf/isis/diff/52c18eb8
Branch: refs/heads/master
Commit: 52c18eb8b31fde5bb977bfca72ddeb323413e0be
Parents: 7f1db56
Author: Dan Haywood <da...@haywood-associates.co.uk>
Authored: Tue Oct 7 08:22:07 2014 +0100
Committer: Dan Haywood <da...@haywood-associates.co.uk>
Committed: Tue Oct 7 08:30:26 2014 +0100
----------------------------------------------------------------------
.../IntegrationTestAbstract.java | 34 ++++++-
.../integtestsupport/IsisSystemForTest.java | 27 +++--
.../metamodel/facets/InteractionHelper.java | 20 ++--
...onInvocationFacetForInteractionAbstract.java | 11 +-
.../system/transaction/IsisTransaction.java | 101 ++++++++++++-------
.../java/integration/ToDoSystemInitializer.java | 4 +
.../integration/tests/ToDoItemIntegTest.java | 1 +
.../integration/tests/ToDoItemsIntegTest.java | 12 ++-
8 files changed, 140 insertions(+), 70 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/isis/blob/52c18eb8/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IntegrationTestAbstract.java
----------------------------------------------------------------------
diff --git a/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IntegrationTestAbstract.java b/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IntegrationTestAbstract.java
index ef743ce..89b422d 100644
--- a/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IntegrationTestAbstract.java
+++ b/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IntegrationTestAbstract.java
@@ -62,20 +62,44 @@ public abstract class IntegrationTestAbstract {
/**
* Intended to be called whenever there is a logically distinct interaction
* with the system.
- *
+ *
* <p>
- * Simply {@link ScenarioExecution#endTran(boolean) ends any existing transaction} and
- * then {@link ScenarioExecution#beginTran() starts a new one}.
+ * Each transaction has its own instances of request-scoped services, most notably
+ * the {@link org.apache.isis.applib.services.command.Command}.
+ * </p>
*
* <p>
- * Typically there is no need to call this method, because (thanks to
- * {@link IsisTransactionRule}) every test is called within its own transaction.
+ * (Unlike {@link #nextSession()}), it <i>is</i> valid to hold references to objects across transactions.
+ * </p>
+ *
+ * @see #nextRequest()
+ * @see #nextSession()
*/
protected void nextTransaction() {
scenarioExecution().endTran(true);
scenarioExecution().beginTran();
}
+ /**
+ * Synonym for {@link #nextTransaction()}.
+ *
+ * @see #nextTransaction()
+ * @see #nextSession()
+ */
+ protected void nextRequest() {
+ nextTransaction();
+ }
+
+ /**
+ * Completes the transaction and session, then opens another session and transaction.
+ *
+ * <p>
+ * Note that any references to objects must be discarded and reacquired.
+ * </p>
+ *
+ * @see #nextTransaction()
+ * @see #nextRequest()
+ */
protected void nextSession() {
scenarioExecution().endTran(true);
scenarioExecution().closeSession();
http://git-wip-us.apache.org/repos/asf/isis/blob/52c18eb8/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IsisSystemForTest.java
----------------------------------------------------------------------
diff --git a/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IsisSystemForTest.java b/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IsisSystemForTest.java
index a8b22ff..b4976ff 100644
--- a/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IsisSystemForTest.java
+++ b/core/integtestsupport/src/main/java/org/apache/isis/core/integtestsupport/IsisSystemForTest.java
@@ -21,18 +21,17 @@ package org.apache.isis.core.integtestsupport;
import java.util.Arrays;
import java.util.List;
-
import com.google.common.collect.Lists;
-
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.runner.Description;
import org.junit.runners.model.Statement;
-
import org.apache.isis.applib.DomainObjectContainer;
import org.apache.isis.applib.fixtures.FixtureClock;
import org.apache.isis.applib.fixtures.InstallableFixture;
+import org.apache.isis.applib.services.command.Command;
+import org.apache.isis.applib.services.command.CommandContext;
import org.apache.isis.core.commons.authentication.AuthenticationSession;
import org.apache.isis.core.commons.config.IsisConfiguration;
import org.apache.isis.core.metamodel.adapter.ObjectAdapter;
@@ -341,9 +340,10 @@ public class IsisSystemForTest implements org.junit.rules.TestRule, DomainServic
final AuthenticationManager authenticationManager = isisSystem.getSessionFactory().getAuthenticationManager();
authenticationSession = authenticationManager.authenticate(authenticationRequest);
- IsisContext.openSession(authenticationSession);
setContainer(getContainer());
-
+
+ openSession();
+
wireAndInstallFixtures();
if(fireListeners.shouldFire()) {
firePostSetupSystem(firstTime);
@@ -399,6 +399,7 @@ public class IsisSystemForTest implements org.junit.rules.TestRule, DomainServic
public void openSession() throws Exception {
openSession(authenticationSession);
+
}
public void openSession(AuthenticationSession authenticationSession) throws Exception {
@@ -619,7 +620,7 @@ public class IsisSystemForTest implements org.junit.rules.TestRule, DomainServic
final IsisTransaction transaction = transactionManager.getTransaction();
if(transaction == null) {
- transactionManager.startTransaction();
+ startTransactionForUser(transactionManager);
return;
}
@@ -627,7 +628,7 @@ public class IsisSystemForTest implements org.junit.rules.TestRule, DomainServic
switch(state) {
case COMMITTED:
case ABORTED:
- transactionManager.startTransaction();
+ startTransactionForUser(transactionManager);
break;
case IN_PROGRESS:
// nothing to do
@@ -641,6 +642,18 @@ public class IsisSystemForTest implements org.junit.rules.TestRule, DomainServic
}
+ private void startTransactionForUser(IsisTransactionManager transactionManager) {
+ transactionManager.startTransaction();
+
+ // specify that this command (if any) is being executed by a 'USER'
+ final CommandContext commandContext = getService(CommandContext.class);
+ Command command;
+ if (commandContext != null) {
+ command = commandContext.getCommand();
+ command.setExecutor(Command.Executor.USER);
+ }
+ }
+
/**
* Either commits or aborts the transaction, depending on the Transaction's {@link org.apache.isis.core.runtime.system.transaction.IsisTransaction#getState()}
*/
http://git-wip-us.apache.org/repos/asf/isis/blob/52c18eb8/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/InteractionHelper.java
----------------------------------------------------------------------
diff --git a/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/InteractionHelper.java b/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/InteractionHelper.java
index a84cd6b..d0d97ef 100644
--- a/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/InteractionHelper.java
+++ b/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/InteractionHelper.java
@@ -63,23 +63,23 @@ public class InteractionHelper {
final Object[] arguments = ObjectAdapter.Util.unwrap(argumentAdapters);
event.setArguments(Arrays.asList(arguments));
if(phase.isExecutingOrLater()) {
+
+ // current event always references the command (originally created by the xactn)
event.setCommand(command);
if(command != null && command instanceof Command2) {
final Command2 command2 = (Command2) command;
// don't overwrite any existing event.
- // this can happen when a bulk action is performed.
-
- // if the last action invoked returns null (or is void)
- // then the Wicket viewer's BulkActionsLinkFactory will attempt to
- // re-run the action that generated the list.
//
- // we don't want to overwrite the event; it would be valid to invoke
- // an action that is non-idempotent, then run a query afterwards.
- // but if the event is overwritten, then will trip up the
- // 'ensureSafeSemantics' check at the end of the xactn.
+ // this can happen when one action invokes another (wrapped), and can also occur if a
+ // bulk action is performed (when the query that produced the list is automatically re-executed).
+ // (This logic is in the Wicket viewer's BulkActionsLinkFactory).
+ //
+ // This also means that the command could refer to a different event (the one of the original
+ // outer-most action that started the xactn) compared to the event that references it.
- if(command2.getActionInteractionEvent() == null) {
+ final ActionInteractionEvent<?> actionInteractionEvent = command2.getActionInteractionEvent();
+ if(actionInteractionEvent == null) {
command2.setActionInteractionEvent(event);
}
}
http://git-wip-us.apache.org/repos/asf/isis/blob/52c18eb8/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/actions/interaction/ActionInvocationFacetForInteractionAbstract.java
----------------------------------------------------------------------
diff --git a/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/actions/interaction/ActionInvocationFacetForInteractionAbstract.java b/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/actions/interaction/ActionInvocationFacetForInteractionAbstract.java
index 57357be..4855f98 100644
--- a/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/actions/interaction/ActionInvocationFacetForInteractionAbstract.java
+++ b/core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/actions/interaction/ActionInvocationFacetForInteractionAbstract.java
@@ -185,7 +185,7 @@ public abstract class ActionInvocationFacetForInteractionAbstract
owningAction, targetAdapter, arguments);
// ... invoke the action
- final InvocationResult invocationResult = internalInvoke(owningAction, targetAdapter, arguments);
+ final InvocationResult invocationResult = internalInvoke(command, owningAction, targetAdapter, arguments);
// ... post the executed event
if (invocationResult.getWhetherInvoked()) {
@@ -200,7 +200,11 @@ public abstract class ActionInvocationFacetForInteractionAbstract
} finally {
- // clean up
+ // clean up event on thread
+
+ // note that if the action invoked some other action via a wrapper then this will already have been cleared
+ // out. That isn't a problem; we only need to pass the event from validate to pre-execute, not to formally
+ // "nest" the events.
actionInteractionFacet.currentInteraction.set(null);
}
}
@@ -214,13 +218,12 @@ public abstract class ActionInvocationFacetForInteractionAbstract
}
protected InvocationResult internalInvoke(
+ final Command command,
final ObjectAction owningAction,
final ObjectAdapter targetAdapter,
final ObjectAdapter[] arguments) {
final Bulk.InteractionContext bulkInteractionContext = getServicesInjector().lookupService(Bulk.InteractionContext.class);
- final CommandContext commandContext = getServicesInjector().lookupService(CommandContext.class);
- final Command command = commandContext != null ? commandContext.getCommand() : null;
try {
final Object[] executionParameters = new Object[arguments.length];
http://git-wip-us.apache.org/repos/asf/isis/blob/52c18eb8/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java
----------------------------------------------------------------------
diff --git a/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java b/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java
index 628045e..9060315 100644
--- a/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java
+++ b/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java
@@ -50,6 +50,7 @@ import org.apache.isis.applib.services.eventbus.ActionInteractionEvent;
import org.apache.isis.applib.services.publish.*;
import org.apache.isis.core.commons.authentication.AuthenticationSession;
import org.apache.isis.core.commons.components.TransactionScopedComponent;
+import org.apache.isis.core.commons.config.IsisConfiguration;
import org.apache.isis.core.commons.ensure.Ensure;
import org.apache.isis.core.commons.exceptions.IsisException;
import org.apache.isis.core.commons.util.ToString;
@@ -519,15 +520,21 @@ public class IsisTransaction implements TransactionScopedComponent {
}
protected void doAudit(final Set<Entry<AdapterAndProperty, PreAndPostValues>> changedObjectProperties) {
- if(auditingService3 == null) {
- return;
- }
-
- // else
- final String currentUser = getTransactionManager().getAuthenticationSession().getUserName();
- final java.sql.Timestamp currentTime = Clock.getTimeAsJavaSqlTimestamp();
- for (Entry<AdapterAndProperty, PreAndPostValues> auditEntry : changedObjectProperties) {
- auditChangedProperty(currentTime, currentUser, auditEntry);
+ try {
+ if(auditingService3 == null) {
+ return;
+ }
+
+ // else
+ final String currentUser = getTransactionManager().getAuthenticationSession().getUserName();
+ final java.sql.Timestamp currentTime = Clock.getTimeAsJavaSqlTimestamp();
+ for (Entry<AdapterAndProperty, PreAndPostValues> auditEntry : changedObjectProperties) {
+ auditChangedProperty(currentTime, currentUser, auditEntry);
+ }
+
+ } finally {
+ // not needed in production, but is required for integration testing
+ this.changedObjectProperties.clear();
}
}
@@ -742,37 +749,46 @@ public class IsisTransaction implements TransactionScopedComponent {
if (event == null) {
return;
}
- final ActionSemantics.Of actionSemantics = event.getActionSemantics();
- if(actionSemantics == null) {
- return;
- }
- if (!actionSemantics.isSafe()) {
- return;
- }
+ try {
+ final ActionSemantics.Of actionSemantics = event.getActionSemantics();
+ if(actionSemantics == null) {
+ return;
+ }
+ if (!actionSemantics.isSafe()) {
+ return;
+ }
- final Set<ObjectAdapter> changedAdapters = findChangedAdapters(changedObjectProperties);
- if(!changedAdapters.isEmpty()) {
- final String msg = "Action '" + event.getIdentifier().toFullIdentityString() + "'" +
- " (with safe semantics)" +
- " caused " + changedAdapters.size() + " object" + (changedAdapters.size() != 1 ? "s" : "") +
- " to be modified";
- LOG.error(msg);
- for (ObjectAdapter changedAdapter : changedAdapters) {
- final StringBuilder builder = new StringBuilder(" > ")
- .append(changedAdapter.getSpecification().getFullIdentifier())
- .append(": ");
- if(!changedAdapter.isDestroyed()) {
- builder.append(changedAdapter.titleString(null));
- } else {
- builder.append("(deleted object)");
+ final Set<ObjectAdapter> changedAdapters = findChangedAdapters(changedObjectProperties);
+ if(!changedAdapters.isEmpty()) {
+ final String msg = "Action '" + event.getIdentifier().toFullIdentityString() + "'" +
+ " (with safe semantics)" +
+ " caused " + changedAdapters.size() + " object" + (changedAdapters.size() != 1 ? "s" : "") +
+ " to be modified";
+ LOG.error(msg);
+ for (ObjectAdapter changedAdapter : changedAdapters) {
+ final StringBuilder builder = new StringBuilder(" > ")
+ .append(changedAdapter.getSpecification().getFullIdentifier())
+ .append(": ");
+ if(!changedAdapter.isDestroyed()) {
+ builder.append(changedAdapter.titleString(null));
+ } else {
+ builder.append("(deleted object)");
+ }
+ LOG.error(builder.toString());
}
- LOG.error(builder.toString());
- }
- final boolean enforceSafeSemantics = IsisContext.getConfiguration().getBoolean(PersistenceConstants.ENFORCE_SAFE_SEMANTICS, PersistenceConstants.ENFORCE_SAFE_SEMANTICS_DEFAULT);
- if(enforceSafeSemantics) {
- throw new RecoverableException(msg);
+ final boolean enforceSafeSemantics = getConfiguration().getBoolean(PersistenceConstants.ENFORCE_SAFE_SEMANTICS, PersistenceConstants.ENFORCE_SAFE_SEMANTICS_DEFAULT);
+ if(enforceSafeSemantics) {
+ throw new RecoverableException(msg);
+ }
}
+ } finally {
+ // irrespective of outcome, clear out the event.
+ // this has no effect in the production app, but benefits integration tests
+ // where otherwise the event from a preceding xactn (eg the action in the 'when') can "leak" into
+ // pertaining to the next phase (eg the assertions in the 'then').
+
+ command2.setActionInteractionEvent(null);
}
}
@@ -787,8 +803,8 @@ public class IsisTransaction implements TransactionScopedComponent {
}
- private void preCommitServices(final Set<Entry<AdapterAndProperty, PreAndPostValues>> changedObjectProperties1) {
- doAudit(changedObjectProperties1);
+ private void preCommitServices(final Set<Entry<AdapterAndProperty, PreAndPostValues>> changedObjectProperties) {
+ doAudit(changedObjectProperties);
final String currentUser = getTransactionManager().getAuthenticationSession().getUserName();
final Timestamp endTimestamp = Clock.getTimeAsJavaSqlTimestamp();
@@ -838,6 +854,11 @@ public class IsisTransaction implements TransactionScopedComponent {
if(commandService != null) {
final Command command = commandContext.getCommand();
commandService.complete(command);
+
+ if(command instanceof Command2) {
+ final Command2 command2 = (Command2) command;
+ command2.setActionInteractionEvent(null);
+ }
}
}
}
@@ -1301,7 +1322,9 @@ public class IsisTransaction implements TransactionScopedComponent {
return IsisContext.getOidMarshaller();
}
+ protected IsisConfiguration getConfiguration() {
+ return IsisContext.getConfiguration();
+ }
-
}
http://git-wip-us.apache.org/repos/asf/isis/blob/52c18eb8/example/application/todoapp/integtests/src/test/java/integration/ToDoSystemInitializer.java
----------------------------------------------------------------------
diff --git a/example/application/todoapp/integtests/src/test/java/integration/ToDoSystemInitializer.java b/example/application/todoapp/integtests/src/test/java/integration/ToDoSystemInitializer.java
index 4f2e933..176c94e 100644
--- a/example/application/todoapp/integtests/src/test/java/integration/ToDoSystemInitializer.java
+++ b/example/application/todoapp/integtests/src/test/java/integration/ToDoSystemInitializer.java
@@ -66,7 +66,11 @@ public class ToDoSystemInitializer {
testConfiguration.addRegisterEntitiesPackagePrefix("dom");
// enable stricter checking
+ //
+ // the consequence of this is having to call 'nextTransaction()' between most of the given/when/then's
+ // because the command2 only ever refers to the event of the originating action.
testConfiguration.put(PersistenceConstants.ENFORCE_SAFE_SEMANTICS, "true");
+
return testConfiguration;
}
}
http://git-wip-us.apache.org/repos/asf/isis/blob/52c18eb8/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 223471e..9b94ab0 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
@@ -82,6 +82,7 @@ public class ToDoItemIntegTest extends AbstractToDoIntegTest {
@Before
public void setUp() throws Exception {
+ super.setUp();
final List<ToDoItem> all = wrap(toDoItems).notYetComplete();
toDoItem = wrap(all.get(0));
nextTransaction();
http://git-wip-us.apache.org/repos/asf/isis/blob/52c18eb8/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemsIntegTest.java
----------------------------------------------------------------------
diff --git a/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemsIntegTest.java b/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemsIntegTest.java
index cf5cb86..bdd84cb 100644
--- a/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemsIntegTest.java
+++ b/example/application/todoapp/integtests/src/test/java/integration/tests/ToDoItemsIntegTest.java
@@ -61,24 +61,26 @@ public class ToDoItemsIntegTest extends AbstractToDoIntegTest {
public void complete_and_notYetComplete() throws Exception {
// given
- List<ToDoItem> notYetCompleteItems = wrap(service(ToDoItems.class)).notYetComplete();
+ List<ToDoItem> notYetCompleteItems = wrap(toDoItems).notYetComplete();
final ToDoItem toDoItem = wrap(notYetCompleteItems.get(0));
+ nextTransaction();
// when
toDoItem.completed();
nextTransaction();
// then
- assertThat(wrap(service(ToDoItems.class)).notYetComplete().size(), is(notYetCompletedSize-1));
- assertThat(wrap(service(ToDoItems.class)).complete().size(), is(completedSize+1));
+ assertThat(wrap(toDoItems).notYetComplete().size(), is(notYetCompletedSize-1));
+ assertThat(wrap(toDoItems).complete().size(), is(completedSize+1));
+ nextTransaction();
// and when
toDoItem.notYetCompleted();
nextTransaction();
// then
- assertThat(wrap(service(ToDoItems.class)).notYetComplete().size(), is(notYetCompletedSize));
- assertThat(wrap(service(ToDoItems.class)).complete().size(), is(completedSize));
+ assertThat(wrap(toDoItems).notYetComplete().size(), is(notYetCompletedSize));
+ assertThat(wrap(toDoItems).complete().size(), is(completedSize));
}
}