-
Notifications
You must be signed in to change notification settings - Fork 162
feat(event-handler): add support for AppSync GraphQL batch resolvers #4218
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
base: main
Are you sure you want to change the base?
feat(event-handler): add support for AppSync GraphQL batch resolvers #4218
Conversation
…lBatchRouteOptions
… AppSyncGraphQLResolver
… error handling behavior
…ncGraphQLResolver
… in AppSyncGraphQLResolver
…events in Router class
…ncGraphQLResolver
…ncAggregateHandlerFn types
…arding raiseOnError
…es for improved type safety and clarity
…ity and consistency
…th aggregate setting
const results: unknown[] = []; | ||
|
||
if (raiseOnError) { | ||
for (const event of events) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should give users the option to do these in parallel with Promise.all
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When raiseOnError
is true
, we stop on the first error. But if we use Promise.all
, I believe we won’t be able to stop at the first error, since the promises will run in parallel.
However, it should be possible when raiseOnError
is false
. Also, do we want to give the user the option for this or just do parallel processing by default, similar to what has been done in AppSyncEventResolver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way Promise.all
works is that the first error causes all in-flight promises to be rejected so I think that's still OK. But we would need to use Promise.allSettled
if raiseOnError
was false. Good point about following what's done in AppSyncEventResolver, I think we should do that.
Small nit about the raiseOnError
variable name, would throwOnError
not be more appropriate. The word raise
is more a Python thing. to my mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout about throwOn...
, we use this in several other places in this version of Powertools for AWS:
As well as probably others, so +1 on changing it to this.
Regarding the Promise.all
, if I remember correctly we didn't use it in the other Event Handler because even though the first rejected promises causes the "overall" promise to also be rejected, it doesn't abort the other in-flight promises, for example:
const promise1 = new Promise((resolve, reject) => {
setTimeout(resolve, 2000, 'resolve1');
}).then((a) => {
console.log('then1');
return a;
});
const promise2 = new Promise((resolve, reject) => {
setTimeout(reject, 1000, 'reject2');
}).then((a) => {
console.log('then2');
return a;
});
const promise3 = new Promise((resolve, reject) => {
setTimeout(resolve, 3000, 'resolve3');
}).then((a) => {
console.log('then3');
return a;
});
Promise.all([promise1, promise2, promise3])
.then((values) => {
console.log('then', values);
})
.catch((err) => {
console.log('catch', err);
});
results in:
catch reject2
then1
then3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, that makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed raiseOnError
to throwOnError
packages/event-handler/src/appsync-graphql/AppSyncGraphQLResolver.ts
Outdated
Show resolved
Hide resolved
packages/event-handler/tests/unit/appsync-graphql/AppSyncGraphQLResolver.test.ts
Outdated
Show resolved
Hide resolved
packages/event-handler/tests/unit/appsync-graphql/AppSyncGraphQLResolver.test.ts
Outdated
Show resolved
Hide resolved
|
Summary
This PR will add the ability to register batch resolvers for AppSync GraphQL API.
Changes
batchResolver
method is introduced to register handler for batch eventsProcess events individually
If you want to process each event individually instead of receiving all events at once, you can set the
aggregate
option tofalse
. In this case, the handler will be called once for each event in the batch,similar to regular resolvers.
Strict error handling
If you want stricter error handling when processing events individually, you can set the
raiseOnError
optionto
true
. In this case, if any event throws an error, the entire batch processing will stop and the errorwill be propagated. Note that
raiseOnError
can only be used whenaggregate
is set tofalse
.You can also specify the type of the arguments using generic type parameters for non-aggregated handlers:
As a decorator:
Also,
onBatchQuery
&onBatchMutation
methods are defined for registering batchQuery
&Mutation
events.aggregate
&raiseOnError
options are also available for these two methods.Issue number: #4100
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.