korbit-ai[bot] commented on code in PR #32228:
URL: https://github.com/apache/superset/pull/32228#discussion_r1951693721
##########
superset/jinja_context.py:
##########
@@ -908,16 +929,32 @@ def metric_macro(metric_key: str, dataset_id:
Optional[int] = None) -> str:
dataset = DatasetDAO.find_by_id(dataset_id)
if not dataset:
raise DatasetNotFoundError(f"Dataset ID {dataset_id} not found.")
+
metrics: dict[str, str] = {
metric.metric_name: metric.expression for metric in dataset.metrics
}
- dataset_name = dataset.table_name
- if metric := metrics.get(metric_key):
- return metric
- raise SupersetTemplateException(
- _(
- "Metric ``%(metric_name)s`` not found in %(dataset_name)s.",
- metric_name=metric_key,
- dataset_name=dataset_name,
+ if metric_key not in metrics:
+ raise SupersetTemplateException(
+ _(
+ "Metric ``%(metric_name)s`` not found in %(dataset_name)s.",
+ metric_name=metric_key,
+ dataset_name=dataset.table_name,
+ )
)
- )
+
+ definition = metrics[metric_key]
+
+ env = SandboxedEnvironment(undefined=DebugUndefined)
+ context = {"metric": partial(safe_proxy, metric_macro)}
+ while has_metric_macro(definition, env):
+ old_definition = definition
+ template = env.from_string(definition)
+ try:
+ definition = template.render(context)
+ except RecursionError as ex:
+ raise SupersetTemplateException("Cyclic metric macro detected")
from ex
+
+ if definition == old_definition:
+ break
Review Comment:
### Weak cycle detection in metric macro resolution <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The infinite loop prevention logic using definition == old_definition will
fail to detect some types of cyclic dependencies where the rendered output
doesn't match exactly with the input but still creates a cycle.
###### Why this matters
If metric A references metric B which references metric A with slight
variations in whitespace or formatting, the current check won't detect the
cycle until a RecursionError occurs.
###### Suggested change ∙ *Feature Preview*
Implement a visited set to track processed metric names and detect cycles
earlier:
```python
def metric_macro(metric_key: str, dataset_id: Optional[int] = None, visited:
Optional[set[str]] = None) -> str:
if visited is None:
visited = set()
if metric_key in visited:
raise SupersetTemplateException(f"Cyclic dependency detected for
metric {metric_key}")
visited.add(metric_key)
# ... rest of the function
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a33dd8f8-f111-4253-8c9b-627b5679f11d?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:7632080c-648e-4b9a-8ccc-5dc48d6726c0 -->
--
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]
