-
Notifications
You must be signed in to change notification settings - Fork 2
Add PreRequestHook for unified request preprocessing #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Introduces a service-level hook that is called before each request is processed, including batch sub-requests. This provides a unified mechanism for authentication, context enrichment, logging, and other preprocessing tasks. Key features: - Works uniformly for single requests and batch sub-requests (including changesets) - Hook can return a modified context to pass values to downstream handlers - Hook can return an error to abort the request with HTTP 403 Forbidden - Eliminates the need for manual SetBatchSubRequestHandler configuration Closes #370 Co-authored-by: NLstn <[email protected]>
- Fixed trailing whitespace in test file - Added clarifying comment about hook wrapper capture Co-authored-by: NLstn <[email protected]>
There was a problem hiding this 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 introduces a PreRequestHook feature that provides a unified mechanism for request preprocessing across all request types, including batch sub-requests and changesets. This addresses the issue where batch sub-requests bypass external middleware, requiring users to manually configure SetBatchSubRequestHandler for authentication and context enrichment.
Key changes:
- Added
PreRequestHooktype andSetPreRequestHookmethod for service-level request preprocessing - Integrated hook execution in
http_runtime.gofor all requests and inbatch.gofor changeset sub-requests - Added comprehensive test coverage with 7 test cases covering single requests, batch operations, changesets, and edge cases
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| odata.go | Defines PreRequestHook type and SetPreRequestHook method with comprehensive documentation |
| http_runtime.go | Calls the pre-request hook before processing each request, with proper error handling |
| internal/handlers/batch.go | Integrates hook for changeset sub-requests with context propagation |
| test/pre_request_hook_test.go | Comprehensive test suite with 7 tests covering various scenarios |
| documentation/advanced-features.md | Documents the feature with examples and comparison to SetBatchSubRequestHandler |
| CHANGELOG.md | Documents the new feature in the Added section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
odata.go
Outdated
| // Wrap the hook for the batch handler. The wrapper is created once and captures | ||
| // the hook variable, so subsequent calls to SetPreRequestHook will update the | ||
| // service's preRequestHook but the batch handler will use its own copy. | ||
| // To update the batch handler's hook, call SetPreRequestHook again. | ||
| s.batchHandler.SetPreRequestHook(func(r *http.Request) (context.Context, error) { | ||
| if hook == nil { | ||
| return nil, nil | ||
| } | ||
| return hook(r) |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closure captures the hook parameter value at the time of the call, not the s.preRequestHook field. This means if SetPreRequestHook is called multiple times, the batch handler will keep using the hook from the first call, not subsequent ones.
The comment on lines 846-849 acknowledges this issue but doesn't fix it. To fix this, the closure should reference s.preRequestHook instead of the hook parameter, like this:
s.batchHandler.SetPreRequestHook(func(r *http.Request) (context.Context, error) {
if s.preRequestHook == nil {
return nil, nil
}
return s.preRequestHook(r)
})This way, the batch handler always calls the current hook value.
| // Wrap the hook for the batch handler. The wrapper is created once and captures | |
| // the hook variable, so subsequent calls to SetPreRequestHook will update the | |
| // service's preRequestHook but the batch handler will use its own copy. | |
| // To update the batch handler's hook, call SetPreRequestHook again. | |
| s.batchHandler.SetPreRequestHook(func(r *http.Request) (context.Context, error) { | |
| if hook == nil { | |
| return nil, nil | |
| } | |
| return hook(r) | |
| // Wrap the hook for the batch handler. The wrapper reads s.preRequestHook on each | |
| // call so that subsequent calls to SetPreRequestHook are honored for batch requests. | |
| s.batchHandler.SetPreRequestHook(func(r *http.Request) (context.Context, error) { | |
| if s.preRequestHook == nil { | |
| return nil, nil | |
| } | |
| return s.preRequestHook(r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 080360a. The closure now references s.preRequestHook instead of the hook parameter, so subsequent calls to SetPreRequestHook are honored for batch requests.
| - [Pre-Request Hook](#pre-request-hook) | ||
| - [Change Tracking and Delta Tokens](#change-tracking-and-delta-tokens) | ||
| - [Read Hooks](#read-hooks) | ||
| - [Tenant Filtering Example](#tenant-filtering-example) | ||
| - [Redacting Sensitive Data](#redacting-sensitive-data) |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table of contents structure is incorrect. The items "Read Hooks", "Tenant Filtering Example", and "Redacting Sensitive Data" (lines 13-15) are indented as sub-items of "Change Tracking and Delta Tokens", but they should be sub-items of "Lifecycle Hooks" based on the typical structure of this documentation.
This creates a confusing navigation structure where lifecycle hook examples appear under the wrong parent section.
| - [Pre-Request Hook](#pre-request-hook) | |
| - [Change Tracking and Delta Tokens](#change-tracking-and-delta-tokens) | |
| - [Read Hooks](#read-hooks) | |
| - [Tenant Filtering Example](#tenant-filtering-example) | |
| - [Redacting Sensitive Data](#redacting-sensitive-data) | |
| - [Pre-Request Hook](#pre-request-hook) | |
| - [Read Hooks](#read-hooks) | |
| - [Tenant Filtering Example](#tenant-filtering-example) | |
| - [Redacting Sensitive Data](#redacting-sensitive-data) | |
| - [Change Tracking and Delta Tokens](#change-tracking-and-delta-tokens) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 080360a. The TOC now correctly shows Read Hooks, Tenant Filtering Example, and Redacting Sensitive Data as sub-items under the appropriate section.
| if hookCallCount < 3 { | ||
| t.Errorf("Expected hook to be called at least 3 times, got %d", hookCallCount) | ||
| } | ||
| } |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for calling SetPreRequestHook multiple times. The current tests only call SetPreRequestHook once per test. There should be a test that:
- Calls
SetPreRequestHookwith a first hook - Makes a batch request to verify the first hook is called
- Calls
SetPreRequestHookwith a different hook - Makes another batch request to verify the second hook is now being called
This would help catch the bug in lines 850-855 of odata.go where the closure captures the parameter instead of the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests in 080360a: TestPreRequestHook_MultipleHookUpdates for single requests and TestPreRequestHook_MultipleHookUpdates_BatchChangeset for batch changesets.
|
@copilot remove the SetBatchSubRequestHandler logic without caring about backwards compatibility |
- Fix closure bug: reference s.preRequestHook instead of captured hook parameter - Fix documentation TOC structure - Add tests for calling SetPreRequestHook multiple times - Remove SetBatchSubRequestHandler entirely (breaking change) - Remove batchSubRequestHandler field from Service struct - Remove SetSubRequestHandler from BatchHandler - Update CHANGELOG with removed feature Co-authored-by: NLstn <[email protected]>
Changes Made
Bug Fix
hookparameter instead ofs.preRequestHookfield, so subsequent calls toSetPreRequestHooknow correctly update the behavior for batch requestsBreaking Change
SetBatchSubRequestHandlermethod entirelybatchSubRequestHandlerfield from Service structSetSubRequestHandlermethod from BatchHandlerSetBatchSubRequestHandlerSetBatchSubRequestHandlerTests Added
TestPreRequestHook_MultipleHookUpdates- verifies hook updates work for single requestsTestPreRequestHook_MultipleHookUpdates_BatchChangeset- verifies hook updates work for batch changesetsOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.