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

aws-appsync: Data Source name does not handle tokens appropriately #18900

Closed
saltman424 opened this issue Feb 9, 2022 · 1 comment · Fixed by #22378
Closed

aws-appsync: Data Source name does not handle tokens appropriately #18900

saltman424 opened this issue Feb 9, 2022 · 1 comment · Fixed by #22378
Assignees
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. p1

Comments

@saltman424
Copy link
Contributor

saltman424 commented Feb 9, 2022

What is the problem?

This line mutates Tokens so that they do not get resolved:

const name = (props.name ?? id).replace(/[\W]+/g, '');

Reproduction Steps

new appsync.NoneDataSource(scope, 'DataSource', {
  api,
  name: `My${cdk.Lazy.string({ produce(): string { return 'Prod' }})}DataSource`
})

What did you expect to happen?

Data Source name = 'MyProdDataSource'

What actually happened?

Data Source name = 'MyTokenTOKEN1234DataSource'

CDK CLI Version

2.11.0

Framework Version

2.11.0

Node.js Version

16.13.1

OS

Microsoft Windows 10 Enterprise

Language

Typescript

Language Version

4.5.5

Other information

This is the cause: bb8d6f6

Possible solutions:

  • Use more sophisticated special character removal logic that identifies tokens and leaves their necessary special characters
  • Check if name isUnresolved and don't apply special character removal if it is
  • Only use special character removal if using the id as the name so that when developers pass an explicit name it is used exactly as-is, and if it has invalid characters, they'll get an error during deployment
  • Similar to the above option, but add logic to determine if an explicitly passed name contains special characters outside of tokens and throw an error during synthesis rather than deployment
@saltman424 saltman424 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 9, 2022
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Feb 9, 2022
@NGL321 NGL321 added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Feb 14, 2022
@TheRealAmazonKendra TheRealAmazonKendra self-assigned this Oct 5, 2022
@mergify mergify bot closed this as completed in #22378 Oct 6, 2022
mergify bot pushed a commit that referenced this issue Oct 6, 2022
Closes #18900

----

### All Submissions:

* [ X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [X ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Oct 6, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

arewa pushed a commit to arewa/aws-cdk that referenced this issue Oct 8, 2022
Closes aws#18900

----

### All Submissions:

* [ X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [X ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
homakk pushed a commit to homakk/aws-cdk that referenced this issue Dec 1, 2022
Closes aws#18900

----

### All Submissions:

* [ X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [X ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*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-appsync Related to AWS AppSync bug This issue is a bug. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants