Copilot commented on code in PR #43188:
URL: https://github.com/apache/superset/pull/43188#discussion_r3797548431
##########
superset/utils/file.py:
##########
@@ -32,5 +37,13 @@ def sanitize_title(title: str) -> str:
def get_filename(model_name: str, model_id: int, skip_id: bool = False) -> str:
model_name = sanitize_title(model_name)
slug = secure_filename(model_name)
- filename = slug if skip_id else f"{slug}_{model_id}"
- return filename if slug else str(model_id)
+ suffix = "" if skip_id else f"_{model_id}"
+ # The name goes into a ZIP entry that already carries an
+ # `<asset>_export_<timestamp>/<type>/` prefix and a `.yaml` suffix, and the
+ # user's own extraction directory sits in front of all of it. A chart
titled
+ # with a couple of hundred characters therefore produced an entry Windows
+ # refuses to extract, even though the archive itself was written fine. Trim
+ # the slug rather than the id: the id is what keeps two similarly titled
+ # assets from colliding inside one archive.
+ slug = slug[: max(MAX_FILENAME_LENGTH - len(suffix), 0)].rstrip("._-")
+ return f"{slug}{suffix}" if slug else str(model_id)
Review Comment:
`rstrip("._-")` is applied unconditionally after slicing, which means even
*non-truncated* slugs will have any trailing `.`, `_`, or `-` removed. This can
change previously-stable export/screenshot filenames and can increase collision
risk at call sites that use `skip_id=True` (since the id is omitted there).
Consider only stripping trailing separators when truncation actually occurs, so
short names are left untouched.
--
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]
