codeant-ai-for-open-source[bot] commented on code in PR #40919:
URL: https://github.com/apache/superset/pull/40919#discussion_r3390461980
##########
superset/constants.py:
##########
@@ -15,17 +15,20 @@
# specific language governing permissions and limitations
# under the License.
-# ATTENTION: If you change any constants, make sure to also change
utils/common.js
+# ATTENTION: If you change any constants, make sure to also change
+# plugins/plugin-chart-echarts/src/constants.ts
# string to use when None values *need* to be converted to/from strings
from enum import Enum
+from flask_babel import gettext as __
+
from superset.utils.backports import StrEnum
DEFAULT_USER_AGENT = "Apache Superset"
-NULL_STRING = "<NULL>"
-EMPTY_STRING = "<empty string>"
+NULL_STRING = __("<NULL>")
+EMPTY_STRING = __("<empty string>")
Review Comment:
**Suggestion:** `gettext` is being executed at module import time, so these
translated constants are resolved once (typically in the default locale) and
then reused for all requests. That breaks per-user localization and can also
desynchronize frontend/backend sentinel values across locales. Use lazy
translation (or translate at usage time) so the value is resolved in the active
request locale. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ⚠️ NULL and empty tokens ignore active request locale.
- ⚠️ Frontend and backend NULL sentinels diverge across locales.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset/constants.py` and note at lines 24-31 that `gettext` is
imported as `__`
and immediately invoked to define `NULL_STRING = __("<NULL>")` and
`EMPTY_STRING =
__("<empty string>")`, so these translations are resolved once at module
import time.
2. In a Flask context, demonstrate locale sensitivity by comparing:
- `from flask_babel import gettext as __; __("<NULL>")` executed inside a
`force_locale("fr")` block (returns French translation), versus
- `from superset.constants import NULL_STRING; NULL_STRING` inside the
same block
(still returns the value computed when `superset.constants` was first
imported,
typically English), proving `NULL_STRING` is not per-request localized.
3. Open `superset/utils/pandas_postprocessing/pivot.py` and observe
`pivot()` at lines
10-32, where the `column_fill_value` default is `NULL_STRING` (line 17-31
comment). When a
chart query uses the pivot post-processing operation (wired via
`superset/utils/pandas_postprocessing/__init__.py:17-31` and the generic
post-processing
pipeline), any missing column labels are filled with this frozen constant,
so users on
non-default locales see the startup-locale token instead of a value in their
active
locale.
4. Open `superset/models/helpers.py` around lines 2520-2533 and note
`handle_single_value`
compares incoming filter values against `NULL_STRING` and `EMPTY_STRING`
(lines 2520-2523)
to turn those sentinel strings back into `None` or `""`. On the frontend, the
corresponding constants are defined in
`superset-frontend/plugins/plugin-chart-echarts/src/constants.ts:32-35` as
functions
`NULL_STRING()`/`EMPTY_STRING()` that call `t('<NULL>')`/`t('<empty
string>')` per current
UI locale. Because the backend constants are frozen at import time, for
users whose locale
differs from the server's startup locale the frontend's localized sentinel
strings may no
longer equal the backend's `NULL_STRING`/`EMPTY_STRING`, causing null/empty
filters not to
be recognized and desynchronizing frontend and backend sentinel handling
across locales.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=120639b7eb6145418e21396d2a437c36&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=120639b7eb6145418e21396d2a437c36&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/constants.py
**Line:** 24:31
**Comment:**
*Logic Error: `gettext` is being executed at module import time, so
these translated constants are resolved once (typically in the default locale)
and then reused for all requests. That breaks per-user localization and can
also desynchronize frontend/backend sentinel values across locales. Use lazy
translation (or translate at usage time) so the value is resolved in the active
request locale.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40919&comment_hash=feab80c4a2ee6127cd8a89b8a30faaa0346d9513b907b31ec198e141044cec91&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40919&comment_hash=feab80c4a2ee6127cd8a89b8a30faaa0346d9513b907b31ec198e141044cec91&reaction=dislike'>👎</a>
##########
superset-frontend/src/utils/common.ts:
##########
@@ -84,9 +86,9 @@ export function optionLabel(opt: OptionValue): string {
export function optionValue(
opt: OptionValue,
-): OptionValue | typeof NULL_STRING {
+): OptionValue | ReturnType<typeof NULL_STRING> {
if (opt === null) {
- return NULL_STRING;
+ return NULL_STRING();
}
return opt;
Review Comment:
**Suggestion:** Converting `null` into the localized display token for the
option `value` collapses two distinct inputs (`null` and a real string equal to
that token) into the same value, which causes ambiguous selections and
incorrect round-tripping. Keep the underlying option value as `null` and only
localize the label. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Option helpers collapse NULL and sentinel string values.
- ⚠️ Filters may treat real sentinel text as SQL NULL.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/utils/common.ts` and note `optionValue` at
lines 87-93 and
`optionFromValue` at lines 96-98: `optionValue` returns `NULL_STRING()` when
`opt ===
null`, and `optionFromValue` uses this as the `value` field of `OptionItem`.
2. In a TypeScript runtime (or unit test), import `optionFromValue` and
`NULL_STRING` from
`superset-frontend/src/utils/common.ts` (re-exported at lines 27-35) and
evaluate:
```ts
const a = optionFromValue(null); // uses optionValue/optionLabel
const b = optionFromValue(NULL_STRING()); // pass a real string equal to
the null token
```
3. Observe that `a.value === b.value` because for `null` input `optionValue`
returns
`NULL_STRING()` (line 90-91), while for `NULL_STRING()` input `optionValue`
returns the
same string unchanged (line 93), even though the original inputs (`null` vs
actual string)
are different.
4. Any consumer using `OptionItem.value` (type `OptionValue |
ReturnType<typeof
NULL_STRING>` at line 45) for identity, selection, or to build backend
filter payloads can
no longer distinguish database rows whose actual value equals the localized
null token
from rows that are truly `NULL`, leading to ambiguous selections and
incorrect
round-tripping once this helper is used in production code.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=efed6b1328d448b0bfef093d2a8cc056&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=efed6b1328d448b0bfef093d2a8cc056&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/utils/common.ts
**Line:** 87:93
**Comment:**
*Logic Error: Converting `null` into the localized display token for
the option `value` collapses two distinct inputs (`null` and a real string
equal to that token) into the same value, which causes ambiguous selections and
incorrect round-tripping. Keep the underlying option value as `null` and only
localize the label.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40919&comment_hash=be1444914a7d9d1dc54237a51781d81878c777f415757d9e3c157128591c380e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40919&comment_hash=be1444914a7d9d1dc54237a51781d81878c777f415757d9e3c157128591c380e&reaction=dislike'>👎</a>
--
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]
