SEPURI-SAI-KRISHNA commented on code in PR #45433:
URL: https://github.com/apache/superset/pull/45433#discussion_r4334752506
##########
tests/unit_tests/utils/csv_tests.py:
##########
@@ -278,6 +278,63 @@ def test_df_to_escaped_csv_non_default_index():
assert rows[2] == ["girl Mary", "safe"]
+def test_df_to_escaped_csv_escapes_index_labels():
+ """
+ Formulas in row labels are escaped too, not just cells and headers.
+
+ Pivot post-processing promotes the row dimension's values into the index,
+ and both export paths write the index out once it is no longer a
+ RangeIndex, so a hostile warehouse string can reach the first column of
+ the file.
+ """
+ df = pd.DataFrame(
+ data={"SUM(num)": [10.0, 20.0]},
+ index=pd.Index(['=cmd|" /C calc"!A0', "safe"], name="dim"),
+ )
+
+ rows = df_to_escaped_csv(df, encoding="utf8",
index=True).strip().split("\n")
+
+ assert rows[1].startswith("\"'=cmd")
+ assert rows[2].startswith("safe,")
+ # The caller's frame is left alone.
+ assert df.index.tolist() == ['=cmd|" /C calc"!A0', "safe"]
+
+
+def test_df_to_escaped_csv_escapes_multiindex_labels_and_axis_names():
+ """
+ Every level of a MultiIndex is escaped, and so are the axis *names*.
+
+ Axis names are a separate attribute from the labels, so ``rename`` alone
+ leaves them untouched while pandas still writes them out as header cells.
+ A pivoted group-by puts a warehouse-controlled column name there.
+ """
+ df = pd.DataFrame(
+ data={"v": [1, 2]},
+ index=pd.MultiIndex.from_tuples(
+ [("a", '=cmd|" /C calc"!A0'), ("b", "safe")],
Review Comment:
You are right. Escaping only the innermost level leaves
`test_df_to_escaped_csv_escapes_multiindex_labels_and_axis_names` green,
because both of its outer labels are safe.
Made the outer label hostile exactly as you suggested, and the first field is
asserted:
```python
index=pd.MultiIndex.from_tuples(
[("=SUM(1+1)", '=cmd|" /C calc"!A0'), ("b", "safe")],
names=["level", "+name"],
),
...
assert rows[1].split(",")[0] == "'=SUM(1+1)"
```
Narrowing the rebuild to the last level now fails it.
I also reverted every other condition in the diff on its own to see which had
a test behind them, which turned up one more with none: the index is rebuilt
with `MultiIndex.from_arrays` rather than `set_levels` because `escape_value`
is not injective, so a warehouse returning both `=a` and `'=a` in one row
dimension has them land on the same label. `set_levels` rejects that with
`ValueError: Level values must be unique: ["'=a", "'=a"] on level 0`, which
would make the export a 500. That is now
`test_df_to_escaped_csv_escapes_colliding_index_labels`.
Three conditions are left with no test, because no input separates them, and
they are in the description as kept for what they say rather than what they
do:
`needs_escaping` testing `escape_value(value) != value` rather than just
`isinstance(value, str)` (rebuilding a safe string level is a no-op, since
`map` keeps object, `string` and `category` dtypes alike), reading the
decision
off `index.levels` rather than always rebuilding (`from_arrays` preserves
every
level's dtype, so the output is byte-identical either way), and passing
`names=index.names` (`get_level_values` carries the level name and `map`
preserves it, so `from_arrays` infers the same names).
--
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]
