codeant-ai-for-open-source[bot] commented on code in PR #38343:
URL: https://github.com/apache/superset/pull/38343#discussion_r2872936997
##########
superset/commands/importers/v1/assets.py:
##########
@@ -136,6 +138,12 @@ def _import(configs: dict[str, Any], sparse: bool = False)
-> None: # noqa: C90
charts.append(chart)
chart_ids[str(chart.uuid)] = chart.id
+ # Handle tags using import_tag function
+ if feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
+ if "tags" in config:
+ target_tag_names = config["tags"]
+ import_tag(target_tag_names, {}, chart.id, "chart",
db.session)
Review Comment:
**Suggestion:** Missing null check for `config["tags"]` before calling
`import_tag`. The schema defines `tags` with `allow_none=True`, meaning the
value can be present in the config but set to `None`. Passing `None` to
`import_tag` instead of a list will likely cause a TypeError or unexpected
behavior when the function tries to iterate over the tags. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Asset import crashes when importing charts with null tags.
- ❌ Bulk import operations fail if any chart has explicit null tags.
- ⚠️ Database transaction rollback triggered, blocking all assets in batch.
```
</details>
```suggestion
# Handle tags using import_tag function
if feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
if config.get("tags"):
target_tag_names = config["tags"]
import_tag(target_tag_names, {}, chart.id, "chart",
db.session)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable TAGGING_SYSTEM feature flag in Superset configuration (required
for PR
functionality).
2. Prepare asset import bundle containing a chart configuration file (e.g.,
`charts/my_chart.yaml`) with `tags: null` explicitly set (valid per
ImportV1ChartSchema
line 1623 which allows null).
3. Trigger asset import via `ImportAssetsCommand.run()` at line 220, which
calls
`_import()` at line 221.
4. In `_import()`, the chart processing loop (lines 133-145) encounters the
chart config
with tags key present but None value.
5. Code at line 143 checks `if "tags" in config:` which evaluates to True
when tags is
explicitly null.
6. Line 144 assigns `target_tag_names = None`, and line 145 calls
`import_tag(None, {},
chart.id, "chart", db.session)`.
7. `import_tag` function (imported from
`superset.commands.importers.v1.utils`) attempts
to iterate over None, raising TypeError and failing the entire asset import
transaction.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/importers/v1/assets.py
**Line:** 141:145
**Comment:**
*Possible Bug: Missing null check for `config["tags"]` before calling
`import_tag`. The schema defines `tags` with `allow_none=True`, meaning the
value can be present in the config but set to `None`. Passing `None` to
`import_tag` instead of a list will likely cause a TypeError or unexpected
behavior when the function tries to iterate over the tags.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38343&comment_hash=5bd53b80469e3bbd442ccb7914122e78ca261746070be4b27e4d75c4a101e1c8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38343&comment_hash=5bd53b80469e3bbd442ccb7914122e78ca261746070be4b27e4d75c4a101e1c8&reaction=dislike'>👎</a>
##########
superset/commands/importers/v1/assets.py:
##########
@@ -154,6 +162,18 @@ def _import(configs: dict[str, Any], sparse: bool = False)
-> None: # noqa: C90
}
dashboard_chart_ids.append(dashboard_chart_id)
+ # Handle tags using import_tag function
+ if feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
+ if "tags" in config:
+ target_tag_names = config["tags"]
+ import_tag(
+ target_tag_names,
+ {},
+ dashboard.id,
+ "dashboard",
+ db.session,
+ )
Review Comment:
**Suggestion:** Missing null check for `config["tags"]` before calling
`import_tag`. The schema allows the `tags` field to be null
(`allow_none=True`), so the code should verify that the value is not None
before passing it to `import_tag`. Current code only checks if the key exists,
which is insufficient when the value is explicitly set to null. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard import fails when tags field is explicitly null.
- ❌ Complete asset import batch aborted due to unhandled TypeError.
- ⚠️ Feature behind TAGGING_SYSTEM flag; affects tagged dashboard workflows.
```
</details>
```suggestion
# Handle tags using import_tag function
if feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
if config.get("tags"):
target_tag_names = config["tags"]
import_tag(
target_tag_names,
{},
dashboard.id,
"dashboard",
db.session,
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable TAGGING_SYSTEM feature flag in Superset configuration environment.
2. Prepare asset import with dashboard configuration (e.g.,
`dashboards/my_dashboard.yaml`) containing `tags: null` (valid per
ImportV1DashboardSchema
line 519 which allows null).
3. Execute import via `ImportAssetsCommand` which invokes `_import()` static
method at
line 128.
4. Dashboard processing loop (lines 148-184) processes the dashboard config.
5. At line 167, check `if "tags" in config:` returns True when tags key
exists with null
value.
6. Line 168 assigns None to `target_tag_names`, lines 169-175 pass None to
`import_tag()`.
7. `import_tag` fails when attempting to iterate over the None value, causing
ImportFailedError and rolling back the import transaction.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/importers/v1/assets.py
**Line:** 165:175
**Comment:**
*Possible Bug: Missing null check for `config["tags"]` before calling
`import_tag`. The schema allows the `tags` field to be null
(`allow_none=True`), so the code should verify that the value is not None
before passing it to `import_tag`. Current code only checks if the key exists,
which is insufficient when the value is explicitly set to null.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38343&comment_hash=a99dd13f557233cb2fbd03dfb345a9427ba292f62a3d3616ae09cca692cb52c9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38343&comment_hash=a99dd13f557233cb2fbd03dfb345a9427ba292f62a3d3616ae09cca692cb52c9&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]
