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

Support resolving AppSync data sources #3061

Closed
wants to merge 1 commit into from

Conversation

ssenchenko
Copy link
Contributor

Issue #, if available

Description of changes

Currently it's possible to reference one of the resources generated by SAM if it is added to referable_properties. For example Api.Stage. Before GraphQL, all those referenced resources always would be a single resource of its type within SAM resource. In GraphQL, we need to reference one of many resources of the same type, specifically a single of many DataSources.

This PR adds functionality to reference one of many resources of the same type like this
!GetAtt SuperCoolAPI.DataSources:AnotherDataSource.Name.

Q: Why to implement it in GraphQL only and not in SamResourceMacro?
A: GraphQL is so far the only resource which needs this. Solution requires to maintain a map between CFN resource logical id and internal (relative) logical id. Implementing it for SamResourceMacro would require some to support that map for each CFN resource creating. Too much of a change.

Q: Why to use DataSources for reference instead of DynamoDBDataSources?
A: We need to reference DynamoDB data source and Lambda data sources for now. And if we add support for more, like Relational DB, DataSources includes them all. Besides, strictly speaking, DynamoDBDataSources or LambdaResolvers SAM internal type is different from AppSync::DataSource type.

Q: Why to use : as a separator in SuperCoolAPI.DataSources:AnotherDataSource.Name before data source name? Why not .?
A: Using . would require changes to existing shortcut parser code for GetAtt. Plus it'd make parsing of that value too complicated and bug-prone because Attribute for GetAtt intrinsic function is not just the last one in the string, it can have more then one field in a row like Dict1.Dict1.Field. Changing a separator seems like an acceptable price to avoid much more complicated change.

Description of how you validated changes

Checklist

Examples?

Transform: AWS::Serverless-2016-10-31
Resources:
  SuperCoolAPI:
    Type: AWS::Serverless::GraphQLApi
    Properties:
      ...
      DynamoDBDataSources:
        MyDataSource:
          TableName: some-table
          TableArn: big-arn
        AnotherDataSource:
          TableName: cool-table
          TableArn: table-arn

  GetTodoResolver:
    Description: "Don't do anything, just reference"
    Type: AWS::AppSync::Resolver
    Properties:
      ApiId: !GetAtt SuperCoolAPI.ApiId
      DataSourceName: !GetAtt SuperCoolAPI.DataSources:AnotherDataSource.Name
      ...

Please reach out in the comments if you want to add an example. Examples will be
added to sam init through aws/aws-sam-cli-app-templates.

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

@ssenchenko ssenchenko requested a review from a team as a code owner March 27, 2023 17:20
},
"Type": "AWS::AppSync::GraphQLApi"
},
"SuperCoolAPIAnotherDataSource": {
Copy link
Contributor

@hoffa hoffa Mar 27, 2023

Choose a reason for hiding this comment

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

It looks like logical ID is <api-id><dynamodb-datasource-name> and currently data sources are under DynamoDBDataSources. When we add another category of data sources, and if customer same name twice (but under different data source property), it'll cause deployment failure due to logical ID clash. Is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A good point, but not the one to address in this PR @pradhapanr

"""
Add the information that resource with given `logical_id` supports the given `property`, and that a reference
to `logical_id.property` resolves to given `value.

Example:

"MyApi.Deployment" -> "MyApiDeployment1234567890"
"SuperCoolAPI.DataSources:MyDataSource" -> "SuperCoolAPIMyDataSource"
Copy link
Contributor

Choose a reason for hiding this comment

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

How necessary is this?

The . notation is useful for things that cannot be easily determined, such as the example above where the deployment includes a hash. For everything else, we document the generated logical IDs, so I'm not sure whether it's worth adding a new concept to refer to resources.

"""
Add the information that resource with given `logical_id` supports the given `property`, and that a reference
to `logical_id.property` resolves to given `value.

Example:

"MyApi.Deployment" -> "MyApiDeployment1234567890"
"SuperCoolAPI.DataSources:MyDataSource" -> "SuperCoolAPIMyDataSource"
Copy link
Contributor

@hoffa hoffa Mar 27, 2023

Choose a reason for hiding this comment

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

Here it's DataSources, but in the properties it's currently DynamoDBDataSources.

What happens when we add something else than DynamoDBDataSources and customer has same name for two data sources (the other is under a new category, not DynamoDBDataSources)? If that's not allowed, perhaps we can have a single DataSources property instead?

@hoffa
Copy link
Contributor

hoffa commented Mar 27, 2023

Decided to not go with this approach mostly because it requires us to be able to resolve arbitrary intrinsic functions, which historically has caused many issues (see #2533).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants