codeant-ai-for-open-source[bot] commented on code in PR #36740:
URL: https://github.com/apache/superset/pull/36740#discussion_r2631635722
##########
superset-frontend/src/dashboard/types.ts:
##########
@@ -130,6 +130,21 @@ export type DashboardState = {
};
chartStates?: Record<string, any>;
};
+/**
+ * Template metadata fields that can be stored either:
+ * 1. Nested under "template_info" key (new format after import)
+ * 2. At the top level of metadata (legacy format)
+ */
+export interface TemplateInfo {
+ is_template?: boolean;
+ is_featured_template?: boolean;
+ template_category?: string;
+ template_thumbnail_url?: string;
Review Comment:
**Suggestion:** The `TemplateInfo` interface types `template_category` and
`template_thumbnail_url` as `string`, but backend responses and related
frontend types (e.g. `DashboardTemplate`) allow these fields to be `null`, so
this mismatch can lead to unsafe assumptions and runtime errors when code uses
these fields as always-strings while receiving `null` from the API. [type error]
**Severity Level:** Minor ⚠️
```suggestion
template_category?: string | null;
template_thumbnail_url?: string | null;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR added TemplateInfo with optional string fields for template_category
and template_thumbnail_url.
If the backend can return null for these keys (common for optional
metadata), typing them as only 'string' is inaccurate and can lead to unsafe
assumptions or the need for non-null assertions elsewhere.
Widening the types to 'string | null' aligns the TypeScript model with
nullable API responses and forces callers to handle the null case explicitly,
preventing runtime surprises.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/types.ts
**Line:** 141:142
**Comment:**
*Type Error: The `TemplateInfo` interface types `template_category` and
`template_thumbnail_url` as `string`, but backend responses and related
frontend types (e.g. `DashboardTemplate`) allow these fields to be `null`, so
this mismatch can lead to unsafe assumptions and runtime errors when code uses
these fields as always-strings while receiving `null` from the API.
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.
```
</details>
##########
superset-frontend/src/dashboard/selectors.ts:
##########
@@ -21,6 +21,9 @@ import { RootState } from 'src/dashboard/types';
/**
* Selector to check if the current dashboard is a template.
* Template dashboards are read-only and cannot be edited.
+ *
+ * Template metadata is stored in the nested "template_info" structure
+ * within the dashboard's metadata.
*/
export const selectIsTemplateDashboard = (state: RootState): boolean =>
- !!state.dashboardInfo?.metadata?.is_template;
+ !!state.dashboardInfo?.metadata?.template_info?.is_template;
Review Comment:
**Suggestion:** The selector only checks the nested
`template_info.is_template` flag and ignores the documented legacy case where
template metadata may still be stored at the top level as
`metadata.is_template`, leading to inconsistent behavior (some parts of the UI
will treat a dashboard as a template while this selector returns false), so the
selector should fall back to the top-level flag when `template_info` is not
present. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
!!(
state.dashboardInfo?.metadata?.template_info?.is_template ??
(state.dashboardInfo?.metadata as any)?.is_template
);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current selector only inspects metadata.template_info.is_template and
will miss dashboards that still set metadata.is_template (legacy format). That
can produce real inconsistent UI behavior (some code paths might treat the
dashboard as a template while this selector does not). The proposed fallback is
a small, targeted compatibility fix that resolves a real logic gap rather than
a cosmetic change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/selectors.ts
**Line:** 29:29
**Comment:**
*Logic Error: The selector only checks the nested
`template_info.is_template` flag and ignores the documented legacy case where
template metadata may still be stored at the top level as
`metadata.is_template`, leading to inconsistent behavior (some parts of the UI
will treat a dashboard as a template while this selector returns false), so the
selector should fall back to the top-level flag when `template_info` is not
present.
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.
```
</details>
##########
superset/daos/dashboard.py:
##########
@@ -145,9 +145,11 @@ def get_templates() -> list[Dashboard]:
templates = []
for dashboard in dashboards:
metadata = json.loads(dashboard.json_metadata or "{}")
- if metadata.get("is_template", False):
+ # Template metadata is stored in the nested template_info structure
+ template_info = metadata.get("template_info", {})
+ if template_info.get("is_template", False):
Review Comment:
**Suggestion:** The new template metadata handling assumes `template_info`
is always a dict, but `DashboardJSONMetadataSchema` explicitly allows
`template_info` to be `null`; if any dashboard has `"template_info": null` in
its `json_metadata`, `template_info.get(...)` will raise an AttributeError at
runtime when iterating templates. Guarding for non-dict values prevents this
crash while keeping the intended behavior. [type error]
**Severity Level:** Minor ⚠️
```suggestion
template_info = metadata.get("template_info") or {}
if isinstance(template_info, dict) and
template_info.get("is_template", False):
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code uses metadata.get("template_info", {}) which only provides
the {} default when the key is missing.
If the key exists but is explicitly null (template_info: null),
metadata.get(...) returns None and template_info.get(...) will raise
AttributeError.
The suggested change (use metadata.get("template_info") or {} and guard with
isinstance(..., dict)) prevents that runtime crash while preserving intended
behavior.
This fixes a real runtime bug and is minimal and safe.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/dashboard.py
**Line:** 149:150
**Comment:**
*Type Error: The new template metadata handling assumes `template_info`
is always a dict, but `DashboardJSONMetadataSchema` explicitly allows
`template_info` to be `null`; if any dashboard has `"template_info": null` in
its `json_metadata`, `template_info.get(...)` will raise an AttributeError at
runtime when iterating templates. Guarding for non-dict values prevents this
crash while keeping the intended behavior.
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.
```
</details>
##########
superset/commands/dashboard_generator/template_analyzer.py:
##########
@@ -133,6 +134,12 @@ def analyze(self, dashboard: Dashboard) ->
TemplateRequirements:
self._extract_dataset_info(dashboard)
)
+ # Extract template context if present
+ metadata = json.loads(dashboard.json_metadata or "{}")
+ template_info = metadata.get("template_info", {}) if
isinstance(metadata, dict) else {}
+ if isinstance(template_info, dict) and "template_context" in
template_info:
+ requirements.template_context =
template_info.get("template_context")
Review Comment:
**Suggestion:** The value stored in `requirements.template_context` is taken
directly from `template_info["template_context"]`, which elsewhere in the
codebase is treated as a JSON string, while
`TemplateRequirements.template_context` and downstream consumers (like the LLM
prompt builder) expect a structured dict; this causes the context to be
double-encoded with `json.dumps` and breaks the intended contract of providing
a structured context object to the reasoning loop. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
template_info = (
metadata.get("template_info", {}) if isinstance(metadata, dict)
else {}
)
if isinstance(template_info, dict) and "template_context" in
template_info:
raw_context = template_info.get("template_context")
# Normalize to a dict so downstream consumers see structured
context
if isinstance(raw_context, str):
try:
requirements.template_context = json.loads(raw_context)
except Exception:
logger.warning(
"Failed to parse template_context JSON for dashboard
%s",
dashboard.id,
)
requirements.template_context = None
elif isinstance(raw_context, dict):
requirements.template_context = raw_context
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The LLM prompt builders and other consumers in this PR expect
template_context to be a structured dict (see
llm_service._build_dataset_generation_prompt which json.dumps(template_context)
and LLM function signatures typed as dict[str, Any]). However the dashboard
API/schema/store often keeps template_context as a JSON string (frontend and
marshmallow schemas show template_context as String). Leaving the raw string
through leads to double-encoding when later json.dumps is applied and can break
prompt formatting or downstream logic. The suggested normalization (attempt to
json.loads if the stored value is a str) fixes a real logic mismatch between
storage (string) and runtime expectation (dict) and is therefore a valid,
non-cosmetic bug fix.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dashboard_generator/template_analyzer.py
**Line:** 139:141
**Comment:**
*Logic Error: The value stored in `requirements.template_context` is
taken directly from `template_info["template_context"]`, which elsewhere in the
codebase is treated as a JSON string, while
`TemplateRequirements.template_context` and downstream consumers (like the LLM
prompt builder) expect a structured dict; this causes the context to be
double-encoded with `json.dumps` and breaks the intended contract of providing
a structured context object to the reasoning loop.
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.
```
</details>
##########
superset/security/manager.py:
##########
@@ -2926,7 +2926,8 @@ def _is_template_dashboard(dashboard: "Dashboard") ->
bool:
return False
try:
metadata = json.loads(dashboard.json_metadata)
- return metadata.get("is_template", False)
+ template_info = metadata.get("template_info", {})
+ return template_info.get("is_template", False)
Review Comment:
**Suggestion:** The updated template detection assumes that
`json.loads(dashboard.json_metadata)` always returns a dict, but if the stored
JSON is valid yet non-dict (for example `"null"`, a list, or a string), then
`metadata.get(...)` and `template_info.get(...)` will raise AttributeError,
causing `_is_template_dashboard` to crash instead of safely treating the
dashboard as non-template. Adding type checks around `metadata` and
`template_info` (and defaulting to `{}` when appropriate) prevents these
runtime errors and keeps access checks robust against unexpected JSON shapes.
[type error]
**Severity Level:** Minor ⚠️
```suggestion
metadata = json.loads(dashboard.json_metadata or "{}")
if not isinstance(metadata, dict):
return False
template_info = metadata.get("template_info", {})
if not isinstance(template_info, dict):
return False
return bool(template_info.get("is_template", False))
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code calls .get() on metadata and template_info without
verifying their types. json.loads(...) can legally return non-dict values
(None, list, string, etc.), which would make metadata.get raise AttributeError
and crash the access check. The proposed change defensively handles non-dict
JSON shapes and returns False for unexpected metadata, preserving behavior and
avoiding runtime errors. This fixes a real robustness bug introduced by the PR
change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 2928:2930
**Comment:**
*Type Error: The updated template detection assumes that
`json.loads(dashboard.json_metadata)` always returns a dict, but if the stored
JSON is valid yet non-dict (for example `"null"`, a list, or a string), then
`metadata.get(...)` and `template_info.get(...)` will raise AttributeError,
causing `_is_template_dashboard` to crash instead of safely treating the
dashboard as non-template. Adding type checks around `metadata` and
`template_info` (and defaulting to `{}` when appropriate) prevents these
runtime errors and keeps access checks robust against unexpected JSON shapes.
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.
```
</details>
##########
superset/dashboards/api.py:
##########
@@ -702,20 +702,26 @@ def templates(self) -> Response:
return self.response_500(message=str(ex))
def _enrich_template_with_metadata(self, dashboard: Dashboard) ->
dict[str, Any]:
- """Extract template metadata from json_metadata into top-level
fields."""
+ """Extract template metadata from json_metadata into top-level fields.
+
+ Template metadata is stored in the nested "template_info" structure
+ within the dashboard's json_metadata.
+ """
metadata = json.loads(dashboard.json_metadata or "{}")
+ template_info = metadata.get("template_info", {})
Review Comment:
**Suggestion:** The new code assumes that `dashboard.json_metadata` always
deserializes into a dictionary; if it is valid JSON but not an object (for
example a list or string), `template_info = metadata.get("template_info", {})`
will raise an `AttributeError` and cause the `/templates` endpoint to fail for
that dashboard instead of gracefully treating it as having no template
metadata. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
template_info = metadata.get("template_info", {}) if
isinstance(metadata, dict) else {}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The code calls json.loads on dashboard.json_metadata and immediately uses
.get on the result.
If the stored JSON is valid but not an object (e.g. "[]" or "\"string\""),
metadata will be a list or str
and metadata.get(...) raises AttributeError at runtime. That would make the
/templates endpoint fail
for dashboards with non-object json_metadata. The proposed guard (use {}
when metadata is not a dict)
fixes a real runtime bug and is a minimal, safe change localized to this
function.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 711:711
**Comment:**
*Logic Error: The new code assumes that `dashboard.json_metadata`
always deserializes into a dictionary; if it is valid JSON but not an object
(for example a list or string), `template_info = metadata.get("template_info",
{})` will raise an `AttributeError` and cause the `/templates` endpoint to fail
for that dashboard instead of gracefully treating it as having no template
metadata.
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.
```
</details>
--
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]
