Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of empty optional params #1332

Closed
wants to merge 1 commit into from
Closed

Improve handling of empty optional params #1332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...va/com/palantir/conjure/java/server/jersey/Java8OptionalDoubleParamConverterProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public <T> ParamConverter<T> getConverter(
public static final class OptionalDoubleParamConverter implements ParamConverter<OptionalDouble> {
@Override
public OptionalDouble fromString(final String value) {
if (value == null) {
if (value == null || value.isEmpty()) {
return OptionalDouble.empty();
}

Expand Down
2 changes: 1 addition & 1 deletion .../java/com/palantir/conjure/java/server/jersey/Java8OptionalIntParamConverterProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public <T> ParamConverter<T> getConverter(
public static final class OptionalIntParamConverter implements ParamConverter<OptionalInt> {
@Override
public OptionalInt fromString(final String value) {
if (value == null) {
if (value == null || value.isEmpty()) {
return OptionalInt.empty();
}

Expand Down
2 changes: 1 addition & 1 deletion ...java/com/palantir/conjure/java/server/jersey/Java8OptionalLongParamConverterProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public <T> ParamConverter<T> getConverter(
public static final class OptionalLongParamConverter implements ParamConverter<OptionalLong> {
@Override
public OptionalLong fromString(final String value) {
if (value == null) {
if (value == null || value.isEmpty()) {
return OptionalLong.empty();
}

Expand Down
40 changes: 34 additions & 6 deletions ...ersey-server/src/test/java/com/palantir/conjure/java/server/jersey/Java8OptionalTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public void before() {
}

@Test
public void testOptionalPresent() throws NoSuchMethodException, SecurityException {
public void testOptionalPresent() {
Response response = target.path("optional").queryParam("value", "val").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("valval");
Expand All @@ -73,28 +73,42 @@ public void testOptionalAbsent() {
}

@Test
public void testQueryParam_optionalPresent() throws NoSuchMethodException, SecurityException {
public void testQueryParam_optionalPresent() {
Response response = target.path("optional/string").queryParam("value", "val").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("val");
}

@Test
public void testQueryParam_optionalEmpty() {
Response response = target.path("optional/string").queryParam("value", "").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("");
}

@Test
public void testQueryParam_optionalAbsent() {
Response response = target.path("optional/string").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("default");
}

@Test
public void testQueryParam_optionalIntPresent() throws NoSuchMethodException, SecurityException {
public void testQueryParam_optionalIntPresent() {
Response response = target.path("optional/int").queryParam("value", "10").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("10");
}

@Test
public void testQueryParam_optionalIntEmpty() {
Response response = target.path("optional/int").queryParam("value", "").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("0");
}

@Test
public void testQueryParam_optionalIntAbsent() {
Response response = target.path("optional/int").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("0");
Expand All @@ -107,14 +121,21 @@ public void testQueryParam_optionalIntInvalid() {
}

@Test
public void testQueryParam_optionalDoublePresent() throws NoSuchMethodException, SecurityException {
public void testQueryParam_optionalDoublePresent() {
Response response = target.path("optional/double").queryParam("value", "1.5").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("1.5");
}

@Test
public void testQueryParam_optionalDoubleEmpty() {
Response response = target.path("optional/double").queryParam("value", "").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("0.0");
}

@Test
public void testQueryParam_optionalDoubleAbsent() {
Response response = target.path("optional/double").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("0.0");
Expand All @@ -127,14 +148,21 @@ public void testQueryParam_optionalDoubleInvalid() {
}

@Test
public void testQueryParam_optionalLongPresent() throws NoSuchMethodException, SecurityException {
public void testQueryParam_optionalLongPresent() {
Response response = target.path("optional/long").queryParam("value", "100").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("100");
}

@Test
public void testQueryParam_optionalLongEmpty() {
Response response = target.path("optional/long").queryParam("value", "").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("0");
}

@Test
public void testQueryParam_optionalLongAbsent() {
Response response = target.path("optional/long").request().get();
assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).isEqualTo("0");
Expand All @@ -148,7 +176,7 @@ public void testQueryParam_optionalLongInvalid() {

public static class OptionalTestServer extends Application<Configuration> {
@Override
public final void run(Configuration _config, final Environment env) throws Exception {
public final void run(Configuration _config, final Environment env) {
env.jersey().register(ConjureJerseyFeature.INSTANCE);
env.jersey().register(new EmptyOptionalTo204ExceptionMapper());
env.jersey().register(new OptionalTestResource());
Expand Down