Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support apiId(Multiple cloudformation stacks) #597

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

HumbleBeck
Copy link

@HumbleBeck HumbleBeck commented Apr 26, 2023

Closes #595

}
// if (

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.

@Hideman85
Copy link

Hideman85 commented Apr 27, 2023

@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
Copy link
Author

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

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

export type AppSyncConfig = {
Copy link

@simonmeyerrr simonmeyerrr May 2, 2023

Choose a reason for hiding this comment

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

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;

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 bboure changed the title Support apiId(Multiple cloudformation stacks) feat: Support apiId(Multiple cloudformation stacks) May 8, 2023
@bboure
Copy link
Collaborator

bboure commented May 8, 2023

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
Copy link

@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
Copy link
Contributor

morficus commented May 26, 2023

@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
Copy link
Collaborator

bboure commented May 26, 2023

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
Copy link
Contributor

@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
Copy link
Contributor

morficus commented Jun 8, 2023

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.

> 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

if (typeof apiId !== 'string') {
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

if (typeof apiId !== 'string') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

- waf
- tags
`);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

endpointResource.Properties,
this.compileAuthenticationProvider(this.config.authentication),
);
if (this.config.authentication) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

hasPipelineFunction(name: string) {
return name in this.config.pipelineFunctions;
return name in (this.config.pipelineFunctions || {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

export type AppSyncConfig = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

},
required: ['name', 'authentication'],
required: ['name'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

bboure commented Jun 8, 2023

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
Copy link
Contributor

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

@morficus
Copy link
Contributor

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
Copy link

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
Copy link
Author

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
Copy link

DaRo0 commented Sep 1, 2023

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
Copy link

@HumbleBeck +1 for getting this merged in.

@bboure
Copy link
Collaborator

bboure commented Nov 10, 2023

What is the status of this @HumbleBeck ?

@DaRo0
Copy link

DaRo0 commented Feb 28, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Single AppSync Api - Multi CloudFormation Stacks - Concurrency issue
10 participants