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

appsync.GraphqlApi.addLambdaDataSource doesn't strip symbols from id name, causing deploy to fail #16169

Closed
jquesada2016 opened this issue Aug 23, 2021 · 6 comments · Fixed by #18765
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@jquesada2016
Copy link

Doing something like the following:

const api = /* create api */
api.addLambdaDataSource(
      "lambda-name-ds",
      /* lambda */,
    );

does not remove the - from the id name, causing the deploy to fail with:

Property validation failure: [Value for property {/Name} does not match pattern {[_A-Za-z][_0-9A-Za-z]*}]

And if we look at the resulting stack.template.json file, we see the error is present:

{
    "lambdadatasourceid": {
        /* ... */
        "Name": "ambda-name-ds"
        /* ... */
    }
}

Reproduction Steps

Just create any appsync GraphQL API and add a lambda datasource that contains -. (haven't tested if other symbols also trip it up)

What did you expect to happen?

I expect the - to be stripped out, just like all other id's in the CDK along with the deploy not failing.

What actually happened?

The deploy fails.

Environment

  • CDK CLI Version : 1.119.0 (build 2921d64)
  • Framework Version: 1.119.0
  • Node.js Version: v16.6.1
  • OS : Ubuntu (WSL 2)
  • Language (Version): TypeScript ~3.9.7

This is 🐛 Bug Report

@jquesada2016 jquesada2016 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 23, 2021
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Aug 23, 2021
@BryanPan342
Copy link
Contributor

We use the id as a fallback name if the name is not specified in the options. I believe this is expected behavior and I cant find other packages that clean the name for you.

@BryanPan342 BryanPan342 added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p2 and removed needs-triage This issue or PR still needs to be triaged. labels Aug 23, 2021
@jquesada2016
Copy link
Author

I use '-' for all my id names in all CDK packages, and they always get stripped away. I could provide an example for you, just let me know what you would like because I'm not sure how to demonstrate it besides what I've mentioned. I never specify the name, just let the CDK generate one based off the id, and deployment never fails, except for the appsync module.

@BryanPan342
Copy link
Contributor

Ah i see where the problem.. the data source was implemented as a construct instead of a resource..

@BryanPan342 BryanPan342 removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 31, 2021
@otaviomacedo
Copy link
Contributor

We can sanitize the id before assigning it to the data source name:

const name = props.name ?? id;

I am unassigning and marking this issue as p2, which means that we are unable to work on this immediately.

We use +1s to help prioritize our work, and are happy to revaluate this issue based on community feedback. You can reach out to the cdk.dev community on Slack to solicit support for reprioritization.

@otaviomacedo otaviomacedo removed their assignment Sep 3, 2021
@otaviomacedo otaviomacedo added the good first issue Related to contributions. See CONTRIBUTING.md label Sep 3, 2021
@readybuilderone
Copy link
Contributor

I tried to created an S3 bucket using CDK with ID including various unusual characters as following:
image

I compared the logical id in the aws console of cloudformation, and found all the unusual characters were stripped.
image

By this logical analogy, I think we should stripe all of the characters except [_0-9A-Za-z] instead of just stripe '-' only. What do you think? @otaviomacedo @BryanPan342 @jquesada2016 , if you don't mind, I'd like try to solve this issue.

@mergify mergify bot closed this as completed in #18765 Feb 4, 2022
mergify bot pushed a commit that referenced this issue Feb 4, 2022
…#18765)

Closes #16169

----

*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 Feb 4, 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.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
…aws#18765)

Closes aws#16169

----

*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. good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants