Nullable fields config value extraction fix (#202)

Tinkoff · Jul 18, 2023 · d76e135 · d76e135
1 parent c7e5efa
commit d76e135
Show file tree

Hide file tree

Showing 6 changed files with 40 additions and 73 deletions.
diff --git a/...ssor/src/main/java/ru/tinkoff/kora/config/annotation/processor/ConfigParserGenerator.java b/...ssor/src/main/java/ru/tinkoff/kora/config/annotation/processor/ConfigParserGenerator.java
@@ -319,17 +319,20 @@ private MethodSpec buildParseField(TypeElement element, ConfigUtils.ConfigField
                     parse.endControlFlow();
                 }
                 parse.endControlFlow();
+            } else if (field.isNullable()) {
+                parse.beginControlFlow("if (value instanceof $T.NullValue nullValue)", ConfigClassNames.configValue);
+                parse.addStatement("return null");
+                parse.endControlFlow();
             }
-            parse.addStatement("var parsed = $L_parser.extract(value)", field.name());
-            parse.beginControlFlow("if (parsed == null)");
             if (field.isNullable()) {
-                parse.addStatement("return null");
+                parse.addStatement("return $L_parser.extract(value)", field.name());
             } else {
+                parse.addStatement("var parsed = $L_parser.extract(value)", field.name());
+                parse.beginControlFlow("if (parsed == null)");
                 parse.addStatement("throw $T.missingValueAfterParse(value)", ConfigClassNames.configValueExtractionException);
+                parse.endControlFlow();
+                parse.addStatement("return parsed");
             }
-            parse.nextControlFlow("else");
-            parse.addStatement("return parsed");
-            parse.endControlFlow();
         }
         return parse.build();
     }

diff --git a/...essor/src/test/java/ru/tinkoff/kora/config/annotation/processor/AnnotationConfigTest.java b/...essor/src/test/java/ru/tinkoff/kora/config/annotation/processor/AnnotationConfigTest.java
@@ -14,7 +14,8 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 public class AnnotationConfigTest extends AbstractConfigTest {
     @Test
@@ -99,10 +100,12 @@ public interface TestConfig {
     @Test
     public void testInterfaceWithUnknownType() {
         var mapper = Mockito.mock(ConfigValueExtractor.class);
-        when(mapper.extract(any())).thenAnswer(invocation -> invocation.getArguments()[0] instanceof ConfigValue.NullValue
-            ? null :
-            Duration.ofDays(3000)
-        );
+        when(mapper.extract(any())).thenAnswer(invocation -> {
+            if (invocation.getArguments()[0] instanceof ConfigValue.NullValue) {
+                throw new IllegalArgumentException();
+            }
+            return Duration.ofDays(3000);
+        });
 
         var extractor = this.compileConfig(List.of(mapper), """
             @ru.tinkoff.kora.config.common.annotation.ConfigValueExtractor
@@ -117,7 +120,7 @@ public interface TestConfig {
         assertThat(extractor.extract(MapConfigFactory.fromMap(Map.of("value", "test")).root()))
             .isEqualTo(newObject("$TestConfig_ConfigValueExtractor$TestConfig_Impl", Duration.ofDays(3000), null));
 
-        verify(mapper, times(2)).extract(any());
+        verify(mapper).extract(any());
     }
 
     @Test

diff --git a/...nfig-symbol-processor/src/main/kotlin/ru/tinkoff/kora/config/ksp/ConfigParserGenerator.kt b/...nfig-symbol-processor/src/main/kotlin/ru/tinkoff/kora/config/ksp/ConfigParserGenerator.kt
@@ -341,23 +341,20 @@ class ConfigParserGenerator(private val resolver: Resolver) {
         parse.addParameter("config", ConfigClassNames.objectValue)
         parse.addStatement("var value = config.get(%N)", "_${field.name}_path")
         val isSupportedType = field.mapping == null && supportedTypes.containsKey(field.typeName)
-        if (field.hasDefault || isSupportedType) {
-            parse.controlFlow("if (value is %T.NullValue)", ConfigClassNames.configValue) {
-                if (field.hasDefault) {
-                    if (typeDecl.classKind == ClassKind.INTERFACE) {
-                        parse.addStatement("return DEFAULTS.%N()", field.name)
-                    } else {
-                        parse.addStatement("return DEFAULTS.%N", field.name)
-                    }
+        parse.controlFlow("if (value is %T.NullValue)", ConfigClassNames.configValue) {
+            if (field.isNullable && !field.hasDefault) {
+                addStatement("return null")
+            } else if (field.hasDefault) {
+                if (typeDecl.classKind == ClassKind.INTERFACE) {
+                    addStatement("return DEFAULTS.%N()", field.name)
                 } else {
-                    if (field.isNullable) {
-                        parse.addStatement("return null")
-                    } else {
-                        parse.addStatement("throw %T.missingValue(value)", ConfigClassNames.configValueExtractionException)
-                    }
+                    addStatement("return DEFAULTS.%N", field.name)
                 }
+            } else if (isSupportedType) {
+                addStatement("throw %T.missingValue(value)", ConfigClassNames.configValueExtractionException)
             }
         }
+
         if (isSupportedType) {
             parse.addStatement("return %L", this.parseSupportedType(field.typeName))
         } else if (field.isNullable) {

diff --git a/...processor/src/test/kotlin/ru/tinkoff/kora/config/symbol/processor/AnnotationConfigTest.kt b/...processor/src/test/kotlin/ru/tinkoff/kora/config/symbol/processor/AnnotationConfigTest.kt
@@ -8,6 +8,8 @@ import org.mockito.Mockito
 import org.mockito.Mockito.verify
 import org.mockito.invocation.InvocationOnMock
 import org.mockito.kotlin.whenever
+import ru.tinkoff.kora.config.common.ConfigValue.NullValue
+import ru.tinkoff.kora.config.common.ConfigValue.StringValue
 import ru.tinkoff.kora.config.common.extractor.ConfigValueExtractionException
 import ru.tinkoff.kora.config.common.extractor.ConfigValueExtractor
 import ru.tinkoff.kora.config.common.factory.MapConfigFactory
@@ -109,20 +111,22 @@ class AnnotationConfigTest : AbstractConfigTest() {
     @Test
     fun testInterfaceWithUnknownType() {
         val mapper = Mockito.mock(ConfigValueExtractor::class.java)
-        whenever(mapper.extract(ArgumentMatchers.any())).thenReturn(Duration.ofDays(3000))
+        whenever(mapper.extract(ArgumentMatchers.isA(StringValue::class.java))).thenReturn(Duration.ofDays(3000))
+        whenever(mapper.extract(ArgumentMatchers.isA(NullValue::class.java))).thenThrow(IllegalArgumentException())
 
         val extractor = compileConfig(
             listOf(mapper), """
             @ConfigValueExtractor
             interface TestConfig {
-              fun value(): java.time.Duration
+              fun value1(): java.time.Duration
+              fun value2(): java.time.Duration?
             }
             
             """.trimIndent()
         )
 
         assertThat(extractor.extract(MapConfigFactory.fromMap(mapOf("value1" to "test")).root()))
-            .isEqualTo(new("\$TestConfig_ConfigValueExtractor\$TestConfig_Impl", Duration.ofDays(3000)))
+            .isEqualTo(new("\$TestConfig_ConfigValueExtractor\$TestConfig_Impl", Duration.ofDays(3000), null))
         verify(mapper).extract(ArgumentMatchers.any())
     }
 
@@ -199,17 +203,18 @@ class AnnotationConfigTest : AbstractConfigTest() {
     @Test
     fun testDataClassWithUnknownType() {
         val mapper = Mockito.mock(ConfigValueExtractor::class.java)
-        whenever(mapper.extract(ArgumentMatchers.any())).thenReturn(Duration.ofDays(3000))
+        whenever(mapper.extract(ArgumentMatchers.isA(StringValue::class.java))).thenReturn(Duration.ofDays(3000))
+        whenever(mapper.extract(ArgumentMatchers.isA(NullValue::class.java))).thenThrow(IllegalArgumentException())
 
         val extractor = compileConfig(
             listOf(mapper), """
             @ConfigValueExtractor
-            data class TestConfig(val value: java.time.Duration)
+            data class TestConfig(val value1: java.time.Duration, val value2: java.time.Duration?)
             """.trimIndent()
         )
 
-        assertThat(extractor.extract(MapConfigFactory.fromMap(mapOf("value" to "test")).root()))
-            .isEqualTo(new("TestConfig", Duration.ofDays(3000)))
+        assertThat(extractor.extract(MapConfigFactory.fromMap(mapOf("value1" to "test")).root()))
+            .isEqualTo(new("TestConfig", Duration.ofDays(3000), null))
         verify(mapper).extract(ArgumentMatchers.any())
     }
 

diff --git a/kafka/kafka/src/main/java/ru/tinkoff/kora/kafka/common/annotation/KafkaProducer.java b/kafka/kafka/src/main/java/ru/tinkoff/kora/kafka/common/annotation/KafkaProducer.java
diff --git a/kafka/kafka/src/main/java/ru/tinkoff/kora/kafka/common/annotation/SomeKafkaProducer.java b/kafka/kafka/src/main/java/ru/tinkoff/kora/kafka/common/annotation/SomeKafkaProducer.java