-
Notifications
You must be signed in to change notification settings - Fork 156
feat(event-handler): add single resolver functionality for AppSync GraphQL API #3999
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 single resolver functionality for AppSync GraphQL API #3999
Conversation
…tions for GraphQL events
…resolution details
…e event objects for GraphQL operations
…vent handling and error formatting
…egistration and resolution behavior
…tion and logging behavior
…pattern in `Router` class
…solver` class with examples and parameter descriptions
… RouteHandlerRegistry and Router tests
…phQLResolver and RouteHandlerRegistry
…n RouteHandlerRegistry
…egistry and related tests
Thanks for finding a bug in the Python documentation, @arnabrahman! You always exceed our standards with these catches ❤️ ! Yeah, the valid value is Thanks |
* @param options - Optional route options. | ||
* @param options.typeName - The name of the GraphQL type to use for the resolver (defaults to 'Query'). | ||
*/ | ||
public onQuery( |
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.
Can we make this and the other generic so that users can pass the type for the arguments?
For example:
app.onQuery<{ postId: string }>('getPost', async ({ postId }) => {
// ^ this is now type string
});
We did something similar for this other handler.
It'll work only for this notation and not work with decorators, but that's ok.
error: console.error, | ||
warn: console.warn, | ||
}; | ||
this.onQueryRegistry = new RouteHandlerRegistry({ |
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've been looking at the implementation in Python and I think we might need to also use a single registry rather than two.
If we keep in mind only handlers for specific fields in queries and mutations this implementation works - and is actually better - but in practice customers can also set handlers on custom types or not use a type at all.
For example, given this schema:
type Merchant {
id: String!
name: String!
description: String
locations: [Location]
}
customers should be able to set a resolver like:
app.resolver(async () => {
// ...
}, {
typeName: 'Merchant',
fieldName: 'location',
});
as well as one like:
app.resolver(async () => {
// ...
}, {
fieldName: 'location',
});
The last one is referred to as "nested mappings" in the Python implementation.
Because of this, I think it makes sense to use a single registry and use field and type names as keys - I don't have a preference around keeping them separate keys in the registry like we're doing now, or concatenating them like Python does - whatever is faster / easier.
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.
Thanks for your patience.
I ran some tests deploying this with an AppSync API and it works well, good work here!
I have left a couple comments:
- one around making the methods that register handlers generic to improve experience
- another to create a more generic
resolver
method that can handle other cases covered by the Python implementation
Besides this, I'd also see if we can add some helper functions like these for scalar types in this PR.
We can address the batchResolver
method in a separate PR.
Thanks again!
const isAppSyncGraphQLEvent = ( | ||
event: unknown | ||
): event is AppSyncGraphQLEvent => { | ||
if (typeof event !== 'object' || event === null || !isRecord(event)) { |
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.
This validation doesn't work now - I wasn't able to see the selectionSetList
field in my tests.
This did:
if (!isRecord(event)) {
return false;
}
return (
'identity' in event &&
'result' in event &&
isRecord(event.request) &&
isRecord(event.request.headers) &&
'domainName' in event.request &&
isRecord(event.info) &&
isString(event.info.fieldName) &&
isString(event.info.parentTypeName) &&
isRecord(event.info.variables) &&
'error' in event &&
'prev' in event &&
isRecord(event.stash) &&
Array.isArray(event.outErrors) &&
isRecord(event.arguments) &&
'source' in event
);
Note that isRecord
and isString
already imply that the value is present (aka not undefined
) so we can probably skip the 'field' in event && isRecord(event.field)
and just use isRecord(event.field)
unless TypeScript complains.
@dreamorosi Thanks for taking the time to review this. I will look at the suggestions and get back on this. |
fieldName | ||
); | ||
if (queryHandlerOptions) { | ||
return await queryHandlerOptions.handler.apply(this, [event.arguments]); |
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 caller of this function does the awaiting so there's no need to await here. Likewise on L136
expect(result).toBeUndefined(); | ||
}); | ||
|
||
it('throw error if there are no onQuery handlers', async () => { |
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.
Nit: should be 'throws`.
expect(console.error).toHaveBeenCalled(); | ||
}); | ||
|
||
it('throw error if there are no onMutation handlers', async () => { |
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.
s/throw/throws
packages/event-handler/tests/unit/appsync-graphql/AppSyncGraphQLResolver.test.ts
Show resolved
Hide resolved
This is great work. One question I have: both the AppSync events and Bedrock agents event handlers pass the it('tool function has access to the event variable', async () => {
// Prepare
const app = new BedrockAgentFunctionResolver();
app.tool(
async (_params, options) => {
return options?.event;
},
{
name: 'event-accessor',
description: 'Accesses the event object',
}
);
const event = createEvent('event-accessor');
// Act
const result = await app.resolve(event, context);
// Assess
expect(result.response.function).toEqual('event-accessor');
expect(result.response.functionResponse.responseBody.TEXT.body).toEqual(
JSON.stringify(event)
);
}); |
…tion with a single resolver method
…nt factories with a unified onGraphqlEventFactory in tests
…ingle onGraphqlEventFactory
…d for handling GraphQL events
…lers and update tests for event and context access
|
|
||
// Assess | ||
expect(console.debug).not.toHaveBeenCalledWith(); | ||
expect(console.debug).not.toHaveBeenCalledWith(); |
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.
How come we're calling this twice?
* | ||
* For strongly typed validation and parsing at runtime, check out the `@aws-lambda-powertools/parser` package. | ||
*/ | ||
type AppSyncGraphQLEvent = { |
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.
This event type is already defined in the aws-lambda
types package: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/aws-lambda/trigger/appsync-resolver.d.ts
protected readonly envService: EnvironmentVariablesService; | ||
|
||
public constructor(options?: GraphQlRouterOptions) { | ||
this.envService = new EnvironmentVariablesService(); |
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.
Now we're using the utilities in @aws-lambda-powertools/commons/utils/env
we should be able to remove this.
this.resolverRegistry = new RouteHandlerRegistry({ | ||
logger: this.logger, | ||
}); | ||
this.isDev = this.envService.isDevMode(); |
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.
Let's use the standalone utility function not the method from envService: https://github.com/aws-powertools/powertools-lambda-typescript/blob/66fb73eab20f46b6deddc8a7037d370595733125/packages/commons/src/envUtils.ts#L221C7-L221C16
} | ||
|
||
const resolverOptions = handler; | ||
return (_target, _propertyKey, descriptor: PropertyDescriptor) => { |
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 decorator implementation will likely have to change to reflect this fix: #3974.
Summary
This PR will add the ability to register single resolvers for AppSync GraphQL API.
Changes
appsync-graphql
utility insideevent-handler
appsync-events
kwargs
. We must match the function signature accordingly. So this is not possible, I resolved the arguments as an object.onQuery
andonMutation
methods for a single resolver. Although this probably improves the developer experience (DX), I feel like it might not be worth it. It could be merged into a singleonResolve
method instead. This way, we can avoid anotherMap
lookup. Need opinion on this.Example usage of the utility: Followed this
Issue number: #1166
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.