korbit-ai[bot] commented on code in PR #32154:
URL: https://github.com/apache/superset/pull/32154#discussion_r1943630553
##########
superset/sqllab/permalink/api.py:
##########
@@ -103,10 +105,10 @@ def post(self) -> Response:
log_to_statsd=False,
)
def get(self, key: str) -> Response:
- """Get permanent link state for SQLLab editor.
+ """Get chart's permanent link state.
---
get:
- summary: Get permanent link state for SQLLab editor.
+ summary: Get chart's permanent link state
Review Comment:
### Incorrect API Documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The API documentation incorrectly refers to 'chart's permanent link state'
when this endpoint actually returns SQLLab's permanent link state.
###### Why this matters
This misleading documentation could cause confusion for API consumers and
developers, potentially leading to integration errors or misuse of the API
endpoint.
###### Suggested change ∙ *Feature Preview*
Update the documentation to correctly reflect that this endpoint returns
SQLLab's permalink state:
```python
def get(self, key: str) -> Response:
"""Get SQLLab's permanent link state.
---
get:
summary: Get SQLLab's permanent link state
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/caf1a467-f435-41bb-a83c-49d342975d9c?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:d7b4da27-9fb1-4003-852c-1afdf47f0ec4 -->
##########
superset/sqllab/permalink/api.py:
##########
@@ -88,6 +88,8 @@
state = self.add_model_schema.load(request.json)
key = CreateSqlLabPermalinkCommand(state=state).run()
url = url_for("SqllabView.root", key=key, _external=True)
+ if "/sqllab/?key=" in url:
+ url = url.replace("/sqllab/?key=", "/sqllab/p/")
Review Comment:
### Unsafe URL Pattern Replacement <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The URL replacement logic uses string contains check and replace, which
could potentially modify unintended parts of the URL if the pattern appears
elsewhere.
###### Why this matters
If the URL contains the pattern '/sqllab/?key=' multiple times or in an
unexpected location, the replacement could lead to malformed URLs.
###### Suggested change ∙ *Feature Preview*
Use URL parsing and building to ensure safe URL transformation:
```python
from urllib.parse import urlparse, parse_qs, urlencode
parsed_url = urlparse(url)
if parsed_url.path == '/sqllab/' and 'key' in parse_qs(parsed_url.query):
key_value = parse_qs(parsed_url.query)['key'][0]
url = url.replace(f'/sqllab/?key={key_value}', f'/sqllab/p/{key_value}')
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9e0d74dc-92e7-458c-88d6-e68eccd2b2bc?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:939c2fd0-07c5-4514-bb0c-81cb0953841d -->
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
