fix(server): Compression Plugin (fetch) response invalid status#929
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a test for gzip-compressing error responses and adjusts CompressionPlugin’s Response construction to explicitly set status, statusText, and headers without spreading the original Response, preserving compression logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant CP as CompressionPlugin
participant H as RPCHandler
C->>CP: HTTP Request (Accept-Encoding: gzip)
CP->>H: Invoke handler
H-->>CP: Throws ORPCError (e.g., UNAUTHORIZED)
rect rgba(230,240,255,0.5)
note over CP: Capture error response
CP->>CP: Serialize error body
CP->>CP: Gzip compress body
CP->>C: HTTP Response (status from error, Content-Encoding: gzip)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
Bot
added
the
size:M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves an issue within the server's Compression Plugin where the HTTP status code of a response, particularly error responses, was not being correctly preserved after compression. The changes ensure that the original status and status text are explicitly carried over to the compressed response, preventing invalid status propagation.
Highlights
- Fix Response Status Propagation: The "CompressionPlugin" now explicitly copies the "status" and "statusText" from the original response when creating a new compressed response. This addresses a bug where spreading the entire response object could lead to incorrect status codes, especially for error responses.
- Add Error Response Compression Test: A new test case has been introduced to "compression-plugin.test.ts" to specifically verify that error responses, such as those from "ORPCError", are correctly compressed and that their original HTTP status code is accurately reflected in the final response.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in the compression plugin where the HTTP status of an error response was not being preserved. The change properly passes the status and statusText when creating the new compressed Response. Additionally, a new test case is introduced to verify the fix and prevent regressions. The changes are solid, and I have one minor suggestion to enhance the new test's robustness.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-event-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/adapters/fetch/compression-plugin.ts (1)
114-118: Add Vary: Accept-Encoding to prevent cache misdeliveries.When the payload is compressed, caches must vary on
Accept-Encoding; otherwise, a compressed response can be served to a client that didn’t ask for it. Please ensureVaryincludesAccept-Encoding(append if present, set if missing).Apply this diff within the same block:
const compressedBody = response.body.pipeThrough(new CompressionStream(encoding)) const compressedHeaders = new Headers(response.headers) compressedHeaders.delete('content-length') // CompressionStream will change the content length compressedHeaders.set('content-encoding', encoding) + // Ensure proper caching semantics for intermediate caches/proxies + const vary = compressedHeaders.get('vary') + if (vary) { + const tokens = vary.split(',').map(v => v.trim().toLowerCase()) + if (!tokens.includes('accept-encoding')) { + compressedHeaders.set('vary', `${vary}, Accept-Encoding`) + } + } else { + compressedHeaders.set('vary', 'Accept-Encoding') + }
🧹 Nitpick comments (3)
packages/server/src/adapters/fetch/compression-plugin.ts (2)
99-106: Honor q-values in Accept-Encoding; avoid choosing encodings with q=0.The current parsing ignores quality factors and may select
gzipeven when a client explicitly setsgzip;q=0. Consider parsing q-values and only allowing encodings withq>0(also respecting*andidentitysemantics).Apply this diff to the selection logic:
- const acceptEncoding = options.request.headers - .get('accept-encoding') - ?.split(',') - .map(enc => enc.trim().split(';')[0]!) - - const encoding = this.encodings.find(enc => acceptEncoding?.includes(enc)) + const accepted = parseAcceptEncoding(options.request.headers.get('accept-encoding')) + const encoding = this.encodings.find(enc => accepted.has(enc))And add this helper (place it after the other helpers in this file):
function parseAcceptEncoding( header: string | null, ): Set<(typeof ORDERED_SUPPORTED_ENCODINGS)[number]> { const allowed = new Set<(typeof ORDERED_SUPPORTED_ENCODINGS)[number]>() if (!header) return allowed // no header => do not compress let starQ: number | undefined for (const token of header.split(',')) { const [nameRaw, ...params] = token.trim().split(';') if (!nameRaw) continue const name = nameRaw.toLowerCase() let q = 1 for (const p of params) { const [k, v] = p.split('=').map(s => s.trim()) if (k === 'q' && v) { const parsed = Number(v) if (!Number.isNaN(parsed)) q = parsed } } if (name === '*') { starQ = q continue } if (q > 0 && (name === 'gzip' || name === 'deflate')) { allowed.add(name) } } // If '*' is allowed, mark all supported encodings as allowed if (starQ !== undefined && starQ > 0) { for (const enc of ORDERED_SUPPORTED_ENCODINGS) allowed.add(enc) } return allowed }Follow-up test idea: add a case with
accept-encoding: gzip;q=0, deflate;q=1and expectdeflate.
55-68: Event Stream “never compress” policy: confirm intent vs. content-disposition exception.Docs say Event Iterator responses are never compressed regardless of filter. The current check skips compression only when
content-typestarts withtext/event-streamand there is nocontent-disposition. Ifcontent-dispositionis present, compression may still occur, which contradicts the doc.If the intent is “never compress SSE”, simplify to:
- if (!hasContentDisposition && contentType?.startsWith('text/event-stream')) { + if (contentType?.startsWith('text/event-stream')) { return false }packages/server/src/adapters/fetch/compression-plugin.test.ts (1)
622-647: Great coverage for compressed error responses; consider asserting JSON shape, content-length removal, and Vary.The test validates status propagation and gzip encoding correctly. Two small improvements:
- Parse the decompressed body as JSON and assert the
codefield explicitly (stronger than substring).- Assert
content-lengthis absent on compressed responses.- If you adopt the
Varyheader suggestion, assert it here too.Apply this diff:
- const decompressed = response?.body?.pipeThrough(new DecompressionStream('gzip')) - const text = await new Response(decompressed).text() - expect(text).toContain(error.code) - expect(response?.headers.get('content-encoding')).toBe('gzip') - expect(response?.status)).toBe(error.status) + const decompressed = response?.body?.pipeThrough(new DecompressionStream('gzip')) + const json = await new Response(decompressed!).json() + expect(json.code).toBe(error.code) + expect(response?.headers.get('content-encoding')).toBe('gzip') + expect(response?.headers.get('content-length')).toBeNull() + // If Vary is added per plugin suggestion: + // expect(response?.headers.get('vary')?.toLowerCase()).toContain('accept-encoding') + expect(response?.status).toBe(error.status)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/server/src/adapters/fetch/compression-plugin.test.ts(2 hunks)packages/server/src/adapters/fetch/compression-plugin.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server/src/adapters/fetch/compression-plugin.test.ts (2)
packages/server/src/builder.ts (1)
os(336-352)packages/server/src/adapters/fetch/compression-plugin.ts (1)
CompressionPlugin(46-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: publish-commit
🔇 Additional comments (2)
packages/server/src/adapters/fetch/compression-plugin.ts (1)
121-125: Fix: preserve status and statusText on compressed responses (good change).Explicitly propagating
statusandstatusTextwhen constructing the newResponsefixes the invalid-status bug for error responses (e.g., ORPCError 4xx/5xx). Looks correct and aligns with the new test.packages/server/src/adapters/fetch/compression-plugin.test.ts (1)
1-1: Importing ORPCError is fine.Using
ORPCErrorfrom@orpc/clientin this server-side test is acceptable and keeps the test close to real usage.
1 participant
Summary by CodeRabbit