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

fix: include custom conversation handlers in lambdaFunctions for downstream processing #361

Merged
merged 6 commits into from
Oct 4, 2024

Conversation

atierian
Copy link
Member

@atierian atierian commented Oct 4, 2024

Related amplify-category-api PR aws-amplify/amplify-category-api#2922

Description of changes

Context
a.conversation({ }) schema definitions optionally allow a handler to be included. This is for some advanced use cases like custom tools within the Lambda function.

Problem
Custom conversation handlers aren't correctly referenced in the Data construct because they're not included in the function map passed from data-schema --> backend-data --> data-construct

Change
The Data construct needs to reference the custom handler Lambda function to create the AppSync data source. defineFunction + a.handler.function do this today by keeping a map of function-name --> DefineFunction, which gets passed to backend-data, then ultimately to the Data construct.

This PR adopts that same methodology for custom conversation handlers.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

changeset-bot bot commented Oct 4, 2024

🦋 Changeset detected

Latest commit: 10f96d3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/data-schema Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

svidgen
svidgen previously approved these changes Oct 4, 2024
Copy link
Member

@svidgen svidgen left a comment

Choose a reason for hiding this comment

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

I would mind seeing an added test or two for this! 😅

stocaaro
stocaaro previously approved these changes Oct 4, 2024
Copy link
Member

@stocaaro stocaaro left a comment

Choose a reason for hiding this comment

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

Anything about this worth putting under test with this change?
Edit: Jon and I had the exact same thought.

@atierian atierian dismissed stale reviews from stocaaro and svidgen via 7be441b October 4, 2024 18:04
@atierian atierian force-pushed the ai.function-map-conversation-handler branch from 2312786 to 7be441b Compare October 4, 2024 18:04
@atierian
Copy link
Member Author

atierian commented Oct 4, 2024

I would mind seeing an added test or two for this! 😅

Anything about this worth putting under test with this change? Edit: Jon and I had the exact same thought.

Good call, thanks. Addressed in 7be441b

@atierian atierian enabled auto-merge (squash) October 4, 2024 18:10
@atierian atierian merged commit 52b79da into main Oct 4, 2024
9 checks passed
@atierian atierian deleted the ai.function-map-conversation-handler branch October 17, 2024 20:34
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.

3 participants