From d6bcfe52a94b9b09592bc8712e55402a9b69d0fd Mon Sep 17 00:00:00 2001 From: Jonathan M Hsieh Date: Fri, 11 Sep 2026 18:55:35 +0000 Subject: [PATCH] refactor(secrets): epoch timestamps, a named list request, and unit tests `SecretInfo` carries `created_at_millis` / `updated_at_millis` as `i64` rather than RFC 3339 strings, matching `created_at_millis` elsewhere in the platform. A caller comparing two timestamps no longer parses anything, and the PyO3 layer hands Python integers instead of stringifying them. `updated_at_millis` is the only observable that a rotation landed -- no API returns a credential -- so it is worth being a number a caller can compare. `RemoteListSecretsRequest` is declared alongside its response instead of being an inline object built field by field, so a reader of one finds the other. `function.rs` gains unit tests for the pure parts that had none: canonical JSON sorts keys at every depth and leaves array order alone -- it is what the version hash is taken over, so both matter -- floats are refused at any depth, and the unknown-key check inspects only the level it is handed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UfmeJ533rQDnPBkMtjerV6 --- python/python/lancedb/db.py | 9 +++- python/python/lancedb/secrets.py | 37 ++++++++------- python/src/connection.rs | 11 ++--- rust/lancedb/src/function.rs | 77 ++++++++++++++++++++++++++++++++ rust/lancedb/src/remote/db.rs | 16 +++++-- rust/lancedb/src/secrets.rs | 13 ++++-- 6 files changed, 130 insertions(+), 33 deletions(-) diff --git a/python/python/lancedb/db.py b/python/python/lancedb/db.py index 804a98080..69da72cdc 100644 --- a/python/python/lancedb/db.py +++ b/python/python/lancedb/db.py @@ -2444,8 +2444,13 @@ class AsyncConnection(object): async def describe_secret(self, name: str) -> SecretInfo: """What this database records about a Secret. Never the value.""" - return SecretInfo.from_json( - await self._inner.describe_secret(validate_secret_name(name)) + name, created_at_millis, updated_at_millis = await self._inner.describe_secret( + validate_secret_name(name) + ) + return SecretInfo( + name=name, + created_at_millis=created_at_millis, + updated_at_millis=updated_at_millis, ) async def list_jobs(self) -> List[JobInfo]: diff --git a/python/python/lancedb/secrets.py b/python/python/lancedb/secrets.py index e04c0054f..27c7ae3ef 100644 --- a/python/python/lancedb/secrets.py +++ b/python/python/lancedb/secrets.py @@ -116,12 +116,12 @@ class SecretInfo: [DBConnection.describe_secret][lancedb.db.DBConnection.describe_secret]. """ - __slots__ = ("_name", "_created_at", "_updated_at") + __slots__ = ("_name", "_created_at_millis", "_updated_at_millis") - def __init__(self, name: str, created_at: str, updated_at: str): + def __init__(self, name: str, created_at_millis: int, updated_at_millis: int): self._name = name - self._created_at = created_at - self._updated_at = updated_at + self._created_at_millis = created_at_millis + self._updated_at_millis = updated_at_millis @property def name(self) -> str: @@ -129,35 +129,40 @@ class SecretInfo: return self._name @property - def created_at(self) -> str: - """When the Secret was created, as an RFC 3339 timestamp.""" - return self._created_at + def created_at_millis(self) -> int: + """When the Secret was created, in milliseconds since the Unix epoch.""" + return self._created_at_millis @property - def updated_at(self) -> str: - """When the Secret's value was last rotated, as an RFC 3339 timestamp.""" - return self._updated_at + def updated_at_millis(self) -> int: + """When the Secret's value was last rotated, in epoch milliseconds. + + The only observable that a rotation landed: no API returns a credential, + so a caller confirms ``alter_secret`` took effect by watching this move. + """ + return self._updated_at_millis @classmethod def from_json(cls, value: dict) -> "SecretInfo": return cls( name=value["name"], - created_at=value["created_at"], - updated_at=value["updated_at"], + created_at_millis=value["created_at_millis"], + updated_at_millis=value["updated_at_millis"], ) def __repr__(self) -> str: return ( - f"SecretInfo(name={self._name!r}, created_at={self._created_at!r}, " - f"updated_at={self._updated_at!r})" + f"SecretInfo(name={self._name!r}, " + f"created_at_millis={self._created_at_millis!r}, " + f"updated_at_millis={self._updated_at_millis!r})" ) def __eq__(self, other: object) -> bool: return ( isinstance(other, SecretInfo) and other._name == self._name - and other._created_at == self._created_at - and other._updated_at == self._updated_at + and other._created_at_millis == self._created_at_millis + and other._updated_at_millis == self._updated_at_millis ) diff --git a/python/src/connection.rs b/python/src/connection.rs index fdb3a2429..c87d8cbe7 100644 --- a/python/src/connection.rs +++ b/python/src/connection.rs @@ -740,17 +740,14 @@ impl Connection { }) } - /// Name and timestamps as a plain mapping. `SecretInfo` carries no value, - /// so there is none to filter out here. + /// Name and timestamps as a plain tuple. `SecretInfo` carries no value, so + /// there is none to filter out here. Timestamps stay integers rather than + /// going through a string, so the caller can compare two without parsing. pub fn describe_secret(self_: PyRef<'_, Self>, name: String) -> PyResult> { let inner = self_.get_inner()?.clone(); future_into_py(self_.py(), async move { let info = inner.describe_secret(name).await.infer_error()?; - Ok(HashMap::from([ - ("name".to_string(), info.name), - ("created_at".to_string(), info.created_at), - ("updated_at".to_string(), info.updated_at), - ])) + Ok((info.name, info.created_at_millis, info.updated_at_millis)) }) } diff --git a/rust/lancedb/src/function.rs b/rust/lancedb/src/function.rs index e14aa1fa8..6ea34610e 100644 --- a/rust/lancedb/src/function.rs +++ b/rust/lancedb/src/function.rs @@ -770,3 +770,80 @@ mod conda_environment_tests { } } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Canonical form is what the FunctionVersion hash is taken over, so key + /// order must come from the keys and not from however serde happened to + /// emit them. Nesting is included because the sort is recursive. + #[test] + fn canonical_json_sorts_keys_at_every_depth() { + let value = serde_json::json!({ + "runtime": {"kind": "python", "env": {"B": "2", "A": "1"}}, + "artifact": {"digest": "sha256:x"}, + "name": "embed", + }); + let mut out = String::new(); + write_canonical_json(&value, &mut out).expect("canonical JSON"); + + assert_eq!( + out, + r#"{"artifact":{"digest":"sha256:x"},"name":"embed","runtime":{"env":{"A":"1","B":"2"},"kind":"python"}}"# + ); + } + + /// Arrays are ordered by the caller, so canonicalization must leave them + /// alone -- sorting them would change what a signature means. + #[test] + fn canonical_json_preserves_array_order() { + let value = serde_json::json!({"inputs": ["b", "a", "c"]}); + let mut out = String::new(); + write_canonical_json(&value, &mut out).expect("canonical JSON"); + + assert_eq!(out, r#"{"inputs":["b","a","c"]}"#); + } + + /// A float has no single canonical spelling, so two clients could hash the + /// same literal differently. Rejected at any depth rather than rounded. + #[test] + fn validate_literal_rejects_floats_at_any_depth() { + for value in [ + serde_json::json!(1.5), + serde_json::json!([1, [2, 3.5]]), + serde_json::json!({"a": {"b": 0.25}}), + ] { + let error = validate_literal(&value).expect_err("floats are not canonical"); + assert!( + error.to_string().contains("floating-point"), + "unexpected error: {error}" + ); + } + + for value in [ + serde_json::json!(1), + serde_json::json!("1.5"), + serde_json::json!([1, {"a": true}]), + serde_json::json!(null), + ] { + validate_literal(&value).expect("non-float literals are canonical"); + } + } + + /// Unknown keys are how a newer server's payload reaches an older client, + /// so the check has to be exact about which level it is looking at. + #[test] + fn has_unknown_keys_only_inspects_the_level_it_is_given() { + let value = serde_json::json!({"name": "embed", "version": "fv_1"}); + assert!(!has_unknown_keys(&value, &["name", "version"])); + assert!(has_unknown_keys(&value, &["name"])); + + // A nested unknown is not this level's business. + let nested = serde_json::json!({"name": {"unexpected": 1}}); + assert!(!has_unknown_keys(&nested, &["name"])); + + // A non-object has no keys to be unknown. + assert!(!has_unknown_keys(&serde_json::json!("embed"), &["name"])); + } +} diff --git a/rust/lancedb/src/remote/db.rs b/rust/lancedb/src/remote/db.rs index 3a04513a5..c89ae11b7 100644 --- a/rust/lancedb/src/remote/db.rs +++ b/rust/lancedb/src/remote/db.rs @@ -588,6 +588,15 @@ struct RemoteDropFunctionResponse { dropped: bool, } +/// One page of a Secret listing. A struct rather than an inline object so the +/// request and the response are declared the same way -- a reader of one finds +/// the other. +#[derive(serde::Serialize)] +struct RemoteListSecretsRequest { + #[serde(skip_serializing_if = "Option::is_none")] + page_token: Option, +} + #[derive(serde::Deserialize)] struct RemoteListSecretsResponse { #[serde(default)] @@ -717,10 +726,9 @@ impl Database for RemoteDatabase { let mut page_token: Option = None; let mut seen_page_tokens = HashSet::new(); loop { - let mut body = serde_json::json!({}); - if let Some(token) = &page_token { - body["page_token"] = serde_json::Value::String(token.clone()); - } + let body = RemoteListSecretsRequest { + page_token: page_token.clone(), + }; let req = self.client.post("/v1/secrets/list").json(&body); let (request_id, response) = self.client.send(req).await?; let response = self.client.check_response(&request_id, response).await?; diff --git a/rust/lancedb/src/secrets.rs b/rust/lancedb/src/secrets.rs index 904260aee..6fe22fbf5 100644 --- a/rust/lancedb/src/secrets.rs +++ b/rust/lancedb/src/secrets.rs @@ -16,8 +16,13 @@ pub struct SecretInfo { /// The Secret's database-scoped name. pub name: String, - /// When the Secret was created, as an RFC 3339 timestamp. - pub created_at: String, - /// When the Secret's value was last rotated, as an RFC 3339 timestamp. - pub updated_at: String, + /// When the Secret was created, in milliseconds since the Unix epoch. + pub created_at_millis: i64, + /// When the Secret's value was last rotated, in milliseconds since the Unix + /// epoch. + /// + /// This is the only observable that a rotation landed: no API returns a + /// credential, so a caller confirms `alter_secret` took effect by watching + /// this move. + pub updated_at_millis: i64, }