diff --git a/rust/lancedb/src/remote/client.rs b/rust/lancedb/src/remote/client.rs index 185e77a11..512eaa977 100644 --- a/rust/lancedb/src/remote/client.rs +++ b/rust/lancedb/src/remote/client.rs @@ -461,6 +461,18 @@ fn validate_id_delimiter(delimiter: String) -> Result { .to_string(), }); } + // The root namespace is addressed by the bare delimiter, so a delimiter that + // is itself a relative segment makes the root listing route disappear: + // `/v1/namespace/./secret/list` resolves to `/v1/namespace/secret/list`. + if delimiter == "." || delimiter == ".." { + return Err(Error::InvalidInput { + message: format!( + "id_delimiter '{delimiter}' is a relative path segment: the root namespace is \ + addressed by the delimiter alone, so a request for it would resolve to a \ + different route" + ), + }); + } if let Some(unsafe_character) = delimiter .chars() .find(|character| matches!(character, '/' | '?' | '#' | '%' | '\\')) @@ -476,6 +488,19 @@ fn validate_id_delimiter(delimiter: String) -> Result { Ok(delimiter) } +impl ClientConfig { + /// Check the settings that a request cannot be built correctly without. + /// + /// Called before a client is constructed from this, so a configuration + /// mistake is reported where it was made rather than as a confusing + /// response later. Public so a caller assembling a config can ask the same + /// question without connecting. + pub fn validate(&self) -> Result<()> { + validate_id_delimiter(self.id_delimiter.clone().unwrap_or_else(|| "$".to_string()))?; + Ok(()) + } +} + impl RestfulLanceDbClient { fn get_timeout(passed: Option, env_var: &str) -> Result> { if let Some(passed) = passed { @@ -501,6 +526,10 @@ impl RestfulLanceDbClient { client_config: ClientConfig, read_consistency_interval: Option, ) -> Result { + // Before anything is built from it: a bad delimiter is a mistake in the + // caller's configuration, and saying so here names it as one. + client_config.validate()?; + // Get the timeouts let timeout = Self::get_timeout(client_config.timeout_config.timeout, "LANCE_CLIENT_TIMEOUT")?; @@ -600,12 +629,10 @@ impl RestfulLanceDbClient { host, retry_config, sender: Sender, - id_delimiter: validate_id_delimiter( - client_config - .id_delimiter - .clone() - .unwrap_or("$".to_string()), - )?, + id_delimiter: client_config + .id_delimiter + .clone() + .unwrap_or("$".to_string()), header_provider: client_config.header_provider, read_consistency_interval, max_bytes_per_request, @@ -1169,7 +1196,7 @@ mod tests { /// component contains one. #[test] fn test_a_delimiter_that_is_path_syntax_is_refused() { - for delimiter in ["/", "?", "#", "%", "a/b", ""] { + for delimiter in ["/", "?", "#", "%", "a/b", "", ".", "..", "/../../function/"] { let error = super::validate_id_delimiter(delimiter.to_string()) .expect_err("a delimiter that is path syntax must be refused"); assert!( @@ -1178,7 +1205,9 @@ mod tests { ); } // The default, and other separators that are not path syntax, stand. - for delimiter in ["$", ".", "-", "_", "|", "::"] { + // A period is fine inside a longer delimiter -- only the exact relative + // segments are a problem. + for delimiter in ["$", "-", "_", "|", "::", ".x.", "..."] { super::validate_id_delimiter(delimiter.to_string()) .unwrap_or_else(|error| panic!("{delimiter:?} must be accepted: {error}")); } diff --git a/rust/lancedb/src/utils/mod.rs b/rust/lancedb/src/utils/mod.rs index 30a66c714..32d54eba0 100644 --- a/rust/lancedb/src/utils/mod.rs +++ b/rust/lancedb/src/utils/mod.rs @@ -157,15 +157,19 @@ pub fn validate_namespace_name(name: &str) -> Result<()> { Ok(()) } -/// The characters a Secret name or a Secret namespace segment may contain, and -/// the longest one either may be. +/// The characters a Secret name or a Secret namespace segment may contain. /// /// Deliberately the set the service admits, and no positional rule on top of /// it: a segment may begin with `_`, `-` or `.`, because LanceDB namespaces /// already do, and anything narrower would put Secrets out of reach inside /// namespaces that already exist. +/// +/// No length bound either. How long a name may be is the service's to decide, +/// the way it is for a table, and a bound here could only disagree with it -- +/// one that is shorter refuses names the catalog would hold, and one that is +/// longer says nothing. static SECRET_NAME_REGEX: std::sync::LazyLock = - std::sync::LazyLock::new(|| regex::Regex::new(r"^[A-Za-z0-9_.\-]{1,255}$").unwrap()); + std::sync::LazyLock::new(|| regex::Regex::new(r"^[A-Za-z0-9_.\-]+$").unwrap()); /// Validate one component of a Secret identifier: a Secret name, or one segment /// of the namespace path holding it. @@ -202,7 +206,7 @@ pub fn validate_secret_component(what: &str, value: &str) -> Result<()> { return Err(Error::InvalidInput { message: format!( "invalid {what} '{value}': it may contain only alphanumeric characters, \ - underscores, hyphens and periods, and may be at most 255 bytes" + underscores, hyphens and periods" ), }); }