codeant-ai-for-open-source[bot] commented on code in PR #41759:
URL: https://github.com/apache/superset/pull/41759#discussion_r3537186654
##########
superset/middleware/legacy_prefix_redirect.py:
##########
@@ -211,6 +227,22 @@ def __call__(
return self.wsgi_app(environ, start_response)
canonical_after_strip = candidate[len(_LEGACY_PREFIX) :] or "/"
+
+ # Special case: /superset/sql/<database_id>/ has no 1:1 path mapping —
+ # Database.sql_url migrated to the query-string shape
/sqllab/?dbid=<id>.
+ # Redirect it explicitly (numeric id only) so legacy SQL Lab deep links
+ # survive one release cycle instead of hard-404ing.
+ if sql_match := _LEGACY_SQL_RE.match(canonical_after_strip):
+ if method != "GET":
+ # The old /superset/sql/<id>/ route was GET-only; a 308 would
+ # have the client retry-POST against /sqllab/ → 405. Emit 410.
+ return _response_with_location(410, None)(environ,
start_response)
+ location =
f"{self.app_root_prefix}/sqllab/?dbid={sql_match.group(1)}"
+ if query_string := environ.get("QUERY_STRING", ""):
+ # location already carries ?dbid=<id>; merge with & not ?.
+ location = f"{location}&{query_string}"
+ return _response_with_location(308, location)(environ,
start_response)
Review Comment:
**Suggestion:** The new SQL deep-link redirect branch is unreachable when
`APPLICATION_ROOT` is `/superset` because the middleware short-circuits before
this block when `_enabled` is false. In that deployment shape,
`/superset/sql/<id>/` will still fall through and 404, so the intended
migration path is incomplete. Handle this SQL special case before the global
disable short-circuit (or carve out an exception for this one route) so SQL
legacy links are redirected even in the `/superset` app-root configuration.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ SQL Lab deep links break under APPLICATION_ROOT "/superset".
- ⚠️ Users see 404 instead of redirect for legacy SQL URLs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Superset with an app-root of "/superset" by setting
SUPERSET_APP_ROOT="/superset" or APPLICATION_ROOT="/superset" so
create_app() in
superset/app.py:6-27 computes app_root="/superset" and wraps
LegacyPrefixRedirectMiddleware(app.wsgi_app, app_root) at
superset/app.py:48-51.
2. In LegacyPrefixRedirectMiddleware.__init__ in
superset/middleware/legacy_prefix_redirect.py:179-197, app_root_prefix is
set to
"/superset" and `_enabled` is computed as False because
`self.app_root_prefix ==
_LEGACY_PREFIX` (line 196), a behavior pinned by
test_degenerate_app_root_equals_superset_disables_shim in
tests/unit_tests/middleware/test_legacy_prefix_redirect.py:103-119, where
shim._enabled is
asserted False for app_root="/superset".
3. Issue a GET request to the legacy SQL Lab deep-link path
"/superset/sql/5/".
LegacyPrefixRedirectMiddleware.__call__ in
superset/middleware/legacy_prefix_redirect.py:198-207 executes and
immediately
short-circuits via `if not self._enabled:` at line 206, returning
`self.wsgi_app(environ,
start_response)` before reaching the SQL special-case block at lines 231-245
(including
the redirect logic starting at line 235: `if sql_match :=
_LEGACY_SQL_RE.match(canonical_after_strip):`).
4. The inner Flask app receives PATH_INFO "/superset/sql/5/", but the
historical
`@expose("/sql/<int:database_id>/")` route has been removed (validated by
Grep finding no
`@expose("/sql` usages and by Database.sql_url in
superset/models/core.py:10-16 now
returning "/sqllab/?dbid=<id>"). With no matching route, the request
ultimately produces
an HTTP 404 instead of the intended 308 redirect to
"/superset/sqllab/?dbid=5", leaving
`/superset/sql/<id>/` deep links broken under APPLICATION_ROOT "/superset".
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e4b927cae05a4caa8323390b7e334eab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e4b927cae05a4caa8323390b7e334eab&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/middleware/legacy_prefix_redirect.py
**Line:** 235:244
**Comment:**
*Incomplete Implementation: The new SQL deep-link redirect branch is
unreachable when `APPLICATION_ROOT` is `/superset` because the middleware
short-circuits before this block when `_enabled` is false. In that deployment
shape, `/superset/sql/<id>/` will still fall through and 404, so the intended
migration path is incomplete. Handle this SQL special case before the global
disable short-circuit (or carve out an exception for this one route) so SQL
legacy links are redirected even in the `/superset` app-root configuration.
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%2F41759&comment_hash=49ab58b22584b7fb552e230d136a2163b8b0586a3c47a9cffe5ec0fe7c5fa4e4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41759&comment_hash=49ab58b22584b7fb552e230d136a2163b8b0586a3c47a9cffe5ec0fe7c5fa4e4&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]
