feat(appsync): import existing graphql api #9254

BryanPan342 · 2020-07-25T00:50:42Z

[ISSUE]
Appsync missing fromXxx functionality, making multiple stack appsync interaction impossible.

[Approach]
Created a base class for GraphQLApi and a IGraphQLApi interface for imports. Added fromXxxAttributes functions to code base that can add dataSources.

[Notes]
Only accessible props from IGraphQLApi are apiId and arn. Only accessible functions from IGraphQLApi are the addXxxDataSource functions.

Added props-physical-name:@aws-cdk/aws-appsync.GraphQLApiProps as linter exception because the breaking change isn't worth the return of making the physical name (name exists already).

Added from-method:@aws-cdk/aws-appsync.GraphQLApi as linter exception because a fromGraphQLApiAttributes function will turn into from_graph_q_l_api_attributes in python.

Fixes #6959

BREAKING CHANGE: appsync.addXxxDataSource name and description props are now optional and in an DataSourceOptions interface.

appsync: the props name and description in addXxxDataSource have been moved into new props options of type DataSourceOptions
appsync: DataSourceOptions.name defaults to id
appsync: DataSourceOptions.description defaults to undefined

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

shivlaks

a great first attempt! thanks for getting this ball rolling.
I think we need to tighten up some of the APIs and their signatures.

would love to also see what @MrArnoldPalmer thinks

packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

packages/@aws-cdk/aws-appsync/README.md

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

shivlaks · 2020-07-29T18:04:57Z

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

@@ -315,6 +372,8 @@ export class GraphQLApi extends Construct {
  public readonly arn: string;
  /**
   * the URL of the endpoint created by AppSync
+   *
+   * @attribute
   */
  public readonly graphQlUrl: string;


curious about the casing here (and everywhere really)... how does this translate to Python where we do snake casing?

does GraphQL get broken down into graph_q_l?

yeah i think in this case would be best to change all of them to graphqlUrl for props

^ this would be a breaking change tho so not sure if thats the best move for something so small..

@MrArnoldPalmer thoughts?

I'm not sure it needs to happen in this PR, but fixing the casing sounds like something we need to do regardless if it requires code that is not idiomatic for Python developers

yeah i think a chore might be better

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

shivlaks · 2020-07-29T18:06:46Z

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

+    construct.addDependsOn(this.schema);
+    return true;


do we need this method? it seems like it's substituting one API call (addDependsOn) with another (addSchemaDependency). any reason users can't just do it directly?

does it require validation? what's valid as a construct to add

there needs to be a dependency add for resolvers and the schema, but i was not comfortable passing schema's around in imports so essentially i needed a way to allow for adding a dependency only if the api wasn't imported.

this basically was my workaround.. if anything i can make this function protected

can we make it internal?

actually, I've thought about this a little more and I think that it might be good to offer a public api to add a dependency.. if the graphql api is in another stack, it will be deployed before the imported stack so the dependency doesn't matter too much?

but in the case that it does we should have a public api to allow for adding dependencies? if not i dont see why a public api is a net bad

if it enables use cases that users cannot achieve, then I agree.
this method calls construct.addDependsOn(schema) and returns true. can users just make that api call instead?

@shivlaks its just really annoying for users to have to do after creating each resolver.

The only reason its public as of right now is that I need IGraphQLApi to have some capability to reference a schema, I can't make it protected or the function will be inaccessible to the Resolver class.

packages/@aws-cdk/aws-appsync/package.json

Pull request has been modified.

packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

shivlaks · 2020-07-30T15:27:04Z

packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

+   * @param description The description of the data source
+   * @param table The DynamoDB table backing this data source [disable-awslint:ref-via-interface]
+   */
+  addDynamoDbDataSource( name: string, description: string, table: ITable ): DynamoDbDataSource;


it's consistent with the rest of the construct library and also compliant with design guidelines

packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

packages/@aws-cdk/aws-appsync/test/appsync-import.test.ts

packages/@aws-cdk/aws-appsync/test/integ.api-import.ts

packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

into appsync-imports

Pull request has been modified.

BryanPan342 · 2020-07-31T19:27:03Z

Wow this PR got fat real quick :(

To make reviewing easier, I will summarize and highlight important files:

FILES ADDRESSED BY BREAKING CHANGES (not that important):

integ.graphl-iam.expected.json
integ.graphl-iam.ts
integ.graphl.expected.json
integ.graphl.ts

INTEG TESTS:

integ.api-import.expected.json
integ.api-import.ts

[NEW] UNIT TEST FILES (mostly the same test but across different data sources):

appsync-dynamodb.test.ts
appsync-none.test.ts
appsync-http.test.ts
appsync-lambda.test.ts

LOGIC (most important):

[NEW] graphqlapi-base.ts: added an interface DataSourceOptions and changed implementation in base class to create defaults
graphqlapi.ts: unchanged
datasource.ts: unchanged
resolver.ts : unchanged

We are still discussing the topic of adding a dependency for resolvers to schema.

aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

Line 90 in a44a9b8

addSchemaDependency(construct: CfnResource): boolean;

packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

shivlaks · 2020-08-03T06:14:55Z

packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

+  /**
+   * The name of the data source
+   *
+   * @default - '<DataSourceType>CDKDefault'


perhaps rephrase toAutomatically generated name and include an example.
I'm not super fond of the default part but I agree CDK should be in the default.

can multiple data sources be added with the same "friendly name" or will that complain? Can I just add 2 data sources of the same type with the defaults?

No, i created a unit test to make sure of this. But essentially each resolver is a construct and a construct must have a unique name.

Pull request has been modified.

BryanPan342

change documentation to reflect breaking change

BryanPan342 · 2020-08-04T18:29:21Z

packages/@aws-cdk/aws-appsync/README.md

+  graphqlApiId: api.apiId,
+  graphqlArn: api.arn,
+});
+importedApi.addDynamoDbDataSource('TableDataSource', 'Table', table);


Suggested change

importedApi.addDynamoDbDataSource('TableDataSource', 'Table', table);

importedApi.addDynamoDbDataSource(table, 'tableId');

and change everywhere else

shivlaks

great job working through all the feedback on this one!
I left a few other minor comments and added a do-not-merge

please ensure that @MrArnoldPalmer also gives his blessing before dropping the label.

packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

shivlaks · 2020-08-05T22:55:43Z

packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

+  /**
+   * the ARN of the API
+   */
+  public abstract readonly arn: string;


did we want to give this a specific prefix before the arn - since we're introducing this, let's make sure it aligns with how we define arn elsewhere

if we change this here i feel like we need to change it for every where else.. including the GraphQLAPI.. i think this breaking change should happen in another pr

packages/@aws-cdk/aws-appsync/test/appsync-dynamodb.test.ts

Pull request has been modified.

mergify · 2020-08-14T23:47:04Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-08-15T00:11:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 5a15cf0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-08-15T00:11:45Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

BryanPan342 added 6 commits July 23, 2020 12:00

initial changes (breaks)

1e0fea7

non breaking

748b1c2

remove unneccesary imports

7b77d43

merging master sucks

fc12e14

Merge branch 'master' into appsync-imports

64b4f98

prelimin tests

3bde2e9

BryanPan342 added pr/do-not-merge This PR should not be merged at this time. @aws-cdk/aws-appsync Related to AWS AppSync labels Jul 25, 2020

BryanPan342 requested review from MrArnoldPalmer and shivlaks July 25, 2020 00:50

BryanPan342 self-assigned this Jul 25, 2020

BryanPan342 added 2 commits July 24, 2020 18:42

update readme

48f7824

more unit tests for core data sources

a68e1f6

BryanPan342 added this to the AppSync to 'developer preview' milestone Jul 29, 2020

shivlaks previously requested changes Jul 29, 2020

View reviewed changes

addresses suggestions

b0ea1a2

Merge branch 'master' into appsync-imports

d1e7c48

BryanPan342 requested a review from shivlaks July 29, 2020 20:06

shivlaks reviewed Jul 30, 2020

View reviewed changes

shivlaks previously requested changes Jul 30, 2020

View reviewed changes

BryanPan342 added 3 commits July 30, 2020 10:13

spacing before param

e136805

prelim changes to shiv's suggestions

07dfea2

Merge branch 'appsync-imports' of https://github.com/BryanPan342/aws-cdk

142c5f5

into appsync-imports

BryanPan342 added 2 commits July 31, 2020 10:53

merge master and update with options

7e9b775

round out unit tests

a44a9b8

BryanPan342 requested a review from shivlaks July 31, 2020 18:56

BryanPan342 added pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version. and removed pr/do-not-merge This PR should not be merged at this time. labels Jul 31, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Jul 31, 2020

BryanPan342 removed the contribution/core This is a PR that came from AWS. label Jul 31, 2020

shivlaks previously requested changes Aug 3, 2020

View reviewed changes

adjust documentation

d7678ba

BryanPan342 commented Aug 4, 2020

View reviewed changes

BryanPan342 added 3 commits August 5, 2020 11:46

changes to addXxxDataSource api and data source implementation

c066ba9

update Readme

ecc9145

update doc strings for addXxxDataSource

4976bab

BryanPan342 requested a review from shivlaks August 5, 2020 20:05

snakecasing

5243ef0

shivlaks added the pr/do-not-merge This PR should not be merged at this time. label Aug 5, 2020

shivlaks previously approved these changes Aug 5, 2020

View reviewed changes

address suggestions

a668e13

BryanPan342 requested a review from shivlaks August 6, 2020 17:59

BryanPan342 changed the title ~~feat(appsync): fromXxx functionality added to appsync~~ feat(appsync): import existing graphql api Aug 14, 2020

shivlaks approved these changes Aug 14, 2020

View reviewed changes

BryanPan342 removed the pr/do-not-merge This PR should not be merged at this time. label Aug 14, 2020

Merge branch 'master' into appsync-imports

5a15cf0

mergify bot merged commit 5732b8e into aws:master Aug 15, 2020

andrestone mentioned this pull request Aug 20, 2020

[appsync]: Data Source breaking change #9854

Closed

BryanPan342 deleted the appsync-imports branch September 8, 2020 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appsync): import existing graphql api #9254

feat(appsync): import existing graphql api #9254

BryanPan342 commented Jul 25, 2020 •

edited

Loading

shivlaks left a comment

shivlaks Jul 29, 2020

BryanPan342 Jul 29, 2020

BryanPan342 Jul 29, 2020

shivlaks Jul 30, 2020

BryanPan342 Jul 30, 2020

shivlaks Jul 29, 2020

BryanPan342 Jul 29, 2020

shivlaks Jul 30, 2020

BryanPan342 Jul 30, 2020

shivlaks Jul 31, 2020

BryanPan342 Jul 31, 2020

shivlaks Jul 30, 2020

BryanPan342 commented Jul 31, 2020

shivlaks Aug 3, 2020

BryanPan342 Aug 3, 2020

BryanPan342 left a comment

BryanPan342 Aug 4, 2020

shivlaks left a comment

shivlaks Aug 5, 2020

BryanPan342 Aug 6, 2020 •

edited

Loading

mergify bot commented Aug 14, 2020

aws-cdk-automation commented Aug 15, 2020

mergify bot commented Aug 15, 2020

	importedApi.addDynamoDbDataSource('TableDataSource', 'Table', table);
	importedApi.addDynamoDbDataSource(table, 'tableId');

feat(appsync): import existing graphql api #9254

feat(appsync): import existing graphql api #9254

Conversation

BryanPan342 commented Jul 25, 2020 • edited Loading

shivlaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanPan342 commented Jul 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanPan342 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivlaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanPan342 Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

mergify bot commented Aug 14, 2020

aws-cdk-automation commented Aug 15, 2020

AWS CodeBuild CI Report

mergify bot commented Aug 15, 2020

BryanPan342 commented Jul 25, 2020 •

edited

Loading

BryanPan342 Aug 6, 2020 •

edited

Loading