From bcb822a2910253039c1f2fc5450825f6386755bf Mon Sep 17 00:00:00 2001 From: Jonathan M Hsieh Date: Mon, 14 Sep 2026 20:22:20 +0000 Subject: [PATCH] docs(secrets): say why the code is as it is, not what it replaced Comments that explained a decision by naming what came before it read as changelog rather than rationale, and they age badly: a reader who never saw the earlier shape learns nothing from being told it existed. This is a new surface, so every comment here now states the property that holds today. - `RemoteCreateFunctionRequest` said a literal "is why `secret_bindings` silently stopped reaching the service". It now says what the struct buys: the compiler decides what reaches the service, and a new field on the registration request is a build error here until it is handled. - Wire-omission comments that appealed to clients predating Secrets or namespaces now state the rule itself -- a binding states a namespace only when it has one, a body carries a binding list only when there is one. - `BodyLogging::Allowed` described logging "as every request did before Secrets existed", which is really the default: a body is diagnostic unless the call that built it says otherwise. - `a_version_without_bindings_keeps_the_original_wire_shape` named a shape no longer worth referencing. Both the Rust and Python tests now name the property they check -- absent decodes as an empty list, and an empty list serializes back to absent. Comments that cite the Table and Function conventions are kept. Those describe what the code sits beside today, which is the reason a reader needs. Tests: lancedb secret unit tests 7, `first_class_function_slice1` 9, `first_class_function_slice2` 3, 93 Python tests across both slices. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UfmeJ533rQDnPBkMtjerV6 --- .../python/tests/test_first_class_function_slice1.py | 8 ++++++-- .../python/tests/test_first_class_function_slice2.py | 4 ++-- rust/lancedb/src/function.rs | 5 +++-- rust/lancedb/src/remote/client.rs | 5 +++-- rust/lancedb/src/remote/db.rs | 12 +++++------- rust/lancedb/tests/first_class_function_slice1.rs | 5 +++-- 6 files changed, 22 insertions(+), 17 deletions(-) diff --git a/python/python/tests/test_first_class_function_slice1.py b/python/python/tests/test_first_class_function_slice1.py index 34fa05364..83dc61841 100644 --- a/python/python/tests/test_first_class_function_slice1.py +++ b/python/python/tests/test_first_class_function_slice1.py @@ -311,8 +311,12 @@ def test_canonical_client_values_carry_bindings_and_no_credentials(): assert_no_secret_values(canonical) -def test_a_version_without_bindings_keeps_the_original_wire_shape(): - """Every Function registered before Secrets existed serializes unchanged.""" +def test_a_version_without_bindings_omits_the_field_in_both_directions(): + """A Function that binds nothing carries no ``secret_bindings`` key. + + Absent decodes as an empty list, and an empty list serializes back to + absent. + """ value = job_result("remote_function_job.json") del value["secret_bindings"] version = FunctionVersion.from_json(json.dumps(value)) diff --git a/python/python/tests/test_first_class_function_slice2.py b/python/python/tests/test_first_class_function_slice2.py index 753c71b70..b01e328d2 100644 --- a/python/python/tests/test_first_class_function_slice2.py +++ b/python/python/tests/test_first_class_function_slice2.py @@ -99,8 +99,8 @@ def test_secret_bound_udf_matches_its_shared_registration_golden(): def test_a_namespaced_binding_records_the_path_and_the_name(): """A binding names the parts, so nothing has to be parsed back out. - A root binding carries no path at all, which is what keeps its wire shape - identical to one written before namespaces existed. + A root binding carries no path at all: the field is absent rather than an + empty list, so a binding states a namespace only when it has one. """ root = EnvVarSecret(secret_name="openai-prod", env_variable="OPENAI_API_KEY") assert root.secret_namespace_path == [] diff --git a/rust/lancedb/src/function.rs b/rust/lancedb/src/function.rs index 403200262..c1d0d46c3 100644 --- a/rust/lancedb/src/function.rs +++ b/rust/lancedb/src/function.rs @@ -503,8 +503,9 @@ pub struct FunctionArtifactRequest { #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] pub struct SecretReference { pub name: String, - /// The namespace holding the Secret. Empty is the root, and is omitted from - /// the wire so a root binding carries no trace of a feature it does not use. + /// The namespace holding the Secret. Empty is the root, and is absent from + /// the wire rather than sent empty: a binding states a namespace only when + /// it has one. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub namespace_path: Vec, } diff --git a/rust/lancedb/src/remote/client.rs b/rust/lancedb/src/remote/client.rs index d0a418002..dddd58ffb 100644 --- a/rust/lancedb/src/remote/client.rs +++ b/rust/lancedb/src/remote/client.rs @@ -412,7 +412,8 @@ fn validate_dns_hostname(hostname: &str) -> Result<()> { /// call that has it. #[derive(Clone, Copy, PartialEq, Eq, Debug)] enum BodyLogging { - /// Log the body at debug, as every request did before Secrets existed. + /// Log the body at debug. The default: a request body is diagnostic unless + /// the call that built it says otherwise. Allowed, /// Never log the body. For a request whose body is a credential. Suppressed, @@ -1254,7 +1255,7 @@ mod tests { } /// A suppressed body is suppressed whatever the content type says, and an - /// allowed one is logged exactly as it was before Secrets existed. + /// allowed one is logged in full. #[test] fn test_body_logging_is_decided_by_the_caller() { assert_ne!(BodyLogging::Allowed, BodyLogging::Suppressed); diff --git a/rust/lancedb/src/remote/db.rs b/rust/lancedb/src/remote/db.rs index 2bbc64eb7..0f38d7c89 100644 --- a/rust/lancedb/src/remote/db.rs +++ b/rust/lancedb/src/remote/db.rs @@ -616,18 +616,16 @@ struct RemoteDropFunctionResponse { /// The create body: every field of [`FunctionRegistrationRequest`] except the /// name, which is the path identifier. /// -/// A struct rather than a `json!` literal naming fields one by one. The literal -/// is why `secret_bindings` silently stopped reaching the service when the -/// route moved -- a field added to the request type does not have to be added -/// here to compile, it just stops being sent. This shape fails to build -/// instead. +/// A struct rather than a literal listing the fields, so that the compiler +/// decides what reaches the service. A field the registration request grows is +/// a build error here until it is handled; a literal would simply not send it. #[derive(serde::Serialize)] struct RemoteCreateFunctionRequest<'a> { artifact: &'a FunctionArtifactRequest, signature: &'a FunctionSignature, runtime: &'a PythonRuntimeSpec, - /// Omitted when the Function binds nothing, so a request from a client that - /// predates Secrets is byte-identical to one that does not use them. + /// Absent when the Function binds nothing, so the body carries a binding + /// list only when there is one to carry. #[serde(skip_serializing_if = "<[SecretBinding]>::is_empty")] secret_bindings: &'a [SecretBinding], } diff --git a/rust/lancedb/tests/first_class_function_slice1.rs b/rust/lancedb/tests/first_class_function_slice1.rs index a4956e657..39ff8584b 100644 --- a/rust/lancedb/tests/first_class_function_slice1.rs +++ b/rust/lancedb/tests/first_class_function_slice1.rs @@ -223,9 +223,10 @@ fn an_unknown_binding_kind_is_forward_decodable() { ); } -/// Every Function registered before Secrets existed serializes unchanged. +/// A Function that binds nothing carries no `secret_bindings` key: absent +/// decodes as an empty list, and an empty list serializes back to absent. #[test] -fn a_version_without_bindings_keeps_the_original_wire_shape() { +fn a_version_without_bindings_omits_the_field_in_both_directions() { let mut result = job_result("remote_function_job.json"); result .as_object_mut()