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 6ce20ab8a feat(c/driver_manager,rust/driver_manager): add manifest version check (#3393) 6ce20ab8a is described below commit 6ce20ab8afdaf73fee1e327fb6df9b082be0ba1f Author: Matt Topol <zotthewiz...@gmail.com> AuthorDate: Fri Sep 5 01:20:22 2025 -0400 feat(c/driver_manager,rust/driver_manager): add manifest version check (#3393) resolves #3387 --------- Co-authored-by: Ian Cook <ianmc...@gmail.com> Co-authored-by: David Li <li.david...@gmail.com> --- c/driver_manager/adbc_driver_manager.cc | 29 ++ c/driver_manager/adbc_driver_manager_test.cc | 18 ++ .../cpp/recipe_driver/driver_example.toml.in | 2 + docs/source/format/driver_manifests.rst | 10 +- docs/source/format/versioning.rst | 12 + go/adbc/drivermgr/adbc_driver_manager.cc | 306 +++++++++++++++++---- .../tests/testthat/test-driver_void.R | 2 + rust/driver_manager/src/lib.rs | 47 ++++ 8 files changed, 368 insertions(+), 58 deletions(-) diff --git a/c/driver_manager/adbc_driver_manager.cc b/c/driver_manager/adbc_driver_manager.cc index 7b49df98e..ed8946e52 100644 --- a/c/driver_manager/adbc_driver_manager.cc +++ b/c/driver_manager/adbc_driver_manager.cc @@ -218,6 +218,7 @@ using char_type = char; /// \brief The location and entrypoint of a resolved driver. struct DriverInfo { std::string manifest_file; + int64_t manifest_version; std::string driver_name; std::filesystem::path lib_path; std::string entrypoint; @@ -262,6 +263,19 @@ class RegistryKey { return value; } + int32_t GetInt(const std::wstring& name, const int32_t default_value) { + if (!is_open()) return default_value; + + DWORD dwValue; + DWORD dataSize = sizeof(dwValue); + DWORD valueType; + auto result = RegQueryValueExW(key_, name.data(), nullptr, &valueType, + (LPBYTE)&dwValue, &dataSize); + if (result != ERROR_SUCCESS) return default_value; + if (valueType != REG_DWORD) return default_value; + return static_cast<int32_t>(dwValue); + } + private: HKEY root_; HKEY key_; @@ -291,6 +305,13 @@ AdbcStatusCode LoadDriverFromRegistry(HKEY root, const std::wstring& driver_name } info.driver_name = Utf8Encode(dkey.GetString(L"name", L"")); + info.manifest_version = int64_t(dkey.GetInt(L"manifest_version", 1)); + if (info.manifest_version != 1) { + SetError(error, "Driver manifest version '" + std::to_string(info.manifest_version) + + "' is not supported by this driver manager."); + return ADBC_STATUS_INVALID_ARGUMENT; + } + info.entrypoint = Utf8Encode(dkey.GetString(L"entrypoint", L"")); info.version = Utf8Encode(dkey.GetString(L"version", L"")); info.source = Utf8Encode(dkey.GetString(L"source", L"")); @@ -318,6 +339,14 @@ AdbcStatusCode LoadDriverManifest(const std::filesystem::path& driver_manifest, info.manifest_file = driver_manifest.string(); info.driver_name = config["name"].value_or(""s); + info.manifest_version = config["manifest_version"].value_or(int64_t(1)); + if (info.manifest_version != 1) { + SetError(error, "Driver manifest version '" + std::to_string(info.manifest_version) + + "' is not supported by this driver manager."); + return ADBC_STATUS_INVALID_ARGUMENT; + } + + info.entrypoint = config.at_path("Driver.entrypoint").value_or(""s); info.version = config["version"].value_or(""s); info.source = config["source"].value_or(""s); diff --git a/c/driver_manager/adbc_driver_manager_test.cc b/c/driver_manager/adbc_driver_manager_test.cc index 2b1c3c29c..b17ec236d 100644 --- a/c/driver_manager/adbc_driver_manager_test.cc +++ b/c/driver_manager/adbc_driver_manager_test.cc @@ -788,6 +788,24 @@ TEST_F(DriverManifest, ManifestEntrypointInvalid) { ASSERT_TRUE(std::filesystem::remove(filepath)); } +TEST_F(DriverManifest, ManifestBadVersion) { + auto filepath = temp_dir / "sqlite.toml"; + toml::table manifest_with_bad_version = simple_manifest; + manifest_with_bad_version.insert("manifest_version", 2); + + std::ofstream test_manifest_file(filepath); + ASSERT_TRUE(test_manifest_file.is_open()); + test_manifest_file << manifest_with_bad_version; + test_manifest_file.close(); + + // Attempt to load the driver + ASSERT_THAT(AdbcFindLoadDriver(filepath.string().data(), nullptr, ADBC_VERSION_1_1_0, + ADBC_LOAD_FLAG_DEFAULT, nullptr, &driver, &error), + IsStatus(ADBC_STATUS_INVALID_ARGUMENT, &error)); + + ASSERT_TRUE(std::filesystem::remove(filepath)); +} + // only build and run test that puts files in the users home directory if // it's been enabled via the build system setting this compile def #ifdef ADBC_DRIVER_MANAGER_TEST_MANIFEST_USER_LEVEL diff --git a/docs/source/cpp/recipe_driver/driver_example.toml.in b/docs/source/cpp/recipe_driver/driver_example.toml.in index 724fbbbcf..9c1f5a404 100644 --- a/docs/source/cpp/recipe_driver/driver_example.toml.in +++ b/docs/source/cpp/recipe_driver/driver_example.toml.in @@ -15,6 +15,8 @@ # specific language governing permissions and limitations # under the License. +manifest_version = 1 + name = 'Driver Example' publisher = 'arrow-adbc-docs' license = 'Apache-2.0' diff --git a/docs/source/format/driver_manifests.rst b/docs/source/format/driver_manifests.rst index 9c26e0ea2..f2d46137a 100644 --- a/docs/source/format/driver_manifests.rst +++ b/docs/source/format/driver_manifests.rst @@ -174,7 +174,7 @@ file path to the dynamic library as the driver name. use adbc_core::driver_manager::ManagedDriver; fn get_driver() -> ManagedDriver { - ManagedDriver::load_dynamic_from_name("/path/to/libadbc_driver.so", None, AdbcVersion::V100).unwrap() + ManagedDriver::load_from_name("/path/to/libadbc_driver.so", None, AdbcVersion::V100).unwrap() } As an alternative to passing the full path to the dynamic library, you may @@ -208,6 +208,8 @@ Below is an example of a driver manifest: .. code-block:: toml + manifest_version = 1 + name = 'Driver Display Name' version = '1.0.0' # driver version publisher = 'string to identify the publisher' @@ -241,6 +243,10 @@ a string (single path) or a table of platform-specific paths. The ``Driver.shar needed to successfully load a driver manifest. The other keys are optional, but provide useful metadata about the driver. +The ``manifest_version`` key, if present, it must be set to 1. It defaults to 1 and can currently only +be set to 1. Driver manager implementations must error upon reading a manifest with +``manifest_version`` higher than 1. + Platform Tuples ^^^^^^^^^^^^^^^ @@ -422,7 +428,7 @@ to control which directories will be searched for manifests, with the behavior b .. tab-item:: Rust :sync: rust - The ``ManagedDriver`` type has a method ``load_dynamic_from_name`` which takes an optional ``load_flags`` parameter. The flags as a ``u32`` with + The ``ManagedDriver`` type has a method ``load_from_name`` which takes an optional ``load_flags`` parameter. The flags as a ``u32`` with the type ``adbc_core::driver_manager::LoadFlags``, which has the following constants: * ``LOAD_FLAG_SEARCH_ENV`` - search the directory paths in the environment variable diff --git a/docs/source/format/versioning.rst b/docs/source/format/versioning.rst index 6a4e689cf..7b7f84548 100644 --- a/docs/source/format/versioning.rst +++ b/docs/source/format/versioning.rst @@ -74,3 +74,15 @@ Similarly, this documentation describes the ADBC API standard version options or API functions are defined), the next version would be 1.2.0. If incompatible changes are made (e.g. changing the signature or semantics of a function), the next version would be 2.0.0. + +The ADBC :doc:`driver manifest <driver_manifests>` TOML format is +versioned separately from the ADBC standard and components. Its +version is an integer currently set to 1. This version number may +optionally be included in the manifest as the value of the +``manifest_version`` key. If present, it must be set to 1. If not +present, it is assumed to be 1. The manifest version number must be +incremented when and only when breaking changes are made to the +driver manifest format. In future manifests with a version higher +than 1, the ``manifest_version`` key will be required. Current +driver manager implementations must error upon reading a manifest +with ``manifest_version`` higher than 1. diff --git a/go/adbc/drivermgr/adbc_driver_manager.cc b/go/adbc/drivermgr/adbc_driver_manager.cc index e8ef6f01a..ed8946e52 100644 --- a/go/adbc/drivermgr/adbc_driver_manager.cc +++ b/go/adbc/drivermgr/adbc_driver_manager.cc @@ -58,6 +58,60 @@ std::filesystem::path InternalAdbcUserConfigDir(); namespace { +/// \brief Where a search path came from (for error reporting) +enum class SearchPathSource { + kEnv, + kUser, + kRegistry, + kSystem, + kAdditional, + kConda, + kUnset, + kDoesNotExist, + kDisabled, +}; + +using SearchPaths = std::vector<std::pair<SearchPathSource, std::filesystem::path>>; + +void AddSearchPathsToError(const SearchPaths& search_paths, std::string& error_message) { + if (!search_paths.empty()) { + error_message += "\nAlso searched these paths for manifests:"; + for (const auto& [source, path] : search_paths) { + error_message += "\n\t"; + switch (source) { + case SearchPathSource::kEnv: + error_message += "ADBC_DRIVER_PATH: "; + break; + case SearchPathSource::kUser: + error_message += "user config dir: "; + break; + case SearchPathSource::kRegistry: + error_message += "Registry: "; + break; + case SearchPathSource::kSystem: + error_message += "system config dir: "; + break; + case SearchPathSource::kAdditional: + error_message += "additional search path: "; + break; + case SearchPathSource::kConda: + error_message += "Conda prefix: "; + break; + case SearchPathSource::kUnset: + error_message += "not set: "; + break; + case SearchPathSource::kDoesNotExist: + error_message += "does not exist: "; + break; + case SearchPathSource::kDisabled: + error_message += "not enabled at build time: "; + break; + } + error_message += path.string(); + } + } +} + // Platform-specific helpers #if defined(_WIN32) @@ -161,9 +215,10 @@ std::wstring Utf8Decode(const std::string& str) { using char_type = char; #endif // _WIN32 -// Driver state +/// \brief The location and entrypoint of a resolved driver. struct DriverInfo { std::string manifest_file; + int64_t manifest_version; std::string driver_name; std::filesystem::path lib_path; std::string entrypoint; @@ -208,6 +263,19 @@ class RegistryKey { return value; } + int32_t GetInt(const std::wstring& name, const int32_t default_value) { + if (!is_open()) return default_value; + + DWORD dwValue; + DWORD dataSize = sizeof(dwValue); + DWORD valueType; + auto result = RegQueryValueExW(key_, name.data(), nullptr, &valueType, + (LPBYTE)&dwValue, &dataSize); + if (result != ERROR_SUCCESS) return default_value; + if (valueType != REG_DWORD) return default_value; + return static_cast<int32_t>(dwValue); + } + private: HKEY root_; HKEY key_; @@ -216,25 +284,43 @@ class RegistryKey { AdbcStatusCode LoadDriverFromRegistry(HKEY root, const std::wstring& driver_name, DriverInfo& info, struct AdbcError* error) { + // N.B. start all error messages with the subkey so that the calling code + // can prepend the name of 'root' to the error message (easier than trying + // to invoke win32 API to get the name of the HKEY) static const LPCWSTR kAdbcDriverRegistry = L"SOFTWARE\\ADBC\\Drivers"; RegistryKey drivers_key(root, kAdbcDriverRegistry); if (!drivers_key.is_open()) { + std::string error_message = "SOFTWARE\\ADBC\\DRIVERS not found"s; + SetError(error, std::move(error_message)); return ADBC_STATUS_NOT_FOUND; } RegistryKey dkey(drivers_key.key(), driver_name); if (!dkey.is_open()) { + std::string error_message = "SOFTWARE\\ADBC\\DRIVERS has no entry for driver \""s; + error_message += Utf8Encode(driver_name); + error_message += "\""s; + SetError(error, std::move(error_message)); return ADBC_STATUS_NOT_FOUND; } info.driver_name = Utf8Encode(dkey.GetString(L"name", L"")); + info.manifest_version = int64_t(dkey.GetInt(L"manifest_version", 1)); + if (info.manifest_version != 1) { + SetError(error, "Driver manifest version '" + std::to_string(info.manifest_version) + + "' is not supported by this driver manager."); + return ADBC_STATUS_INVALID_ARGUMENT; + } + info.entrypoint = Utf8Encode(dkey.GetString(L"entrypoint", L"")); info.version = Utf8Encode(dkey.GetString(L"version", L"")); info.source = Utf8Encode(dkey.GetString(L"source", L"")); info.lib_path = std::filesystem::path(dkey.GetString(L"driver", L"")); if (info.lib_path.empty()) { - SetError(error, "Driver path not found in registry for \""s + - Utf8Encode(driver_name) + "\""); + std::string error_message = "SOFTWARE\\ADBC\\DRIVERS\\"s; + error_message += Utf8Encode(driver_name); + error_message += " has no driver path"s; + SetError(error, std::move(error_message)); return ADBC_STATUS_NOT_FOUND; } return ADBC_STATUS_OK; @@ -253,27 +339,81 @@ AdbcStatusCode LoadDriverManifest(const std::filesystem::path& driver_manifest, info.manifest_file = driver_manifest.string(); info.driver_name = config["name"].value_or(""s); + info.manifest_version = config["manifest_version"].value_or(int64_t(1)); + if (info.manifest_version != 1) { + SetError(error, "Driver manifest version '" + std::to_string(info.manifest_version) + + "' is not supported by this driver manager."); + return ADBC_STATUS_INVALID_ARGUMENT; + } + info.entrypoint = config.at_path("Driver.entrypoint").value_or(""s); info.version = config["version"].value_or(""s); info.source = config["source"].value_or(""s); + auto entrypoint = config.at_path("Driver.entrypoint"); + if (entrypoint) { + if (auto* ep = entrypoint.as_string()) { + info.entrypoint = ep->get(); + } else { + SetError(error, "Driver entrypoint not a string in manifest '"s + + driver_manifest.string() + "'"s); + return ADBC_STATUS_INVALID_ARGUMENT; + } + } + auto driver = config.at_path("Driver.shared"); if (toml::table* platforms = driver.as_table()) { - info.lib_path = platforms->at_path(adbc::CurrentArch()).value_or(""s); + auto view = platforms->at_path(adbc::CurrentArch()); + if (!view) { + std::string message = "Driver path not found in manifest '"; + message += driver_manifest.string(); + message += "' for current architecture '"; + message += adbc::CurrentArch(); + message += "'. Architectures found:"; + for (const auto& [key, val] : *platforms) { + message += " "; + message += key; + } + SetError(error, std::move(message)); + return ADBC_STATUS_NOT_FOUND; + } else if (auto* path = view.as_string()) { + if (path->get().empty()) { + std::string message = "Driver path is an empty string in manifest '"; + message += driver_manifest.string(); + message += "' for current architecture '"; + message += adbc::CurrentArch(); + message += "'"; + SetError(error, std::move(message)); + return ADBC_STATUS_INVALID_ARGUMENT; + } + + info.lib_path = path->get(); + return ADBC_STATUS_OK; + } else { + std::string message = "Driver path not found in manifest '"; + message += driver_manifest.string(); + message += "' for current architecture '"; + message += adbc::CurrentArch(); + message += "'. Value was not a string"; + SetError(error, std::move(message)); + return ADBC_STATUS_INVALID_ARGUMENT; + } + return ADBC_STATUS_OK; } else if (auto* path = driver.as_string()) { info.lib_path = path->get(); + if (info.lib_path.empty()) { + SetError(error, "Driver path is an empty string in manifest '"s + + driver_manifest.string() + "'"s); + return ADBC_STATUS_INVALID_ARGUMENT; + } + return ADBC_STATUS_OK; } - - if (info.lib_path.empty()) { - SetError(error, "Driver path not found in manifest '"s + driver_manifest.string() + - "' for current architecture '" + adbc::CurrentArch() + "'"); - return ADBC_STATUS_NOT_FOUND; - } - - return ADBC_STATUS_OK; + SetError(error, "Driver path not defined in manifest '"s + driver_manifest.string() + + "'. `Driver.shared` must be a string or table"s); + return ADBC_STATUS_NOT_FOUND; } -std::vector<std::filesystem::path> GetEnvPaths(const char_type* env_var) { +SearchPaths GetEnvPaths(const char_type* env_var) { #ifdef _WIN32 size_t required_size; @@ -293,26 +433,33 @@ std::vector<std::filesystem::path> GetEnvPaths(const char_type* env_var) { } std::string path(path_var); #endif // _WIN32 - return InternalAdbcParsePath(path); + SearchPaths paths; + for (auto path : InternalAdbcParsePath(path)) { + paths.emplace_back(SearchPathSource::kEnv, path); + } + return paths; } -std::vector<std::filesystem::path> GetSearchPaths(const AdbcLoadFlags levels) { - std::vector<std::filesystem::path> paths; - if (levels & ADBC_LOAD_FLAG_SEARCH_ENV) { #ifdef _WIN32 - static const wchar_t* env_var = L"ADBC_DRIVER_PATH"; +static const wchar_t* kAdbcDriverPath = L"ADBC_DRIVER_PATH"; #else - static const char* env_var = "ADBC_DRIVER_PATH"; +static const char* kAdbcDriverPath = "ADBC_DRIVER_PATH"; #endif // _WIN32 + +SearchPaths GetSearchPaths(const AdbcLoadFlags levels) { + SearchPaths paths; + if (levels & ADBC_LOAD_FLAG_SEARCH_ENV) { // Check the ADBC_DRIVER_PATH environment variable - paths = GetEnvPaths(env_var); + paths = GetEnvPaths(kAdbcDriverPath); } if (levels & ADBC_LOAD_FLAG_SEARCH_USER) { // Check the user configuration directory std::filesystem::path user_config_dir = InternalAdbcUserConfigDir(); if (!user_config_dir.empty() && std::filesystem::exists(user_config_dir)) { - paths.push_back(user_config_dir); + paths.emplace_back(SearchPathSource::kUser, std::move(user_config_dir)); + } else { + paths.emplace_back(SearchPathSource::kDoesNotExist, std::move(user_config_dir)); } } @@ -324,12 +471,16 @@ std::vector<std::filesystem::path> GetSearchPaths(const AdbcLoadFlags levels) { const std::filesystem::path system_config_dir( "/Library/Application Support/ADBC/Drivers"); if (std::filesystem::exists(system_config_dir)) { - paths.push_back(system_config_dir); + paths.emplace_back(SearchPathSource::kSystem, std::move(system_config_dir)); + } else { + paths.emplace_back(SearchPathSource::kDoesNotExist, std::move(system_config_dir)); } #elif !defined(_WIN32) const std::filesystem::path system_config_dir("/etc/adbc/drivers"); if (std::filesystem::exists(system_config_dir)) { - paths.push_back(system_config_dir); + paths.emplace_back(SearchPathSource::kSystem, std::move(system_config_dir)); + } else { + paths.emplace_back(SearchPathSource::kDoesNotExist, std::move(system_config_dir)); } #endif // defined(__APPLE__) } @@ -365,11 +516,13 @@ struct ManagedLibrary { ~ManagedLibrary() { Release(); } void Release() { - // TODO(apache/arrow-adbc#204): causes tests to segfault - // Need to refcount the driver DLL; also, errors may retain a reference to - // release() from the DLL - how to handle this? + // TODO(apache/arrow-adbc#204): causes tests to segfault. Need to + // refcount the driver DLL; also, errors may retain a reference to + // release() from the DLL - how to handle this? It's unlikely we can + // actually do this - in general shared libraries are not safe to unload. } + /// \brief Resolve the driver name to a concrete location. AdbcStatusCode GetDriverInfo( const std::string_view driver_name, const AdbcLoadFlags load_options, const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info, @@ -379,6 +532,7 @@ struct ManagedLibrary { return ADBC_STATUS_INVALID_ARGUMENT; } + // First try to treat the given driver name as a path to a manifest or shared library std::filesystem::path driver_path(driver_name); const bool allow_relative_paths = load_options & ADBC_LOAD_FLAG_ALLOW_RELATIVE_PATHS; if (driver_path.has_extension()) { @@ -393,14 +547,14 @@ struct ManagedLibrary { auto status = LoadDriverManifest(driver_path, info, error); if (status == ADBC_STATUS_OK) { - return Load(info.lib_path.c_str(), error); + return Load(info.lib_path.c_str(), {}, error); } return status; } // if the extension is not .toml, then just try to load the provided // path as if it was an absolute path to a driver library - return Load(driver_path.c_str(), error); + return Load(driver_path.c_str(), {}, error); } if (driver_path.is_absolute()) { @@ -410,14 +564,14 @@ struct ManagedLibrary { if (std::filesystem::exists(driver_path)) { auto status = LoadDriverManifest(driver_path, info, error); if (status == ADBC_STATUS_OK) { - return Load(info.lib_path.c_str(), error); + return Load(info.lib_path.c_str(), {}, error); } } driver_path.replace_extension(""); info.lib_path = driver_path; // otherwise just try to load the provided path as if it was an absolute path - return Load(driver_path.c_str(), error); + return Load(driver_path.c_str(), {}, error); } if (driver_path.has_extension()) { @@ -435,7 +589,7 @@ struct ManagedLibrary { #endif // defined(_WIN32) if (HasExtension(driver_path, kPlatformLibrarySuffix)) { info.lib_path = driver_path; - return Load(driver_path.c_str(), error); + return Load(driver_path.c_str(), {}, error); } SetError(error, "Driver name has unrecognized extension: " + @@ -448,10 +602,15 @@ struct ManagedLibrary { return FindDriver(driver_path, load_options, additional_search_paths, info, error); } - AdbcStatusCode SearchPaths(const std::filesystem::path& driver_path, - const std::vector<std::filesystem::path>& search_paths, - DriverInfo& info, struct AdbcError* error) { - for (const auto& search_path : search_paths) { + AdbcStatusCode SearchPathsForDriver(const std::filesystem::path& driver_path, + const SearchPaths& search_paths, DriverInfo& info, + struct AdbcError* error) { + for (const auto& [source, search_path] : search_paths) { + if (source == SearchPathSource::kRegistry || source == SearchPathSource::kUnset || + source == SearchPathSource::kDoesNotExist || + source == SearchPathSource::kDisabled) { + continue; + } std::filesystem::path full_path = search_path / driver_path; // check for toml first, then dll @@ -459,12 +618,12 @@ struct ManagedLibrary { if (std::filesystem::exists(full_path)) { auto status = LoadDriverManifest(full_path, info, nullptr); if (status == ADBC_STATUS_OK) { - return Load(info.lib_path.c_str(), error); + return Load(info.lib_path.c_str(), search_paths, error); } } full_path.replace_extension(""); // remove the .toml extension - auto status = Load(full_path.c_str(), nullptr); + auto status = Load(full_path.c_str(), search_paths, nullptr); if (status == ADBC_STATUS_OK) { return status; } @@ -482,12 +641,17 @@ struct ManagedLibrary { return ADBC_STATUS_INVALID_ARGUMENT; } + SearchPaths search_paths; { // First search the paths in the env var `ADBC_DRIVER_PATH`. // Then search the runtime application-defined additional search paths. - auto search_paths = GetSearchPaths(load_options & ADBC_LOAD_FLAG_SEARCH_ENV); - search_paths.insert(search_paths.end(), additional_search_paths.begin(), - additional_search_paths.end()); + search_paths = GetSearchPaths(load_options & ADBC_LOAD_FLAG_SEARCH_ENV); + if (search_paths.empty()) { + search_paths.emplace_back(SearchPathSource::kUnset, "ADBC_DRIVER_PATH"); + } + for (const auto& path : additional_search_paths) { + search_paths.emplace_back(SearchPathSource::kAdditional, path); + } #if ADBC_CONDA_BUILD // Then, if this is a conda build, search in the conda environment if @@ -501,13 +665,18 @@ struct ManagedLibrary { auto venv = GetEnvPaths(conda_name); if (!venv.empty()) { for (const auto& venv_path : venv) { - search_paths.push_back(venv_path / "etc" / "adbc" / "drivers"); + search_paths.emplace_back(SearchPathSource::kConda, + venv_path / "etc" / "adbc" / "drivers"); } } } +#else + if (load_options & ADBC_LOAD_FLAG_SEARCH_ENV) { + search_paths.emplace_back(SearchPathSource::kDisabled, "Conda prefix"); + } #endif // ADBC_CONDA_BUILD - auto status = SearchPaths(driver_path, search_paths, info, error); + auto status = SearchPathsForDriver(driver_path, search_paths, info, error); if (status == ADBC_STATUS_OK) { return status; } @@ -522,14 +691,23 @@ struct ManagedLibrary { auto status = LoadDriverFromRegistry(HKEY_CURRENT_USER, driver_path.native(), info, error); if (status == ADBC_STATUS_OK) { - return Load(info.lib_path.c_str(), error); + return Load(info.lib_path.c_str(), {}, error); + } + if (error && error->message) { + std::string message = "HKEY_CURRENT_USER\\"s; + message += error->message; + search_paths.emplace_back(SearchPathSource::kRegistry, std::move(message)); + } else { + search_paths.emplace_back(SearchPathSource::kRegistry, + "not found in HKEY_CURRENT_USER"); } - status = SearchPaths(driver_path, GetSearchPaths(ADBC_LOAD_FLAG_SEARCH_USER), info, - error); + auto user_paths = GetSearchPaths(ADBC_LOAD_FLAG_SEARCH_USER); + status = SearchPathsForDriver(driver_path, user_paths, info, error); if (status == ADBC_STATUS_OK) { return status; } + search_paths.insert(search_paths.end(), user_paths.begin(), user_paths.end()); } if (load_options & ADBC_LOAD_FLAG_SEARCH_SYSTEM) { @@ -537,33 +715,46 @@ struct ManagedLibrary { auto status = LoadDriverFromRegistry(HKEY_LOCAL_MACHINE, driver_path.native(), info, error); if (status == ADBC_STATUS_OK) { - return Load(info.lib_path.c_str(), error); + return Load(info.lib_path.c_str(), {}, error); + } + if (error && error->message) { + std::string message = "HKEY_LOCAL_MACHINE\\"s; + message += error->message; + search_paths.emplace_back(SearchPathSource::kRegistry, std::move(message)); + } else { + search_paths.emplace_back(SearchPathSource::kRegistry, + "not found in HKEY_LOCAL_MACHINE"); } - status = SearchPaths(driver_path, GetSearchPaths(ADBC_LOAD_FLAG_SEARCH_SYSTEM), - info, error); + auto system_paths = GetSearchPaths(ADBC_LOAD_FLAG_SEARCH_SYSTEM); + status = SearchPathsForDriver(driver_path, system_paths, info, error); if (status == ADBC_STATUS_OK) { return status; } + search_paths.insert(search_paths.end(), system_paths.begin(), system_paths.end()); } info.lib_path = driver_path; - return Load(driver_path.c_str(), error); + return Load(driver_path.c_str(), search_paths, error); #else // Otherwise, search the configured paths. - auto search_paths = GetSearchPaths(load_options & ~ADBC_LOAD_FLAG_SEARCH_ENV); - auto status = SearchPaths(driver_path, search_paths, info, error); + SearchPaths more_search_paths = + GetSearchPaths(load_options & ~ADBC_LOAD_FLAG_SEARCH_ENV); + auto status = SearchPathsForDriver(driver_path, more_search_paths, info, error); if (status == ADBC_STATUS_NOT_FOUND) { // If we reach here, we didn't find the driver in any of the paths // so let's just attempt to load it as default behavior. + search_paths.insert(search_paths.end(), more_search_paths.begin(), + more_search_paths.end()); info.lib_path = driver_path; - return Load(driver_path.c_str(), error); + return Load(driver_path.c_str(), search_paths, error); } return status; #endif // _WIN32 } - AdbcStatusCode Load(const char_type* library, struct AdbcError* error) { + AdbcStatusCode Load(const char_type* library, const SearchPaths& attempted_paths, + struct AdbcError* error) { std::string error_message; #if defined(_WIN32) HMODULE handle = LoadLibraryExW(library, NULL, 0); @@ -583,8 +774,9 @@ struct ManagedLibrary { } } if (!handle) { + AddSearchPathsToError(attempted_paths, error_message); SetError(error, error_message); - return ADBC_STATUS_INTERNAL; + return ADBC_STATUS_NOT_FOUND; } else { this->handle = handle; } @@ -628,8 +820,9 @@ struct ManagedLibrary { if (handle) { this->handle = handle; } else { + AddSearchPathsToError(attempted_paths, error_message); SetError(error, error_message); - return ADBC_STATUS_INTERNAL; + return ADBC_STATUS_NOT_FOUND; } #endif // defined(_WIN32) return ADBC_STATUS_OK; @@ -2223,7 +2416,6 @@ AdbcStatusCode AdbcFindLoadDriver(const char* driver_name, const char* entrypoin SetError(error, "Driver pointer is null"); return ADBC_STATUS_INVALID_ARGUMENT; } - auto* driver = reinterpret_cast<struct AdbcDriver*>(raw_driver); if (!driver_name) { SetError(error, "Driver name is null"); return ADBC_STATUS_INVALID_ARGUMENT; @@ -2240,6 +2432,8 @@ AdbcStatusCode AdbcFindLoadDriver(const char* driver_name, const char* entrypoin additional_paths = InternalAdbcParsePath(additional_search_path_list); } + auto* driver = reinterpret_cast<struct AdbcDriver*>(raw_driver); + AdbcStatusCode status = library.GetDriverInfo(driver_name, load_options, additional_paths, info, error); if (status != ADBC_STATUS_OK) { diff --git a/r/adbcdrivermanager/tests/testthat/test-driver_void.R b/r/adbcdrivermanager/tests/testthat/test-driver_void.R index 1ec8ff33b..657f42118 100644 --- a/r/adbcdrivermanager/tests/testthat/test-driver_void.R +++ b/r/adbcdrivermanager/tests/testthat/test-driver_void.R @@ -61,6 +61,8 @@ test_that("drivers are loaded using load_flags", { test_that("drivers can be loaded by manifest path", { toml_content <- sprintf(" +manifest_version = 1 + name = 'Void Driver' [ADBC] diff --git a/rust/driver_manager/src/lib.rs b/rust/driver_manager/src/lib.rs index 40fd40a78..4bf299b4b 100644 --- a/rust/driver_manager/src/lib.rs +++ b/rust/driver_manager/src/lib.rs @@ -183,6 +183,20 @@ impl DriverInfo { let manifest = DeTable::parse(&contents) .map_err(|e| Error::with_message_and_status(e.to_string(), Status::InvalidArguments))?; + let manifest_version = manifest + .get_ref() + .get("manifest_version") + .and_then(|v| v.get_ref().as_integer()) + .map(|v| v.as_str()) + .unwrap_or("1"); + + if manifest_version != "1" { + return Err(Error::with_message_and_status( + format!("Unsupported manifest version: {manifest_version}"), + Status::InvalidArguments, + )); + } + // leave these out until we add logging that would actually utilize them // let driver_name = get_optional_key(&manifest, "name"); // let version = get_optional_key(&manifest, "version"); @@ -675,6 +689,15 @@ fn load_driver_from_registry( ) })?; + let manifest_version = drivers_key.get_u32("manifest_version").unwrap_or(1); + + if manifest_version != 1 { + return Err(Error::with_message_and_status( + format!("Unsupported manifest version: {manifest_version}"), + Status::InvalidArguments, + )); + } + let entrypoint_val = drivers_key .get_string("entrypoint") .ok() @@ -1896,6 +1919,8 @@ mod tests { fn manifest_without_driver() -> &'static str { r#" + manifest_version = 1 + name = 'SQLite3' publisher = 'arrow-adbc' version = '1.0.0' @@ -2221,6 +2246,28 @@ mod tests { .expect("Failed to close/remove temporary directory"); } + #[test] + #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)] + fn test_manifest_bad_version() { + let manifest = "manifest_version = 2\n".to_owned() + &simple_manifest().to_owned(); + let (tmp_dir, manifest_path) = + write_manifest_to_tempfile(PathBuf::from("sqlite.toml"), manifest); + + let err = ManagedDriver::load_from_name( + manifest_path, + None, + AdbcVersion::V100, + LOAD_FLAG_DEFAULT, + None, + ) + .unwrap_err(); + assert_eq!(err.status, Status::InvalidArguments); + + tmp_dir + .close() + .expect("Failed to close/remove temporary directory"); + } + #[test] #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)] fn test_manifest_wrong_arch() {