Typed search attributes #1612
Typed search attributes #1612
Conversation
| * Additional indexed information attached to the Schedule. More info: | ||
| * https://docs.temporal.io/docs/typescript/search-attributes | ||
| * | ||
| * Values are always converted using {@link JsonPayloadConverter}, even when a custom Data Converter is provided. |
There was a problem hiding this comment.
We should also mention that SA don't get encoded (i.e. they don't go through PayloadCodec). That's important as people may not realize that there are security implication in using SA. Please make sure that's fact is clear everywhere that a user would supply search attributes.
There was a problem hiding this comment.
Added small note that users shouldn't include sensitive information in SAs
| @@ -342,6 +354,7 @@ export async function decodeScheduleAction( | |||
| throw new TypeError('Unsupported schedule action'); | |||
| } | |||
|
|
|||
| // TODO(thomas): move to internal package (or at least to the helpers.ts file) | |||
There was a problem hiding this comment.
Please also create encodeSearchAttributes and encodeTypedSearchAttributes help functions. Move those four helper functions, and anything else that is specific to encoding/decoding SAs, to common/src/converter/payload-search-attributes.ts. And make sure to use those helpers everywhere (e.g. workflow-client, schedule-client, workflow/src/internals.ts, etc).
There was a problem hiding this comment.
Actually, it might even be desirable to have an encodeSearchAttributes function, which accepts both untyped and typed SAs. There's some little details in how to properly do that; duplicating that code is undesirable.
There was a problem hiding this comment.
Done.
Yeah that reduced a lot of the duplicated code, didn't realize how much was being re-written.
Wrote encodeUnifiedSearchAttributes as a single encoding function for both forms of search attributes.
| throw new ValueError('Could not convert typed search attribute to payload'); | ||
| } | ||
| // Note: this shouldn't be the case but the compiler complains without this check. | ||
| if (payload.metadata === undefined || payload.metadata === null) { |
There was a problem hiding this comment.
Please you the following idiom whenever you get this:
| if (payload.metadata === undefined || payload.metadata === null) { | |
| if (payload.metadata == null) { |
For reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/null#difference_between_null_and_undefined
| const type = typedSearchAttribute[0]; | ||
| let value = typedSearchAttribute[1]; | ||
| if (type === 'DATETIME') { |
There was a problem hiding this comment.
| const type = typedSearchAttribute[0]; | |
| let value = typedSearchAttribute[1]; | |
| if (type === 'DATETIME') { | |
| let [type, value] = typedSearchAttribute; | |
| if (type === 'DATETIME') { |
There was a problem hiding this comment.
I had it like this initially, but the linter complains because type never changes and wants it to be const, hence this awkwardness.
| let value = this.jsonConverter.fromPayload(payload); | ||
| // If no 'type' metadata field or no given value, we skip. | ||
| if (payload.metadata.type === undefined || value === undefined) { | ||
| return undefined as T; | ||
| } | ||
| const type = toSearchAttributeType(decode(payload.metadata.type)); |
There was a problem hiding this comment.
In that case, better make the return type of this function T | undefined, to make it clear that the caller has the responsibility of handling that possibility.
There was a problem hiding this comment.
I can't change the type of this function because it breaks the interface . The PayloadConverter interface definition seems very restrictive, the way it's typed makes it difficult to get much type-safety in the toPayload and fromPayload definitions.
In any case, I left a comment noting why this was the case and what the suggested usage is. Not clear to me how to make this better.
| * | ||
| * Values are always converted using {@link JsonPayloadConverter}, even when a custom Data Converter is provided. | ||
| */ | ||
| typedSearchAttributes?: TypedSearchAttributePair[]; |
There was a problem hiding this comment.
I think that one will be tricky on update. We'd be receiving existing SAs, which would be an object rather than an array of Pair. We'll discuss that on Monday.
There was a problem hiding this comment.
Changed to TypedSearchAttributePair[] | TypedSearchAttributes as discussed offline.
I like this much more.
- remove ITypedSearchAttributes, expose concrete class TypedSearchAttributes - update constructor to take a list of pairs, consistent with the usage of pairs and class throughout the API - `Options` objects (i.e. user input) allow for both pairs and the concrete class as input types - created `TypedSearchAttributeUpdate`/`Pair` types, to be used for upserts and updates, the only difference with these types and the existing `TypedSearchAttribute/Pair` types is that they except `null` values (for deletion) - fixed the `upsert` method to accomodate these types - `updateSearchAttributes` now returns a new copy of `TypedSearchAttributes` to avoid mutation of existing `TypedSearchAttributes` objects - a myriad of small changes to accomodate the type changes
|
Most of the necessary revisions are done, fixing failing tests and adding new ones. |
Type enforcement for typed search attributes
What was changed
Why?
Checklist
Closes
How was this tested: