Deserialized Search Attributes API and full Test Server implementation for Search Attributes #1067

Spikhalskiy · 2022-03-08T01:24:13Z

What was changed

Test server now implements and verifies search attributes upserts and in start call on par with the real server
New methods are added that deserialize search attributes from Payload.

How was this tested

Unit tests covering new methods and test server functionality

Closes #673
Closes #617

Blocked by:

Spikhalskiy · 2022-04-12T01:12:00Z

temporal-sdk/src/main/java/io/temporal/internal/common/HistoryJsonUtils.java

@@ -83,26 +78,4 @@ private static String convertEnumValues(
    }
    return parsed.jsonString();
  }
-


This code was generalized and moved to ProtobufEnumNameUtils to be reused in Search Attributes code

cretz · 2022-04-12T13:36:09Z

temporal-sdk/src/main/java/io/temporal/common/SearchAttribute.java

+   * io.temporal.workflow.Workflow#upsertSearchAttributes(Map)} will lead to unsetting the search
+   * attribute with the corresponded name if any present.
+   */
+  public static final List<Object> UNSET_VALUE = Collections.emptyList();


To save me from trying to dig through the source here, how does this get represented in the temporal.api.common.v1.SearchAttributes API? As a "binary/null" payload? And the server knows that is the equivalent of a delete?

Also, is there never a case where I might want to set an empty list for my Keyword search attribute? Or from an ES perspective is an empty list equivalent to unset?

EDIT: I am now reading temporalio/temporal#561. My only concern now is we make this value future proof w/ whatever decision they might come to here.

The server doesn't explicitly delete on '[]', it persists an empty collection until that task is done. But it works effectively as a deletion already now, yes.
it's a json payload.

cretz · 2022-04-12T13:44:42Z

temporal-sdk/src/main/java/io/temporal/internal/common/ProtoEnumNameUtils.java

@@ -0,0 +1,80 @@
+/*
+ *  Copyright (C) 2020 Temporal Technologies, Inc. All Rights Reserved.


I personally have been fixing the year on these on new files but I understand that for Java specifically some of the other company names need to stay.

cretz · 2022-04-12T13:45:59Z

temporal-sdk/src/main/java/io/temporal/internal/common/ProtoEnumNameUtils.java

+
+/**
+ * Because in Go enums are in a shared namespace, protobuf best practices is to prefix the enums
+ * with enum type prefix to create unique names. The final result is verbose and when we use a


protobuf best practices is to prefix the enums with enum type prefix to create unique names.

FWIW, I don't believe that's true in the industry. We just happen to have written ours like that, but most people follow something similar to https://google.aip.dev/126. Not that it matters here because we have added that prefix, just mentioning it.

Well, in this AIP:
"The other values should not be prefixed by the name of the enum itself. This generally requires users to write MyState.MYSTATE_ACTIVE in their code, which is unnecessarily verbose."
But this is also right under:
"To avoid sharing values, APIs may prefix enum values with the name of the enum. In this case, they must do so consistently within the enum."

But I will get rid of "best practices" statement.

cretz · 2022-04-12T13:47:19Z

temporal-sdk/src/main/java/io/temporal/internal/common/ProtoEnumNameUtils.java

+  public static String uniqueToSimplifiedName(Enum<?> enumm) {
+    String protoEnumName = enumm.name();
+    String prefix = ENUM_CLASS_TO_PREFIX.get(enumm.getClass());
+    if (!protoEnumName.startsWith(prefix)) {


Is this safe if the map didn't have this class present? Or do we expect it is always present?

I will add an explicit exception, thanks for pointing out. It's not always safe, the enums have to be added.

cretz · 2022-04-12T13:58:18Z

temporal-sdk/src/main/java/io/temporal/internal/common/SearchAttributePayloadConverter.java

+                .collect(Collectors.toList());
+
+        if (indexValues.size() == 1) {
+          type = indexValues.get(0);


So a collection of integers is considered type INDEXED_VALUE_TYPE_INT for metadata purposes even though it may be keyword for SA? I think that makes sense, we just need to document this metadata value for all SDKs is the type of data inside the collection.

cretz · 2022-04-12T14:00:22Z

temporal-sdk/src/main/java/io/temporal/internal/common/SearchAttributePayloadConverter.java

+      return IndexedValueType.INDEXED_VALUE_TYPE_DOUBLE;
+    } else if (Boolean.class.equals(type)) {
+      return IndexedValueType.INDEXED_VALUE_TYPE_BOOL;
+    } else if (OffsetDateTime.class.equals(type)) {


Hrmm, I wonder if Instant or even java.util.Date should be supported? Probably not if we expect to be able to decode to the same type we receive.

Date is probably not doable safely looking at the underlying ES Type.

Instant is a really good thought, but yes, to your point, if we do Instant, we will have to return Instant ideally. And it will restrict us because OffsetDateTime is a more powerful class. But maybe I will actually support Instant on the upsert and will add a note that dates are always deserialized into OffsetDateTime.

cretz · 2022-04-12T14:04:21Z

temporal-sdk/src/main/java/io/temporal/internal/common/SearchAttributePayloadConverter.java

+  public Payload encode(@Nonnull Object obj) {
+    if (obj instanceof Collection && ((Collection<?>) obj).size() == 1) {
+      // always serialize an empty collection as one value
+      obj = ((Collection<?>) obj).iterator().next();


Hrmm, it is a bit confusing that if I encode a string list of size 1 and decode it I get a string but if I encode a string list of size 2 and decode it I get a list. If we did encode this as a single-item list, would the server just send us back a single item anyways?

cretz · 2022-04-12T14:13:53Z

temporal-sdk/src/main/java/io/temporal/internal/common/SearchAttributesUtil.java

+    if (isEmpty(searchAttributes)) {
+      return Collections.emptyMap();
+    }
+    Map<String, List<?>> deserializedMap = new HashMap<>();


Since the empty-collection type is rare, should probably give an initial capacity here

cretz · 2022-04-12T14:19:29Z

temporal-sdk/src/main/java/io/temporal/internal/sync/WorkflowInternal.java

+  public static <T> List<T> getSearchAttributeValues(String name) {
+    SearchAttributes searchAttributes = getRootWorkflowContext().getContext().getSearchAttributes();
+    if (searchAttributes == null) {
+      return null;


I wonder if this should return an empty list for no value. Won't getSearchAttributes map values be empty lists instead of null?

It seems we return null in all cases of no attributes, no attribute, and no attribute value. So they can't use nullability to differentiate set or not anyways. If not, I haven't checked, but we probably need to state in the javadoc that this can never return an empty list then.

Well, we use an empty list as a "delete" token, so I would day it's safer to deserialize it always to null to users.

If not, I haven't checked, but we probably need to state in the javadoc that this can never return an empty list then.

👍

cretz · 2022-04-12T14:56:39Z

Per off-PR discussion, marking approve with more off-PR discussion to come.

Specifically, here is the scenario in Java with this PR:

Encode
- string -> SA string
- list of one string -> SA string
- list of multiple strings -> SA list of multiple strings
Decode
- SA string -> list of one string (can be read as just string w/ separate getter though)
- SA list of multiple strings -> list of multiple strings (fails if trying to use that single-item getter though)

While this makes sense for Java, in Go I think we might not unwrap a list of one string as just a string but rather leave as list. Unsure about TypeScript.

bergundy · 2022-04-12T16:45:31Z

@cretz We haven't implemented search attributes in TS yet.
We will do that soon and always use Arrays when reading attributes and collections for writing.
We will also verify that the values are valid ElasticSearch types.

…n for Search Attributes

Spikhalskiy force-pushed the issue-617-1 branch 10 times, most recently from 59778e2 to 969ee08 Compare March 14, 2022 18:03

Spikhalskiy force-pushed the issue-617-1 branch 20 times, most recently from 71980f2 to 97bb9af Compare March 22, 2022 07:54

Spikhalskiy force-pushed the issue-617-1 branch 7 times, most recently from b431dbf to e0d3317 Compare April 12, 2022 01:06

Spikhalskiy commented Apr 12, 2022

View reviewed changes

Spikhalskiy force-pushed the issue-617-1 branch from e0d3317 to 6320485 Compare April 12, 2022 01:14

Spikhalskiy marked this pull request as ready for review April 12, 2022 01:26

Spikhalskiy requested review from mfateev, Sushisource, cretz, bergundy and mmcshane as code owners April 12, 2022 01:26

Spikhalskiy changed the title ~~Deserialize Search Attributes~~ Deserialized Search Attributes API and full Test Server implementation for Search Attributes Apr 12, 2022

cretz reviewed Apr 12, 2022

View reviewed changes

cretz approved these changes Apr 12, 2022

View reviewed changes

Spikhalskiy force-pushed the issue-617-1 branch from 6320485 to 606bf6d Compare April 12, 2022 16:00

Spikhalskiy force-pushed the issue-617-1 branch 4 times, most recently from 00b4753 to bb8e62b Compare April 12, 2022 20:10

Deserialized Search Attributes API and full Test Server implementatio…

dfe871a

…n for Search Attributes

Spikhalskiy force-pushed the issue-617-1 branch from bb8e62b to dfe871a Compare April 12, 2022 21:10

Spikhalskiy merged commit 47d881b into temporalio:master Apr 12, 2022

Spikhalskiy deleted the issue-617-1 branch April 15, 2022 19:59

cretz mentioned this pull request Jun 29, 2023

[Test server] ConcurrentModificationException on Workflow.upsertSearchAttributes in tests #1802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Deserialized Search Attributes API and full Test Server implementation for Search Attributes #1067

Deserialized Search Attributes API and full Test Server implementation for Search Attributes #1067

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -0,0 +1,80 @@
		/*
		* Copyright (C) 2020 Temporal Technologies, Inc. All Rights Reserved.

Deserialized Search Attributes API and full Test Server implementation for Search Attributes #1067

Deserialized Search Attributes API and full Test Server implementation for Search Attributes #1067

Uh oh!

Conversation

Uh oh!

What was changed

How was this tested

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!