Skip to content

Commit

Permalink
fix(appsync): unexpected resolver replacement
Browse files Browse the repository at this point in the history 
Fixes an issue where users couldn't control the ID's of appsync
resolvers and functions which would cause them to be replaced whenever
the data source of the resolver or function was replaced. For resolvers,
this could cause a deployment error as cloudformation would attempt to
create the new instance of the resolver before deleting the old one
throwing 'only one resolver per field' error.

Removing `dataSource.createFunction` and `dataSource.createResolver` in
favor of the same methods on the `GraphQlApi` construct makes it clear
that the parent scope of resolvers and functions is an API and not a
data source and therefore wont be replaced when changing data sources.

BREAKING CHANGE: removes `createFunction` and `createResolver` from
`IDataSource`.

Fixes: #13269
  • Loading branch information
@MrArnoldPalmer
MrArnoldPalmer committed Dec 13, 2022
1 parent 6e4b4ea commit f63578a
Show file tree
Hide file tree
Showing 14 changed files with 138 additions and 93 deletions.
10 changes: 5 additions & 5 deletions packages/@aws-cdk/aws-appsync/lib/appsync-function.ts
View file
Expand Up @@ -31,6 +31,10 @@ export interface BaseAppsyncFunctionProps {
* @default - no response mapping template
*/
readonly responseMappingTemplate?: MappingTemplate;
/**
* the data source linked to this AppSync Function
*/
readonly dataSource: BaseDataSource;
}

/**
Expand All @@ -41,10 +45,6 @@ export interface AppsyncFunctionProps extends BaseAppsyncFunctionProps {
* the GraphQL Api linked to this AppSync Function
*/
readonly api: IGraphqlApi;
/**
* the data source linked to this AppSync Function
*/
readonly dataSource: BaseDataSource;
}

/**
Expand Down Expand Up @@ -145,4 +145,4 @@ export class AppsyncFunction extends Resource implements IAppsyncFunction {
this.function.addDependsOn(this.dataSource.ds);
props.api.addSchemaDependency(this.function);
}
}
}
24 changes: 0 additions & 24 deletions packages/@aws-cdk/aws-appsync/lib/data-source.ts
View file
Expand Up @@ -7,10 +7,8 @@ import { IServerlessCluster } from '@aws-cdk/aws-rds';
import { ISecret } from '@aws-cdk/aws-secretsmanager';
import { IResolvable, Lazy, Stack, Token } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { BaseAppsyncFunctionProps, AppsyncFunction } from './appsync-function';
import { CfnDataSource } from './appsync.generated';
import { IGraphqlApi } from './graphqlapi-base';
import { BaseResolverProps, Resolver } from './resolver';

/**
* Base properties for an AppSync datasource
Expand Down Expand Up @@ -128,28 +126,6 @@ export abstract class BaseDataSource extends Construct {
this.name = name;
this.api = props.api;
}

/**
* 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`, {
api: this.api,
dataSource: this,
...props,
});
}

/**
* 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`, {
api: this.api,
dataSource: this,
...props,
});
}
}

/**
Expand Down
22 changes: 19 additions & 3 deletions packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts
View file
Expand Up @@ -5,6 +5,7 @@ import { IDomain as IOpenSearchDomain } from '@aws-cdk/aws-opensearchservice';
import { IServerlessCluster } from '@aws-cdk/aws-rds';
import { ISecret } from '@aws-cdk/aws-secretsmanager';
import { CfnResource, IResource, Resource } from '@aws-cdk/core';
import { AppsyncFunction, BaseAppsyncFunctionProps } from './appsync-function';
import { DynamoDbDataSource, HttpDataSource, LambdaDataSource, NoneDataSource, RdsDataSource, AwsIamConfig, ElasticsearchDataSource, OpenSearchDataSource } from './data-source';
import { Resolver, ExtendedResolverProps } from './resolver';

Expand Down Expand Up @@ -134,7 +135,12 @@ 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;

/**
* creates a new appsync function for this datasource and API using the given properties
*/
createFunction(id: string, props: BaseAppsyncFunctionProps): AppsyncFunction;

/**
* Add schema dependency if not imported
Expand Down Expand Up @@ -285,8 +291,18 @@ 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,
});
}

/**
* creates a new appsync function for this datasource and API using the given properties
*/
public createFunction(id: string, props: BaseAppsyncFunctionProps): AppsyncFunction {
return new AppsyncFunction(this, id, {
api: this,
...props,
});
Expand Down
15 changes: 10 additions & 5 deletions packages/@aws-cdk/aws-appsync/test/appsync-caching-config.test.ts
View file
Expand Up @@ -33,7 +33,8 @@ describe('Lambda caching config', () => {
// WHEN
const lambdaDS = api.addLambdaDataSource('LambdaDS', func);

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

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

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

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

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

ds.createResolver({
api.createResolver('TestField', {
dataSource: ds,
typeName: 'test',
fieldName: 'field',
});
Expand Down Expand Up @@ -164,4 +165,4 @@ describe('adding lambda data source from imported api', () => {
ApiId: { 'Fn::GetAtt': ['baseApiCDA4D43A', 'ApiId'] },
});
});
});
});
8 changes: 5 additions & 3 deletions packages/@aws-cdk/aws-appsync/test/appsync-mapping-template.test.ts
View file
Expand Up @@ -34,7 +34,8 @@ describe('Lambda Mapping Templates', () => {
// WHEN
const lambdaDS = api.addLambdaDataSource('LambdaDS', func);

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

lambdaDS.createResolver({
api.createResolver('PostRelatedPosts', {
dataSource: lambdaDS,
typeName: 'Post',
fieldName: 'relatedPosts',
requestMappingTemplate: appsync.MappingTemplate.lambdaRequest('$util.toJson($ctx)', 'BatchInvoke'),
Expand All @@ -67,4 +69,4 @@ describe('Lambda Mapping Templates', () => {
MaxBatchSize: 10,
});
});
});
});
21 changes: 13 additions & 8 deletions packages/@aws-cdk/aws-appsync/test/appsync.test.ts
View file
Expand Up @@ -21,13 +21,15 @@ beforeEach(() => {
test('appsync should configure pipeline when pipelineConfig has contents', () => {
// WHEN
const ds = api.addNoneDataSource('none');
const test1 = ds.createFunction({
const test1 = api.createFunction('Test1Function', {
dataSource: ds,
name: 'test1',
});
const test2 = ds.createFunction({
const test2 = api.createFunction('Test2Function', {
dataSource: ds,
name: 'test2',
});
api.createResolver({
api.createResolver('TestTest2', {
typeName: 'test',
fieldName: 'test2',
pipelineConfig: [test1, test2],
Expand All @@ -48,16 +50,19 @@ 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 = api.createFunction('Test1Function', {
dataSource: ds,
name: 'test1',
});
const test2 = ds.createFunction({
const test2 = api.createFunction('Test2Function', {
dataSource: ds,
name: 'test2',
});

// THEN
expect(() => {
ds.createResolver({
api.createResolver('TestTest2', {
dataSource: ds,
typeName: 'test',
fieldName: 'test2',
pipelineConfig: [test1, test2],
Expand All @@ -83,7 +88,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 +242,4 @@ test('log retention should not appear when no retention time is specified', () =

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

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

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

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

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

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

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

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

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

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

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

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

app.synth();
app.synth();
6 changes: 4 additions & 2 deletions packages/@aws-cdk/aws-appsync/test/integ.auth-apikey.ts
View file
Expand Up @@ -46,14 +46,16 @@ const testTable = new Table(stack, 'TestTable', {

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

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

testDS.createResolver({
api.createResolver('MutationAddTest', {
dataSource: testDS,
typeName: 'Mutation',
fieldName: 'addTest',
requestMappingTemplate: MappingTemplate.dynamoDbPutItem(PrimaryKey.partition('id').auto(), Values.projecting('test')),
Expand Down
5 changes: 3 additions & 2 deletions packages/@aws-cdk/aws-appsync/test/integ.graphql-elasticsearch.ts
View file
Expand Up @@ -27,7 +27,8 @@ const api = new appsync.GraphqlApi(stack, 'api', {

const ds = api.addElasticsearchDataSource('ds', domain);

ds.createResolver({
api.createResolver('QueryGetTests', {
dataSource: ds,
typeName: 'Query',
fieldName: 'getTests',
requestMappingTemplate: appsync.MappingTemplate.fromString(JSON.stringify({
Expand Down Expand Up @@ -63,4 +64,4 @@ ds.createResolver({
})),
});

app.synth();
app.synth();

0 comments on commit f63578a

Please sign in to comment.