feat(appsync): separating schema from graphql api #9903

BryanPan342 · 2020-08-21T21:55:37Z

Separating GraphQL Schema from GraphQL Api to simplify GraphQL Api Props. GraphQL Schema is now its own class and employs static functions to construct GraphQL API.

By default, GraphQL Api will be configured to a code-first approach. To override this, use the schema property to specify a method of schema declaration. For example,

const api = appsync.GraphQLApi(stack, 'api', {
  name: 'myApi',
  schema: appsync.Schema.fromAsset(join(__dirname, 'schema.graphl')),
});

BREAKING CHANGES: AppSync GraphQL Schema declared through static functions as opposed to two separate properties

appsync: props SchemaDefinition and SchemaDefinitionFile have been condensed down to a singular property schema
appsync: no longer directly exposes CfnGraphQLSchema from GraphQLApi.schema

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

BryanPan342 · 2020-08-24T22:22:57Z

@asterikx I remember us talking about something similar to this but for s3.. i have a draft for s3 but will change it to follow this flow! We have also been talking about using assets recently but we will need to flesh this out more.

@andrestone thoughts on this? I think this is the most I'm willing to commit feature-wise for this PR, but will add more to both the Schema and GraphQLApi classes.

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

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

andrestone · 2020-08-25T13:07:38Z

Great stuff!

I played with the current build a bit and, after this PR, it feels like the only thing that is really missing is a better experience to code the mapping templates. When coding some basic examples I could rarely use the MappingTemplate helper methods, had to fallback to fromString / fromFile all the time.

Next step, I guess?

BryanPan342 · 2020-08-25T16:03:35Z

Great stuff!

I played with the current build a bit and, after this PR, it feels like the only thing that is really missing is a better experience to code the mapping templates. When coding some basic examples I could rarely use the MappingTemplate helper methods, had to fallback to fromString / fromFile all the time.

Next step, I guess?

Yeah there are a couple things that I still need to address to round off the code-first approach. Mainly, functions to simplify the creation of queries/mutations/subscriptions, supporting input and enums, and broadening support on directives.

Other than that, I totally agree.. Mapping Templates is definitely the next step. I have been looking into Direct Lambda Resolvers which honestly is so fantastic for AppSync users. But thinking about helping push @duarten's RFC for Mapping Templates.

andrestone · 2020-08-25T16:25:56Z

Other than that, I totally agree.. Mapping Templates is definitely the next step. I have been looking into Direct Lambda Resolvers which honestly is so fantastic for AppSync users. But thinking about helping push @duarten's RFC for Mapping Templates.

I actually find that post a bit misleading. Lambda resolvers were always "direct", the only thing that changes is that now there's no need to declare it manually in a resolver (which is a pretty straightforward step, anyway). There are many downsides (and, ofc, upsides) on using Lambda to resolve your fields (cost is most likely the important one) and I guess it should be considered as a last alternative.

The RFC path feels like it's a longer loop. I guess there's a lot of room for improvement on the MappingTemplate static methods.

Let me know if you're planning to work on those, otherwise I might give it a try.

BryanPan342 · 2020-08-25T16:34:27Z

Lambda resolvers were always "direct", the only thing that changes is that now there's no need to declare it manually in a resolver (which is pretty straightforward, anyway). There are many downsides (and, ofc, upsides) on using Lambda to resolve your fields (cost is most likely the most important one) and I guess it should be considered as a last alternative.

Mmmm yeah I see what you mean! There are definitely tradeoffs with everything. I think the Direct Lambda Resolver will benefit people who use the Console or CloudFormation Templates more than CDK users. Mainly because as an IaC service, CDK can over functions to make creating Mapping Templates more fluid 🙃

The RFC path feels like it's a longer loop. I guess there's a lot of room for improvement on the MappingTemplate static methods.

What functions were you thinking of making? I think one of the hardest parts of the MappingTemplate implementation right now is that there is no central focus/model. It was mostly created through community contribution which is great for some use cases, but I'm sure everyone would love it to be more comprehensive!

I'm willing to riff on some static functions to make Mapping Templates a lot more usable though while we wait on @duarten's RFC! If you want we can create a tracking issue for this and start creating artifacts so other community members can contribute to the discussion as well :)

andrestone · 2020-08-25T16:57:46Z

Mmmm yeah I see what you mean! There are definitely tradeoffs with everything. I think the Direct Lambda Resolver will benefit people who use the Console or CloudFormation Templates more than CDK users. Mainly because as an IaC service, CDK can over functions to make creating Mapping Templates more fluid 🙃

It's a bold can. As of today, writing VTL in the console is the best bad alternative there is :) That's the main reason why I think we should have a minimally pleasant experience in CDK. I mean, it's ok to write plain VTL here and there but if I have to write all of them, even for a relatively simple API, I'd rather do it in the console and then copy/pasta to my CDK / .vtl files (which is a pretty awful thing to do).

I think one of the hardest parts of the MappingTemplate implementation right now is that there is no central focus/model.

I agree and I think some sort of non-disruptive design must be implemented (nothing major I'd say).

I'm willing to riff on some static functions to make Mapping Templates a lot more usable though while we wait on @duarten's RFC! If you want we can create a tracking issue for this and start creating artifacts so other community members can contribute to the discussion as well :)

Not sure if a tracking issue is needed. I think a small refactoring would be enough to support a better design and would allow us to move faster with this.

Any thoughts, @shivlaks and @MrArnoldPalmer?

MrArnoldPalmer · 2020-08-26T16:25:38Z

@andrestone I'm all for some basic utils to help users in the meantime while we flesh out a more full experience. I would also really appreciate any community feedback on @duarten's proposal/library for the future "big" solution.

I wrote this issue awhile back to track mapping template improvements #8216. We can use that to track changes we want to add.

MrArnoldPalmer

💯

MrArnoldPalmer · 2020-08-26T16:28:34Z

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

-    }
-    const sep = delimiter ?? '';
-    this.schema.definition = `${this.schema.definition}${sep}${addition}\n`;
+  public addToSchema(addition: string, delimiter?: string): void {


NIT: I wonder if we need to surface this here, we could probably just allow users to call api.schema.addToSchema if they need the escape hatch.

good idea.. might be misleading to other schema-first users as well...

Ill remove it and the addObjectType and addInterfaceType in the following PR :)

actually nvm the queue to merge is fat.. ill remove it rn lmao

mergify · 2020-08-26T16:46:54Z

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 · 2020-08-26T21:30:58Z

@Mergifyio refresh

mergify · 2020-08-26T21:31:32Z

Command refresh: success

aws-cdk-automation · 2020-08-26T23:35:33Z

AWS CodeBuild CI Report

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

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

mergify · 2020-08-26T23:35:49Z

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 30 commits July 22, 2020 14:31

complete SchemaBaseType

46df9f0

add doc strings to some classes

84a7b4c

Merge branch 'master' into appsync-schema

f3350e3

preliminary changes

ede2020

create enum for schemaDefinition

d9271ec

adjusted SchemaDefinition to allow for S3

7904e62

change addType to addDefinition for temporary inclusion

4591a70

s3 testing

da20dcd

Merge branch 'master' into shema

6ede81e

prelim changes

4d48a2d

prelim run of addType

63e35b3

reorg schema functions

1b82340

== to ===

fad768a

chore up schema functions

5bec482

remove readme section on s3 assets

96021db

remove unneccessary prop this.types

0184c25

remove props

2a7c641

expected json file

076ebdf

listing implementation corrected

a351a04

update readme

8637009

fix bug in integ

0ecd95f

pls fix build bug

7005315

add todo

28f7e93

dont change list or required

fc98566

prelim testing for the OO design

c27a158

new implementation

72d8be4

oops.. forgot to stage changes

bff3e3b

revert changes to prevent merge conflicts

47d9c89

updated api for generating attribute types

1e2dd60

Merge branch 'master' into shema

d8ee61c

allow public definition

0fd0f2c

BryanPan342 added 3 commits August 24, 2020 15:33

Merge branch 'master' into schema

ba9f337

address changes

7ffe7c7

update readme

70d584e

andrestone reviewed Aug 25, 2020

View reviewed changes

packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved

andrestone reviewed Aug 25, 2020

View reviewed changes

packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved

andrestone reviewed Aug 25, 2020

View reviewed changes

packages/@aws-cdk/aws-appsync/README.md Show resolved Hide resolved

andrestone reviewed Aug 25, 2020

View reviewed changes

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts Outdated Show resolved Hide resolved

BryanPan342 added 2 commits August 25, 2020 08:55

address suggestions

8bb0a2a

Merge branch 'master' into schema

f08f4b8

jsii bug

b3f6843

MrArnoldPalmer approved these changes Aug 26, 2020

View reviewed changes

BryanPan342 added 2 commits August 26, 2020 12:54

remove addXxxType

b871a2a

Merge branch 'master' into schema

6f4f5f6

BryanPan342 added 2 commits August 26, 2020 14:39

Merge branch 'master' into schema

b24be5e

Merge branch 'master' into schema

4119220

mergify bot merged commit 8d71fa1 into aws:master Aug 26, 2020

BryanPan342 deleted the schema branch September 8, 2020 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appsync): separating schema from graphql api #9903

feat(appsync): separating schema from graphql api #9903

BryanPan342 commented Aug 21, 2020 •

edited

Loading

BryanPan342 commented Aug 24, 2020

andrestone commented Aug 25, 2020

BryanPan342 commented Aug 25, 2020

andrestone commented Aug 25, 2020 •

edited

Loading

BryanPan342 commented Aug 25, 2020

andrestone commented Aug 25, 2020 •

edited

Loading

MrArnoldPalmer commented Aug 26, 2020

MrArnoldPalmer left a comment

MrArnoldPalmer Aug 26, 2020

BryanPan342 Aug 26, 2020

BryanPan342 Aug 26, 2020

mergify bot commented Aug 26, 2020

BryanPan342 commented Aug 26, 2020

mergify bot commented Aug 26, 2020

aws-cdk-automation commented Aug 26, 2020

mergify bot commented Aug 26, 2020

feat(appsync): separating schema from graphql api #9903

feat(appsync): separating schema from graphql api #9903

Conversation

BryanPan342 commented Aug 21, 2020 • edited Loading

BryanPan342 commented Aug 24, 2020

andrestone commented Aug 25, 2020

BryanPan342 commented Aug 25, 2020

andrestone commented Aug 25, 2020 • edited Loading

BryanPan342 commented Aug 25, 2020

andrestone commented Aug 25, 2020 • edited Loading

MrArnoldPalmer commented Aug 26, 2020

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

MrArnoldPalmer Aug 26, 2020

Choose a reason for hiding this comment

BryanPan342 Aug 26, 2020

Choose a reason for hiding this comment

BryanPan342 Aug 26, 2020

Choose a reason for hiding this comment

mergify bot commented Aug 26, 2020

BryanPan342 commented Aug 26, 2020

mergify bot commented Aug 26, 2020

aws-cdk-automation commented Aug 26, 2020

AWS CodeBuild CI Report

mergify bot commented Aug 26, 2020

BryanPan342 commented Aug 21, 2020 •

edited

Loading

andrestone commented Aug 25, 2020 •

edited

Loading

andrestone commented Aug 25, 2020 •

edited

Loading