This is an automated email from the ASF dual-hosted git repository.
bowenliang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kyuubi.git
The following commit(s) were added to refs/heads/master by this push:
new 9b932df25 [KYUUBI #5213] [Improvement] Check config value by enum
values
9b932df25 is described below
commit 9b932df25af17c345c22fa4759c5e61ae57c24f4
Author: Bowen Liang <[email protected]>
AuthorDate: Wed Aug 30 22:32:39 2023 +0800
[KYUUBI #5213] [Improvement] Check config value by enum values
### _Why are the changes needed?_
- check config values in the range of targeted enum values
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including
negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [x] [Run
test](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests)
locally before make a pull request
### _Was this patch authored or co-authored using generative AI tooling?_
No.
Closes #5213 from bowenliang123/config-checkenum.
Closes #5213
857af7c6b [Bowen Liang] is valid enum
2862e5e7c [Bowen Liang] embrace
afe6a5333 [Bowen Liang] assert
00d22f73b [liangbowen] support checking config in the range of enumeration
values
Lead-authored-by: Bowen Liang <[email protected]>
Co-authored-by: liangbowen <[email protected]>
Signed-off-by: liangbowen <[email protected]>
---
.../org/apache/kyuubi/config/ConfigBuilder.scala | 22 +++++++++++++++++
.../org/apache/kyuubi/config/KyuubiConf.scala | 21 ++++++----------
.../apache/kyuubi/config/ConfigBuilderSuite.scala | 18 ++++++++++++++
.../KyuubiAuthenticationFactorySuite.scala | 7 +++---
.../service/authentication/SaslQOPSuite.scala | 2 +-
.../apache/kyuubi/ha/HighAvailabilityConf.scala | 6 ++---
.../org/apache/kyuubi/metrics/MetricsConf.scala | 4 +---
.../scala/org/apache/kyuubi/util/EnumUtils.scala | 28 +++++++---------------
.../org/apache/kyuubi/util/AssertionUtils.scala | 2 ++
9 files changed, 66 insertions(+), 44 deletions(-)
diff --git
a/kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigBuilder.scala
b/kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigBuilder.scala
index b64e5c5eb..d6de40241 100644
--- a/kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigBuilder.scala
+++ b/kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigBuilder.scala
@@ -24,6 +24,8 @@ import java.util.regex.PatternSyntaxException
import scala.util.{Failure, Success, Try}
import scala.util.matching.Regex
+import org.apache.kyuubi.util.EnumUtils._
+
private[kyuubi] case class ConfigBuilder(key: String) {
private[config] var _doc = ""
@@ -203,6 +205,26 @@ private[kyuubi] case class TypedConfigBuilder[T](
}
}
+ /** Checks if the user-provided value for the config matches the value set
of the enumeration. */
+ def checkValues(enumeration: Enumeration): TypedConfigBuilder[T] = {
+ transform { v =>
+ val isValid = v match {
+ case iter: Iterable[Any] => isValidEnums(enumeration, iter)
+ case name => isValidEnum(enumeration, name)
+ }
+ if (!isValid) {
+ val actualValueStr = v match {
+ case iter: Iterable[Any] => iter.mkString(",")
+ case value => value.toString
+ }
+ throw new IllegalArgumentException(
+ s"The value of ${parent.key} should be one of
${enumeration.values.mkString(", ")}," +
+ s" but was $actualValueStr")
+ }
+ v
+ }
+ }
+
/** Turns the config entry into a sequence of values of the underlying type.
*/
def toSequence(sp: String = ","): TypedConfigBuilder[Seq[T]] = {
parent._type = "seq"
diff --git
a/kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
b/kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
index 2c158b8e8..bec69e95e 100644
--- a/kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
+++ b/kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
@@ -415,9 +415,7 @@ object KyuubiConf {
.stringConf
.transformToUpperCase
.toSequence()
- .checkValue(
- _.forall(FrontendProtocols.values.map(_.toString).contains),
- s"the frontend protocol should be one or more of
${FrontendProtocols.values.mkString(",")}")
+ .checkValues(FrontendProtocols)
.createWithDefault(Seq(
FrontendProtocols.THRIFT_BINARY.toString,
FrontendProtocols.REST.toString))
@@ -802,9 +800,7 @@ object KyuubiConf {
.stringConf
.transformToUpperCase
.toSet()
- .checkValue(
- _.forall(AuthTypes.values.map(_.toString).contains),
- s"the authentication type should be one or more of
${AuthTypes.values.mkString(",")}")
+ .checkValues(AuthTypes)
.createWithDefault(Set(AuthTypes.NONE.toString))
val AUTHENTICATION_CUSTOM_CLASS: OptionalConfigEntry[String] =
@@ -1036,7 +1032,7 @@ object KyuubiConf {
.version("1.0.0")
.serverOnly
.stringConf
- .checkValues(SaslQOP.values.map(_.toString))
+ .checkValues(SaslQOP)
.transformToLowerCase
.createWithDefault(SaslQOP.AUTH.toString)
@@ -1926,7 +1922,7 @@ object KyuubiConf {
.version("1.0.0")
.stringConf
.transformToUpperCase
- .checkValues(ShareLevel.values.map(_.toString))
+ .checkValues(ShareLevel)
.createWithDefault(ShareLevel.USER.toString)
// [ZooKeeper Data Model]
@@ -2007,7 +2003,7 @@ object KyuubiConf {
.version("1.4.0")
.stringConf
.transformToUpperCase
- .checkValues(EngineType.values.map(_.toString))
+ .checkValues(EngineType)
.createWithDefault(EngineType.SPARK_SQL.toString)
val ENGINE_POOL_IGNORE_SUBDOMAIN: ConfigEntry[Boolean] =
@@ -2395,10 +2391,7 @@ object KyuubiConf {
.version("1.7.0")
.stringConf
.transformToUpperCase
- .checkValue(
- mode => Set("PLAIN", "JSON").contains(mode),
- "Invalid value for 'kyuubi.operation.plan.only.output.style'. Valid
values are " +
- "'plain', 'json'.")
+ .checkValues(Set("PLAIN", "JSON"))
.createWithDefault(PlainStyle.name)
val OPERATION_PLAN_ONLY_EXCLUDES: ConfigEntry[Set[String]] =
@@ -2446,7 +2439,7 @@ object KyuubiConf {
.version("1.5.0")
.stringConf
.transformToUpperCase
- .checkValues(OperationLanguages.values.map(_.toString))
+ .checkValues(OperationLanguages)
.createWithDefault(OperationLanguages.SQL.toString)
val SESSION_CONF_ADVISOR: OptionalConfigEntry[String] =
diff --git
a/kyuubi-common/src/test/scala/org/apache/kyuubi/config/ConfigBuilderSuite.scala
b/kyuubi-common/src/test/scala/org/apache/kyuubi/config/ConfigBuilderSuite.scala
index 86dc51155..78429d27c 100644
---
a/kyuubi-common/src/test/scala/org/apache/kyuubi/config/ConfigBuilderSuite.scala
+++
b/kyuubi-common/src/test/scala/org/apache/kyuubi/config/ConfigBuilderSuite.scala
@@ -18,6 +18,7 @@
package org.apache.kyuubi.config
import org.apache.kyuubi.KyuubiFunSuite
+import org.apache.kyuubi.util.AssertionUtils._
class ConfigBuilderSuite extends KyuubiFunSuite {
@@ -125,4 +126,21 @@ class ConfigBuilderSuite extends KyuubiFunSuite {
val e = intercept[IllegalArgumentException](kyuubiConf.get(intConf))
assert(e.getMessage equals "'-1' in kyuubi.invalid.config is invalid. must
be positive integer")
}
+
+ test("invalid config for enum") {
+ object TempEnum extends Enumeration {
+ type TempEnum = Value
+ val ValA, ValB = Value
+ }
+ val stringConf = ConfigBuilder("kyuubi.invalid.config.enum")
+ .stringConf
+ .checkValues(TempEnum)
+ .createWithDefault("ValA")
+ assert(stringConf.key === "kyuubi.invalid.config.enum")
+ assert(stringConf.defaultVal.get === "ValA")
+ val kyuubiConf = KyuubiConf().set(stringConf.key, "ValC")
+ KyuubiConf.register(stringConf)
+ interceptEquals[IllegalArgumentException] { kyuubiConf.get(stringConf) }(
+ "The value of kyuubi.invalid.config.enum should be one of ValA, ValB,
but was ValC")
+ }
}
diff --git
a/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/KyuubiAuthenticationFactorySuite.scala
b/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/KyuubiAuthenticationFactorySuite.scala
index 949342629..316c9b2df 100644
---
a/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/KyuubiAuthenticationFactorySuite.scala
+++
b/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/KyuubiAuthenticationFactorySuite.scala
@@ -25,6 +25,7 @@ import org.apache.thrift.transport.TSaslServerTransport
import org.apache.kyuubi.{KyuubiFunSuite, KyuubiSQLException}
import org.apache.kyuubi.config.KyuubiConf
import
org.apache.kyuubi.service.authentication.PlainSASLServer.SaslPlainProvider
+import org.apache.kyuubi.util.AssertionUtils._
import org.apache.kyuubi.util.KyuubiHadoopUtils
class KyuubiAuthenticationFactorySuite extends KyuubiFunSuite {
@@ -57,9 +58,9 @@ class KyuubiAuthenticationFactorySuite extends KyuubiFunSuite
{
test("AuthType Other") {
val conf = KyuubiConf().set(KyuubiConf.AUTHENTICATION_METHOD,
Set("INVALID"))
- val e = intercept[IllegalArgumentException](new
KyuubiAuthenticationFactory(conf))
- assert(e.getMessage contains "the authentication type should be one or
more of" +
- " NOSASL,NONE,LDAP,JDBC,KERBEROS,CUSTOM")
+ interceptEquals[IllegalArgumentException] { new
KyuubiAuthenticationFactory(conf) }(
+ "The value of kyuubi.authentication should be one of" +
+ " NOSASL, NONE, LDAP, JDBC, KERBEROS, CUSTOM, but was INVALID")
}
test("AuthType LDAP") {
diff --git
a/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/SaslQOPSuite.scala
b/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/SaslQOPSuite.scala
index c48f12aa7..6cf2793d2 100644
---
a/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/SaslQOPSuite.scala
+++
b/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/SaslQOPSuite.scala
@@ -34,7 +34,7 @@ class SaslQOPSuite extends KyuubiFunSuite {
val e = intercept[IllegalArgumentException](conf.get(SASL_QOP))
assert(e.getMessage ===
"The value of kyuubi.authentication.sasl.qop should be one of" +
- " auth, auth-conf, auth-int, but was abc")
+ " auth, auth-int, auth-conf, but was abc")
}
}
diff --git
a/kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
b/kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
index 28305ac52..626557008 100644
--- a/kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
+++ b/kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
@@ -79,7 +79,7 @@ object HighAvailabilityConf {
s"${AuthTypes.values.mkString("<ul><li>", "</li><li> ",
"</li></ul>")}")
.version("1.3.2")
.stringConf
- .checkValues(AuthTypes.values.map(_.toString))
+ .checkValues(AuthTypes)
.createWithDefault(AuthTypes.NONE.toString)
val HA_ZK_ENGINE_AUTH_TYPE: ConfigEntry[String] =
@@ -88,7 +88,7 @@ object HighAvailabilityConf {
s"${AuthTypes.values.mkString("<ul><li>", "</li><li> ",
"</li></ul>")}")
.version("1.3.2")
.stringConf
- .checkValues(AuthTypes.values.map(_.toString))
+ .checkValues(AuthTypes)
.createWithDefault(AuthTypes.NONE.toString)
val HA_ZK_AUTH_SERVER_PRINCIPAL: OptionalConfigEntry[String] =
@@ -160,7 +160,7 @@ object HighAvailabilityConf {
s" ${RetryPolicies.values.mkString("<ul><li>", "</li><li> ",
"</li></ul>")}")
.version("1.0.0")
.stringConf
- .checkValues(RetryPolicies.values.map(_.toString))
+ .checkValues(RetryPolicies)
.createWithDefault(RetryPolicies.EXPONENTIAL_BACKOFF.toString)
val HA_ZK_NODE_TIMEOUT: ConfigEntry[Long] =
diff --git
a/kyuubi-metrics/src/main/scala/org/apache/kyuubi/metrics/MetricsConf.scala
b/kyuubi-metrics/src/main/scala/org/apache/kyuubi/metrics/MetricsConf.scala
index b22aa8909..fe11f6eb1 100644
--- a/kyuubi-metrics/src/main/scala/org/apache/kyuubi/metrics/MetricsConf.scala
+++ b/kyuubi-metrics/src/main/scala/org/apache/kyuubi/metrics/MetricsConf.scala
@@ -45,9 +45,7 @@ object MetricsConf {
.stringConf
.transformToUpperCase
.toSet()
- .checkValue(
- _.forall(ReporterType.values.map(_.toString).contains),
- s"the reporter type should be one or more of
${ReporterType.values.mkString(",")}")
+ .checkValues(ReporterType)
.createWithDefault(Set(JSON.toString))
val METRICS_CONSOLE_INTERVAL: ConfigEntry[Long] =
buildConf("kyuubi.metrics.console.interval")
diff --git
a/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/SaslQOPSuite.scala
b/kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/EnumUtils.scala
similarity index 52%
copy from
kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/SaslQOPSuite.scala
copy to kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/EnumUtils.scala
index c48f12aa7..0ab3be381 100644
---
a/kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/SaslQOPSuite.scala
+++ b/kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/EnumUtils.scala
@@ -14,27 +14,15 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+package org.apache.kyuubi.util
-package org.apache.kyuubi.service.authentication
+import scala.util.Try
-import org.apache.kyuubi.KyuubiFunSuite
-import org.apache.kyuubi.config.KyuubiConf
-import org.apache.kyuubi.config.KyuubiConf.SASL_QOP
-
-class SaslQOPSuite extends KyuubiFunSuite {
-
- test("sasl qop") {
- val conf = KyuubiConf(false)
- assert(conf.get(SASL_QOP) === SaslQOP.AUTH.toString)
- SaslQOP.values.foreach { q =>
- conf.set(SASL_QOP, q.toString)
- assert(SaslQOP.withName(conf.get(SASL_QOP)) === q)
- }
- conf.set(SASL_QOP, "abc")
- val e = intercept[IllegalArgumentException](conf.get(SASL_QOP))
- assert(e.getMessage ===
- "The value of kyuubi.authentication.sasl.qop should be one of" +
- " auth, auth-conf, auth-int, but was abc")
- }
+object EnumUtils {
+ def isValidEnum(enumeration: Enumeration, enumName: Any): Boolean = Try {
+ enumeration.withName(enumName.toString)
+ }.isSuccess
+ def isValidEnums(enumeration: Enumeration, enumNames: Iterable[Any]):
Boolean =
+ enumNames.forall(isValidEnum(enumeration, _))
}
diff --git
a/kyuubi-util-scala/src/test/scala/org/apache/kyuubi/util/AssertionUtils.scala
b/kyuubi-util-scala/src/test/scala/org/apache/kyuubi/util/AssertionUtils.scala
index a6cd9b862..e12eb1bd4 100644
---
a/kyuubi-util-scala/src/test/scala/org/apache/kyuubi/util/AssertionUtils.scala
+++
b/kyuubi-util-scala/src/test/scala/org/apache/kyuubi/util/AssertionUtils.scala
@@ -61,6 +61,7 @@ object AssertionUtils {
def interceptEquals[T <: Exception](f: => Any)(expected: String)(implicit
classTag: ClassTag[T],
pos: source.Position): Unit = {
+ assert(expected != null)
val exception = intercept[T](f)(classTag, pos)
assertResult(expected)(exception.getMessage)
}
@@ -72,6 +73,7 @@ object AssertionUtils {
def interceptContains[T <: Exception](f: => Any)(contained: String)(implicit
classTag: ClassTag[T],
pos: source.Position): Unit = {
+ assert(contained != null)
val exception = intercept[T](f)(classTag, pos)
assert(exception.getMessage.contains(contained))
}
