Addressing points raised in detektJvmMain task (#203)

* Removed "exception as control flow" in KLoggerNameResolver.unwrapCompanionClass(); * Consolidated MDC cleanup code in KotlinLoggingMDC
oshai · Dec 11, 2021 · 981daea · 981daea
1 parent 2cce41a
commit 981daea
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 44 deletions.
diff --git a/src/jvmMain/kotlin/mu/KLogger.kt b/src/jvmMain/kotlin/mu/KLogger.kt
@@ -9,6 +9,7 @@ import org.slf4j.Logger
  * logger.info{"this is $lazy evaluated string"}
  *```
  */
+@Suppress("TooManyFunctions")
 public actual interface KLogger : Logger {
 
     /**

diff --git a/src/jvmMain/kotlin/mu/KotlinLoggingMDC.kt b/src/jvmMain/kotlin/mu/KotlinLoggingMDC.kt
@@ -47,29 +47,20 @@ public inline fun <T> withLoggingContext(
     body: () -> T
 ): T {
     val pairForMDC = pair.filter { it.second != null }
-    val previousMDC = if (restorePrevious) {
-        pairForMDC.map { (k, _) ->
-            k to MDC.get(k)
+    val cleanupCallbacks = pairForMDC.map { (mdcKey, _) ->
+        val mdcValue = MDC.get(mdcKey)
+        if (restorePrevious && mdcValue != null) {
+            { MDC.put(mdcKey, mdcValue) }
+        } else {
+            { MDC.remove(mdcKey) }
         }
-    } else {
-        null
     }
 
     try {
         pairForMDC.forEach { MDC.put(it.first, it.second) }
         return body()
     } finally {
-        if (restorePrevious) {
-            previousMDC?.forEach {
-                if (it.second != null) {
-                    MDC.put(it.first, it.second)
-                } else {
-                    MDC.remove(it.first)
-                }
-            }
-        } else {
-            pairForMDC.forEach { MDC.remove(it.first) }
-        }
+        cleanupCallbacks.forEach { it.invoke() }
     }
 }
 
@@ -91,7 +82,14 @@ public inline fun <T> withLoggingContext(
     restorePrevious: Boolean = true,
     body: () -> T
 ): T {
-    val previousMDC = map.mapValues { MDC.get(it.key) }
+    val cleanupCallbacks = map.map {
+        val mdcValue = MDC.get(it.key)
+        if (restorePrevious && mdcValue != null) {
+            { MDC.put(it.key, mdcValue) }
+        } else {
+            { MDC.remove(it.key) }
+        }
+    }
 
     try {
         map.forEach {
@@ -101,16 +99,6 @@ public inline fun <T> withLoggingContext(
         }
         return body()
     } finally {
-        if (restorePrevious) {
-            previousMDC.forEach {
-                if (it.value != null) {
-                    MDC.put(it.key, it.value)
-                } else {
-                    MDC.remove(it.key)
-                }
-            }
-        } else {
-            previousMDC.forEach { MDC.remove(it.key) }
-        }
+        cleanupCallbacks.forEach { it.invoke() }
     }
 }
diff --git a/src/jvmMain/kotlin/mu/internal/KLoggerNameResolver.kt b/src/jvmMain/kotlin/mu/internal/KLoggerNameResolver.kt
@@ -33,18 +33,18 @@ internal object KLoggerNameResolver {
      * unwrap companion class to enclosing class given a Java Class
      */
     private inline fun <T : Any> unwrapCompanionClass(clazz: Class<T>): Class<*> {
-        if (clazz.enclosingClass != null) {
+        return clazz.enclosingClass?.let { enclosingClass ->
             try {
-                val field = clazz.enclosingClass.getDeclaredField(clazz.simpleName)
-                if (Modifier.isStatic(field.modifiers) && field.type == clazz) {
-                    // && field.get(null) === obj
-                    // the above might be safer but problematic with initialization order
-                    return clazz.enclosingClass
-                }
-            } catch (e: Exception) {
-                //ok, it is not a companion object
+                enclosingClass.declaredFields.find { field ->
+                    field.name == clazz.simpleName &&
+                        Modifier.isStatic(field.modifiers) &&
+                        field.type == clazz
+                }?.run { enclosingClass }
+            } catch (se: SecurityException) {
+                // The security manager isn't properly set up, so it won't be possible
+                // to search for the target declared field.
+                null
             }
-        }
-        return clazz
+        } ?: clazz
     }
 }
diff --git a/src/jvmMain/kotlin/mu/internal/LocationAwareKLogger.kt b/src/jvmMain/kotlin/mu/internal/LocationAwareKLogger.kt
@@ -11,6 +11,7 @@ import org.slf4j.spi.LocationAwareLogger
  * A class wrapping a [LocationAwareLogger] instance preserving
  * location information with the correct fully qualified class name.
  */
+@Suppress("VariableNaming", "TooManyFunctions")
 internal class LocationAwareKLogger(override val underlyingLogger: LocationAwareLogger) : KLogger,
     Logger by underlyingLogger {
 
@@ -558,7 +559,7 @@ internal class LocationAwareKLogger(override val underlyingLogger: LocationAware
     override fun entry(vararg argArray: Any?) {
         if (underlyingLogger.isTraceEnabled(ENTRY)) {
             val tp = MessageFormatter.arrayFormat(buildMessagePattern(argArray.size), argArray)
-            underlyingLogger.log(ENTRY, fqcn, LocationAwareLogger.TRACE_INT, tp.message, null, null);
+            underlyingLogger.log(ENTRY, fqcn, LocationAwareLogger.TRACE_INT, tp.message, null, null)
         }
     }
 

diff --git a/src/jvmMain/kotlin/mu/internal/LocationIgnorantKLogger.kt b/src/jvmMain/kotlin/mu/internal/LocationIgnorantKLogger.kt
@@ -12,6 +12,7 @@ import org.slf4j.Marker
  * the rest of the methods are delegated to [Logger]
  * Hence no implemented methods
  */
+@Suppress("TooManyFunctions")
 internal class LocationIgnorantKLogger(override val underlyingLogger: Logger) : KLogger, Logger by underlyingLogger {
 
     /**

diff --git a/src/jvmTest/kotlin/mu/KotlinLoggingMDCTest.kt b/src/jvmTest/kotlin/mu/KotlinLoggingMDCTest.kt
@@ -6,9 +6,13 @@ import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Assertions.assertNull
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.TestInstance
 import org.junit.jupiter.api.assertAll
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.ValueSource
 import org.slf4j.MDC
 
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
 class KotlinLoggingMDCTest {
     init {
         Configurator.setRootLevel(Level.TRACE)
@@ -160,8 +164,9 @@ class KotlinLoggingMDCTest {
         assertEquals("f", MDC.get("e"))
     }
 
-    @Test
-    fun `map withLoggingContext`() {
+    @ParameterizedTest
+    @ValueSource(booleans = [true, false])
+    fun `map withLoggingContext`(restorePrevious: Boolean) {
         assertAll(
             { assertNull(MDC.get("a")) },
             { assertNull(MDC.get("c")) },
@@ -173,7 +178,7 @@ class KotlinLoggingMDCTest {
         MDC.put("e", "g")
         MDC.put("k", "l")
 
-        withLoggingContext(mapOf("a" to "b", "c" to "d", "e" to null, "f" to "h")) {
+        withLoggingContext(mapOf("a" to "b", "c" to "d", "e" to null, "f" to "h"), restorePrevious) {
             assertAll(
                 { assertEquals("b", MDC.get("a")) },
                 { assertEquals("d", MDC.get("c")) },
@@ -204,7 +209,13 @@ class KotlinLoggingMDCTest {
         assertAll(
             { assertNull(MDC.get("a")) },
             { assertNull(MDC.get("c")) },
-            { assertEquals("g", MDC.get("e")) },
+            {
+                if (restorePrevious) {
+                    assertEquals("g", MDC.get("e"))
+                } else {
+                    assertNull(MDC.get("e"))
+                }
+            },
             { assertNull(MDC.get("f")) },
             { assertEquals("l", MDC.get("k")) },
         )