Skip to content

Commit

Permalink
modify path rule to have either http method or path mapping
Browse files Browse the repository at this point in the history
  • Loading branch information
@jrhee17
jrhee17 committed Oct 10, 2019
1 parent 51e2787 commit 042691a
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 92 deletions.
52 changes: 37 additions & 15 deletions core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -512,13 +513,46 @@ private static Set<MediaType> listToSet(List<MediaType> types, Class<?> annotati

/**
* Returns path patterns for each {@link HttpMethod}. The path pattern might be specified by
* {@link Path} or HTTP method annotations such as {@link Get} and {@link Post}.
* {@link Path} or HTTP method annotations such as {@link Get} and {@link Post}. Path patterns
* may be specified by either HTTP method annotations, or {@link Path} annotations but not both
* simultaneously.
*/
private static Map<HttpMethod, Set<String>> getHttpMethodPatternsMap(Method method,
Set<Annotation> methodAnnotations) {
final Map<HttpMethod, Set<String>> httpMethodPatternMap = new EnumMap<>(HttpMethod.class);
final Set<String> pathPatterns = findAll(method, Path.class).stream().map(Path::value)
.collect(toImmutableSet());
.collect(toImmutableSet());
final boolean usePathPatterns = !pathPatterns.isEmpty();

final Map<HttpMethod, Set<String>> httpMethodAnnotatedPatternMap =
getHttpMethodAnnotatedPatternMap(methodAnnotations);
if (httpMethodAnnotatedPatternMap.keySet().isEmpty()) {
throw new IllegalArgumentException(method.getDeclaringClass().getName() + '#' + method.getName() +
" must have an HTTP method annotation.");
}
return httpMethodAnnotatedPatternMap.entrySet().stream().collect(
ImmutableMap.toImmutableMap(
Entry::getKey,
entry -> {
final Set<String> httpMethodPaths = entry.getValue();
if (usePathPatterns && !httpMethodPaths.isEmpty()) {
throw new IllegalArgumentException(
method.getDeclaringClass().getName() + '#' + method.getName() +
" cannot specify both an HTTP mapping and a Path mapping.");
}
if (usePathPatterns) {
httpMethodPaths.addAll(pathPatterns);
}
if (httpMethodPaths.isEmpty()) {
throw new IllegalArgumentException("A path pattern should be specified by" +
" @Path or HTTP method annotations.");
}
return ImmutableSet.copyOf(httpMethodPaths);
}));
}

private static Map<HttpMethod, Set<String>> getHttpMethodAnnotatedPatternMap(
Set<Annotation> methodAnnotations) {
final Map<HttpMethod, Set<String>> httpMethodPatternMap = new EnumMap<>(HttpMethod.class);
methodAnnotations.stream()
.filter(annotation -> HTTP_METHOD_MAP.containsKey(annotation.annotationType()))
.forEach(annotation -> {
Expand All @@ -530,18 +564,6 @@ private static Map<HttpMethod, Set<String>> getHttpMethodPatternsMap(Method meth
patterns.add(value);
}
});
final Set<HttpMethod> methods = httpMethodPatternMap.keySet();
if (methods.isEmpty()) {
throw new IllegalArgumentException(method.getDeclaringClass().getName() + '#' + method.getName() +
" must have an HTTP method annotation.");
}
httpMethodPatternMap.forEach((k, v) -> v.addAll(pathPatterns));
httpMethodPatternMap.values().forEach(paths -> {
if (paths.isEmpty()) {
throw new IllegalArgumentException(
"A path pattern should be specified by @Path or HTTP method annotations.");
}
});
return httpMethodPatternMap;
}

Expand Down
4 changes: 2 additions & 2 deletions ...test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpDocServicePluginTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ public void testMultiPath() {
final Set<String> paths = methods.get("multiGet").endpoints()
.stream().map(EndpointInfo::pathMapping)
.collect(toImmutableSet());
assertThat(paths).isEqualTo(ImmutableSet.of("exact:/path1", "exact:/path2"));
assertThat(paths).containsOnly("exact:/path1", "exact:/path2");
}

private static void checkFooService(ServiceInfo fooServiceInfo) {
Expand Down Expand Up @@ -358,7 +358,7 @@ private static Map<String, ServiceInfo> services(DocServiceFilter include,
DocServiceFilter exclude,
Object... services) {
final ServerBuilder builder = Server.builder();
Arrays.stream(services).forEach(service -> builder.annotatedService(service));
Arrays.stream(services).forEach(builder::annotatedService);
final Server server = builder.build();
final ServiceSpecification specification =
plugin.generateSpecification(ImmutableSet.copyOf(server.serviceConfigs()),
Expand Down
26 changes: 17 additions & 9 deletions ...c/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import com.linecorp.armeria.server.annotation.Get;
import com.linecorp.armeria.server.annotation.Header;
import com.linecorp.armeria.server.annotation.JacksonRequestConverterFunction;
import com.linecorp.armeria.server.annotation.Options;
import com.linecorp.armeria.server.annotation.Param;
import com.linecorp.armeria.server.annotation.Path;
import com.linecorp.armeria.server.annotation.Post;
Expand Down Expand Up @@ -189,13 +188,6 @@ public void root(@Header("a") ArrayList<String> a,
@Get
@Post
@Path("/a")
@Path("/b")
public void root(BeanB b) {}
});

Server.builder().annotatedService(new Object() {
@Path("/")
@Get("/")
public void root() {}
});

Expand Down Expand Up @@ -342,11 +334,27 @@ public void root(BeanA a) {}
})).isInstanceOf(IllegalArgumentException.class);

assertThatThrownBy(() -> Server.builder().annotatedService(new Object() {
@Options
@Get
@Post("/")
public void root() {}
})).isInstanceOf(IllegalArgumentException.class);

assertThatThrownBy(() -> Server.builder().annotatedService(new Object() {
@Get("/")
@Path("/")
public void root() {}
})).isInstanceOf(IllegalArgumentException.class);

assertThatThrownBy(() -> Server.builder().annotatedService(new Object() {
@Path("")
public void root() {}
})).isInstanceOf(IllegalArgumentException.class);

// TODO: Check if we must support this
assertThatThrownBy(() -> Server.builder().annotatedService(new Object() {
@Path("/")
public void root() {}
})).isInstanceOf(IllegalArgumentException.class);
}

private static class NoDefaultConstructorList<E> extends ArrayList<E> {
Expand Down
50 changes: 25 additions & 25 deletions ...c/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,22 @@
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.assertj.core.util.Lists;
import org.junit.Test;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;

import com.linecorp.armeria.common.HttpMethod;
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.HttpStatus;
import com.linecorp.armeria.common.logging.LogLevel;
import com.linecorp.armeria.internal.annotation.AnnotatedHttpServiceFactory.DecoratorAndOrder;
import com.linecorp.armeria.server.DecoratingServiceFunction;
import com.linecorp.armeria.server.Route;
import com.linecorp.armeria.server.Service;
import com.linecorp.armeria.server.ServiceRequestContext;
import com.linecorp.armeria.server.SimpleDecoratingHttpService;
Expand Down Expand Up @@ -190,39 +189,40 @@ public void testCreateAnnotatedServiceElementWithoutExplicitPathOnMethod() {
public void testMultiPathSuccessGetMapping() {
final List<AnnotatedHttpServiceElement> getServiceElements =
getServiceElements(new MultiPathSuccessService(), "getMapping", HttpMethod.GET);
assertThat(getServiceElements).hasSize(1);
assertThat(getServiceElements.get(0).route().methods()).isEqualTo(ImmutableSet.of(HttpMethod.GET));
assertThat(getServiceElements.get(0).route().paths()).isEqualTo(ImmutableList.of("/get", "/get"));
final Set<Route> routes = getServiceElements.stream().map(AnnotatedHttpServiceElement::route).collect(
Collectors.toSet());
assertThat(routes).containsOnly(Route.builder().path("/getMapping").methods(HttpMethod.GET).build());
}

@Test
public void testMultiPathSuccessGetPostMapping() {
final List<AnnotatedHttpServiceElement> getServiceElements = getServiceElements(
new MultiPathSuccessService(), "getPostMapping", HttpMethod.GET);
assertThat(getServiceElements).hasSize(1);
assertThat(getServiceElements.get(0).route().methods()).isEqualTo(ImmutableSet.of(HttpMethod.GET));
assertThat(getServiceElements.get(0).route().paths()).isEqualTo(ImmutableList.of("/get", "/get"));
final Set<Route> getRoutes = getServiceElements.stream().map(AnnotatedHttpServiceElement::route)
.collect(Collectors.toSet());
assertThat(getRoutes).containsOnly(Route.builder().path("/getMapping").methods(HttpMethod.GET).build());

final List<AnnotatedHttpServiceElement> postServiceElements = getServiceElements(
new MultiPathSuccessService(), "getPostMapping", HttpMethod.POST);
assertThat(postServiceElements).hasSize(1);
assertThat(postServiceElements.get(0).route().methods()).isEqualTo(ImmutableSet.of(HttpMethod.POST));
assertThat(postServiceElements.get(0).route().paths()).isEqualTo(ImmutableList.of("/post", "/post"));
final Set<Route> postRoutes = postServiceElements.stream().map(AnnotatedHttpServiceElement::route)
.collect(Collectors.toSet());
assertThat(postRoutes).containsOnly(Route.builder().path("/postMapping").methods(HttpMethod.POST)
.build());
}

@Test
public void testMultiPathSuccessNoGetPostMappingPathMapping() {
public void testMultiPathSuccessGetPostMappingByPath() {
final List<AnnotatedHttpServiceElement> getServiceElements = getServiceElements(
new MultiPathSuccessService(), "noGetPostMappingPathMapping", HttpMethod.GET);
assertThat(getServiceElements).hasSize(1);
assertThat(getServiceElements.get(0).route().methods()).isEqualTo(ImmutableSet.of(HttpMethod.GET));
assertThat(getServiceElements.get(0).route().paths()).isEqualTo(ImmutableList.of("/path", "/path"));
new MultiPathSuccessService(), "getPostMappingByPath", HttpMethod.GET);
final Set<Route> getRoutes = getServiceElements.stream().map(AnnotatedHttpServiceElement::route)
.collect(Collectors.toSet());
assertThat(getRoutes).containsOnly(Route.builder().path("/path").methods(HttpMethod.GET).build());

final List<AnnotatedHttpServiceElement> postServiceElements = getServiceElements(
new MultiPathSuccessService(), "noGetPostMappingPathMapping", HttpMethod.POST);
assertThat(postServiceElements).hasSize(1);
assertThat(postServiceElements.get(0).route().methods()).isEqualTo(ImmutableSet.of(HttpMethod.POST));
assertThat(postServiceElements.get(0).route().paths()).isEqualTo(ImmutableList.of("/path", "/path"));
new MultiPathSuccessService(), "getPostMappingByPath", HttpMethod.POST);
final Set<Route> postRoutes = postServiceElements.stream().map(AnnotatedHttpServiceElement::route)
.collect(Collectors.toSet());
assertThat(postRoutes).containsOnly(Route.builder().path("/path").methods(HttpMethod.POST).build());
}

@Test
Expand Down Expand Up @@ -431,21 +431,21 @@ public HttpResponse trace() {

static class MultiPathSuccessService {

@Get("/get")
@Get("/getMapping")
public HttpResponse getMapping() {
return HttpResponse.of(HttpStatus.OK);
}

@Get("/get")
@Post("/post")
@Get("/getMapping")
@Post("/postMapping")
public HttpResponse getPostMapping() {
return HttpResponse.of(HttpStatus.OK);
}

@Get
@Post
@Path("/path")
public HttpResponse noGetPostMappingPathMapping() {
public HttpResponse getPostMappingByPath() {
return HttpResponse.of(HttpStatus.OK);
}
}
Expand Down
71 changes: 41 additions & 30 deletions core/src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -679,38 +679,44 @@ public String headerWithoutValue(RequestContext ctx,
public static class MyAnnotatedService12 {

@Get
@Path("/multiGet1")
@Path("/multiGet2")
public String multipleGet(RequestContext ctx) {
@Path("/pathMapping1")
@Path("/pathMapping2")
public String pathMapping(RequestContext ctx) {
return "multiGet";
}

@Get
@Path("/duplicateGet")
@Path("/duplicateGet")
public String duplicateGet(RequestContext ctx) {
return "duplicateGet";
@Path("/duplicatePath")
@Path("/duplicatePath")
public String duplicatePath(RequestContext ctx) {
return "duplicatePath";
}

@Get
@Path("/multiGetWithParam1/{param}")
@Path("/multiGetWithParam2/{param}")
public String multiGetWithParam(RequestContext ctx, @Param String param) {
@Path("/pathSameParam1/{param}")
@Path("/pathSameParam2/{param}")
public String pathSameParam(RequestContext ctx, @Param String param) {
return param;
}

@Get
@Path("/multiGetDiffParam1/{param1}")
@Path("/multiGetDiffParam2/{param2}")
public String multiGetDiffParam(RequestContext ctx, @Param String param1, @Param String param2) {
return param1 + "_" + param2;
@Path("/pathDiffParam1/{param1}")
@Path("/pathDiffParam2/{param2}")
public String pathDiffParam(RequestContext ctx, @Param String param1, @Param String param2) {
return param1 + '_' + param2;
}

@Get
@Post
@Path("/multiGetPost1")
@Path("/multiGetPost2")
public String multiGetPost(RequestContext ctx) {
@Path("/getPostWithPathMapping1")
@Path("/getPostWithPathMapping2")
public String getPostWithPathMapping(RequestContext ctx) {
return ctx.path();
}

@Get("/getMapping")
@Post("/postMapping")
public String getPostMapping(RequestContext ctx) {
return ctx.path();
}
}
Expand Down Expand Up @@ -1031,23 +1037,28 @@ public void testReturnVoid() throws Exception {
@Test
public void testMultiplePaths() throws Exception {
try (CloseableHttpClient hc = HttpClients.createMinimal()) {
testStatusCode(hc, get("/12/multiGet1"), 200);
testStatusCode(hc, get("/12/multiGet2"), 200);
testStatusCode(hc, get("/12/pathMapping1"), 200);
testStatusCode(hc, get("/12/pathMapping2"), 200);

testStatusCode(hc, get("/12/duplicatePath"), 200);

testStatusCode(hc, get("/12/duplicateGet"), 200);
testBody(hc, get("/12/pathSameParam1/param"), "param");
testBody(hc, get("/12/pathSameParam2/param"), "param");

testBody(hc, get("/12/multiGetWithParam1/param"), "param");
testBody(hc, get("/12/multiGetWithParam2/param"), "param");
testStatusCode(hc, get("/12/pathDiffParam1/param1"), 400);
testBody(hc, get("/12/pathDiffParam1/param1?param2=param2"), "param1_param2");
testStatusCode(hc, get("/12/pathDiffParam2/param2"), 400);
testBody(hc, get("/12/pathDiffParam2/param2?param1=param1"), "param1_param2");

testStatusCode(hc, get("/12/multiGetDiffParam1/param1"), 400);
testBody(hc, get("/12/multiGetDiffParam1/param1?param2=param2"), "param1_param2");
testStatusCode(hc, get("/12/multiGetDiffParam2/param2"), 400);
testBody(hc, get("/12/multiGetDiffParam2/param2?param1=param1"), "param1_param2");
testBody(hc, get("/12/getPostWithPathMapping1"), "/12/getPostWithPathMapping1");
testBody(hc, post("/12/getPostWithPathMapping1"), "/12/getPostWithPathMapping1");
testBody(hc, get("/12/getPostWithPathMapping2"), "/12/getPostWithPathMapping2");
testBody(hc, post("/12/getPostWithPathMapping2"), "/12/getPostWithPathMapping2");

testBody(hc, get("/12/multiGetPost1"), "/12/multiGetPost1");
testBody(hc, post("/12/multiGetPost1"), "/12/multiGetPost1");
testBody(hc, get("/12/multiGetPost2"), "/12/multiGetPost2");
testBody(hc, post("/12/multiGetPost2"), "/12/multiGetPost2");
testBody(hc, get("/12/getMapping"), "/12/getMapping");
testStatusCode(hc, post("/12/getMapping"), 405);
testStatusCode(hc, get("/12/postMapping"), 405);
testBody(hc, post("/12/postMapping"), "/12/postMapping");
}
}

Expand Down
Loading

0 comments on commit 042691a

Please sign in to comment.