-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(stepfunctions): add support JSONata and variables #32343
base: main
Are you sure you want to change the base?
feat(stepfunctions): add support JSONata and variables #32343
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave it a first pass-through. Good stuff! 😸
One thing we enforce is that a state machine whose QueryLanguage
is set to JSONata
will not accept any states with JSONPath
as their QueryLanguage
. Can this also be enforced here?
@melalawi Thank you for your reviewing😀 aws-cdk/packages/aws-cdk-lib/aws-stepfunctions/lib/states/state.ts Lines 566 to 576 in 08d810d
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this huuge effort to enable JSONata
with State Machine Chainable constructs. Appreciate the effort.
I'm still going through some of the changes, but overall it looks good and I don't see any breaking changes. I left some minor feedback on the parts that I've reviewed.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
…com/WinterYukky/aws-cdk into feat/step-functions/support-jsonata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good but the amount of integ tests to cover the JSONata
usage is a bit worrying to me. I would like to request add some more integ tests and happy to approve.
@@ -556,23 +562,23 @@ function validateJsonPath(path: string) { | |||
]; | |||
const intrinsicFunctionFullNames = intrinsicFunctionNames.map((fn) => `States.${fn}`); | |||
if (path !== '$' | |||
&& !path.startsWith('$.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course. Until now, JSONPath only supported paths starting with $.
or $$.
(e.g. $.foo.bar
). If any other path is specified, a validation error is thrown.
Variables, on the other hand, start with $
without a dot (e.g. $foo.bar
). Without this change, you will be left unable to specify variables in JSONPath.
const stateMachine = new sfn.StateMachine(stack, 'StateMachine', { | ||
definitionBody: sfn.DefinitionBody.fromChainable( | ||
sfn.Pass.jsonPath(stack, 'JsonPathPass').next( | ||
sfn.Pass.jsonata(stack, 'JsonataPass', { | ||
outputs: { | ||
result: '{% $states.input.init + 1 %}', | ||
}, | ||
}), | ||
), | ||
), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this integ test is enough as the changes are huge. Can you please add more tests that integrate with different step function tasks (perhaps not every step function task is needed but we should definitely add a few more integ tests that test the deployment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your effort on this, I just have a few suggestions to improve the README
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/apigateway/base-types.ts
Show resolved
Hide resolved
@@ -9,7 +9,165 @@ to call other AWS services. | |||
Defining a workflow looks like this (for the [Step Functions Job Poller | |||
example](https://docs.aws.amazon.com/step-functions/latest/dg/job-status-poller-sample.html)): | |||
|
|||
## Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we mean to delete this example? The current flow doesn't quite make sense given the previous sentence: "Defining a workflow looks like this (for the Step Functions Job Poller
example):"
Co-authored-by: Grace Luo <[email protected]>
Co-authored-by: Grace Luo <[email protected]>
Pull request has been modified.
Co-authored-by: Grace Luo <[email protected]>
…com/WinterYukky/aws-cdk into feat/step-functions/support-jsonata
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@WinterYukky Thank you for working on this! I have been searching for hours trying to find this support to no avail. Finally thought to check the GitHub issues/PRs and found this! It looks like it is close. |
Issue #32262
Closes #32262.
Reason for this change
For to use JSONata and variables of Step Function feature on AWS CDK. JSONata is new query language of Step Function, It is simple and powerful language. JSONata and variables is recommend for new state machine.
Description of changes
JSONata support
Add
jsonPath()
andjsonata()
factory methods to state constructs. For example,One option would be to simply add JSONata-specific properties to the Props of the existing State construct, but in this case, the JSONata-specific properties will be displayed to the JSONPath user. Conversely, it was thought that the development experience would deteriorate if JSONata users were shown JSONPath-specific properties. As a countermeasure, we decomposed the existing Props into JSONPath-specific properties and created
jsonPath()
andjsonata()
factory methods to separate type suggestions for JSONPath users and JSONata users.The existing initialization method, the constructor, is backward compatible because it accepts
JSONPath
andJSONata
properties. However, to use this interface directly is a lot of noise. This noise is a source of confusion for SFn beginners, and I thought it was necessary to solve this problem.Therefore, we use the factory methods for each query language.
※ The
output
property is used in the image example, but it is actuallyoutputs
. Check out this comment for the reasons for this decision.jsonPath()
does not have a JSONata-specific propertyoutputs
.jsonata()
does not have JSONPath-specific properties such asxxxPath
.Variables
Add
assign
to state constructs.assign
can be used from eitherJSONata
orJSONPath
.For example,
Description of how you validated changes
Added unit test. Integration tests are not yet.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license