Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typed search attributes #1612

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Typed search attributes #1612

wants to merge 8 commits into from

Conversation

THardy98
Copy link
Contributor

@THardy98 THardy98 commented Jan 24, 2025

What was changed

Introduces TypedSearchAttributes to the Typescript SDK, allowing users to supply and receive type-enforced search attributes. TypedSearchAttributes are intended to replace the existing (not type-enforced) SearchAttributes which have been marked as @deprecated in its existing usages.

Why?

Brings parity with other SDKs which have a typed search attribute implementation.
Brings parity to how the server represents search attributes.
Provides a typed, structured interface to use search attributes.

For reviewers

Points of interest:

  • typed-search-attributes.ts contains the API
  • test-typed-search-attributes.ts contains tests and demonstrates API usage
  • worflow.ts for changes to upsertSearchAttributes (mutations to search attributes in workflow info)
  • payload-search-attributes.ts contains decoding/encoding logic for search attributes (both legacy and typed)

  1. Closes [Feature Request] Typed Search Attributes #1232

  2. How was this tested:
    Added tests, see test-typed-search-attributes.ts

  3. Any docs updates needed?
    Likely as we are deprecating existing SearchAttributes

@THardy98 THardy98 requested a review from a team as a code owner January 24, 2025 13:39
@THardy98 THardy98 marked this pull request as draft January 24, 2025 13:39
@THardy98 THardy98 requested a review from mjameswh January 24, 2025 16:54
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@THardy98 THardy98 Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please you the following idiom whenever you get this:

Suggested change
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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 344 to 346
const type = typedSearchAttribute[0];
let value = typedSearchAttribute[1];
if (type === 'DATETIME') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const type = typedSearchAttribute[0];
let value = typedSearchAttribute[1];
if (type === 'DATETIME') {
let [type, value] = typedSearchAttribute;
if (type === 'DATETIME') {

Copy link
Contributor Author

@THardy98 THardy98 Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it like this initially, but the linter complains because type never changes and wants it to be const, hence this awkwardness.

Comment on lines 371 to 376
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to TypedSearchAttributePair[] | TypedSearchAttributes as discussed offline.

I like this much more.

@THardy98
Copy link
Contributor Author

Most of the necessary revisions are done, fixing failing tests and adding new ones.

@THardy98 THardy98 marked this pull request as ready for review February 6, 2025 08:54
- 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
@THardy98 THardy98 force-pushed the typed-search-attributes branch from e6c6b31 to af0754e Compare February 6, 2025 18:29
@THardy98
Copy link
Contributor Author

THardy98 commented Feb 6, 2025

Updated the PR description with an overview for reviewers.

Some remaining points of uncertainty:

  • a couple TODO comments in places where I follow an existing pattern but am unsure if this is still the case (particularly those in payload-search-attributes.ts and internals.ts
  • I serialized expected responses in tests (i.e. JSON.parse(JSON.stringify(...))) to handle JSON converted responses from query. Figured this was okay, but just want to double-check
  • wondering if it's worth to test the isTypeSearchAttributePair type guard directly, or if the existing tests are sufficient

@THardy98 THardy98 force-pushed the typed-search-attributes branch from 9d727b3 to 3877147 Compare February 6, 2025 23:54
@THardy98 THardy98 force-pushed the typed-search-attributes branch from 3877147 to 3261083 Compare February 7, 2025 00:09
@THardy98 THardy98 requested a review from mjameswh February 7, 2025 00:32
@THardy98
Copy link
Contributor Author

THardy98 commented Feb 7, 2025

Who do I need to get in touch with to update docs? (considering we're deprecating a user-facing feature in exchange for another)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Typed Search Attributes
2 participants