Skip to content

Commit 538508b

Browse files
Quinn-With-Two-NsQuinn-With-Two-Ns
authored
Apply data converter context in more places (#1896)
Add data converter context to memo, lastFailure and schedules
1 parent 4fe296e commit 538508b

File tree

8 files changed

+141
-23
lines changed

8 files changed

+141
-23
lines changed

‎temporal-sdk/src/main/java/io/temporal/client/WorkflowExecutionMetadata.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import io.temporal.common.converter.DataConverter;
3030
import io.temporal.internal.common.ProtobufTimeUtils;
3131
import io.temporal.internal.common.SearchAttributesUtil;
32+
import io.temporal.payload.context.WorkflowSerializationContext;
3233
import java.lang.reflect.Type;
3334
import java.time.Instant;
3435
import java.util.Collections;
@@ -123,7 +124,11 @@ public <T> T getMemo(String key, Class<T> valueClass, Type genericType) {
123124
if (memo == null) {
124125
return null;
125126
}
126-
return dataConverter.fromPayload(memo, valueClass, genericType);
127+
return dataConverter
128+
.withContext(
129+
new WorkflowSerializationContext(
130+
info.getParentNamespaceId(), info.getExecution().getWorkflowId()))
131+
.fromPayload(memo, valueClass, genericType);
127132
}
128133

129134
@Nonnull

‎temporal-sdk/src/main/java/io/temporal/internal/client/RootScheduleClientInvoker.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ public void createSchedule(CreateScheduleInput input) {
6868
.setSchedule(scheduleRequestHeader.scheduleToProto(input.getSchedule()));
6969

7070
if (input.getOptions().getMemo() != null) {
71+
// TODO we don't have a workflow context here, maybe we need a schedule context?
7172
request.setMemo(
7273
Memo.newBuilder()
7374
.putAllFields(

‎temporal-sdk/src/main/java/io/temporal/internal/client/RootWorkflowClientInvoker.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public WorkflowStartOutput start(WorkflowStartInput input) {
8383
(input.getOptions().getMemo() != null)
8484
? Memo.newBuilder()
8585
.putAllFields(
86-
intoPayloadMap(clientOptions.getDataConverter(), input.getOptions().getMemo()))
86+
intoPayloadMap(dataConverterWithWorkflowContext, input.getOptions().getMemo()))
8787
.build()
8888
: null;
8989

@@ -169,7 +169,7 @@ public WorkflowSignalWithStartOutput signalWithStart(WorkflowSignalWithStartInpu
169169
? Memo.newBuilder()
170170
.putAllFields(
171171
intoPayloadMap(
172-
clientOptions.getDataConverter(),
172+
dataConverterWithWorkflowContext,
173173
workflowStartInput.getOptions().getMemo()))
174174
.build()
175175
: null;

‎temporal-sdk/src/main/java/io/temporal/internal/client/ScheduleProtoUtil.java

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,13 @@
3737
import io.temporal.client.WorkflowOptions;
3838
import io.temporal.client.schedules.*;
3939
import io.temporal.common.context.ContextPropagator;
40+
import io.temporal.common.converter.DataConverter;
4041
import io.temporal.common.converter.EncodedValues;
4142
import io.temporal.internal.client.external.GenericWorkflowClient;
4243
import io.temporal.internal.common.ProtobufTimeUtils;
4344
import io.temporal.internal.common.RetryOptionsUtils;
4445
import io.temporal.internal.common.SearchAttributesUtil;
46+
import io.temporal.payload.context.WorkflowSerializationContext;
4547
import java.time.Instant;
4648
import java.util.*;
4749
import java.util.stream.Collectors;
@@ -82,6 +84,13 @@ private io.temporal.common.interceptors.Header extractContextsAndConvertToBytes(
8284
public ScheduleAction actionToProto(io.temporal.client.schedules.ScheduleAction action) {
8385
if (action instanceof ScheduleActionStartWorkflow) {
8486
ScheduleActionStartWorkflow startWorkflowAction = (ScheduleActionStartWorkflow) action;
87+
DataConverter dataConverterWithWorkflowContext =
88+
clientOptions
89+
.getDataConverter()
90+
.withContext(
91+
new WorkflowSerializationContext(
92+
clientOptions.getNamespace(),
93+
startWorkflowAction.getOptions().getWorkflowId()));
8594

8695
WorkflowOptions wfOptions = startWorkflowAction.getOptions();
8796
// Disallow some options
@@ -113,7 +122,7 @@ public ScheduleAction actionToProto(io.temporal.client.schedules.ScheduleAction
113122
ProtobufTimeUtils.toProtoDuration(wfOptions.getWorkflowTaskTimeout()))
114123
.setTaskQueue(TaskQueue.newBuilder().setName(wfOptions.getTaskQueue()).build());
115124

116-
startWorkflowAction.getArguments().setDataConverter(clientOptions.getDataConverter());
125+
startWorkflowAction.getArguments().setDataConverter(dataConverterWithWorkflowContext);
117126
Optional<Payloads> inputArgs = startWorkflowAction.getArguments().toPayloads();
118127
if (inputArgs.isPresent()) {
119128
workflowRequest.setInput(inputArgs.get());
@@ -128,7 +137,7 @@ public ScheduleAction actionToProto(io.temporal.client.schedules.ScheduleAction
128137
item.getKey(), ((EncodedValues) item.getValue()).toPayloads().get().getPayloads(0));
129138
} else {
130139
memo.put(
131-
item.getKey(), clientOptions.getDataConverter().toPayload(item.getValue()).get());
140+
item.getKey(), dataConverterWithWorkflowContext.toPayload(item.getValue()).get());
132141
}
133142
}
134143
workflowRequest.setMemo(Memo.newBuilder().putAllFields(memo).build());
@@ -388,12 +397,19 @@ public io.temporal.client.schedules.ScheduleAction protoToAction(@Nonnull Schedu
388397
Objects.requireNonNull(action);
389398
if (action.hasStartWorkflow()) {
390399
NewWorkflowExecutionInfo startWfAction = action.getStartWorkflow();
400+
DataConverter dataConverterWithWorkflowContext =
401+
clientOptions
402+
.getDataConverter()
403+
.withContext(
404+
new WorkflowSerializationContext(
405+
clientOptions.getNamespace(), startWfAction.getWorkflowId()));
406+
391407
ScheduleActionStartWorkflow.Builder builder = ScheduleActionStartWorkflow.newBuilder();
392408
builder.setWorkflowType(startWfAction.getWorkflowType().getName());
393409

394410
builder.setRawArguments(
395411
new EncodedValues(
396-
Optional.of(startWfAction.getInput()), clientOptions.getDataConverter()));
412+
Optional.of(startWfAction.getInput()), dataConverterWithWorkflowContext));
397413

398414
WorkflowOptions.Builder wfOptionsBuilder = WorkflowOptions.newBuilder();
399415
// set required options
@@ -420,7 +436,7 @@ public io.temporal.client.schedules.ScheduleAction protoToAction(@Nonnull Schedu
420436
EncodedValues encodedMemo =
421437
new EncodedValues(
422438
Optional.of(Payloads.newBuilder().addPayloads(memo.getValue()).build()),
423-
clientOptions.getDataConverter());
439+
dataConverterWithWorkflowContext);
424440
memos.put(memo.getKey(), encodedMemo);
425441
}
426442
wfOptionsBuilder.setMemo(memos);

‎temporal-sdk/src/main/java/io/temporal/internal/sync/SyncWorkflowContext.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1008,6 +1008,10 @@ public DataConverter getDataConverter() {
10081008
return dataConverter;
10091009
}
10101010

1011+
public DataConverter getDataConverterWithCurrentWorkflowContext() {
1012+
return dataConverterWithCurrentWorkflowContext;
1013+
}
1014+
10111015
boolean isReplaying() {
10121016
return replayContext.isReplaying();
10131017
}

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -555,7 +555,7 @@ public static <T> T getMemo(String key, Class<T> valueClass, Type genericType) {
555555
return null;
556556
}
557557

558-
return getDataConverter().fromPayload(memo, valueClass, genericType);
558+
return getDataConverterWithCurrentWorkflowContext().fromPayload(memo, valueClass, genericType);
559559
}
560560

561561
public static <R> R retry(
@@ -693,20 +693,24 @@ public static io.temporal.common.SearchAttributes getTypedSearchAttributes() {
693693
}
694694

695695
public static void upsertSearchAttributes(Map<String, ?> searchAttributes) {
696-
assertNotReadOnly("upset search attribute");
696+
assertNotReadOnly("upsert search attribute");
697697
getWorkflowOutboundInterceptor().upsertSearchAttributes(searchAttributes);
698698
}
699699

700700
public static void upsertTypedSearchAttributes(
701701
SearchAttributeUpdate<?>... searchAttributeUpdates) {
702-
assertNotReadOnly("upset search attribute");
702+
assertNotReadOnly("upsert search attribute");
703703
getWorkflowOutboundInterceptor().upsertTypedSearchAttributes(searchAttributeUpdates);
704704
}
705705

706706
public static DataConverter getDataConverter() {
707707
return getRootWorkflowContext().getDataConverter();
708708
}
709709

710+
static DataConverter getDataConverterWithCurrentWorkflowContext() {
711+
return getRootWorkflowContext().getDataConverterWithCurrentWorkflowContext();
712+
}
713+
710714
/**
711715
* Name of the workflow type the interface defines. It is either the interface short name * or
712716
* value of {@link WorkflowMethod#name()} parameter.
@@ -723,7 +727,7 @@ public static Optional<Exception> getPreviousRunFailure() {
723727
return Optional.ofNullable(getRootWorkflowContext().getReplayContext().getPreviousRunFailure())
724728
// Temporal Failure Values are additional user payload and serialized using user data
725729
// converter
726-
.map(f -> getDataConverter().failureToException(f));
730+
.map(f -> getDataConverterWithCurrentWorkflowContext().failureToException(f));
727731
}
728732

729733
private static WorkflowOutboundCallsInterceptor getWorkflowOutboundInterceptor() {

‎temporal-sdk/src/main/java/io/temporal/payload/context/HasWorkflowSerializationContext.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ public interface HasWorkflowSerializationContext extends SerializationContext {
4141
* @return workflowId of the Workflow Execution the Serialization Target belongs to. If the Target
4242
* is a Workflow itself, this method will return the Target's Workflow ID (not the ID of the
4343
* parent workflow).
44+
* <p>WARNING: When used in the context of a schedule workflow the workflowId may differ on
45+
* serialization and deserialization.
4446
*/
4547
String getWorkflowId();
4648
}

‎temporal-sdk/src/test/java/io/temporal/functional/serialization/WorkflowIdSignedPayloadsTest.java

Lines changed: 98 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,34 +21,39 @@
2121
package io.temporal.functional.serialization;
2222

2323
import static org.junit.Assert.*;
24+
import static org.junit.Assume.assumeFalse;
25+
import static org.junit.Assume.assumeTrue;
2426

27+
import com.google.common.collect.ImmutableMap;
2528
import com.google.protobuf.ByteString;
2629
import io.temporal.activity.*;
2730
import io.temporal.api.common.v1.Payload;
28-
import io.temporal.client.WorkflowClientOptions;
29-
import io.temporal.common.converter.CodecDataConverter;
30-
import io.temporal.common.converter.DataConverter;
31-
import io.temporal.common.converter.DefaultDataConverter;
32-
import io.temporal.common.converter.EncodingKeys;
31+
import io.temporal.api.common.v1.WorkflowExecution;
32+
import io.temporal.client.*;
33+
import io.temporal.client.schedules.*;
34+
import io.temporal.common.converter.*;
35+
import io.temporal.failure.CanceledFailure;
3336
import io.temporal.payload.codec.PayloadCodec;
3437
import io.temporal.payload.codec.PayloadCodecException;
3538
import io.temporal.payload.context.ActivitySerializationContext;
3639
import io.temporal.payload.context.HasWorkflowSerializationContext;
3740
import io.temporal.payload.context.SerializationContext;
41+
import io.temporal.testing.internal.SDKTestOptions;
3842
import io.temporal.testing.internal.SDKTestWorkflowRule;
43+
import io.temporal.workflow.ChildWorkflowOptions;
44+
import io.temporal.workflow.ContinueAsNewOptions;
3945
import io.temporal.workflow.Workflow;
46+
import io.temporal.workflow.shared.TestWorkflowWithCronScheduleImpl;
4047
import io.temporal.workflow.shared.TestWorkflows;
4148
import java.io.IOException;
4249
import java.time.Duration;
43-
import java.util.Arrays;
44-
import java.util.Collections;
45-
import java.util.List;
46-
import java.util.Optional;
50+
import java.util.*;
4751
import java.util.stream.Collectors;
4852
import javax.annotation.Nonnull;
4953
import javax.annotation.Nullable;
5054
import org.junit.Rule;
5155
import org.junit.Test;
56+
import org.junit.rules.TestName;
5257

5358
/**
5459
* This test emulates a scenario when users may be using WorkflowId in their encoding to sign every
@@ -58,6 +63,9 @@
5863
* explode on decoding.
5964
*/
6065
public class WorkflowIdSignedPayloadsTest {
66+
private static final String MEMO_KEY = "testKey";
67+
private static final String MEMO_VALUE = "testValue";
68+
private static final Map<String, Object> MEMO = ImmutableMap.of(MEMO_KEY, MEMO_VALUE);
6169
private final SimpleActivity heartbeatingActivity = new HeartbeatingIfNotLocalActivityImpl();
6270
private final ManualCompletionActivity manualCompletionActivity =
6371
new ManualCompletionActivityImpl();
@@ -70,19 +78,90 @@ public class WorkflowIdSignedPayloadsTest {
7078
@Rule
7179
public SDKTestWorkflowRule testWorkflowRule =
7280
SDKTestWorkflowRule.newBuilder()
73-
.setWorkflowTypes(SimpleWorkflowWithAnActivity.class)
81+
.setWorkflowTypes(
82+
SimpleWorkflowWithAnActivity.class, TestWorkflowWithCronScheduleImpl.class)
7483
.setWorkflowClientOptions(
7584
WorkflowClientOptions.newBuilder().setDataConverter(codecDataConverter).build())
7685
.setActivityImplementations(heartbeatingActivity, manualCompletionActivity)
7786
.build();
7887

88+
@Rule public TestName testName = new TestName();
89+
7990
@Test
8091
public void testSimpleWorkflowWithAnActivity() {
8192
TestWorkflows.TestWorkflow1 workflowStub =
8293
testWorkflowRule.newWorkflowStubTimeoutOptions(TestWorkflows.TestWorkflow1.class);
8394
assertEquals("result", workflowStub.execute("input"));
8495
}
8596

97+
@Test
98+
public void testSimpleWorkflowWithMemo() throws InterruptedException {
99+
assumeTrue(
100+
"skipping as test server does not support list", SDKTestWorkflowRule.useExternalService);
101+
102+
WorkflowOptions options =
103+
SDKTestOptions.newWorkflowOptionsWithTimeouts(testWorkflowRule.getTaskQueue());
104+
options = WorkflowOptions.newBuilder(options).setMemo(MEMO).build();
105+
TestWorkflows.TestWorkflow1 workflowStub =
106+
testWorkflowRule
107+
.getWorkflowClient()
108+
.newWorkflowStub(TestWorkflows.TestWorkflow1.class, options);
109+
assertEquals("result", workflowStub.execute("input"));
110+
WorkflowExecution execution = WorkflowStub.fromTyped(workflowStub).getExecution();
111+
String workflowId = execution.getWorkflowId();
112+
String runId = execution.getRunId();
113+
114+
// listWorkflowExecutions is Visibility API
115+
// Temporal Visibility has latency and is not transactional with the Server API call
116+
Thread.sleep(4_000);
117+
118+
List<WorkflowExecutionMetadata> executions =
119+
testWorkflowRule
120+
.getWorkflowClient()
121+
.listExecutions("WorkflowId = '" + workflowId + "' AND " + " RunId = '" + runId + "'")
122+
.collect(Collectors.toList());
123+
assertEquals(1, executions.size());
124+
assertEquals(MEMO_VALUE, executions.get(0).getMemo(MEMO_KEY, String.class));
125+
}
126+
127+
@Test
128+
public void testSimpleCronWorkflow() {
129+
assumeFalse("skipping as test will timeout", SDKTestWorkflowRule.useExternalService);
130+
131+
WorkflowOptions options =
132+
SDKTestOptions.newWorkflowOptionsWithTimeouts(testWorkflowRule.getTaskQueue());
133+
options =
134+
WorkflowOptions.newBuilder(options)
135+
.setWorkflowRunTimeout(Duration.ofHours(1))
136+
.setCronSchedule("0 */6 * * *")
137+
.build();
138+
TestWorkflows.TestWorkflowWithCronSchedule workflow =
139+
testWorkflowRule
140+
.getWorkflowClient()
141+
.newWorkflowStub(TestWorkflows.TestWorkflowWithCronSchedule.class, options);
142+
143+
testWorkflowRule.registerDelayedCallback(
144+
Duration.ofDays(1), WorkflowStub.fromTyped(workflow)::cancel);
145+
WorkflowClient.start(workflow::execute, testName.getMethodName());
146+
147+
try {
148+
workflow.execute(testName.getMethodName());
149+
fail("unreachable");
150+
} catch (WorkflowFailedException e) {
151+
assertTrue(e.getCause() instanceof CanceledFailure);
152+
}
153+
154+
Map<Integer, String> lastCompletionResults =
155+
TestWorkflowWithCronScheduleImpl.lastCompletionResults.get(testName.getMethodName());
156+
assertEquals(4, lastCompletionResults.size());
157+
// Run 3 failed. So on run 4 we get the last completion result from run 2.
158+
assertEquals("run 2", lastCompletionResults.get(4));
159+
// The last failure ought to be the one from run 3
160+
assertTrue(TestWorkflowWithCronScheduleImpl.lastFail.isPresent());
161+
assertTrue(
162+
TestWorkflowWithCronScheduleImpl.lastFail.get().getMessage().contains("simulated error"));
163+
}
164+
86165
@ActivityInterface
87166
public interface SimpleActivity {
88167
@ActivityMethod(name = "simple")
@@ -159,14 +238,21 @@ public String execute(String input) {
159238
assertEquals("result", result);
160239
// Child Workflow
161240
if (!Workflow.getInfo().getParentWorkflowId().isPresent()) {
241+
ChildWorkflowOptions childOptions = ChildWorkflowOptions.newBuilder().setMemo(MEMO).build();
162242
TestWorkflows.TestWorkflow1 child =
163-
Workflow.newChildWorkflowStub(TestWorkflows.TestWorkflow1.class);
243+
Workflow.newChildWorkflowStub(TestWorkflows.TestWorkflow1.class, childOptions);
164244
result = child.execute(input);
165245
assertEquals("result", result);
166246
}
247+
// Memo
248+
String memoValue = (String) Workflow.getMemo(MEMO_KEY, String.class);
249+
if (memoValue != null) {
250+
assertEquals(MEMO_VALUE, memoValue);
251+
}
167252
// continueAsNew
168253
if (!Workflow.getInfo().getContinuedExecutionRunId().isPresent()) {
169-
Workflow.continueAsNew(input);
254+
ContinueAsNewOptions casOptions = ContinueAsNewOptions.newBuilder().setMemo(MEMO).build();
255+
Workflow.continueAsNew(casOptions, input);
170256
}
171257
return result;
172258
}

0 commit comments

Comments
 (0)