Skip to content

feat(step-functions): support and utilize new intrinsics#468

Merged
thantos merged 97 commits intomainfrom
sussman/sfn_new_intrinsics
Sep 16, 2022
Merged

feat(step-functions): support and utilize new intrinsics#468
thantos merged 97 commits intomainfrom
sussman/sfn_new_intrinsics

Conversation

@thantos
Copy link
Collaborator

@thantos thantos commented Sep 1, 2022

Depends on #510

Closes #77 Closes #282 Closes #330 Closes #395

  • Utility intrinsics
  • Use intrinsics to support ECMA/TS
    • ArrayContains - $.arr.includes()
    • ArrayGetItem - $.arr[$.index]
      • $.index in $arr
    • ArrayRange
      • split
    • ArrayLength - $.arr.length
    • JsonMerge - { ...obj, ...obj2 } only supports objects 😦
    • StringSplit - string.split
    • MathAdd - +=, +, -, -=, --, ++
      • Add warning on validation (only ints, floored)
      • Add caveat to docs (only ints, floored)
      • Clean up validation and docs
  • Validation
    • Success for array index access
    • Fail for object index access
  • Support tests on AWS
    • Env Flag
    • Clean up

Note: new intrinsics are not supported by localstack yet.

@netlify
Copy link

netlify bot commented Sep 1, 2022

Deploy Preview for effortless-malabi-1c3e77 ready!

Name Link
🔨 Latest commit 2b7af54
🔍 Latest deploy log https://app.netlify.com/sites/effortless-malabi-1c3e77/deploys/6324bc9725725700099f8132
😎 Deploy Preview https://deploy-preview-468--effortless-malabi-1c3e77.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

* @param
*/
export const range = makeStepFunctionIntegration<
"SFN.Range",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be States.Range? To match the intrinsic name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I am not using the full names of the intrinsics. (ArrayRange => Range) because "array" is covered by the type.

Comment on lines 644 to 685
>("SFN.Base64Encode", {
asl(call, context) {
const [data] = call.args;

if (!data) {
throw new SynthError(
ErrorCodes.Invalid_Input,
"Expected Base64 data argument to be provided."
);
}

return context.evalContext(call, ({ evalExprToJsonPathOrLiteral }) => {
const dataOut = evalExprToJsonPathOrLiteral(data.expr);

if (
ASLGraph.isLiteralValue(dataOut) &&
typeof dataOut.value !== "string"
) {
throw new SynthError(
ErrorCodes.Invalid_Input,
"Expected base64Encode data argument to be a string or reference."
);
}
const temp = context.newHeapVariable();
return {
Type: "Pass",
Parameters: {
"out.$": `States.Base64Encode(${
ASLGraph.isJsonPath(dataOut)
? dataOut.jsonPath
: `'${dataOut.value}'`
})`,
},
ResultPath: temp,
Next: ASLGraph.DeferNext,
output: {
jsonPath: `${temp}.out`,
},
};
});
},
});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is anyway to simplify these new intrinsic. They seem to have a lot of code in common. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a helper to assign a single parameter key and output it. Was applicable in many places.

credentials: clientConfig.credentials,
};
sdkProvider.sdkOptions = {
// @ts-ignore
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was existing code that was moved.

Might be assigning some not so public props :)

image

(parent) => {
return new StepFunction(parent, "sfn", async () => {
const odd = $SFN
.range(1, 11, 2)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use variables here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Will add more to the test.

Signed-off-by: github-actions <github-actions@github.com>
@thantos thantos requested a review from sam-goodwin September 14, 2022 12:37
Copy link
Owner

@sam-goodwin sam-goodwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, forgot to send these comments through lol

src/asl/synth.ts Outdated
ResultPath: `$.${errorVariableName}`,
},
ASLGraph.assignJsonPathOrIntrinsic(
`States.StringToJson($.${errorVariableName}.Cause)`,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be using one of the ASLGrapj.intrinscStringToJson calls?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! My plan was to sweep through and update the whole code base to use these helpers after this change. Tried to get all of the intrinsics added in this change, but the helpers came much later than some of the other changes. Will clean up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated most of them now

src/asl/synth.ts Outdated
Comment on lines 2187 to 2191
// postfix
// assign left to heap
// mutate left
// assign left
// return heap
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what to do with these comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha, just me trying to figure out the logic...

src/asl/synth.ts Outdated
Comment on lines 2271 to 2283
if (ASLGraph.isLiteralValue(left)) {
if (
ASLGraph.isLiteralString(left) ||
ASLGraph.isLiteralNumber(left)
) {
return ASLGraph.isLiteralValue(right)
? {
value:
right.value &&
typeof right.value === "object" &&
left.value in right.value,
containsJsonPath: false,
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to need some comments.

Comment on lines 1319 to 1331
* Step Functions Arithmetic Only Supports Integer
*
* Step Functions only supports integer addition via the `States.MathAdd`.
*
* ```ts
* new StepFunction(stack, "sfn", async (input: { a: number }) => {
* return 1.5 + input.a;
* });
* ```
*
* If the above machine is given input: `{ a: 0.5 }`, the result will be `1`.
*
* Effectively resulting in: `Math.floor(1.5) + Math.floor(0.5)` => `1`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be described more simply.

Step Functions can only add integers. When adding floating point numbers, the service will floor each of the numbers before adding them.

1.5 + 1.5 = 2

src/validate.ts Outdated
return [
newError(
node,
ErrorCodes.Cannot_perform_arithmetic_or_bitwise_computations_on_variables_in_Step_Function
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we update this error code with the new addition change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logic, forgot about the text.

@thantos thantos changed the base branch from main to sussman/build_perf September 16, 2022 15:43
Signed-off-by: github-actions <github-actions@github.com>
@sam-goodwin
Copy link
Owner

Is this ready? Looks good to me

Base automatically changed from sussman/build_perf to main September 16, 2022 18:06
@thantos
Copy link
Collaborator Author

thantos commented Sep 16, 2022

Missed the approve and it was witting on #510, will push soon.

@thantos thantos merged commit 1c5030f into main Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

2 participants