Skip to content

Commit 938024f

Browse files
kaladaykaladay
committed
MODWRKFLOW-72: Improve handling of delete behavior.
Add new exception `WorkflowDeploymentNotFound` to more properly communicate 404s. Provide new function called `fetchDeploymentDefinitions()` for getting all activated workflows without throwing an exception. This allows the delete method to get the response without treating a 404 as a problem. Update the existing `fetchDeploymentDefinition()` to call the new function but still throw the 404. The `fetchDeploymentDefinition()` now throws the new `WorkflowDeploymentNotFound` exception. The `workflowId` is now passed to `fetchDeploymentDefinition()` to report the **Workflow ID** in the 404. Handle the NULL case for when the **Deployment ID** is not found. Add function comments to both the new method and the updated method. There is a lot of work that needs to be done for the unit tests to get them into better shape. This attempts to only address the necessary or practical changes only. The `Exception` is being caught when a specific exception, like `RestClientException` should instead be caught. This behavior is a problem because exceptions other than `RestClientException` that are thrown within are then caught again and then rethrown as a different type.
1 parent 9b26247 commit 938024f

6 files changed

Lines changed: 400 additions & 175 deletions

File tree

‎service/src/main/java/org/folio/rest/workflow/controller/WorkflowController.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import java.util.regex.Pattern;
88
import org.apache.commons.logging.Log;
99
import org.apache.commons.logging.LogFactory;
10+
import org.folio.rest.workflow.exception.WorkflowDeploymentNotFound;
1011
import org.folio.rest.workflow.exception.WorkflowEngineServiceException;
1112
import org.folio.rest.workflow.exception.WorkflowImportException;
1213
import org.folio.rest.workflow.exception.WorkflowNotFoundException;
@@ -138,7 +139,7 @@ public JsonNode workflowHistory(
138139
@PathVariable String id,
139140
@TenantHeader String tenant,
140141
@TokenHeader String token
141-
) throws WorkflowEngineServiceException {
142+
) throws WorkflowDeploymentNotFound, WorkflowEngineServiceException {
142143
LOG.debug(String.format("Retrieving History: %s", sanitize(id)));
143144
return workflowEngineService.history(id, tenant, token);
144145
}
@@ -149,7 +150,7 @@ public JsonNode startWorkflow(
149150
@TenantHeader String tenant,
150151
@TokenHeader String token,
151152
@RequestBody JsonNode context
152-
) throws WorkflowEngineServiceException {
153+
) throws WorkflowDeploymentNotFound, WorkflowEngineServiceException, WorkflowNotFoundException {
153154
LOG.info(String.format("Starting: %s with context %s", sanitize(id), sanitize(context)));
154155
return workflowEngineService.start(id, tenant, token, context);
155156
}

‎service/src/main/java/org/folio/rest/workflow/controller/advice/WorkflowControllerAdvice.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import org.folio.rest.workflow.exception.WorkflowAlreadyActiveException;
66
import org.folio.rest.workflow.exception.WorkflowCreateAlreadyExistsException;
77
import org.folio.rest.workflow.exception.WorkflowDeploymentException;
8+
import org.folio.rest.workflow.exception.WorkflowDeploymentNotFound;
89
import org.folio.rest.workflow.exception.WorkflowEngineServiceException;
910
import org.folio.rest.workflow.exception.WorkflowImportException;
1011
import org.folio.rest.workflow.exception.WorkflowNotFoundException;
@@ -75,6 +76,12 @@ public ResponseEntity<String> handleWorkflowDeploymentException(WorkflowDeployme
7576
return buildError(exception, HttpStatus.INTERNAL_SERVER_ERROR);
7677
}
7778

79+
@ResponseStatus(HttpStatus.NOT_FOUND)
80+
@ExceptionHandler(WorkflowDeploymentNotFound.class)
81+
public ResponseEntity<String> handleWorkflowDeploymentNotFound(WorkflowDeploymentNotFound exception) {
82+
return buildError(exception, HttpStatus.NOT_FOUND);
83+
}
84+
7885
@ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
7986
@ExceptionHandler(WorkflowEngineServiceException.class)
8087
public ResponseEntity<String> handleWorkflowEngineServiceException(WorkflowEngineServiceException exception) {

‎service/src/main/java/org/folio/rest/workflow/exception/WorkflowDeploymentNotFound.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package org.folio.rest.workflow.exception;
2+
3+
/**
4+
* For providing a 404 when any Workflow deployment is not found in the Workflow Engine.
5+
*/
6+
public class WorkflowDeploymentNotFound extends Exception {
7+
8+
private static final long serialVersionUID = 424162623670077L;
9+
10+
public WorkflowDeploymentNotFound(String message) {
11+
super(message);
12+
}
13+
14+
public WorkflowDeploymentNotFound(String message, Exception e) {
15+
super(message, e);
16+
}
17+
18+
public WorkflowDeploymentNotFound(int code) {
19+
super(Integer.toString(code));
20+
}
21+
22+
public WorkflowDeploymentNotFound(int code, Exception e) {
23+
super(Integer.toString(code), e);
24+
}
25+
26+
}

‎service/src/main/java/org/folio/rest/workflow/service/WorkflowEngineService.java‎

Lines changed: 114 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@
77
import org.apache.commons.logging.LogFactory;
88
import org.folio.rest.workflow.dto.WorkflowDto;
99
import org.folio.rest.workflow.dto.WorkflowOperationalDto;
10+
import org.folio.rest.workflow.exception.WorkflowDeploymentNotFound;
1011
import org.folio.rest.workflow.exception.WorkflowEngineServiceException;
1112
import org.folio.rest.workflow.exception.WorkflowNotFoundException;
1213
import org.folio.rest.workflow.model.Workflow;
1314
import org.folio.rest.workflow.model.repo.WorkflowRepo;
14-
import org.mockito.ArgumentMatchers;
1515
import org.springframework.beans.factory.annotation.Value;
1616
import org.springframework.boot.restclient.RestTemplateBuilder;
1717
import org.springframework.http.HttpEntity;
@@ -20,6 +20,7 @@
2020
import org.springframework.http.HttpStatus;
2121
import org.springframework.http.ResponseEntity;
2222
import org.springframework.stereotype.Service;
23+
import org.springframework.web.client.RestClientException;
2324
import org.springframework.web.client.RestTemplate;
2425
import tools.jackson.databind.JsonNode;
2526
import tools.jackson.databind.json.JsonMapper;
@@ -95,13 +96,34 @@ public Workflow deactivate(String workflowId, String tenant, String token)
9596
public void delete(String workflowId, String tenant, String token)
9697
throws WorkflowEngineServiceException {
9798

98-
final ArrayNode node = fetchProcessInstanceHistory(workflowId, tenant, token);
99-
100-
if (!node.isEmpty()) {
101-
try {
102-
deactivate(workflowId, tenant, token);
103-
} catch (WorkflowEngineServiceException e) {
104-
throw new WorkflowEngineServiceException(String.format("Failed to delete Workflow '%s' due to deactivation failure: %s!", workflowId, e.getMessage()), e);
99+
final WorkflowOperationalDto workflow = workflowRepo.getViewById(workflowId, WorkflowOperationalDto.class);
100+
final String id = workflow.getDeploymentId();
101+
final String version = workflow.getVersionTag();
102+
103+
// Deployment ID will not exist if it has never been activated.
104+
if (id != null) {
105+
final ResponseEntity<ArrayNode> response = fetchDeploymentDefinitions(id, version, tenant, token);
106+
107+
if (response.getStatusCode() == HttpStatus.OK || response.getStatusCode() == HttpStatus.NOT_FOUND) {
108+
final ArrayNode definitions = !response.hasBody()
109+
? null
110+
: response.getBody();
111+
112+
if (definitions != null && response.getStatusCode() != HttpStatus.NOT_FOUND && !definitions.isEmpty()) {
113+
try {
114+
deactivate(workflowId, tenant, token);
115+
} catch (WorkflowEngineServiceException e) {
116+
final String message = String.format(
117+
"Failed to delete Workflow ID '%s' for Deployment ID '%s' and Version Tag '%s' due to deactivation failure: %s!",
118+
workflowId,
119+
id,
120+
version,
121+
e.getMessage()
122+
);
123+
124+
throw new WorkflowEngineServiceException(message, e);
125+
}
126+
}
105127
}
106128
}
107129

@@ -122,13 +144,23 @@ public void exists(String workflowId) throws WorkflowNotFoundException {
122144
}
123145

124146
public JsonNode start(String workflowId, String tenant, String token, JsonNode context)
125-
throws WorkflowEngineServiceException {
147+
throws WorkflowDeploymentNotFound, WorkflowEngineServiceException, WorkflowNotFoundException {
126148

127149
WorkflowOperationalDto workflow = workflowRepo.getViewById(workflowId, WorkflowOperationalDto.class);
150+
151+
if (workflow == null) {
152+
throw new WorkflowNotFoundException(String.format("Workflow ID '%s'", workflowId));
153+
}
154+
128155
String id = workflow.getDeploymentId();
129156
String version = workflow.getVersionTag();
130157

131-
JsonNode definition = fetchDeploymentDefinition(id, version, tenant, token);
158+
JsonNode definition = fetchFirstDeploymentDefinition(workflowId, id, version, tenant, token);
159+
160+
if (!definition.has("id") ) {
161+
throw new WorkflowEngineServiceException(String.format("Workflow ID '%s' with Deployment ID '%s' has no definition ID!", workflowId, id));
162+
}
163+
132164
String definitionId = definition.get("id").asString();
133165

134166
HttpEntity<JsonNode> contextHttpEntity = new HttpEntity<>(context, headers(tenant, token));
@@ -137,20 +169,26 @@ public JsonNode start(String workflowId, String tenant, String token, JsonNode c
137169
Map<String, Object> params = Map.of("arg1", definitionId);
138170

139171
try {
140-
ResponseEntity<JsonNode> response = exchange(url, HttpMethod.POST, contextHttpEntity, JsonNode.class, params);
172+
final ResponseEntity<JsonNode> response = exchange(url, HttpMethod.POST, contextHttpEntity, JsonNode.class, params);
173+
174+
if (response.getStatusCode() == HttpStatus.NOT_FOUND) {
175+
throw new WorkflowNotFoundException(String.format("Workflow ID '%s'", workflowId));
176+
}
141177

142178
return response.getBody();
143-
} catch (Exception e) {
179+
} catch (RestClientException e) {
144180
throw new WorkflowEngineServiceException(String.format("Failed to start workflow: %s!", e.getMessage()), e);
145181
}
146182
}
147183

148-
public JsonNode history(String workflowId, String tenant, String token) throws WorkflowEngineServiceException {
184+
public JsonNode history(String workflowId, String tenant, String token)
185+
throws WorkflowDeploymentNotFound, WorkflowEngineServiceException {
186+
149187
WorkflowOperationalDto workflow = workflowRepo.getViewById(workflowId, WorkflowOperationalDto.class);
150-
String deploymentId = workflow.getDeploymentId();
188+
String id = workflow.getDeploymentId();
151189
String version = workflow.getVersionTag();
152190

153-
JsonNode processDefinition = fetchDeploymentDefinition(deploymentId, version, tenant, token);
191+
JsonNode processDefinition = fetchFirstDeploymentDefinition(workflowId, id, version, tenant, token);
154192
String processDefinitionId = processDefinition.get("id").asString();
155193

156194
ArrayNode instances = fetchProcessInstanceHistory(processDefinitionId, tenant, token);
@@ -168,24 +206,66 @@ public JsonNode history(String workflowId, String tenant, String token) throws W
168206
return instances;
169207
}
170208

171-
private JsonNode fetchDeploymentDefinition(String deploymentId, String version, String tenant, String token)
172-
throws WorkflowEngineServiceException {
173-
174-
HttpEntity<Void> httpEntity = new HttpEntity<>(headers(tenant, token));
209+
/**
210+
* Fetch the first matching deployment for the given workflow.
211+
*
212+
* @param workflowId The Workflow ID, for use in exception logs.
213+
* @param deploymentId The Deployment ID to find.
214+
* @param version The version tag to use.
215+
* @param tenant The FOLIO tenant.
216+
* @param token The session token.
217+
*
218+
* @return The first matching response.
219+
*
220+
* @throws WorkflowDeploymentNotFound On not found.
221+
* @throws WorkflowEngineServiceException On error.
222+
*/
223+
private JsonNode fetchFirstDeploymentDefinition(String workflowId, String deploymentId, String version, String tenant, String token)
224+
throws WorkflowDeploymentNotFound, WorkflowEngineServiceException {
175225

176-
String url = String.format(PROCESS_DEFINITION_GET_URL_TEMPLATE, okapiUrl, restPath);
177-
Map<String, Object> params = Map.of("arg1", deploymentId, "arg2", version);
226+
final ResponseEntity<ArrayNode> response = fetchDeploymentDefinitions(deploymentId, version, tenant, token);
178227

179-
try {
180-
ResponseEntity<ArrayNode> response = exchange(url, HttpMethod.GET, httpEntity, ArrayNode.class, params);
228+
if (response.getStatusCode() == HttpStatus.OK || response.getStatusCode() == HttpStatus.NOT_FOUND) {
229+
final ArrayNode definitions = !response.hasBody()
230+
? null
231+
: response.getBody();
181232

182-
ArrayNode definitions = response.getBody();
183-
if (response.getStatusCode() == HttpStatus.OK && definitions != null && !definitions.isEmpty()) {
184-
return definitions.get(0);
233+
if (definitions == null || response.getStatusCode() == HttpStatus.NOT_FOUND || definitions.isEmpty() || definitions.get(0) == null) {
234+
throw new WorkflowDeploymentNotFound(String.format("Workflow with Deployment ID '%s' for Workflow ID '%s' is not found.", deploymentId, workflowId));
185235
}
186236

187-
throw new WorkflowEngineServiceException("Unable to get workflow process definition from workflow engine!");
188-
} catch (Exception e) {
237+
return definitions.get(0);
238+
}
239+
240+
throw new WorkflowEngineServiceException("Unable to get workflow process definition from workflow engine!");
241+
}
242+
243+
/**
244+
* Fetch all deployments, but return the response entity to allow caller to handle.
245+
*
246+
* @param deploymentId The activated Workflow deployment ID.
247+
* @param version The Workflow version number.
248+
* @param tenant The tenant.
249+
* @param token The token.
250+
*
251+
* @return The response entity.
252+
*
253+
* @throws WorkflowEngineServiceException On error.
254+
*/
255+
private ResponseEntity<ArrayNode> fetchDeploymentDefinitions(String deploymentId, String version, String tenant, String token)
256+
throws WorkflowEngineServiceException {
257+
258+
if (deploymentId == null) {
259+
throw new WorkflowEngineServiceException("Failed to deployment definition: Deployment ID is missing!");
260+
}
261+
262+
final HttpEntity<Void> httpEntity = new HttpEntity<>(headers(tenant, token));
263+
final String url = String.format(PROCESS_DEFINITION_GET_URL_TEMPLATE, okapiUrl, restPath);
264+
final Map<String, Object> params = Map.of("arg1", deploymentId, "arg2", version);
265+
266+
try {
267+
return exchange(url, HttpMethod.GET, httpEntity, ArrayNode.class, params);
268+
} catch (RestClientException e) {
189269
throw new WorkflowEngineServiceException(String.format("Failed to deployment definition: %s!", e.getMessage()), e);
190270
}
191271
}
@@ -207,7 +287,7 @@ private ArrayNode fetchProcessInstanceHistory(String processDefinitionId, String
207287
}
208288

209289
throw new WorkflowEngineServiceException("Unable to get workflow process instance history from workflow engine!");
210-
} catch (Exception e) {
290+
} catch (RestClientException e) {
211291
throw new WorkflowEngineServiceException(String.format("Failed to fetch process instance history: %s!", e.getMessage()), e);
212292
}
213293
}
@@ -230,7 +310,7 @@ private ArrayNode fetchIncidentsHistory(String processInstanceId, String tenant,
230310
}
231311

232312
return incidents;
233-
} catch (Exception e) {
313+
} catch (RestClientException e) {
234314
throw new WorkflowEngineServiceException(String.format("Failed to fetch incident history: %s!", e.getMessage()), e);
235315
}
236316
}
@@ -273,7 +353,7 @@ private Workflow sendWorkflowRequest(WorkflowDto workflow, String requestPath, S
273353
return workflowRepo.save(responseWorkflow);
274354
}
275355
}
276-
} catch (Exception e) {
356+
} catch (RestClientException e) {
277357
throw new WorkflowEngineServiceException(String.format("Failed to send workflow request: %s!", e.getMessage()), e);
278358
}
279359

@@ -296,9 +376,9 @@ private HttpHeaders headers(String tenant, String token) {
296376
return requestHeaders;
297377
}
298378

299-
private <T> ResponseEntity<T> exchange(String url, HttpMethod method, HttpEntity<?> request, Class<T> responseType, Map<String, ?> params) {
300-
LOG.debug(String.format("Exchange for %s %s %s", responseType.getSimpleName(), method, url));
301-
return this.restTemplate.exchange(url, method, request, responseType, (Object[]) new String[0], params);
379+
private <T> ResponseEntity<T> exchange(String url, HttpMethod method, HttpEntity<?> request, Class<T> responseType, Map<String, ? extends Object> params) {
380+
LOG.debug(String.format("Exchange for %s %s %s %s", responseType.getSimpleName(), method, url, params));
381+
return this.restTemplate.exchange(url, method, request, responseType, params);
302382
}
303383

304384
}

‎service/src/test/java/org/folio/rest/workflow/controller/advice/WorkflowControllerAdviceTest.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import java.util.stream.Stream;
1212
import org.folio.rest.workflow.exception.WorkflowAlreadyActiveException;
1313
import org.folio.rest.workflow.exception.WorkflowDeploymentException;
14+
import org.folio.rest.workflow.exception.WorkflowDeploymentNotFound;
1415
import org.folio.rest.workflow.exception.WorkflowEngineServiceException;
1516
import org.folio.rest.workflow.exception.WorkflowImportAlreadyImported;
1617
import org.folio.rest.workflow.exception.WorkflowImportException;
@@ -42,6 +43,8 @@ class WorkflowControllerAdviceTest {
4243

4344
private static final WorkflowDeploymentException WD_EXC = new WorkflowDeploymentException();
4445

46+
private static final WorkflowDeploymentNotFound WDNF_EXC = new WorkflowDeploymentNotFound(VALUE);
47+
4548
private static final WorkflowEngineServiceException WES_EXC = new WorkflowEngineServiceException(VALUE);
4649

4750
private static final WorkflowImportAlreadyImported WIAI_EXC = new WorkflowImportAlreadyImported(VALUE);
@@ -116,6 +119,20 @@ void handleWorkflowDeploymentExceptionTest() {
116119
assertTrue(matchBody(response, simpleName));
117120
}
118121

122+
@Test
123+
void handleWorkflowDeploymentNotFoundTest() {
124+
125+
final String simpleName = WorkflowDeploymentNotFound.class.getSimpleName();
126+
final ResponseEntity<String> response = advice.handleWorkflowDeploymentNotFound(WDNF_EXC);
127+
128+
assertNotNull(response);
129+
assertNotNull(response.getBody());
130+
131+
assertEquals(HttpStatus.NOT_FOUND, response.getStatusCode());
132+
assertEquals(MediaType.APPLICATION_JSON, response.getHeaders().getContentType());
133+
assertTrue(matchBody(response, simpleName));
134+
}
135+
119136
@Test
120137
void handleWorkflowEngineServiceExceptionTest() {
121138

0 commit comments

Comments
 (0)