From c19a10a0c560b0d3179c8cc69a77c43542a3b87a Mon Sep 17 00:00:00 2001 From: AWSHurneyt Date: Fri, 12 Apr 2024 19:28:47 -0700 Subject: [PATCH] Removed CommonUtilsException assets and references. (#639) * Added validation for the clusters field. Refactored ClusterMetricsInput validiation to throw 4xx-level CommonUtilsExceptions instead of 5xx-level IllegalArgumentException. Signed-off-by: AWSHurneyt * Moved some regex from alerting plugin to common utils. Signed-off-by: AWSHurneyt * Moved cluster-based regex to separate file. Signed-off-by: AWSHurneyt * Fixed ktlint error. Signed-off-by: AWSHurneyt * Fixed regex. Moved cluster-related regexes. Signed-off-by: AWSHurneyt * Removed CommonUtilsException. Team decided IllegalArgumentExceptions should be caught in the plugins themselves. Signed-off-by: AWSHurneyt --------- Signed-off-by: AWSHurneyt --- .../alerting/model/ClusterMetricsInput.kt | 76 ++++++++----------- .../alerting/util/CommonUtilsException.kt | 71 ----------------- .../model/ClusterMetricsInputTests.kt | 37 +++++---- 3 files changed, 51 insertions(+), 133 deletions(-) delete mode 100644 src/main/kotlin/org/opensearch/commons/alerting/util/CommonUtilsException.kt diff --git a/src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt b/src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt index 7da1e208..4eb9ecfe 100644 --- a/src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt +++ b/src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt @@ -3,7 +3,6 @@ package org.opensearch.commons.alerting.model import org.apache.commons.validator.routines.UrlValidator import org.apache.hc.core5.net.URIBuilder import org.opensearch.common.CheckedFunction -import org.opensearch.commons.alerting.util.CommonUtilsException import org.opensearch.commons.utils.CLUSTER_NAME_REGEX import org.opensearch.core.ParseField import org.opensearch.core.common.io.stream.StreamInput @@ -33,46 +32,41 @@ data class ClusterMetricsInput( // Verify parameters are valid during creation init { - // Wrap any validation exceptions in CommonUtilsException. - try { - require(validateFields()) { - "The uri.api_type field, uri.path field, or uri.uri field must be defined." - } + require(validateFields()) { + "The uri.api_type field, uri.path field, or uri.uri field must be defined." + } - // Create an UrlValidator that only accepts "http" and "https" as valid scheme and allows local URLs. - val urlValidator = UrlValidator(arrayOf("http", "https"), UrlValidator.ALLOW_LOCAL_URLS) + // Create an UrlValidator that only accepts "http" and "https" as valid scheme and allows local URLs. + val urlValidator = UrlValidator(arrayOf("http", "https"), UrlValidator.ALLOW_LOCAL_URLS) - // Build url field by field if not provided as whole. - constructedUri = toConstructedUri() + // Build url field by field if not provided as whole. + constructedUri = toConstructedUri() - require(urlValidator.isValid(constructedUri.toString())) { - "Invalid URI constructed from the path and path_params inputs, or the url input." - } + require(urlValidator.isValid(constructedUri.toString())) { + "Invalid URI constructed from the path and path_params inputs, or the url input." + } - if (url.isNotEmpty() && validateFieldsNotEmpty()) { - require(constructedUri == constructUrlFromInputs()) { - "The provided URL and URI fields form different URLs." - } + if (url.isNotEmpty() && validateFieldsNotEmpty()) { + require(constructedUri == constructUrlFromInputs()) { + "The provided URL and URI fields form different URLs." } + } - require(constructedUri.host.lowercase() == SUPPORTED_HOST) { - "Only host '$SUPPORTED_HOST' is supported." - } - require(constructedUri.port == SUPPORTED_PORT) { - "Only port '$SUPPORTED_PORT' is supported." - } + require(constructedUri.host.lowercase() == SUPPORTED_HOST) { + "Only host '$SUPPORTED_HOST' is supported." + } + require(constructedUri.port == SUPPORTED_PORT) { + "Only port '$SUPPORTED_PORT' is supported." + } - if (clusters.isNotEmpty()) { - require(clusters.all { CLUSTER_NAME_REGEX.matches(it) }) { - "Cluster names are not valid." - } + if (clusters.isNotEmpty()) { + require(clusters.all { CLUSTER_NAME_REGEX.matches(it) }) { + "Cluster names are not valid." } - - clusterMetricType = findApiType(constructedUri.path) - this.parseEmptyFields() - } catch (exception: Exception) { - throw CommonUtilsException.wrap(exception) } + + clusterMetricType = findApiType(constructedUri.path) + this.parseEmptyFields() } @Throws(IOException::class) @@ -171,7 +165,7 @@ data class ClusterMetricsInput( /** * Isolates just the path parameters from the [ClusterMetricsInput] URI. * @return The path parameters portion of the [ClusterMetricsInput] URI. - * @throws CommonUtilsException if the [ClusterMetricType] requires path parameters, but none are supplied; + * @throws [IllegalArgumentException] if the [ClusterMetricType] requires path parameters, but none are supplied; * or when path parameters are provided for an [ClusterMetricType] that does not use path parameters. */ fun parsePathParams(): String { @@ -191,22 +185,18 @@ data class ClusterMetricsInput( pathParams = pathParams.trim('/') ILLEGAL_PATH_PARAMETER_CHARACTERS.forEach { character -> if (pathParams.contains(character)) { - throw CommonUtilsException.wrap( - IllegalArgumentException( - "The provided path parameters contain invalid characters or spaces. Please omit: " + ILLEGAL_PATH_PARAMETER_CHARACTERS.joinToString( - " " - ) - ) + throw IllegalArgumentException( + "The provided path parameters contain invalid characters or spaces. Please omit: " + ILLEGAL_PATH_PARAMETER_CHARACTERS.joinToString(" ") ) } } } if (apiType.requiresPathParams && pathParams.isEmpty()) { - throw CommonUtilsException.wrap(IllegalArgumentException("The API requires path parameters.")) + throw IllegalArgumentException("The API requires path parameters.") } if (!apiType.supportsPathParams && pathParams.isNotEmpty()) { - throw CommonUtilsException.wrap(IllegalArgumentException("The API does not use path parameters.")) + throw IllegalArgumentException("The API does not use path parameters.") } return pathParams @@ -216,7 +206,7 @@ data class ClusterMetricsInput( * Examines the path of a [ClusterMetricsInput] to determine which API is being called. * @param uriPath The path to examine. * @return The [ClusterMetricType] associated with the [ClusterMetricsInput] monitor. - * @throws CommonUtilsException when the API to call cannot be determined from the URI. + * @throws [IllegalArgumentException] when the API to call cannot be determined from the URI. */ private fun findApiType(uriPath: String): ClusterMetricType { var apiType = ClusterMetricType.BLANK @@ -228,7 +218,7 @@ data class ClusterMetricsInput( } } if (apiType.isBlank()) { - throw CommonUtilsException.wrap(IllegalArgumentException("The API could not be determined from the provided URI.")) + throw IllegalArgumentException("The API could not be determined from the provided URI.") } return apiType } diff --git a/src/main/kotlin/org/opensearch/commons/alerting/util/CommonUtilsException.kt b/src/main/kotlin/org/opensearch/commons/alerting/util/CommonUtilsException.kt deleted file mode 100644 index 0fb19610..00000000 --- a/src/main/kotlin/org/opensearch/commons/alerting/util/CommonUtilsException.kt +++ /dev/null @@ -1,71 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.commons.alerting.util - -import org.apache.logging.log4j.LogManager -import org.opensearch.OpenSearchException -import org.opensearch.OpenSearchSecurityException -import org.opensearch.OpenSearchStatusException -import org.opensearch.core.common.Strings -import org.opensearch.core.rest.RestStatus -import org.opensearch.index.IndexNotFoundException -import org.opensearch.index.engine.VersionConflictEngineException -import org.opensearch.indices.InvalidIndexNameException - -private val log = LogManager.getLogger(CommonUtilsException::class.java) - -class CommonUtilsException(message: String, val status: RestStatus, ex: Exception) : OpenSearchException(message, ex) { - - override fun status(): RestStatus { - return status - } - - companion object { - - @JvmStatic - fun wrap(ex: Exception): OpenSearchException { - log.error("Common utils error: $ex") - - var friendlyMsg = "Unknown error" - var status = RestStatus.INTERNAL_SERVER_ERROR - when (ex) { - is IndexNotFoundException -> { - status = ex.status() - friendlyMsg = "Configured indices are not found: ${ex.index}" - } - is OpenSearchSecurityException -> { - status = ex.status() - friendlyMsg = "User doesn't have permissions to execute this action. Contact administrator." - } - is OpenSearchStatusException -> { - status = ex.status() - friendlyMsg = ex.message as String - } - is IllegalArgumentException -> { - status = RestStatus.BAD_REQUEST - friendlyMsg = ex.message as String - } - is VersionConflictEngineException -> { - status = ex.status() - friendlyMsg = ex.message as String - } - is InvalidIndexNameException -> { - status = RestStatus.BAD_REQUEST - friendlyMsg = ex.message as String - } - else -> { - if (!Strings.isNullOrEmpty(ex.message)) { - friendlyMsg = ex.message as String - } - } - } - // Wrapping the origin exception as runtime to avoid it being formatted. - // Currently, alerting-kibana is using `error.root_cause.reason` as text in the toast message. - // Below logic is to set friendly message to error.root_cause.reason. - return CommonUtilsException(friendlyMsg, status, Exception("${ex.javaClass.name}: ${ex.message}")) - } - } -} diff --git a/src/test/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInputTests.kt b/src/test/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInputTests.kt index 8e92403c..0e739e7f 100644 --- a/src/test/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInputTests.kt +++ b/src/test/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInputTests.kt @@ -1,7 +1,6 @@ package org.opensearch.commons.alerting.model import org.junit.jupiter.api.Test -import org.opensearch.commons.alerting.util.CommonUtilsException import kotlin.test.assertEquals import kotlin.test.assertFailsWith @@ -79,7 +78,7 @@ class ClusterMetricsInputTests { path = "///" // WHEN + THEN - assertFailsWith("Invalid URL.") { + assertFailsWith("Invalid URL.") { ClusterMetricsInput(path, pathParams, url) } } @@ -90,7 +89,7 @@ class ClusterMetricsInputTests { url = "///" // WHEN + THEN - assertFailsWith("Invalid URL.") { + assertFailsWith("Invalid URL.") { ClusterMetricsInput(path, pathParams, url) } } @@ -135,7 +134,7 @@ class ClusterMetricsInputTests { url = "http://localhost:9200/_cluster/stats" // WHEN + THEN - assertFailsWith("The provided URL and URI fields form different URLs.") { + assertFailsWith("The provided URL and URI fields form different URLs.") { ClusterMetricsInput(path, pathParams, url) } } @@ -147,7 +146,7 @@ class ClusterMetricsInputTests { url = "http://localhost:9200/_cluster/stats/index1,index2,index3,index4,index5" // WHEN + THEN - assertFailsWith("The provided URL and URI fields form different URLs.") { + assertFailsWith("The provided URL and URI fields form different URLs.") { ClusterMetricsInput(path, pathParams, url) } } @@ -160,7 +159,7 @@ class ClusterMetricsInputTests { url = "" // WHEN + THEN - assertFailsWith("The uri.api_type field, uri.path field, or uri.uri field must be defined.") { + assertFailsWith("The uri.api_type field, uri.path field, or uri.uri field must be defined.") { ClusterMetricsInput(path, pathParams, url) } } @@ -173,7 +172,7 @@ class ClusterMetricsInputTests { url = "" // WHEN + THEN - assertFailsWith("The uri.api_type field, uri.path field, or uri.uri field must be defined.") { + assertFailsWith("The uri.api_type field, uri.path field, or uri.uri field must be defined.") { ClusterMetricsInput(path, pathParams, url) } } @@ -185,7 +184,7 @@ class ClusterMetricsInputTests { url = "invalidScheme://localhost:9200/_cluster/health" // WHEN + THEN - assertFailsWith("Invalid URL.") { + assertFailsWith("Invalid URL.") { ClusterMetricsInput(path, pathParams, url) } } @@ -197,7 +196,7 @@ class ClusterMetricsInputTests { url = "http://127.0.0.1:9200/_cluster/health" // WHEN + THEN - assertFailsWith("Only host '${ClusterMetricsInput.SUPPORTED_HOST}' is supported.") { + assertFailsWith("Only host '${ClusterMetricsInput.SUPPORTED_HOST}' is supported.") { ClusterMetricsInput(path, pathParams, url) } } @@ -209,7 +208,7 @@ class ClusterMetricsInputTests { url = "http://localhost:${ClusterMetricsInput.SUPPORTED_PORT + 1}/_cluster/health" // WHEN + THEN - assertFailsWith("Only port '${ClusterMetricsInput.SUPPORTED_PORT}' is supported.") { + assertFailsWith("Only port '${ClusterMetricsInput.SUPPORTED_PORT}' is supported.") { ClusterMetricsInput(path, pathParams, url) } } @@ -269,7 +268,7 @@ class ClusterMetricsInputTests { path = "/_cat/snapshots" // WHEN + THEN - assertFailsWith("The API requires path parameters.") { + assertFailsWith("The API requires path parameters.") { ClusterMetricsInput(path, pathParams, url) } } @@ -282,7 +281,7 @@ class ClusterMetricsInputTests { val clusterMetricsInput = ClusterMetricsInput(path, pathParams, url) // WHEN + THEN - assertFailsWith("The API does not use path parameters.") { + assertFailsWith("The API does not use path parameters.") { clusterMetricsInput.parsePathParams() } } @@ -296,7 +295,7 @@ class ClusterMetricsInputTests { val clusterMetricsInput = ClusterMetricsInput(path, pathParams, url) // WHEN + THEN - assertFailsWith( + assertFailsWith( "The provided path parameters contain invalid characters or spaces. Please omit: " + ILLEGAL_PATH_PARAMETER_CHARACTERS.joinToString(" ") ) { clusterMetricsInput.parsePathParams() @@ -394,7 +393,7 @@ class ClusterMetricsInputTests { path = "/_cat/paws" // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput(path, pathParams, url) } } @@ -406,7 +405,7 @@ class ClusterMetricsInputTests { pathParams = "index1,index2,index3,index4,index5" // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput(path, pathParams, url) } } @@ -418,7 +417,7 @@ class ClusterMetricsInputTests { url = "http://localhost:9200/_cat/paws" // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput(path, pathParams, url) } } @@ -430,7 +429,7 @@ class ClusterMetricsInputTests { url = "http://localhost:9200/_cat/paws/index1,index2,index3,index4,index5" // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput(path, pathParams, url) } } @@ -527,7 +526,7 @@ class ClusterMetricsInputTests { val clusters = listOf(it) // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput( path = path, pathParams = pathParams, @@ -547,7 +546,7 @@ class ClusterMetricsInputTests { val clusters = invalidClusters // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput( path = path, pathParams = pathParams,