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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
TestProxyRequesthave 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
TestRouterTransformationWithOAuthV2do 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
TestTransformValidationUnmarshallingErrorcould 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 theproxyRequestmethod.Verification successful
The logging statements in the
proxyRequestmethod use theDebugnlevel, which is generally appropriate for debugging purposes. The information logged includes:
oauthV2EnabledstatustraceParentstatus during various stages (transform, batch transform, process)jobIdandjobMetadata.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
InfotoDebugfor OAuthV2-related messages.Tests
Refactor
oauthV2Enabledin the proxy request method to streamline response handling.