codeant-ai-for-open-source[bot] commented on code in PR #37523:
URL: https://github.com/apache/superset/pull/37523#discussion_r2741098075
##########
superset/app.py:
##########
@@ -69,21 +69,23 @@ def create_app(
if not app.config["STATIC_ASSETS_PREFIX"]:
app.config["STATIC_ASSETS_PREFIX"] = app_root
# Prefix APP_ICON path with subdirectory root for subdirectory
deployments
- if (
- app.config.get("APP_ICON", "").startswith("/static/")
- and app_root != "/"
- ):
- app.config["APP_ICON"] = f"{app_root}{app.config['APP_ICON']}"
- # Also update theme tokens for subdirectory deployments
- for theme_key in ("THEME_DEFAULT", "THEME_DARK"):
- theme = app.config[theme_key]
- token = theme.get("token", {})
- # Update brandLogoUrl if it points to /static/
- if token.get("brandLogoUrl", "").startswith("/static/"):
- token["brandLogoUrl"] =
f"{app_root}{token['brandLogoUrl']}"
- # Update brandLogoHref if it's the default "/"
- if token.get("brandLogoHref") == "/":
- token["brandLogoHref"] = app_root
+ app_icon = app.config.get("APP_ICON", "")
+ if app_icon.startswith("/static/"):
+ app.config["APP_ICON"] = f"{app_root}{app_icon}"
Review Comment:
**Suggestion:** Concatenating `app_root` and asset paths can produce
duplicate slashes when `app_root` ends with a trailing slash (e.g.,
"/analytics/" + "/static/..."). Normalize `app_root` (rstrip the trailing
slash) before joining to avoid URLs with "//" which can break static asset
resolution in some environments. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Static asset URLs may contain double slashes causing 404s.
- ⚠️ Subdirectory deployment static asset handling affected.
```
</details>
```suggestion
app.config["APP_ICON"] = f"{app_root.rstrip('/')}{app_icon}"
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Set the application root to a value that includes a trailing slash, e.g.
export SUPERSET_APP_ROOT="/analytics/" (value read at
`superset/app.py:62-64` into `app_root`).
2. Ensure a config provides `APP_ICON` as a leading-/static path, e.g.
`APP_ICON = "/static/images/icon.png"`, loaded by
`app.config.from_object(...)`
at `superset/app.py:55-58`.
3. Start the app and observe `create_app()` executing the join at
`superset/app.py:72-74`. The composed value becomes
"/analytics//static/..."
when `app_root` ends with `/`.
4. The double-slash URL is emitted to front-end config and can cause asset
resolution problems in some environments (CDNs, proxies) when the browser
requests the double-slash path. This reproduces the malformed URL
generation
at `superset/app.py:74`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/app.py
**Line:** 74:74
**Comment:**
*Logic Error: Concatenating `app_root` and asset paths can produce
duplicate slashes when `app_root` ends with a trailing slash (e.g.,
"/analytics/" + "/static/..."). Normalize `app_root` (rstrip the trailing
slash) before joining to avoid URLs with "//" which can break static asset
resolution in some environments.
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.
```
</details>
##########
superset/app.py:
##########
@@ -69,21 +69,23 @@ def create_app(
if not app.config["STATIC_ASSETS_PREFIX"]:
app.config["STATIC_ASSETS_PREFIX"] = app_root
# Prefix APP_ICON path with subdirectory root for subdirectory
deployments
- if (
- app.config.get("APP_ICON", "").startswith("/static/")
- and app_root != "/"
- ):
- app.config["APP_ICON"] = f"{app_root}{app.config['APP_ICON']}"
- # Also update theme tokens for subdirectory deployments
- for theme_key in ("THEME_DEFAULT", "THEME_DARK"):
- theme = app.config[theme_key]
- token = theme.get("token", {})
- # Update brandLogoUrl if it points to /static/
- if token.get("brandLogoUrl", "").startswith("/static/"):
- token["brandLogoUrl"] =
f"{app_root}{token['brandLogoUrl']}"
- # Update brandLogoHref if it's the default "/"
- if token.get("brandLogoHref") == "/":
- token["brandLogoHref"] = app_root
+ app_icon = app.config.get("APP_ICON", "")
+ if app_icon.startswith("/static/"):
+ app.config["APP_ICON"] = f"{app_root}{app_icon}"
+
+ # Also update theme tokens for subdirectory deployments
+ for theme_key in ("THEME_DEFAULT", "THEME_DARK"):
+ theme = app.config[theme_key]
+ token = theme.get("token", {})
+
+ # Update URLs if they point to /static/
+ for url_key in ("brandSpinnerUrl", "brandLogoUrl"):
+ url = token.get(url_key, "")
+ if url.startswith("/static/"):
+ token[url_key] = f"{app_root}{url}"
+ # Update brandLogoHref if it's the default "/"
+ if token.get("brandLogoHref") == "/":
+ token["brandLogoHref"] = app_root
Review Comment:
**Suggestion:** Using `theme.get("token", {})` returns a newly created dict
when the `token` key is absent, but that temporary dict is never written back
to `theme`, so updates to URLs won't be persisted. Ensure a created default
`token` dict is assigned back to `theme`; also guard `.startswith` calls by
ensuring `url` is a string and normalize `app_root` when composing new URLs.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Theme asset URL updates silently not persisted in config.
- ⚠️ Spinner/logo asset resolution in subdirectory deployments.
```
</details>
```suggestion
theme = app.config.get(theme_key)
if not isinstance(theme, dict):
continue
token = theme.get("token")
if not isinstance(token, dict):
token = {}
theme["token"] = token
# Update URLs if they point to /static/
for url_key in ("brandSpinnerUrl", "brandLogoUrl"):
url = token.get(url_key) or ""
if isinstance(url, str) and url.startswith("/static/"):
token[url_key] = f"{app_root.rstrip('/')}{url}"
# Update brandLogoHref if it's the default "/"
if token.get("brandLogoHref") == "/":
token["brandLogoHref"] = app_root.rstrip('/')
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a config module that defines `THEME_DEFAULT` (or `THEME_DARK`) but
omits
the `token` mapping entirely, then set `SUPERSET_CONFIG` to that module.
2. Start Superset which calls `create_app()` in `superset/app.py` and enters
the
theme-update loop at `superset/app.py:76-88`. The code runs
`token = theme.get("token", {})` resulting in a transient dict when
`token`
is absent.
3. The code updates keys on the transient `token` (e.g. `brandSpinnerUrl`) at
`superset/app.py:82-85`, but since that dict was not assigned back into
`theme`,
the `theme` stored in `app.config` remains unchanged.
4. As a result, subdirectory prefixing for theme asset URLs is not persisted
and
front-end continues to reference unprefixed `/static/...` paths
(spinner/logo
remains broken). This reproduces the loss of updates exactly at the theme
loop
in `superset/app.py:76-88`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/app.py
**Line:** 78:88
**Comment:**
*Logic Error: Using `theme.get("token", {})` returns a newly created
dict when the `token` key is absent, but that temporary dict is never written
back to `theme`, so updates to URLs won't be persisted. Ensure a created
default `token` dict is assigned back to `theme`; also guard `.startswith`
calls by ensuring `url` is a string and normalize `app_root` when composing new
URLs.
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.
```
</details>
--
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]
