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.

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(events): support embedded string variables #13487

Merged
merged 8 commits into from
Jun 4, 2021

Conversation

thantos
Copy link
Contributor

@thantos thantos commented Mar 9, 2021

Event Bridge transformers have been updated to support embedded variable replacement within strings within objects.

{
   data: "some string <myValue>"
}

Previously input transformers only supported string when they were the only value of an object, or just static strings.

// Before Event Bridges's change
{
   data: <myValue>, // OK
   data2: "some string", // OK
   data3: "some string <myValue>" // NOT OK 
}

The CDK solution was to assume that developers knew this restriction, wrap the string variable in special characters, and replace the double quotes plus special character set with nothing after token replacement.

This caused issues like #9191. Where string tokens (EventField) within a string would give a cryptic error during Cfn deployment due the resulting invalid object string generated (missing a closing double quote and leaving the special characters behind).

Solution:

Removed the special character sequence addition and stripping and instead only replace any instances of "<myValue>" that are added.

  • Iterate over the known input transform keys to reduce possible unexpected impact and give developers a backdoor to change their keys in the worst case.
  • Edge Case: "<myValue>" can appear with escaped quote sequences "something \"quoted\"<myValue>". This is a valid string variable replacement case. Used a lookback regex ((?<!\\\\)\"\<${key}\>\") to avoid the prefix escaped quote when replacing transform input keys with quote-less keys.

Tradeoffs

Removed the addition of special characters to find the keys in the final json string. Instead search for the specific pattern of transform input keys that should exist within the output and handle the edge case describe above.

This SHOULD cover all edge cases as it is not valid to have a trailing quote without an escape ("<myValue>"" //not valid) and it is not valid to have a prefix quote that is not escaped (""<myValue>" // not valid).

This was done to reduce the small change of overlapping with a developer's content, to be more targeted, and because the above should prove that the edge case is covered.

https://docs.aws.amazon.com/eventbridge/latest/APIReference/API_InputTransformer.html

fixes #9191


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Mar 9, 2021

@github-actions github-actions bot added the @aws-cdk/aws-events Related to CloudWatch Events label Mar 9, 2021
@thantos thantos force-pushed the thantos/aws-event-field-concat branch 2 times, most recently from 4f7d161 to 69e5eff Compare March 9, 2021 22:35
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Thanks for the work, but please use the PR body to describe the problem, the solution you came up with and any potential trade offs that had to be made. This is both useful for me to judge the contribution right now, as well as will come in useful later if we ever need to do software archeology.

I see changes in the PR that I didn't expect and don't seem safe. I don't know what to make of them, or why you thought these changes won't cause any problems.

fixes #1234 hardly gives me the context I need to properly evaluate the impact of this change.

packages/@aws-cdk/aws-events/test/test.input.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed rix0rrr’s stale review March 22, 2021 22:40

Pull request has been modified.

@thantos thantos force-pushed the thantos/aws-event-field-concat branch from 3a5dbc1 to 16e3870 Compare March 23, 2021 13:11
@thantos thantos force-pushed the thantos/aws-event-field-concat branch from 16e3870 to e5d7b70 Compare March 23, 2021 13:16
@gitpod-io
Copy link

gitpod-io bot commented May 19, 2021

@rix0rrr rix0rrr changed the title feat(aws-events): update eventfield to support embedded string variables feat(events): support embedded string variables Jun 4, 2021
@mergify
Copy link
Contributor

mergify bot commented Jun 4, 2021

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit a5d27aa into aws:master Jun 4, 2021
@mergify
Copy link
Contributor

mergify bot commented Jun 4, 2021

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

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
Event Bridge transformers have been updated to support embedded variable replacement within strings within objects. 

```
{
   data: "some string <myValue>"
}
```

Previously input transformers only supported string when they were the only value of an object, or just static strings.

```
// Before Event Bridges's change
{
   data: <myValue>, // OK
   data2: "some string", // OK
   data3: "some string <myValue>" // NOT OK 
}
```

The CDK solution was to assume that developers knew this restriction, wrap the string variable in special characters, and replace the double quotes plus special character set with nothing after token replacement.

This caused issues like aws#9191. Where string tokens (`EventField`) within a string would give a cryptic error during Cfn deployment due the resulting invalid object string generated (missing a closing double quote and leaving the special characters behind). 

### Solution:

Removed the special character sequence addition and stripping and instead only replace any instances of `"<myValue>"` that are added.

* Iterate over the known input transform keys to reduce possible unexpected impact and give developers a backdoor to change their keys in the worst case.
* Edge Case: `"<myValue>"` can appear with escaped quote sequences `"something \"quoted\"<myValue>"`. This is a valid string variable replacement case. Used a lookback regex (`(?<!\\\\)\"\<${key}\>\"`) to avoid the prefix escaped quote when replacing transform input keys with quote-less keys. 

### Tradeoffs

Removed the addition of special characters to find the keys in the final json string. Instead search for the specific pattern of transform input keys that should exist within the output and handle the edge case describe above.

This SHOULD cover all edge cases as it is not valid to have a trailing quote without an escape (`"<myValue>"" //not valid`) and it is not valid to have a prefix quote that is not escaped (`""<myValue>" // not valid`).

This was done to reduce the small change of overlapping with a developer's content, to be more targeted, and because the above should prove that the edge case is covered.


https://docs.aws.amazon.com/eventbridge/latest/APIReference/API_InputTransformer.html

fixes aws#9191



----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-events Related to CloudWatch Events
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[aws-events] concatenating event fields should produce an error
3 participants