codeant-ai-for-open-source[bot] commented on code in PR #40908: URL: https://github.com/apache/superset/pull/40908#discussion_r3382412447
########## superset/openapi/manager.py: ########## @@ -0,0 +1,116 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""APPLICATION_ROOT-aware OpenAPI spec and Swagger UI. + +Serves the OpenAPI spec and Swagger UI in a way that works when Superset is +deployed behind a URL prefix (reverse proxy) via ``APPLICATION_ROOT``. Enabled +by the ``FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT`` config flag. +""" + +from typing import Any + +from apispec import APISpec +from apispec.ext.marshmallow import MarshmallowPlugin +from apispec.ext.marshmallow.common import resolve_schema_cls +from flask import current_app, request +from flask_appbuilder.api import BaseApi, expose, protect, safe +from flask_appbuilder.baseviews import BaseView +from flask_appbuilder.security.decorators import has_access + +from superset.superset_typing import FlaskResponse + + +def resolver(schema: Any) -> str: + schema_cls = resolve_schema_cls(schema) + name = schema_cls.__name__ + if name == "MetaSchema" and hasattr(schema_cls, "Meta"): + return f"{schema_cls.Meta.parent_schema_name}.{schema_cls.Meta.model.__name__}" + if name.endswith("Schema"): + return name[:-6] or name + return name + + +class SupersetOpenApi(BaseApi): + route_base = "/api" + allow_browser_login = True + + @expose("/<version>/_openapi") + @protect() + @safe + def get(self, version: str) -> FlaskResponse: Review Comment: **Suggestion:** This registers another handler on `/api/<version>/_openapi`, which already exists in the current API stack (`/api/v1/_openapi` is already exercised by integration tests). Enabling this feature alongside the default Swagger setting creates duplicate URL rules for the same path, so routing order determines which implementation runs and the APPLICATION_ROOT-aware behavior may not take effect. Avoid reusing the same route unless the original one is disabled, or gate registration to ensure only one OpenAPI endpoint is active. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ APPLICATION_ROOT-aware OpenAPI endpoint never handles `/api/v1/_openapi`. - ⚠️ Swagger still serves spec without URL prefix support. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Observe that the OpenAPI spec endpoint `/api/v1/_openapi` already exists and is exercised by tests: `tests/integration_tests/base_api_tests.py:3-15` and `:46-55` both call `self.client.get("api/v1/_openapi")` and assert `status_code == 200`, showing a pre-existing handler from Flask-AppBuilder. 2. Enable the new APPLICATION_ROOT-aware Swagger feature by setting `FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT = True` while keeping the default `FAB_API_SWAGGER_UI = True` (both defaults are defined in `superset/config.py:392-401` and there is no code disabling the FAB handler when the new flag is enabled). 3. During application initialization, `SupersetAppInitializer` registers the new API class via `appbuilder.add_api(SupersetOpenApi)` when the new flag is true (`superset/initialization/__init__.py:467-469`), adding a new route with `route_base = "/api"` and `@expose("/<version>/_openapi")` (defined in `superset/openapi/manager.py:47-54`), which yields the same URL path `/api/v1/_openapi` as the existing FAB OpenAPI endpoint. 4. With both the original FAB OpenAPI endpoint and `SupersetOpenApi.get()` mapped to `/api/v1/_openapi`, Flask's URL map ends up with duplicate rules for the same path and method; the first-registered rule (the FAB handler, verified by the pre-existing tests) continues to satisfy requests to `/api/v1/_openapi`, so `SupersetOpenApi.get()` is never invoked and its APPLICATION_ROOT-aware `APISpec` configuration at `superset/openapi/manager.py:87-99` is not used, leaving the OpenAPI servers URL unaffected even when the new feature flag is enabled. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=40ede3c421db4c9798dca3e4c7027e8a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=40ede3c421db4c9798dca3e4c7027e8a&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/openapi/manager.py **Line:** 47:54 **Comment:** *Api Mismatch: This registers another handler on `/api/<version>/_openapi`, which already exists in the current API stack (`/api/v1/_openapi` is already exercised by integration tests). Enabling this feature alongside the default Swagger setting creates duplicate URL rules for the same path, so routing order determines which implementation runs and the APPLICATION_ROOT-aware behavior may not take effect. Avoid reusing the same route unless the original one is disabled, or gate registration to ensure only one OpenAPI endpoint is active. 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%2F40908&comment_hash=d85739aeaebde19e837e36f0ceb3e77ea7cdda3b0b1094fdad80df5d30c8327e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40908&comment_hash=d85739aeaebde19e837e36f0ceb3e77ea7cdda3b0b1094fdad80df5d30c8327e&reaction=dislike'>👎</a> ########## superset/openapi/manager.py: ########## @@ -0,0 +1,116 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""APPLICATION_ROOT-aware OpenAPI spec and Swagger UI. + +Serves the OpenAPI spec and Swagger UI in a way that works when Superset is +deployed behind a URL prefix (reverse proxy) via ``APPLICATION_ROOT``. Enabled +by the ``FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT`` config flag. +""" + +from typing import Any + +from apispec import APISpec +from apispec.ext.marshmallow import MarshmallowPlugin +from apispec.ext.marshmallow.common import resolve_schema_cls +from flask import current_app, request +from flask_appbuilder.api import BaseApi, expose, protect, safe +from flask_appbuilder.baseviews import BaseView +from flask_appbuilder.security.decorators import has_access + +from superset.superset_typing import FlaskResponse + + +def resolver(schema: Any) -> str: + schema_cls = resolve_schema_cls(schema) + name = schema_cls.__name__ + if name == "MetaSchema" and hasattr(schema_cls, "Meta"): + return f"{schema_cls.Meta.parent_schema_name}.{schema_cls.Meta.model.__name__}" + if name.endswith("Schema"): + return name[:-6] or name + return name + + +class SupersetOpenApi(BaseApi): + route_base = "/api" + allow_browser_login = True + + @expose("/<version>/_openapi") + @protect() + @safe + def get(self, version: str) -> FlaskResponse: + """Render the OpenAPI spec for every view that belongs to a version. + --- + get: + description: >- + Get the OpenAPI spec for a specific API version + parameters: + - in: path + schema: + type: string + name: version + responses: + 200: + description: The OpenAPI spec + content: + application/json: + schema: + type: object + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ + version_found = False + api_spec = self._create_api_spec(version) + for base_api in current_app.appbuilder.baseviews: + if isinstance(base_api, BaseApi) and base_api.version == version: + base_api.add_api_spec(api_spec) + version_found = True + if version_found: + return self.response(200, **api_spec.to_dict()) + return self.response_404() + + @staticmethod + def _create_api_spec(version: str) -> APISpec: + app_root = current_app.config.get("APPLICATION_ROOT", "/") + default_server = {"url": request.host_url.rstrip("/") + app_root} + servers = current_app.config.get("FAB_OPENAPI_SERVERS", [default_server]) + return APISpec( + title=current_app.appbuilder.app_name, + version=version, + openapi_version="3.0.2", + info={"description": current_app.appbuilder.app_name}, + plugins=[MarshmallowPlugin(schema_name_resolver=resolver)], + servers=servers, + ) + + +class SupersetSwaggerView(BaseView): + route_base = "/swagger" + default_view = "show" + openapi_uri = "/api/{}/_openapi" + + @expose("/<version>") + @has_access + def show(self, version: str) -> FlaskResponse: + app_root = current_app.config.get("APPLICATION_ROOT", "") + self.openapi_uri + return self.render_template( + current_app.config.get( + "FAB_API_SWAGGER_TEMPLATE", "appbuilder/swagger/swagger.html" + ), + openapi_uri=app_root.format(version), + ) Review Comment: **Suggestion:** Building the Swagger spec URL by directly concatenating `APPLICATION_ROOT` can produce an invalid protocol-relative URL (`//api/...`) when `APPLICATION_ROOT` is `/` (the Flask default) or has a trailing slash. In that case Swagger UI will request the wrong host/path and fail to load the OpenAPI spec. Normalize the prefix (strip trailing slash and treat `/` as empty) before appending `openapi_uri`. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Swagger UI requests spec from invalid protocol-relative URL. - ⚠️ OpenAPI docs unusable when APPLICATION_ROOT "/" with flag. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure Superset with `FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT = True` while leaving `APPLICATION_ROOT` at its default of "/" (see `superset/config.py:392-401` where both flags are defined and default values are set). 2. Start the application; during initialization `SupersetAppInitializer.init_app()` registers `SupersetSwaggerView` when `FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT` is true (`superset/initialization/__init__.py:440-469`, `appbuilder.add_view_no_menu(SupersetSwaggerView)`). 3. Issue a GET request to the Swagger UI endpoint `/swagger/v1`, which is routed to `SupersetSwaggerView.show()` at `superset/openapi/manager.py:107-115` via `route_base = "/swagger"` and `@expose("/<version>")`. 4. Inside `show()`, with `APPLICATION_ROOT == "/"`, line `110` computes `app_root = "/" + "/api/{}/_openapi"` resulting in `"//api/{}/_openapi"`, and this is passed to the Swagger template as `openapi_uri` (lines `111-115`); the browser then requests `//api/v1/_openapi`, a protocol-relative URL that resolves to host `api` instead of the Superset host, so the OpenAPI spec fails to load in Swagger UI. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=259ec950fe2c4cfd8a9138f66a9e372d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=259ec950fe2c4cfd8a9138f66a9e372d&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/openapi/manager.py **Line:** 110:116 **Comment:** *Logic Error: Building the Swagger spec URL by directly concatenating `APPLICATION_ROOT` can produce an invalid protocol-relative URL (`//api/...`) when `APPLICATION_ROOT` is `/` (the Flask default) or has a trailing slash. In that case Swagger UI will request the wrong host/path and fail to load the OpenAPI spec. Normalize the prefix (strip trailing slash and treat `/` as empty) before appending `openapi_uri`. 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%2F40908&comment_hash=3369007b909454afa6d3fd70177b9fe831160bcc512fd6b6219d0be7c9e89323&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40908&comment_hash=3369007b909454afa6d3fd70177b9fe831160bcc512fd6b6219d0be7c9e89323&reaction=dislike'>👎</a> ########## superset/initialization/__init__.py: ########## @@ -463,6 +464,9 @@ def init_views(self) -> None: appbuilder.add_view_no_menu(RedirectView) appbuilder.add_view_no_menu(RoleRestAPI) appbuilder.add_view_no_menu(UserInfoView) + if self.config.get("FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT", False): + appbuilder.add_api(SupersetOpenApi) + appbuilder.add_view_no_menu(SupersetSwaggerView) Review Comment: **Suggestion:** The new gate only checks `FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT` and ignores `FAB_API_SWAGGER_UI`, so Swagger/OpenAPI endpoints can be re-enabled even when operators explicitly disabled Swagger globally. This breaks the existing config contract and can unintentionally expose API documentation. Gate this block on both flags (or otherwise ensure the global Swagger disable still takes precedence). [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Swagger UI exposed despite FAB_API_SWAGGER_UI set False. - ⚠️ Public REST API surface easier to enumerate externally. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure Superset so Swagger UI is globally disabled by setting `FAB_API_SWAGGER_UI = False` in `superset_config.py`, using the documented behavior in `UPDATING.md:20-22` and `superset/config.py:13-16` (where `FAB_API_SWAGGER_UI` is defined as the Swagger UI enable flag). 2. In the same `superset_config.py`, enable the new prefix-aware Swagger implementation by setting `FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT = True`, which the code defines and documents in `superset/config.py:17-22`. 3. Start Superset so that `superset.initialization.__init__.AppInitializer.init_views` runs; at `superset/initialization/__init__.py:467-469` the code checks only `self.config.get("FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT", False)` and, when True, calls `appbuilder.add_api(SupersetOpenApi)` and `appbuilder.add_view_no_menu(SupersetSwaggerView)` regardless of `FAB_API_SWAGGER_UI`. 4. Send a GET request to `/swagger/v1` (the documented Swagger UI path in `docs/docs/faq.mdx:13-18` and `UPDATING.md:20-22`); despite `FAB_API_SWAGGER_UI = False`, the newly registered `SupersetSwaggerView` from `superset/openapi/manager.py:102-109` serves the Swagger UI, effectively re-enabling API documentation while the global disable flag is set. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7a88d3f813864e7693e56d15137ba57e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7a88d3f813864e7693e56d15137ba57e&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/initialization/__init__.py **Line:** 467:469 **Comment:** *Incorrect Condition Logic: The new gate only checks `FAB_API_SWAGGER_UI_SUPERSET_APP_ROOT` and ignores `FAB_API_SWAGGER_UI`, so Swagger/OpenAPI endpoints can be re-enabled even when operators explicitly disabled Swagger globally. This breaks the existing config contract and can unintentionally expose API documentation. Gate this block on both flags (or otherwise ensure the global Swagger disable still takes precedence). 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%2F40908&comment_hash=ac94b80ec0660333ac509b55874cd6c43f4f3e9e2d48baa7bf2243e2c1803919&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40908&comment_hash=ac94b80ec0660333ac509b55874cd6c43f4f3e9e2d48baa7bf2243e2c1803919&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]
