Unhelpful error message when a required parameter is not configured (#161)

* Parameter name in error messages when parameter is missing

* Trying new approach

* New approach

* Fixing test errors

* Only add not null values

* undo changes

* Refactoring

* Comments

* Removed default value setter

* Use default value on ressource parameter  with value and default value

* Reduced number of loops for parsing parameter set

* Changes requested in code review.

Author: fmeheust

ojdbc-provider-common/src/main/java/oracle/jdbc/provider/parameter/ParameterSetBuilder.java

@@ -44,13 +44,17 @@
 public interface ParameterSetBuilder {
 
   /**
-   * Adds a parameter with a given name and value.
+   * Adds a parameter with a given name and value. If the value is null, the 
+   * ParameterSet will behave as if no value has been set for the parameter. 
+   * Calling this method with a null value allows the ParameterSet to create 
+   * error messages that identify the {@code name} of missing parameters.
    *
    * @param <T> The type of value that is assigned to the parameter
-   * @param name The parameter name
+   * @param name The parameter name. Not null.
    * @param parameter The parameter that is assigned with the {@code name} and
-   *                  {@code value} in this set
-   * @param value The value
+   *                  {@code value} in this set. Not null.
+   * @param value The value, or null if the parameter is not configured with a 
+   *              value.
    * @return A reference to this object
    */
   <T> ParameterSetBuilder add(String name, Parameter<T> parameter, T value);

ojdbc-provider-common/src/main/java/oracle/jdbc/provider/parameter/ParameterSetBuilderImpl.java

@@ -52,7 +52,9 @@ final class ParameterSetBuilderImpl implements ParameterSetBuilder {
 
   @Override
   public <T> ParameterSetBuilder add(String name, Parameter<T> parameter, T value) {
-    parameterValues.put(parameter, value);
+    if (value != null) {
+      parameterValues.put(parameter, value);
+    }
     parameterNames.put(parameter, name);
     return this;
   }

ojdbc-provider-common/src/main/java/oracle/jdbc/provider/parameter/ParameterSetParser.java

@@ -189,6 +189,11 @@ <T> Builder addParameter(
      * the parsed input includes an assignment such as "x=0", then "0" is input
      * to the {@code valueSetter}.
      * </p><p>
+     * The {@code valueSetter} must be able to handle a null input. A null input will occur
+     * when no value is set for the parameter. The {@code valueSetter} may simply pass
+     * the null value onto the {@code ParameterSetBuilder}, or it may pass in a default
+     * value.
+     * </p><p>
      * This method is designed for cases where a single parameter in text format
      * may map to multiple {@link Parameter} objects. The {@code valueSetter}
      * function can perform multiple calls to set each parameter, as in this
@@ -212,52 +217,5 @@ <T> Builder addParameter(
     Builder addParameter(
       String name, BiConsumer<String, ParameterSetBuilder> valueSetter);
 
-    /**
-     * <p>
-     * Adds a parameter that a parser identifies by a given
-     * {@code name}, and invokes a {@code defaultValueSetter} to assign a value
-     * if the name is not present, or invokes a {@code valueSetter} function for
-     * assigning the value if the name is present. The input to the
-     * {@code defaultValueSetter} function is a {@link ParameterSetBuilder} that
-     * builds the parsed {@link ParameterSet}. The input to the
-     * {@code valueSetter} is the value as it appears in text form, and a
-     * {@link ParameterSetBuilder} that builds the parsed {@link ParameterSet}. If
-     * the name passed to this method is "x", and the parsed input includes an
-     * assignment such as "x=0", then "0" is input to the {@code valueSetter}.
-     * </p><p>
-     * This method is designed for cases where a single parameter in text format
-     * may map to multiple {@link Parameter} objects. The
-     * {@code defaultValueSetter} and {@code valueSetter} functions can perform
-     * multiple calls to set each parameter, as in this example:
-     * </p>
-     * <pre>{@code
-     * builder.addParameter(
-     *   "coordinate",
-     *   (parameterSetBuilder) -> {
-     *     // Assign the default value of 0 to X, Y, and Z
-     *     parameterSetBuilder.add("coordinate", X, 0);
-     *     parameterSetBuilder.add("coordinate", Y, 0);
-     *     parameterSetBuilder.add("coordinate", Z, 0);
-     *   },
-     *   (value, parameterSetBuilder) -> {
-     *     // Split "x,y,z" formatted value
-     *     String[] xyz = value.split(",");
-     *     parameterSetBuilder.add("coordinate", X, xyz[0]);
-     *     parameterSetBuilder.add("coordinate", Y, xyz[1]);
-     *     parameterSetBuilder.add("coordinate", Z, xyz[2]);
-     * });
-     * }</pre>
-     *
-     * @param name Name of the parsed parameter. Not null.
-     * @param defaultValueSetter Parses and sets the default value of
-     *        parameter(s). Not null.
-     * @param valueSetter Parses and sets the value of parameter(s) from text
-     *        input. Not null.
-     * @return This builder
-     */
-    Builder addParameter(
-      String name,
-      Consumer<ParameterSetBuilder> defaultValueSetter,
-      BiConsumer<String, ParameterSetBuilder> valueSetter);
   }
 }

ojdbc-provider-common/src/main/java/oracle/jdbc/provider/parameter/ParameterSetParserImpl.java

@@ -38,17 +38,21 @@
 
 package  oracle.jdbc.provider.parameter;
 
-import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.Map.Entry;
+import java.util.Set;
 import java.util.function.BiConsumer;
-import java.util.function.Consumer;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 
 import static java.lang.String.format;
 import static java.util.Objects.requireNonNull;
 
+import java.util.Arrays;
+
 final class ParameterSetParserImpl implements ParameterSetParser {
 
   /** Parameter value parsers mapped to a case insensitive parameter name */
@@ -65,14 +69,19 @@ public ParameterSet parseNamedValues(Map<String, String> namedValues) {
 
     ParameterSetBuilder builder = ParameterSet.builder();
 
-    for (ParameterParser parameterParser : parameterParsers.values())
-      parameterParser.setDefaultValue(builder);
+    // Make a copy of the keyset containing lower case keys, from which we will
+    // remove those present in the namedValues map
+    Set<String> missingKeys = parameterParsers
+        .keySet().stream().map(String::toLowerCase).collect(Collectors.toSet());
 
     for (Map.Entry<String, String> namedValue : namedValues.entrySet()) {
 
       String name = namedValue.getKey();
       ParameterParser parameterParser = parameterParsers.get(name);
 
+      // remove key from missing keys
+      missingKeys.remove(name.toLowerCase());
+
       if (parameterParser == null) {
         throw new IllegalArgumentException(format(
           "Unrecognized parameter name: \"%s\". Valid parameter names are: %s",
@@ -82,6 +91,11 @@ public ParameterSet parseNamedValues(Map<String, String> namedValues) {
       parameterParser.setValue(builder, namedValue.getValue());
     }
 
+    // Set value null to parameters that were not present in namedValues
+    for (String missingKey : missingKeys) {
+      parameterParsers.get(missingKey).setValue(builder, null);
+    }
+
     return builder.build();
   }
 
@@ -106,7 +120,6 @@ public ParameterSetParser build() {
     public Builder addParameter(String name, Parameter<String> parameter) {
       addParameterParser(
           name,
-          ParameterParser.SET_NO_DEFAULT,
           (value, builder) -> builder.add(name, parameter, value));
       return this;
     }
@@ -115,9 +128,9 @@ public Builder addParameter(String name, Parameter<String> parameter) {
     public Builder addParameter(
             String name, Parameter<String> parameter, String defaultValue) {
       addParameterParser(
-              name,
-              (builder) -> builder.add(name, parameter, defaultValue),
-              (value, builder) -> builder.add(name, parameter, value));
+          name,
+          (value, builder) -> builder.add(name, parameter, 
+                              value == null ? defaultValue : value));
       return this;
     }
 
@@ -126,7 +139,6 @@ public <T> Builder addParameter(
       String name, Parameter<T> parameter, Function<String, T> valueParser) {
       addParameterParser(
           name,
-          ParameterParser.SET_NO_DEFAULT,
           (value, builder) ->
               builder.add(name, parameter, valueParser.apply(value)));
       return this;
@@ -138,37 +150,29 @@ public <T> Builder addParameter(
       Function<String, T> valueParser) {
       addParameterParser(
           name,
-          builder -> builder.add(name, parameter, defaultValue),
-          (value, builder) ->
-              builder.add(name, parameter, valueParser.apply(value)));
+          (value, builder) -> {
+            // If the value is null set the default value.
+            if (value != null)
+              builder.add(name, parameter, valueParser.apply(value));
+            else
+              builder.add(name, parameter, defaultValue);
+          });
       return this;
     }
 
     @Override
     public Builder addParameter(
       String name, BiConsumer<String, ParameterSetBuilder> valueSetter) {
-      addParameterParser(
-          name, ParameterParser.SET_NO_DEFAULT, valueSetter);
-      return this;
-    }
-
-    @Override
-    public Builder addParameter(
-      String name,
-      Consumer<ParameterSetBuilder> defaultValueSetter,
-      BiConsumer<String, ParameterSetBuilder> valueSetter) {
-
-      addParameterParser(name, defaultValueSetter, valueSetter);
+      addParameterParser(name, valueSetter);
       return this;
     }
 
     private void addParameterParser(
         String name,
-        Consumer<ParameterSetBuilder> defaultValueSetter,
         BiConsumer<String, ParameterSetBuilder> valueSetter) {
 
       ParameterParser parameterParser =
-        new ParameterParser(defaultValueSetter, valueSetter);
+        new ParameterParser(valueSetter);
       parameterParsers.put(name, parameterParser);
     }
 
@@ -180,27 +184,14 @@ private void addParameterParser(
    */
   private static final class ParameterParser {
 
-    /** A consumer that sets no default value for a parameter */
-    private static final Consumer<ParameterSetBuilder> SET_NO_DEFAULT =
-        builder -> { };
-
-    /** A consumer that sets a default value for a parameter */
-    private final Consumer<ParameterSetBuilder> defaultValueSetter;
-
     /** A consumer that parses and sets a value for a parameter */
     private final BiConsumer<String, ParameterSetBuilder> valueSetter;
 
     private ParameterParser(
-        Consumer<ParameterSetBuilder> defaultValueSetter,
         BiConsumer<String, ParameterSetBuilder> valueSetter) {
-      this.defaultValueSetter = defaultValueSetter;
       this.valueSetter = valueSetter;
     }
 
-    void setDefaultValue(ParameterSetBuilder builder) {
-      defaultValueSetter.accept(builder);
-    }
-
     void setValue(ParameterSetBuilder builder, String value) {
       valueSetter.accept(value, builder);
     }

ojdbc-provider-common/src/main/java/oracle/jdbc/provider/resource/ResourceParameter.java

@@ -43,7 +43,6 @@
 import oracle.jdbc.spi.OracleResourceProvider;
 
 import java.util.function.BiConsumer;
-import java.util.function.Consumer;
 import java.util.function.Function;
 
 /**
@@ -71,11 +70,6 @@ public final class ResourceParameter
    */
   private final boolean isRequired;
 
-  /**
-   * Configures a {@code ParameterSetBuilder} with a default value, if any.
-   */
-  private final Consumer<ParameterSetBuilder> defaultValueSetter;
-
   /**
    * Configures a {@code ParameterSetBuilder} with a value parsed from a
    * {@code String}.
@@ -111,10 +105,12 @@ public ResourceParameter(
     this(name, defaultValue,
       providerParameter.isRequired(),
       providerParameter.isSensitive(),
-      defaultValue == null
-        ? builder -> { }
-        : builder -> builder.add(name, providerParameter, defaultValue),
-      (value, builder) -> builder.add(name, providerParameter, value));
+      (value, builder) -> {
+        if (value != null)
+          builder.add(name, providerParameter, value);
+        else
+          builder.add(name, providerParameter, defaultValue);
+      });
   }
 
   /**
@@ -136,12 +132,17 @@ public <T> ResourceParameter(
     this(name, defaultValue,
       providerParameter.isRequired(),
       providerParameter.isSensitive(),
-      defaultValue == null
-        ? builder -> { }
-        : builder ->
-             builder.add(name, providerParameter, parser.apply(defaultValue)),
-      (value, builder) ->
-        builder.add(name, providerParameter, parser.apply(value)));
+      (value, builder) -> {
+        if (value != null) 
+          builder.add(name, providerParameter, parser.apply(value));
+        else
+          // If no value is set, set default value or null. Default value needs
+          // to be parsed, null should not be parsed.
+          if (defaultValue != null) 
+            builder.add(name, providerParameter, parser.apply(defaultValue));
+          else 
+            builder.add(name, providerParameter, null);
+      });
   }
 
   /**
@@ -157,20 +158,16 @@ public <T> ResourceParameter(
    * parameter, or {@code false} if not.
    * @param isSensitive {@code true} if value of the parameter is security
    * sensitive, or {@code false} if not.
-   * @param defaultValueSetter Configures the default value of the parameter,
-   * if any.
    * @param parser Parses the value of the parameter. Not null.
    */
   public ResourceParameter(
     String name, String defaultValue, boolean isRequired,
     boolean isSensitive,
-    Consumer<ParameterSetBuilder> defaultValueSetter,
     BiConsumer<String, ParameterSetBuilder> parser) {
     this.name = name;
     this.defaultValue = defaultValue;
     this.isRequired = isRequired;
     this.isSensitive = isSensitive;
-    this.defaultValueSetter = defaultValueSetter;
     this.parser = parser;
   }
 
@@ -201,7 +198,6 @@ public CharSequence defaultValue() {
   void configureParser(ParameterSetParser.Builder builder) {
     builder.addParameter(
       name,
-      defaultValueSetter,
       parser);
   }
 

ojdbc-provider-oci/src/test/java/oracle/jdbc/provider/oci/resource/DatabaseConnectionStringProviderTest.java

@@ -47,10 +47,11 @@
 import java.util.HashMap;
 import java.util.Map;
 
-import static oracle.jdbc.provider.TestProperties.getOrAbort;
 import static oracle.jdbc.provider.resource.ResourceProviderTestUtil.createParameterValues;
 import static oracle.jdbc.provider.resource.ResourceProviderTestUtil.findProvider;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 /** Verifies {@link DatabaseConnectionStringProvider} */
 public class DatabaseConnectionStringProviderTest {
@@ -77,6 +78,20 @@ public void test() {
     assertNotNull(descriptor);
   }
 
+  @Test
+  public void missingOCIDTest() {
+    Map<String, CharSequence> testParameters = new HashMap<>();
+    testParameters.put("authenticationMethod", "config-file");
+
+    Map<Parameter, CharSequence> parameterValues =
+      createParameterValues(PROVIDER, testParameters);
+    IllegalStateException illegalStateException = assertThrows(IllegalStateException.class, () -> {
+      PROVIDER.getConnectionString(parameterValues);
+    });
+    assertEquals("No value defined for parameter \"ocid\"", 
+    illegalStateException.getCause().getMessage(), "Wrong error message");
+  }
+
   @Test
   public void testWithConsumerGroup() {
     Map<String, CharSequence> testParameters = new HashMap<>();