This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git
The following commit(s) were added to refs/heads/main by this push:
new 0e4651c3f fix(c/driver_manager): do not parse driver/URI if both are
set (#3790)
0e4651c3f is described below
commit 0e4651c3f35f4049cebca6a860b4cad61bd1d4cd
Author: David Li <[email protected]>
AuthorDate: Sun Dec 14 11:16:24 2025 +0900
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.
---
c/driver_manager/adbc_driver_manager.cc | 63 +++++++++++++---------------
c/driver_manager/adbc_driver_manager_test.cc | 38 ++++++++++++++++-
go/adbc/drivermgr/adbc_driver_manager.cc | 63 +++++++++++++---------------
3 files changed, 93 insertions(+), 71 deletions(-)
diff --git a/c/driver_manager/adbc_driver_manager.cc
b/c/driver_manager/adbc_driver_manager.cc
index b2fdb173f..cc3b07631 100644
--- a/c/driver_manager/adbc_driver_manager.cc
+++ b/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;
diff --git a/c/driver_manager/adbc_driver_manager_test.cc
b/c/driver_manager/adbc_driver_manager_test.cc
index e52e717f1..e91e41278 100644
--- a/c/driver_manager/adbc_driver_manager_test.cc
+++ b/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
diff --git a/go/adbc/drivermgr/adbc_driver_manager.cc
b/go/adbc/drivermgr/adbc_driver_manager.cc
index b2fdb173f..cc3b07631 100644
--- a/go/adbc/drivermgr/adbc_driver_manager.cc
+++ b/go/adbc/drivermgr/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;
