fix(stepfunctions): improve Task payload encoding #2706
Conversation
Improve referencing data fields for StepFunctions tasks, in preparation of callback task implementaion. Get rid of `JsonPath`, and in its place we have 2 new classes: - `DataField`, for fields that come from the user payload (`$.My.Field`). Settle on the term "data" since that's the term used in most of StepFunctions' docs. - `ContextField`, for fields that come from the service-defined task "context" (like `$$.Execution.StartTime`, and in particular `$$.Task.Token`). These classes have been moved from the `-tasks` module to the `aws-stepfunctions` module, where it seems to make more sense for them to live. Add support for SQS and SNS tasks to publish an arbitrary JSON structure that can reference fields from context and execution data. Remove `NumberValue` since we can now encode Tokens in regular number values. BREAKING CHANGES: - `JsonPath.stringFromPath` (and others) are now called `DataField.fromStringAt()`. The `DataField` class now lives in the main stepfunctions module. - `SendToQueue` property `messageBody` => `message`. - `PublishToTopic` property `messageObject` used to take a pre-JSONified object, now directly accepts a JSON object. - Instead of passing `NumberValue`s to StepFunctions tasks, pass regular numbers.
| @@ -48,6 +55,9 @@ export interface SendToQueueProps { | |||
| */ | |||
| export class SendToQueue implements sfn.IStepFunctionsTask { | |||
| constructor(private readonly queue: sqs.IQueue, private readonly props: SendToQueueProps) { | |||
| if ((props.message === undefined) === (props.messageObject === undefined)) { | |||
There was a problem hiding this comment.
Perhaps a single required property of a union-like class instead of two optionals with runtime check?
[That seems like a repeating pattern between string and Map<string => any>. Maybe we can have a standard type for this?]
There was a problem hiding this comment.
I considered it, but I think it will be worse in the typical case. I'm expecting JSON object to be the common scenario and I'd hate to made that even harder to pass.
By the way, you're making the exact opposite argument in #2716. Which is it? :)
There was a problem hiding this comment.
I agree that this is not ideal, but The problem here is that it is a required argument, and having two optional arguments that are mutually exclusive results in poor IDE experience and we don’t have union types.
Makes me wonder if perhaps we can implement automatic support for union types in Jsii so that we can use unions in typescript and other languages will be able to generate union-like classes....
There was a problem hiding this comment.
That would solve so much.
There was a problem hiding this comment.
I did it for consistency with Event Targets. That does mean we now have this abomination:
const task2 = new sfn.Task(this, 'Send2', {
task: new tasks.SendToQueue(queue, {
messageBody: TaskInput.fromObject({
field1: 'somedata',
field2: Data.stringAt('$.field2'),
}),
// Only for FIFO queues
messageGroupId: '1234'
})
});Guess there's no getting around it... :x
There was a problem hiding this comment.
How would you do it?
The only issue is how to automatically come up with a name for the union type. Anonymous won't be possible and any automatic mechanism will not be backwards compatible. Don't know if we get enough information from the TypeScript compiler, otherwise we could maybe force type unions to only be allowed if they're declared first:
export type Foo = Bar | Baz;Then we can generate:
class Foo {
static fromBar(Bar bar): Foo { }
static fromBaz(Baz baz): Foo { }
Bar getBar() { }
Baz getBaz() { }
}
As an optimization, if the union type occurs in a props type for which we're already generating a builder, we could generate builder overloads like we do now:
class Builder {
Builder withFoo(Foo foo) { }
Builder withFoo(Bar foo) { }
Builder withFoo(Baz foo) { }
}
There was a problem hiding this comment.
Yes, we will need to have an official name for the union type and also come up with a way to render the names of the various methods in case the union includes primitives/lists/maps...
packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs-tasks.test.ts
Outdated
Show resolved
Hide resolved
| */ | ||
| public static containsTaskToken(obj?: {[key: string]: any}): boolean { | ||
| const paths = findReferencedPaths(obj); | ||
| return paths.has('$$.Task.Token') || paths.has('$$.Task') || paths.has('$$'); |
There was a problem hiding this comment.
I don't understand the idea here. As far as I know, it is mandatory for users to specify "$$.Task.Token" in the top level of "Payload".
There was a problem hiding this comment.
Mandated by whom? Will SFN reject the state machine definition if the token is not contained?
I'm asking because given $$.Task or $$, a user could still get to the value and send it in SendTaskSuccess. But if SFN's API will actively reject a a StateMachine definition that doesn't contain that field, then we have to cater to the strictest validation.
There was a problem hiding this comment.
Also, re: top-level, will SFN reject the following state machine definition:
"Parameters": {
"MessageBody": {
"Input": "Some String",
"MoreData": {
"Something": "$.Field",
"Token": "$$.Task.Token"
}
}
}
There was a problem hiding this comment.
Thank you for your explanation. I understand better now.
To correct my mistake:
Users can include "$$.Task.Token" anywhere under "Parameters". To pass this variable to the target service, the token should be included in the subfield "Payload" for Lambda, "MessageBody" for SQS, "Message" for SNS. So your example works:
"Parameters": {
"MessageBody": {
"Input": "Some String",
"MoreData": {
"Something.$": "$.Field",
"Token.$": "$$.Task.Token"
}
}
}
In addition, I checked in the code as well as tested myself, Step Functions does not reject if the token field is missing for callback(.waitForTaskToken) tasks.
There was a problem hiding this comment.
Cool. I will encourage the validation in the CDK, because we're all about early validation and early feedback here. But at least we can set up our own best practices then.
| @@ -0,0 +1,58 @@ | |||
| import { Context, Data } from "./fields"; | |||
Improve referencing data fields for StepFunctions tasks, in preparation
of callback task implementaion.
Get rid of
JsonPath, and in its place we have 2 new classes:Data, for fields that come from the user payload(
$.My.Field). Settle on the term "data" since that's theterm used in most of StepFunctions' docs.
Context, for fields that come from the service-definedtask "context" (like
$$.Execution.StartTime, and in particular$$.Task.Token).These classes have been moved from the
-tasksmodule to theaws-stepfunctionsmodule, where it seems to make more sense for themto live.
Add support for SQS and SNS tasks to publish an arbitrary JSON
structure that can reference fields from context and execution data.
Remove
NumberValuesince we can now encode Tokens in regular numbervalues.
BREAKING CHANGES:
JsonPath.stringFromPath(and others) are now calledData.stringAt().The
DataFieldclass now lives in the main stepfunctions module.PublishToTopicpropertymessageObjectused to take a pre-JSONifiedobject, now pass
sfn.TaskInput.fromObject()into themessagefield.NumberValues to StepFunctions tasks, pass regularnumbers.
Pull Request Checklist
designfolderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.