Skip to content

Commit 4d2f165

Browse files
authored
Make Proto Mappers Unopinionated (#1038)
Proto objects throw a NullPointerException if you pass a null value into any setFoo method. Our current mappers make strong assumptions about what fields are and aren't necessary, and any time they don't match either the server or the client logic we get a vague NullPointerException. Instead, map all objects and don't assume that any field is non-null. The server or client should enforce the validity of the requests rather than the mapper. Additionally don't mutate Maps that we receive from Thrift/Proto in WorkflowContext.
1 parent fa8456a commit 4d2f165

File tree

4 files changed

+415
-223
lines changed

4 files changed

+415
-223
lines changed

src/main/java/com/uber/cadence/internal/compatibility/proto/DecisionMapper.java

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ static Decision decision(com.uber.cadence.Decision d) {
7777
d.getScheduleActivityTaskDecisionAttributes();
7878
ScheduleActivityTaskDecisionAttributes.Builder builder =
7979
ScheduleActivityTaskDecisionAttributes.newBuilder()
80-
.setActivityId(attr.getActivityId())
8180
.setActivityType(activityType(attr.getActivityType()))
8281
.setTaskList(taskList(attr.getTaskList()))
8382
.setInput(payload(attr.getInput()))
@@ -89,6 +88,9 @@ static Decision decision(com.uber.cadence.Decision d) {
8988
.setHeartbeatTimeout(secondsToDuration(attr.getHeartbeatTimeoutSeconds()))
9089
.setHeader(header(attr.getHeader()))
9190
.setRequestLocalDispatch(attr.isRequestLocalDispatch());
91+
if (attr.getActivityId() != null) {
92+
builder.setActivityId(attr.getActivityId());
93+
}
9294
if (attr.getRetryPolicy() != null) {
9395
builder.setRetryPolicy(retryPolicy(attr.getRetryPolicy()));
9496
}
@@ -102,19 +104,27 @@ static Decision decision(com.uber.cadence.Decision d) {
102104
{
103105
com.uber.cadence.RequestCancelActivityTaskDecisionAttributes attr =
104106
d.getRequestCancelActivityTaskDecisionAttributes();
105-
decision.setRequestCancelActivityTaskDecisionAttributes(
106-
RequestCancelActivityTaskDecisionAttributes.newBuilder()
107-
.setActivityId(attr.getActivityId()));
107+
108+
RequestCancelActivityTaskDecisionAttributes.Builder builder =
109+
RequestCancelActivityTaskDecisionAttributes.newBuilder();
110+
if (attr.getActivityId() != null) {
111+
builder.setActivityId(attr.getActivityId());
112+
}
113+
decision.setRequestCancelActivityTaskDecisionAttributes(builder.build());
108114
}
109115
break;
110116
case StartTimer:
111117
{
112118
com.uber.cadence.StartTimerDecisionAttributes attr = d.getStartTimerDecisionAttributes();
113-
decision.setStartTimerDecisionAttributes(
119+
StartTimerDecisionAttributes.Builder builder =
114120
StartTimerDecisionAttributes.newBuilder()
115-
.setTimerId(attr.getTimerId())
116121
.setStartToFireTimeout(
117-
secondsToDuration(longToInt(attr.getStartToFireTimeoutSeconds()))));
122+
secondsToDuration(longToInt(attr.getStartToFireTimeoutSeconds())));
123+
if (attr.getTimerId() != null) {
124+
builder.setTimerId(attr.getTimerId());
125+
}
126+
127+
decision.setStartTimerDecisionAttributes(builder.build());
118128
}
119129
break;
120130
case CompleteWorkflowExecution:
@@ -139,8 +149,12 @@ static Decision decision(com.uber.cadence.Decision d) {
139149
{
140150
com.uber.cadence.CancelTimerDecisionAttributes attr =
141151
d.getCancelTimerDecisionAttributes();
142-
decision.setCancelTimerDecisionAttributes(
143-
CancelTimerDecisionAttributes.newBuilder().setTimerId(attr.getTimerId()));
152+
CancelTimerDecisionAttributes.Builder builder =
153+
CancelTimerDecisionAttributes.newBuilder();
154+
if (attr.getTimerId() != null) {
155+
builder.setTimerId(attr.getTimerId());
156+
}
157+
decision.setCancelTimerDecisionAttributes(builder.build());
144158
}
145159
break;
146160
case CancelWorkflowExecution:
@@ -158,9 +172,11 @@ static Decision decision(com.uber.cadence.Decision d) {
158172
d.getRequestCancelExternalWorkflowExecutionDecisionAttributes();
159173
RequestCancelExternalWorkflowExecutionDecisionAttributes.Builder builder =
160174
RequestCancelExternalWorkflowExecutionDecisionAttributes.newBuilder()
161-
.setDomain(attr.getDomain())
162175
.setWorkflowExecution(workflowRunPair(attr.getWorkflowId(), attr.getRunId()))
163176
.setChildWorkflowOnly(attr.isChildWorkflowOnly());
177+
if (attr.getDomain() != null) {
178+
builder.setDomain(attr.getDomain());
179+
}
164180
if (attr.getControl() != null) {
165181
builder.setControl(arrayToByteString(attr.getControl()));
166182
}
@@ -203,8 +219,6 @@ static Decision decision(com.uber.cadence.Decision d) {
203219
d.getStartChildWorkflowExecutionDecisionAttributes();
204220
StartChildWorkflowExecutionDecisionAttributes.Builder builder =
205221
StartChildWorkflowExecutionDecisionAttributes.newBuilder()
206-
.setDomain(attr.getDomain())
207-
.setWorkflowId(attr.getWorkflowId())
208222
.setWorkflowType(workflowType(attr.getWorkflowType()))
209223
.setTaskList(taskList(attr.getTaskList()))
210224
.setInput(payload(attr.getInput()))
@@ -217,6 +231,12 @@ static Decision decision(com.uber.cadence.Decision d) {
217231
.setHeader(header(attr.getHeader()))
218232
.setMemo(memo(attr.getMemo()))
219233
.setSearchAttributes(searchAttributes(attr.getSearchAttributes()));
234+
if (attr.getDomain() != null) {
235+
builder.setDomain(attr.getDomain());
236+
}
237+
if (attr.getWorkflowId() != null) {
238+
builder.setWorkflowId(attr.getWorkflowId());
239+
}
220240
if (attr.getRetryPolicy() != null) {
221241
builder.setRetryPolicy(retryPolicy(attr.getRetryPolicy()));
222242
}
@@ -235,11 +255,15 @@ static Decision decision(com.uber.cadence.Decision d) {
235255
d.getSignalExternalWorkflowExecutionDecisionAttributes();
236256
SignalExternalWorkflowExecutionDecisionAttributes.Builder builder =
237257
SignalExternalWorkflowExecutionDecisionAttributes.newBuilder()
238-
.setDomain(attr.getDomain())
239258
.setWorkflowExecution(workflowExecution(attr.getExecution()))
240-
.setSignalName(attr.getSignalName())
241259
.setInput(payload(attr.getInput()))
242260
.setChildWorkflowOnly(attr.isChildWorkflowOnly());
261+
if (attr.getDomain() != null) {
262+
builder.setDomain(attr.getDomain());
263+
}
264+
if (attr.getSignalName() != null) {
265+
builder.setSignalName(attr.getSignalName());
266+
}
243267
if (attr.getControl() != null) {
244268
builder.setControl(arrayToByteString(attr.getControl()));
245269
}
@@ -259,11 +283,14 @@ static Decision decision(com.uber.cadence.Decision d) {
259283
{
260284
com.uber.cadence.RecordMarkerDecisionAttributes attr =
261285
d.getRecordMarkerDecisionAttributes();
262-
decision.setRecordMarkerDecisionAttributes(
286+
RecordMarkerDecisionAttributes.Builder builder =
263287
RecordMarkerDecisionAttributes.newBuilder()
264-
.setMarkerName(attr.getMarkerName())
265288
.setDetails(payload(attr.getDetails()))
266-
.setHeader(header(attr.getHeader())));
289+
.setHeader(header(attr.getHeader()));
290+
if (attr.getMarkerName() != null) {
291+
builder.setMarkerName(attr.getMarkerName());
292+
}
293+
decision.setRecordMarkerDecisionAttributes(builder.build());
267294
}
268295
break;
269296
default:

0 commit comments

Comments
 (0)