codeant-ai-for-open-source[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3762478438
##########
superset/commands/dataset/update.py:
##########
@@ -183,21 +192,33 @@ def _validate_dataset_source(self, exceptions:
list[ValidationError]) -> None:
):
exceptions.append(DatasetExistsValidationError(table))
- self._validate_sql_access(db, catalog, schema, exceptions)
+ # Repointing a physical dataset (or converting a virtual dataset to a
+ # physical one) runs the same data-access check as the create path.
+ sql = self._properties.get("sql", self._model.sql)
+ if not sql and (
+ source_changed or ("sql" in self._properties and self._model.sql)
+ ):
+ try:
+ security_manager.raise_for_access(database=db, table=table)
+ except SupersetSecurityException as ex:
+
exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
+
Review Comment:
**Suggestion:** The database-repoint path already calls `raise_for_access`
for the target table above, but this new block runs the same check again
whenever `database_changed` is true. If access is denied, the same validation
error is appended twice, producing duplicate errors in the update response.
Avoid repeating the table-level check when the database-repoint check has
already covered it. [logic error]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Dataset updates to unauthorized replacement databases return duplicate
validation errors.
- โ ๏ธ API clients receive redundant `DatasetDataAccessIsNotAllowed` entries.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b5f09afd0de7491196cfb87cb6194d6f&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=b5f09afd0de7491196cfb87cb6194d6f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/update.py
**Line:** 195:205
**Comment:**
*Logic Error: The database-repoint path already calls
`raise_for_access` for the target table above, but this new block runs the same
check again whenever `database_changed` is true. If access is denied, the same
validation error is appended twice, producing duplicate errors in the update
response. Avoid repeating the table-level check when the database-repoint check
has already covered it.
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%2F42929&comment_hash=f8061338afbe0f58bad2d70af77dacbf61b2a8ecf14e4bcc067ab53af0d77bfb&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=f8061338afbe0f58bad2d70af77dacbf61b2a8ecf14e4bcc067ab53af0d77bfb&reaction=dislike'>๐</a>
##########
superset/commands/report/base.py:
##########
@@ -58,6 +68,52 @@ def run(self) -> Any:
def validate(self) -> None:
pass
+ def validate_alert_query(
+ self,
+ database: Database,
+ sql: str,
+ exceptions: list[ValidationError],
+ ) -> None:
+ """
+ Validate alert SQL at save time: it must parse as a single statement,
+ must not mutate state unless the database allows DML, and the saving
+ user must be authorized for the tables it reads. Templated SQL that
+ only parses after rendering is validated at execution time on the
+ rendered query.
+ """
+ contains_jinja = bool(_JINJA_BLOCK_RE.search(sql))
+ try:
+ script = SQLScript(sql, engine=database.backend)
+ except SupersetParseError as ex:
+ if not contains_jinja:
+ exceptions.append(
+ ValidationError(
+ _("Invalid SQL: %(error)s", error=ex.error.message),
+ field_name="sql",
+ )
+ )
+ return
+ if len(script.statements) != 1:
+ exceptions.append(AlertQueryMultipleStatementsValidationError())
Review Comment:
**Suggestion:** Templated SQL is supposed to defer static validation until
execution, but `contains_jinja` is only consulted when parsing raises an
exception. If the unresolved template happens to parse as a single SQL
statement, this code still applies statement-count, DML, and authorization
checks to the unrendered SQL and can reject valid templates based on
placeholder-shaped table references. Skip all save-time static checks whenever
Jinja is present; the rendered execution path already performs the required
validation and authorization. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ Valid Jinja-based alerts can fail during create or update validation.
- โ ๏ธ Dynamic table references are checked against unresolved SQL.
- โ ๏ธ Alert execution never reaches rendered-query validation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0ea0dac794994b66aac03f185fe0d3a3&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=0ea0dac794994b66aac03f185fe0d3a3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/base.py
**Line:** 84:97
**Comment:**
*Api Mismatch: Templated SQL is supposed to defer static validation
until execution, but `contains_jinja` is only consulted when parsing raises an
exception. If the unresolved template happens to parse as a single SQL
statement, this code still applies statement-count, DML, and authorization
checks to the unrendered SQL and can reject valid templates based on
placeholder-shaped table references. Skip all save-time static checks whenever
Jinja is present; the rendered execution path already performs the required
validation and authorization.
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%2F42929&comment_hash=d002c91052eb845d08f037b1df23ec4c5abafb532edf48fd25110a867cb3fab5&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=d002c91052eb845d08f037b1df23ec4c5abafb532edf48fd25110a867cb3fab5&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]
