Skip to content

Commit

Permalink
HWKALERTS-257 increase use of 400 resturn codes for illegal/bad args
Browse files Browse the repository at this point in the history 
Simplify catch blocks by introducing a new utility to handle responses
on exceptions. As part of the changes do some code cleanup in the handlers:
- remove unnecessary log.isDebugEnabled clauses
- prefer log.debugf over log.debug
- remove unnecessary else-clauses after returns
- reorder response code annotations to be ascending numerically

HWKALERTS-256 fix some guards in handlers
- use isEmpty() in a few more places
- consolidate code with new CommonUtil.checkTags()
  • Loading branch information
@jshaughn
jshaughn committed May 1, 2017
1 parent 60e34ed commit a630a08
Show file tree
Hide file tree
Showing 12 changed files with 365 additions and 646 deletions.
2 changes: 1 addition & 1 deletion hawkular-alerts-rest-tests/src/test/groovy/org/hawkular/alerts/rest/LifecycleITest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ class LifecycleITest extends AbstractITestBase {

// FETCH alerts for bogus value name/value tag syntax, should fail
resp = client.get(path: "", query: [startTime:start,tags:"test-autodisable-tname/test-autodisable-tvalue"] )
assertEquals(500, resp.status)
assertEquals(400, resp.status)

// FETCH alerts for just triggers generated in test t01, by time, should be 1
resp = client.get(path: "", query: [startTime:t01Start,endTime:t02Start] )
Expand Down
2 changes: 1 addition & 1 deletion hawkular-alerts-rest-tests/src/test/groovy/org/hawkular/alerts/rest/TriggersITest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ class TriggersITest extends AbstractITestBase {

// ensure a group trigger can not be removed with the standard remove trigger
resp = client.delete(path: "triggers/group-trigger")
assertEquals(500, resp.status)
assertEquals(400, resp.status)

// remove group trigger
resp = client.delete(path: "triggers/groups/group-trigger")
Expand Down
18 changes: 8 additions & 10 deletions .../hawkular-alerts-rest-api/src/main/java/org/hawkular/alerts/rest/ActionPluginHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,18 +70,17 @@ public ActionPluginHandler() {
response = String.class, responseContainer = "List")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Successfully fetched list of actions plugins."),
@ApiResponse(code = 400, message = "Bad Request/Invalid Parameters.", response = ApiError.class),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class)
})
public Response findActionPlugins() {
try {
Collection<String> actionPlugins = definitions.getActionPlugins();
if (log.isDebugEnabled()) {
log.debug("ActionPlugins: " + actionPlugins);
}
log.debugf("ActionPlugins: %s", actionPlugins);
return ResponseUtil.ok(actionPlugins);

} catch (Exception e) {
log.debug(e.getMessage(), e);
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}

Expand All @@ -94,6 +93,7 @@ public Response findActionPlugins() {
response = String.class, responseContainer = "List")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Action Plugin found."),
@ApiResponse(code = 400, message = "Bad Request/Invalid Parameters.", response = ApiError.class),
@ApiResponse(code = 404, message = "Action Plugin not found.", response = ApiError.class),
@ApiResponse(code = 500, message = "Internal server error", response = ApiError.class)
})
Expand All @@ -102,16 +102,14 @@ public Response getActionPlugin(@ApiParam(value = "Action plugin to query.", req
final String actionPlugin) {
try {
Set<String> actionPluginProps = definitions.getActionPlugin(actionPlugin);
if (log.isDebugEnabled()) {
log.debug("ActionPlugin: " + actionPlugin + " - Properties: " + actionPluginProps);
}
log.debugf("ActionPlugin: %s, Properties: %s", actionPlugin, actionPluginProps);
if (isEmpty(actionPluginProps)) {
return ResponseUtil.notFound("actionPlugin: " + actionPlugin + " not found");
}
return ResponseUtil.ok(actionPluginProps);

} catch (Exception e) {
log.debug(e.getMessage(), e);
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}
}
105 changes: 39 additions & 66 deletions ...-rest/hawkular-alerts-rest-api/src/main/java/org/hawkular/alerts/rest/ActionsHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,18 +90,17 @@ public ActionsHandler() {
response = Collection.class, responseContainer = "Map")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Successfully fetched map of action ids grouped by plugin."),
@ApiResponse(code = 400, message = "Bad Request/Invalid Parameters.", response = ApiError.class),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class)
})
public Response findActionIds() {
try {
Map<String, Set<String>> actions = definitions.getActionDefinitionIds(tenantId);
if (log.isDebugEnabled()) {
log.debug("Actions: " + actions);
}
log.debugf("Actions: %s", actions);
return ResponseUtil.ok(actions);

} catch (Exception e) {
log.debug(e.getMessage(), e);
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}

Expand All @@ -112,6 +111,7 @@ public Response findActionIds() {
response = String.class, responseContainer = "List")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Successfully fetched list of action ids."),
@ApiResponse(code = 400, message = "Bad Request/Invalid Parameters.", response = ApiError.class),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class)
})
public Response findActionIdsByPlugin(@ApiParam(value = "Action plugin to filter query for action ids.",
Expand All @@ -120,13 +120,11 @@ public Response findActionIdsByPlugin(@ApiParam(value = "Action plugin to filter
final String actionPlugin) {
try {
Collection<String> actions = definitions.getActionDefinitionIds(tenantId, actionPlugin);
if (log.isDebugEnabled()) {
log.debug("Actions: " + actions);
}
log.debugf("Actions: %s", actions);
return ResponseUtil.ok(actions);

} catch (Exception e) {
log.debug(e.getMessage(), e);
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}

Expand All @@ -137,6 +135,7 @@ public Response findActionIdsByPlugin(@ApiParam(value = "Action plugin to filter
response = ActionDefinition.class)
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success, Action found."),
@ApiResponse(code = 400, message = "Bad Request/Invalid Parameters.", response = ApiError.class),
@ApiResponse(code = 404, message = "No Action found.", response = ApiError.class),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class)
})
Expand All @@ -148,17 +147,15 @@ public Response getActionDefinition(@ApiParam(value = "Action plugin.", required
final String actionId) {
try {
ActionDefinition actionDefinition = definitions.getActionDefinition(tenantId, actionPlugin, actionId);
if (log.isDebugEnabled()) {
log.debug("ActionDefinition: " + actionDefinition);
}
log.debugf("ActionDefinition: %s", actionDefinition);
if (actionDefinition == null) {
return ResponseUtil.notFound("Not action found for actionPlugin: " + actionPlugin + " and actionId: "
+ actionId);
}
return ResponseUtil.ok(actionDefinition);

} catch (Exception e) {
log.debug(e.getMessage(), e);
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}

Expand All @@ -171,9 +168,9 @@ public Response getActionDefinition(@ApiParam(value = "Action plugin.", required
response = ActionDefinition.class)
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success, ActionDefinition Created."),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class),
@ApiResponse(code = 400, message = "Existing ActionDefinition/Invalid Parameters",
response = ApiError.class)
response = ApiError.class),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class)
})
public Response createActionDefinition(
@ApiParam(value = "ActionDefinition to be created.",
Expand All @@ -196,19 +193,13 @@ public Response createActionDefinition(
if (definitions.getActionDefinition(tenantId, actionDefinition.getActionPlugin(),
actionDefinition.getActionId()) != null) {
return ResponseUtil.badRequest("Existing ActionDefinition: " + actionDefinition);
} else {
definitions.addActionDefinition(tenantId, actionDefinition);
if (log.isDebugEnabled()) {
log.debug("ActionDefinition: " + actionDefinition);
}
return ResponseUtil.ok(actionDefinition);
}
definitions.addActionDefinition(tenantId, actionDefinition);
log.debugf("ActionDefinition: %s", actionDefinition);
return ResponseUtil.ok(actionDefinition);

} catch (Exception e) {
log.debug(e.getMessage(), e);
if (e.getCause() != null && e.getCause() instanceof IllegalArgumentException) {
return ResponseUtil.badRequest("Bad arguments: " + e.getMessage());
}
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}

Expand All @@ -220,9 +211,9 @@ public Response createActionDefinition(
response = ActionDefinition.class)
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success, ActionDefinition Updated."),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class),
@ApiResponse(code = 400, message = "Bad Request/Invalid Parameters.", response = ApiError.class),
@ApiResponse(code = 404, message = "ActionDefinition not found for update.", response = ApiError.class)
@ApiResponse(code = 404, message = "ActionDefinition not found for update.", response = ApiError.class),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class)
})
public Response updateActionDefinition(
@ApiParam(value = "ActionDefinition to be updated.",
Expand All @@ -243,19 +234,13 @@ public Response updateActionDefinition(
if (definitions.getActionDefinition(tenantId, actionDefinition.getActionPlugin(),
actionDefinition.getActionId()) != null) {
definitions.updateActionDefinition(tenantId, actionDefinition);
if (log.isDebugEnabled()) {
log.debug("ActionDefinition: " + actionDefinition);
}
log.debugf("ActionDefinition: %s", actionDefinition);
return ResponseUtil.ok(actionDefinition);
} else {
return ResponseUtil.notFound("ActionDefinition: " + actionDefinition + " not found for update");
}
return ResponseUtil.notFound("ActionDefinition: " + actionDefinition + " not found for update");

} catch (Exception e) {
log.debug(e.getMessage(), e);
if (e.getCause() != null && e.getCause() instanceof IllegalArgumentException) {
return ResponseUtil.badRequest("Bad arguments: " + e.getMessage());
}
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}

Expand All @@ -264,8 +249,9 @@ public Response updateActionDefinition(
@ApiOperation(value = "Delete an existing ActionDefinition.")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success, ActionDefinition Deleted."),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class),
@ApiResponse(code = 404, message = "ActionDefinition not found for delete.", response = ApiError.class)
@ApiResponse(code = 400, message = "Bad Request/Invalid Parameters.", response = ApiError.class),
@ApiResponse(code = 404, message = "ActionDefinition not found for delete.", response = ApiError.class),
@ApiResponse(code = 500, message = "Internal server error.", response = ApiError.class)
})
public Response deleteActionDefinition(
@ApiParam(value = "Action plugin.", required = true)
Expand All @@ -277,17 +263,14 @@ public Response deleteActionDefinition(
try {
if (definitions.getActionDefinition(tenantId, actionPlugin, actionId) != null) {
definitions.removeActionDefinition(tenantId, actionPlugin, actionId);
if (log.isDebugEnabled()) {
log.debug("ActionPlugin: " + actionPlugin + " ActionId: " + actionId);
}
log.debugf("ActionPlugin: %s, ActionId: %s", actionPlugin, actionId);
return ResponseUtil.ok();
} else {
return ResponseUtil.notFound("ActionPlugin: " + actionPlugin + " ActionId: " + actionId +
" not found for delete");
}
return ResponseUtil
.notFound("ActionPlugin: " + actionPlugin + " ActionId: " + actionId + " not found for delete");

} catch (Exception e) {
log.debug(e.getMessage(), e);
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}

Expand Down Expand Up @@ -338,19 +321,14 @@ public Response findActionsHistory(
ActionsCriteria criteria = buildCriteria(startTime, endTime, actionPlugins, actionIds, alertIds, results,
thin);
Page<Action> actionPage = actions.getActions(tenantId, criteria, pager);
if (log.isDebugEnabled()) {
log.debug("Actions: " + actionPage);
}
log.debugf("Actions: %s", actionPage);
if (isEmpty(actionPage)) {
return ResponseUtil.ok(actionPage);
}
return ResponseUtil.paginatedOk(actionPage, uri);

} catch (Exception e) {
log.debug(e.getMessage(), e);
if (e.getCause() != null && e.getCause() instanceof IllegalArgumentException) {
return ResponseUtil.badRequest("Bad arguments: " + e.getMessage());
}
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}

Expand Down Expand Up @@ -394,16 +372,11 @@ public Response deleteActionsHistory(
ActionsCriteria criteria = buildCriteria(startTime, endTime, actionPlugins, actionIds, alertIds, results,
false);
int numDeleted = actions.deleteActions(tenantId, criteria);
if (log.isDebugEnabled()) {
log.debug("Actions deleted: " + numDeleted);
}
log.debugf("Actions deleted: %d", numDeleted);
return ResponseUtil.ok(new ApiDeleted(numDeleted));

} catch (Exception e) {
log.debug(e.getMessage(), e);
if (e.getCause() != null && e.getCause() instanceof IllegalArgumentException) {
return ResponseUtil.badRequest("Bad arguments: " + e.getMessage());
}
return ResponseUtil.internalError(e);
return ResponseUtil.onException(e, log);
}
}

Expand Down

0 comments on commit a630a08

Please sign in to comment.