codeant-ai-for-open-source[bot] commented on code in PR #42095:
URL: https://github.com/apache/superset/pull/42095#discussion_r3591325938
##########
superset/models/helpers.py:
##########
@@ -749,6 +749,109 @@ def modified(self) -> Markup:
_NO_BYPASS: frozenset[type] = frozenset()
+def _make_metric_line_comments_safe( # noqa: C901
+ expression: str,
+) -> tuple[str, bool]:
+ """Convert SQL line comments without regenerating otherwise valid SQL."""
+ result: list[str] = []
+ index = 0
+ quote: str | None = None
+ dollar_quote: str | None = None
+ backslash_escapes = False
+ in_block_comment = False
+ while index < len(expression):
+ if in_block_comment:
+ end = expression.find("*/", index)
+ if end < 0:
+ return expression, False
+ result.append(expression[index : end + 2])
+ index = end + 2
+ in_block_comment = False
+ continue
+ if dollar_quote:
+ end = expression.find(dollar_quote, index)
+ if end < 0:
+ return expression, False
+ result.append(expression[index : end + len(dollar_quote)])
+ index = end + len(dollar_quote)
+ dollar_quote = None
+ continue
+ if quote:
+ character = expression[index]
+ result.append(character)
+ index += 1
+ if backslash_escapes and character == "\\" and index <
len(expression):
+ result.append(expression[index])
+ index += 1
+ elif character == quote:
+ if index < len(expression) and expression[index] == quote:
+ result.append(expression[index])
+ index += 1
+ else:
+ quote = None
+ backslash_escapes = False
+ continue
+
+ if expression.startswith("/*", index):
+ in_block_comment = True
+ continue
+ if expression.startswith("--", index):
+ line_end = len(expression)
+ for separator in ("\n", "\r"):
+ separator_index = expression.find(separator, index + 2)
+ if separator_index >= 0:
+ line_end = min(line_end, separator_index)
+ contents = expression[index + 2 : line_end]
+ if "*/" in contents:
+ return expression, False
+ result.append(f"/*{contents} */")
+ index = line_end
+ continue
+ if expression[index] in {"'", '"'}:
+ quote = expression[index]
+ backslash_escapes = (
+ quote == "'"
+ and index > 0
+ and expression[index - 1] in {"E", "e"}
+ and (index == 1 or not expression[index - 2].isalnum())
+ )
+ result.append(quote)
+ index += 1
+ continue
+ if expression[index] == "$":
+ match = re.match(r"\$[A-Za-z_][A-Za-z0-9_]*\$|\$\$",
expression[index:])
+ if match:
+ dollar_quote = match.group(0)
+ result.append(dollar_quote)
+ index += len(dollar_quote)
+ continue
+ result.append(expression[index])
+ index += 1
+ if quote or dollar_quote or in_block_comment:
+ return expression, False
+ return "".join(result), True
+
+
+def _normalize_custom_sql_metric(
+ expression: str,
+ engine: str,
+ normalizer: Callable[[str], str],
+) -> tuple[str, bool]:
+ """Normalize a metric and report whether its source is safe to preserve."""
+ expression = normalizer(expression)
+ if engine not in {"postgresql", "redshift"}:
+ return expression, False
+
+ expression, source_is_safe = _make_metric_line_comments_safe(expression)
+ if source_is_safe:
+ return expression, True
+
Review Comment:
**Suggestion:** The safe-return branch skips the trailing-semicolon trimming
that `sanitize_clause` normally enforces. `_normalize_custom_sql_metric` is
also used by saved metrics (`SqlMetric.get_sqla_col`), and that caller does not
re-sanitize, so a saved metric like `SUM(x);` can be emitted with `;` inside
the SELECT list and break query compilation/execution. Apply the same
`rstrip().rstrip(";").rstrip()` normalization before returning the
`source_is_safe` path. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Saved PostgreSQL metrics ending semicolon cause SQL errors.
- ⚠️ Redshift charts using such metrics fail to render.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Define a saved SQL metric on a PostgreSQL or Redshift dataset via the UI,
with an
expression ending in a semicolon (for example `SUM(amount);`). Saved metrics
are
represented by `SqlMetric` in
`superset/connectors/sqla/models.py:1185-1275`, using the
`expression` column as the raw SQL.
2. Build a chart that uses this saved metric either as a regular metric or
as a series
limit metric. When the query is built, `SqlaTable._get_series_orderby` in
`superset/connectors/sqla/models.py:1889-1929` (mirrored by
`SupersetQuery._get_series_orderby` in `superset/models/helpers.py:2924-32`)
resolves the
metric name through `metrics_by_name[series_limit_metric].get_sqla_col(...)`.
3. In `SqlMetric.get_sqla_col`
(`superset/connectors/sqla/models.py:1230-75`), the metric
expression is read from `self.expression` and passed to
`_normalize_custom_sql_metric(expression, self.table.database.backend,
self.table.database.db_engine_spec.normalize_custom_sql_metric)` defined in
`superset/models/helpers.py:835-133`.
4. For PostgreSQL and Redshift engines, `_normalize_custom_sql_metric` first
applies the
engine normalizer (Postgres implementation at
`superset/db_engine_specs/postgres.py:57-59`
only canonicalizes `DATE_TRUNC` units), then calls
`_make_metric_line_comments_safe`.
Because the expression has no line comments,
`_make_metric_line_comments_safe` returns the
original text unchanged and `source_is_safe=True`, and
`_normalize_custom_sql_metric`
returns the unmodified `expression` without calling `sanitize_clause` or
trimming the
trailing semicolon (helpers.py:842-848).
5. `SqlMetric.get_sqla_col` ignores `_source_is_safe` and constructs
`sqla_col =
literal_column(expression)` using the still-terminated SQL, then labels it
via
`Database.make_sqla_column_compatible` (`superset/models/core.py:1366-23`).
The generated
query ends up with a SELECT list containing `SUM(amount);` as a column
expression, which
breaks compilation because `;` terminates the statement and is invalid
inside a SELECT
list, leading to a database syntax error. This differs from the adhoc metric
path in
`_process_sql_expression` (`superset/models/helpers.py:23-56`), where even
when
`source_is_safe` is true, `source_expression` is post-trimmed with
`.rstrip().rstrip(";").rstrip()` to remove trailing terminators.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ae2932c5564741638b2af220283abec6&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=ae2932c5564741638b2af220283abec6&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/models/helpers.py
**Line:** 846:848
**Comment:**
*Api Mismatch: The safe-return branch skips the trailing-semicolon
trimming that `sanitize_clause` normally enforces.
`_normalize_custom_sql_metric` is also used by saved metrics
(`SqlMetric.get_sqla_col`), and that caller does not re-sanitize, so a saved
metric like `SUM(x);` can be emitted with `;` inside the SELECT list and break
query compilation/execution. Apply the same `rstrip().rstrip(";").rstrip()`
normalization before returning the `source_is_safe` path.
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%2F42095&comment_hash=2c1c3f057593bb828516d80a86245a5ed3e8e32b2de57157db233d5f8c12fa62&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42095&comment_hash=2c1c3f057593bb828516d80a86245a5ed3e8e32b2de57157db233d5f8c12fa62&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]
