From 4252e34bcf03a70444c61a53e0e8526b51026315 Mon Sep 17 00:00:00 2001 From: Rob Zienert Date: Wed, 15 Jan 2020 11:18:51 -0800 Subject: [PATCH] feat(kotlin): Adding detekt static code analysis for Kotlin modules (#450) * feat(kotlin): Adding detekt static code analysis * chore(kotlin): Apply detekt fixes --- .detekt.yml | 7 +++++++ build.gradle | 1 + gradle/kotlin.gradle | 18 ++++++++++++++++++ .../expressions/ExpressionFunctionProvider.kt | 14 +++++++++++--- .../kork/plugins/SpinnakerPluginManager.kt | 6 +++--- .../kork/plugins/SpringExtensionFactory.kt | 16 ++++++++++------ .../config/SpringEnvironmentConfigResolver.kt | 3 ++- .../loaders/SpinnakerDefaultPluginLoader.kt | 3 ++- .../sql/JooqToSpringExceptionTransformer.kt | 5 +++-- .../sql/config/ConnectionPoolProperties.kt | 1 + .../sql/config/DefaultSqlConfiguration.kt | 14 +++++++++++--- .../config/SecondaryPoolDialectCondition.kt | 19 ++++++++++++++----- .../kork/sql/config/SqlProperties.kt | 7 ++++--- .../kork/sql/health/SqlHealthProvider.kt | 3 ++- 14 files changed, 89 insertions(+), 28 deletions(-) create mode 100644 .detekt.yml diff --git a/.detekt.yml b/.detekt.yml new file mode 100644 index 000000000..697f68af9 --- /dev/null +++ b/.detekt.yml @@ -0,0 +1,7 @@ +style: + ThrowsCount: + active: false + +formatting: + ParameterListWrapping: + active: false diff --git a/build.gradle b/build.gradle index f8869204d..f7e998a16 100644 --- a/build.gradle +++ b/build.gradle @@ -35,6 +35,7 @@ buildscript { } classpath("com.netflix.nebula:nebula-kotlin-plugin:1.3.50") classpath("org.jetbrains.kotlin:kotlin-allopen:1.3.50") + classpath("io.gitlab.arturbosch.detekt:detekt-gradle-plugin:1.2.2") } } diff --git a/gradle/kotlin.gradle b/gradle/kotlin.gradle index b25858606..6775bc041 100644 --- a/gradle/kotlin.gradle +++ b/gradle/kotlin.gradle @@ -16,6 +16,7 @@ apply plugin: "nebula.kotlin" apply plugin: "kotlin-spring" +apply plugin: "io.gitlab.arturbosch.detekt" dependencies { testImplementation "org.junit.jupiter:junit-jupiter-api" @@ -31,6 +32,9 @@ dependencies { testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine" testRuntimeOnly "org.junit.vintage:junit-vintage-engine" testRuntimeOnly "org.jetbrains.spek:spek-junit-platform-engine" + + + detektPlugins "io.gitlab.arturbosch.detekt:detekt-formatting:1.2.2" } test { @@ -45,3 +49,17 @@ compileKotlin { jvmTarget = "1.8" } } + +detekt { + parallel = true + config = files("$rootDir/.detekt.yml") + buildUponDefaultConfig = true + reports { + xml { + enabled = false + } + txt { + enabled = false + } + } +} diff --git a/kork-expressions/src/main/kotlin/com/netflix/spinnaker/kork/expressions/ExpressionFunctionProvider.kt b/kork-expressions/src/main/kotlin/com/netflix/spinnaker/kork/expressions/ExpressionFunctionProvider.kt index 4c28d760f..16b5f6475 100644 --- a/kork-expressions/src/main/kotlin/com/netflix/spinnaker/kork/expressions/ExpressionFunctionProvider.kt +++ b/kork-expressions/src/main/kotlin/com/netflix/spinnaker/kork/expressions/ExpressionFunctionProvider.kt @@ -87,14 +87,21 @@ interface ExpressionFunctionProvider : ExtensionPoint { val parameters: List, val documentation: FunctionDocumentation? ) { - @Deprecated("Please use the overload with description", replaceWith = ReplaceWith("FunctionDefinition(name, \"\", functionParameters)")) + @Deprecated( + "Please use the overload with description", + replaceWith = ReplaceWith("FunctionDefinition(name, \"\", functionParameters)")) constructor(name: String, vararg functionParameters: FunctionParameter) : this(name, "", listOf(*functionParameters), null) constructor(name: String, description: String, vararg functionParameters: FunctionParameter) : this(name, description, listOf(*functionParameters), null) - constructor(name: String, description: String, documentation: FunctionDocumentation, vararg functionParameters: FunctionParameter) : + constructor( + name: String, + description: String, + documentation: FunctionDocumentation, + vararg functionParameters: FunctionParameter + ) : this(name, description, listOf(*functionParameters), documentation) } @@ -131,7 +138,8 @@ interface ExpressionFunctionProvider : ExtensionPoint { * This is used by deck to display in-line docs for a SpEL function * * @param usage example usage, e.g. "#stage('bake in us-east').hasSucceeded" - * @param description explanation of the usage sample, markdown supported e.g. "checks if the bake stage has completed successfully" + * @param description explanation of the usage sample, markdown supported + * e.g. "checks if the bake stage has completed successfully" */ data class FunctionUsageExample( val usage: String, diff --git a/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/SpinnakerPluginManager.kt b/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/SpinnakerPluginManager.kt index e9d070121..07253dee9 100644 --- a/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/SpinnakerPluginManager.kt +++ b/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/SpinnakerPluginManager.kt @@ -24,8 +24,8 @@ import com.netflix.spinnaker.kork.plugins.loaders.SpinnakerJarPluginLoader import org.pf4j.CompoundPluginLoader import org.pf4j.DefaultPluginManager import org.pf4j.ExtensionFactory -import org.pf4j.PluginLoader import org.pf4j.PluginDescriptorFinder +import org.pf4j.PluginLoader import org.pf4j.PluginStatusProvider import org.pf4j.PluginWrapper import java.nio.file.Path @@ -46,11 +46,11 @@ open class SpinnakerPluginManager( private val springExtensionFactory: ExtensionFactory = SpringExtensionFactory(this, configResolver) - private inner class ExtensionFactoryDelegate() : ExtensionFactory { + private inner class ExtensionFactoryDelegate : ExtensionFactory { override fun create(extensionClass: Class?): T = springExtensionFactory.create(extensionClass) } - private inner class PluginStatusProviderDelegate() : PluginStatusProvider { + private inner class PluginStatusProviderDelegate : PluginStatusProvider { override fun disablePlugin(pluginId: String?) = statusProvider.disablePlugin(pluginId) override fun isPluginDisabled(pluginId: String?): Boolean = statusProvider.isPluginDisabled(pluginId) diff --git a/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/SpringExtensionFactory.kt b/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/SpringExtensionFactory.kt index a40702951..0b24688aa 100644 --- a/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/SpringExtensionFactory.kt +++ b/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/SpringExtensionFactory.kt @@ -102,6 +102,7 @@ class SpringExtensionFactory( /** * Load an extension config, provided a set of [ConfigCoordinates]. */ + @Suppress("ThrowsCount") private fun loadConfig(extension: Any, coordinates: ConfigCoordinates) { if (extension !is ConfigurableExtension<*>) { return @@ -127,15 +128,18 @@ class SpringExtensionFactory( } catch (se: SecurityException) { throw SystemException(se) } catch (iae: IllegalAccessException) { - throw SystemException("Could not access setConfiguration method on extension: ${extension.javaClass.name}", iae) + throw SystemException( + "Could not access setConfiguration method on extension: ${extension.javaClass.name}", iae) } catch (iae: IllegalArgumentException) { - throw SystemException("Configuration on extension appears to be invalid: ${extension.javaClass.name}", iae) + throw SystemException( + "Configuration on extension appears to be invalid: ${extension.javaClass.name}", iae) } catch (ite: InvocationTargetException) { - throw SystemException("Failed to invoke setConfiguration on extension: ${extension.javaClass.name}", ite) + throw SystemException( + "Failed to invoke setConfiguration on extension: ${extension.javaClass.name}", ite) } } - ?: throw SystemException( - "Could not find configuration class '${resolvedType.getGeneric(0)}' for extension: ${extension.javaClass.name}") + ?: throw SystemException("Could not find configuration class " + + "'${resolvedType.getGeneric(0)}' for extension: ${extension.javaClass.name}") } } @@ -147,7 +151,7 @@ class SpringExtensionFactory( private fun createWithoutSpring(extensionClass: Class): T? { try { return extensionClass.newInstance() - } catch (e: Exception) { + } catch (@Suppress("TooGenericExceptionCaught") e: Exception) { log.error(e.message, e) } return null diff --git a/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/config/SpringEnvironmentConfigResolver.kt b/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/config/SpringEnvironmentConfigResolver.kt index 7b8650f4d..334917a1a 100644 --- a/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/config/SpringEnvironmentConfigResolver.kt +++ b/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/config/SpringEnvironmentConfigResolver.kt @@ -32,6 +32,7 @@ import org.slf4j.LoggerFactory import org.springframework.core.env.ConfigurableEnvironment import org.springframework.core.env.EnumerablePropertySource import java.lang.reflect.ParameterizedType +import java.lang.RuntimeException import java.util.Properties /** @@ -99,7 +100,7 @@ class SpringEnvironmentConfigResolver( throw IntegrationException("Failed reading extension config: Input appears invalid", pe) } catch (me: JsonMappingException) { throw IntegrationException("Failed reading extension config: Could not map provided config to expected shape", me) - } catch (e: Exception) { + } catch (@Suppress("TooGenericExceptionCaught") e: RuntimeException) { throw SystemException("Failed resolving extension config for an unexpected reason", e) } } diff --git a/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/loaders/SpinnakerDefaultPluginLoader.kt b/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/loaders/SpinnakerDefaultPluginLoader.kt index 5ea4c63d4..9957063a6 100644 --- a/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/loaders/SpinnakerDefaultPluginLoader.kt +++ b/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/loaders/SpinnakerDefaultPluginLoader.kt @@ -34,7 +34,8 @@ class SpinnakerDefaultPluginLoader( override fun createPluginClassLoader(pluginPath: Path, pluginDescriptor: PluginDescriptor): PluginClassLoader { if (pluginDescriptor !is SpinnakerPluginDescriptor) { - log.debug("Descriptor for ${pluginDescriptor.pluginId} is not SpinnakerPluginDescriptor: Falling back to default behavior") + log.debug("Descriptor for ${pluginDescriptor.pluginId} is not SpinnakerPluginDescriptor: " + + "Falling back to default behavior") return super.createPluginClassLoader(pluginPath, pluginDescriptor) } diff --git a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/JooqToSpringExceptionTransformer.kt b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/JooqToSpringExceptionTransformer.kt index aa2341b20..0e9ecc1c9 100644 --- a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/JooqToSpringExceptionTransformer.kt +++ b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/JooqToSpringExceptionTransformer.kt @@ -25,10 +25,11 @@ class JooqToSpringExceptionTransformer : DefaultExecuteListener() { override fun exception(ctx: ExecuteContext) { if (ctx.sqlException() != null) { val dialect = ctx.configuration().dialect() - val translator = if (dialect != null) + val translator = if (dialect != null) { SQLErrorCodeSQLExceptionTranslator(dialect.name) - else + } else { SQLStateSQLExceptionTranslator() + } ctx.exception(translator.translate("jOOQ", ctx.sql(), ctx.sqlException())) } } diff --git a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/ConnectionPoolProperties.kt b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/ConnectionPoolProperties.kt index 245c4a33d..13e1861fb 100644 --- a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/ConnectionPoolProperties.kt +++ b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/ConnectionPoolProperties.kt @@ -18,6 +18,7 @@ package com.netflix.spinnaker.kork.sql.config import org.jooq.SQLDialect import java.util.concurrent.TimeUnit +@Suppress("MagicNumber") data class ConnectionPoolProperties( var dialect: SQLDialect = SQLDialect.MYSQL, var jdbcUrl: String? = null, diff --git a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/DefaultSqlConfiguration.kt b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/DefaultSqlConfiguration.kt index a928f5e3c..a1470b7f2 100644 --- a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/DefaultSqlConfiguration.kt +++ b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/DefaultSqlConfiguration.kt @@ -40,10 +40,10 @@ import org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfiguration import org.springframework.boot.autoconfigure.jdbc.metadata.DataSourcePoolMetadataProvidersConfiguration import org.springframework.boot.context.properties.EnableConfigurationProperties import org.springframework.context.annotation.Bean +import org.springframework.context.annotation.Conditional import org.springframework.context.annotation.Configuration import org.springframework.context.annotation.DependsOn import org.springframework.context.annotation.Import -import org.springframework.context.annotation.Conditional import org.springframework.jdbc.datasource.DataSourceTransactionManager import org.springframework.jdbc.datasource.TransactionAwareDataSourceProxy import sun.net.InetAddressCachePolicy @@ -78,6 +78,7 @@ class DefaultSqlConfiguration { fun secondaryLiquibase(properties: SqlProperties): SpringLiquibase = SpringLiquibaseProxy(properties.secondaryMigration) + @Suppress("ReturnCount", "ThrowsCount") @DependsOn("liquibase") @Bean fun dataSource(dataSourceFactory: DataSourceFactory, properties: SqlProperties): DataSource { @@ -174,8 +175,14 @@ class DefaultSqlConfiguration { @Bean(destroyMethod = "close") @Conditional(SecondaryPoolDialectCondition::class) - fun secondaryJooq(connectionProvider: DataSourceConnectionProvider, sqlProperties: SqlProperties): DSLContext { - val secondaryPool: ConnectionPoolProperties = sqlProperties.connectionPools.filter { !it.value.default }.values.first() + fun secondaryJooq( + connectionProvider: DataSourceConnectionProvider, + sqlProperties: SqlProperties + ): DSLContext { + val secondaryPool: ConnectionPoolProperties = sqlProperties.connectionPools + .filter { !it.value.default } + .values + .first() val secondaryJooqConfig: DefaultConfiguration = DefaultConfiguration().apply { set(*DefaultExecuteListenerProvider.providers( JooqToSpringExceptionTransformer(), @@ -224,6 +231,7 @@ private fun forceInetAddressCachePolicy() { class InetAddressOverrideFailure(cause: Throwable) : IllegalStateException(cause) +@Suppress("ThrowsCount") private fun validateDefaultTargetDataSources(targets: Collection) { if (targets.isEmpty()) { throw BeanCreationException("At least one connection pool must be configured") diff --git a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SecondaryPoolDialectCondition.kt b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SecondaryPoolDialectCondition.kt index eb96687c1..babe5932e 100644 --- a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SecondaryPoolDialectCondition.kt +++ b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SecondaryPoolDialectCondition.kt @@ -18,11 +18,11 @@ package com.netflix.spinnaker.kork.sql.config import org.springframework.boot.autoconfigure.condition.ConditionOutcome import org.springframework.boot.autoconfigure.condition.SpringBootCondition -import org.springframework.context.annotation.ConditionContext -import org.springframework.core.type.AnnotatedTypeMetadata -import org.springframework.boot.context.properties.source.ConfigurationPropertyName import org.springframework.boot.context.properties.bind.Bindable import org.springframework.boot.context.properties.bind.Binder +import org.springframework.boot.context.properties.source.ConfigurationPropertyName +import org.springframework.context.annotation.ConditionContext +import org.springframework.core.type.AnnotatedTypeMetadata class SecondaryPoolDialectCondition : SpringBootCondition() { @@ -39,9 +39,18 @@ class SecondaryPoolDialectCondition : SpringBootCondition() { return false } - val defaultPool: ConnectionPoolProperties = sqlProperties.connectionPools.filter { it.value.default }.values.first() - val secondaryPool: ConnectionPoolProperties = sqlProperties.connectionPools.filter { !it.value.default }.values.first() + val defaultPool: ConnectionPoolProperties = sqlProperties.connectionPools.first(default = true) + val secondaryPool: ConnectionPoolProperties = sqlProperties.connectionPools.first(default = false) return defaultPool.dialect != secondaryPool.dialect } + + private fun MutableMap.first(default: Boolean): ConnectionPoolProperties = + filter { + if (default) { + it.value.default + } else { + !it.value.default + } + }.values.first() } diff --git a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SqlProperties.kt b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SqlProperties.kt index 069146cd4..dc48687bc 100644 --- a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SqlProperties.kt +++ b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SqlProperties.kt @@ -35,10 +35,11 @@ data class SqlProperties( } return connectionPool!! } - if (connectionPools.size == 1) { - return connectionPools.values.first() + return if (connectionPools.size == 1) { + connectionPools.values.first() + } else { + connectionPools.values.first { it.default } } - return connectionPools.values.first { it.default } } } diff --git a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/health/SqlHealthProvider.kt b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/health/SqlHealthProvider.kt index e522a8bb1..92a90118f 100644 --- a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/health/SqlHealthProvider.kt +++ b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/health/SqlHealthProvider.kt @@ -34,6 +34,7 @@ class SqlHealthProvider( private val log = LoggerFactory.getLogger(javaClass) + @Suppress("VariableNaming") internal val _enabled = AtomicBoolean(false) private val _healthException: AtomicReference = AtomicReference() @@ -66,7 +67,7 @@ class SqlHealthProvider( } } unhealthyCounter.set(0) - } catch (e: Exception) { + } catch (@Suppress("TooGenericExceptionCaught") e: Exception) { _healthException.set(e) healthyCounter.set(0) unhealthyCounter.incrementAndGet().also { unhealthyCount ->