Skip to content

Commit

Permalink
Fix that overloaded methods are not displayed in DocService (#4545)
Browse files Browse the repository at this point in the history 
Motivation:
Overloaded methods in annotated services are not displayed correctly in DocService.
Armeria DocService was built for Thrift services in which overloading methods are not possible, so we didn't consider overloaded methods at that time.
Also, it was for Thrift types, the `TypeSignature` wasn't structured well to represent other types used in `gRPC` and annotated services. 

Modifications:
- Add `id` to `MethodInfo` that uniquely identifies each method.
  - It is formed of `serviceName/methodName/httpMethod`
  - The `methodName` has increased `-n` at the end when the method is overloaded.
- Fix a bug where parsing the same named type recursively.
- (Breaking)
  - `NamedTypeInfo` is now `DescriptiveTypeInfo`.
    - `NamedTypeInfoProvider` is now `DescriptiveTypeInfoProvider`
  - Interface `TypeSigniture`.
    - Add subclasses with `TypeSignatureType` that represents each `TypeSignature`

Result:
- You can now send a debug request via `DocService` when the method is overloaded.
- Well-structured `TypeSignature`

To-do:
- Add `JsonSchemaType` to `TypeSignature` for creating the JSON schema easily.
  • Loading branch information
@minwoox
minwoox authored Dec 6, 2022
1 parent e76d242 commit ef64fde
Show file tree
Hide file tree
Showing 63 changed files with 1,159 additions and 886 deletions.
98 changes: 53 additions & 45 deletions .../main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServicePlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@
import com.linecorp.armeria.server.annotation.Param;
import com.linecorp.armeria.server.annotation.RequestObject;
import com.linecorp.armeria.server.docs.DescriptionInfo;
import com.linecorp.armeria.server.docs.DescriptiveTypeInfo;
import com.linecorp.armeria.server.docs.DescriptiveTypeInfoProvider;
import com.linecorp.armeria.server.docs.DescriptiveTypeSignature;
import com.linecorp.armeria.server.docs.DocServiceFilter;
import com.linecorp.armeria.server.docs.DocServicePlugin;
import com.linecorp.armeria.server.docs.EndpointInfo;
Expand All @@ -73,8 +76,6 @@
import com.linecorp.armeria.server.docs.FieldLocation;
import com.linecorp.armeria.server.docs.FieldRequirement;
import com.linecorp.armeria.server.docs.MethodInfo;
import com.linecorp.armeria.server.docs.NamedTypeInfo;
import com.linecorp.armeria.server.docs.NamedTypeInfoProvider;
import com.linecorp.armeria.server.docs.ServiceInfo;
import com.linecorp.armeria.server.docs.ServiceSpecification;
import com.linecorp.armeria.server.docs.StructInfo;
Expand Down Expand Up @@ -115,10 +116,10 @@ public final class AnnotatedDocServicePlugin implements DocServicePlugin {
private static final ObjectWriter objectWriter = JacksonUtil.newDefaultObjectMapper()
.writerWithDefaultPrettyPrinter();

private static final NamedTypeInfoProvider defaultRequestNamedTypeInfoProvider =
new DefaultNamedTypeInfoProvider(true);
private static final NamedTypeInfoProvider defaultResponseNamedTypeInfoProvider =
new DefaultNamedTypeInfoProvider(false);
private static final DescriptiveTypeInfoProvider DEFAULT_REQUEST_DESCRIPTIVE_TYPE_INFO_PROVIDER =
new DefaultDescriptiveTypeInfoProvider(true);
private static final DescriptiveTypeInfoProvider DEFAULT_RESPONSE_DESCRIPTIVE_TYPE_INFO_PROVIDER =
new DefaultDescriptiveTypeInfoProvider(false);

@Override
public String name() {
Expand All @@ -133,10 +134,10 @@ public String name() {
@Override
public ServiceSpecification generateSpecification(Set<ServiceConfig> serviceConfigs,
DocServiceFilter filter,
NamedTypeInfoProvider namedTypeInfoProvider) {
DescriptiveTypeInfoProvider descriptiveTypeInfoProvider) {
requireNonNull(serviceConfigs, "serviceConfigs");
requireNonNull(filter, "filter");
requireNonNull(namedTypeInfoProvider, "namedTypeInfoProvider");
requireNonNull(descriptiveTypeInfoProvider, "descriptiveTypeInfoProvider");

final Map<Class<?>, Set<MethodInfo>> methodInfos = new HashMap<>();
final Map<Class<?>, DescriptionInfo> serviceDescription = new HashMap<>();
Expand All @@ -150,12 +151,12 @@ public ServiceSpecification generateSpecification(Set<ServiceConfig> serviceConf
return;
}
addMethodInfo(methodInfos, sc.virtualHost().hostnamePattern(), service, serviceClass,
namedTypeInfoProvider);
descriptiveTypeInfoProvider);
addServiceDescription(serviceDescription, serviceClass);
}
});

return generate(serviceDescription, methodInfos, namedTypeInfoProvider);
return generate(serviceDescription, methodInfos, descriptiveTypeInfoProvider);
}

private static void addServiceDescription(Map<Class<?>, DescriptionInfo> serviceDescription,
Expand All @@ -165,17 +166,20 @@ private static void addServiceDescription(Map<Class<?>, DescriptionInfo> service

private static void addMethodInfo(Map<Class<?>, Set<MethodInfo>> methodInfos,
String hostnamePattern, AnnotatedService service,
Class<?> serviceClass, NamedTypeInfoProvider namedTypeInfoProvider) {
Class<?> serviceClass,
DescriptiveTypeInfoProvider descriptiveTypeInfoProvider) {
final Route route = service.route();
final EndpointInfo endpoint = endpointInfo(route, hostnamePattern);
final Method method = service.method();
final String name = method.getName();
final int overloadId = service.overloadId();
final TypeSignature returnTypeSignature = getReturnTypeSignature(method);
final List<FieldInfo> fieldInfos = fieldInfos(service.annotatedValueResolvers(), namedTypeInfoProvider);
final List<FieldInfo> fieldInfos = fieldInfos(service.annotatedValueResolvers(),
descriptiveTypeInfoProvider);
route.methods().forEach(
httpMethod -> {
final MethodInfo methodInfo = new MethodInfo(
name, returnTypeSignature, fieldInfos, ImmutableList.of(), // Ignore exceptions.
serviceClass.getName(), method.getName(), overloadId, returnTypeSignature,
fieldInfos, ImmutableList.of(),
ImmutableList.of(endpoint), httpMethod,
AnnotatedServiceFactory.findDescription(method));

Expand Down Expand Up @@ -239,10 +243,10 @@ private static Set<MediaType> availableMimeTypes(Route route) {
}

private static List<FieldInfo> fieldInfos(List<AnnotatedValueResolver> resolvers,
NamedTypeInfoProvider namedTypeInfoProvider) {
DescriptiveTypeInfoProvider descriptiveTypeInfoProvider) {
final ImmutableList.Builder<FieldInfo> fieldInfosBuilder = ImmutableList.builder();
for (AnnotatedValueResolver resolver : resolvers) {
final FieldInfo fieldInfo = fieldInfo(resolver, namedTypeInfoProvider);
final FieldInfo fieldInfo = fieldInfo(resolver, descriptiveTypeInfoProvider);
if (fieldInfo != null) {
fieldInfosBuilder.add(fieldInfo);
}
Expand All @@ -252,7 +256,7 @@ private static List<FieldInfo> fieldInfos(List<AnnotatedValueResolver> resolvers

@Nullable
private static FieldInfo fieldInfo(AnnotatedValueResolver resolver,
NamedTypeInfoProvider namedTypeInfoProvider) {
DescriptiveTypeInfoProvider descriptiveTypeInfoProvider) {
final Class<? extends Annotation> annotationType = resolver.annotationType();
if (annotationType == RequestObject.class) {
final BeanFactoryId beanFactoryId = resolver.beanFactoryId();
Expand All @@ -266,19 +270,22 @@ private static FieldInfo fieldInfo(AnnotatedValueResolver resolver,
if (!resolvers.isEmpty()) {
// Just use the simple name of the bean class as the field name.
return FieldInfo.builder(beanFactoryId.type().getSimpleName(), BEAN,
fieldInfos(resolvers, namedTypeInfoProvider))
fieldInfos(resolvers, descriptiveTypeInfoProvider))
.build();
}
} else {
// NamedTypeInfoProvider may provide NamedTypedInfo for the implicit request object.
// DescriptiveTypeInfoProvider may provide DescriptiveTypeInfo for the implicit request object.
final Class<?> elementType = resolver.elementType();
NamedTypeInfo namedTypeInfo = namedTypeInfoProvider.newNamedTypeInfo(elementType);
if (namedTypeInfo == null) {
namedTypeInfo = defaultRequestNamedTypeInfoProvider.newNamedTypeInfo(elementType);
DescriptiveTypeInfo descriptiveTypeInfo =
descriptiveTypeInfoProvider.newDescriptiveTypeInfo(elementType);
if (descriptiveTypeInfo == null) {
descriptiveTypeInfo =
DEFAULT_REQUEST_DESCRIPTIVE_TYPE_INFO_PROVIDER.newDescriptiveTypeInfo(elementType);
}
if (namedTypeInfo instanceof StructInfo && !((StructInfo) namedTypeInfo).fields().isEmpty()) {
return FieldInfo.builder(namedTypeInfo.name(), OBJECT,
((StructInfo) namedTypeInfo).fields())
if (descriptiveTypeInfo instanceof StructInfo &&
!((StructInfo) descriptiveTypeInfo).fields().isEmpty()) {
return FieldInfo.builder(descriptiveTypeInfo.name(), OBJECT,
((StructInfo) descriptiveTypeInfo).fields())
.requirement(resolver.shouldExist() ?
FieldRequirement.REQUIRED : FieldRequirement.OPTIONAL)
.build();
Expand Down Expand Up @@ -412,7 +419,7 @@ static TypeSignature toTypeSignature(Type type) {
return TypeSignature.ofList(toTypeSignature(clazz.getComponentType()));
}

return TypeSignature.ofNamed(clazz);
return TypeSignature.ofStruct(clazz);
}

static TypeSignature toTypeSignature(JavaType type) {
Expand Down Expand Up @@ -451,7 +458,7 @@ private static FieldLocation location(AnnotatedValueResolver resolver) {
@VisibleForTesting
static ServiceSpecification generate(Map<Class<?>, DescriptionInfo> serviceDescription,
Map<Class<?>, Set<MethodInfo>> methodInfos,
NamedTypeInfoProvider namedTypeInfoProvider) {
DescriptiveTypeInfoProvider descriptiveTypeInfoProvider) {
final Set<ServiceInfo> serviceInfos = methodInfos
.entrySet().stream()
.map(entry -> {
Expand All @@ -461,35 +468,36 @@ static ServiceSpecification generate(Map<Class<?>, DescriptionInfo> serviceDescr
})
.collect(toImmutableSet());

final Set<TypeSignature> requestNamedTypes =
final Set<DescriptiveTypeSignature> requestDescriptiveTypes =
serviceInfos.stream()
.flatMap(s -> s.findNamedTypes(true).stream())
.flatMap(s -> s.findDescriptiveTypes(true).stream())
.collect(toImmutableSet());

return ServiceSpecification.generate(
serviceInfos,
typeSignature -> newNamedTypeInfo(typeSignature, namedTypeInfoProvider, requestNamedTypes));
typeSignature -> newDescriptiveTypeInfo(typeSignature, descriptiveTypeInfoProvider,
requestDescriptiveTypes));
}

private static NamedTypeInfo newNamedTypeInfo(TypeSignature typeSignature, NamedTypeInfoProvider provider,
Set<TypeSignature> requestNamedTypes) {
final Object typeDescriptor = typeSignature.namedTypeDescriptor();
if (typeDescriptor == null) {
throw new IllegalArgumentException("cannot create a named type from: " + typeSignature);
private static DescriptiveTypeInfo newDescriptiveTypeInfo(
DescriptiveTypeSignature typeSignature,
DescriptiveTypeInfoProvider provider,
Set<DescriptiveTypeSignature> requestDescriptiveTypes) {
final Object typeDescriptor = typeSignature.descriptor();
DescriptiveTypeInfo descriptiveTypeInfo = provider.newDescriptiveTypeInfo(typeDescriptor);
if (descriptiveTypeInfo != null) {
return descriptiveTypeInfo;
}

NamedTypeInfo namedTypeInfo = provider.newNamedTypeInfo(typeDescriptor);
if (namedTypeInfo != null) {
return namedTypeInfo;
}

if (requestNamedTypes.contains(typeSignature)) {
namedTypeInfo = defaultRequestNamedTypeInfoProvider.newNamedTypeInfo(typeDescriptor);
if (requestDescriptiveTypes.contains(typeSignature)) {
descriptiveTypeInfo =
DEFAULT_REQUEST_DESCRIPTIVE_TYPE_INFO_PROVIDER.newDescriptiveTypeInfo(typeDescriptor);
} else {
namedTypeInfo = defaultResponseNamedTypeInfoProvider.newNamedTypeInfo(typeDescriptor);
descriptiveTypeInfo =
DEFAULT_RESPONSE_DESCRIPTIVE_TYPE_INFO_PROVIDER.newDescriptiveTypeInfo(typeDescriptor);
}
if (namedTypeInfo != null) {
return namedTypeInfo;
if (descriptiveTypeInfo != null) {
return descriptiveTypeInfo;
} else {
// An unresolved StructInfo.
return new StructInfo(typeSignature.name(), ImmutableList.of());
Expand Down
10 changes: 9 additions & 1 deletion core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public final class AnnotatedService implements HttpService {

private final Object object;
private final Method method;
private final int overloadId;
private final MethodHandle methodHandle;
@Nullable
private final MethodHandle callKotlinSuspendingMethod;
Expand All @@ -109,7 +110,7 @@ public final class AnnotatedService implements HttpService {
private final boolean serviceNameSetByAnnotation;

AnnotatedService(Object object, Method method,
List<AnnotatedValueResolver> resolvers,
int overloadId, List<AnnotatedValueResolver> resolvers,
List<ExceptionHandlerFunction> exceptionHandlers,
List<ResponseConverterFunction> responseConverters,
Route route,
Expand All @@ -119,6 +120,9 @@ public final class AnnotatedService implements HttpService {
boolean useBlockingTaskExecutor) {
this.object = requireNonNull(object, "object");
this.method = requireNonNull(method, "method");
checkArgument(overloadId >= 0, "overloadId: %s (expected: >= 0)", overloadId);
this.overloadId = overloadId;

checkArgument(!method.isVarArgs(), "%s#%s declared to take a variable number of arguments",
method.getDeclaringClass().getSimpleName(), method.getName());
isKotlinSuspendingMethod = KotlinUtil.isSuspendingFunction(method);
Expand Down Expand Up @@ -233,6 +237,10 @@ Method method() {
return method;
}

int overloadId() {
return overloadId;
}

List<AnnotatedValueResolver> annotatedValueResolvers() {
return resolvers;
}
Expand Down
30 changes: 21 additions & 9 deletions ...rc/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import java.util.ArrayList;
import java.util.Comparator;
import java.util.EnumMap;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
Expand Down Expand Up @@ -159,13 +160,24 @@ public static List<AnnotatedServiceElement> find(
List<ExceptionHandlerFunction> exceptionHandlerFunctions,
DependencyInjector dependencyInjector, @Nullable String queryDelimiter) {
final List<Method> methods = requestMappingMethods(object);
return methods.stream()
.flatMap((Method method) ->
create(pathPrefix, object, method, useBlockingTaskExecutor,
requestConverterFunctions, responseConverterFunctions,
exceptionHandlerFunctions, dependencyInjector, queryDelimiter
).stream())
.collect(toImmutableList());
final Builder<AnnotatedServiceElement> builder = ImmutableList.builder();

final Map<String, Integer> overloadIds = new HashMap<>();
// Can't sort methods to find the overloaded methods because methods are ordered using @Order.
for (Method method : methods) {
final String methodName = method.getName();
final int overloadId;
if (overloadIds.containsKey(methodName)) {
overloadId = overloadIds.get(methodName) + 1;
} else {
overloadId = 0;
}
overloadIds.put(methodName, overloadId);
builder.addAll(create(pathPrefix, object, method, overloadId, useBlockingTaskExecutor,
requestConverterFunctions, responseConverterFunctions,
exceptionHandlerFunctions, dependencyInjector, queryDelimiter));
}
return builder.build();
}

private static HttpStatus defaultResponseStatus(Method method, Class<?> clazz) {
Expand Down Expand Up @@ -219,7 +231,7 @@ private static <T extends Annotation> void setAdditionalHeader(HttpHeadersBuilde
*/
@VisibleForTesting
static List<AnnotatedServiceElement> create(String pathPrefix, Object object, Method method,
boolean useBlockingTaskExecutor,
int overloadId, boolean useBlockingTaskExecutor,
List<RequestConverterFunction> baseRequestConverters,
List<ResponseConverterFunction> baseResponseConverters,
List<ExceptionHandlerFunction> baseExceptionHandlers,
Expand Down Expand Up @@ -271,7 +283,7 @@ static List<AnnotatedServiceElement> create(String pathPrefix, Object object, Me
queryDelimiter);
return new AnnotatedServiceElement(
route,
new AnnotatedService(object, method, resolvers, eh, res, route, defaultStatus,
new AnnotatedService(object, method, overloadId, resolvers, eh, res, route, defaultStatus,
responseHeaders, responseTrailers, needToUseBlockingTaskExecutor),
decorator(method, clazz, dependencyInjector));
}).collect(toImmutableList());
Expand Down
Loading

0 comments on commit ef64fde

Please sign in to comment.