Skip to content

Commit

Permalink
refactor(engine): Code Review points
Browse files Browse the repository at this point in the history 
- Remove execute method from FetchAndLockBuilder
- Refactor FetchAndLockBuilderImpl to use a List instead of a Map and make it similar to the QueryAPI for the sake of consistency
- Add tests for covering invalid usages of fetchAndLock API
  • Loading branch information
@psavidis
psavidis committed Nov 30, 2023
1 parent 11ff757 commit 3818ff3
Show file tree
Hide file tree
Showing 9 changed files with 104 additions and 97 deletions.
14 changes: 7 additions & 7 deletions ...est/src/main/java/org/camunda/bpm/engine/rest/dto/externaltask/FetchExternalTasksDto.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,19 +203,19 @@ public void setIncludeExtensionProperties(boolean includeExtensionProperties) {
}
}

public FetchAndLockBuilder buildQuery(ProcessEngine processEngine) {
public ExternalTaskQueryTopicBuilder buildQuery(ProcessEngine processEngine) {
FetchAndLockBuilder fetchAndLockBuilder = getBuilder(processEngine);

if (CollectionUtil.isEmpty(topics)) {
return fetchAndLockBuilder;
}

return configureTopics(fetchAndLockBuilder);
}

protected FetchAndLockBuilder configureTopics(FetchAndLockBuilder builder) {
protected ExternalTaskQueryTopicBuilder configureTopics(FetchAndLockBuilder builder) {
ExternalTaskQueryTopicBuilder topicBuilder = builder.subscribe();

if (CollectionUtil.isEmpty(topics)) {
return topicBuilder;
}

topics.forEach(topic -> {
topicBuilder.topic(topic.getTopicName(), topic.getLockDuration());

Expand Down Expand Up @@ -272,7 +272,7 @@ protected FetchAndLockBuilder configureTopics(FetchAndLockBuilder builder) {
}
});

return builder;
return topicBuilder;
}

protected FetchAndLockBuilder getBuilder(ProcessEngine engine) {
Expand Down
5 changes: 3 additions & 2 deletions ...t/engine-rest/src/main/java/org/camunda/bpm/engine/rest/impl/FetchAndLockHandlerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import javax.ws.rs.core.Response.Status;
import org.camunda.bpm.engine.IdentityService;
import org.camunda.bpm.engine.ProcessEngine;
import org.camunda.bpm.engine.externaltask.FetchAndLockBuilder;
import org.camunda.bpm.engine.externaltask.ExternalTaskQueryTopicBuilder;
import org.camunda.bpm.engine.externaltask.LockedExternalTask;
import org.camunda.bpm.engine.impl.ProcessEngineImpl;
import org.camunda.bpm.engine.impl.identity.Authentication;
Expand Down Expand Up @@ -262,8 +262,9 @@ protected FetchAndLockResult tryFetchAndLock(FetchAndLockRequest request) {
}

protected List<LockedExternalTaskDto> executeFetchAndLock(FetchExternalTasksExtendedDto fetchingDto, ProcessEngine processEngine) {
FetchAndLockBuilder fetchBuilder = fetchingDto.buildQuery(processEngine);
ExternalTaskQueryTopicBuilder fetchBuilder = fetchingDto.buildQuery(processEngine);
List<LockedExternalTask> externalTasks = fetchBuilder.execute();

return LockedExternalTaskDto.fromLockedExternalTasks(externalTasks);
}

Expand Down
52 changes: 26 additions & 26 deletions ...est/src/test/java/org/camunda/bpm/engine/rest/ExternalTaskRestServiceInteractionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public void setUpRuntimeData() {
@Test
public void testFetchAndLock() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand All @@ -226,15 +226,15 @@ public void testFetchAndLock() {

inOrder.verify(fetchTopicBuilder).topic("aTopicName", 12354L);
inOrder.verify(fetchTopicBuilder).variables(Arrays.asList(EXAMPLE_VARIABLE_INSTANCE_NAME));
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchTopicBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchAndLockWithBusinessKey() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand Down Expand Up @@ -263,15 +263,15 @@ public void testFetchAndLockWithBusinessKey() {
inOrder.verify(fetchTopicBuilder).topic("aTopicName", 12354L);
inOrder.verify(fetchTopicBuilder).businessKey(EXAMPLE_BUSINESS_KEY);
inOrder.verify(fetchTopicBuilder).variables(Arrays.asList(EXAMPLE_VARIABLE_INSTANCE_NAME));
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchAndLockWithProcessDefinition() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand Down Expand Up @@ -304,15 +304,15 @@ public void testFetchAndLockWithProcessDefinition() {
inOrder.verify(fetchTopicBuilder).processDefinitionIdIn(EXAMPLE_PROCESS_DEFINITION_ID);
inOrder.verify(fetchTopicBuilder).processDefinitionKey(EXAMPLE_PROCESS_DEFINITION_KEY);
inOrder.verify(fetchTopicBuilder).processDefinitionKeyIn(EXAMPLE_PROCESS_DEFINITION_KEY);
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchAndLockWithVariableValue() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand Down Expand Up @@ -347,15 +347,15 @@ public void testFetchAndLockWithVariableValue() {
inOrder.verify(fetchTopicBuilder).businessKey(EXAMPLE_BUSINESS_KEY);
inOrder.verify(fetchTopicBuilder).variables(Arrays.asList(EXAMPLE_VARIABLE_INSTANCE_NAME));
inOrder.verify(fetchTopicBuilder).processInstanceVariableEquals(variableValueParameter);
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchAndLockWithCreateTimeAsc() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

Map<String, Object> parameters = new HashMap<>();
parameters.put("maxTasks", 5);
Expand Down Expand Up @@ -395,15 +395,15 @@ public void testFetchAndLockWithCreateTimeAsc() {
inOrder.verify(fetchTopicBuilder).businessKey(EXAMPLE_BUSINESS_KEY);
inOrder.verify(fetchTopicBuilder).variables(Arrays.asList(EXAMPLE_VARIABLE_INSTANCE_NAME));
inOrder.verify(fetchTopicBuilder).processInstanceVariableEquals(variableValueParameter);
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchAndLockWithCreateTimeDesc() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

Map<String, Object> parameters = new HashMap<>();
parameters.put("maxTasks", 5);
Expand Down Expand Up @@ -443,15 +443,15 @@ public void testFetchAndLockWithCreateTimeDesc() {
inOrder.verify(fetchTopicBuilder).businessKey(EXAMPLE_BUSINESS_KEY);
inOrder.verify(fetchTopicBuilder).variables(Arrays.asList(EXAMPLE_VARIABLE_INSTANCE_NAME));
inOrder.verify(fetchTopicBuilder).processInstanceVariableEquals(variableValueParameter);
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchAndLockWithCreateTimeWithoutOrder() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

Map<String, Object> parameters = new HashMap<>();
parameters.put("maxTasks", 5);
Expand Down Expand Up @@ -491,15 +491,15 @@ public void testFetchAndLockWithCreateTimeWithoutOrder() {
inOrder.verify(fetchTopicBuilder).businessKey(EXAMPLE_BUSINESS_KEY);
inOrder.verify(fetchTopicBuilder).variables(Arrays.asList(EXAMPLE_VARIABLE_INSTANCE_NAME));
inOrder.verify(fetchTopicBuilder).processInstanceVariableEquals(variableValueParameter);
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchWithoutVariables() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand Down Expand Up @@ -532,15 +532,15 @@ public void testFetchWithoutVariables() {
inOrder.verify(fetchAndLockBuilder).subscribe();

inOrder.verify(fetchTopicBuilder).topic("aTopicName", 12354L);
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchTopicBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchAndLockWithTenant() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand Down Expand Up @@ -570,15 +570,15 @@ public void testFetchAndLockWithTenant() {
inOrder.verify(fetchTopicBuilder).topic("aTopicName", 12354L);
inOrder.verify(fetchTopicBuilder).withoutTenantId();
inOrder.verify(fetchTopicBuilder).tenantIdIn("tenant2");
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchAndLockByProcessDefinitionVersionTag() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand All @@ -604,15 +604,15 @@ public void testFetchAndLockByProcessDefinitionVersionTag() {

inOrder.verify(fetchTopicBuilder).topic("aTopicName", 12354L);
inOrder.verify(fetchTopicBuilder).processDefinitionVersionTag("versionTag");
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testFetchAndLockIncludeExtensionProperties() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand All @@ -638,15 +638,15 @@ public void testFetchAndLockIncludeExtensionProperties() {

inOrder.verify(fetchTopicBuilder).topic("aTopicName", 12354L);
inOrder.verify(fetchTopicBuilder).includeExtensionProperties();
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, fetchTopicBuilder, externalTaskService);
}

@Test
public void testEnableCustomObjectDeserialization() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand Down Expand Up @@ -683,15 +683,15 @@ public void testEnableCustomObjectDeserialization() {

inOrder.verify(fetchTopicBuilder).variables(Arrays.asList(EXAMPLE_VARIABLE_INSTANCE_NAME));
inOrder.verify(fetchTopicBuilder).enableCustomObjectDeserialization();
inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchAndLockBuilder, externalTaskService);
}

@Test
public void testLocalVariables() {
// given
when(fetchAndLockBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));
when(fetchTopicBuilder.execute()).thenReturn(Arrays.asList(lockedExternalTaskMock));

// when
Map<String, Object> parameters = new HashMap<>();
Expand Down Expand Up @@ -729,7 +729,7 @@ public void testLocalVariables() {
inOrder.verify(topicBuilder).variables(Arrays.asList(EXAMPLE_VARIABLE_INSTANCE_NAME));
inOrder.verify(topicBuilder).localVariables();

inOrder.verify(fetchAndLockBuilder).execute();
inOrder.verify(fetchTopicBuilder).execute();

verifyNoMoreInteractions(fetchTopicBuilder, fetchTopicBuilder, externalTaskService);
}
Expand Down
Loading

0 comments on commit 3818ff3

Please sign in to comment.