refactor(appsync): strongly type schema definition mode

[BryanPan342]

[ISSUE]
schema definition mode is not strongly typed.

[APPROACH]
Make input prop schemaDefinition take a enum that allows users to choose between 2 modes: CODE and FILE.

[NOTE]
This approach was taken in preparation for a code-first approach to schema generation with CDK. All of the functions tagged @experimental are subject to change.

BREAKING CHANGE: appsync prop schemaDefinition no longer takes string, instead it is required to configure schema definition mode.

• appsync: schemaDefinition takes param SchemaDefinition.XXX to declare how schema will be configured
  • SchemaDefinition.CODE allows schema definition through CDK
  • SchemaDefinition.FILE allows schema definition through schema.graphql file

Fixes #9301

---

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

[BryanPan342]

EDIT: Decided against having this s3-assets in this PR because it covered too much surface area

---

@asterikx I implemented the ability to use s3-assets with this PR. I know it doesn't have the complete suite of functionality that your issue brought up, but thought that this might be a good compromise for now!

Would love to know your thoughts! You can check out an example of it in action here.

Addresses #8052

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[MrArnoldPalmer]

Since this removes the ability to pass the schemaDefinition prop as a string, we can probably simplify it a bit. If the user passes the schemaDefinitionFile prop, we know they are using a schema file and not defining with the construct methods. Otherwise if schemaDefinitionFile is undefined then they are defining via code.

[BryanPan342]

Since this removes the ability to pass the schemaDefinition prop as a string, we can probably simplify it a bit. If the user passes the schemaDefinitionFile prop, we know they are using a schema file and not defining with the construct methods. Otherwise if schemaDefinitionFile is undefined then they are defining via code.

Yeah originally I think that was the move I was thinking, but then I wanted to add functionality for s3Location and wanted to overload the schemaDefinitionFile prop.

@MrArnoldPalmer do you think it would be better to skip this optimization?

[MrArnoldPalmer]

That makes sense. With future plans to add more options I think that's a good enough reason to keep the Enum. 👍👍

[mergify]

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).

[asterikx]

@BryanPan342 sorry for being late to the party.

I understand the idea of overloading the the schemaDefinitionFile prop.
But I think it can all be handled by the schemaDefinition prop (so that schemaDefinitionFile could be removed).
I would have imagined the schemaDefinition prop (or just schema for short) similar to the code prop of lambda.FunctionProps.

schema: appsync.Schema.fromInline(schema)

schema: appsync.Schema.fromAsset(path)

schema: appsync.Schema.fromBucket(bucket, key[, objectVersion])

There's one catch with my suggested approach. In case of appsync.Schema.fromAsset(...) it's unclear if that schema file should be inlined into the synthesized CFN template or uploaded to S3 (whereas Lambda code must be uploaded to S3 either way). Maybe the CDK can be opinionated here and follow best practices if any.

That is just my 2 cents :)

[BryanPan342]

@asterikx

Thanks for the feedback!

I would have imagined the schemaDefinition prop (or just schema for short) similar to the code prop of lambda.FunctionProps.

This is an interesting approach but would require a bit of refactoring. Currently, the GraphQL Api and Schema are generated in the same construct despite being separate Cfn resources. Originally, we were thinking about offering a code-first approach to creating the Schema and its resolvers (see #9305) so an enum seemed logical.

I'm guessing the implementation of this Schema class would look something like this?

class Schema extends Construct {
  
  // No definition for now because of code-first implementation
  fromInline(): Schema {
    return new Schema(SchemaDefinition.CODE, '');
  }

  fromFile(file: string): Schema {
    return new Schema(SchemaDefinition.FILE, readFileSync(file).toString('UTF-8'));
  }

  public readonly mode: SchemaDefinition;

  public readonly schema: CfnGraphQLSchema;

  private constructor(mode: SchemaDefinition, definition: string) {
    this.mode = mode;
    this.schema = new CfnGraphQLSchema(this, 'Schema', {
      apiId: <?>,     // Not sure how we can pass in the apiId here.. if anything we can leave it blank and fill it in later
      definition,
    });
  }
}

I wouldn't mind something along the lines of this, to be frank, but would love @MrArnoldPalmer opinion.

There's one catch with my suggested approach. In case of appsync.Schema.fromAsset(...) it's unclear if that schema file should be inlined into the synthesized CFN template or uploaded to S3 (whereas Lambda code must be uploaded to S3 either way). Maybe the CDK can be opinionated here and follow best practices if any.

I think we can use the CFN template because if the CFN template gets too big, then it will turn it into an s3 asset anyways. I believe the max will turn out to be 460 kb (see limits. I think Lambda uses an asset mainly because those lambda functions can end up being huge, like 200 Mb.

[asterikx]

I'm guessing the implementation of this Schema class would look something like this?

Yeah, I think it could be very similar to the Code class.
A Lambda executes code, an AppSync API executes a schema. I'm speaking mainly from a developer's point of view and might not consider CFN specifics.

I'm a bit hesitant about that code-first approach. I left a #9305 (comment) under the issue.