feat(appsync): merged APIs by jumic · Pull Request #26895 · aws/aws-cdk

jumic · 2023-08-26T14:11:12Z

Add support for AppSync Merged API feature.

At the moment, a GraphQL schema can be passed using the schema property. I deprecated this property because it is not used for merged APIs.
Depecreated syntax:

const api = new appsync.GraphqlApi(this, 'Api', {
  name: 'demo',
  schema: appsync.SchemaFile.fromAsset(path.join(__dirname, 'schema.graphql')),
});

Instead, I introduced a new property apiSource that can be used to create a AppSync GraphQL API or Merged API.
GraphQL API:

const api = new appsync.GraphqlApi(this, 'Api', {
  name: 'demo',
  apiSource: appsync.ApiSource.fromSchema(appsync.SchemaFile.fromAsset(path.join(__dirname, 'schema.graphql'))),
  // short version
  apiSource: appsync.ApiSource.fromFile(path.join(__dirname, 'schema.graphql')),
});

Merged API:

const api = new appsync.GraphqlApi(this, 'Api', {
  name: 'demo',
  apiSource: appsync.ApiSource.fromSourceApis({
    sourceApis: [
      {
        sourceApi: firstApi,
        mergeType: appsync.MergeType.MANUAL_MERGE,
      },
      {
        sourceApi: secondApi,
        mergeType: appsync.MergeType.AUTO_MERGE,
      },
    ],
  }),
});

Closes #25960.

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

rix0rrr

Thanks, I love it!

I have one lingering concern on naming, but I'm also having a hard time coming up with something 😅

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

rix0rrr · 2023-08-29T10:10:47Z

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

  authorizationConfig: {},
  name: 'myApi',
-  schema: appsync.SchemaFile.fromAsset(path.join(__dirname, 'myApi.graphql')),
+  apiSource: appsync.ApiSource.fromFile(path.join(__dirname, 'myApi.graphql')),


I know I came up with apiSource myself, but I'm not a 100% happy with it 😅 . Have you given any thought to alternatives? Is there a term that AppSync itself uses for that might apply here?

Probably schema should be part of the name. Both GraphQL APIs and Merged APIs have a schema configuration in the AWS console:

GraphQL API: Design your schema using GraphQL SDL, attach resolvers, and quickly deploy AWS resources.

Merged API: Your Merged API’s schema is created from the schemas of your included Source APIs, and is read only. Changes to the schema must be initiated in your source APIs.

How about schemaSource or schemaDefinition?

@elthrasher @Lock128 After using the Merged API in our hackathon project: Do you have any feedback or ideas for a new name? We are looking for a better name than apiSource (see examples in the pull request).

I like "definition". How about just definition?: Definition or apiDefinition?: ApiDefinition ?

Let's call it definition. Other attributes like name don't use api as a prefix, so I'm fine with definition.

rix0rrr · 2023-08-29T10:11:23Z

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

+  /**
+   * Merging option used to associate the source API to the Merged API
+   *
+   * @default - Manual merge, requires the user to trigger schema merges manually.


Ooo, what does that mean?

Doesn't automatic sound like a better default?

Developers must manually press the Merge Now button in the AWS console to merge changes from source APIs into the Merged API. This is the default behaviour in CloudFormation (see documentation) and when you manually create a Merged API in the AWS console.

However, I used the AUTO_MERGE merge type in my project. So it's fine for me if we change it default to AUTO_MERGE in CDK. Please let me know which default value we should use.

Whatever default we choose should be friendly to CI/CD. If a merge cannot be triggered from CloudFormation, then I think automatic should be the default.

I'm impressed this service team actually built this feature! Let's use it!

Okay, I changed the default to AUTO_MERGE.

rix0rrr · 2023-08-29T10:11:45Z

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

+   * @param filePath the file path of the schema file
+   * @returns API Source with schema from file
+   */
+  public static fromFile(filePath: string): ApiSource {


rix0rrr · 2023-08-29T10:13:09Z

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

    this.validateAuthorizationProps(modes);

+    if ((props.schema !== undefined) === (props.apiSource !== undefined)) {
+      throw new Error('You cannot specify both properties schema and apiSource.');


And you must do at least one (so, exactly one).

(props.schema !== undefined) === (props.apiSource !== undefined) will also be true if both are undefined.

Done, another if-statement checks if schema or definition is set.

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

rix0rrr · 2023-08-29T10:15:24Z

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

+   * API source (schema file or source APIs) for this GraphQL Api
   */
-  public readonly schema: ISchema;
+  public readonly apiSource: ApiSource;


Let's make it easy on ourselves and make this private. I don't want to expose things on the public API unless absolutely necessary.

Wouldn't that be a breaking change? Currently, I can access the schema of the construct, but later I cannot?

schema will still be there, it just got moved.

(But it will throw if the API was not created from a schema)

Should schema be deprecated? I deprecated it in this PR. That means, it will be removed in CDK v3.
The schema will then be inaccessible, which could be considered a breaking change (information is no longer available). However, I am not sure how this property is used by other developers.

Thanks for letting me know, I had missed that.

Let's leave .schema as it is for now without deprecating. I'm also not quite sure what people would use it for; I'm inclined to think it's not necessary, but I also don't want to make too many assumptions.

Pull request has been modified.

rix0rrr

Thanks!

mergify · 2023-08-30T08:47:40Z

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

aws-cdk-automation · 2023-08-30T09:13:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 7f76948
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-08-30T09:13:59Z

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

jumic · 2023-08-30T09:18:52Z

Thanks!

@rix0rrr Thank you also for your help with this PR. 👍

ndejaco2 · 2023-09-05T22:01:00Z

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

+      this.mergedApiExecutionRole = sourceApiOptions.mergedApiExecutionRole;
+    } else {
+      const sourceApiArns = sourceApiOptions.sourceApis?.map(sourceApiOption => {
+        return sourceApiOption.sourceApi.arn;


We also need this role to be able to access all top level fields like: :*. Can you fix this to add both the source api arn alone and the source api arn with wildcard for all top level fields? https://docs.aws.amazon.com/appsync/latest/devguide/merged-api.html#execution-roles-merged-api

Thanks for pointing this out. I will fix it next week. I'm on holiday at the moment.

Hey, I am actually going to be in this code for an enhancement here so I can go ahead and submit these changes if you want?

TheOptimist · 2023-11-02T20:20:20Z

I don't mean to necro this, but the deprecation comment when using schema still references apiSource, which was really confusing until I came to read this PR :)

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

Line 412 in 3c7ca51

* @deprecated use apiSource.schema instead

What's next? I can't see myself having the time to make a PR for this change any time soon...an issue for documentation? Can someone else make a PR faster than me (probably)?

ndejaco2 · 2023-11-03T21:58:10Z

I don't mean to necro this, but the deprecation comment when using schema still references apiSource, which was really confusing until I came to read this PR :)

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

Line 412 in 3c7ca51

* @deprecated use apiSource.schema instead

What's next? I can't see myself having the time to make a PR for this change any time soon...an issue for documentation? Can someone else make a PR faster than me (probably)?

#27831

feat(appsync): support Merged APIs

0606fd4

aws-cdk-automation requested a review from a team August 26, 2023 14:11

github-actions bot added distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Aug 26, 2023

jumic mentioned this pull request Aug 26, 2023

AppSync Merged API L2 construct aws/aws-cdk-rfcs#526

Closed

11 tasks

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 26, 2023

rix0rrr changed the title ~~feat(appsync): support Merged APIs~~ feat(appsync): merged APIs Aug 29, 2023

rix0rrr previously requested changes Aug 29, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 29, 2023

change default merge type, rename apiSource to definition, small fixes

644df41

jumic requested a review from rix0rrr August 29, 2023 21:31

rix0rrr approved these changes Aug 30, 2023

View reviewed changes

Merge branch 'main' into appsync-merged-api

7f76948

mergify bot merged commit fe930a5 into aws:main Aug 30, 2023

github-actions bot mentioned this pull request Sep 1, 2023

Monthly PRs metrics report - Aug 2023 #26965

Closed

ndejaco2 reviewed Sep 5, 2023

View reviewed changes

jumic mentioned this pull request Sep 6, 2023

fix(appsync): Source APIs are not auto-merged by default #27025

Closed

This was referenced Sep 13, 2023

feat(appsync): Standalone L2 construct for SourceApiAssociation #27121

Merged

(aws-appsync): Custom Resource to Perform Manual Merge of a Source API And Signal Cloudformation Based On Merge Operation Result #27170

Closed

Conversation

jumic commented Aug 26, 2023

Uh oh!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 30, 2023

Uh oh!

aws-cdk-automation commented Aug 30, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 30, 2023

Uh oh!

jumic commented Aug 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheOptimist commented Nov 2, 2023

Uh oh!

ndejaco2 commented Nov 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rix0rrr Aug 29, 2023 •

edited

Loading

rix0rrr Aug 29, 2023 •

edited

Loading