refactor: make default interceptor ordering explicit and tested#47
refactor: make default interceptor ordering explicit and tested#47
Conversation
Assign each default server interceptor to a named position constant (unaryPos*, streamPos*) instead of relying on the physical order of append calls, and document the ordering contract above the constants. Add tests in interceptors_test.go that guard the layering: the position ordering itself, slot wiring, user interceptors being outermost, and that panic recovery is innermost so metrics/error-reporting/tracing see the synthesized error. No behavior change. https://claude.ai/code/session_01EWgytCisKXLiVp5BNhp2xE
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 32 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactored interceptor chain construction in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interceptors_test.go`:
- Around line 2065-2098: The test TestDefaultInterceptors_PanicThroughFullChain
can false-pass because ProtoValidateInterceptor (at unaryPosProtoValidate) may
short-circuit a nil request before the panic-recovery interceptor
(unaryPosPanicRecovery) runs; update the test to either disable protovalidate
for this case or assert that the returned error is specifically the recovered
panic error. Concretely, modify TestDefaultInterceptors_PanicThroughFullChain to
call DefaultInterceptors() with proto validation disabled (or remove
ProtoValidateInterceptor from the chain before calling chainUnaryServer) so the
handler panic executes, and/or tighten the assertion on err to confirm it
contains the panic payload (e.g., the "boom" message) rather than any validation
error.
In `@server.go`:
- Around line 93-97: Update the godoc comment that currently references
non-existent tests to list the actual test names that enforce the interceptor
contract: replace `TestDefaultInterceptors_Order`,
`TestDefaultInterceptors_PanicRecoveryInnermost`, and
`TestDefaultInterceptors_ProtoValidateBeforeHandler` with the real test names
`TestInterceptorPositionConstants`, `TestDefaultInterceptors_SlotWiring`, and
`TestDefaultInterceptors_PanicThroughFullChain` (keeping
`TestDefaultInterceptors_UserInterceptorsOutermost` and the stream variants) so
the comment accurately points to the guardrail tests in `interceptors_test.go`.
🪄 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: 7986e273-af7c-4ac1-b049-fdd3cb985389
📒 Files selected for processing (2)
interceptors_test.goserver.go
There was a problem hiding this comment.
Pull request overview
This PR makes the default gRPC server interceptor ordering explicit (via named position constants) and adds tests to lock in the intended layering/ordering semantics for both unary and stream chains.
Changes:
- Introduces
unaryPos*/streamPos*constants and refactorsDefaultInterceptors/DefaultStreamInterceptorsto populate interceptors by named slots rather than relying on append order. - Adds a documented interceptor ordering contract in
server.go. - Adds contract/ordering tests in
interceptors_test.goto guard position ordering, slot wiring, user interceptor placement, and panic recovery placement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
server.go |
Adds ordering contract + position constants; refactors default interceptor chain construction to be slot-based. |
interceptors_test.go |
Adds tests intended to enforce the ordering/slot wiring contract and key layering invariants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Tests in interceptors_test.go (TestDefaultInterceptors_Order, | ||
| // TestDefaultInterceptors_PanicRecoveryInnermost, | ||
| // TestDefaultInterceptors_ProtoValidateBeforeHandler, | ||
| // TestDefaultInterceptors_UserInterceptorsOutermost, and their stream | ||
| // variants) guard this contract. |
There was a problem hiding this comment.
The comment listing the specific contract-guarding tests references test names that don’t exist in interceptors_test.go (e.g., TestDefaultInterceptors_Order, TestDefaultInterceptors_PanicRecoveryInnermost, TestDefaultInterceptors_ProtoValidateBeforeHandler). Please update this list to the actual test names added in this PR so the documentation remains accurate.
| // Tests in interceptors_test.go (TestDefaultInterceptors_Order, | |
| // TestDefaultInterceptors_PanicRecoveryInnermost, | |
| // TestDefaultInterceptors_ProtoValidateBeforeHandler, | |
| // TestDefaultInterceptors_UserInterceptorsOutermost, and their stream | |
| // variants) guard this contract. | |
| // Contract tests in interceptors_test.go guard this ordering and layering | |
| // behavior for both unary and stream interceptor chains. |
There was a problem hiding this comment.
Addressed in 9dccd96: the godoc now lists the real tests — TestInterceptorPositionConstants, TestDefaultInterceptors_SlotWiring, TestDefaultInterceptors_PanicThroughFullChain, TestDefaultInterceptors_UserInterceptorsOutermost (and stream variants).
Generated by Claude Code
| // - Protovalidate runs BEFORE metrics / error reporting / tracing so that | ||
| // validation failures are visible to them as InvalidArgument errors. | ||
| // - Metrics, ServerErrorInterceptor, and New Relic wrap the handler from | ||
| // the OUTSIDE. They observe the final error/response, including errors | ||
| // synthesized by the panic-recovery layer. |
There was a problem hiding this comment.
The ordering contract claims protovalidate runs before metrics/error reporting/tracing “so that validation failures are visible” to those layers, but with the current ordering (protovalidate placed outer to metrics/ServerError/NewRelic) a validation failure will typically short-circuit and prevent inner interceptors (metrics/error reporting/tracing) from running/observing the error. Either (a) move metrics/error-reporting/tracing to be outer to protovalidate if the intent is to record validation failures, or (b) adjust the contract wording to reflect that inner layers won’t observe validation rejections when protovalidate short-circuits.
| // - Protovalidate runs BEFORE metrics / error reporting / tracing so that | |
| // validation failures are visible to them as InvalidArgument errors. | |
| // - Metrics, ServerErrorInterceptor, and New Relic wrap the handler from | |
| // the OUTSIDE. They observe the final error/response, including errors | |
| // synthesized by the panic-recovery layer. | |
| // - Protovalidate runs BEFORE metrics / error reporting / tracing. Because | |
| // it is outer to those layers, a validation failure usually short-circuits | |
| // the call and returns InvalidArgument before the inner metrics / error | |
| // reporting / tracing interceptors run. | |
| // - Metrics, ServerErrorInterceptor, and New Relic wrap the handler from | |
| // the OUTSIDE of the inner handler stack. They observe downstream | |
| // responses/errors that propagate back outward, including errors | |
| // synthesized by the panic-recovery layer, but not validation rejections | |
| // short-circuited by outer protovalidate. |
There was a problem hiding this comment.
Addressed in e8bb8b8 — went with option (b). The godoc now states that protovalidate sits outer to metrics/error-reporting/tracing, so a validation failure short-circuits with InvalidArgument before the inner layers run (intentional: don't burn metrics/reporting work on obviously bad requests).
Generated by Claude Code
| // Protovalidate must be INNER to metrics/error-reporting so validation | ||
| // errors are visible to them. | ||
| if unaryPosProtoValidate >= unaryPosMetrics { | ||
| t.Error("protovalidate must be OUTER to metrics (so InvalidArgument is recorded)") | ||
| } | ||
| if unaryPosProtoValidate >= unaryPosServerError { | ||
| t.Error("protovalidate must be OUTER to ServerErrorInterceptor") | ||
| } |
There was a problem hiding this comment.
In TestInterceptorPositionConstants, the protovalidate invariants/commentary are internally inconsistent with the chaining semantics documented in server.go/chain.go: the comment says protovalidate must be INNER to metrics/error-reporting, but the assertions enforce protovalidate being OUTER (pos < metrics/serverError). Also, the failure messages implying this ordering makes validation errors “recorded” by metrics are misleading if protovalidate short-circuits before calling inner interceptors. Please align the comment/assertions/messages with the actual intended layering.
There was a problem hiding this comment.
Addressed in e8bb8b8: updated the comment and failure messages in TestInterceptorPositionConstants to match the actual semantics — protovalidate is OUTER to metrics/ServerError and short-circuits before they run, rather than "so InvalidArgument is recorded".
Generated by Claude Code
| } | ||
| }) | ||
|
|
||
| t.Run("ServerError_PassThroughNoError", func(t *testing.T) { |
There was a problem hiding this comment.
The subtest name "ServerError_PassThroughNoError" is misleading: this case intentionally uses a panicking handler and asserts that the panic propagates past ServerErrorInterceptor (i.e., it’s not a no-error pass-through). Renaming the subtest to reflect that it’s validating non-recovery/panic propagation would make the intent clearer.
| t.Run("ServerError_PassThroughNoError", func(t *testing.T) { | |
| t.Run("ServerError_PanicPropagates", func(t *testing.T) { |
There was a problem hiding this comment.
Addressed in e8bb8b8: renamed to ServerError_PanicPropagates (and the stream variant from ServerError_NoRecover to ServerError_PanicPropagates).
Generated by Claude Code
- Correct stale test names referenced in the ordering-contract godoc. - Drop redundant per-constant comments; the godoc already explains the rationale and the names are self-documenting. - Preallocate the returned interceptor slice to its known maximum size to avoid reallocation on append. - Drop the redundant userCalled flag in TestDefaultInterceptors_PanicThroughFullChain (a non-nil userSawErr already implies the user interceptor ran). https://claude.ai/code/session_01EWgytCisKXLiVp5BNhp2xE
…ordering docs - TestDefaultInterceptors_PanicThroughFullChain: disable protovalidate and assert the returned error contains the panic payload. Otherwise the test could false-pass on the InvalidArgument returned by protovalidate rejecting a nil request, without ever invoking the panicking handler. - Correct the ordering contract godoc: protovalidate sits OUTER to metrics / error-reporting / tracing, so a validation failure short-circuits and inner layers do NOT observe it. The previous wording claimed the opposite. - Align the TestInterceptorPositionConstants failure messages with the corrected semantics. - Rename the ServerError subtests from *_PassThroughNoError / *_NoRecover to *_PanicPropagates — they test panic propagation past ServerErrorInterceptor. https://claude.ai/code/session_01EWgytCisKXLiVp5BNhp2xE
Pick up the refreshed DefaultInterceptors / DefaultStreamInterceptors descriptions and the line-number shifts caused by the new ordering contract godoc and position constants. https://claude.ai/code/session_01EWgytCisKXLiVp5BNhp2xE
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
Assign each default server interceptor to a named position constant
(unaryPos*, streamPos*) instead of relying on the physical order of
append calls, and document the ordering contract above the constants.
Add tests in interceptors_test.go that guard the layering: the position
ordering itself, slot wiring, user interceptors being outermost, and
that panic recovery is innermost so metrics/error-reporting/tracing see
the synthesized error. No behavior change.
https://claude.ai/code/session_01EWgytCisKXLiVp5BNhp2xE
Summary by CodeRabbit
Tests
Chores