-
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
I have also begun exploring the implementation of the batch resolver. I am trying the powertools-python batch resolver example. Schema: type Post {
post_id: ID!
title: String
author: String
relatedPosts: [Post]
}
type Query {
getPost(post_id: ID!): Post
}
schema {
query: Query
} from __future__ import annotations
from typing import Any, TypedDict
from aws_lambda_powertools import Logger
from aws_lambda_powertools.event_handler import AppSyncResolver
from aws_lambda_powertools.utilities.data_classes import AppSyncResolverEvent
from aws_lambda_powertools.utilities.typing import LambdaContext
app = AppSyncResolver()
logger = Logger()
posts = {
"1": { "post_id": "1", "title": "First book", "author": "Author1", },
"2": { "post_id": "2", "title": "Second book", "author": "Author2", },
"3": { "post_id": "3", "title": "Third book", "author": "Author3", },
"4": { "post_id": "4", "title": "Fourth book", "author": "Author4", },
"5": { "post_id": "5", "title": "Fifth book", "author": "Author5", }
}
posts_related = {
"1": [posts["4"]],
"2": [posts["3"], posts["5"]],
"3": [posts["2"], posts["1"]],
"4": [posts["2"], posts["1"]],
"5": [],
}
def search_batch_posts(posts: list) -> dict[str, Any]:
return {post_id: posts_related.get(post_id) for post_id in posts}
class Post(TypedDict, total=False):
post_id: str
title: str
author: str
@app.resolver(type_name="Query", field_name="getPost")
def get_post(
post_id: str = ""
) -> Post:
return posts.get(post_id, {})
@app.batch_resolver(type_name="Query", field_name="relatedPosts")
def related_posts(event: list[AppSyncResolverEvent]) -> list[Any]:
# Extract all post_ids in order
post_ids: list = [record.source.get("post_id") for record in event]
# Get unique post_ids while preserving order
unique_post_ids = list(dict.fromkeys(post_ids))
# Fetch posts in a single batch operation
fetched_posts = search_batch_posts(unique_post_ids)
# Return results in original order
return [fetched_posts.get(post_id) for post_id in post_ids]
def lambda_handler(event, context: LambdaContext) -> dict:
return app.resolve(event, context) If I try this query query MyQuery {
getPost(post_id: "2") {
relatedPosts {
post_id
author
relatedPosts {
post_id
author
}
}
}
} There is an error: If I change the typeName to Result: {
"data": {
"getPost": {
"relatedPosts": [
{
"post_id": "3",
"author": "Author3",
"relatedPosts": [
{
"post_id": "2",
"author": "Author2"
},
{
"post_id": "1",
"author": "Author1"
}
]
},
{
"post_id": "5",
"author": "Author5",
"relatedPosts": []
}
]
}
}
} The reason is probably because parentTypeName value is Not sure what I am doing wrong here. @dreamorosi @leandrodamascena |
Hi @arnabrahman, thank you so much for the PR. I'm going to need a bit more time to review this, I was out of office yesterday and catching up with things today. I expect to provide a first review tomorrow morning. |
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.
Amazing work as usual @arnabrahman - I haven't tested this on an AppSync API (will try tomorrow) but the implementation looks good. I've only left a couple minor comments.
I'll request also @leandrodamascena's review (tomorrow is ok), since he knows more about this resolver than me.
Good idea also splitting the PR for single vs batch resolvers.
|
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
import { AppSyncGraphQLResolver } from '../../../src/appsync-graphql/AppSyncGraphQLResolver.js'; | ||
import { ResolverNotFoundException } from '../../../src/appsync-graphql/errors.js'; | ||
|
||
describe('Class: AppSyncGraphQLResolver', () => { |
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.
We should add a test where we don't provide a logger, just to ensure that our fallback works:
it('logs only warnings and errors using global console object if no logger supplied', async () => {
const app = new AppSyncGraphQLResolver();
// rest of test
// Assess
expect(console.debug).not.toHaveBeenCalled();
})
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.
Also we should have a test to verify we respect the environment variable AWS_LAMBDA_LOG_LEVEL
being set to DEBUG
. Example here:
Line 98 in d124369
it('emits debug message when AWS_LAMBDA_LOG_LEVEL is set to DEBUG', () => { |
@@ -0,0 +1,144 @@ | |||
import { Router } from 'src/appsync-graphql/Router.js'; |
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.
Similar to previous comment, we should test cresting the Router without supplying a logger
object to the constructor.
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)
);
}); |
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.