[JBPM-9969] failure to create AsyncSignalEventCommand through REST API #2074
Conversation
There was a problem hiding this comment.
Looks good to me, just minor comments in the test and a proposal for making more robust the code (proper IllegalArgumentException thrown) in case processInstanceId is not present.
It could potentially fail in other points (with REST and json, as it is not type-safe when obtaining the processInstanceId):
| @@ -48,10 +48,9 @@ public ExecutionResults execute(CommandContext ctx) throws Exception { | |||
| if (runtimeManager == null) { | |||
| throw new IllegalArgumentException("No runtime manager found for deployment id " + deploymentId); | |||
| } | |||
| RuntimeEngine engine = runtimeManager.getRuntimeEngine(ProcessInstanceIdContext.get(processInstanceId)); | |||
| RuntimeEngine engine = runtimeManager.getRuntimeEngine(ProcessInstanceIdContext.get(processInstanceId.longValue())); | |||
There was a problem hiding this comment.
If the context doesn't contain processInstanceId, it will throw a NPE.
Could the code be protected including also the processInstanceId as required, same as deploymentId and signal in the previous "if" comparison, wdyt?
| public void testAsyncSignalEventInteger1Command() throws Exception { | ||
| AsyncSignalEventCommand command = new AsyncSignalEventCommand(); | ||
| CommandContext ctx = new CommandContext(); | ||
| ctx.setData("processInstanceId", new Integer(2)); |
There was a problem hiding this comment.
Deprecated constructor
| ctx.setData("processInstanceId", new Integer(2)); | |
| ctx.setData("processInstanceId", Integer.valueOf(2)); |
| public void testAsyncSignalEventInteger2Command() throws Exception { | ||
| AsyncSignalEventCommand command = new AsyncSignalEventCommand(); | ||
| CommandContext ctx = new CommandContext(); | ||
| ctx.setData("ProcessInstanceId", new Integer(2)); |
| public void testAsyncSignalEventLong1Command() throws Exception { | ||
| AsyncSignalEventCommand command = new AsyncSignalEventCommand(); | ||
| CommandContext ctx = new CommandContext(); | ||
| ctx.setData("processInstanceId", new Long(2L)); |
| public void testAsyncSignalEventLong2Command() throws Exception { | ||
| AsyncSignalEventCommand command = new AsyncSignalEventCommand(); | ||
| CommandContext ctx = new CommandContext(); | ||
| ctx.setData("ProcessInstanceId", new Long(2L)); |
|
|
||
| public class AsyncSignalEventCommandTest { | ||
|
|
||
| // check we don't get NullPointer |
There was a problem hiding this comment.
ClassCastException, and you could add for clarification "when processInstanceId comes from a REST call with json (non type-safe)"
elguardian
force-pushed
the
JBPM-9969
branch
from
9128f74
to
adde05e
Compare
elguardian
force-pushed
the
JBPM-9969
branch
from
adde05e
to
057873b
Compare
There was a problem hiding this comment.
Looks good to me, awesome work @elguardian !
Just waiting for CI to be green
|
SonarCloud Quality Gate failed. |
Jira: https://issues.redhat.com/browse/JBPM-9969