feat(rust/driver_manager): implement connection profiles#3973
feat(rust/driver_manager): implement connection profiles#3973zeroshade wants to merge 18 commits intoapache:mainfrom
Conversation
felipecrv
left a comment
felipecrv
left a comment
There was a problem hiding this comment.
Not a full review yet. But it's a good start.
| // an immutable struct of function pointers. Wrapping the driver in a `Mutex` | ||
| // would prevent any parallelism between driver calls, which is not desirable. | ||
|
|
||
| pub mod connection_profiles; |
There was a problem hiding this comment.
Use singular connection_profile. Or even better, just profile.
| /// | ||
| /// URIs can specify either a direct driver connection or a profile to load. | ||
| #[derive(Debug)] | ||
| pub(crate) enum SearchResult<'a> { |
There was a problem hiding this comment.
There is a struct called SearchHit already with the actual loaded library object. Need a name for this enum that is not so close to SearchHit in meaning. Suggestion DriverLocator (as it wraps a universinal resource LOCATORS).
There was a problem hiding this comment.
Then you can drop the Driver from the enum entry.
DriverLocator = Uri | ProfilePath
| drv = ManagedDriver::load_from_name( | ||
| driver, | ||
| entrypoint, | ||
| version, | ||
| load_flags, | ||
| additional_search_paths.clone(), | ||
| )?; |
There was a problem hiding this comment.
This can become
DriverLibrary::search(name, load_flags, additional_search_paths)?and you can make every branch return SearchHit. Then at the end you can:
let entrypoint = search_hit.resolve_entrypoint(entrypoint).to_vec();
Self::load_from_library(search_hit.library, entrypoint.as_ref(), version)Then, at the end of all this, you can extract a function that returns SearchHit to search.rs and keep the load_from_library call here. This is what enables different users of search.rs (which I intend to make more public in the upstream too) to instantiate structs that are not ManagedDriver.
There was a problem hiding this comment.
Looks like this is not a trivial change, but you get the intention: move search logic to search.rs and keep only the final instantiation of ManagedDriver in this file.
| let profile_opts: Vec<(OptionDatabase, OptionValue)> = profile | ||
| .get_options()? | ||
| .into_iter() | ||
| .map(|(k, v)| { |
There was a problem hiding this comment.
| .map(|(k, v)| { | |
| .map(|(k, v)| -> Result<(OptionDatabase, OptionValue), Error> { |
| .map(|(k, v)| { | ||
| if let OptionValue::String(s) = v { | ||
| let result = process_profile_value(&s)?; | ||
| Ok::<(OptionDatabase, OptionValue), Error>((k, result)) |
There was a problem hiding this comment.
| Ok::<(OptionDatabase, OptionValue), Error>((k, result)) | |
| Ok((k, result)) |
| // Convert the name to a PathBuf to ensure proper platform-specific path handling. | ||
| // This normalizes forward slashes to backslashes on Windows. | ||
| let profile_path = PathBuf::from_slash(name.as_ref()); | ||
| let profile_path = profile_path.as_path(); |
There was a problem hiding this comment.
You're shadowing the profile_path: PathBuf but then you are allocating a new PathBuf from the Path to return it.
You should invert: use profile_path.as_path() where you need a Path and delete the .to_path_buf() and use just profile_path: PathBuf.
| Err(Error::with_message_and_status( | ||
| format!("Profile not found: {}", profile_path.display()), | ||
| Status::NotFound, | ||
| )) |
There was a problem hiding this comment.
I think you should let the caller try to open the absolute path and provide an error message (there) about the file failing to open instead of the profile path not being found. There is nothing to "find" because it's an absolute path.
| } | ||
|
|
||
| // No .toml extension - add it | ||
| return Ok(profile_path.with_extension("toml")); |
There was a problem hiding this comment.
Specially considering that you don't stat the file here. Which is good.
|
|
||
| // Handle absolute paths | ||
| if profile_path.is_absolute() { | ||
| let has_toml_ext = profile_path.extension().is_some_and(|ext| ext == "toml"); |
There was a problem hiding this comment.
This variable can be defined right after profile_path is defined.
| if has_toml_ext { | ||
| // Name already has .toml extension, use it as-is | ||
| let full_path = path.join(profile_path); | ||
| if full_path.is_file() { | ||
| return Some(full_path); | ||
| } | ||
| } | ||
| // Try adding .toml extension | ||
| let mut full_path = path.join(profile_path); | ||
| full_path.set_extension("toml"); | ||
| if full_path.is_file() { | ||
| return Some(full_path); | ||
| } | ||
| None |
There was a problem hiding this comment.
Not just a style change. It avoid checking the same path twice when it has the .toml extension and doesn't exist.
| if has_toml_ext { | |
| // Name already has .toml extension, use it as-is | |
| let full_path = path.join(profile_path); | |
| if full_path.is_file() { | |
| return Some(full_path); | |
| } | |
| } | |
| // Try adding .toml extension | |
| let mut full_path = path.join(profile_path); | |
| full_path.set_extension("toml"); | |
| if full_path.is_file() { | |
| return Some(full_path); | |
| } | |
| None | |
| let mut full_path = path.join(profile_path); | |
| if has_toml_ext { | |
| // Name already has .toml extension, use it as-is | |
| if full_path.is_file() { | |
| Some(full_path) | |
| } else { | |
| None | |
| } | |
| } else { | |
| // Try adding .toml extension | |
| full_path.set_extension("toml"); | |
| if full_path.is_file() { | |
| Some(full_path) | |
| } else { | |
| None | |
| } | |
| } |
2 participants
Implementing connection profiles for the Rust driver manager including the default FilesystemProfileProvider and updating the objects and tests accordingly.