From 0bc86556ccefda815f17f838f9f5b2ef41a30938 Mon Sep 17 00:00:00 2001 From: Felipe Orozco Date: Wed, 4 Jul 2018 13:22:35 +0100 Subject: [PATCH] fix optional param filtering (#28) 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 ## After this PR The issue is resolved and we preserve ordering of parameters --- .../java/services/JerseyServiceGenerator.java | 46 +++++++++++-------- .../src/test/resources/example-service.yml | 3 +- .../test/api/TestService.java.jersey | 24 ++++++---- .../api/TestServiceRetrofit.java.retrofit | 3 +- ...eRetrofit.java.retrofit_completable_future | 3 +- 5 files changed, 49 insertions(+), 30 deletions(-) diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/services/JerseyServiceGenerator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/services/JerseyServiceGenerator.java index 15b345865..e1a97e040 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/services/JerseyServiceGenerator.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/services/JerseyServiceGenerator.java @@ -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; @@ -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; @@ -178,10 +178,7 @@ private List generateCompatibilityBackfillServiceMethods( List 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()))); @@ -195,33 +192,44 @@ private MethodSpec createCompatibilityBackfillMethod( TypeMapper returnTypeMapper, TypeMapper methodTypeMapper, List extraArgs) { - List params = createServiceMethodParameters(endpointDef, methodTypeMapper, false); - + // ensure the correct ordering of parameters by creating the complete sorted parameter list + List sortedParams = createServiceMethodParameters(endpointDef, methodTypeMapper, false); + List> 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 fillerValues = Lists.newArrayList(); - for (ArgumentDefinition arg : extraArgs) { - sb.append("$L, "); - fillerValues.add(arg.getType().accept(TYPE_DEFAULT_VALUE)); - } + List values = IntStream.range(0, sortedParams.size()).mapToObj(i -> { + Optional 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 methodCallArgs = ImmutableList.builder() .add(endpointDef.getEndpointName().get()) - .addAll(params) - .addAll(fillerValues) + .addAll(values) .build(); methodBuilder.addStatement(sb.toString(), methodCallArgs.toArray(new Object[0])); diff --git a/conjure-java-core/src/test/resources/example-service.yml b/conjure-java-core/src/test/resources/example-service.yml index 5b7a3100a..09df9338a 100644 --- a/conjure-java-core/src/test/resources/example-service.yml +++ b/conjure-java-core/src/test/resources/example-service.yml @@ -149,8 +149,9 @@ services: returns: optional testQueryParams: - http: GET /test-query-params + http: POST /test-query-params args: + query: string something: type: ResourceIdentifier param-id: different diff --git a/conjure-java-core/src/test/resources/test/api/TestService.java.jersey b/conjure-java-core/src/test/resources/test/api/TestService.java.jersey index a28e7d781..3669ec116 100644 --- a/conjure-java-core/src/test/resources/test/api/TestService.java.jersey +++ b/conjure-java-core/src/test/resources/test/api/TestService.java.jersey @@ -100,7 +100,7 @@ public interface TestService { @HeaderParam("Authorization") AuthHeader authHeader, @PathParam("datasetRid") ResourceIdentifier datasetRid); - @GET + @POST @Path("catalog/test-query-params") int testQueryParams( @HeaderParam("Authorization") AuthHeader authHeader, @@ -108,7 +108,8 @@ public interface TestService { @QueryParam("implicit") ResourceIdentifier implicit, @QueryParam("optionalMiddle") Optional optionalMiddle, @QueryParam("setEnd") Set setEnd, - @QueryParam("optionalEnd") Optional optionalEnd); + @QueryParam("optionalEnd") Optional optionalEnd, + String query); @GET @Path("catalog/boolean") @@ -124,14 +125,18 @@ 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 @@ -139,14 +144,16 @@ public interface TestService { AuthHeader authHeader, ResourceIdentifier something, ResourceIdentifier implicit, - Optional optionalMiddle) { + Optional optionalMiddle, + String query) { return testQueryParams( authHeader, something, implicit, optionalMiddle, Collections.emptySet(), - Optional.empty()); + Optional.empty(), + query); } @Deprecated @@ -155,8 +162,9 @@ public interface TestService { ResourceIdentifier something, ResourceIdentifier implicit, Optional optionalMiddle, - Set setEnd) { + Set setEnd, + String query) { return testQueryParams( - authHeader, something, implicit, optionalMiddle, setEnd, Optional.empty()); + authHeader, something, implicit, optionalMiddle, setEnd, Optional.empty(), query); } } diff --git a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit b/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit index 00b6aedf0..81eebb09d 100644 --- a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit +++ b/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit @@ -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 testQueryParams( @Header("Authorization") AuthHeader authHeader, + @Body String query, @Query("different") ResourceIdentifier something, @Query("optionalMiddle") Optional optionalMiddle, @Query("implicit") ResourceIdentifier implicit, diff --git a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_completable_future b/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_completable_future index 48e4dc3dc..2a7831141 100644 --- a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_completable_future +++ b/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_completable_future @@ -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 testQueryParams( @Header("Authorization") AuthHeader authHeader, + @Body String query, @Query("different") ResourceIdentifier something, @Query("optionalMiddle") Optional optionalMiddle, @Query("implicit") ResourceIdentifier implicit,