Skip to content

Node 22 and Azure Functions v4 migration#13

Open
MarcAstr0 wants to merge 10 commits intoboostercloud:mainfrom
Optum:node_22
Open

Node 22 and Azure Functions v4 migration#13
MarcAstr0 wants to merge 10 commits intoboostercloud:mainfrom
Optum:node_22

Conversation

@MarcAstr0
Copy link
Collaborator

## Description

This PR adds Node 22 support to the Webhook Rocket. The changes in this PR are similar to those introduced in #1593 of the framework. This PR also addresses several vulnerabilities found in the dependencies.

This version of the rocket is compatible with 4.x versions of the main framework.

Changes

  • Updated rocket-webhook-azure-infrastructure package to generate v4-compatible Azure Functions
  • Updated rocket-webhook-azure to work with the v4 @azure/functions SDK
  • Added overrides to package.json to address several vulnerabilities across dependencies
  • Updated .nvmrc file
  • Updated dependencies across all packages to support Node 22

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates the Azure Webhook Rocket to Node 22 and Azure Functions v4 compatibility (Booster Framework v4.x), while also updating dependency constraints and applying dependency overrides to address vulnerabilities.

Changes:

  • Bump peer dependency compatibility across packages from Booster Framework v3.x to v4.x.
  • Update Azure infrastructure generation for Functions v4 (including Node 22 runtime in the Function App stack and new V4 function mounting path).
  • Update tooling/dependencies (Lerna 9, cdktf/provider versions, Node typings) and add root overrides for vulnerability mitigation.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/rocket-webhook-types/package.json Update peer dependency minimums to Booster Framework v4.
packages/rocket-webhook-core/package.json Update peer dependency minimums to Booster Framework v4.
packages/rocket-webhook-local-infrastructure/package.json Update peer deps to v4, bump express, update local infra provider + Node typings for Node 22.
packages/rocket-webhook-azure-infrastructure/package.json Update peer deps/dev deps to Booster v4 ecosystem, bump cdktf/provider + Node typings.
packages/rocket-webhook-azure-infrastructure/src/synth/terraform-function-app.ts Set Azure Function App Node runtime to ~22 and Functions extension ~4.
packages/rocket-webhook-azure-infrastructure/src/synth/synth.ts Minor refactor/formatting; continues wiring gateway overrides to the Function App.
packages/rocket-webhook-azure-infrastructure/src/index.ts Switch infra rocket entrypoint to expose mountFunctionsV4.
packages/rocket-webhook-azure-infrastructure/src/functions/functions.ts Implement mountFunctionsV4 returning v4 function app definitions and generated code.
packages/rocket-webhook-azure-infrastructure/src/functions/webhook-function.ts Replace v3 function.json-style definition with generated @azure/functions v4 registration code.
package.json Update peer deps to v4, bump lerna, add overrides to mitigate vulnerabilities.
lerna.json Remove legacy command config (bootstrap/hoist settings).
.nvmrc Update Node LTS to Node 22 (lts/jod).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@alvaroloes alvaroloes left a comment

Choose a reason for hiding this comment

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

Codex: I found one compatibility regression in the generated v4 Azure function names. The new normalization can merge distinct webhook routes into the same registration name, so I’d fix that before merging this migration.


static generateFunctionsCode(endpoint: string): string {
return `
app.http(${JSON.stringify(endpoint.replace(/[^a-zA-Z0-9_]/g, '_'))}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Codex: This normalization is broader than the previous v3 naming logic (/ -> _) and can collapse distinct routes into the same Azure function name. For example, foo-bar and foo/bar now both register as foo_bar, so two different webhooks end up sharing one function identity in functions.js. That is a compatibility regression for existing apps that already use both patterns.

Copy link
Contributor

@alvaroloes alvaroloes left a comment

Choose a reason for hiding this comment

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

Claude: Exhaustive Code Review — Node 22 & Azure Functions v4 Migration


🟢 General Assessment

Solid migration PR. The move from Azure Functions v3 model (function.json + bindings) to the v4 programmatic model (app.http(...)) is well-structured. Dependency bumps and vulnerability overrides look reasonable. A few items worth addressing below.


🔴 Bug Fix (already included — good catch!)

packages/rocket-webhook-azure-infrastructure/src/index.ts

The bind() context was previously Synth for both Functions.mountFunctions and Functions.getFunctionAppName. This PR correctly fixes it to Functions:

// Before (bug):
mountFunctions: Functions.mountFunctions.bind(Synth, params),
// After (fixed):
mountFunctionsV4: Functions.mountFunctionsV4.bind(Functions, params),

This was a latent bug — Synth doesn't have these static methods, so this inside them would have been wrong. Worth mentioning in the PR description as a fix.


🟡 Suggestions & Observations

1. webhook-function.ts — No input validation on endpoint

The function name is sanitized via endpoint.replace(...), and the route uses JSON.stringify(endpoint) (safe against injection), but there's no validation that endpoint is a reasonable value (non-empty string). Consider adding a guard:

if (!endpoint || typeof endpoint !== 'string') {
  throw new Error(`Invalid webhook endpoint: "${endpoint}"`)
}

2. webhook-function.ts — Multipart handling may lose binary data

const buffer = Buffer.from(await request.arrayBuffer())
rawBody = buffer.toString()  // defaults to UTF-8

buffer.toString() with UTF-8 encoding can corrupt binary multipart content (e.g., file uploads). If rawBody is only used for logging, this is acceptable. If downstream consumers rely on rawBody for multipart payloads, consider buffer.toString('latin1') or storing the raw buffer directly.

3. webhook-function.ts — Silent JSON parse error

The catch (e) block silently falls back to rawBody. Consider adding a trace for production troubleshooting:

} catch (e) {
  context.warn('Body is not valid JSON, using raw text')
  body = rawBody
}

4. webhook-function.ts — Response body serialization edge case

const responseBody = ((typeof result.body === 'object' && result.body !== null) && !Buffer.isBuffer(result.body))
  ? JSON.stringify(result.body) : result.body

Doesn't handle exotic objects (Date, Map, Set). If the contract guarantees only plain objects/strings/Buffers, a comment documenting this assumption would help.

5. Missing: Semver version bump

lerna.json version is still 1.0.1. Since peer deps are bumped from >=3.1.2 to >=4.0.0 (breaking change), this should be bumped to 2.0.0 following semver.

6. Missing: Tests for generated handler code

No test changes in this PR. The v4 handler is generated as a string, making it impossible to unit test directly. Consider adding integration tests or extracting handler logic into a testable module.

7. lerna.json — Verify CI replacement

The command.bootstrap block was removed (correct for Lerna 9). Verify that npm ci in CI correctly replaces the old lerna bootstrap --hoist --strict behavior.

8. synth.ts — Formatting mixed with logic

Changes are purely cosmetic (trailing commas, semicolons). Mixing formatting with functional changes makes diffs harder to review. Consider separating in future PRs.


📋 Summary

Category Status
Node 22 compatibility
Azure Functions v4 migration
Vulnerability overrides
bind() context bug fix
Input validation ⚠️ Could be improved
Multipart binary handling ⚠️ Potential data loss
Semver version bump ❌ Missing
Tests ❌ Missing

Overall: Approve with suggestions. Core migration is correct and well-executed. Main gaps are the missing version bump and lack of tests for the generated handler code.

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