Skip to content

Commit

Permalink
Fix a bug where duplicate parameters are shown in DocService (#4645)
Browse files Browse the repository at this point in the history 
Motivation:

When an annotation is applied to Kotlin data class, Kotlin compiler constructor parameters with the annotation and adds the fields as well.
```kt
data class ExampleQueries(
    @param
    val application: String,
    @param
    val topic: String,
    @param
    val group: String,
)
```

Annotated services extract both the fields from the constructor and fields using reflection. The problem was reported by #3454 and fixed by #3461. However, there is a bug in detecting duplicates.
See for the detail. #3454 (comment)

Modifications:

- Check both `httpElementName` and `anntationType()` if they are not equal to provide a consistent comparison.

Result:

You no longer see duplicate parameters in `DocService` when parameter annotations for a request object are added to Kotlin data class.
  • Loading branch information
@ikhoon
ikhoon committed Feb 8, 2023
1 parent 3713d52 commit e971022
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 18 deletions.
43 changes: 34 additions & 9 deletions ...in/java/com/linecorp/armeria/internal/server/annotation/AnnotatedBeanFactoryRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@
import static org.reflections.ReflectionUtils.getAllMethods;
import static org.reflections.ReflectionUtils.getConstructors;

import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.AbstractMap.SimpleImmutableEntry;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
Expand All @@ -42,6 +44,7 @@
import com.google.common.collect.ImmutableMap.Builder;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.primitives.Ints;

import com.linecorp.armeria.common.DependencyInjector;
import com.linecorp.armeria.common.annotation.Nullable;
Expand Down Expand Up @@ -115,13 +118,32 @@ static AnnotatedBeanFactory<?> find(@Nullable BeanFactoryId beanFactoryId) {

static Set<AnnotatedValueResolver> uniqueResolverSet() {
return new TreeSet<>((o1, o2) -> {
final String o1Name = o1.httpElementName();
final String o2Name = o2.httpElementName();
if (o1Name.equals(o2Name) && o1.annotationType() == o2.annotationType()) {
String o1Name = o1.httpElementName();
String o2Name = o2.httpElementName();
final Class<? extends Annotation> o1AnnotationType = o1.annotationType();
final Class<? extends Annotation> o2AnnotationType = o2.annotationType();
if (o1AnnotationType == o2AnnotationType && o1Name.equals(o2Name)) {
return 0;
}
// We are not ordering, but just finding duplicate elements.
return -1;

// TreeSet internally creates a binary tree. A consistent comparison for the two inputs is
// necessary to traverse the binary tree correctly and check uniqueness.
// If compare(o1, o2) returns -1, compare(o2, o1) should return 1 or a positive number.
if (o1AnnotationType != null) {
o1Name += '/' + o1AnnotationType.getName();
}
if (o2AnnotationType != null) {
o2Name += '/' + o2AnnotationType.getName();
}
final int result = o1Name.compareTo(o2Name);
if (result != 0) {
return result;
}
// It is still not a good idea to return 0 since the equality for `httpElementName()` and
// `annotationType()` is broken. `o1AnnotationType` and `o2AnnotationType` may be the same
// class, but loaded by two class different loaders. The hash comparison is used to return a
// non-zero value for the different classes.
return Ints.compare(Objects.hashCode(o1AnnotationType), Objects.hashCode(o2AnnotationType));
});
}

Expand Down Expand Up @@ -250,18 +272,21 @@ private static Map<Method, List<AnnotatedValueResolver>> findMethods(
addToFirstIfExists(objectResolvers, converters, dependencyInjector),
dependencyInjector);
if (!resolvers.isEmpty()) {
int redundant = 0;
final List<AnnotatedValueResolver> duplicateResolvers = new ArrayList<>(resolvers.size());
for (AnnotatedValueResolver resolver : resolvers) {
if (!uniques.add(resolver)) {
redundant++;
warnDuplicateResolver(resolver, method.toGenericString());
duplicateResolvers.add(resolver);
}
}
if (redundant == resolvers.size()) {
if (duplicateResolvers.size() == resolvers.size()) {
// Prevent redundant injection only when all parameters are redundant.
// Otherwise, we'd better to inject the method rather than ignore it.
continue;
}

for (AnnotatedValueResolver duplicateResolver : duplicateResolvers) {
warnDuplicateResolver(duplicateResolver, method.toGenericString());
}
methodsBuilder.put(method, resolvers);
}
} catch (NoAnnotatedParameterException ignored) {
Expand Down
42 changes: 33 additions & 9 deletions kotlin/src/test/kotlin/com/linecorp/armeria/server/kotlin/DataClassDocServiceTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,23 @@ class DataClassDocServiceTest {
.content()

assertThat(jsonNode.get("services")[0]["methods"][0]["parameters"][0]["typeSignature"].asText())
.isEqualTo("com.linecorp.armeria.server.kotlin.ExampleQueries")
.isEqualTo("com.linecorp.armeria.server.kotlin.ExampleQueries1")
assertThat(jsonNode.get("structs")[0]["name"].asText())
.isEqualTo("com.linecorp.armeria.server.kotlin.ExampleQueries")
val fields = jsonNode.get("structs")[0]["fields"] as ArrayNode
assertThat(fields).hasSize(2)
assertThat(fields[0]["name"].asText()).isEqualTo("name")
assertThat(fields[1]["name"].asText()).isEqualTo("limit")
.isEqualTo("com.linecorp.armeria.server.kotlin.ExampleQueries1")
val fields1 = jsonNode.get("structs")[0]["fields"] as ArrayNode
assertThat(fields1).hasSize(2)
assertThat(fields1[0]["name"].asText()).isEqualTo("name")
assertThat(fields1[1]["name"].asText()).isEqualTo("limit")

assertThat(jsonNode.get("services")[0]["methods"][1]["parameters"][0]["typeSignature"].asText())
.isEqualTo("com.linecorp.armeria.server.kotlin.ExampleQueries2")
assertThat(jsonNode.get("structs")[1]["name"].asText())
.isEqualTo("com.linecorp.armeria.server.kotlin.ExampleQueries2")
val fields2 = jsonNode.get("structs")[1]["fields"] as ArrayNode
assertThat(fields2).hasSize(3)
assertThat(fields2[0]["name"].asText()).isEqualTo("application")
assertThat(fields2[1]["name"].asText()).isEqualTo("topic")
assertThat(fields2[2]["name"].asText()).isEqualTo("group")
}

companion object {
Expand All @@ -65,18 +75,32 @@ class DataClassDocServiceTest {
}

class MyKotlinService {
@Get("/example")
fun getId(@Suppress("UNUSED_PARAMETER") queries: ExampleQueries): String {
@Get("/example1")
fun getIdV1(@Suppress("UNUSED_PARAMETER") queries: ExampleQueries1): String {
return "example"
}

@Get("/example2")
fun getIdV2(@Suppress("UNUSED_PARAMETER") queries: ExampleQueries2): String {
return "example"
}
}

data class ExampleQueries(
data class ExampleQueries1(
@Param
val name: String,

@Param @Default
val limit: Int?
)

data class ExampleQueries2(
@Param
val application: String,
@Param
val topic: String,
@Param
val group: String,
)

data class ExampleBody(val name: String, val limit: Int?)

0 comments on commit e971022

Please sign in to comment.