fix: partial response handling oauthv2 is enabled #4653
Conversation
- chore: propagate interceptor response to jobsdb - chore: add documentation - chore: add test-cases
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 @@
## release/1.25.x #4653 +/- ##
==================================================
- Coverage 74.61% 74.55% -0.06%
==================================================
Files 412 412
Lines 48699 48702 +3
==================================================
- Hits 36335 36312 -23
- Misses 9995 10011 +16
- Partials 2369 2379 +10 ☔ View full report in Codecov by Sentry. |
| } else if oauthV2Enabled { | ||
| proxyRequestResponse.RespStatusCodes, proxyRequestResponse.RespBodys = w.prepareResponsesForJobs(&destinationJob, proxyRequestResponse.ProxyRequestStatusCode, proxyRequestResponse.ProxyRequestResponseBody) | ||
| proxyRequestResponse.RespContentType = http.DetectContentType([]byte(proxyRequestResponse.ProxyRequestResponseBody)) |
There was a problem hiding this comment.
Didn't understood why we are removing this?
There was a problem hiding this comment.
What if transportResponse.InterceptorResponse.StatusCode is 0 for a successful response here?
There was a problem hiding this comment.
Didn’t understood why we are removing this?
The response contract for /v1/<DEST_TYPE>/proxy endpoint in transformer is
200 OK
{
“output”: {
“message”: “”,
“response”: [
{“statusCode”: 400, “metadata”: {jobId: 1}},
{“statusCode”: 502, “metadata”: {jobId: 2}},
],
“authErrorCategory”: “”
}
}When there is no problem with LB, we would always get 200 from the endpoint. And based on the logic of prepareResponsesForJobs, we would set proxy response status-code(200) to all the jobs.
Before this change, jobId: 1 would be set with 200 instead of 400 & jobId: 2 would be set with 200 instead of 500
Such logic is unnecessary in this part. And we moved this logic into router/transformer.go when the interceptor receives erreneous status-code(due to OAuth / transformer)
This is the reason we are removing this part of source-code. Let me know if it makes sense
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