fix(c/driver_manager): do not parse driver/URI if both are set (#3790)

This fixes a regression in behavior. We only want to activate the new
behavior if only one of driver or URI is set.

Author: lidavidm

c/driver_manager/adbc_driver_manager.cc

@@ -68,7 +68,7 @@ struct ParseDriverUriResult {
 };
 
 ADBC_EXPORT
-std::optional<ParseDriverUriResult> InternalAdbcParseDriverUri(std::string_view& str);
+std::optional<ParseDriverUriResult> InternalAdbcParseDriverUri(std::string_view str);
 
 namespace {
 
@@ -1549,7 +1549,7 @@ std::string InternalAdbcDriverManagerDefaultEntrypoint(const std::string& driver
 }
 
 ADBC_EXPORT
-std::optional<ParseDriverUriResult> InternalAdbcParseDriverUri(std::string_view& str) {
+std::optional<ParseDriverUriResult> InternalAdbcParseDriverUri(std::string_view str) {
   std::string::size_type pos = str.find(":");
   if (pos == std::string::npos) {
     return std::nullopt;
@@ -1725,36 +1725,9 @@ AdbcStatusCode AdbcDatabaseSetOption(struct AdbcDatabase* database, const char*
 
   TempDatabase* args = reinterpret_cast<TempDatabase*>(database->private_data);
   if (std::strcmp(key, "driver") == 0) {
-    std::string_view v{value};
-    auto result = InternalAdbcParseDriverUri(v);
-    if (!result) {
-      args->driver = std::string{v};
-    } else {
-      args->driver = std::string{result->driver};
-      if (result->uri) {
-        args->options["uri"] = std::string{*result->uri};
-      }
-    }
+    args->driver = value;
   } else if (std::strcmp(key, "entrypoint") == 0) {
     args->entrypoint = value;
-  } else if (std::strcmp(key, "uri") == 0) {
-    if (!args->driver.empty()) {  // if driver is already set, just set uri
-      args->options[key] = value;
-    } else {
-      std::string_view v{value};
-      auto result = InternalAdbcParseDriverUri(v);
-      if (!result) {
-        SetError(error, "Invalid URI: missing scheme");
-        return ADBC_STATUS_INVALID_ARGUMENT;
-      }
-
-      args->driver = std::string{result->driver};
-      if (!result->uri) {
-        SetError(error, "Invalid URI: " + std::string{value});
-        return ADBC_STATUS_INVALID_ARGUMENT;
-      }
-      args->options["uri"] = std::string{*result->uri};
-    }
   } else {
     args->options[key] = value;
   }
@@ -1847,11 +1820,31 @@ AdbcStatusCode AdbcDatabaseInit(struct AdbcDatabase* database, struct AdbcError*
     return ADBC_STATUS_INVALID_STATE;
   }
   TempDatabase* args = reinterpret_cast<TempDatabase*>(database->private_data);
-  if (args->init_func) {
-    // Do nothing
-  } else if (args->driver.empty()) {
-    SetError(error, "Must provide 'driver' parameter");
-    return ADBC_STATUS_INVALID_ARGUMENT;
+  if (!args->init_func) {
+    const auto uri = args->options.find("uri");
+    if (args->driver.empty() && uri != args->options.end()) {
+      std::string owned_uri = uri->second;
+      auto result = InternalAdbcParseDriverUri(owned_uri);
+      if (result && result->uri) {
+        args->driver = std::string{result->driver};
+        args->options["uri"] = std::string{*result->uri};
+      }
+    } else if (!args->driver.empty() && uri == args->options.end()) {
+      std::string owned_driver = args->driver;
+      auto result = InternalAdbcParseDriverUri(owned_driver);
+      if (result) {
+        args->driver = std::string{result->driver};
+        if (result->uri) {
+          args->options["uri"] = std::string{*result->uri};
+        }
+      }
+    }
+
+    if (args->driver.empty()) {
+      SetError(error,
+               "Must provide 'driver' parameter (or encode driver in 'uri' parameter)");
+      return ADBC_STATUS_INVALID_ARGUMENT;
+    }
   }
 
   database->private_driver = new AdbcDriver;

c/driver_manager/adbc_driver_manager_test.cc

@@ -45,7 +45,7 @@ struct ParseDriverUriResult {
   std::optional<std::string_view> uri;
 };
 
-std::optional<ParseDriverUriResult> InternalAdbcParseDriverUri(std::string_view& str);
+std::optional<ParseDriverUriResult> InternalAdbcParseDriverUri(std::string_view str);
 
 // Tests of the SQLite example driver, except using the driver manager
 
@@ -1190,4 +1190,40 @@ shared = "adbc_driver_sqlite")";
   ASSERT_TRUE(std::filesystem::remove(filepath));
 }
 
+TEST_F(DriverManifest, DriverFromDriverAndUri) {
+  // Regression test: if we set both driver and URI, then we shouldn't try to
+  // extract driver from URI or vice versa.
+
+  auto filepath = temp_dir / "sqlite.toml";
+  std::ofstream test_manifest_file(filepath);
+  ASSERT_TRUE(test_manifest_file.is_open());
+  test_manifest_file << R"([Driver]
+shared = "adbc_driver_sqlite")";
+  test_manifest_file.close();
+
+  const std::string uri = "foo.db";
+  std::vector<std::vector<std::pair<std::string, std::string>>> options_cases = {
+      {{"driver", "sqlite"}, {"uri", uri}},
+      {{"uri", uri}, {"driver", "sqlite"}},
+  };
+  for (const auto& options : options_cases) {
+    adbc_validation::Handle<struct AdbcDatabase> database;
+    ASSERT_THAT(AdbcDatabaseNew(&database.value, &error), IsOkStatus(&error));
+    for (const auto& option : options) {
+      ASSERT_THAT(AdbcDatabaseSetOption(&database.value, option.first.c_str(),
+                                        option.second.c_str(), &error),
+                  IsOkStatus(&error));
+    }
+
+    std::string search_path = temp_dir.string();
+    ASSERT_THAT(AdbcDriverManagerDatabaseSetAdditionalSearchPathList(
+                    &database.value, search_path.data(), &error),
+                IsOkStatus(&error));
+    ASSERT_THAT(AdbcDatabaseInit(&database.value, &error),
+                IsStatus(ADBC_STATUS_OK, &error));
+  }
+
+  ASSERT_TRUE(std::filesystem::remove(filepath));
+}
+
 }  // namespace adbc