fix(stepfunctions-tasks): cannot specify part of execution data or task context as input to the RunLambda service integration
#7428
Conversation
…sk context as input to the service integration
The Lambda service integration requires an input field called `payload`.
We modeled this as a `{[key: string]: any}` which precludes the usage of
execution data or context data as inputs to a Lambda function.
Fix:
Change the type of `payload` to be `TaskInput` which has already modeled
a type union for task classes that accept multiple types of payload.
This will enable usage of literal strings, objects, execution data, and
task context.
Rationale:
Although this is a breaking change, the workarounds for enabling usage of
different types is not user friendly and incomplete as all of the types
above cannot be expressed in the current modeling of `payload`
Fixes #7371
BREAKING CHANGE:
`payload` in RunLambdaTask is now of type `TaskInput`.
You can migrate your current assignment to payload by supplying it to the `TaskInput.fromObject()` API
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
If this change does not reflect in the README in any way, that means there is a missing example in there.
I would expect the first example of invoking a Lambda in the README to show how to pass the entire execution state to the Lambda (just like the old Lambda invocation used to do).
| @@ -12,7 +12,7 @@ export interface RunLambdaTaskProps { | |||
| * | |||
| * @default - No payload | |||
| */ | |||
| readonly payload?: { [key: string]: any }; | |||
| readonly payload?: sfn.TaskInput; | |||
There was a problem hiding this comment.
Is it worth introducing a new field for this? (Let's say, input?: sfn.TaskInput) and @deprecateing the old one?
Are you okay taking the breakage?
Next question: since we're touching this anyway, does it make sense to change the @default while we're at it?
There was a problem hiding this comment.
I'm ok with the breaking change, given the migration is straight forward.
Why do you suggest changing the default? It seems to me that the current default makes sense, no?
There was a problem hiding this comment.
I don't see any problems with no payload being the default as long as there is a code example that shows how to set "Payload.$": "$" as @rix0rrr suggests.
$ (the entire input) could be a sensible default value, mostly out of convenience for users of the "old-style" InvokeFunction which always sent the effective input as the payload. I noted some caveats in #7371 (comment).
It may be worth adding test coverage for setting payload to $ since that's such a common use case.
There was a problem hiding this comment.
I'm also okay with taking the breakage here for the same reason as @nija-at
good point: I do think the @default should be the entire task input while we're at it as I think that's a more common usage than sending the empty object into a Lambda. I've left it as is for now, but happy to make that change
There was a problem hiding this comment.
I think we should optimize for the most common use case. What do we expect MOST people to do? @wong-a I would hope your team has some info on the most common ways people use each task type.
Furthermore, naively I would say that changing payload to $ by default is a non-breaking change. If people leave it empty, today they would get no parameters, so not expect them and not read them, so adding more parameters should be fine.
There's also the case of what the default outputPath should be. Feels like for maximum convenience it should be $.Payload, and similar for all other task types. But changing that is not backwards compatible so I guess that ship has sailed, huh...?
There was a problem hiding this comment.
Hey @wong-a, since we got you here anyway... what is the official term for the JSON blob that travels along with the execution, which is updated using inputPath, outputPath, etc?
I was struggling to find a good, clear and concise name for it while writing the library, so ultimately settled on something unsatisfying like Data. I wonder what you call it internally?
I guess we could do ExecutionState or something, but any term I can come up with feels so generic as to be worthless... :/
There was a problem hiding this comment.
What do we expect MOST people to do? @wong-a I would hope your team has some info on the most common ways people use each task type.
The old-style Lambda is the most used Task type, which uses the entire effective input and I think most customers would be happy with it as the default. If someone wants to pass an empty object as the payload they can easily do so by passing an empty object as the value of Payload.
what is the official term for the JSON blob that travels along with the execution, which is updated using inputPath, outputPath, etc?
There's no special name for it. It's generally referred to within the ASL spec and Step Functions documentation as "state input" or output.
There was a problem hiding this comment.
+1. I would further and say that a change to the README is useful given that this is a breaking change. We should have a couple of code snippets in the README of the common use cases. |
I've added some common cases. I'm in the middle of rewriting all of the Tasks section into |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@nija-at @rix0rrr - I started drafting the updated README for One thing I've noticed is inconsistency in how we name these task integrations. Some services have I guess it would be nice if the naming wasn't so heterogeneous. Do we want to align the naming? If we do, an alternative could be to rename RunLambdaTask to whatever we settle on. I personally like the |
Would be nice to align. If I was starting off, I would do We should be ok changing the names of more recent ones that were added (batch, dynamo, emr, etc.). We'll break fewer customers. I'm sure most people using stepfunctions will use lambda. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
I agree, I'll take a stab at a first approximation in another PR |
packages/@aws-cdk/aws-stepfunctions-tasks/test/lambda/integ.run-lambda.ts
Show resolved
Hide resolved
Co-Authored-By: Niranjan Jayakar <nija@amazon.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Commit Message
fix(stepfunctions-tasks): cannot specify part of execution data or task context as input to the
RunLambdaservice integration (#7428)The Lambda service integration requires an input field called
payload.We modeled this as a
{[key: string]: any}which precludes the usage ofexecution data or context data as inputs to a Lambda function.
Fix:
Change the type of
payloadto beTaskInputwhich has already modeleda type union for task classes that accept multiple types of payload.
This will enable usage of literal strings, objects, execution data, and
task context as input types supported for invoking a Lambda function.
Rationale:
Although this is a breaking change, the workarounds for enabling usage of
different types is not user friendly and incomplete as all of the types
above cannot be expressed in the current modeling of
payloadFixes #7371
BREAKING CHANGE:
payloadin RunLambdaTask is now of typeTaskInputand has a default of the state input instead of the empty object.You can migrate your current assignment to payload by supplying it to the
TaskInput.fromObject()APIEnd Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license