Skip to content
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.

Sign up for GitHub

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

fix(stepfunctions): improve Task payload encoding #2706

Merged
merged 2 commits into from
Jun 4, 2019
Merged

fix(stepfunctions): improve Task payload encoding #2706

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f196130
fix(stepfunctions): improve Task payload encoding
rix0rrr May 31, 2019
b8ac1bd
Address review comments
rix0rrr Jun 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/@aws-cdk/aws-stepfunctions-tasks/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,4 @@ export * from './run-ecs-task-base-types';
export * from './publish-to-topic';
export * from './send-to-queue';
export * from './run-ecs-ec2-task';
export * from './run-ecs-fargate-task';
export * from './number-value';
export * from './json-path';
export * from './run-ecs-fargate-task';
119 changes: 0 additions & 119 deletions packages/@aws-cdk/aws-stepfunctions-tasks/lib/json-path.ts
View file
Open in desktop

This file was deleted.

53 changes: 0 additions & 53 deletions packages/@aws-cdk/aws-stepfunctions-tasks/lib/number-value.ts
View file
Open in desktop

This file was deleted.

22 changes: 10 additions & 12 deletions packages/@aws-cdk/aws-stepfunctions-tasks/lib/publish-to-topic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,24 @@
import iam = require('@aws-cdk/aws-iam');
import sns = require('@aws-cdk/aws-sns');
import sfn = require('@aws-cdk/aws-stepfunctions');
import cdk = require('@aws-cdk/cdk');
import { renderString } from './json-path';

/**
* Properties for PublishTask
*/
export interface PublishToTopicProps {
/**
* The text message to send to the queue.
* The text message to send to the topic.
*
* Exactly one of `message` and `messageObject` is required.
* @default - Exactly one of `message` and `messageObject` is required.
*/
readonly message?: string;

/**
* Object to be JSON-encoded and used as message
*
* Exactly one of `message`, `messageObject` and `messagePath` is required.
* @default - Exactly one of `message` and `messageObject` is required.
*/
readonly messageObject?: string;
readonly messageObject?: {[key: string]: any};

/**
* If true, send a different message to every subscription type
Expand Down Expand Up @@ -53,7 +51,7 @@ export class PublishToTopic implements sfn.IStepFunctionsTask {
}
}

public bind(task: sfn.Task): sfn.StepFunctionsTaskProperties {
public bind(_task: sfn.Task): sfn.StepFunctionsTaskProperties {
return {
resourceArn: 'arn:aws:states:::sns:publish',
policyStatements: [new iam.PolicyStatement()
Expand All @@ -62,11 +60,11 @@ export class PublishToTopic implements sfn.IStepFunctionsTask {
],
parameters: {
TopicArn: this.topic.topicArn,
...(this.props.messageObject
? { Message: new cdk.Token(() => task.node.stringifyJson(this.props.messageObject)) }
: renderString('Message', this.props.message)),
MessageStructure: this.props.messagePerSubscriptionType ? "json" : undefined,
...renderString('Subject', this.props.subject),
...sfn.FieldUtils.renderObject({
Message: this.props.message || this.props.messageObject,
MessageStructure: this.props.messagePerSubscriptionType ? "json" : undefined,
Subject: this.props.subject,
})
}
};
}
Expand Down
8 changes: 3 additions & 5 deletions packages/@aws-cdk/aws-stepfunctions-tasks/lib/run-ecs-task-base-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { NumberValue } from "./number-value";

export interface ContainerOverride {
/**
* Name of the container inside the task definition
Expand All @@ -23,21 +21,21 @@ export interface ContainerOverride {
*
* @Default The default value from the task definition.
*/
readonly cpu?: NumberValue;
readonly cpu?: number;

/**
* Hard memory limit on the container
*
* @Default The default value from the task definition.
*/
readonly memoryLimit?: NumberValue;
readonly memoryLimit?: number;

/**
* Soft memory limit on the container
*
* @Default The default value from the task definition.
*/
readonly memoryReservation?: NumberValue;
readonly memoryReservation?: number;
}

/**
Expand Down
19 changes: 9 additions & 10 deletions packages/@aws-cdk/aws-stepfunctions-tasks/lib/run-ecs-task-base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import ecs = require('@aws-cdk/aws-ecs');
import iam = require('@aws-cdk/aws-iam');
import sfn = require('@aws-cdk/aws-stepfunctions');
import cdk = require('@aws-cdk/cdk');
import { renderNumber, renderString, renderStringList } from './json-path';
import { ContainerOverride } from './run-ecs-task-base-types';

/**
Expand Down Expand Up @@ -162,17 +161,17 @@ function renderOverrides(containerOverrides?: ContainerOverride[]) {

const ret = new Array<any>();
for (const override of containerOverrides) {
ret.push({
...renderString('Name', override.containerName),
...renderStringList('Command', override.command),
...renderNumber('Cpu', override.cpu),
...renderNumber('Memory', override.memoryLimit),
...renderNumber('MemoryReservation', override.memoryReservation),
ret.push(sfn.FieldUtils.renderObject({
Name: override.containerName,
Command: override.command,
Cpu: override.cpu,
Memory: override.memoryLimit,
MemoryReservation: override.memoryReservation,
Environment: override.environment && override.environment.map(e => ({
...renderString('Name', e.name),
...renderString('Value', e.value),
Name: e.name,
Value: e.value,
}))
});
}));
}

return { ContainerOverrides: ret };
Expand Down
30 changes: 21 additions & 9 deletions packages/@aws-cdk/aws-stepfunctions-tasks/lib/send-to-queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,24 @@
import iam = require('@aws-cdk/aws-iam');
import sqs = require('@aws-cdk/aws-sqs');
import sfn = require('@aws-cdk/aws-stepfunctions');
import { renderNumber, renderString } from './json-path';
import { NumberValue } from './number-value';

/**
* Properties for SendMessageTask
*/
export interface SendToQueueProps {
/**
* The message body to send to the queue.
* The text message to send to the topic.
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
*
* @default - Exactly one of `message` and `messageObject` is required.
*/
readonly message?: string;

/**
* Object to be JSON-encoded and used as message
*
* @default - Exactly one of `message` and `messageObject` is required.
*/
readonly messageBody: string;
readonly messageObject?: {[key: string]: any};

/**
* The length of time, in seconds, for which to delay a specific message.
Expand All @@ -20,7 +27,7 @@ export interface SendToQueueProps {
*
* @default Default value of the queue is used
*/
readonly delaySeconds?: NumberValue;
readonly delaySeconds?: number;

/**
* The token used for deduplication of sent messages.
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeeessss!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would solve so much.

Copy link
Contributor

Choose a reason for hiding this comment

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

How would you do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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) { }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

throw new Error(`Supply exactly one of 'message' or 'messageObject'`);
}
}

public bind(_task: sfn.Task): sfn.StepFunctionsTaskProperties {
Expand All @@ -59,10 +69,12 @@ export class SendToQueue implements sfn.IStepFunctionsTask {
],
parameters: {
QueueUrl: this.queue.queueUrl,
...renderString('MessageBody', this.props.messageBody),
...renderNumber('DelaySeconds', this.props.delaySeconds),
...renderString('MessageDeduplicationId', this.props.messageDeduplicationId),
...renderString('MessageGroupId', this.props.messageGroupId),
...sfn.FieldUtils.renderObject({
MessageBody: this.props.message || this.props.messageObject,
DelaySeconds: this.props.delaySeconds,
MessageDeduplicationId: this.props.messageDeduplicationId,
MessageGroupId: this.props.messageGroupId,
})
}
};
}
Expand Down
Loading