Skip to content

fix: merge partial route options with global options - #397

Open
Vinayak1337 wants to merge 1 commit into
fastify:mainfrom
Vinayak1337:fix-route-option-merge
Open

fix: merge partial route options with global options#397
Vinayak1337 wants to merge 1 commit into
fastify:mainfrom
Vinayak1337:fix-route-option-merge

Conversation

@Vinayak1337

@Vinayak1337 Vinayak1337 commented May 11, 2026

Copy link
Copy Markdown

Fixes #340

What changed

  • Merge raw global plugin options with partial route-level compress and decompress options before processing them once.
  • Preserve route option precedence without changing the existing option processors or public API.
  • Add regression coverage for inherited handlers and settings, including the forceRequestEncoding scenario raised during review.

Why

The previous Object.assign({}, globalParams, processParams(routeOptions)) flow allowed defaults and undefined fields from the processed route object to overwrite configured global values. Processing one coherent { ...globalOptions, ...routeOptions } object applies the intended route > global > default precedence and follows the simpler approach demonstrated in #398.

Tests

  • node --test test/routes-compress.test.js test/routes-decompress.test.js
  • npm run lint
  • npm test
  • git diff --check
  • npm run benchmark --if-present (executed; the harness rejected the measurements because the unchanged control varied by 12% on a busy machine)

No documentation or type changes are needed because this fixes existing option precedence without changing the public API.

Checklist

@Vinayak1337
Vinayak1337 force-pushed the fix-route-option-merge branch 2 times, most recently from 0487e7a to 64e54e5 Compare May 11, 2026 17:27
@ranjeetcao

ranjeetcao commented May 17, 2026

Copy link
Copy Markdown

👋 I just closed #398 in favor of this one (it hadn't surfaced in my issue-thread search). One small suggestion if you take another pass: this regression also affects the compress-side onUnsupportedEncoding hook (not just the decompress hooks), and the current test coverage here doesn't exercise that path. A minimal test sketch:

describe('When routes `compress` options are partial — issue #340 :', async () => {
  test('it should fall back to global `onUnsupportedEncoding` when the route only sets `threshold`', async (t) => {
    // ... (paste the test from my branch)
  })
})

Full version on my closed PR if useful: https://github.com/fastify/fastify-compress/pull/398/files

@Vinayak1337

Copy link
Copy Markdown
Author

Thanks for catching this, @ranjeetcao. That path was a real coverage gap.

I added a focused regression test for the route-level compress: { threshold: 1 } case to make sure it falls back to the global onUnsupportedEncoding hook.

I also re-ran:

  • node --test test/routes-compress.test.js
  • npm run lint
  • npm test

Appreciate the pointer.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

Copilot AI left a comment

Copy link
Copy Markdown

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 fixes option resolution precedence so that partial route-level compress/decompress option objects inherit from processed global plugin options first, and only fall back to plugin defaults when neither route nor global provided a value (fixing #340).

Changes:

  • Refactored option processing to support process*Params(routeOpts, globalParams) merging without re-expanding missing route values to plugin defaults.
  • Added regression tests to ensure partial route overrides preserve global handlers/settings (onInvalidRequestPayload, removeContentLengthHeader: false).

Reviewed changes

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

File Description
index.js Refactors compress/decompress parameter processing to properly merge route-level partial options with processed global options.
test/routes-decompress.test.js Adds coverage ensuring a partial route decompress override preserves the global onInvalidRequestPayload handler.
test/routes-compress.test.js Adds coverage ensuring a partial route compress override preserves global removeContentLengthHeader: false (and adds a related partial-options test).

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

Copilot AI left a comment

Copy link
Copy Markdown

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 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

index.js:262

  • baseParams.encodings may already have been narrowed to the global forceRequestEncoding below. Consequently, with global { requestEncodings: ['gzip', 'deflate'], forceRequestEncoding: 'gzip' }, a route override { forceRequestEncoding: 'deflate' } inherits only ['gzip'] and rejects every request, although deflate is allowed by the global list. Preserve the unforced request-encoding list separately and narrow only the effective route list after resolving its force option; this case should also have regression coverage.
    encodings: encodingsOrBase(opts, 'requestEncodings', baseParams),
    forceEncoding: forceEncodingOrBase(opts, baseParams)
  }

  if (params.forceEncoding && params.encodings.includes(params.forceEncoding)) {
    params.encodings = [params.forceEncoding]

@Fdawgs
Fdawgs removed their request for review August 11, 2026 16:39

@Fdawgs Fdawgs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Incredibly hard to review compared to #398. Needs simplifying.

@Vinayak1337
Vinayak1337 force-pushed the fix-route-option-merge branch from 4ba2e6d to 4155222 Compare August 11, 2026 20:37
@Vinayak1337

Vinayak1337 commented Aug 11, 2026

Copy link
Copy Markdown
Author

Thanks @Fdawgs — I simplified the implementation to follow the approach in #398: raw global and route options are merged first, then passed through the existing processor once. The processor refactor and merge commit are gone, and the branch is rebased onto current main.

I also trimmed the tests to the minimum focused coverage: the issue #340 invalid-payload-handler case, compression-handler inheritance, removeContentLengthHeader: false, and the forceRequestEncoding / global requestEncodings scenario raised by Copilot.

Local validation:

  • node --test test/routes-compress.test.js test/routes-decompress.test.js (22 passed)
  • npm run lint
  • npm test (238 unit tests plus tstyche passed)
  • git diff --check

The fresh GitHub Actions run is currently marked action_required before any jobs were created, so it appears to need maintainer approval to run the fork workflow.

Signed-off-by: Vinayak <34209962+Vinayak1337@users.noreply.github.com>
@Vinayak1337
Vinayak1337 force-pushed the fix-route-option-merge branch from 4155222 to 99df941 Compare August 11, 2026 20:59
@Vinayak1337
Vinayak1337 requested a review from Fdawgs August 11, 2026 21: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.

global options are not taked into account when route options are defined partially

5 participants