Handle empty protocol option

Author: Michael-A-McMahon

src/main/java/oracle/r2dbc/impl/OracleReactiveJdbcAdapter.java

@@ -346,17 +346,6 @@ public DataSource createDataSource(ConnectionFactoryOptions options) {
    * specified in the javadoc of
    * {@link #createDataSource(ConnectionFactoryOptions)}
    * </p><p>
-   * If the {@link ConnectionFactoryOptions#SSL} option is set, then the JDBC
-   * URL is composed with the tcps protocol, as in:
-   * {@code jdbc:oracle:thins:@tcps:...}. The {@code SSL} option is interpreted
-   * as a strict directive to use TLS, and so it takes precedence over any value
-   * that may otherwise be specified by the {@code PROTOCOL} option.
-   * </p><p>
-   * If the {@code SSL} option is not set, then the URL is composed with any
-   * value set for {@link ConnectionFactoryOptions#PROTOCOL} option. For
-   * instance, if the {@code PROTOCOL} option is set to "ldap" then the URL
-   * is composed as: {@code jdbc:oracle:thin:@ldap://...}.
-   * </p><p>
    * For consistency with the Oracle JDBC URL, an Oracle R2DBC URL might include
    * multiple space separated LDAP addresses, where the space is percent encoded,
    * like this:
@@ -389,30 +378,77 @@ private static String composeJdbcUrl(ConnectionFactoryOptions options) {
       validateDescriptorOptions(options);
       return "jdbc:oracle:thin:@" + descriptor;
     }
-    else {
-      Object protocol =
-        Boolean.TRUE.equals(parseOptionValue(
-          SSL, options, Boolean.class, Boolean::valueOf))
-          ? "tcps"
-          : options.getValue(PROTOCOL);
-      Object host = options.getRequiredValue(HOST);
-      Integer port = parseOptionValue(
-        PORT, options, Integer.class, Integer::valueOf);
-      Object serviceName = options.getValue(DATABASE);
-
-      Object dnMatch =
-        options.getValue(OracleR2dbcOptions.TLS_SERVER_DN_MATCH);
-
-      return String.format("jdbc:oracle:thin:@%s%s%s%s?%s=%s",
-        protocol == null ? "" : protocol + "://",
-        host,
-        port != null ? (":" + port) : "",
-        serviceName != null ? ("/" + serviceName) : "",
-        // Workaround for Oracle JDBC bug #33150409. DN matching is enabled
-        // unless the property is set as a query parameter.
-        OracleR2dbcOptions.TLS_SERVER_DN_MATCH.name(),
-        dnMatch == null ? "false" : dnMatch);
-    }
+
+    String protocol = composeJdbcProtocol(options);
+
+    Object host = options.getRequiredValue(HOST);
+
+    Integer port =
+      parseOptionValue(PORT, options, Integer.class, Integer::valueOf);
+
+    Object serviceName = options.getValue(DATABASE);
+
+    Object dnMatch =
+      options.getValue(OracleR2dbcOptions.TLS_SERVER_DN_MATCH);
+
+    return String.format("jdbc:oracle:thin:@%s%s%s%s?%s=%s",
+      protocol,
+      host,
+      port != null ? (":" + port) : "",
+      serviceName != null ? ("/" + serviceName) : "",
+      // Workaround for Oracle JDBC bug #33150409. DN matching is enabled
+      // unless the property is set as a query parameter.
+      OracleR2dbcOptions.TLS_SERVER_DN_MATCH.name(),
+      dnMatch == null ? "false" : dnMatch);
+  }
+
+  /**
+   * <p>
+   * Composes the protocol section of an Oracle JDBC URL. This is an optional
+   * section that may appear after the '@' symbol. For instance, the follow URL
+   * would specify the "tcps" protocol:
+   * </p><p>
+   * <pre>
+   *   jdbc:oracle:thin:@tcps://...
+   * </pre>
+   * </p><p>
+   * If {@link ConnectionFactoryOptions#SSL} is set, then "tcps://" is returned.
+   * The {@code SSL} option is interpreted as a strict directive to use TLS, and
+   * so it takes precedence over any value that may be specified with the
+   * {@link ConnectionFactoryOptions#PROTOCOL} option.
+   * </p><p>
+   * Otherwise, if the {@code SSL} option is not set, then the protocol section
+   * is composed with any value set to the
+   * {@link ConnectionFactoryOptions#PROTOCOL} option. For
+   * instance, if the {@code PROTOCOL} option is set to "ldap" then the URL
+   * is composed as: {@code jdbc:oracle:thin:@ldap://...}.
+   * </p><p>
+   * If the {@code PROTOCOL} option is set to an empty string, this is
+   * considered equivalent to not setting the option at all. The R2DBC Pool
+   * library is known to set an empty string as the protocol .
+   * </p>
+   * @param options Options that may or may not specify a protocol. Not null.
+   * @return The specified protocol, or an empty string if none is specified.
+   */
+  private static String composeJdbcProtocol(ConnectionFactoryOptions options) {
+
+    Boolean isSSL =
+      parseOptionValue(SSL, options, Boolean.class, Boolean::valueOf);
+
+    if (Boolean.TRUE.equals(isSSL))
+      return "tcps://";
+
+    Object protocolObject = options.getValue(PROTOCOL);
+
+    if (protocolObject == null)
+      return "";
+
+    String protocol = protocolObject.toString();
+
+    if (protocol.isEmpty())
+      return "";
+
+    return protocol + "://";
   }
 
   /**

src/test/java/oracle/r2dbc/impl/OracleConnectionFactoryImplTest.java

@@ -189,4 +189,5 @@ public void testGetMetadata() {
         .getMetadata()
         .getName());
   }
+
 }

src/test/java/oracle/r2dbc/impl/OracleReactiveJdbcAdapterTest.java

@@ -28,15 +28,18 @@
 import io.r2dbc.spi.Option;
 import io.r2dbc.spi.R2dbcTimeoutException;
 import io.r2dbc.spi.Result;
+import io.r2dbc.spi.Statement;
 import oracle.jdbc.OracleConnection;
 import oracle.jdbc.datasource.OracleDataSource;
 import oracle.r2dbc.OracleR2dbcOptions;
 import oracle.r2dbc.test.DatabaseConfig;
 import oracle.r2dbc.util.TestContextFactory;
+import org.junit.jupiter.api.Assumptions;
 import org.junit.jupiter.api.Test;
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
 
+import javax.xml.crypto.Data;
 import java.io.IOException;
 import java.nio.channels.ServerSocketChannel;
 import java.nio.channels.SocketChannel;
@@ -46,6 +49,7 @@
 import java.sql.SQLException;
 import java.time.Duration;
 import java.time.ZonedDateTime;
+import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Properties;
@@ -68,6 +72,7 @@
 import static io.r2dbc.spi.ConnectionFactoryOptions.HOST;
 import static io.r2dbc.spi.ConnectionFactoryOptions.PASSWORD;
 import static io.r2dbc.spi.ConnectionFactoryOptions.PORT;
+import static io.r2dbc.spi.ConnectionFactoryOptions.PROTOCOL;
 import static io.r2dbc.spi.ConnectionFactoryOptions.STATEMENT_TIMEOUT;
 import static io.r2dbc.spi.ConnectionFactoryOptions.USER;
 import static java.lang.String.format;
@@ -85,6 +90,7 @@
 import static oracle.r2dbc.util.Awaits.awaitExecution;
 import static oracle.r2dbc.util.Awaits.awaitNone;
 import static oracle.r2dbc.util.Awaits.awaitOne;
+import static oracle.r2dbc.util.Awaits.awaitQuery;
 import static oracle.r2dbc.util.Awaits.awaitUpdate;
 import static oracle.r2dbc.util.Awaits.tryAwaitExecution;
 import static oracle.r2dbc.util.Awaits.tryAwaitNone;
@@ -603,20 +609,6 @@ public void testMultiLdapUrl() throws Exception {
     }
   }
 
-  /**
-   * Returns an Oracle Net Descriptor having the values configured by
-   * {@link DatabaseConfig}
-   * @return An Oracle Net Descriptor for the test database.
-   */
-  private static String createDescriptor() {
-    return format(
-      "(DESCRIPTION=(ADDRESS=(HOST=%s)(PORT=%d)(PROTOCOL=%s))" +
-        "(CONNECT_DATA=(SERVICE_NAME=%s)))",
-      host(), port(),
-      Objects.requireNonNullElse(protocol(), "tcp"),
-      serviceName());
-  }
-
   /**
    * Verifies the {@link OracleR2dbcOptions#TIMEZONE_AS_REGION} option
    */
@@ -665,6 +657,60 @@ public void testTimezoneAsRegion() {
     }
   }
 
+
+  /**
+   * Verifies behavior when {@link ConnectionFactoryOptions#PROTOCOL} is set
+   * to an empty string. In this case, the driver is expected to behave as if
+   * no protocol were specified.
+   */
+  @Test
+  public void testEmptyProtocol() {
+    Assumptions.assumeTrue(
+      DatabaseConfig.protocol() == null,
+      "Test requires no PROTOCOL in config.properties");
+
+    ConnectionFactoryOptions.Builder optionsBuilder =
+      ConnectionFactoryOptions.builder()
+        .option(PROTOCOL, "")
+        .option(DRIVER, "oracle")
+        .option(HOST, DatabaseConfig.host())
+        .option(PORT, DatabaseConfig.port())
+        .option(DATABASE, DatabaseConfig.serviceName())
+        .option(USER, DatabaseConfig.user())
+        .option(PASSWORD, DatabaseConfig.password());
+
+    Duration timeout = DatabaseConfig.connectTimeout();
+    if (timeout != null)
+      optionsBuilder.option(CONNECT_TIMEOUT, timeout);
+
+    ConnectionFactoryOptions options = optionsBuilder.build();
+
+    Connection connection = awaitOne(ConnectionFactories.get(options).create());
+    try {
+      Statement statement =
+        connection.createStatement("SELECT 1 FROM sys.dual");
+
+      awaitQuery(List.of(1), row -> row.get(0, Integer.class), statement);
+    }
+    finally {
+      tryAwaitNone(connection.close());
+    }
+  }
+
+  /**
+   * Returns an Oracle Net Descriptor having the values configured by
+   * {@link DatabaseConfig}
+   * @return An Oracle Net Descriptor for the test database.
+   */
+  private static String createDescriptor() {
+    return format(
+      "(DESCRIPTION=(ADDRESS=(HOST=%s)(PORT=%d)(PROTOCOL=%s))" +
+        "(CONNECT_DATA=(SERVICE_NAME=%s)))",
+      host(), port(),
+      Objects.requireNonNullElse(protocol(), "tcp"),
+      serviceName());
+  }
+
   /**
    * Verifies that an attempt to connect with a {@code listeningChannel}
    * results in an {@link R2dbcTimeoutException}.