Fix PR #550 Optional type wrapping of query parameters (#555)

- Adds tests that were missing in HelloControllerTest that actually exercises the feature
- Fixes the controller URLs removing incorrect {myOptional} from urls
- Fixes PathTypeConversion to handle null values
- Adds missing javadoc on new public methods of PathTypeConversion
- Adds some useful unit tests for TypeMap.OptionalHandler

Author: rbygrave

http-api/src/main/java/io/avaje/http/api/PathTypeConversion.java

@@ -53,19 +53,42 @@ private static void checkNull(String value) {
     }
   }
 
-  public static <T> List<T> list(Function<String, T> func, List<String> params) {
-    return params.stream().map(func).collect(Collectors.toList());
+  /**
+   * Return the list of parameters using a type conversion function.
+   */
+  public static <T> List<T> list(Function<String, T> typeConversion, List<String> params) {
+    return params.stream().map(typeConversion).collect(Collectors.toList());
   }
 
-  public static <T> Set<T> set(Function<String, T> func, List<String> params) {
-    return params.stream().map(func).collect(Collectors.toSet());
+  /**
+   * Return the parameters as a Set using a type conversion function.
+   */
+  public static <T> Set<T> set(Function<String, T> typeConversion, List<String> params) {
+    return params.stream().map(typeConversion).collect(Collectors.toSet());
   }
 
-  public static <T> Optional<T> optional(Function<String, T> func, String value) {
-    return Optional.ofNullable(func.apply(value));
+  /**
+   * Return an Optional taking a type conversion function and string value.
+   *
+   * @param typeConversion The type conversion function
+   * @param value          The nullable value
+   * @param <T>            The ty
+   * @return The Optional typed value
+   */
+  public static <T> Optional<T> optional(Function<String, T> typeConversion, String value) {
+    return value == null ? Optional.empty() : Optional.ofNullable(typeConversion.apply(value));
   }
 
-  /** Convert to int. */
+  /**
+   * Return an Optional for a nullable String value.
+   */
+  public static Optional<String> optional(String value) {
+    return Optional.ofNullable(value);
+  }
+
+  /**
+   * Convert to int.
+   */
   public static int asInt(String value) {
     checkNull(value);
     try {
@@ -75,7 +98,9 @@ public static int asInt(String value) {
     }
   }
 
-  /** Convert to enum. */
+  /**
+   * Convert to enum.
+   */
   @SuppressWarnings({"rawtypes"})
   public static <T> Enum asEnum(Class<T> clazz, String value) {
     checkNull(value);
@@ -86,6 +111,9 @@ public static <T> Enum asEnum(Class<T> clazz, String value) {
     }
   }
 
+  /**
+   * Convert to an Enum of the given type.
+   */
   @SuppressWarnings({"unchecked", "rawtypes"})
   public static <T> Enum convertEnum(Class<T> clazz, String value) {
     try {
@@ -260,7 +288,9 @@ public static Integer asInteger(String value) {
     }
   }
 
-  /** Convert to enum. */
+  /**
+   * Convert to enum of the given type.
+   */
   @SuppressWarnings({"rawtypes"})
   public static <T> Enum toEnum(Class<T> clazz, String value) {
     if (isNullOrEmpty(value)) {

http-generator-core/src/main/java/io/avaje/http/generator/core/TypeMap.java

@@ -347,9 +347,8 @@ static class CollectionHandler implements TypeHandler {
     private String toMethod;
 
     CollectionHandler(TypeHandler handler, boolean set, boolean isEnum) {
-
       this.importTypes = new ArrayList<>(handler.importTypes());
-      importTypes.add("io.avaje.http.api.PathTypeConversion");
+      this.importTypes.add("io.avaje.http.api.PathTypeConversion");
       this.shortName = handler.shortName();
       this.toMethod =
           (set ? "set" : "list")
@@ -369,7 +368,6 @@ public boolean isPrimitive() {
 
     @Override
     public List<String> importTypes() {
-
       return importTypes;
     }
 
@@ -393,21 +391,24 @@ static class OptionalHandler implements TypeHandler {
 
     private final List<String> importTypes;
     private final String shortName;
-    private String toMethod;
+    private final String toMethod;
 
     OptionalHandler(TypeHandler handler, boolean isEnum) {
-
       this.importTypes = new ArrayList<>(handler.importTypes());
-      importTypes.add("io.avaje.http.api.PathTypeConversion");
+      this.importTypes.add("io.avaje.http.api.PathTypeConversion");
       this.shortName = handler.shortName();
-      this.toMethod =
-        "optional("
-        + (isEnum
-           ? "qp -> " + handler.toMethod() + " qp)"
-           : "PathTypeConversion::as" + shortName)
-        + ", ";
+      this.toMethod = buildToMethod(handler, isEnum);
+    }
 
-      this.toMethod = toMethod.replace("PathTypeConversion::asString", "Object::toString");
+    static String buildToMethod(TypeHandler handler, boolean isEnum) {
+      if (isEnum) {
+        return "optional(qp -> " + handler.toMethod() + " qp), ";
+      }
+      if ("String".equals(handler.shortName())) {
+        return "optional(";
+      } else {
+        return "optional(PathTypeConversion::as" + handler.shortName() + ", ";
+      }
     }
 
     @Override
@@ -417,7 +418,6 @@ public boolean isPrimitive() {
 
     @Override
     public List<String> importTypes() {
-
       return importTypes;
     }
 

http-generator-core/src/test/java/io/avaje/http/generator/core/TypeMapTest.java

@@ -69,4 +69,25 @@ void get_BigInt() {
     assertThat(handler.asMethod()).isEqualTo("asBigInteger(");
     assertFalse(handler.isPrimitive());
   }
+
+  @Test
+  void get_OptionalInteger() {
+    TypeHandler handler = TypeMap.get("java.lang.Integer");
+    TypeMap.OptionalHandler optionalHandler = new TypeMap.OptionalHandler(handler, false);
+    assertThat(optionalHandler.toMethod()).isEqualTo("optional(PathTypeConversion::asInteger, ");
+  }
+
+  @Test
+  void get_OptionalString() {
+    TypeHandler handler = TypeMap.get("java.lang.String");
+    TypeMap.OptionalHandler optionalHandler = new TypeMap.OptionalHandler(handler, false);
+    assertThat(optionalHandler.toMethod()).isEqualTo("optional(");
+  }
+
+  @Test
+  void get_OptionalEnum() {
+    TypeHandler handler = TypeMap.enumParamHandler(UType.parse("org.my.MyEnum"));
+    TypeMap.OptionalHandler optionalHandler = new TypeMap.OptionalHandler(handler, true);
+    assertThat(optionalHandler.toMethod()).isEqualTo("optional(qp -> (MyEnum) toEnum(MyEnum.class,  qp), ");
+  }
 }

tests/test-jex/src/main/java/org/example/web/myapp/WebController.java

@@ -178,21 +178,24 @@ String takesNestedEnum(Foo.NestedEnum myEnum) {
     return "takesNestedEnum-" + myEnum;
   }
 
+  @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
   @Produces(value = "text/plain")
-  @Get("takesOptional{myOptional}")
-  String takesOptional(@QueryParam("myOptional") Optional<Long> myOptional) {
+  @Get("takesOptional")
+  String takesOptional(Optional<Long> myOptional) {
     return "takesOptional-" + myOptional;
   }
 
+  @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
   @Produces(value = "text/plain")
-  @Get("takesOptionalEnum{myOptional}")
+  @Get("takesOptionalEnum")
   String takesOptionalEnum(@QueryParam("myOptional") Optional<Foo.NestedEnum> myOptional) {
     return "takesOptionalEnum-" + myOptional;
   }
 
+  @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
   @Produces(value = "text/plain")
-  @Get("takesOptionalEnum{myOptional}")
-  String takesOptionalString(@QueryParam("myOptional") Optional<String> myOptional) {
+  @Get("takesOptionalString")
+  String takesOptionalString(@QueryParam Optional<String> myOptional) {
     return "takesOptionalString-" + myOptional;
   }
 }

tests/test-jex/src/test/java/org/example/web/HelloControllerTest.java

@@ -106,4 +106,74 @@ void validation_expect_HttpException() {
     final ErrorResponse errBean = ex.bean(ErrorResponse.class);
     assertThat(errBean.get("name")).isEqualTo("must not be null");
   }
+
+  @Test
+  void optionalQueryParamLong() {
+    HttpResponse<String> res = client.request()
+      .path("hello/takesOptional")
+      .GET()
+      .asString();
+
+    assertThat(res.statusCode()).isEqualTo(200);
+    assertThat(res.body()).isEqualTo("takesOptional-Optional.empty");
+  }
+
+  @Test
+  void optionalQueryParamLong_withValue() {
+    HttpResponse<String> res = client.request()
+      .path("hello/takesOptional")
+      .queryParam("myOptional","42")
+      .GET()
+      .asString();
+
+    assertThat(res.statusCode()).isEqualTo(200);
+    assertThat(res.body()).isEqualTo("takesOptional-Optional[42]");
+  }
+
+  @Test
+  void optionalQueryParamEnum() {
+    HttpResponse<String> res = client.request()
+      .path("hello/takesOptionalEnum")
+      .GET()
+      .asString();
+
+    assertThat(res.statusCode()).isEqualTo(200);
+    assertThat(res.body()).isEqualTo("takesOptionalEnum-Optional.empty");
+  }
+
+  @Test
+  void optionalQueryParamEnum_withValue() {
+    HttpResponse<String> res = client.request()
+      .path("hello/takesOptionalEnum")
+      .queryParam("myOptional","B")
+      .GET()
+      .asString();
+
+    assertThat(res.statusCode()).isEqualTo(200);
+    assertThat(res.body()).isEqualTo("takesOptionalEnum-Optional[B]");
+  }
+
+  @Test
+  void optionalQueryParamString() {
+    HttpResponse<String> res = client.request()
+      .path("hello/takesOptionalString")
+      .GET()
+      .asString();
+
+    assertThat(res.statusCode()).isEqualTo(200);
+    assertThat(res.body()).isEqualTo("takesOptionalString-Optional.empty");
+  }
+
+  @Test
+  void optionalQueryParamString_withValue() {
+    HttpResponse<String> res = client.request()
+      .path("hello/takesOptionalString")
+      .queryParam("myOptional","foo")
+      .GET()
+      .asString();
+
+    assertThat(res.statusCode()).isEqualTo(200);
+    assertThat(res.body()).isEqualTo("takesOptionalString-Optional[foo]");
+  }
+
 }