From b7bfff38ff0efb8f2dcfa4477ef36152b54776b5 Mon Sep 17 00:00:00 2001 From: Joe Ellis Date: Mon, 16 Jul 2018 08:57:24 -0400 Subject: [PATCH] [improvement] FeatureFlag to make Response the return type for binary Jersey Responses (#37) ## Before this PR Currently binary response types are rendered as `StreamingOutput`. There's a bug in Jersey which causes a failure part way through streaming back that response to appear as a successful response to the client: https://github.com/eclipse-ee4j/jersey/issues/3850 ## After this PR By rendering `Response` as the return type we are still able to use `StreamingOutput` as the body of the response, but we are also able to include the `Content-Length` header when we know the size of the response. Including the content length allows clients to determine there was an error, even when the stream appeared to have been closed successfully. --- .../palantir/conjure/java/FeatureFlags.java | 7 ++++++ .../java/services/JerseyServiceGenerator.java | 14 ++++++++++-- .../JerseyReturnTypeClassNameVisitor.java | 14 ++++++++++-- .../java/JerseyServiceGeneratorTests.java | 22 +++++++++++++++++++ ...TestService.java.jersey.binary_as_response | 19 ++++++++++++++++ .../conjure/java/cli/CliConfiguration.java | 4 ++++ .../conjure/java/cli/ConjureJavaCli.java | 4 +++- .../conjure/java/cli/ConjureJavaCliTest.java | 7 ++++-- 8 files changed, 84 insertions(+), 7 deletions(-) create mode 100644 conjure-java-core/src/test/resources/test/api/TestService.java.jersey.binary_as_response diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/FeatureFlags.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/FeatureFlags.java index df17c11c9..8b5660c6a 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/FeatureFlags.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/FeatureFlags.java @@ -16,7 +16,9 @@ package com.palantir.conjure.java; +import com.palantir.conjure.java.services.JerseyServiceGenerator; import com.palantir.conjure.java.services.Retrofit2ServiceGenerator; +import javax.ws.rs.core.Response; public enum FeatureFlags { /** @@ -25,4 +27,9 @@ public enum FeatureFlags { */ RetrofitCompletableFutures, + /** + * Instructs the {@link JerseyServiceGenerator} to generate binary response endpoints with the {@link Response} + * type. + */ + JerseyBinaryAsResponse, } 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 4f5f0ef2e..0529340b1 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,8 +19,10 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import com.palantir.conjure.java.ConjureAnnotations; +import com.palantir.conjure.java.FeatureFlags; import com.palantir.conjure.java.types.JerseyMethodTypeClassNameVisitor; import com.palantir.conjure.java.types.JerseyReturnTypeClassNameVisitor; import com.palantir.conjure.java.types.TypeMapper; @@ -65,13 +67,21 @@ public final class JerseyServiceGenerator implements ServiceGenerator { - public JerseyServiceGenerator() {} + private final Set experimentalFeatures; + + public JerseyServiceGenerator() { + this(ImmutableSet.of()); + } + + public JerseyServiceGenerator(Set experimentalFeatures) { + this.experimentalFeatures = ImmutableSet.copyOf(experimentalFeatures); + } @Override public Set generate(ConjureDefinition conjureDefinition) { TypeMapper returnTypeMapper = new TypeMapper( conjureDefinition.getTypes(), - types -> new JerseyReturnTypeClassNameVisitor(types)); + types -> new JerseyReturnTypeClassNameVisitor(types, experimentalFeatures)); TypeMapper methodTypeMapper = new TypeMapper( conjureDefinition.getTypes(), JerseyMethodTypeClassNameVisitor::new); return conjureDefinition.getServices().stream() diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/JerseyReturnTypeClassNameVisitor.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/JerseyReturnTypeClassNameVisitor.java index ffa0a0cb5..92cc27184 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/JerseyReturnTypeClassNameVisitor.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/JerseyReturnTypeClassNameVisitor.java @@ -16,6 +16,7 @@ package com.palantir.conjure.java.types; +import com.palantir.conjure.java.FeatureFlags; import com.palantir.conjure.spec.ExternalReference; import com.palantir.conjure.spec.ListType; import com.palantir.conjure.spec.MapType; @@ -26,13 +27,18 @@ import com.squareup.javapoet.ClassName; import com.squareup.javapoet.TypeName; import java.util.List; +import java.util.Set; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.StreamingOutput; public final class JerseyReturnTypeClassNameVisitor implements ClassNameVisitor { private final DefaultClassNameVisitor delegate; + private final boolean binaryAsResponse; - public JerseyReturnTypeClassNameVisitor(List types) { + public JerseyReturnTypeClassNameVisitor(List types, Set featureFlags) { this.delegate = new DefaultClassNameVisitor(types); + this.binaryAsResponse = featureFlags.contains(FeatureFlags.JerseyBinaryAsResponse); } @Override @@ -53,7 +59,11 @@ public TypeName visitOptional(OptionalType type) { @Override public TypeName visitPrimitive(PrimitiveType type) { if (type.get() == PrimitiveType.Value.BINARY) { - return ClassName.get(javax.ws.rs.core.StreamingOutput.class); + if (binaryAsResponse) { + return ClassName.get(Response.class); + } else { + return ClassName.get(StreamingOutput.class); + } } return delegate.visitPrimitive(type); } diff --git a/conjure-java-core/src/test/java/com/palantir/conjure/java/JerseyServiceGeneratorTests.java b/conjure-java-core/src/test/java/com/palantir/conjure/java/JerseyServiceGeneratorTests.java index 3cda2af0a..d283d232a 100644 --- a/conjure-java-core/src/test/java/com/palantir/conjure/java/JerseyServiceGeneratorTests.java +++ b/conjure-java-core/src/test/java/com/palantir/conjure/java/JerseyServiceGeneratorTests.java @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.assertThat; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.palantir.conjure.defs.Conjure; import com.palantir.conjure.java.services.JerseyServiceGenerator; import com.palantir.conjure.spec.ConjureDefinition; @@ -87,6 +88,27 @@ public void testBinaryReturnInputStream() throws IOException { } } + @Test + public void testBinaryReturnResponse() throws IOException { + ConjureDefinition def = Conjure.parse( + ImmutableList.of(new File("src/test/resources/example-binary.yml"))); + List files = new JerseyServiceGenerator(ImmutableSet.of(FeatureFlags.JerseyBinaryAsResponse)) + .emit(def, folder.getRoot()); + + for (Path file : files) { + if (Boolean.valueOf(System.getProperty("recreate", "false"))) { + Path output = Paths.get( + "src/test/resources/test/api/" + file.getFileName() + ".jersey.binary_as_response"); + Files.delete(output); + Files.copy(file, output); + } + + assertThat(TestUtils.readFromFile(file)).isEqualTo( + TestUtils.readFromFile(Paths.get( + "src/test/resources/test/api/" + file.getFileName() + ".jersey.binary_as_response"))); + } + } + private void testServiceGeneration(String conjureFile) throws IOException { ConjureDefinition def = Conjure.parse( ImmutableList.of(new File("src/test/resources/" + conjureFile + ".yml"))); diff --git a/conjure-java-core/src/test/resources/test/api/TestService.java.jersey.binary_as_response b/conjure-java-core/src/test/resources/test/api/TestService.java.jersey.binary_as_response new file mode 100644 index 000000000..8e97a9c1b --- /dev/null +++ b/conjure-java-core/src/test/resources/test/api/TestService.java.jersey.binary_as_response @@ -0,0 +1,19 @@ +package test.api; + +import javax.annotation.Generated; +import javax.ws.rs.Consumes; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.Produces; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; + +@Consumes(MediaType.APPLICATION_JSON) +@Produces(MediaType.APPLICATION_JSON) +@Path("/") +@Generated("com.palantir.conjure.java.services.JerseyServiceGenerator") +public interface TestService { + @GET + @Produces(MediaType.APPLICATION_OCTET_STREAM) + Response getBinary(); +} diff --git a/conjure-java/src/main/java/com/palantir/conjure/java/cli/CliConfiguration.java b/conjure-java/src/main/java/com/palantir/conjure/java/cli/CliConfiguration.java index 2d8f6462c..cd3a73e95 100644 --- a/conjure-java/src/main/java/com/palantir/conjure/java/cli/CliConfiguration.java +++ b/conjure-java/src/main/java/com/palantir/conjure/java/cli/CliConfiguration.java @@ -30,6 +30,7 @@ public abstract class CliConfiguration { public static final String JERSEY_OPTION = "jersey"; public static final String RETROFIT_OPTION = "retrofit"; public static final String RETROFIT_COMPLETABLE_FUTURES = "retrofitCompletableFutures"; + public static final String JERSEY_BINARY_AS_RESPONSE = "jerseyBinaryAsResponse"; abstract File target(); @@ -91,6 +92,9 @@ static CliConfiguration of(String target, String outputDirectory, Option[] optio case RETROFIT_COMPLETABLE_FUTURES: flagsBuilder.add(FeatureFlags.RetrofitCompletableFutures); break; + case JERSEY_BINARY_AS_RESPONSE: + flagsBuilder.add(FeatureFlags.JerseyBinaryAsResponse); + break; default: break; } diff --git a/conjure-java/src/main/java/com/palantir/conjure/java/cli/ConjureJavaCli.java b/conjure-java/src/main/java/com/palantir/conjure/java/cli/ConjureJavaCli.java index e7176ff4d..0b2e4bb43 100644 --- a/conjure-java/src/main/java/com/palantir/conjure/java/cli/ConjureJavaCli.java +++ b/conjure-java/src/main/java/com/palantir/conjure/java/cli/ConjureJavaCli.java @@ -57,6 +57,8 @@ static CliConfiguration parseCliConfiguration(String[] args) { "Generate retrofit interfaces for streaming/async clients")); options.addOption(new Option(CliConfiguration.RETROFIT_COMPLETABLE_FUTURES, "Generate retrofit services which return Java8 CompletableFuture instead of OkHttp Call")); + options.addOption(new Option(CliConfiguration.JERSEY_BINARY_AS_RESPONSE, + "Generate jersey interfaces which return Response instead of StreamingOutput")); try { CommandLine cmd = parser.parse(options, args, false); @@ -75,7 +77,7 @@ static void generate(CliConfiguration config) { try { ConjureDefinition conjureDefinition = OBJECT_MAPPER.readValue(config.target(), ConjureDefinition.class); TypeGenerator typeGenerator = new ObjectGenerator(); - ServiceGenerator jerseyGenerator = new JerseyServiceGenerator(); + ServiceGenerator jerseyGenerator = new JerseyServiceGenerator(config.featureFlags()); ServiceGenerator retrofitGenerator = new Retrofit2ServiceGenerator(config.featureFlags()); if (config.generateObjects()) { diff --git a/conjure-java/src/test/java/com/palantir/conjure/java/cli/ConjureJavaCliTest.java b/conjure-java/src/test/java/com/palantir/conjure/java/cli/ConjureJavaCliTest.java index a52b8f403..240127b1e 100644 --- a/conjure-java/src/test/java/com/palantir/conjure/java/cli/ConjureJavaCliTest.java +++ b/conjure-java/src/test/java/com/palantir/conjure/java/cli/ConjureJavaCliTest.java @@ -64,13 +64,16 @@ public void parseFeatureFlags() { targetFile.getAbsolutePath(), folder.getRoot().getAbsolutePath(), "--objects", - "--retrofitCompletableFutures" + "--retrofitCompletableFutures", + "--jerseyBinaryAsResponse" }; CliConfiguration expectedConfiguration = CliConfiguration.builder() .target(targetFile) .outputDirectory(folder.getRoot()) .generateObjects(true) - .featureFlags(ImmutableSet.of(FeatureFlags.RetrofitCompletableFutures)) + .featureFlags(ImmutableSet.of( + FeatureFlags.RetrofitCompletableFutures, + FeatureFlags.JerseyBinaryAsResponse)) .build(); assertThat(ConjureJavaCli.parseCliConfiguration(args)).isEqualTo(expectedConfiguration); }