feat(api-gateway): Introduce Query format Convertion API#10286
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10286 +/- ##
==========================================
- Coverage 83.41% 78.81% -4.61%
==========================================
Files 250 465 +215
Lines 75261 91944 +16683
Branches 0 3357 +3357
==========================================
+ Hits 62780 72462 +9682
- Misses 12481 18992 +6511
- Partials 0 490 +490
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e2d7f30 to
78d3093
Compare
|
Claude finished @ovr's task in 3m 20s —— View job Code Review: feat(api-gateway): Introduce Query format Conversion API
SummaryThis PR introduces a new Issues Found🔴 Bug — Wrong variable in error message
🟠 Bug / Consistency — Non-null assertion instead of safe accessor
🟡 Missing input validation
Security
Performance
Test CoverageThe 4 test cases cover:
Missing test scenarios worth considering:
The Codecov report shows 8.33% patch coverage (11 lines missing). This is because the gateway.ts Design Notes
VerdictThe first two issues (wrong variable in error message, non-null assertion) should be fixed before merge. The input validation suggestion is recommended but lower priority. Overall this is a solid, well-scoped addition. |
| match logical_plan { | ||
| LogicalPlan::Extension(extension) => { | ||
| if let Some(cube_scan) = extension.node.as_any().downcast_ref::<CubeScanNode>() { | ||
| return Ok(Rest4SqlResponse::Ok { | ||
| status: "ok".to_string(), | ||
| query: Box::new(cube_scan.request.clone()), | ||
| }); | ||
| } | ||
|
|
||
| Ok(Rest4SqlResponse::Error { | ||
| status: "error".to_string(), | ||
| error: "Provided sql query can not be converted to rest query.".to_string(), | ||
| }) | ||
| } | ||
| _ => Ok(Rest4SqlResponse::Error { | ||
| status: "error".to_string(), | ||
| error: "Provided sql query can not be converted to rest query.".to_string(), | ||
| }), | ||
| } |
There was a problem hiding this comment.
Nit / Improvement: The two error arms (non-CubeScanNode extension and non-Extension plan) produce identical error responses. Consider collapsing them with a single fallthrough:
match logical_plan {
LogicalPlan::Extension(extension) => {
if let Some(cube_scan) = extension.node.as_any().downcast_ref::<CubeScanNode>() {
return Ok(Rest4SqlResponse::Ok {
status: "ok".to_string(),
query: Box::new(cube_scan.request.clone()),
});
}
Ok(Rest4SqlResponse::Error {
status: "error".to_string(),
error: "Provided sql query can not be converted to rest query.".to_string(),
})
}
_ => Ok(Rest4SqlResponse::Error {
status: "error".to_string(),
error: "Provided sql query can not be converted to rest query.".to_string(),
}),
}Not blocking, but a future refactor could use a single error path for clarity. Also, it might be useful to include diagnostic information about why conversion failed (e.g. the plan type) to help users debug their queries.
| let native_auth_ctx = Arc::new(NativeSQLAuthContext { | ||
| user: Some(String::from("unknown")), | ||
| superuser: false, | ||
| security_context: NonDebugInRelease::from(security_context), | ||
| }); |
There was a problem hiding this comment.
Question: The hardcoded user "unknown" is used here and in sql4sql.rs. Is this intentional? When a REST API request is made with a valid JWT token, the security context is passed through, but the user field is always "unknown". Could this cause issues with any session-level authorization checks or audit logging?
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
This PR introduces new API endpoint
/v1/convertthat allows you to convert queries between different formats. Right now onlysql→restconversion is implemented, but this might be extended in the future.Check List