Skip to content

Commit

Permalink
fix optional param filtering (#28)
Browse files Browse the repository at this point in the history 
Closes #27.

## Before this PR
There was an issue where the generated services would not compile if they had optional path params and body params
<!-- Describe the problem you encountered with the current state of the world (or link to an issue) and why it's important to fix now. -->

## After this PR
<!-- Describe at a high-level why this approach is better. -->
The issue is resolved and we preserve ordering of parameters 

<!-- Reference any existing GitHub issues, e.g. 'fixes #000' or 'relevant to #000' -->
  • Loading branch information
@ferozco @iamdanfox
ferozco authored and iamdanfox committed Jul 4, 2018
1 parent 569b017 commit 0bc8655
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 30 deletions.
46 changes: 27 additions & 19 deletions ...re-java-core/src/main/java/com/palantir/conjure/java/services/JerseyServiceGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import static com.google.common.base.Preconditions.checkArgument;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.palantir.conjure.java.ConjureAnnotations;
import com.palantir.conjure.java.types.JerseyMethodTypeClassNameVisitor;
Expand Down Expand Up @@ -59,6 +58,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import javax.lang.model.element.Modifier;
import org.apache.commons.lang3.StringUtils;

Expand Down Expand Up @@ -178,10 +178,7 @@ private List<MethodSpec> generateCompatibilityBackfillServiceMethods(
List<MethodSpec> alternateMethods = Lists.newArrayList();
for (int i = 0; i < queryArgs.size(); i++) {
alternateMethods.add(createCompatibilityBackfillMethod(
EndpointDefinition.builder()
.from(endpointDef)
.args(Iterables.concat(args, queryArgs.subList(0, i)))
.build(),
endpointDef,
returnTypeMapper,
methodTypeMapper,
queryArgs.subList(i, queryArgs.size())));
Expand All @@ -195,33 +192,44 @@ private MethodSpec createCompatibilityBackfillMethod(
TypeMapper returnTypeMapper,
TypeMapper methodTypeMapper,
List<ArgumentDefinition> extraArgs) {
List<ParameterSpec> params = createServiceMethodParameters(endpointDef, methodTypeMapper, false);

// ensure the correct ordering of parameters by creating the complete sorted parameter list
List<ParameterSpec> sortedParams = createServiceMethodParameters(endpointDef, methodTypeMapper, false);
List<Optional<ArgumentDefinition>> sortedMaybeExtraArgs = sortedParams.stream().map(param ->
extraArgs.stream()
.filter(arg -> arg.getArgName().get().equals(param.name))
.findFirst())
.collect(Collectors.toList());

// omit extraArgs from the back fill method signature
MethodSpec.Builder methodBuilder = MethodSpec.methodBuilder(endpointDef.getEndpointName().get())
.addModifiers(Modifier.PUBLIC, Modifier.DEFAULT)
.addAnnotation(Deprecated.class)
.addParameters(params);
.addParameters(IntStream.range(0, sortedParams.size())
.filter(i -> !sortedMaybeExtraArgs.get(i).isPresent())
.mapToObj(sortedParams::get)
.collect(Collectors.toList()));

endpointDef.getReturns().ifPresent(type -> methodBuilder.returns(returnTypeMapper.getClassName(type)));

// replace extraArgs with default values when invoking the complete method
StringBuilder sb = new StringBuilder("return $N(");
for (ParameterSpec param : params) {
sb.append("$N, ");
}

List<CodeBlock> fillerValues = Lists.newArrayList();
for (ArgumentDefinition arg : extraArgs) {
sb.append("$L, ");
fillerValues.add(arg.getType().accept(TYPE_DEFAULT_VALUE));
}
List<Object> values = IntStream.range(0, sortedParams.size()).mapToObj(i -> {
Optional<ArgumentDefinition> maybeArgDef = sortedMaybeExtraArgs.get(i);
if (maybeArgDef.isPresent()) {
sb.append("$L, ");
return maybeArgDef.get().getType().accept(TYPE_DEFAULT_VALUE);
} else {
sb.append("$N, ");
return sortedParams.get(i);
}
}).collect(Collectors.toList());
// trim the end
sb.setLength(sb.length() - 2);
sb.append(")");

ImmutableList<Object> methodCallArgs = ImmutableList.builder()
.add(endpointDef.getEndpointName().get())
.addAll(params)
.addAll(fillerValues)
.addAll(values)
.build();

methodBuilder.addStatement(sb.toString(), methodCallArgs.toArray(new Object[0]));
Expand Down
3 changes: 2 additions & 1 deletion conjure-java-core/src/test/resources/example-service.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,9 @@ services:
returns: optional<string>

testQueryParams:
http: GET /test-query-params
http: POST /test-query-params
args:
query: string
something:
type: ResourceIdentifier
param-id: different
Expand Down
24 changes: 16 additions & 8 deletions conjure-java-core/src/test/resources/test/api/TestService.java.jersey
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,16 @@ public interface TestService {
@HeaderParam("Authorization") AuthHeader authHeader,
@PathParam("datasetRid") ResourceIdentifier datasetRid);

@GET
@POST
@Path("catalog/test-query-params")
int testQueryParams(
@HeaderParam("Authorization") AuthHeader authHeader,
@QueryParam("different") ResourceIdentifier something,
@QueryParam("implicit") ResourceIdentifier implicit,
@QueryParam("optionalMiddle") Optional<ResourceIdentifier> optionalMiddle,
@QueryParam("setEnd") Set<String> setEnd,
@QueryParam("optionalEnd") Optional<ResourceIdentifier> optionalEnd);
@QueryParam("optionalEnd") Optional<ResourceIdentifier> optionalEnd,
String query);

@GET
@Path("catalog/boolean")
Expand All @@ -124,29 +125,35 @@ public interface TestService {

@Deprecated
default int testQueryParams(
AuthHeader authHeader, ResourceIdentifier something, ResourceIdentifier implicit) {
AuthHeader authHeader,
ResourceIdentifier something,
ResourceIdentifier implicit,
String query) {
return testQueryParams(
authHeader,
something,
implicit,
Optional.empty(),
Collections.emptySet(),
Optional.empty());
Optional.empty(),
query);
}

@Deprecated
default int testQueryParams(
AuthHeader authHeader,
ResourceIdentifier something,
ResourceIdentifier implicit,
Optional<ResourceIdentifier> optionalMiddle) {
Optional<ResourceIdentifier> optionalMiddle,
String query) {
return testQueryParams(
authHeader,
something,
implicit,
optionalMiddle,
Collections.emptySet(),
Optional.empty());
Optional.empty(),
query);
}

@Deprecated
Expand All @@ -155,8 +162,9 @@ public interface TestService {
ResourceIdentifier something,
ResourceIdentifier implicit,
Optional<ResourceIdentifier> optionalMiddle,
Set<String> setEnd) {
Set<String> setEnd,
String query) {
return testQueryParams(
authHeader, something, implicit, optionalMiddle, setEnd, Optional.empty());
authHeader, something, implicit, optionalMiddle, setEnd, Optional.empty(), query);
}
}
3 changes: 2 additions & 1 deletion conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,10 @@ public interface TestServiceRetrofit {
@Header("Authorization") AuthHeader authHeader,
@Path("datasetRid") ResourceIdentifier datasetRid);

@GET("./catalog/test-query-params")
@POST("./catalog/test-query-params")
Call<Integer> testQueryParams(
@Header("Authorization") AuthHeader authHeader,
@Body String query,
@Query("different") ResourceIdentifier something,
@Query("optionalMiddle") Optional<ResourceIdentifier> optionalMiddle,
@Query("implicit") ResourceIdentifier implicit,
Expand Down
3 changes: 2 additions & 1 deletion ...ava-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_completable_future
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,10 @@ public interface TestServiceRetrofit {
@Header("Authorization") AuthHeader authHeader,
@Path("datasetRid") ResourceIdentifier datasetRid);

@GET("./catalog/test-query-params")
@POST("./catalog/test-query-params")
CompletableFuture<Integer> testQueryParams(
@Header("Authorization") AuthHeader authHeader,
@Body String query,
@Query("different") ResourceIdentifier something,
@Query("optionalMiddle") Optional<ResourceIdentifier> optionalMiddle,
@Query("implicit") ResourceIdentifier implicit,
Expand Down

0 comments on commit 0bc8655

Please sign in to comment.