This is an automated email from the ASF dual-hosted git repository. akshayrai09 pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push: new 1be4b5c [TE] Replace response message with exceptions; Reuse notification endpoint in Create Alert (#3780) 1be4b5c is described below commit 1be4b5ced82d669d095c1f74760b585499e66f3a Author: Akshay Rai <akshayra...@gmail.com> AuthorDate: Fri Feb 1 15:13:47 2019 -0800 [TE] Replace response message with exceptions; Reuse notification endpoint in Create Alert (#3780) --- .../detection/validators/ConfigValidator.java | 11 +- .../validators/DetectionAlertConfigValidator.java | 60 ++-- .../thirdeye/detection/yaml/YamlResource.java | 331 +++++++++------------ .../thirdeye/detection/yaml/YamlResourceTest.java | 146 +++++---- 4 files changed, 257 insertions(+), 291 deletions(-) diff --git a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/ConfigValidator.java b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/ConfigValidator.java index d77f25f..4879674 100644 --- a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/ConfigValidator.java +++ b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/ConfigValidator.java @@ -19,6 +19,7 @@ package org.apache.pinot.thirdeye.detection.validators; +import javax.xml.bind.ValidationException; import org.apache.pinot.thirdeye.datalayer.bao.AlertConfigManager; import org.apache.pinot.thirdeye.datalayer.bao.ApplicationManager; import org.apache.pinot.thirdeye.datasource.DAORegistry; @@ -44,21 +45,17 @@ public abstract class ConfigValidator { * the yaml exists and is parsable */ @SuppressWarnings("unchecked") - public boolean validateYAMLConfig(String yamlConfig, Map<String, String> responseMessage) { + public boolean validateYAMLConfig(String yamlConfig) throws ValidationException { // Check if YAML is empty or not if (StringUtils.isEmpty(yamlConfig)) { - responseMessage.put("message", "The config file cannot be blank."); - responseMessage.put("more-info", "Payload in the request is empty"); - return false; + throw new ValidationException("The Yaml Payload in the request is empty."); } // Check if the YAML is parsable try { Map<String, Object> yamlConfigMap = (Map<String, Object>) YAML.load(yamlConfig); } catch (Exception e) { - responseMessage.put("message", "There was an error parsing the yaml file. Check for syntax issues."); - responseMessage.put("more-info", "Error parsing YAML" + e); - return false; + throw new ValidationException("Error parsing the Yaml input. Check for syntax issues."); } return true; diff --git a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/DetectionAlertConfigValidator.java b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/DetectionAlertConfigValidator.java index 8f2e0de..bd145ad 100644 --- a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/DetectionAlertConfigValidator.java +++ b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/DetectionAlertConfigValidator.java @@ -19,10 +19,12 @@ package org.apache.pinot.thirdeye.detection.validators; +import javax.xml.bind.ValidationException; import org.apache.pinot.thirdeye.datalayer.dto.DetectionAlertConfigDTO; import java.util.List; import java.util.Map; import org.apache.commons.lang.StringUtils; +import org.apache.pinot.thirdeye.datalayer.util.Predicate; import org.apache.pinot.thirdeye.datasource.DAORegistry; import org.junit.Assert; @@ -42,63 +44,57 @@ public class DetectionAlertConfigValidator extends ConfigValidator { * Perform validation on the alert config like verifying if all the required fields are set */ @SuppressWarnings("unchecked") - public boolean validateConfig(DetectionAlertConfigDTO alertConfig, Map<String, String> responseMessage) { - boolean isValid = true; - + public void validateConfig(DetectionAlertConfigDTO alertConfig) throws ValidationException { // Check for all the required fields in the alert if (StringUtils.isEmpty(alertConfig.getName())) { - responseMessage.put("message", "Subscription group name field cannot be left empty."); - return false; + throw new ValidationException("Subscription group name field cannot be left empty."); } if (StringUtils.isEmpty(alertConfig.getApplication())) { - responseMessage.put("message", "Application field cannot be left empty"); - return false; + throw new ValidationException("Application field cannot be left empty"); } if (StringUtils.isEmpty(alertConfig.getFrom())) { - responseMessage.put("message", "From address field cannot be left empty"); - return false; + throw new ValidationException("From address field cannot be left empty"); } if (alertConfig.getProperties() == null || alertConfig.getProperties().get(PROP_CLASS_NAME) == null || StringUtils.isEmpty(alertConfig.getProperties().get(PROP_CLASS_NAME).toString())) { - responseMessage.put("message", "'Type' field cannot be left empty."); - return false; + throw new ValidationException("'Type' field cannot be left empty."); } // At least one alertScheme is required if (alertConfig.getAlertSchemes() == null || alertConfig.getAlertSchemes().size() == 0) { - responseMessage.put("message", "Alert scheme cannot be left empty"); - return false; + throw new ValidationException("Alert scheme cannot be left empty"); } // Properties cannot be empty if (alertConfig.getProperties() == null || alertConfig.getProperties().isEmpty()) { - responseMessage.put("message", "Alert properties cannot be left empty. Please specify the recipients," + throw new ValidationException("Alert properties cannot be left empty. Please specify the recipients," + " subscribed detections, and type."); - return false; } // detectionConfigIds cannot be empty List<Long> detectionIds = (List<Long>) alertConfig.getProperties().get(PROP_DETECTION_CONFIG_IDS); if (detectionIds == null || detectionIds.isEmpty()) { - responseMessage.put("message", "A notification group should subscribe to at least one alert. If you wish to" + throw new ValidationException("A notification group should subscribe to at least one alert. If you wish to" + " unsubscribe, set active to false."); - return false; } // At least one recipient must be specified Map<String, Object> recipients = (Map<String, Object>) alertConfig.getProperties().get(PROP_RECIPIENTS); if (recipients == null || recipients.isEmpty() || ((List<String>) recipients.get("to")).isEmpty()) { - responseMessage.put("message", "Please specify at least one recipient in the notification group. If you wish to" + throw new ValidationException("Please specify at least one recipient in the notification group. If you wish to" + " unsubscribe, set active to false."); - return false; } // Application name should be valid if (DAORegistry.getInstance().getApplicationDAO().findByName(alertConfig.getApplication()).size() == 0) { - responseMessage.put("message", "Application name doesn't exist in our registry. Please use an existing" + throw new ValidationException("Application name doesn't exist in our registry. Please use an existing" + " application name. You may search for registered applications from the ThirdEye dashboard or reach out" + " to ask_thirdeye if you wish to setup a new application."); - return false; } - // TODO add more checks like cron validity, email validity, alert type check, scheme type check etc. + // Check for duplicates + List<DetectionAlertConfigDTO> alertConfigDTOS = DAORegistry.getInstance().getDetectionAlertConfigManager() + .findByPredicate(Predicate.EQ("name", alertConfig.getName())); + if (!alertConfigDTOS.isEmpty()) { + throw new ValidationException("Subscription group name is already taken. Please use a different name."); + } - return isValid; + // TODO add more checks like cron validity, email validity, alert type check, scheme type check etc. } /** @@ -106,30 +102,22 @@ public class DetectionAlertConfigValidator extends ConfigValidator { * updated by the user. */ @SuppressWarnings("unchecked") - public boolean validateUpdatedConfig(DetectionAlertConfigDTO updatedAlertConfig, - DetectionAlertConfigDTO oldAlertConfig, Map<String, String> responseMessage) { - boolean isValid = true; - - if (!validateConfig(updatedAlertConfig, responseMessage)) { - isValid = false; - } + public void validateUpdatedConfig(DetectionAlertConfigDTO updatedAlertConfig, DetectionAlertConfigDTO oldAlertConfig) + throws ValidationException { + validateConfig(updatedAlertConfig); Long newHighWatermark = updatedAlertConfig.getHighWaterMark(); Long oldHighWatermark = oldAlertConfig.getHighWaterMark(); if (newHighWatermark != null && oldHighWatermark != null && newHighWatermark.longValue() != oldHighWatermark) { - responseMessage.put("message", "HighWaterMark has been modified. This is not allowed"); - isValid = false; + throw new ValidationException("HighWaterMark has been modified. This is not allowed"); } if (updatedAlertConfig.getVectorClocks() != null) { for (Map.Entry<Long, Long> vectorClock : updatedAlertConfig.getVectorClocks().entrySet()) { if (!oldAlertConfig.getVectorClocks().containsKey(vectorClock.getKey()) || oldAlertConfig.getVectorClocks().get(vectorClock.getKey()).longValue() != vectorClock.getValue()) { - responseMessage.put("message", "Vector clock has been modified. This is not allowed."); - isValid = false; + throw new ValidationException("Vector clock has been modified. This is not allowed."); } } } - - return isValid; } } diff --git a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/yaml/YamlResource.java b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/yaml/YamlResource.java index d87386c..d1cdd6a 100644 --- a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/yaml/YamlResource.java +++ b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/yaml/YamlResource.java @@ -43,6 +43,7 @@ import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import javax.xml.bind.ValidationException; import org.apache.commons.collections.MapUtils; import org.apache.commons.lang3.StringUtils; import org.apache.pinot.thirdeye.api.Constants; @@ -78,14 +79,13 @@ public class YamlResource { protected static final Logger LOG = LoggerFactory.getLogger(YamlResource.class); private static ObjectMapper OBJECT_MAPPER = new ObjectMapper(); - public static final String PROP_SUBS_GROUP_NAME = "subscriptionGroupName"; public static final String PROP_DETECTION_NAME = "detectionName"; private final DetectionConfigManager detectionConfigDAO; private final DetectionAlertConfigManager detectionAlertConfigDAO; private final YamlDetectionTranslatorLoader translatorLoader; private final YamlDetectionAlertConfigTranslator alertConfigTranslator; - private final DetectionAlertConfigValidator alertValidator; + private final DetectionAlertConfigValidator notificationValidator; private final DataProvider provider; private final MetricConfigManager metricDAO; private final DatasetConfigManager datasetDAO; @@ -98,7 +98,7 @@ public class YamlResource { this.detectionConfigDAO = DAORegistry.getInstance().getDetectionConfigManager(); this.detectionAlertConfigDAO = DAORegistry.getInstance().getDetectionAlertConfigManager(); this.translatorLoader = new YamlDetectionTranslatorLoader(); - this.alertValidator = DetectionAlertConfigValidator.getInstance(); + this.notificationValidator = DetectionAlertConfigValidator.getInstance(); this.alertConfigTranslator = new YamlDetectionAlertConfigTranslator(this.detectionConfigDAO); this.metricDAO = DAORegistry.getInstance().getMetricConfigDAO(); this.datasetDAO = DAORegistry.getInstance().getDatasetConfigDAO(); @@ -118,6 +118,53 @@ public class YamlResource { this.provider = new DefaultDataProvider(metricDAO, datasetDAO, eventDAO, anomalyDAO, timeseriesLoader, aggregationLoader, loader); } + public DetectionConfigDTO translateToDetectionConfig(Map<String, Object> yamlConfig, Map<String, String> responseMessage) { + return buildDetectionConfigFromYaml(0, 0, yamlConfig, null, responseMessage); + } + + /* + * Build the detection config from a yaml. + * Returns null if building or validation failed. Error messages stored in responseMessage. + */ + private DetectionConfigDTO buildDetectionConfigFromYaml(long startTime, long endTime, Map<String, Object> yamlConfig, + DetectionConfigDTO existingDetectionConfig, Map<String, String> responseMessage) { + if (startTime == 0L && endTime == 0L) { + // default tuning window 28 days + endTime = System.currentTimeMillis(); + startTime = endTime - TimeUnit.DAYS.toMillis(28); + } + + try{ + YamlDetectionConfigTranslator translator = this.translatorLoader.from(yamlConfig, this.provider); + DetectionConfigDTO detectionConfig = translator.withTrainingWindow(startTime, endTime) + .withExistingDetectionConfig(existingDetectionConfig) + .generateDetectionConfig(); + validatePipeline(detectionConfig); + return detectionConfig; + } catch (InvocationTargetException e){ + // exception thrown in validate pipeline via reflection + LOG.error("Validate pipeline error", e); + responseMessage.put("message", e.getCause().getMessage()); + } catch (Exception e) { + LOG.error("yaml translation error", e); + responseMessage.put("message", e.getMessage()); + } + return null; + } + + /* + * Init the pipeline to check if detection pipeline property is valid semantically. + */ + private void validatePipeline(DetectionConfigDTO detectionConfig) throws Exception { + Long id = detectionConfig.getId(); + // swap out id + detectionConfig.setId(-1L); + // try to load the detection pipeline and init all the components + this.loader.from(provider, detectionConfig, 0, 0); + // set id back + detectionConfig.setId(id); + } + @POST @Path("/create-alert") @Produces(MediaType.APPLICATION_JSON) @@ -137,7 +184,7 @@ public class YamlResource { return Response.status(Response.Status.BAD_REQUEST).entity(ImmutableMap.of("message", "notification yaml is missing")).build(); } - // get detection yaml + // Detection String detectionYaml = yamls.get("detection"); Map<String, Object> detectionYamlConfig; @@ -168,70 +215,17 @@ public class YamlResource { } Preconditions.checkNotNull(detectionConfigId, "Save detection config failed"); - // notification - // TODO: Inject detectionConfigId into detection alert config - DetectionAlertConfigDTO alertConfig = createDetectionAlertConfig(yamls.get("notification"), responseMessage); - if (alertConfig == null) { - // revert detection DTO - this.detectionConfigDAO.deleteById(detectionConfigId); - return Response.status(Response.Status.BAD_REQUEST).entity(responseMessage).build(); - } - Long detectionAlertConfigId = this.detectionAlertConfigDAO.save(alertConfig); - if (detectionAlertConfigId == null){ + // Notification + String notificationYaml = yamls.get("notification"); + Response response = createDetectionAlertConfigApi(notificationYaml); + if (response.getStatusInfo() != Response.Status.OK) { // revert detection DTO this.detectionConfigDAO.deleteById(detectionConfigId); - return Response.serverError().entity(ImmutableMap.of("message", "Save detection alert config failed")).build(); + return Response.serverError().entity(response.getEntity()).build(); } - LOG.info("saved detection alert config id {}", detectionAlertConfigId); + long alertId = Long.parseLong(((Map<String, String>) response.getEntity()).get("detectionAlertConfigId")); - return Response.ok().entity(ImmutableMap.of("detectionConfigId", detectionConfig.getId(), "detectionAlertConfigId", alertConfig.getId())).build(); - } - - public DetectionConfigDTO translateToDetectionConfig(Map<String, Object> yamlConfig, Map<String, String> responseMessage) { - return buildDetectionConfigFromYaml(0, 0, yamlConfig, null, responseMessage); - } - - /* - * Build the detection config from a yaml. - * Returns null if building or validation failed. Error messages stored in responseMessage. - */ - private DetectionConfigDTO buildDetectionConfigFromYaml(long startTime, long endTime, Map<String, Object> yamlConfig, - DetectionConfigDTO existingDetectionConfig, Map<String, String> responseMessage) { - if (startTime == 0L && endTime == 0L) { - // default tuning window 28 days - endTime = System.currentTimeMillis(); - startTime = endTime - TimeUnit.DAYS.toMillis(28); - } - - try{ - YamlDetectionConfigTranslator translator = this.translatorLoader.from(yamlConfig, this.provider); - DetectionConfigDTO detectionConfig = translator.withTrainingWindow(startTime, endTime) - .withExistingDetectionConfig(existingDetectionConfig) - .generateDetectionConfig(); - validatePipeline(detectionConfig); - return detectionConfig; - } catch (InvocationTargetException e){ - // exception thrown in validate pipeline via reflection - LOG.error("Validate pipeline error", e); - responseMessage.put("message", e.getCause().getMessage()); - } catch (Exception e) { - LOG.error("yaml translation error", e); - responseMessage.put("message", e.getMessage()); - } - return null; - } - - /* - * Init the pipeline to check if detection pipeline property is valid semantically. - */ - private void validatePipeline(DetectionConfigDTO detectionConfig) throws Exception { - Long id = detectionConfig.getId(); - // swap out id - detectionConfig.setId(-1L); - // try to load the detection pipeline and init all the components - this.loader.from(provider, detectionConfig, 0, 0); - // set id back - detectionConfig.setId(id); + return Response.ok().entity(ImmutableMap.of("detectionConfigId", detectionConfig.getId(), "detectionAlertConfigId", alertId)).build(); } /** @@ -323,154 +317,126 @@ public class YamlResource { return Response.ok(detectionConfig).build(); } + @SuppressWarnings("unchecked") + DetectionAlertConfigDTO createDetectionAlertConfig(String yamlAlertConfig) throws ValidationException { + notificationValidator.validateYAMLConfig(yamlAlertConfig); + + // Translate config from YAML to detection alert config (JSON) + TreeMap<String, Object> newAlertConfigMap = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + newAlertConfigMap.putAll((Map<String, Object>) this.yaml.load(yamlAlertConfig)); + DetectionAlertConfigDTO alertConfig = this.alertConfigTranslator.translate(newAlertConfigMap); + alertConfig.setYaml(yamlAlertConfig); + + // Validate the config before saving it + notificationValidator.validateConfig(alertConfig); + + return alertConfig; + } + + @SuppressWarnings("unchecked") + DetectionAlertConfigDTO updateDetectionAlertConfig(long oldAlertConfigID, String yamlAlertConfig) throws ValidationException { + DetectionAlertConfigDTO oldAlertConfig = this.detectionAlertConfigDAO.findById(oldAlertConfigID); + if (oldAlertConfig == null) { + throw new RuntimeException("Cannot find subscription group " + oldAlertConfigID); + } + notificationValidator.validateYAMLConfig(yamlAlertConfig); + + // Translate payload to detection alert config + TreeMap<String, Object> newAlertConfigMap = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + newAlertConfigMap.putAll((Map<String, Object>) this.yaml.load(yamlAlertConfig)); + DetectionAlertConfigDTO newAlertConfig = this.alertConfigTranslator.translate(newAlertConfigMap); + + // Update existing alert config with the newly supplied config. + DetectionAlertConfigDTO updatedAlertConfig = updateDetectionAlertConfig(oldAlertConfig, newAlertConfig); + updatedAlertConfig.setYaml(yamlAlertConfig); + + // Validate before updating the config + notificationValidator.validateUpdatedConfig(updatedAlertConfig, oldAlertConfig); + + return updatedAlertConfig; + } + + /** + * Update the existing {@code oldAlertConfig} with the new {@code newAlertConfig} + * + * Update all the fields except the vector clocks and high watermark. The clocks and watermarks + * are managed by the platform. They shouldn't be reset by the user. + */ + DetectionAlertConfigDTO updateDetectionAlertConfig(DetectionAlertConfigDTO oldAlertConfig, + DetectionAlertConfigDTO newAlertConfig) { + oldAlertConfig.setName(newAlertConfig.getName()); + oldAlertConfig.setCronExpression(newAlertConfig.getCronExpression()); + oldAlertConfig.setApplication(newAlertConfig.getApplication()); + oldAlertConfig.setFrom(newAlertConfig.getFrom()); + oldAlertConfig.setSubjectType(newAlertConfig.getSubjectType()); + oldAlertConfig.setReferenceLinks(newAlertConfig.getReferenceLinks()); + oldAlertConfig.setActive(newAlertConfig.isActive()); + oldAlertConfig.setAlertSchemes(newAlertConfig.getAlertSchemes()); + oldAlertConfig.setAlertSuppressors(newAlertConfig.getAlertSuppressors()); + oldAlertConfig.setOnlyFetchLegacyAnomalies(newAlertConfig.isOnlyFetchLegacyAnomalies()); + oldAlertConfig.setProperties(newAlertConfig.getProperties()); + + return oldAlertConfig; + } + @POST @Path("/notification") @Produces(MediaType.APPLICATION_JSON) @Consumes(MediaType.TEXT_PLAIN) @ApiOperation("Create a notification group using a YAML config") @SuppressWarnings("unchecked") - public Response createDetectionAlertConfig( + public Response createDetectionAlertConfigApi( @ApiParam("payload") String yamlAlertConfig) { Map<String, String> responseMessage = new HashMap<>(); Long detectionAlertConfigId; try { - DetectionAlertConfigDTO alertConfig = createDetectionAlertConfig(yamlAlertConfig, responseMessage); - if (alertConfig == null) { - return Response.status(Response.Status.BAD_REQUEST).entity(responseMessage).build(); - } + DetectionAlertConfigDTO alertConfig = createDetectionAlertConfig(yamlAlertConfig); + Preconditions.checkNotNull(alertConfig); detectionAlertConfigId = this.detectionAlertConfigDAO.save(alertConfig); - if (detectionAlertConfigId == null) { - responseMessage.put("message", "Failed to save the detection alert config."); - responseMessage.put("more-info", "Check for potential DB issues. YAML alert config = " + yamlAlertConfig); - return Response.serverError().entity(responseMessage).build(); - } + Preconditions.checkNotNull(detectionAlertConfigId); } catch (Exception e) { - responseMessage.put("message", "Failed to save the detection alert config."); - responseMessage.put("more-info", "Exception = " + e); + LOG.error("Error creating notification group with payload " + yamlAlertConfig, e); + responseMessage.put("message", "Failed to create the notification group."); + responseMessage.put("more-info", "Error = " + e.getMessage()); return Response.serverError().entity(responseMessage).build(); } - responseMessage.put("message", "The YAML alert config was saved successfully."); + LOG.info("Notification group created with id " + detectionAlertConfigId + " using payload " + yamlAlertConfig); + responseMessage.put("message", "The notification group was created successfully."); responseMessage.put("more-info", "Record saved with id " + detectionAlertConfigId); return Response.ok().entity(responseMessage).build(); } - - @SuppressWarnings("unchecked") - public DetectionAlertConfigDTO createDetectionAlertConfig(String yamlAlertConfig, Map<String, String> responseMessage ) { - if (!alertValidator.validateYAMLConfig(yamlAlertConfig, responseMessage)) { - return null; - } - - TreeMap<String, Object> newAlertConfigMap = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); - newAlertConfigMap.putAll((Map<String, Object>) this.yaml.load(yamlAlertConfig)); - - // Check if a subscription group with the name already exists - String subsGroupName = MapUtils.getString(newAlertConfigMap, PROP_SUBS_GROUP_NAME); - if (StringUtils.isEmpty(subsGroupName)) { - responseMessage.put("message", "Subscription group name field cannot be left empty."); - return null; - } - List<DetectionAlertConfigDTO> alertConfigDTOS = this.detectionAlertConfigDAO - .findByPredicate(Predicate.EQ("name", MapUtils.getString(newAlertConfigMap, PROP_SUBS_GROUP_NAME))); - if (!alertConfigDTOS.isEmpty()) { - responseMessage.put("message", "Subscription group name is already taken. Please use a different name."); - return null; - } - - // Translate config from YAML to detection alert config (JSON) - DetectionAlertConfigDTO alertConfig; - try { - alertConfig = this.alertConfigTranslator.translate(newAlertConfigMap); - } catch (Exception e){ - responseMessage.put("message", e.getMessage()); - return null; - } - alertConfig.setYaml(yamlAlertConfig); - - // Validate the config before saving it - if (!alertValidator.validateConfig(alertConfig, responseMessage)) { - return null; - } - - return alertConfig; - } - @PUT @Path("/notification/{id}") @Produces(MediaType.APPLICATION_JSON) @Consumes(MediaType.TEXT_PLAIN) @ApiOperation("Edit a notification group using a YAML config") - @SuppressWarnings("unchecked") - public Response updateDetectionAlertConfig( + public Response updateDetectionAlertConfigApi( @ApiParam("payload") String yamlAlertConfig, @ApiParam("the detection alert config id to edit") @PathParam("id") long id) { Map<String, String> responseMessage = new HashMap<>(); try { - DetectionAlertConfigDTO alertDTO = this.detectionAlertConfigDAO.findById(id); - DetectionAlertConfigDTO updatedAlertConfig = updateDetectionAlertConfig(alertDTO, yamlAlertConfig, responseMessage); - if (updatedAlertConfig == null) { - return Response.status(Response.Status.BAD_REQUEST).entity(responseMessage).build(); - } + DetectionAlertConfigDTO updatedAlertConfig = updateDetectionAlertConfig(id, yamlAlertConfig); + Preconditions.checkNotNull(updatedAlertConfig); int detectionAlertConfigId = this.detectionAlertConfigDAO.update(updatedAlertConfig); if (detectionAlertConfigId <= 0) { - responseMessage.put("message", "Failed to update the detection alert config."); - responseMessage.put("more-info", "Zero records updated. Check for DB issues. YAML config = " + yamlAlertConfig); - return Response.serverError().entity(responseMessage).build(); + throw new RuntimeException("Failed to update the detection alert config."); } } catch (Exception e) { - responseMessage.put("message", "Failed to update the detection alert config."); - responseMessage.put("more-info", "Exception = " + e); + LOG.error("Error updating notification group " + id + " with payload " + yamlAlertConfig, e); + responseMessage.put("message", "Failed to update the notification group " + id); + responseMessage.put("more-info", "Error = " + e.getMessage()); return Response.serverError().entity(responseMessage).build(); } + LOG.info("Notification group " + id + " updated successfully with payload " + yamlAlertConfig); responseMessage.put("message", "The YAML alert config was updated successfully."); return Response.ok().entity(responseMessage).build(); } - public DetectionAlertConfigDTO updateDetectionAlertConfig(DetectionAlertConfigDTO oldAlertConfig, String yamlAlertConfig, - Map<String,String> responseMessage) { - if (oldAlertConfig == null) { - responseMessage.put("message", "Cannot find subscription group"); - return null; - } - - if (!alertValidator.validateYAMLConfig(yamlAlertConfig, responseMessage)) { - return null; - } - - TreeMap<String, Object> newAlertConfigMap = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); - newAlertConfigMap.putAll((Map<String, Object>) this.yaml.load(yamlAlertConfig)); - - // Search for the detection alert config's reference in the db - String subsGroupName = MapUtils.getString(newAlertConfigMap, PROP_SUBS_GROUP_NAME); - if (StringUtils.isEmpty(subsGroupName)) { - responseMessage.put("message", "Subscription group name field cannot be left empty."); - return null; - } - - DetectionAlertConfigDTO newAlertConfig; - try { - newAlertConfig = this.alertConfigTranslator.translate(newAlertConfigMap); - } catch (Exception e){ - responseMessage.put("message", e.getMessage()); - return null; - } - - // Translate config from YAML to detection alert config (JSON) - DetectionAlertConfigDTO updatedAlertConfig = updateDetectionAlertConfig(oldAlertConfig, newAlertConfig); - updatedAlertConfig.setYaml(yamlAlertConfig); - - // Validate before updating the config - if (!alertValidator.validateUpdatedConfig(updatedAlertConfig, oldAlertConfig, responseMessage)) { - return null; - } - - return updatedAlertConfig; - } - @POST @Path("/preview") @Produces(MediaType.APPLICATION_JSON) @@ -535,27 +501,4 @@ public class YamlResource { } return yamlObjects; } - - /** - * Update the existing {@code oldAlertConfig} with the new {@code newAlertConfig} - * - * Update all the fields except the vector clocks and high watermark. The clocks and watermarks - * are managed by the platform. They shouldn't be reset by the user. - */ - public DetectionAlertConfigDTO updateDetectionAlertConfig(DetectionAlertConfigDTO oldAlertConfig, - DetectionAlertConfigDTO newAlertConfig) { - oldAlertConfig.setName(newAlertConfig.getName()); - oldAlertConfig.setCronExpression(newAlertConfig.getCronExpression()); - oldAlertConfig.setApplication(newAlertConfig.getApplication()); - oldAlertConfig.setFrom(newAlertConfig.getFrom()); - oldAlertConfig.setSubjectType(newAlertConfig.getSubjectType()); - oldAlertConfig.setReferenceLinks(newAlertConfig.getReferenceLinks()); - oldAlertConfig.setActive(newAlertConfig.isActive()); - oldAlertConfig.setAlertSchemes(newAlertConfig.getAlertSchemes()); - oldAlertConfig.setAlertSuppressors(newAlertConfig.getAlertSuppressors()); - oldAlertConfig.setOnlyFetchLegacyAnomalies(newAlertConfig.isOnlyFetchLegacyAnomalies()); - oldAlertConfig.setProperties(newAlertConfig.getProperties()); - - return oldAlertConfig; - } } diff --git a/thirdeye/thirdeye-pinot/src/test/java/org/apache/pinot/thirdeye/detection/yaml/YamlResourceTest.java b/thirdeye/thirdeye-pinot/src/test/java/org/apache/pinot/thirdeye/detection/yaml/YamlResourceTest.java index b09b548..7b53bfa 100644 --- a/thirdeye/thirdeye-pinot/src/test/java/org/apache/pinot/thirdeye/detection/yaml/YamlResourceTest.java +++ b/thirdeye/thirdeye-pinot/src/test/java/org/apache/pinot/thirdeye/detection/yaml/YamlResourceTest.java @@ -7,14 +7,15 @@ import org.apache.pinot.thirdeye.datalayer.dto.DetectionAlertConfigDTO; import org.apache.pinot.thirdeye.datalayer.dto.DetectionConfigDTO; import org.apache.pinot.thirdeye.datasource.DAORegistry; import org.apache.pinot.thirdeye.detection.annotation.registry.DetectionAlertRegistry; -import org.apache.pinot.thirdeye.detection.annotation.registry.DetectionRegistry; import java.io.IOException; import java.util.HashMap; import java.util.Map; import org.apache.commons.io.IOUtils; import org.testng.Assert; import org.testng.annotations.AfterClass; +import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeClass; +import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -24,7 +25,7 @@ public class YamlResourceTest { private YamlResource yamlResource; private DAORegistry daoRegistry; - @BeforeClass + @BeforeMethod public void beforeClass() { testDAOProvider = DAOTestBase.getInstance(); this.yamlResource = new YamlResource(); @@ -40,96 +41,129 @@ public class YamlResourceTest { DetectionAlertRegistry.getInstance().registerAlertFilter("DIMENSIONAL_ALERTER_PIPELINE", "DimClass"); } - @AfterClass(alwaysRun = true) + @AfterMethod(alwaysRun = true) void afterClass() { testDAOProvider.cleanup(); } - @Test(priority=1) + @Test public void testCreateDetectionAlertConfig() throws IOException { - Map<String, String> responseMessage = new HashMap<>(); - DetectionAlertConfigDTO alertDTO; - String blankYaml = ""; - alertDTO = this.yamlResource.createDetectionAlertConfig(blankYaml, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "The config file cannot be blank."); + try { + this.yamlResource.createDetectionAlertConfig(blankYaml); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "The Yaml Payload in the request is empty."); + } String inValidYaml = "application:test:application"; - alertDTO = this.yamlResource.createDetectionAlertConfig(inValidYaml, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "There was an error parsing the yaml file. Check for syntax issues."); + try { + this.yamlResource.createDetectionAlertConfig(inValidYaml); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "Error parsing the Yaml input. Check for syntax issues."); + } String noSubscriptGroupYaml = "application: test_application"; - alertDTO = this.yamlResource.createDetectionAlertConfig(noSubscriptGroupYaml, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "Subscription group name field cannot be left empty."); + try { + this.yamlResource.createDetectionAlertConfig(noSubscriptGroupYaml); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "Subscription group name field cannot be left empty."); + } String appFieldMissingYaml = IOUtils.toString(this.getClass().getResourceAsStream("alertconfig/alert-config-1.yaml")); - alertDTO = this.yamlResource.createDetectionAlertConfig(appFieldMissingYaml, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "Application field cannot be left empty"); + try { + this.yamlResource.createDetectionAlertConfig(appFieldMissingYaml); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "Application field cannot be left empty"); + } String appMissingYaml = IOUtils.toString(this.getClass().getResourceAsStream("alertconfig/alert-config-2.yaml")); - alertDTO = this.yamlResource.createDetectionAlertConfig(appMissingYaml, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "Application name doesn't exist in our registry." - + " Please use an existing application name. You may search for registered applications from the ThirdEye" - + " dashboard or reach out to ask_thirdeye if you wish to setup a new application."); + try { + this.yamlResource.createDetectionAlertConfig(appMissingYaml); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "Application name doesn't exist in our registry." + + " Please use an existing application name. You may search for registered applications from the ThirdEye" + + " dashboard or reach out to ask_thirdeye if you wish to setup a new application."); + } DetectionAlertConfigDTO oldAlertDTO = new DetectionAlertConfigDTO(); oldAlertDTO.setName("test_group"); daoRegistry.getDetectionAlertConfigManager().save(oldAlertDTO); - String groupExists = IOUtils.toString(this.getClass().getResourceAsStream("alertconfig/alert-config-3.yaml")); - alertDTO = this.yamlResource.createDetectionAlertConfig(groupExists, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "Subscription group name is already taken. Please use a different name."); - ApplicationDTO request = new ApplicationDTO(); request.setApplication("test_application"); request.setRecipients("a...@abc.in"); daoRegistry.getApplicationDAO().save(request); + String groupExists = IOUtils.toString(this.getClass().getResourceAsStream("alertconfig/alert-config-3.yaml")); + try { + this.yamlResource.createDetectionAlertConfig(groupExists); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "Subscription group name is already taken. Please use a different name."); + } + String validYaml = IOUtils.toString(this.getClass().getResourceAsStream("alertconfig/alert-config-4.yaml")); - alertDTO = this.yamlResource.createDetectionAlertConfig(validYaml, responseMessage); - Assert.assertNotNull(alertDTO); - Assert.assertEquals(alertDTO.getName(), "Subscription Group Name"); + try { + DetectionAlertConfigDTO alert = this.yamlResource.createDetectionAlertConfig(validYaml); + Assert.assertNotNull(alert); + Assert.assertEquals(alert.getName(), "Subscription Group Name"); + } catch (Exception e) { + Assert.fail("Exception should not be thrown for valid yaml"); + } } - @Test(priority=2) + @Test public void testUpdateDetectionAlertConfig() throws IOException { DetectionAlertConfigDTO oldAlertDTO = new DetectionAlertConfigDTO(); oldAlertDTO.setName("Subscription Group Name"); oldAlertDTO.setApplication("Random Application"); - daoRegistry.getDetectionAlertConfigManager().save(oldAlertDTO); + long oldId = daoRegistry.getDetectionAlertConfigManager().save(oldAlertDTO); - Map<String, String> responseMessage = new HashMap<>(); DetectionAlertConfigDTO alertDTO; - alertDTO = this.yamlResource.updateDetectionAlertConfig(null, "", responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "Cannot find subscription group"); + try { + this.yamlResource.updateDetectionAlertConfig(-1, ""); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "Cannot find subscription group -1"); + } String blankYaml = ""; - alertDTO = this.yamlResource.updateDetectionAlertConfig(oldAlertDTO, blankYaml, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "The config file cannot be blank."); + try { + this.yamlResource.updateDetectionAlertConfig(oldId, blankYaml); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "The Yaml Payload in the request is empty."); + } String inValidYaml = "application:test:application"; - alertDTO = this.yamlResource.updateDetectionAlertConfig(oldAlertDTO, inValidYaml, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "There was an error parsing the yaml file. Check for syntax issues."); + try { + this.yamlResource.updateDetectionAlertConfig(oldId, inValidYaml); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "Error parsing the Yaml input. Check for syntax issues."); + } String noSubscriptGroupYaml = "application: test_application"; - alertDTO = this.yamlResource.updateDetectionAlertConfig(oldAlertDTO, noSubscriptGroupYaml, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "Subscription group name field cannot be left empty."); + try { + this.yamlResource.updateDetectionAlertConfig(oldId, noSubscriptGroupYaml); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "Subscription group name field cannot be left empty."); + } String appFieldMissingYaml = IOUtils.toString(this.getClass().getResourceAsStream("alertconfig/alert-config-1.yaml")); - alertDTO = this.yamlResource.updateDetectionAlertConfig(oldAlertDTO, appFieldMissingYaml, responseMessage); - Assert.assertNull(alertDTO); - Assert.assertEquals(responseMessage.get("message"), "Application field cannot be left empty"); + try { + this.yamlResource.updateDetectionAlertConfig(oldId, appFieldMissingYaml); + Assert.fail("Exception not thrown on empty yaml"); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), "Application field cannot be left empty"); + } ApplicationDTO request = new ApplicationDTO(); request.setApplication("test_application"); @@ -137,10 +171,14 @@ public class YamlResourceTest { daoRegistry.getApplicationDAO().save(request); String validYaml = IOUtils.toString(this.getClass().getResourceAsStream("alertconfig/alert-config-3.yaml")); - alertDTO = this.yamlResource.updateDetectionAlertConfig(oldAlertDTO, validYaml, responseMessage); - Assert.assertNotNull(alertDTO); - Assert.assertEquals(alertDTO.getName(), "test_group"); - Assert.assertEquals(alertDTO.getApplication(), "test_application"); + try { + alertDTO = this.yamlResource.updateDetectionAlertConfig(oldId, validYaml); + Assert.assertNotNull(alertDTO); + Assert.assertEquals(alertDTO.getName(), "test_group"); + Assert.assertEquals(alertDTO.getApplication(), "test_application"); + } catch (Exception e) { + Assert.fail("Exception should not be thrown for valid yaml" + e.getMessage()); + } } } --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org