refactor(conversation-transformer): use file templates for JS resolvers #2897
Conversation
| // This unfortunately needs to be an inline template because an s3 mapping template doesn't allow the CDK | ||
| // to substitute token values, which is necessary for this resolver function due to its reference of | ||
| // `ctx.api.graphqlUrl`. | ||
| return MappingTemplate.inlineTemplateFromString(resolver); |
There was a problem hiding this comment.
In a follow up, I'm planning on moving the ctx.api.graphqlUrl reference to the "Resolver code" portion of the pipeline along with the existing ctx.stash. typeName / fieldName / etc values.
This will allow us to make this an S3 asset and support hotswapping for schema defined values like system prompt, tools, model (LLM), and model inference configuration.
| @@ -0,0 +1,56 @@ | |||
| import { util } from '@aws-appsync/utils'; | |||
|
|
|||
| export function request(ctx) { | |||
There was a problem hiding this comment.
For future PR:
- Please add a doc comment noting the replacement logic
- Rename the file to something other than
.js(e.g.,.js.templateor something), since this isn't actually executable JS as-is
There was a problem hiding this comment.
Good call.
I addressed 2. in b05fbac because it's core to the scope of change.
I'll follow up for 1. in #2897 (comment)
|
|
||
| export function request(ctx) { | ||
| const { args, request, prev } = ctx; | ||
| [[TOOL_DEFINITIONS_LINE]] |
There was a problem hiding this comment.
Here and throughout: Does your template substitution logic guarantee that these will be:
- Always one line?
- Always appropriately escaped for the expected quotes?
How can we ensure that the template always uses the expected quoting scheme so the caller passing the substitution values always escapes appropriately?
There was a problem hiding this comment.
- Always one line?
Not always one line. For example, [[TOOL_DEFINITIONS_LINE]] here can be:
const dataTools = toolDefinitions.tools;
const toolsConfiguration = {
dataTools,
clientTools,
};
- Always appropriately escaped for the expected quotes?
The only safeguard today are the existing snapshots, which is admittedly not great. I plan to add integration tests using AppSync's EvaluateCode API for the resolver functions in the future (reference). That will give us confidence in future changes.
How can we ensure that the template always uses the expected quoting scheme so the caller passing the substitution values always escapes appropriately?
Same answer as 2. Additional context -- the majority of these values are derived from GraphQL inputs (field name, structured directive input types) where values that would cause escaping issues would have failed upstream in GraphQL validation. The exceptions to this rule are the model identifier and system prompt, which are string inputs provided when defining a conversation route. The system prompt is already stringified to prevent issues here. The model identifier can also be stringified prior to substitution -- I'll look at this in a follow up.
|
|
||
| let resolver = fs.readFileSync(path.join(__dirname, 'invoke-lambda-resolver-fn.js'), 'utf8'); | ||
| Object.entries(substitutions).forEach(([key, value]) => { | ||
| const replaced = resolver.replace(new RegExp(`\\[\\[${key}\\]\\]`, 'g'), value); |
There was a problem hiding this comment.
One day Node will give us a String.prototype.replaceAll method so we can forego this nonsense. :(
| @@ -0,0 +1,30 @@ | |||
| export function request(ctx) { | |||
| const { authFilter } = ctx.stash; | |||
|
|
|||
There was a problem hiding this comment.
Here and throughout the PR: let's add a defensive existence assertion for authFilter. I'm comfortable that the existing implementation sets it appropriately, but if a future update accidentally forgets it, I'd prefer to catch that early instead of performing a query without the filter in place. (It's possible that the toDynamoDBFilterExpression util below would fail in that case, but I'd prefer to make the check explicit.)
There was a problem hiding this comment.
Unless you strongly object, I'll include this in a follow up (I have a draft PR queued up that makes some changes here). I know that this is a safe (and correct) change to make, but I'd really prefer to keep this PR non-functional.
There was a problem hiding this comment.
Noted in the aforementioned draft PR as a reminder.
#2910 (comment)
...amplify-graphql-conversation-transformer/src/resolvers/write-message-to-table-resolver-fn.js
Outdated
Show resolved
Hide resolved
...es/amplify-graphql-conversation-transformer/src/resolvers/write-message-to-table-resolver.ts
Show resolved
Hide resolved
...fy-graphql-conversation-transformer/src/transformer-steps/conversation-resolver-generator.ts
Show resolved
Hide resolved
Description of changes
Note
No functional changes
[[KEY]]marker for template substitution.copy-js-resolver-templatesto move JS templates to/libas part ofbuildscript.packages/amplify-graphql-conversation-transformer/src/resolvers/*.jsto.prettierignoreWhy use
[[KEY]]as the marker?This marker format satisfies two key goals:
[[KEY1]]containing"[[KEY2]]".I'm satisfied that
[[KEY]]is the appropriate format here. That said if you have an alternative in mind that satisfies both 1. and 2., and has other benefits, I'm happy to consider it.CDK / CloudFormation Parameters Changed
N/A
Issue #, if available
N/A
Description of how you validated changes
Checklist
yarn testpassesE2E test run linkedTests are changed or addedRelevant documentation is changed or added (and PR referenced)New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policiesAny CDK or CloudFormation parameter changes are called out explicitlyBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.