-
Notifications
You must be signed in to change notification settings - Fork 297
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
fix: partial response handling oauthv2 is enabled #4653
Conversation
- chore: propagate interceptor response to jobsdb - chore: add documentation - chore: add test-cases
WalkthroughThe changes primarily focus on refining logging and response handling within the transformer and worker components. Logging levels for specific OAuthV2-related messages are adjusted from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Transformer
participant OAuthInterceptor
participant Backend
Client->>Transformer: Send Request
Transformer->>OAuthInterceptor: Check OAuthV2
OAuthInterceptor-->>Transformer: Interceptor Response
Transformer->>Backend: Forward Request
Backend-->>Transformer: Response
Transformer-->>Client: Forward Response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4653 +/- ##
==========================================
+ Coverage 74.61% 74.64% +0.03%
==========================================
Files 414 414
Lines 48654 48657 +3
==========================================
+ Hits 36304 36322 +18
+ Misses 9966 9955 -11
+ Partials 2384 2380 -4 ☔ View full report in Codecov by Sentry. |
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.
Approved with minor changes.
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.
Actionable comments posted: 3
Outside diff range and nitpick comments (4)
router/transformer/transformer_test.go (4)
Line range hint
745-754
: Refactor repeated test case structures for efficiency.The test cases in
TestProxyRequest
have a lot of repeated setup and teardown code. Consider using table-driven tests or helper functions to reduce redundancy and improve maintainability.Also applies to: 799-799, 814-817, 831-840, 885-885, 895-896, 900-903, 917-926, 971-971, 986-989, 1003-1012, 1057-1057, 1072-1075, 1090-1099, 1144-1144, 1156-1156, 1200-1200, 1213-1213, 1252-1252, 1264-1264, 1302-1302, 1313-1313
Line range hint
745-754
: Ensure proper cleanup and resource management in tests.The tests in
TestRouterTransformationWithOAuthV2
do not consistently ensure that all resources (like HTTP test servers) are properly closed. This can lead to resource leaks during testing.+ defer svr.Close() + defer cfgBeSvr.Close()Also applies to: 799-799, 814-817, 831-840, 885-885, 895-896, 900-903, 917-926, 971-971, 986-989, 1003-1012, 1057-1057, 1072-1075, 1090-1099, 1144-1144, 1156-1156, 1200-1200, 1213-1213, 1252-1252, 1264-1264, 1302-1302, 1313-1313
Line range hint
745-754
: Use constants for repeated literal values.In
TestTransformNoValidationErrors
, the same URL and other configuration values are repeated multiple times. Consider defining these as constants at the beginning of your test file to improve maintainability and reduce the chance of errors in future changes.Also applies to: 799-799, 814-817, 831-840, 885-885, 895-896, 900-903, 917-926, 971-971, 986-989, 1003-1012, 1057-1057, 1072-1075, 1090-1099, 1144-1144, 1156-1156, 1200-1200, 1213-1213, 1252-1252, 1264-1264, 1302-1302, 1313-1313
Line range hint
745-754
: Improve error handling and messaging.The function
TestTransformValidationUnmarshallingError
could improve its error handling by providing more specific error messages and ensuring that all potential error paths are accounted for.+ if err != nil { + t.Fatalf("Expected no error, got %v", err) + }Also applies to: 799-799, 814-817, 831-840, 885-885, 895-896, 900-903, 917-926, 971-971, 986-989, 1003-1012, 1057-1057, 1072-1075, 1090-1099, 1144-1144, 1156-1156, 1200-1200, 1213-1213, 1252-1252, 1264-1264, 1302-1302, 1313-1313
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- router/transformer/transformer.go (3 hunks)
- router/transformer/transformer_test.go (26 hunks)
- router/worker.go (1 hunks)
Files skipped from review due to trivial changes (1)
- router/transformer/transformer.go
Additional comments not posted (1)
router/worker.go (1)
802-804
: Ensure appropriate logging levels in theproxyRequest
method.Verification successful
The logging statements in the
proxyRequest
method use theDebugn
level, which is generally appropriate for debugging purposes. The information logged includes:
oauthV2Enabled
statustraceParent
status during various stages (transform, batch transform, process)jobId
andjobMetadata.JobID
- A message indicating the sending of a request for an OAuth destination
These logging statements do not appear to expose any sensitive information and are appropriate for debugging purposes.
- [router/worker.go:802-804]
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the logging levels in the `proxyRequest` method are appropriate and do not expose sensitive information. # Test: Review the logging statements and their levels in the method. rg --type go $'logger.Debugn' --context 5 router/worker.goLength of output: 2050
Description
When partial responses are to be handled for destinations that are on proxy v1, we are setting 200 status for all of them.
Through this PR, we are aiming to fix this.
Note: additionally info logging for checking oauthv2 flag has been moved to debug logging
Linear Ticket
Resolves INT-2100
Security
Summary by CodeRabbit
Bug Fixes
Info
toDebug
for OAuthV2-related messages.Tests
Refactor
oauthV2Enabled
in the proxy request method to streamline response handling.