feat(stepfunctions-tasks): add sns publish with message attributes

[moofish32]

This PR address #4702

I am primarily solving for the use case of using SNS publish with
message attributes for sns filtering. If EventBridge integrations have a
fast follow for wait for task token features, that would solve my use
case. Because I'm solving for filtering and based on anecdotal
experience String is the primary attribute type. I've included Binary
because the pattern is the same. I have not implemented Number or
String.Array because they are not Lambda supported and I believe less
common.

I chose to attempt to solve for consumer ergonomics with:

    const task = new SnsPublish(stack, 'Publish', {
      topic,
      message: sfn.TaskInput.fromText('Publish this message'),
      messageAttributes: {
        cake: {
          type: SnsMessageAttributeType.STRING,
          value: sfn.TaskInput.fromText('chocolate'),
        },
        cakePic: {
          type: SnsMessageAttributeType.BINARY,
          value: sfn.TaskInput.fromDataAt('$.cake.pic'),
        },
      },
    });

This results in a value member on the message attribute interface.
While I think that maps best for the SNS json definition and does yield
this ugly line in the private render methods:

      attrs[a][`${attr.type}Value`] = attr.value.value;

Is there a better way?

I did not implement SQS message attributes yet. Based on my limited
knowledge the StepFunction integrations are similar(potentially
identical if we again only support Binary and String). If the answer is
one implementation, should I move the below code snippets into something
generically named MessageAttribute within the stepfunctions package
perhaps as part of base-task?

/**
 * SNS Message Attribute type enum
 */
export enum SnsMessageAttributeType {

  /**
   * String type attributes.
   *
   * @see https://docs.aws.amazon.com/sns/latest/dg/sns-message-attributes.html#SNSMessageAttributes.DataTypes
   */
  STRING = 'String',

  /**
   * Binary type attributes.
   *
   * @see https://docs.aws.amazon.com/sns/latest/dg/sns-message-attributes.html#SNSMessageAttributes.DataTypes
   */
  BINARY = 'Binary',
}

/**
 * SNS Message Attribute
 */
export interface SnsMessageAttribute {
  /**
   * MessageAttributeType is the type of data being passed.
   *
   *
   * @see https://docs.aws.amazon.com/sns/latest/dg/sns-message-attributes.html#SNSMessageAttributes.DataTypes
   */
  readonly type: SnsMessageAttributeType;

  /**
   * The value of the data being passed
   */
  readonly value: sfn.TaskInput;
}

[gitpod-io]

[moofish32]

Adding in line comments similar to the summary for comment tracking.

[moofish32]

@shivlaks I think you indicated some interest in a PR here, if you have something ready feel free to push and take the lead.

[moofish32]

Ok I've realized I did not understand the value/flexibility of sfn.TaskInput.fromObject. I think we can simplify by using that function. Can anybody from the CDK team comment if that is a better path? I have pushed a very rough implementation example using this approach.

If we can agree that we will only support String types then this solution could simplify to a structure more like:

    const pubTask = new SnsPublish(stack, 'Publish', {
      topic,
      message: sfn.TaskInput.fromText('Publish this message'),
      messageAttributes: {
        cake: sfn.TaskInput.fromText('chocolate'),
        attribute2: sfn.TaskInput.fromDataAt('$.attribute2'),
      },
    });

I prefer the above because it's the only use case I've seen in the wild and most simple to declare.

@shivlaks are you able to help here at all?

Pull request has been modified.

[moofish32]

Ok I've realized I did not understand the value/flexibility of sfn.TaskInput.fromObject. I think we can simplify by using that function. Can anybody from the CDK team comment if that is a better path? I have pushed a very rough implementation example using this approach.

If we can agree that we will only support String types then this solution could simplify to a structure more like:

    const pubTask = new SnsPublish(stack, 'Publish', {
      topic,
      message: sfn.TaskInput.fromText('Publish this message'),
      messageAttributes: {
        cake: sfn.TaskInput.fromText('chocolate'),
        attribute2: sfn.TaskInput.fromDataAt('$.attribute2'),
      },
    });

I prefer the above because it's the only use case I've seen in the wild and most simple to declare.

@shivlaks are you able to help here at all?

I think I still have a question here. If we agree String support is the only type we can simplify a lot of code.

export interface MessageAttributes { [key: string]: string; }
// usage becomes
const pubTask = new SnsPublish(stack, 'Publish', {
      topic,
      message: sfn.TaskInput.fromText('Publish this message'),
      messageAttributes: {
        cake: 'chocolate',
        attribute2: sfn.JsonPathAt('$.attribute2'),
      },
    });

I think part of my confusion is where do I need to enforce they use sfn.TaskInput versus just using literal strings and JsonPath?

[BenChaimberg]

I think overall, I'd like to support more than just strings. The current split of only allowing task input in the attribute value is fine for me. This is a design decision that we should align on, but I think it's a realistic tradeoff as allowing too much flexibility removes all the nice type checking, serialization, etc. that we can do

[moofish32]

I’ve got some free time next week. I like the feedback I’ll make some changes.

…

On Fri, Jul 2, 2021 at 10:00 PM Ben Chaimberg ***@***.***> wrote: I think overall, I'd like to support more than just strings. The current split of only allowing task input in the attribute value is fine for me. This is a design decision that we should align on, but I think it's a realistic tradeoff as allowing too much flexibility removes all the nice type checking, serialization, etc. that we can do — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14817 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFCFWBZTQYA5LZNHEC7F73TV2KPXANCNFSM45IRPQDQ> .

Pull request has been modified.

[moofish32]

This comment #14817 (comment) represents what I believe to be blocking finishing this work.

[BenChaimberg]

I like the auto-detect, feels very powerful for our customers! Just a few major comments, rest are naming/implementation details

[moofish32]

I agree with comments @BenChaimberg and will get a refactor up this week.

Pull request has been modified.

Pull request has been modified.

[moofish32]

@BenChaimberg - I think I've now addressed all of your comments. Can you please give me another review?

[BenChaimberg]

This is looking great! Just a couple of minor things and I think we'll be good to go!

Pull request has been modified.

Pull request has been modified.

[mergify]

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-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 0b6dbc7
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

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).