luizotavio32 commented on code in PR #43560:
URL: https://github.com/apache/superset/pull/43560#discussion_r3865538303
##########
superset/utils/excel.py:
##########
@@ -21,21 +21,33 @@
from superset.utils.core import GenericDataType
+# Leading characters that turn a cell into a formula in spreadsheet apps.
+FORMULA_PREFIXES = {"=", "+", "-", "@"}
+
+
+def _quote_formula(value: Any) -> Any:
+ """Prefix a string with a quote when it would parse as a formula."""
+ return (
+ f"'{value}"
+ if isinstance(value, str) and len(value) and value[0] in
FORMULA_PREFIXES
+ else value
+ )
+
def quote_formulas(df: pd.DataFrame) -> pd.DataFrame:
"""
Make sure to quote any formulas for security reasons.
"""
- formula_prefixes = {"=", "+", "-", "@"}
-
- for col in df.select_dtypes(include="object").columns:
- df[col] = df[col].apply(
- lambda x: (
- f"'{x}"
- if isinstance(x, str) and len(x) and x[0] in formula_prefixes
- else x
- )
- )
+ # Columns are addressed by position rather than by label: a dataframe can
+ # carry duplicate column labels (the verbose_map rename in
+ # QueryContextProcessor.get_data can collapse two columns onto the same
+ # name), and ``df[label]`` then yields a DataFrame instead of a Series.
+ # ``DataFrame.apply`` would hand whole columns to the mapper rather than
+ # individual cells, silently leaving formulas unquoted.
+ for idx in range(len(df.columns)):
+ series = df.iloc[:, idx]
+ if series.dtype == object:
+ df.isetitem(idx, series.map(_quote_formula))
Review Comment:
Good catch — accepted in e9e2c004c5, with one clarification worth recording
for reviewers.
The claim checks out. Under pandas 3 string semantics (simulated here with
`pd.options.future.infer_string = True` on pandas 2.1.4):
```
--- default (pandas 2.x legacy strings) ---
dtype : object
dtype == object : True
select_dtypes(object) : ['f']
is_string_dtype : True
--- future.infer_string=True (pandas 3.x semantics) ---
dtype : string
dtype == object : False
select_dtypes(object) : [] <-- pre-existing check also misses it
is_string_dtype : True
```
The clarification: this is **not introduced by this PR**. The code being
replaced was `for col in df.select_dtypes(include="object").columns`, which is
equally object-only and equally skips the dedicated string dtype — as the third
line above shows. So it is a latent pandas-3 forward-compatibility issue that
my rewrite carried over faithfully, not a new bypass. It is also inert on the
currently pinned pandas (2.x), where string columns are still `object`.
Fixed anyway, since these are the exact lines this PR touches. I used a
line-wrapped form to stay within ruff's 88-char limit rather than the suggested
single line, which exceeded it:
```python
if pd.api.types.is_object_dtype(series.dtype) or
pd.api.types.is_string_dtype(
series.dtype
):
```
Covered by a new test, `test_quote_formulas_with_dedicated_string_dtype`,
which builds an explicit `dtype="string"` column rather than mutating pandas'
global options, so it is deterministic and order-independent. It fails under
the old object-only check (formula left unquoted) and passes now.
--
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]
