korbit-ai[bot] commented on code in PR #32701:
URL: https://github.com/apache/superset/pull/32701#discussion_r1999191908
##########
superset-frontend/src/explore/exploreUtils/index.js:
##########
@@ -300,6 +301,10 @@ export const getSimpleSQLExpression = (subject, operator,
comparator) => {
[...MULTI_OPERATORS]
.map(op => OPERATOR_ENUM_TO_OPERATOR_TYPE[op].operation)
.indexOf(operator) >= 0;
+ const showComparator =
+ DISABLE_INPUT_OPERATORS.map(
+ op => OPERATOR_ENUM_TO_OPERATOR_TYPE[op].operation,
+ ).indexOf(operator) === -1;
Review Comment:
### Missing null check in operator mapping <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The logic fails to handle edge cases where
OPERATOR_ENUM_TO_OPERATOR_TYPE[op] could be undefined, potentially causing
runtime errors.
###### Why this matters
If DISABLE_INPUT_OPERATORS contains an operator that doesn't exist in
OPERATOR_ENUM_TO_OPERATOR_TYPE, accessing .operation on undefined will cause
the application to crash.
###### Suggested change ∙ *Feature Preview*
Add a null check using optional chaining to safely handle undefined
operators:
```javascript
const showComparator =
DISABLE_INPUT_OPERATORS.map(
op => OPERATOR_ENUM_TO_OPERATOR_TYPE[op]?.operation,
).indexOf(operator) === -1;
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/84a9f738-3d10-4d22-91d6-ca21be790ffe?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:73f5303a-5157-4d71-95f7-e2019cb5d4b8 -->
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js:
##########
@@ -36,18 +36,8 @@ export default class AdhocFilter {
this.operator = adhocFilter.operator?.toUpperCase();
this.operatorId = adhocFilter.operatorId;
this.comparator = adhocFilter.comparator;
- if (
- [Operators.IsTrue, Operators.IsFalse].indexOf(adhocFilter.operatorId)
>=
- 0
- ) {
- this.comparator = adhocFilter.operatorId === Operators.IsTrue;
- }
- if (
- [Operators.IsNull, Operators.IsNotNull].indexOf(
- adhocFilter.operatorId,
- ) >= 0
- ) {
- this.comparator = null;
+ if (DISABLE_INPUT_OPERATORS.indexOf(adhocFilter.operatorId) >= 0) {
+ this.comparator = undefined;
}
Review Comment:
### Boolean Filter Value Loss <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Setting comparator to 'undefined' for disabled input operators when the
value should be maintained for boolean operators (IsTrue/IsFalse)
###### Why this matters
This change breaks the boolean filter functionality as it loses the
true/false value needed for IS TRUE/IS FALSE SQL operators, leading to
incorrect query generation
###### Suggested change ∙ *Feature Preview*
```javascript
if (DISABLE_INPUT_OPERATORS.indexOf(adhocFilter.operatorId) >= 0) {
if (adhocFilter.operatorId === Operators.IsTrue) {
this.comparator = true;
} else if (adhocFilter.operatorId === Operators.IsFalse) {
this.comparator = false;
} else {
this.comparator = undefined;
}
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a3e991f-289d-4bf5-b2ae-463f62fd9798?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a08fbc82-a2d5-478f-ac90-22e6e9a4c2be -->
--
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]
