Skip to content

Commit

Permalink
Address comments by @trustin
Browse files Browse the repository at this point in the history
  • Loading branch information
ikhoon committed Mar 9, 2020
1 parent a159727 commit 773418f
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 68 deletions.
Expand Up @@ -31,7 +31,6 @@

import com.linecorp.armeria.common.HttpHeaders;
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.internal.common.PathAndQuery;
import com.linecorp.armeria.server.ServiceRequestContext;

/**
Expand Down Expand Up @@ -189,44 +188,40 @@ private DocServiceBuilder exampleHttpHeaders0(String serviceName, String methodN
/**
* Adds the specified example paths for the method with the specified service and method name.
*/
public DocServiceBuilder examplePaths(Class<?> serviceType, String methodName, String... examplePaths) {
public DocServiceBuilder examplePaths(Class<?> serviceType, String methodName, String... paths) {
requireNonNull(serviceType, "serviceType");
return examplePaths(serviceType.getName(), methodName, examplePaths);
return examplePaths(serviceType.getName(), methodName, paths);
}

/**
* Adds the specified example paths for the method with the specified service and method name.
*/
public DocServiceBuilder examplePaths(Class<?> serviceType, String methodName,
Iterable<String> examplePaths) {
public DocServiceBuilder examplePaths(Class<?> serviceType, String methodName, Iterable<String> paths) {
requireNonNull(serviceType, "serviceType");
return examplePaths(serviceType.getName(), methodName, examplePaths);
return examplePaths(serviceType.getName(), methodName, paths);
}

/**
* Adds the specified example paths for the method with the specified service and method name.
*/
public DocServiceBuilder examplePaths(String serviceName, String methodName, String... examplePaths) {
requireNonNull(examplePaths, "examplePaths");
return examplePaths(serviceName, methodName, ImmutableList.copyOf(examplePaths));
public DocServiceBuilder examplePaths(String serviceName, String methodName, String... paths) {
requireNonNull(paths, "paths");
return examplePaths(serviceName, methodName, ImmutableList.copyOf(paths));
}

/**
* Adds the specified example paths for the method with the specified service and method name.
*/
public DocServiceBuilder examplePaths(String serviceName, String methodName,
Iterable<String> examplePaths) {
public DocServiceBuilder examplePaths(String serviceName, String methodName, Iterable<String> paths) {
requireNonNull(serviceName, "serviceName");
checkArgument(!serviceName.isEmpty(), "serviceName is empty.");
requireNonNull(methodName, "methodName");
checkArgument(!methodName.isEmpty(), "methodName is empty.");
requireNonNull(examplePaths, "examplePaths");
for (String examplePath : examplePaths) {
requireNonNull(examplePath, "examplePaths contains null");
examplePaths.forEach(path -> checkArgument(PathAndQuery.parse(path) != null,
"examplePaths contain an invalid path: {}", path));
this.examplePaths.computeIfAbsent(serviceName, unused -> ArrayListMultimap.create())
.put(methodName, examplePath);
requireNonNull(paths, "paths");
for (String examplePath : paths) {
requireNonNull(examplePath, "paths contains null");
examplePaths.computeIfAbsent(serviceName, unused -> ArrayListMultimap.create())
.put(methodName, examplePath);
}
return this;
}
Expand All @@ -242,9 +237,10 @@ public DocServiceBuilder exampleQueries(Class<?> serviceType, String methodName,
/**
* Adds the specified example query strings for the method with the specified service and method name.
*/
public DocServiceBuilder exampleQueries(Class<?> serviceType, String methodName, Iterable<String> queries) {
public DocServiceBuilder exampleQueries(Class<?> serviceType, String methodName,
Iterable<String> queryStrings) {
requireNonNull(serviceType, "serviceType");
return exampleQueries(serviceType.getName(), methodName, queries);
return exampleQueries(serviceType.getName(), methodName, queryStrings);
}

/**
Expand All @@ -258,17 +254,15 @@ public DocServiceBuilder exampleQueries(String serviceName, String methodName, S
/**
* Adds the specified example query strings for the method with the specified service and method name.
*/
public DocServiceBuilder exampleQueries(String serviceName, String methodName, Iterable<String> queries) {
public DocServiceBuilder exampleQueries(String serviceName, String methodName,
Iterable<String> queryStrings) {
requireNonNull(serviceName, "serviceName");
checkArgument(!serviceName.isEmpty(), "serviceName is empty.");
requireNonNull(methodName, "methodName");
checkArgument(!methodName.isEmpty(), "methodName is empty.");
requireNonNull(queries, "queries");
for (String query : queries) {
requireNonNull(query, "queries contains null");
final PathAndQuery pathAndQuery = PathAndQuery.parse(query.startsWith("?") ? query : '?' + query);
checkArgument(pathAndQuery != null,
"exampleQueries contain an invalid query string: {}", query);
requireNonNull(queryStrings, "queryStrings");
for (String query : queryStrings) {
requireNonNull(query, "queryStrings contains null");
exampleQueries.computeIfAbsent(serviceName, unused -> ArrayListMultimap.create())
.put(methodName, query);
}
Expand Down
Expand Up @@ -16,6 +16,7 @@

package com.linecorp.armeria.server.docs;

import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;

Expand All @@ -36,6 +37,7 @@
import com.linecorp.armeria.common.HttpHeaders;
import com.linecorp.armeria.common.HttpMethod;
import com.linecorp.armeria.common.util.UnstableApi;
import com.linecorp.armeria.internal.common.PathAndQuery;
import com.linecorp.armeria.server.Service;

/**
Expand Down Expand Up @@ -114,8 +116,18 @@ public MethodInfo(String name,
this.exampleHttpHeaders = ImmutableList.copyOf(requireNonNull(exampleHttpHeaders,
"exampleHttpHeaders"));
this.exampleRequests = ImmutableList.copyOf(requireNonNull(exampleRequests, "exampleRequests"));

this.examplePaths = ImmutableList.copyOf(requireNonNull(examplePaths, "examplePaths"));
this.examplePaths.forEach(path -> checkArgument(PathAndQuery.parse(path) != null,
"examplePaths contains an invalid path: {}", path));

this.exampleQueries = ImmutableList.copyOf(requireNonNull(exampleQueries, "exampleQueries"));
this.exampleQueries.forEach(query -> {
final PathAndQuery pathAndQuery = PathAndQuery.parse(query.startsWith("?") ? query : '?' + query);

This comment has been minimized.

Copy link
@trustin

trustin Mar 9, 2020

Member
  • PathAndQuery can validate a path and a query at once.
  • How about using the normalized path and query? i.e. Use PathAndQuery.path() and query() after parsing.

This comment has been minimized.

Copy link
@trustin

trustin Mar 9, 2020

Member

Probably better calling PathAndQuery twice as you did in this commit. But I think normalized form should still be used.

This comment has been minimized.

Copy link
@ikhoon

ikhoon Mar 9, 2020

Author Contributor

ow about using the normalized path and query?

Oh, that is a nice idea. 👍

checkArgument(pathAndQuery != null,
"exampleQueries contains an invalid query string: {}", query);
});

this.httpMethod = requireNonNull(httpMethod, "httpMethod");
this.docString = Strings.emptyToNull(docString);
}
Expand Down

This file was deleted.

@@ -0,0 +1,55 @@
/*
* Copyright 2020 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.server.docs;

import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.util.List;

import org.junit.jupiter.api.Test;

import com.google.common.collect.ImmutableList;

import com.linecorp.armeria.common.HttpMethod;

class MethodInfoTest {

private static MethodInfo newMethodInfo(List<String> examplePaths, List<String> exampleQueries) {
return new MethodInfo("foo", TypeSignature.ofBase("T"),
/* parameters */ ImmutableList.of(), /* exceptionSignatures */ ImmutableList.of(),
/* endpoints */ ImmutableList.of(), /* exampleHeaders */ ImmutableList.of(),
/* exampleRequests */ ImmutableList.of(),
examplePaths, exampleQueries,
HttpMethod.GET, null);
}

@Test
void invalidQueryString() {
final String invalidQuery = "?%x";
assertThatThrownBy(() -> newMethodInfo(ImmutableList.of(), ImmutableList.of(invalidQuery)))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("contains an invalid query string");
}

@Test
void invalidPath() {
final String invalidPath = "a/b";
assertThatThrownBy(() -> newMethodInfo(ImmutableList.of(invalidPath), ImmutableList.of()))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("contains an invalid path");
}
}

0 comments on commit 773418f

Please sign in to comment.