feat(appsync): support S3 mapping template locations for resolvers and pipeline functions#13669
Open
nitishagar wants to merge 1 commit into
Open
feat(appsync): support S3 mapping template locations for resolvers and pipeline functions#13669nitishagar wants to merge 1 commit into
nitishagar wants to merge 1 commit into
Conversation
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds ChangesAppSync S3 Mapping Template Locations
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Config as serverless.yml
participant Validator as validateConfig
participant Resolver as Resolver.compile
participant PipelineFunction as PipelineFunction.compile
participant CloudFormation as CFN template
Config->>Validator: resolver/pipelineFunction config with requestS3Location
Validator->>Validator: schema + exclusivity checks
Validator->>Resolver: valid resolver config
Resolver->>Resolver: resolveS3Location()
Resolver->>CloudFormation: set RequestMappingTemplateS3Location / ResponseMappingTemplateS3Location
Validator->>PipelineFunction: valid pipeline function config
PipelineFunction->>PipelineFunction: resolveS3Location()
PipelineFunction->>CloudFormation: set RequestMappingTemplateS3Location / ResponseMappingTemplateS3Location
Related issues: Suggested labels: enhancement, appsync Suggested reviewers: none identified 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/serverless/lib/plugins/aws/appsync/validation.js (1)
898-927: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating duplicate mutual-exclusion logic.
The same "request/requestS3Location", "response/responseS3Location", and "code + S3Location" exclusivity rules are re-implemented independently in
Resolver.jsandPipelineFunction.js(compile()), in addition to here. This is likely intentional (compile() can be invoked directly in tests without a priorvalidateConfig()call, so it needs its own guard), but the error-message text and exact truthiness semantics can drift between the three copies over time (see related comment onResolver.js, lines 35-44, about acodetruthiness/in-operator discrepancy that has already crept in).Extracting a single shared helper (used by both
validateConfigand the twocompile()methods) would remove this class of drift risk going forward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/serverless/lib/plugins/aws/appsync/validation.js` around lines 898 - 927, The mutual-exclusion checks for request/requestS3Location, response/responseS3Location, and code+S3Location are duplicated across validation.js, Resolver.js, and PipelineFunction.js, which risks message and truthiness drift. Extract the shared exclusivity logic into one helper and have both validateConfig and the compile() paths in Resolver and PipelineFunction call it so all three places enforce the same rules and error text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/sf/providers/aws/guide/appsync/pipeline-functions.md`:
- Around line 39-40: Update the `requestS3Location` and `responseS3Location`
docs in the AppSync pipeline functions guide to also mention the
JavaScript-runtime restriction enforced by `PipelineFunction.js`: these S3
template locations are not allowed when `code` is set. Keep the existing
mutual-exclusion note with `request`/`response`, and add the `code`
incompatibility so the `PipelineFunction` configuration options are described
consistently with the validator.
In `@docs/sf/providers/aws/guide/appsync/resolvers.md`:
- Around line 48-49: Update the AppSync resolver config reference to state that
requestS3Location and responseS3Location are VTL-only and cannot be used when
code is set. In the resolver docs section that describes these fields, add the
code incompatibility alongside the existing request/response mutual exclusivity,
matching the behavior enforced by the resolver configuration handling. Use the
existing requestS3Location, responseS3Location, and code terminology so readers
can see the constraint clearly.
In `@packages/serverless/lib/plugins/aws/appsync/resources/Resolver.js`:
- Around line 17-26: The resolveS3Location helper currently returns an object
for AppSync template locations, but RequestMappingTemplateS3Location and
ResponseMappingTemplateS3Location must be plain S3 URI strings. Update
resolveS3Location in Resolver.js to return a single string (bucket/key style)
and stop including Version, then adjust any resolver/function template assembly
that uses requestS3Location or responseS3Location to consume the string value.
Also update the associated tests and snapshots to reflect the string output and
remove expectations around the object shape.
In `@packages/serverless/lib/plugins/aws/appsync/validation.js`:
- Around line 943-963: `validateConfig()` is not using the same merge behavior
as `getAppSyncConfig()`, so split resolver and pipeline-function entries may be
validated incompletely. Update the resolver and pipeline function checks in
`validation.js` to validate the normalized deep-merged config (or reuse the same
`lodash.merge`-based merging used by `getAppSyncConfig()`), and keep the
`notBoth()` checks in the `resolversMap` and `pipelineFunctionsMap` loops so
`request`/`requestS3Location` and `code`/S3 conflicts are caught correctly.
In `@packages/serverless/test/unit/lib/plugins/aws/appsync/resolvers.test.js`:
- Around line 57-121: The resolver tests are asserting the wrong shape for
RequestMappingTemplateS3Location, so update the expectations in the AppSync
resolver test cases to match the CloudFormation schema once resolveS3Location
and Resolver.js are corrected. Focus on the compileResolver coverage in this
spec and replace the object-based assertions around
RequestMappingTemplateS3Location/ResponseMappingTemplateS3Location with the
proper string-based contract, while keeping the existing checks that code cannot
be combined with requestS3Location.
---
Nitpick comments:
In `@packages/serverless/lib/plugins/aws/appsync/validation.js`:
- Around line 898-927: The mutual-exclusion checks for
request/requestS3Location, response/responseS3Location, and code+S3Location are
duplicated across validation.js, Resolver.js, and PipelineFunction.js, which
risks message and truthiness drift. Extract the shared exclusivity logic into
one helper and have both validateConfig and the compile() paths in Resolver and
PipelineFunction call it so all three places enforce the same rules and error
text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 296e47ff-b790-4e21-b2a1-4f88154f23d0
⛔ Files ignored due to path filters (1)
packages/serverless/test/unit/lib/plugins/aws/appsync/__snapshots__/resolvers.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
docs/sf/providers/aws/guide/appsync/pipeline-functions.mddocs/sf/providers/aws/guide/appsync/resolvers.mdpackages/serverless/lib/plugins/aws/appsync/resources/PipelineFunction.jspackages/serverless/lib/plugins/aws/appsync/resources/Resolver.jspackages/serverless/lib/plugins/aws/appsync/validation.jspackages/serverless/test/unit/lib/plugins/aws/appsync/resolvers.test.jspackages/serverless/test/unit/lib/plugins/aws/appsync/validation/pipelineFunctions.test.jspackages/serverless/test/unit/lib/plugins/aws/appsync/validation/resolvers.test.js
Comment on lines
+943
to
+963
| const resolversMap = flattenMerge(data.resolvers) | ||
| for (const [key, resolver] of Object.entries(resolversMap)) { | ||
| if (typeof resolver === 'object' && resolver !== null) { | ||
| notBoth(resolver, `resolver '${key}'`) | ||
| if (Array.isArray(resolver.functions)) { | ||
| for (const f of resolver.functions) { | ||
| if (typeof f === 'object' && f !== null) { | ||
| notBoth(f, `inline pipeline function in resolver '${key}'`) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const pipelineFunctionsMap = flattenMerge(data.pipelineFunctions) | ||
| for (const [name, func] of Object.entries(pipelineFunctionsMap)) { | ||
| if (typeof func === 'object' && func !== null) { | ||
| notBoth(func, `pipeline function '${name}'`) | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any existing config merge logic for resolvers/pipelineFunctions
# arrays-of-maps (deep vs shallow) elsewhere in the plugin.
rg -n -C3 'resolvers|pipelineFunctions' packages/serverless/lib/plugins/aws/appsync/index.js 2>/dev/null
fd -e js . packages/serverless/lib/plugins/aws/appsync --exec rg -l 'merge\(' {} \;Repository: serverless/serverless
Length of output: 675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## file list\n'
git ls-files 'packages/serverless/lib/plugins/aws/appsync/*.js'
printf '\n## outlines\n'
ast-grep outline packages/serverless/lib/plugins/aws/appsync/get-appsync-config.js --view expanded || true
ast-grep outline packages/serverless/lib/plugins/aws/appsync/validation.js --view expanded || true
printf '\n## merge/search hits\n'
rg -n -C 3 'merge\(|Object\.assign|flattenMerge|resolvers|pipelineFunctions' \
packages/serverless/lib/plugins/aws/appsync/get-appsync-config.js \
packages/serverless/lib/plugins/aws/appsync/validation.js \
packages/serverless/lib/plugins/aws/appsync/index.js
printf '\n## focused reads\n'
sed -n '1,220p' packages/serverless/lib/plugins/aws/appsync/get-appsync-config.js
printf '\n---\n'
sed -n '900,990p' packages/serverless/lib/plugins/aws/appsync/validation.jsRepository: serverless/serverless
Length of output: 18534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## validateConfig call sites\n'
rg -n -C 3 'validateConfig\(' packages/serverless/lib/plugins/aws/appsync
printf '\n## config flow around getAppSyncConfig and validateConfig\n'
sed -n '1,220p' packages/serverless/lib/plugins/aws/appsync/index.js
printf '\n---\n'
sed -n '1,220p' packages/serverless/lib/plugins/aws/appsync/get-appsync-config.js
printf '\n## semantic probe: shallow Object.assign vs deep merge on split keys\n'
node - <<'JS'
const merge = require('lodash/merge')
const raw = {
resolvers: [
{ 'Query.user': { request: 'inline.vtl' } },
{ 'Query.user': { requestS3Location: 's3://bucket/request.vtl' } },
],
pipelineFunctions: [
{ 'FnA': { code: 'exports.handler = () => {}' } },
{ 'FnA': { requestS3Location: 's3://bucket/request.vtl' } },
],
}
const shallow = Object.assign({}, ...raw.resolvers)
const deep = merge({}, ...raw.resolvers)
console.log(JSON.stringify({ shallow, deep }, null, 2))
console.log('shallow resolver keys:', Object.keys(shallow['Query.user'] || {}))
console.log('deep resolver keys:', Object.keys(deep['Query.user'] || {}))
JSRepository: serverless/serverless
Length of output: 12738
Use the same deep merge as getAppSyncConfig()
getAppSyncConfig() deep-merges array-of-map inputs with lodash.merge, but validateConfig() flattens them with Object.assign. For split resolver/pipeline-function entries, a later partial entry can overwrite earlier fields and let notBoth() miss request/requestS3Location or code/S3 conflicts. Reuse the same merge behavior here, or validate the normalized config.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/serverless/lib/plugins/aws/appsync/validation.js` around lines 943 -
963, `validateConfig()` is not using the same merge behavior as
`getAppSyncConfig()`, so split resolver and pipeline-function entries may be
validated incompletely. Update the resolver and pipeline function checks in
`validation.js` to validate the normalized deep-merged config (or reuse the same
`lodash.merge`-based merging used by `getAppSyncConfig()`), and keep the
`notBoth()` checks in the `resolversMap` and `pipelineFunctionsMap` loops so
`request`/`requestS3Location` and `code`/S3 conflicts are caught correctly.
…d pipeline functions Add requestS3Location / responseS3Location options to AppSync resolver and pipeline function configuration. When set, CloudFormation emits RequestMappingTemplateS3Location / ResponseMappingTemplateS3Location instead of inlining the template body, avoiding the 1 MB CloudFormation template limit. - Extend isVTLResolver predicate to recognise S3-only resolvers as VTL - Add resolveS3Location helper (lowerCamelCase in, PascalCase CFN out) - Guard against code + *S3Location (VTL-only scope) - Guard against request + requestS3Location (per-side mutual exclusivity) - Add appSyncS3Location AJV definition with required bucket/key, optional version - Add post-AJV notBoth pass in validateConfig for clear error messages - Unit tests for UNIT resolver, S3-only PIPELINE resolver, and pipeline function - Validation tests for schema shape and mutual-exclusivity errors - Docs updated for resolvers.md and pipeline-functions.md Closes serverless#13249
32edab5 to
f1fefaa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
requestS3Location/responseS3Locationoptions to AppSync resolver and pipeline function configuration so VTL templates can be referenced by S3 location instead of inlined into the CloudFormation templateisVTLResolverpredicate to recognise S3-only resolvers as VTL, fixing silent misclassification of S3-only PIPELINE resolvers as JSappSyncS3Location) and a post-AJV mutual-exclusivity pass with precise error messagesMotivation
Large AppSync APIs can exceed CloudFormation's 1 MB processed-template limit when VTL templates are inlined. Referencing templates by S3 location (
RequestMappingTemplateS3Location/ResponseMappingTemplateS3Location) is the AWS-recommended workaround, but the Framework had no way to express this.Changes
validation.js: newappSyncS3Locationdefinition (bucket+keyrequired,versionoptional,additionalProperties:false); refs inresolverConfigandpipelineFunctionConfig; post-AJVnotBothpass for mutual-exclusivity andcode+S3 errorsresources/Resolver.js: extendedisVTLResolverpredicate;resolveS3Locationhelper (exported); compile-time guards; per-side S3/inline branchesresources/PipelineFunction.js: same guards and per-side branches; importsresolveS3LocationfromResolver.jscode+S3resolvers.mdandpipeline-functions.mdupdated with field descriptions and YAML exampleBackward Compatibility
The new fields are optional. All existing configurations compile byte-for-byte identically (the predicate extension is purely additive).
Closes #13249
Summary by CodeRabbit
requestS3Location/responseS3Locationoptions (with{ bucket, key }, plus optional versioning).codewith S3 locations).