korbit-ai[bot] commented on code in PR #33024:
URL: https://github.com/apache/superset/pull/33024#discussion_r2031828122
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -252,28 +252,32 @@ export function querySuccess(query, results) {
return { type: QUERY_SUCCESS, query, results };
}
-export function queryFailed(query, msg, link, errors) {
+export function logFailedQuery(query, errors) {
return function (dispatch) {
const eventData = {
has_err: true,
start_offset: query.startDttm,
ts: new Date().getTime(),
};
- errors?.forEach(({ error_type: errorType, extra }) => {
- const messages = extra?.issue_codes?.map(({ message }) => message) || [
- errorType,
- ];
- messages.forEach(message => {
+ errors?.forEach(({ error_type: errorType, message, extra }) => {
+ const issueCodes = extra?.issue_codes?.map(({ code }) => code) || [-1];
Review Comment:
### Incomplete Error Log Structure Handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The logFailedQuery function doesn't handle the case where an error object
has a message but no error_type or extra properties, which could happen with
some error responses.
###### Why this matters
If an error response doesn't follow the expected structure, some error logs
will be missing critical information, making troubleshooting more difficult.
###### Suggested change ∙ *Feature Preview*
Modify the error processing to handle cases where error properties may be
missing:
```javascript
errors?.forEach(({ error_type: errorType, message, extra }) => {
const issueCodes = extra?.issue_codes?.map(({ code }) => code) || [-1];
const errorTypeToLog = errorType || 'unknown';
const messageToLog = message || errorType || 'Unknown error';
issueCodes.forEach(issueCode => {
dispatch(
logEvent(LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY, {
...eventData,
error_type: errorTypeToLog,
issue_code: issueCode,
error_details: messageToLog,
}),
);
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5983a28c-19c6-4f5d-a43c-68cfae5eb41e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5983a28c-19c6-4f5d-a43c-68cfae5eb41e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5983a28c-19c6-4f5d-a43c-68cfae5eb41e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5983a28c-19c6-4f5d-a43c-68cfae5eb41e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5983a28c-19c6-4f5d-a43c-68cfae5eb41e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f255b918-690e-4276-89c0-2961c8a1744e -->
[](f255b918-690e-4276-89c0-2961c8a1744e)
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -97,6 +112,17 @@
{},
) ?? {};
dispatch(refreshQueries(queries));
+ jsonPayload.result.forEach(query => {
+ const { id, dbId, state } = query;
+ if (
+ asyncFetchDbs.current.has(dbId) &&
+ !failedQueries.current.has(id) &&
+ state === QueryState.Failed
+ ) {
+ dispatch(logFailedQuery(query, query.extra?.errors));
+ failedQueries.current.set(id, true);
+ }
+ });
Review Comment:
### Inefficient Redux Dispatch Pattern <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple Redux dispatches within a loop can trigger multiple re-renders.
###### Why this matters
Each dispatch in the loop can cause a separate render cycle, potentially
causing performance issues with large result sets.
###### Suggested change ∙ *Feature Preview*
Batch the failed queries and dispatch once:
```typescript
const failedQueriesToLog = jsonPayload.result.filter(query =>
asyncFetchDbs.current.has(query.dbId) &&
!failedQueries.current.has(query.id) &&
query.state === QueryState.Failed
);
failedQueriesToLog.forEach(query => {
failedQueries.current.set(query.id, true);
});
if (failedQueriesToLog.length) {
dispatch(logFailedQueries(failedQueriesToLog));
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bcaded6-0d0e-4f83-ab27-3724c97649ae/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bcaded6-0d0e-4f83-ab27-3724c97649ae?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bcaded6-0d0e-4f83-ab27-3724c97649ae?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bcaded6-0d0e-4f83-ab27-3724c97649ae?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bcaded6-0d0e-4f83-ab27-3724c97649ae)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7adfb313-d53e-4a11-a644-eda6cdd340b0 -->
[](7adfb313-d53e-4a11-a644-eda6cdd340b0)
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -67,6 +71,17 @@ function QueryAutoRefresh({
// pendingRequest check ensures we only have one active http call to check
for query statuses
const pendingRequestRef = useRef(false);
const cleanInactiveRequestRef = useRef(false);
+ const failedQueries = useRef(lruCache(1000));
+ const databases = useSelector<SqlLabRootState>(
+ ({ sqlLab }) => sqlLab.databases,
+ ) as Record<string, DatabaseObject>;
Review Comment:
### Unclear Redux selector typing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Type casting with 'as' makes it unclear what the exact type structure is
expected from the Redux store.
###### Why this matters
Using type assertions instead of proper generic type parameters can hide
type mismatches and make the code harder to maintain.
###### Suggested change ∙ *Feature Preview*
Use proper generic type parameters with useSelector:
```typescript
const databases = useSelector<SqlLabRootState, Record<string,
DatabaseObject>>(
({ sqlLab }) => sqlLab.databases
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9752c1-9d79-430c-bd41-ea86679e1264/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9752c1-9d79-430c-bd41-ea86679e1264?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9752c1-9d79-430c-bd41-ea86679e1264?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9752c1-9d79-430c-bd41-ea86679e1264?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9752c1-9d79-430c-bd41-ea86679e1264)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:13ec8f85-9289-489b-98da-458aabcfa3a8 -->
[](13ec8f85-9289-489b-98da-458aabcfa3a8)
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -252,28 +252,32 @@
return { type: QUERY_SUCCESS, query, results };
}
-export function queryFailed(query, msg, link, errors) {
+export function logFailedQuery(query, errors) {
return function (dispatch) {
const eventData = {
has_err: true,
start_offset: query.startDttm,
ts: new Date().getTime(),
};
- errors?.forEach(({ error_type: errorType, extra }) => {
- const messages = extra?.issue_codes?.map(({ message }) => message) || [
- errorType,
- ];
- messages.forEach(message => {
+ errors?.forEach(({ error_type: errorType, message, extra }) => {
+ const issueCodes = extra?.issue_codes?.map(({ code }) => code) || [-1];
+ issueCodes.forEach(issueCode => {
dispatch(
logEvent(LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY, {
...eventData,
error_type: errorType,
+ issue_code: issueCode,
error_details: message,
}),
);
});
Review Comment:
### Fragmented Error Logging <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The logFailedQuery function logs each error issue code separately, creating
multiple log entries for a single query failure. This can lead to log
fragmentation and make error correlation more difficult.
###### Why this matters
When analyzing logs for failed queries, having multiple fragmented log
entries makes it harder to understand the complete failure context and can
complicate log aggregation and analysis.
###### Suggested change ∙ *Feature Preview*
```javascript
const errorLogs = errors?.map(({ error_type: errorType, message, extra }) =>
({
...eventData,
error_type: errorType,
issue_codes: extra?.issue_codes?.map(({ code }) => code) || [-1],
error_details: message,
}));
dispatch(logEvent(LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY, errorLogs));
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/351fd4c1-8cca-4494-ae92-9679b3017948/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/351fd4c1-8cca-4494-ae92-9679b3017948?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/351fd4c1-8cca-4494-ae92-9679b3017948?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/351fd4c1-8cca-4494-ae92-9679b3017948?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/351fd4c1-8cca-4494-ae92-9679b3017948)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:86885c0b-b438-4d6c-93a7-b9d863c88fb0 -->
[](86885c0b-b438-4d6c-93a7-b9d863c88fb0)
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -252,28 +252,32 @@
return { type: QUERY_SUCCESS, query, results };
}
-export function queryFailed(query, msg, link, errors) {
+export function logFailedQuery(query, errors) {
return function (dispatch) {
const eventData = {
has_err: true,
start_offset: query.startDttm,
ts: new Date().getTime(),
};
- errors?.forEach(({ error_type: errorType, extra }) => {
- const messages = extra?.issue_codes?.map(({ message }) => message) || [
- errorType,
- ];
- messages.forEach(message => {
+ errors?.forEach(({ error_type: errorType, message, extra }) => {
+ const issueCodes = extra?.issue_codes?.map(({ code }) => code) || [-1];
+ issueCodes.forEach(issueCode => {
dispatch(
logEvent(LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY, {
...eventData,
error_type: errorType,
+ issue_code: issueCode,
error_details: message,
}),
);
Review Comment:
### Unsanitized Error Details in Logs <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Error messages and stack traces from failed queries are being logged without
sanitization, potentially exposing sensitive information about database
structure or query content.
###### Why this matters
Raw error details in logs could be exploited by attackers to gain insights
about the database schema, query patterns, or system architecture to plan more
targeted attacks.
###### Suggested change ∙ *Feature Preview*
Filter or sanitize error messages before logging:
```javascript
const sanitizeErrorMsg = (msg) => {
// Remove sensitive details like table names, column names etc
return msg.replace(/(?:FROM|JOIN|WHERE)\s+[\w\.]+/gi, '[FILTERED]');
};
// In the code:
error_details: sanitizeErrorMsg(message)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bad42-f89e-46ae-b955-3e8fa3da3c6d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bad42-f89e-46ae-b955-3e8fa3da3c6d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bad42-f89e-46ae-b955-3e8fa3da3c6d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bad42-f89e-46ae-b955-3e8fa3da3c6d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bad42-f89e-46ae-b955-3e8fa3da3c6d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:23cf123b-6a54-4b19-860d-0b9969f4498f -->
[](23cf123b-6a54-4b19-860d-0b9969f4498f)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
