From d76e135d016a794d7c0c3b4667237329c16ed630 Mon Sep 17 00:00:00 2001 From: Anton Duyun Date: Tue, 18 Jul 2023 18:03:44 +0300 Subject: [PATCH] Nullable fields config value extraction fix (#202) --- .../processor/ConfigParserGenerator.java | 15 ++++---- .../processor/AnnotationConfigTest.java | 15 ++++---- .../kora/config/ksp/ConfigParserGenerator.kt | 23 ++++++------ .../symbol/processor/AnnotationConfigTest.kt | 19 ++++++---- .../common/annotation/KafkaProducer.java | 5 --- .../common/annotation/SomeKafkaProducer.java | 36 ------------------- 6 files changed, 40 insertions(+), 73 deletions(-) delete mode 100644 kafka/kafka/src/main/java/ru/tinkoff/kora/kafka/common/annotation/KafkaProducer.java delete mode 100644 kafka/kafka/src/main/java/ru/tinkoff/kora/kafka/common/annotation/SomeKafkaProducer.java diff --git a/config/config-annotation-processor/src/main/java/ru/tinkoff/kora/config/annotation/processor/ConfigParserGenerator.java b/config/config-annotation-processor/src/main/java/ru/tinkoff/kora/config/annotation/processor/ConfigParserGenerator.java index 71f669b92..b9555dbb4 100644 --- a/config/config-annotation-processor/src/main/java/ru/tinkoff/kora/config/annotation/processor/ConfigParserGenerator.java +++ b/config/config-annotation-processor/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/config/config-annotation-processor/src/test/java/ru/tinkoff/kora/config/annotation/processor/AnnotationConfigTest.java b/config/config-annotation-processor/src/test/java/ru/tinkoff/kora/config/annotation/processor/AnnotationConfigTest.java index bffbdd138..bd94c4bb1 100644 --- a/config/config-annotation-processor/src/test/java/ru/tinkoff/kora/config/annotation/processor/AnnotationConfigTest.java +++ b/config/config-annotation-processor/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/config/config-symbol-processor/src/main/kotlin/ru/tinkoff/kora/config/ksp/ConfigParserGenerator.kt b/config/config-symbol-processor/src/main/kotlin/ru/tinkoff/kora/config/ksp/ConfigParserGenerator.kt index 5ee69a66d..8cf5eb1a5 100644 --- a/config/config-symbol-processor/src/main/kotlin/ru/tinkoff/kora/config/ksp/ConfigParserGenerator.kt +++ b/config/config-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/config/config-symbol-processor/src/test/kotlin/ru/tinkoff/kora/config/symbol/processor/AnnotationConfigTest.kt b/config/config-symbol-processor/src/test/kotlin/ru/tinkoff/kora/config/symbol/processor/AnnotationConfigTest.kt index 330ae3f3e..95be03851 100644 --- a/config/config-symbol-processor/src/test/kotlin/ru/tinkoff/kora/config/symbol/processor/AnnotationConfigTest.kt +++ b/config/config-symbol-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 deleted file mode 100644 index 319f110e3..000000000 --- a/kafka/kafka/src/main/java/ru/tinkoff/kora/kafka/common/annotation/KafkaProducer.java +++ /dev/null @@ -1,5 +0,0 @@ -package ru.tinkoff.kora.kafka.common.annotation; - -public @interface KafkaProducer { - String value(); -} 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 deleted file mode 100644 index 8e5a7167f..000000000 --- a/kafka/kafka/src/main/java/ru/tinkoff/kora/kafka/common/annotation/SomeKafkaProducer.java +++ /dev/null @@ -1,36 +0,0 @@ -package ru.tinkoff.kora.kafka.common.annotation; - -import org.apache.kafka.clients.producer.Producer; -import org.apache.kafka.common.serialization.Deserializer; -import ru.tinkoff.kora.common.Module; -import ru.tinkoff.kora.common.Tag; - -//@KafkaProducer("some-config-path") -//public interface SomeKafkaProducer extends Producer { -// -// @Module -// interface SomewModule { -// default SomeKafkaProducer producerNoTag(Deserializer key, Deserializer value) { -// // generated class -// throw new RuntimeException(); -// } -// -// @Tag(SomeKafkaProducer.class) -// default Transactional producerNoTagTx(Deserializer key, Deserializer value) { -// // generated class -// throw new RuntimeException(); -// } -// -// @Tag(SomeKafkaProducer.class) -// default SomeKafkaProducer producerNoTag(SomeKafkaProducer noTag) { -// // generated class -// return noTag; -// } -// } -// -// interface Transactional { -// Transaction tx(); -// -// interface Transaction extends AutoCloseable, Producer {} -// } -//}