Skip to content

Commit

Permalink
fix(appsync): unexpected resolver replacement (#23322)
Browse files Browse the repository at this point in the history
fix(appsync): unstable IDs on resolvers and functions

Fixes an issue that would cause unexpected resource replacement for
appsync resolvers and functions because of construct nesting and ID
generation.

Changes `createResolver` and `createFunction` methods on `GraphQlApi`
and `DataSource` constructs to require explicitly passing an ID.
Additionally changes the scope of the constructs created in
`createResolver` and `createFunction` on the `DataSource` construct to
be `this.api` instead of `this`. This allows users to change the data
sources of resolvers and functions while keeping the IDs stable and
avoiding resource replacement.

This helps to avoid the `only one resolver per field` error that occurs
when deleting a resolver on a field, and adding a new one within the
same deployment.

BREAKING CHANGE: `DataSource.createResolver`,
`DataSource.createFunction`, and `GraphQlApi.createResolver` now require
2 arguments instead of 1.

Fixes: #13269

----

### All Submissions:

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

### Adding new Construct Runtime Dependencies:

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

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
  • Loading branch information
MrArnoldPalmer authored Dec 14, 2022
1 parent 3f706e2 commit 6dc15d4
Show file tree
Hide file tree
Showing 44 changed files with 2,823 additions and 2,318 deletions.
14 changes: 7 additions & 7 deletions packages/@aws-cdk/aws-appsync/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,15 @@ const demoDS = api.addDynamoDbDataSource('demoDataSource', demoTable);
// Resolver for the Query "getDemos" that scans the DynamoDb table and returns the entire list.
// Resolver Mapping Template Reference:
// https://docs.aws.amazon.com/appsync/latest/devguide/resolver-mapping-template-reference-dynamodb.html
demoDS.createResolver({
demoDS.createResolver('QueryGetDemosResolver', {
typeName: 'Query',
fieldName: 'getDemos',
requestMappingTemplate: appsync.MappingTemplate.dynamoDbScanTable(),
responseMappingTemplate: appsync.MappingTemplate.dynamoDbResultList(),
});

// Resolver for the Mutation "addDemo" that puts the item into the DynamoDb table.
demoDS.createResolver({
demoDS.createResolver('MutationAddDemoResolver', {
typeName: 'Mutation',
fieldName: 'addDemo',
requestMappingTemplate: appsync.MappingTemplate.dynamoDbPutItem(
Expand All @@ -98,7 +98,7 @@ demoDS.createResolver({
});

//To enable DynamoDB read consistency with the `MappingTemplate`:
demoDS.createResolver({
demoDS.createResolver('QueryGetDemosConsistentResolver', {
typeName: 'Query',
fieldName: 'getDemosConsistent',
requestMappingTemplate: appsync.MappingTemplate.dynamoDbScanTable(true),
Expand Down Expand Up @@ -137,7 +137,7 @@ declare const api: appsync.GraphqlApi;
const rdsDS = api.addRdsDataSource('rds', cluster, secret, 'demos');

// Set up a resolver for an RDS query.
rdsDS.createResolver({
rdsDS.createResolver('QueryGetDemosRdsResolver', {
typeName: 'Query',
fieldName: 'getDemosRds',
requestMappingTemplate: appsync.MappingTemplate.fromString(`
Expand All @@ -154,7 +154,7 @@ rdsDS.createResolver({
});

// Set up a resolver for an RDS mutation.
rdsDS.createResolver({
rdsDS.createResolver('MutationAddDemoRdsResolver', {
typeName: 'Mutation',
fieldName: 'addDemoRds',
requestMappingTemplate: appsync.MappingTemplate.fromString(`
Expand Down Expand Up @@ -244,7 +244,7 @@ const httpDs = api.addHttpDataSource(
}
);

httpDs.createResolver({
httpDs.createResolver('MutationCallStepFunctionResolver', {
typeName: 'Mutation',
fieldName: 'callStepFunction',
requestMappingTemplate: appsync.MappingTemplate.fromFile('request.vtl'),
Expand Down Expand Up @@ -275,7 +275,7 @@ const domain = new opensearch.Domain(this, 'Domain', {
declare const api: appsync.GraphqlApi;
const ds = api.addOpenSearchDataSource('ds', domain);

ds.createResolver({
ds.createResolver('QueryGetTestsResolver', {
typeName: 'Query',
fieldName: 'getTests',
requestMappingTemplate: appsync.MappingTemplate.fromString(JSON.stringify({
Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-appsync/lib/data-source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ export abstract class BaseDataSource extends Construct {
/**
* creates a new resolver for this datasource and API using the given properties
*/
public createResolver(props: BaseResolverProps): Resolver {
return new Resolver(this, `${props.typeName}${props.fieldName}Resolver`, {
public createResolver(id: string, props: BaseResolverProps): Resolver {
return new Resolver(this.api, id, {
api: this.api,
dataSource: this,
...props,
Expand All @@ -143,8 +143,8 @@ export abstract class BaseDataSource extends Construct {
/**
* creates a new appsync function for this datasource and API using the given properties
*/
public createFunction(props: BaseAppsyncFunctionProps): AppsyncFunction {
return new AppsyncFunction(this, `${props.name}Function`, {
public createFunction(id: string, props: BaseAppsyncFunctionProps): AppsyncFunction {
return new AppsyncFunction(this.api, id, {
api: this.api,
dataSource: this,
...props,
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export interface IGraphqlApi extends IResource {
/**
* creates a new resolver for this datasource and API using the given properties
*/
createResolver(props: ExtendedResolverProps): Resolver;
createResolver(id: string, props: ExtendedResolverProps): Resolver;

/**
* Add schema dependency if not imported
Expand Down Expand Up @@ -285,8 +285,8 @@ export abstract class GraphqlApiBase extends Resource implements IGraphqlApi {
/**
* creates a new resolver for this datasource and API using the given properties
*/
public createResolver(props: ExtendedResolverProps): Resolver {
return new Resolver(this, `${props.typeName}${props.fieldName}Resolver`, {
public createResolver(id: string, props: ExtendedResolverProps): Resolver {
return new Resolver(this, id, {
api: this,
...props,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('Lambda caching config', () => {
// WHEN
const lambdaDS = api.addLambdaDataSource('LambdaDS', func);

lambdaDS.createResolver({
lambdaDS.createResolver('QueryAllPosts', {
typeName: 'Query',
fieldName: 'allPosts',
});
Expand All @@ -50,7 +50,7 @@ describe('Lambda caching config', () => {
// WHEN
const lambdaDS = api.addLambdaDataSource('LambdaDS', func);

lambdaDS.createResolver({
lambdaDS.createResolver('QueryAllPosts', {
typeName: 'Query',
fieldName: 'allPosts',
cachingConfig: {
Expand All @@ -77,7 +77,7 @@ describe('Lambda caching config', () => {

// THEN
expect(() => {
lambdaDS.createResolver({
lambdaDS.createResolver('QueryAllPosts', {
typeName: 'Query',
fieldName: 'allPosts',
cachingConfig: {
Expand All @@ -95,7 +95,7 @@ describe('Lambda caching config', () => {

// THEN
expect(() => {
lambdaDS.createResolver({
lambdaDS.createResolver('QueryAllPosts', {
typeName: 'Query',
fieldName: 'allPosts',
cachingConfig: {
Expand All @@ -113,7 +113,7 @@ describe('Lambda caching config', () => {

// THEN
expect(() => {
lambdaDS.createResolver({
lambdaDS.createResolver('QueryAllPosts', {
typeName: 'Query',
fieldName: 'allPosts',
cachingConfig: {
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-appsync/test/appsync-lambda.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe('Lambda Data Source configuration', () => {
description: 'custom description',
});

ds.createResolver({
ds.createResolver('TestField', {
typeName: 'test',
fieldName: 'field',
});
Expand Down Expand Up @@ -164,4 +164,4 @@ describe('adding lambda data source from imported api', () => {
ApiId: { 'Fn::GetAtt': ['baseApiCDA4D43A', 'ApiId'] },
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ describe('Lambda Mapping Templates', () => {
// WHEN
const lambdaDS = api.addLambdaDataSource('LambdaDS', func);

lambdaDS.createResolver({
lambdaDS.createResolver('QueryAllPosts', {
typeName: 'Query',
fieldName: 'allPosts',
requestMappingTemplate: appsync.MappingTemplate.lambdaRequest(),
Expand All @@ -52,7 +52,7 @@ describe('Lambda Mapping Templates', () => {
// WHEN
const lambdaDS = api.addLambdaDataSource('LambdaDS', func);

lambdaDS.createResolver({
lambdaDS.createResolver('PostRelatedPosts', {
typeName: 'Post',
fieldName: 'relatedPosts',
requestMappingTemplate: appsync.MappingTemplate.lambdaRequest('$util.toJson($ctx)', 'BatchInvoke'),
Expand All @@ -67,4 +67,4 @@ describe('Lambda Mapping Templates', () => {
MaxBatchSize: 10,
});
});
});
});
21 changes: 11 additions & 10 deletions packages/@aws-cdk/aws-appsync/test/appsync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ beforeEach(() => {
test('appsync should configure pipeline when pipelineConfig has contents', () => {
// WHEN
const ds = api.addNoneDataSource('none');
const test1 = ds.createFunction({
const test1 = ds.createFunction('Test1Function', {
name: 'test1',
});
const test2 = ds.createFunction({
const test2 = ds.createFunction('Test2Function', {
name: 'test2',
});
api.createResolver({
api.createResolver('TestTest2', {
typeName: 'test',
fieldName: 'test2',
pipelineConfig: [test1, test2],
Expand All @@ -38,8 +38,8 @@ test('appsync should configure pipeline when pipelineConfig has contents', () =>
Kind: 'PIPELINE',
PipelineConfig: {
Functions: [
{ 'Fn::GetAtt': ['apinonetest1FunctionEF63046F', 'FunctionId'] },
{ 'Fn::GetAtt': ['apinonetest2Function615111D0', 'FunctionId'] },
{ 'Fn::GetAtt': ['apiTest1Function793605E9', 'FunctionId'] },
{ 'Fn::GetAtt': ['apiTest2FunctionB704A7AD', 'FunctionId'] },
],
},
});
Expand All @@ -48,16 +48,17 @@ test('appsync should configure pipeline when pipelineConfig has contents', () =>
test('appsync should error when creating pipeline resolver with data source', () => {
// WHEN
const ds = api.addNoneDataSource('none');
const test1 = ds.createFunction({
const test1 = ds.createFunction('Test1Function', {
name: 'test1',
});
const test2 = ds.createFunction({
const test2 = ds.createFunction('Test2Function', {
name: 'test2',
});

// THEN
expect(() => {
ds.createResolver({
api.createResolver('TestTest2', {
dataSource: ds,
typeName: 'test',
fieldName: 'test2',
pipelineConfig: [test1, test2],
Expand All @@ -83,7 +84,7 @@ test('appsync should configure resolver as unit when pipelineConfig is empty', (

test('appsync should configure resolver as unit when pipelineConfig is empty array', () => {
// WHEN
api.createResolver({
api.createResolver('TestTest2', {
typeName: 'test',
fieldName: 'test2',
pipelineConfig: [],
Expand Down Expand Up @@ -237,4 +238,4 @@ test('log retention should not appear when no retention time is specified', () =

// THEN
Template.fromStack(stack).resourceCountIs('Custom::LogRetention', 0);
});
});
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-appsync/test/integ.api-import.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ const testTable = new db.Table(stack, 'TestTable', {

const testDS = api.addDynamoDbDataSource('ds', testTable);

testDS.createResolver({
testDS.createResolver('QueryGetTests', {
typeName: 'Query',
fieldName: 'getTests',
requestMappingTemplate: appsync.MappingTemplate.dynamoDbScanTable(),
responseMappingTemplate: appsync.MappingTemplate.dynamoDbResultList(),
});

testDS.createResolver({
testDS.createResolver('MutationAddTest', {
typeName: 'Mutation',
fieldName: 'addTest',
requestMappingTemplate: appsync.MappingTemplate.dynamoDbPutItem(appsync.PrimaryKey.partition('id').auto(), appsync.Values.projecting('test')),
Expand All @@ -64,7 +64,7 @@ const api2 = appsync.GraphqlApi.fromGraphqlApiAttributes(stack, 'api2', {

const none = api2.addNoneDataSource('none');

const func = none.createFunction({
const func = none.createFunction('PipelineFunction', {
name: 'pipeline_function',
requestMappingTemplate: appsync.MappingTemplate.fromString(JSON.stringify({
version: '2017-02-28',
Expand All @@ -87,4 +87,4 @@ new appsync.Resolver(stack, 'pipeline_resolver', {
})),
});

app.synth();
app.synth();
12 changes: 6 additions & 6 deletions packages/@aws-cdk/aws-appsync/test/integ.appsync-lambda.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,40 +47,40 @@ const requestPayload = (field: string, { withArgs = false, withSource = false })
};
const responseMappingTemplate = appsync.MappingTemplate.lambdaResult();

lambdaDS.createResolver({
lambdaDS.createResolver('QueryGetPost', {
typeName: 'Query',
fieldName: 'getPost',
requestMappingTemplate: appsync.MappingTemplate.lambdaRequest(requestPayload('getPost', { withArgs: true })),
responseMappingTemplate,
});

lambdaDS.createResolver({
lambdaDS.createResolver('QueryAllPosts', {
typeName: 'Query',
fieldName: 'allPosts',
requestMappingTemplate: appsync.MappingTemplate.lambdaRequest(requestPayload('allPosts', {})),
responseMappingTemplate,
});

lambdaDS.createResolver({
lambdaDS.createResolver('MutationAddPost', {
typeName: 'Mutation',
fieldName: 'addPost',
requestMappingTemplate: appsync.MappingTemplate.lambdaRequest(requestPayload('addPost', { withArgs: true })),
responseMappingTemplate,
});

lambdaDS.createResolver({
lambdaDS.createResolver('PostRelatedPosts', {
typeName: 'Post',
fieldName: 'relatedPosts',
requestMappingTemplate: appsync.MappingTemplate.lambdaRequest(requestPayload('relatedPosts', { withSource: true }), 'BatchInvoke'),
responseMappingTemplate,
});

lambdaDS.createResolver({
lambdaDS.createResolver('PostRelatedPostsMaxBatchSize', {
typeName: 'Post',
fieldName: 'relatedPostsMaxBatchSize',
requestMappingTemplate: appsync.MappingTemplate.lambdaRequest(requestPayload('relatedPostsMaxBatchSize', { withSource: true }), 'BatchInvoke'),
responseMappingTemplate,
maxBatchSize: 2,
});

app.synth();
app.synth();
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{
"version": "21.0.0",
"version": "22.0.0",
"files": {
"b0462850439179659920597f4327262b24073af4f4969622163b0a295fce1dda": {
"de7d932209c6d07c0ba0e631387676246a8182018b2b7423dc18b52baec3e984": {
"source": {
"path": "aws-appsync-integ.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "b0462850439179659920597f4327262b24073af4f4969622163b0a295fce1dda.json",
"objectKey": "de7d932209c6d07c0ba0e631387676246a8182018b2b7423dc18b52baec3e984.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@
}
}
},
"ApitestDataSourceQuerygetTestsResolverA3BBB672": {
"ApiQueryGetTestsF8C40170": {
"Type": "AWS::AppSync::Resolver",
"Properties": {
"ApiId": {
Expand All @@ -142,7 +142,7 @@
"ApitestDataSource96AE54D5"
]
},
"ApitestDataSourceMutationaddTestResolver36203D6B": {
"ApiMutationAddTestBF148084": {
"Type": "AWS::AppSync::Resolver",
"Properties": {
"ApiId": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"21.0.0"}
{"version":"22.0.0"}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "21.0.0",
"version": "22.0.0",
"testCases": {
"integ.auth-apikey": {
"stacks": [
Expand Down
Loading

0 comments on commit 6dc15d4

Please sign in to comment.