feat: Support apiId(Multiple cloudformation stacks) #597

HumbleBeck · 2023-04-26T13:28:54Z

Closes #595

jeremycare · 2023-04-26T15:17:38Z

src/resources/Api.ts

    }
+    //   if (


You might have forgotten to remove this comment.

Hideman85 · 2023-04-27T14:10:19Z

@HumbleBeck I have the same need, first thank you for your work and this is already helping me.
Now I have the following:

// serverless-compose.yml
services:
  service-root:
    path: serverless-definitions/service-root
  service-partA:
    path: serverless-definitions/service-partA
    dependsOn: service-root
    params:
      ParameterApiId: ${service-root.ParameterApiId}
  service-partB:
    path: serverless-definitions/service-partB
    dependsOn: service-root
    params:
      ParameterApiId: ${service-root.ParameterApiId}

And I'm getting this error in the other stacks:

Template format error: Unresolved resource dependencies [GraphQlSchema, ParameterApiId] in the Resources block of the template

Checkking the generated CF and getting on all the resolvers "DependsOn": ["GraphQlSchema"], but this resource is only on the root one

Do you think you can have a look at it? Or am I doing something wrong?

HumbleBeck · 2023-04-27T14:32:27Z

hey @Hideman85, indeed, I think I've missed this part thank you Just pushed a commit, it should fix it.

simonmeyerrr · 2023-05-02T13:55:44Z

src/types/plugin.ts

@@ -2,9 +2,9 @@ import { CfnWafRuleStatement, IntrinsicFunction } from './cloudFormation';

 export type AppSyncConfig = {


If most of the parameters are ignored when using apiId you should define type like this to avoid misusage of the config, it can be nice for those who use this type in their serverless.ts:

export type BaseAppSyncConfig = { dataSources: Record<string, DataSourceConfig>; resolvers: Record<string, ResolverConfig>; pipelineFunctions: Record<string, PipelineFunctionConfig>; substitutions?: Substitutions; caching?: CachingConfig; }; export type NewAppSyncConfig = BaseAppSyncConfig & { name: string; schema: string[]; authentication: Auth; additionalAuthentications: Auth[]; domain?: DomainConfig; apiKeys?: Record<string, ApiKeyConfig>; xrayEnabled?: boolean; logging?: LoggingConfig; waf?: WafConfig; tags?: Record<string, string>; }; export type ExistingAppSyncConfig = BaseAppSyncConfig & { apiId: string | IntrinsicFunction; }; export type AppSyncConfig = NewAppSyncConfig | ExistingAppSyncConfig;

I agree on this and it joins what I commented earlier.
The same should happen in the validation json schema.
I would use something like a union.

export type BaseAppSyncConfig = { dataSources: Record<string, DataSourceConfig>; resolvers: Record<string, ResolverConfig>; pipelineFunctions: Record<string, PipelineFunctionConfig>; substitutions?: Substitutions; caching?: CachingConfig; }; export type FullAppSyncConfig = BaseAppSyncConfig & { name: string; schema: string[]; authentication: Auth; additionalAuthentications: Auth[]; domain?: DomainConfig; apiKeys?: Record<string, ApiKeyConfig>; xrayEnabled?: boolean; logging?: LoggingConfig; waf?: WafConfig; tags?: Record<string, string>; }; export type SharedAppSyncConfig = BaseAppSyncConfig & { apiId: string; }; export type AppSyncConfig = FullAppSyncConfig | SharedAppSyncConfig

(not tested, might need adjustments)

bboure · 2023-05-08T09:04:31Z

Thanks all for the great feedback and this PR

I will take everything into consideration and have a look at this as soon as I can

Hideman85 · 2023-05-25T07:26:11Z

@bboure Sorry to disturb you, have you considered merging this fix for now? Right now I'm using the fork in our flow and this add quite some overhead in our process. This would be awesome to have this released 🙏

morficus · 2023-05-26T03:05:53Z

@bboure +1 for getting this merged in.

my use case is that I have a large AppSync API with v1 of this plugin. we are upgrading to v2 and, at the same time, splitting the API into "services" and using serverless-compose to manage them. being able to have an individual "service" update parts of the AppSync API would be phenomenal since it would make things more manageable and speed up deployments.

thank you @HumbleBeck for the awesome work 🙏

bboure · 2023-05-26T06:04:25Z

Sorry, I'll try to have a look at this over the weekend.

Meanwhile, AWS released support for merged APIs. I would like to have a look at it too

I think both are probably good to support but I'd like to understand how they fit together, etc.

Thanks for the work and your patience

morficus · 2023-05-26T13:22:28Z

@bboure OH WOW 😲 I did not hear about the new "merged API" functionality. AppSync finally has a solution for "schema federation" like Apollo. I'm going to have to dig into the blog post over the weekend.

Totally agree that it would be great to support both.

morficus · 2023-06-08T14:16:43Z

hey @bboure -- sorry to bug you again 😅 wondering if you were able to put some thought toward this PR or if you had a different approach in mind.

bboure · 2023-06-08T16:04:22Z

doc/general-config.md

+> Note: you should never specify this parameter if you're managing your AppSync through this plugin since it results in removing your API.
+
+### Schema
+After specifying this parameter, you need to manually keep your schema up to date or from the main stack where your root AppSync API is defined. The plugin is not taking into account schema property due to AppSync limitation and inability to merge schemas across multiple stacks


This whole section is confusing and probably not necessary.

Instead, we should add clear guidelines on how and why you should use the apiId parameters.
A bit like described here for API Gateway.

bboure · 2023-06-08T16:25:56Z

src/index.ts

@@ -410,6 +421,10 @@ class ServerlessAppsyncPlugin {
  async getIntrospection() {
    const apiId = await this.getApiId();

+    if (typeof apiId !== 'string') {
+      return;


just returning is probably not good enough.
I would show a warning.

In fact I would probably not allow calling any of these commands (assoc domain, etc) from a "child" service stack.

Please call this command from the service where the AppSync API is created.

bboure · 2023-06-08T16:27:33Z

src/index.ts

@@ -377,6 +384,10 @@ class ServerlessAppsyncPlugin {
  async gatherData() {
    const apiId = await this.getApiId();

+    if (typeof apiId !== 'string') {


Suggested change

if (typeof apiId !== 'string') {

if (!apiId) {

if for some reason, apiId is an empty string, it probably does not make sense to continue either.

bboure · 2023-06-08T16:32:48Z

src/resources/Api.ts

+            - waf
+            - tags
+        `);
+    }


I'd just validate this.
i.e. don't allow these fields is present in the config along with apiId
The validator will take care of showing the warning/error (depending on configuration)

bboure · 2023-06-08T16:36:59Z

src/resources/Api.ts

-      endpointResource.Properties,
-      this.compileAuthenticationProvider(this.config.authentication),
-    );
+    if (this.config.authentication) {


authentication is always required in this context.
There is probably some TS magic to do (i.e. a type union type or something)

bboure · 2023-06-08T16:48:59Z

src/resources/Api.ts

  }

  hasPipelineFunction(name: string) {
-    return name in this.config.pipelineFunctions;
+    return name in (this.config.pipelineFunctions || {});


I understand why you made those optional, but I'd rather keep them as required.
If you look here, those are actually already optional from a config point of view.
Then, getAppSyncConfig() makes sure to fill them with empty {} if needed for when it's injected in the compiler.

bboure · 2023-06-08T16:53:18Z

src/types/plugin.ts

@@ -2,9 +2,9 @@ import { CfnWafRuleStatement, IntrinsicFunction } from './cloudFormation';

 export type AppSyncConfig = {


I agree on this and it joins what I commented earlier.
The same should happen in the validation json schema.
I would use something like a union.

export type BaseAppSyncConfig = { dataSources: Record<string, DataSourceConfig>; resolvers: Record<string, ResolverConfig>; pipelineFunctions: Record<string, PipelineFunctionConfig>; substitutions?: Substitutions; caching?: CachingConfig; }; export type FullAppSyncConfig = BaseAppSyncConfig & { name: string; schema: string[]; authentication: Auth; additionalAuthentications: Auth[]; domain?: DomainConfig; apiKeys?: Record<string, ApiKeyConfig>; xrayEnabled?: boolean; logging?: LoggingConfig; waf?: WafConfig; tags?: Record<string, string>; }; export type SharedAppSyncConfig = BaseAppSyncConfig & { apiId: string; }; export type AppSyncConfig = FullAppSyncConfig | SharedAppSyncConfig

(not tested, might need adjustments)

bboure · 2023-06-08T16:55:38Z

src/validation.ts

  },
-  required: ['name', 'authentication'],
+  required: ['name'],


as mentioned above, we should have 2 schemas: one for a usual appsync config (main api or single stack) and one for "external API" which only accepts supporters fields.
This way, authentication can remain required in single stack scenarios or for main stack and be "forbidden" for sub stacks

bboure · 2023-06-08T16:58:51Z

Thank you for your patience, I had a look at the PR. It looks good but I left some comments for improvements. Espscially on typing and validation.

A few more thoughts/questions:

should we allow exporting/importing data sources from other stacks?
e.g. I create my data sources in a main stack (could be a single-table DynamoDB), and then I reference it from resolvers in other stacks.
Right now, we use an intrinsic function to get the name, which will not work if it's in a different stack. I assume passing the value/name as-is should work.

The same goes for pipeline functions.

We also need to update the migration guide and remove the reference that using existing APIs is not supported since this is being re-introduced.
I'd really like to have a full guide on what this is for and when and how to use it.

Thank you all for the effort!

morficus · 2023-06-12T03:05:27Z

cc: @HumbleBeck ☝️ Check out the comments left on the PR.

morficus · 2023-06-12T03:11:27Z

I'd really like to have a full guide on what this is for and when and how to use it.

@bboure I can volunteer to help with this part.

should we allow exporting/importing data sources from other stacks?

my vote on this is no, because as you said, passing the name as-is works. also sharing the same data source between stacks smells like an anti-pattern (unless using single-table design with DynamoDB, which I have not seen people do when using AppSync)

alexandre-snr · 2023-06-28T13:19:28Z

should we allow exporting/importing data sources from other stacks?

I am interested in this feature, I'd vote yes for the DynamoDB single tables, I feel like this is a very important use case

HumbleBeck · 2023-06-28T14:24:16Z

Hey guys, Thanks for waiting. I was occupied with chores. I'll review the comments by EOW. Meanwhile, can I ask you, @morficus, to help me with documentation/guide? I'll drop this part from my PR.

DaRo0 · 2023-09-01T09:37:22Z

I'm really interested in this feature! we can't update our library until this is finished. Just out of curiosity, @HumbleBeck were you able to move forward with your PR? Thanks a lot everyone for the great effort!

BohdanShuliaka · 2023-09-01T09:40:52Z

@HumbleBeck +1 for getting this merged in.

bboure · 2023-11-10T15:02:28Z

What is the status of this @HumbleBeck ?

DaRo0 · 2024-02-28T10:38:02Z

@bboure @HumbleBeck did this fell under the table? I don't know if I can help somehow, besides having no knowledge at all

HumbleBeck added 2 commits April 26, 2023 16:26

apiId

9426060

update snapshots

3c5c8f5

jeremycare reviewed Apr 26, 2023

View reviewed changes

src/resources/Api.ts Outdated

}

// if (

Copy link

jeremycare Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might have forgotten to remove this comment.

HumbleBeck reacted with thumbs up emoji

cleanup

489896d

drop depends on schema if apiId is provided

5c699d1

simonmeyerrr reviewed May 2, 2023

View reviewed changes

bboure changed the title ~~Support apiId(Multiple cloudformation stacks)~~ feat: Support apiId(Multiple cloudformation stacks) May 8, 2023

bboure reviewed Jun 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support apiId(Multiple cloudformation stacks) #597

feat: Support apiId(Multiple cloudformation stacks) #597

HumbleBeck commented Apr 26, 2023 •

edited

jeremycare Apr 26, 2023

Hideman85 commented Apr 27, 2023 •

edited

HumbleBeck commented Apr 27, 2023

simonmeyerrr May 2, 2023 •

edited

bboure Jun 8, 2023

bboure commented May 8, 2023

Hideman85 commented May 25, 2023

morficus commented May 26, 2023 •

edited

bboure commented May 26, 2023

morficus commented May 26, 2023

morficus commented Jun 8, 2023

bboure Jun 8, 2023

bboure Jun 8, 2023

bboure Jun 8, 2023

bboure Jun 8, 2023

bboure Jun 8, 2023

bboure Jun 8, 2023

bboure Jun 8, 2023

bboure Jun 8, 2023

bboure commented Jun 8, 2023

morficus commented Jun 12, 2023

morficus commented Jun 12, 2023

alexandre-snr commented Jun 28, 2023

HumbleBeck commented Jun 28, 2023

DaRo0 commented Sep 1, 2023

BohdanShuliaka commented Sep 1, 2023

bboure commented Nov 10, 2023

DaRo0 commented Feb 28, 2024

		@@ -2,9 +2,9 @@ import { CfnWafRuleStatement, IntrinsicFunction } from './cloudFormation';

		export type AppSyncConfig = {

feat: Support apiId(Multiple cloudformation stacks) #597

Are you sure you want to change the base?

feat: Support apiId(Multiple cloudformation stacks) #597

Conversation

HumbleBeck commented Apr 26, 2023 • edited

Choose a reason for hiding this comment

Hideman85 commented Apr 27, 2023 • edited

HumbleBeck commented Apr 27, 2023

simonmeyerrr May 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboure commented May 8, 2023

Hideman85 commented May 25, 2023

morficus commented May 26, 2023 • edited

bboure commented May 26, 2023

morficus commented May 26, 2023

morficus commented Jun 8, 2023

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

bboure commented Jun 8, 2023

morficus commented Jun 12, 2023

morficus commented Jun 12, 2023

alexandre-snr commented Jun 28, 2023

HumbleBeck commented Jun 28, 2023

DaRo0 commented Sep 1, 2023

BohdanShuliaka commented Sep 1, 2023

bboure commented Nov 10, 2023

DaRo0 commented Feb 28, 2024

HumbleBeck commented Apr 26, 2023 •

edited

Hideman85 commented Apr 27, 2023 •

edited

simonmeyerrr May 2, 2023 •

edited

morficus commented May 26, 2023 •

edited