feat: add typed search attributes API#1346
Conversation
Add type-safe SearchAttributeKey<T>, TypedSearchAttributes, and SearchAttributeUpdate types that provide compile-time type safety for search attribute operations, matching Go/Python/TS SDK parity. Core types (common-wasm): - SearchAttributeKey<T> with const constructors for all 7 types - Sealed SearchAttributeValue trait for bool/i64/f64/String/Timestamp/Vec<String> - TypedSearchAttributes collection with type-safe get/set - Correct wire format: metadata["encoding"]=json/plain, metadata["type"]=IndexedValueType Integration: - WorkflowContext: typed_search_attributes() getter + upsert_typed_search_attributes() - WorkflowStartOptions: typed_search_attributes field (typed takes precedence) - ChildWorkflowOptions: typed_search_attributes field - ContinueAsNewOptions: typed_search_attributes field Existing raw search_attributes fields/methods remain unchanged (additive). 19 unit tests + 1 doc-test covering all value type round-trips, proto conversion, unset behavior, and Keyword vs Text disambiguation.
chris-olszewski
left a comment
There was a problem hiding this comment.
The "typed" variants of these APIs should wholesale replace the current raw payload APIs for working with search attributes.
| metadata.insert( | ||
| ENCODING_PAYLOAD_KEY.to_string(), | ||
| JSON_ENCODING_VAL.as_bytes().to_vec(), | ||
| ); |
There was a problem hiding this comment.
We should reuse the default JSON payload converter here instead of directly calling serde_json and building up the metadata ourselves e.g. https://github.com/temporalio/sdk-ruby/blob/11dc8dbf63a6911aa53e22cb40acf597b89e9a68/temporalio/lib/temporalio/search_attributes.rb#L149
Replace raw HashMap<String, Payload> search attribute APIs with type-safe alternatives across client, workflow, and test code. Core changes (crates/common-wasm): - New Timestamp newtype (decoupled from prost_types::Timestamp) with Display, Ord, Hash, and From/TryFrom conversions for SystemTime and prost_types::Timestamp. Pre-epoch timestamps normalized per protobuf spec. - impl_simple_search_attribute_value! macro for bool/i64/String/Vec<String> - Manual f64 impl rejecting NaN/Infinity (serde_json serializes as null) - SearchAttributeKey<T> is now Copy with try_value_set() fallible variant - TypedSearchAttributes gains keys(), raw_payload(), try_get(), into_proto() - chrono added with minimal features (alloc only) for WASM safety - SecondsFormat::Nanos for cross-SDK timestamp consistency - 38 unit tests including edge cases (pre-epoch, NaN, boundaries, malformed) API removals (breaking): - WorkflowStartOptions.search_attributes: HashMap → TypedSearchAttributes - ChildWorkflowOptions.search_attributes: HashMap → TypedSearchAttributes - ContinueAsNewOptions.search_attributes: SearchAttributes → TypedSearchAttributes - upsert_search_attributes() now takes SearchAttributeUpdate, not (String, Payload) - Merged typed_search_attributes fields/methods into search_attributes Addresses PR temporalio#1346 review feedback from chris-olszewski.
|
@chris-olszewski - thanks for review. I think addressed, and found some other improvements! |
chris-olszewski
left a comment
There was a problem hiding this comment.
This is a far better API. I think it makes sense to use this new typed search attributes everywhere and remove existing methods for accessing the raw proto. If a user wants to work with the raw proto they can use the conversion and work with it directly.
Just a few other things to clean up before we merge.
| } | ||
| } | ||
|
|
||
| impl From<prost_types::Timestamp> for Timestamp { |
There was a problem hiding this comment.
If we end up going with additional checks at construction time this should be failable conversion
| impl From<prost_types::Timestamp> for Timestamp { | |
| impl TryFrom<prost_types::Timestamp> for Timestamp { |
| /// Returns a reference to the raw payload for the given attribute name, | ||
| /// if present. This is useful for advanced use cases such as forwarding | ||
| /// payloads without deserializing them. | ||
| pub fn raw_payload(&self, name: &str) -> Option<&Payload> { | ||
| self.fields.get(name) | ||
| } | ||
|
|
||
| /// Convert to the proto wire representation. | ||
| pub fn to_proto(&self) -> ProtoSearchAttributes { | ||
| ProtoSearchAttributes { | ||
| indexed_fields: self.fields.clone(), | ||
| } | ||
| } | ||
|
|
||
| /// Convert to the proto wire representation, consuming `self` to avoid | ||
| /// cloning. | ||
| pub fn into_proto(self) -> ProtoSearchAttributes { | ||
| ProtoSearchAttributes { | ||
| indexed_fields: self.fields, | ||
| } | ||
| } |
There was a problem hiding this comment.
What do you think about just providing a impl From<TypedSearchAttributes> for ProtoSearchAttributes instead of the special accessor methods/ad-hoc conversion methods?
| fn encode_json_search_attr<T: serde::Serialize>( | ||
| value: &T, | ||
| indexed_value_type: IndexedValueType, | ||
| ) -> Result<Payload, SearchAttributeError> { | ||
| let data = serde_json::to_vec(value)?; | ||
| let mut metadata = HashMap::with_capacity(2); | ||
| metadata.insert("encoding".to_string(), b"json/plain".to_vec()); | ||
| metadata.insert( | ||
| TYPE_METADATA_KEY.to_string(), | ||
| type_metadata_str(indexed_value_type).as_bytes().to_vec(), | ||
| ); | ||
| Ok(Payload { | ||
| metadata, | ||
| data, | ||
| ..Default::default() | ||
| }) | ||
| } |
There was a problem hiding this comment.
We should use the payload converter directly, not emulate behavior
| fn encode_json_search_attr<T: serde::Serialize>( | |
| value: &T, | |
| indexed_value_type: IndexedValueType, | |
| ) -> Result<Payload, SearchAttributeError> { | |
| let data = serde_json::to_vec(value)?; | |
| let mut metadata = HashMap::with_capacity(2); | |
| metadata.insert("encoding".to_string(), b"json/plain".to_vec()); | |
| metadata.insert( | |
| TYPE_METADATA_KEY.to_string(), | |
| type_metadata_str(indexed_value_type).as_bytes().to_vec(), | |
| ); | |
| Ok(Payload { | |
| metadata, | |
| data, | |
| ..Default::default() | |
| }) | |
| } | |
| fn encode_json_search_attr<T: TemporalSerializable + 'static>( | |
| value: &T, | |
| indexed_value_type: IndexedValueType, | |
| ) -> Result<Payload, SearchAttributeError> { | |
| let pc = PayloadConverter::serde_json(); | |
| let context = SerializationContext { | |
| converter: &pc, | |
| data: &SerializationContextData::None, | |
| }; | |
| let mut payload = pc.to_payload(&context, value)?; | |
| payload.metadata.insert( | |
| TYPE_METADATA_KEY.to_string(), | |
| type_metadata_str(indexed_value_type).as_bytes().to_vec(), | |
| ); | |
| Ok(payload) | |
| } |
| fn decode_json_search_attr<T: serde::de::DeserializeOwned>( | ||
| payload: &Payload, | ||
| ) -> Result<T, SearchAttributeError> { | ||
| let encoding = | ||
| payload | ||
| .metadata | ||
| .get("encoding") | ||
| .ok_or_else(|| SearchAttributeError::InvalidPayload { | ||
| reason: "missing encoding metadata".into(), | ||
| })?; | ||
| if encoding.as_slice() != b"json/plain" { | ||
| return Err(SearchAttributeError::InvalidPayload { | ||
| reason: format!( | ||
| "expected encoding 'json/plain', got '{}'", | ||
| String::from_utf8_lossy(encoding) | ||
| ), | ||
| }); | ||
| } | ||
| Ok(serde_json::from_slice(&payload.data)?) | ||
| } |
There was a problem hiding this comment.
Similar to above, we can just use the JSON payload converter directly
| fn decode_json_search_attr<T: serde::de::DeserializeOwned>( | |
| payload: &Payload, | |
| ) -> Result<T, SearchAttributeError> { | |
| let encoding = | |
| payload | |
| .metadata | |
| .get("encoding") | |
| .ok_or_else(|| SearchAttributeError::InvalidPayload { | |
| reason: "missing encoding metadata".into(), | |
| })?; | |
| if encoding.as_slice() != b"json/plain" { | |
| return Err(SearchAttributeError::InvalidPayload { | |
| reason: format!( | |
| "expected encoding 'json/plain', got '{}'", | |
| String::from_utf8_lossy(encoding) | |
| ), | |
| }); | |
| } | |
| Ok(serde_json::from_slice(&payload.data)?) | |
| } | |
| fn decode_json_search_attr<T: TemporalDeserializable + 'static>( | |
| payload: Payload, | |
| ) -> Result<T, SearchAttributeError> { | |
| let pc = PayloadConverter::serde_json(); | |
| let context = SerializationContext { | |
| converter: &pc, | |
| data: &SerializationContextData::None, | |
| }; | |
| Ok(pc.from_payload(&context, payload)?) | |
| } |
Co-authored-by: Chris Olszewski <chrisdolszewski@gmail.com>
Co-authored-by: Chris Olszewski <chrisdolszewski@gmail.com>
Co-authored-by: Chris Olszewski <chrisdolszewski@gmail.com>
Co-authored-by: Chris Olszewski <chrisdolszewski@gmail.com>
Rename TypedSearchAttributes → SearchAttributes (drop redundant prefix since the untyped version no longer exists in the public API). Replace, don't duplicate: remove old raw proto search_attributes() accessors from SyncWorkflowContext and WorkflowContext, making the typed search_attributes() the sole accessor. Proto imports aliased as ProtoSearchAttributes where needed to avoid name collisions. Additional fixes from reviewer suggestions: - Remove unused Timestamp import from doc example - Fix wording: 'matching Go SDK convention' → 'kept consistent across all SDKs' - Strengthen test assertions: assert exact payload bytes instead of !is_empty() - Clean up unused Ref/Deref imports All 38 unit tests + 6 continue_as_new tests pass. Zero warnings.
|
still workign on this. should have it later or else soon |
Applied all Critical, High, and Medium findings from 4-expert review: Core fixes: - C-1: upsert_search_attributes now updates local state correctly (unset removes keys instead of inserting empty payloads) - C-2: Fixed continue_as_new test compilation (SearchAttributes::default) Safety & correctness: - H-1: Timestamp fields now private with clamped constructor + getters - H-2: Forward-compat wildcard arm in default_indexed_value_type - H-3: WorkflowContextView uses typed SearchAttributes (not proto) - H-4: WASM safety docs on chrono Cargo.toml dependency Performance & ergonomics: - M-1: to_proto() -> into_proto() to avoid cloning - M-2/M-8: tracing::warn on deserialization failures in get() - M-3: Derive PartialEq on SearchAttributes - M-4: SearchAttributes::apply() for single-update mutations - M-5/M-6/M-7: Expanded documentation coverage API additions: - L-1: unwrap_or_default in updates_to_proto - L-3: Expanded docs for default_indexed_value_type - L-4: #[non_exhaustive] on SearchAttributeError - L-5: From<ProtoSearchAttributes> for owned conversion - Timestamp::new(), seconds(), nanos(), to_prost() public API Tests (12 new): - Timestamp clamping (negative nanos, excessive nanos) - Timestamp to_prost round-trip - SearchAttributes::apply (insert + remove) - From<ProtoSearchAttributes> owned conversion - PartialEq equality and inequality - From<Proto> trait matches from_proto method - Upsert read-after-write local state - Upsert unset removes from local state - Upsert multiple updates last-wins - Upsert merges with initial search attributes - WorkflowContextView returns typed search attributes Reviewed by: Principal Rust, Security, Distributed Systems, and Temporal engineers. 0 critical findings.
|
Should be ok enough now. I see some other improvements, but can add those later |
|
Please fixup the formatting and compile errors for |
|
🤦 -- of course. I thought was all working/clean locally. Let me get that fixed up |
- Run cargo fmt --all to fix formatting differences - Fix eager.rs: SearchAttributes .into() -> .into_proto() (same pattern as the other into_proto conversions)
chris-olszewski
left a comment
There was a problem hiding this comment.
It appears the test failure might be legitimate.
- upsert_search_attrs::sends_upsert: expected value 3→2 (workflow starts SA_INT=1, increments to 2; old raw API had extra WFT cycle) - timestamp_from_system_time: align nanos to 100ns boundary for Windows (SystemTime uses FILETIME with 100ns tick resolution, truncating 123_456_789→123_456_700)
|
Everything appears to be passing locally now fixed failure around Also, addressed a |
|
weird -- I'll take another pass through. Running locally didn't surface errors. I'll work on getting my local to be able to run the same as github CI. |
Replace `IndexedValueType::Unspecified | _` with explicit `IndexedValueType::Unspecified` match (reviewer's suggestion that was lost during rebase). This preserves exhaustive match checking so new proto variants cause compile errors rather than silently mapping to Unspecified.
Co-authored-by: Chris Olszewski <chrisdolszewski@gmail.com>
|
@chris-olszewski -- thanks for the little bits to help get this over the line. Do we think at some point it can make sense for me to be able to trigger CI. That seems a good way to remove you from the mix, and keep development more focused [ for contributors that are eventually trusted ]. |
Will raise with the team. |
I think this isn't likely. From a security perspective, the ability to kick off CI is arbitrary code execution inside of our github environment. Potentially there's a story where individuals get favored contributor status of some kind, but we'd need to work that out with our security and legal folks. |
|
Ya, @tconley1428 -- I get it. Just highlighting where could help reduce friction, increase velocity. Also, raises a question of whether non-employee contributors can do anything more than contribute code. Is there a community plan? How is merit/seniority/longer-term [ volunteer ] efforts to the project, acknowledged and rewarded. Often that is via sustained contributions -> commit permissions [ and/or kicking off CI ] -> something like a technical steering committee, etc etc. Though, temporal is not in an OSS foundation, so doesn't have as clear of conventions to follow. Worthwhile food for thought though -- which is def distinct from this thread. |
|
Yeah, definitely a place where we need to outline practices better. Despite being open source for quite some time, the volume of external contributions has ramped up substantially in the last 6 months or so, probably due in part to increasing popularity of the platform, but also due to the rise of AI coding. We're starting to draft some guidance for guidance for open source contributions here: temporalio/sdk-python#1625 (Very early ideation on the contents, feel free to comment where you have thoughts), and we're talking with our open source folks internally to figure out where we stand between "We own this, even though it is open source" and the other extreme of "Everything goes through open source committee and guidance, etc." Something that'll evolve over time I think and above my pay grade to decide. |
|
Again - completely understood. I've worked with OSPO offices. Well aware of the challenges. Pushing towards hopefully making some decisions :-p |
Summary
Adds a type-safe Search Attributes API to the Rust SDK, providing parity with Go, TypeScript, and Python SDKs.
Closes #1337
What Changed
Core Types (
crates/common-wasm/src/search_attributes.rs)SearchAttributeKey<T>— const-constructible typed keys for all 7 Temporal indexed value typesSearchAttributeValue— sealed trait implemented forbool,i64,f64,String,prost_types::Timestamp,Vec<String>TypedSearchAttributes— type-safe collection with.get()/.to_proto()/.from_proto()SearchAttributeUpdate— type-erased set/unset for heterogeneous upsertsmetadata["encoding"]=json/plain,metadata["type"]=IndexedValueType(matches Go SDK)Integration
WorkflowContext:typed_search_attributes()getter +upsert_typed_search_attributes()WorkflowStartOptions:typed_search_attributesfieldChildWorkflowOptions:typed_search_attributesfieldContinueAsNewOptions:typed_search_attributesfieldDesign Decisions
prost_types::Timestampfor datetime (already in dep graph), not chronosearch_attributesfields/methods unchanged; if both set, typed takes precedencecommon-wasm— shared across workflow + client crates via re-exportBefore / After
Testing
cargo check— 0 errors, 0 warnings