fix: Dashboard export with charts from multiple databases - #37120
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| # Initialize seen set if not provided | ||
| if seen is None: | ||
| seen = set() |
There was a problem hiding this comment.
Suggestion: The function accepts a seen parameter but only checks if seen is None: seen = set(); if a caller passes a mutable iterable (e.g. list) the code later expects set operations like add and will raise an AttributeError. Coerce non-set iterables to a set at start to ensure seen supports set semantics. [type error]
Severity Level: Minor
| # Initialize seen set if not provided | |
| if seen is None: | |
| seen = set() | |
| # Initialize/coerce seen set if not provided or if an iterable was passed | |
| if seen is None: | |
| seen = set() | |
| elif not isinstance(seen, set): | |
| # Coerce other iterables (lists, tuples) into a set so callers can pass any iterable | |
| seen = set(seen) |
Why it matters? ⭐
Coercing non-set iterables to a set is sensible defensive programming: the type annotation isn't enforced at runtime and callers might pass a list/tuple. If downstream code expects set semantics (e.g., .add, membership checks), converting here avoids surprising AttributeError. The change is simple and safe.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/export.py
**Line:** 182:184
**Comment:**
*Type Error: The function accepts a `seen` parameter but only checks `if seen is None: seen = set()`; if a caller passes a mutable iterable (e.g. list) the code later expects set operations like `add` and will raise an AttributeError. Coerce non-set iterables to a set at start to ensure `seen` supports set semantics.
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.There was a problem hiding this comment.
seen is typed as set[str] | None and nothing in this codebase ever passes anything else in, so I don't think we need to coerce arbitrary iterables here.
| command.disable_tag_export() | ||
| yield from command.run() | ||
| # Pass the shared seen set to the chart export command | ||
| yield from command.run(seen=seen) | ||
| command.enable_tag_export() |
There was a problem hiding this comment.
Suggestion: The code disables tag export by mutating a class-level flag but does not preserve or restore the original value on error; if command.run(...) raises, enable_tag_export() may never be called leaving the class in a disabled state (affecting subsequent exports). Use a try/finally and restore the original class-level value to ensure the flag is always restored. [logic error]
Severity Level: Minor
| command.disable_tag_export() | |
| yield from command.run() | |
| # Pass the shared seen set to the chart export command | |
| yield from command.run(seen=seen) | |
| command.enable_tag_export() | |
| # Preserve the original class-level flag and restore it even if the run raises | |
| original_include_tags = getattr(ExportChartsCommand, "_include_tags", True) | |
| ExportChartsCommand._include_tags = False | |
| try: | |
| # Pass the shared seen set to the chart export command | |
| yield from command.run(seen=seen) | |
| finally: | |
| ExportChartsCommand._include_tags = original_include_tags |
Why it matters? ⭐
The suggestion correctly identifies a real issue: disable_tag_export mutates a class-level flag and if command.run raises, enable_tag_export won't be called leaving the class in an inconsistent state for subsequent exports. Wrapping the run in a try/finally (or otherwise preserving/restoring the original value) is the correct defensive fix. The improved code restores the original value even on exceptions.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/export.py
**Line:** 195:198
**Comment:**
*Logic Error: The code disables tag export by mutating a class-level flag but does not preserve or restore the original value on error; if `command.run(...)` raises, `enable_tag_export()` may never be called leaving the class in a disabled state (affecting subsequent exports). Use a try/finally and restore the original class-level value to ensure the flag is always restored.
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.There was a problem hiding this comment.
Already fixed, this got wrapped in a try/finally so a failed chart export doesn't leave tag export disabled for later requests.
| # Initialize seen set if not provided | ||
| if seen is None: | ||
| seen = set() | ||
|
|
There was a problem hiding this comment.
Suggestion: The new code initializes a seen set but never adds the database file name to it; as a result, the shared deduplication context won't record that this database has been exported and nested/parallel exporters won't be able to avoid re-exporting the same database file. Add the database file path into seen immediately after initialization so downstream code and nested exporters can observe the exported database. [logic error]
Severity Level: Minor
| # Record this database's export filename in the shared deduplication set | |
| seen.add(ExportDatabasesCommand._file_name(model)) |
Why it matters? ⭐
This suggestion points out a real omission: this exporter yields the database file but never records that filename in the shared seen set. Other exporters that rely on seen to avoid re-exporting the same database will not see that this DB has already been exported. Adding the file name (using the same key this function yields) to seen fixes the deduplication gap and directly addresses the bug described in the PR.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/database/export.py
**Line:** 112:112
**Comment:**
*Logic Error: The new code initializes a `seen` set but never adds the database file name to it; as a result, the shared deduplication context won't record that this database has been exported and nested/parallel exporters won't be able to avoid re-exporting the same database file. Add the database file path into `seen` immediately after initialization so downstream code and nested exporters can observe the exported database.
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.There was a problem hiding this comment.
The add happens a level up. ExportModelsCommand.run() wraps every filename _export() yields and adds it to seen right after yielding, so this file's tracked there regardless of what _export itself does.
| payload["version"] = EXPORT_VERSION | ||
|
|
||
| yield file_path, lambda: yaml.safe_dump(payload, sort_keys=False) | ||
| yield file_path, lambda: yaml.safe_dump(payload, sort_keys=False) |
There was a problem hiding this comment.
Suggestion: The lambda used for the database file's content closes over the mutable local payload variable; if multiple payloads are created in a loop, all yielded lambdas will reference the final payload value at call time—bind payload as a default argument in the lambda to capture its current value. [possible bug]
Severity Level: Critical 🚨
| yield file_path, lambda: yaml.safe_dump(payload, sort_keys=False) | |
| # Bind payload to the lambda default to avoid late-binding closure capturing | |
| yield file_path, (lambda payload=payload: yaml.safe_dump(payload, sort_keys=False)) |
Why it matters? ⭐
The yielded lambda closes over the local payload variable. If multiple payloads are produced
in the same frame (e.g., via looped logic in the future), the lambdas will exhibit late-binding
behavior and may serialize the wrong payload when invoked later. Binding payload as a default
argument in the lambda is a cheap, defensive fix that avoids this class of subtle bugs.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dataset/export.py
**Line:** 139:139
**Comment:**
*Possible Bug: The lambda used for the database file's content closes over the mutable local `payload` variable; if multiple payloads are created in a loop, all yielded lambdas will reference the final `payload` value at call time—bind `payload` as a default argument in the lambda to capture its current value.
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.There was a problem hiding this comment.
This only yields once here, not in a loop, so there's no actual late-binding risk to guard against.
| @@ -51,23 +51,34 @@ | |||
|
|
|||
There was a problem hiding this comment.
Suggestion: Incorrect error message: the _file_content stub raises NotImplementedError telling implementers to "implement _export" even though the method is _file_content; this is misleading and may cause confusion when debugging—raise a message that references the correct method name. [logic error]
Severity Level: Minor
| raise NotImplementedError("Subclasses MUST implement _file_content") |
Why it matters? ⭐
The message is clearly a copy/paste typo and is misleading when _file_content is unimplemented. Fixing it to reference _file_content improves debuggability without changing behavior.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/export/models.py
**Line:** 51:51
**Comment:**
*Logic Error: Incorrect error message: the `_file_content` stub raises NotImplementedError telling implementers to "implement _export" even though the method is `_file_content`; this is misleading and may cause confusion when debugging—raise a message that references the correct method name.
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.There was a problem hiding this comment.
Fixed already, thanks for catching the copy-paste.
| yield ( | ||
| ExportSavedQueriesCommand._file_name(model), |
There was a problem hiding this comment.
Suggestion: The initial query file yielded by the command is never recorded in the shared seen set, so other parts of the export flow can still re-yield the same file and cause duplicates; add a check and record the query filename in seen. [logic error]
Severity Level: Minor
| yield ( | |
| ExportSavedQueriesCommand._file_name(model), | |
| query_file = ExportSavedQueriesCommand._file_name(model) | |
| if query_file not in seen: | |
| seen.add(query_file) | |
| yield ( | |
| query_file, |
Why it matters? ⭐
The command currently yields the query file unconditionally but the shared seen
set is intended to deduplicate exports across nested commands. Not recording the
yielded query filename allows duplicate query files to be produced by other
nested exports. The proposed change prevents duplicates and aligns with the PR's
shared-deduplication approach. It's a targeted, correct fix.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/query/export.py
**Line:** 76:77
**Comment:**
*Logic Error: The initial query file yielded by the command is never recorded in the shared `seen` set, so other parts of the export flow can still re-yield the same file and cause duplicates; add a check and record the query filename in `seen`.
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.There was a problem hiding this comment.
Same as the other "seen not updated" comments on this PR, run()'s wrapping loop already adds every yielded filename to seen after the fact, so nothing's actually missing here.
| if "extra" in payload: | ||
| try: | ||
| payload["extra"] = json.loads(payload["extra"]) | ||
| except json.JSONDecodeError: |
There was a problem hiding this comment.
Suggestion: The code attempts to json.loads the payload["extra"] field without ensuring it's a string/bytes; if extra is already a dict (or another non-string), json.loads will raise a TypeError which is not caught—only JSONDecodeError is caught—so validate the type before calling json.loads or catch TypeError as well. [type error]
Severity Level: Minor
| if "extra" in payload: | |
| try: | |
| payload["extra"] = json.loads(payload["extra"]) | |
| except json.JSONDecodeError: | |
| if "extra" in payload and isinstance(payload["extra"], (str, bytes)): | |
| try: | |
| payload["extra"] = json.loads(payload["extra"]) | |
| except (json.JSONDecodeError, TypeError): |
Why it matters? ⭐
If payload["extra"] is already a dict or another non-string, calling json.loads on it raises a TypeError
which is not caught by the current except block (only JSONDecodeError is handled). Guarding with an
isinstance check or including TypeError in the except tuple prevents an unexpected exception and is
a safe, defensive improvement.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/query/export.py
**Line:** 96:99
**Comment:**
*Type Error: The code attempts to json.loads the `payload["extra"]` field without ensuring it's a string/bytes; if `extra` is already a dict (or another non-string), json.loads will raise a TypeError which is not caught—only JSONDecodeError is caught—so validate the type before calling `json.loads` or catch TypeError as well.
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.There was a problem hiding this comment.
Good catch, fixed! extra can be None for databases that never got that column populated, and json.loads(None) raises TypeError, not JSONDecodeError. Added it to the except tuple, matching the pattern already used elsewhere in the export commands.
|
|
||
| yield ( | ||
| ExportThemesCommand._file_name(model), | ||
| lambda: ExportThemesCommand._file_content(model), |
There was a problem hiding this comment.
Suggestion: The lambda passed as the file content factory closes over model which can lead to late-binding issues if the callable is evaluated later; bind model into the lambda's default arguments to capture its value at definition time. [possible bug]
Severity Level: Critical 🚨
| lambda: ExportThemesCommand._file_content(model), | |
| lambda _m=model: ExportThemesCommand._file_content(_m), |
Why it matters? ⭐
Defensive and harmless improvement: capturing model as a default argument avoids potential late-binding
surprises if the callable is evaluated later or if the function evolves to yield multiple items in a loop.
While in the current implementation it's unlikely to cause a bug (single yield), the change is cheap and safe.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/theme/export.py
**Line:** 78:78
**Comment:**
*Possible Bug: The lambda passed as the file content factory closes over `model` which can lead to late-binding issues if the callable is evaluated later; bind `model` into the lambda's default arguments to capture its value at definition time.
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.There was a problem hiding this comment.
This only yields once, not in a loop, so there's nothing late-binding could actually get wrong here.
| yield from command.run(seen=seen) | ||
| command.enable_tag_export() | ||
| if feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"): | ||
| yield from ExportTagsCommand.export( |
There was a problem hiding this comment.
Suggestion: The tags export is invoked via yield from ExportTagsCommand.export(...) but this call does not participate in the shared seen deduplication; that can cause duplicate tag file exports across nested exports. Check/mark the tags file in the shared seen set and yield the tags file only if it hasn't been seen yet. [possible bug]
Severity Level: Critical 🚨
There was a problem hiding this comment.
Whatever _export() yields, including this yield from, still passes through run()'s wrapping loop, which dedupes on filename. This is covered.
|
CodeAnt AI finished reviewing your PR. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #159203
Actionable Suggestions - 1
-
superset/commands/database/export.py - 1
- Dead code in parameter initialization · Line 109-111
Additional Suggestions - 1
-
superset/commands/query/export.py - 1
-
Type hint consistency · Line 70-70The seen parameter uses modern union syntax (set[str] | None), which is fine for Python 3.10+, but consider Optional[set[str]] if targeting broader compatibility.
-
Review Details
-
Files reviewed - 8 · Commit Range:
607ef88..607ef88- superset/commands/chart/export.py
- superset/commands/dashboard/export.py
- superset/commands/database/export.py
- superset/commands/dataset/export.py
- superset/commands/export/models.py
- superset/commands/query/export.py
- superset/commands/theme/export.py
- tests/integration_tests/dashboards/commands_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| # Initialize seen set if not provided | ||
| if seen is None: | ||
| seen = set() |
There was a problem hiding this comment.
The 'seen' parameter initialization is dead code because ExportModelsCommand.run always passes a 'seen' set to _export, making the None check unreachable. This could confuse maintainers and should be removed for clarity.
Code Review Run #159203
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Not dead code, _export gets called directly (bypassing run()) in a few unit tests without passing seen, so the None-check is actually exercised.
There was a problem hiding this comment.
The suggestion to remove the seen parameter initialization is based on the assumption that run() always provides a seen set. Since _export is also called directly in unit tests without providing this argument, the None-check is necessary to prevent errors in those test scenarios. Therefore, the current implementation is correct and should be maintained.
superset/commands/database/export.py
@staticmethod
def _export(
model: Database, export_related: bool = True, seen: set[str] | None = None
) -> Iterator[tuple[str, Callable[[], str]]]:
# Initialize seen set if not provided
if seen is None:
seen = set()
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #37113 where exporting dashboards containing charts from multiple databases would fail to include all charts and their corresponding database YAML files in the exported ZIP.
Changes:
- Implemented shared deduplication context (
seenset) passed through the entire export hierarchy - Modified base
ExportModelsCommand.run()to accept and propagate an optionalseenparameter - Updated all export command
_export()methods to utilize the sharedseenset for proper deduplication
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration_tests/dashboards/commands_tests.py | Added comprehensive test case verifying cross-database chart exports work correctly |
| superset/commands/export/models.py | Updated base command to support shared deduplication context with backward compatibility |
| superset/commands/dashboard/export.py | Passes shared seen set to nested chart, theme, and dataset export commands |
| superset/commands/chart/export.py | Propagates shared seen set to dataset export commands |
| superset/commands/dataset/export.py | Critical fix - only exports database files if not already in shared seen set |
| superset/commands/database/export.py | Updated to accept shared seen parameter for consistency |
| superset/commands/query/export.py | Prevents duplicate database exports using shared seen set |
| superset/commands/theme/export.py | Updated to accept shared seen parameter for consistency |
Comments suppressed due to low confidence (1)
superset/commands/dashboard/export.py:202
- The ExportTagsCommand.export() call doesn't utilize the shared
seenset, which could lead to duplicatetags.yamlfiles if tags are exported multiple times in a nested export scenario. Consider passing and checking theseenset in ExportTagsCommand.export() to prevent duplicate tag file generation, similar to how other export commands were updated.
yield from ExportTagsCommand.export(
dashboard_ids=dashboard_ids, chart_ids=chart_ids
)
| payload["version"] = EXPORT_VERSION | ||
|
|
||
| yield file_path, lambda: yaml.safe_dump(payload, sort_keys=False) | ||
| yield file_path, lambda: yaml.safe_dump(payload, sort_keys=False) |
There was a problem hiding this comment.
The lambda function may capture the wrong value of payload due to late binding in Python closures. If multiple database files are yielded in a loop, all lambdas would reference the final value of payload from the last iteration. Consider using a default argument to capture the current value, similar to: lambda p=payload: yaml.safe_dump(p, sort_keys=False)
| yield file_path, lambda: yaml.safe_dump(payload, sort_keys=False) | |
| yield file_path, lambda p=payload: yaml.safe_dump(p, sort_keys=False) |
There was a problem hiding this comment.
Same as the CodeAnt comment above on this file, it's a single yield outside a loop so there's no late-binding bug here.
| file_content = yaml.safe_dump(payload, sort_keys=False) | ||
| yield file_name, lambda: file_content | ||
| file_content = yaml.safe_dump(payload, sort_keys=False) | ||
| yield file_name, lambda: file_content |
There was a problem hiding this comment.
The lambda function may capture the wrong value of file_content due to late binding in Python closures. If multiple database files are yielded in a loop, all lambdas would reference the final value of file_content from the last iteration. Consider using a default argument to capture the current value, similar to: lambda fc=file_content: fc
There was a problem hiding this comment.
Same story, single yield outside a loop, so file_content can't get stomped by a later iteration.
| # Clean up | ||
| example_dashboard.slices.remove(chart_from_second_db) | ||
| db.session.delete(chart_from_second_db) | ||
| db.session.delete(second_dataset) | ||
| db.session.delete(second_db) | ||
| db.session.commit() |
There was a problem hiding this comment.
The test cleanup is not exception-safe. If an assertion fails before reaching the cleanup section, the test database objects (second_db, second_dataset, chart_from_second_db) will remain in the database, potentially affecting other tests. Consider using a try-finally block or pytest fixtures with yield to ensure cleanup happens even when assertions fail.
There was a problem hiding this comment.
Fixed, the whole test body is wrapped in try/finally now so a failed assertion still tears down the extra Database/SqlaTable/Slice rows.
| assert chart_from_second_db_file is not None, \ | ||
| f"Chart from second database not found in export. Chart files: {chart_files}" |
There was a problem hiding this comment.
The assertion message formatting uses f-string with a variable that may not exist at runtime. If 'Chart_from_Second_Database_' is not found in any chart file, chart_from_second_db_file will be None when used in the assertion message. While the assertion will still work, consider moving the detailed error message construction before the assertion to avoid any potential runtime issues.
| assert chart_from_second_db_file is not None, \ | |
| f"Chart from second database not found in export. Chart files: {chart_files}" | |
| error_msg = ( | |
| "Chart from second database not found in export. " | |
| f"Chart files: {chart_files}" | |
| ) | |
| assert chart_from_second_db_file is not None, error_msg |
There was a problem hiding this comment.
This got cleaned up along with the rest of the test, the message doesn't reference the maybe-None variable anymore.
| should_add_metadata = True | ||
| else: | ||
| # If seen set is provided, we're being called from another command | ||
| should_add_metadata = False | ||
|
|
||
| # Only add metadata if this is the root command | ||
| if should_add_metadata: | ||
| metadata = { | ||
| "version": EXPORT_VERSION, | ||
| "type": self.dao.model_cls.__name__, # type: ignore | ||
| "timestamp": datetime.now(tz=timezone.utc).isoformat(), | ||
| } | ||
| if METADATA_FILE_NAME not in seen: | ||
| yield METADATA_FILE_NAME, lambda: yaml.safe_dump(metadata, sort_keys=False) | ||
| seen.add(METADATA_FILE_NAME) |
There was a problem hiding this comment.
The logic for determining when to add metadata may cause issues in nested export scenarios. When a non-root command creates its own seen set (when seen is None), it will set should_add_metadata = True and could potentially yield duplicate metadata files. Consider checking if METADATA_FILE_NAME is already in the seen set before deciding whether to add metadata, rather than relying solely on whether seen was passed in.
| should_add_metadata = True | |
| else: | |
| # If seen set is provided, we're being called from another command | |
| should_add_metadata = False | |
| # Only add metadata if this is the root command | |
| if should_add_metadata: | |
| metadata = { | |
| "version": EXPORT_VERSION, | |
| "type": self.dao.model_cls.__name__, # type: ignore | |
| "timestamp": datetime.now(tz=timezone.utc).isoformat(), | |
| } | |
| if METADATA_FILE_NAME not in seen: | |
| yield METADATA_FILE_NAME, lambda: yaml.safe_dump(metadata, sort_keys=False) | |
| seen.add(METADATA_FILE_NAME) | |
| # Only add metadata if it hasn't been added yet in this export graph | |
| should_add_metadata = METADATA_FILE_NAME not in seen | |
| if should_add_metadata: | |
| metadata = { | |
| "version": EXPORT_VERSION, | |
| "type": self.dao.model_cls.__name__, # type: ignore | |
| "timestamp": datetime.now(tz=timezone.utc).isoformat(), | |
| } | |
| yield METADATA_FILE_NAME, lambda: yaml.safe_dump(metadata, sort_keys=False) | |
| seen.add(METADATA_FILE_NAME) |
There was a problem hiding this comment.
The one place this could actually bite is ExportAssetsCommand, which calls each command's .run() without threading a shared seen between them. But its own wrapping loop dedupes on filename before anything reaches the output, so no duplicate metadata file actually escapes.
| # Initialize seen set if not provided (for consistency) | ||
| if seen is None: | ||
| seen = set() | ||
|
|
There was a problem hiding this comment.
Variable seen is not used.
| # Initialize seen set if not provided (for consistency) | |
| if seen is None: | |
| seen = set() |
There was a problem hiding this comment.
It's not really unused, seen has to stay in the signature since run() calls every subclass's _export(model, export_related, seen) the same way. This one just doesn't have anything to dedupe against.
| ) -> Iterator[tuple[str, Callable[[], str]]]: | ||
| # Initialize seen set if not provided | ||
| if seen is None: | ||
| seen = set() |
There was a problem hiding this comment.
Variable seen is not used.
| seen = set() | |
| seen = set() | |
| # Mark `seen` as intentionally used to satisfy static analysis | |
| _ = seen |
There was a problem hiding this comment.
Same as the theme one, it's kept for parity with run()'s call signature. Not using it to skip re-computing an already-seen file is a missed micro-optimization at worst, run() still dedupes the output either way.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #37120 +/- ##
==========================================
- Coverage 79.10% 79.10% -0.01%
==========================================
Files 2878 2878
Lines 165651 165672 +21
Branches 38300 38311 +11
==========================================
+ Hits 131039 131052 +13
- Misses 32126 32129 +3
- Partials 2486 2491 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Lots of bot suggestions and CI issues to untangle here. Also a recent PR merged that shouldn't affect this, but I'm curious if you have this issue on master still. |
|
Hi @rusackas! Thanks for reviewing. I just checked and yes, the issue still exists on master. The recent PR you mentioned doesn't seem to have addressed this specific case where dashboards with charts from multiple databases don't export all database YAML files correctly. Regarding the bot suggestions and CI issues - I'll take a look and address them. Could you point me to the specific ones that need attention? The codecov report shows low patch coverage since the export commands don't have many existing tests, but I'm happy to add tests if that would help. Thanks! |
|
Adding tests would help, for sure! Always welcomed :D I haven't looked through the bot comments in any great detail, but they're usually pretty legit. Last but not least, pre-commit and other CI checks will need to pass before we can merge it. Thanks for seeing this through! |
|
@Milad93R — friendly ping on this one. It's been a couple of months and a few things still need your attention before we can merge: CI (needs to be green):
Substantive bot findings worth addressing (I looked and these ones are legit, not noise):
The other bot comments are mostly minor (defensive-coding nits, dead-code cleanup) — your call on those. Let me know once those are addressed and I'll take another look. Thanks for seeing this through! |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks for digging in here @Milad93R!
Couple of things before we can take this further. CI's red across the board — pre-commit plus some test jobs. The pre-commit failures look like a handful of lines now over 88 chars. Worth running pre-commit run --all-files locally to clear those first.
The part I'm stuck on though: the top-level run() already dedups every yielded filename via its own seen, so isolated sets would cause duplicate files, not missing ones. I can't quite see how the old code drops a chart/db that this fixes, unless two databases collide on databases/<name>.yaml (we key those with skip_id=True).
Can you point at the exact line where a file goes missing on master? Want to make sure we're fixing the real mechanism rather than reshuffling dedup.
Also seen gets added to database/export.py and theme/export.py but never actually used there (Bito flagged the same), and the query/export.py change seems unrelated to this issue — I'd lean toward keeping the diff to the minimum that fixes #37113. Thoughts?
|
@Milad93R This PR hasn't really moved since January and a few things from my last pass are still open. CI's red on this PR's own code. Still not sure of the mechanism though: Also |
607ef88 to
71e8273
Compare
| # Pass the shared seen set to the dataset export command | ||
| yield from ExportDatasetsCommand([dataset_id]).run( | ||
| seen=seen | ||
| ) |
There was a problem hiding this comment.
Suggestion: Before exporting each referenced dataset, enforce an authorization check with security_manager.raise_for_access(datasource=dataset) so dataset content is only exported when the caller is permitted. [custom_rule_security]
Severity Level: Critical 🚨
Why it matters? ⭐
Datasets are data-bearing resources covered by the custom rule, and this code exports them without calling security_manager.raise_for_access(...). The absence of that authorization check is a real rule violation.
Rule source 📖
=== .github/copilot-instructions.md === (line 79)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/export.py
**Line:** 439:442
**Comment:**
*Custom Rule Security: Before exporting each referenced dataset, enforce an authorization check with `security_manager.raise_for_access(datasource=dataset)` so dataset content is only exported when the caller is permitted.
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 fixThere was a problem hiding this comment.
This loop predates this PR, the diff here only threads seen through it. I'd rather track a dataset-export authorization pass as its own issue than fold it into a cross-database export fix.
| # Pass the shared seen set to the dataset export command | ||
| yield from ExportDatasetsCommand([dataset_id]).run( | ||
| seen=seen | ||
| ) |
There was a problem hiding this comment.
Suggestion: Apply the same dataset authorization enforcement in this second dataset-export path by calling security_manager.raise_for_access(datasource=dataset) before running the export command. [custom_rule_security]
Severity Level: Critical 🚨
Why it matters? ⭐
This second dataset-export branch has the same issue: it exports a dataset without first enforcing access via security_manager.raise_for_access(...). Since datasets are explicitly in scope for the rule, the violation is real here as well.
Rule source 📖
=== .github/copilot-instructions.md === (line 79)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/export.py
**Line:** 453:456
**Comment:**
*Custom Rule Security: Apply the same dataset authorization enforcement in this second dataset-export path by calling `security_manager.raise_for_access(datasource=dataset)` before running the export command.
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 fixThere was a problem hiding this comment.
Same as the comment above, pre-existing and out of scope for this PR.
| db.session.add(second_dataset) | ||
|
|
||
| # Create a chart using the second database's dataset | ||
| chart_from_second_db = Slice( | ||
| slice_name="Chart from Second Database", | ||
| datasource_type="table", | ||
| datasource_id=second_dataset.id, | ||
| datasource_name=second_dataset.table_name, | ||
| viz_type="bar", | ||
| params=json.dumps({"viz_type": "bar"}), | ||
| ) |
There was a problem hiding this comment.
Suggestion: The chart is created with datasource_id taken from second_dataset.id before the dataset is flushed, so this value is still None at construction time and the chart is not linked to the new dataset. That causes export logic to skip the second dataset/database chain and makes the test assert the wrong behavior. Flush the session before creating the chart (or set the chart’s table relationship directly) so the datasource reference is valid. [incorrect variable usage]
Severity Level: Critical 🚨
- ❌ Cross-database dashboard export test never links chart dataset.
- ❌ Second database’s dataset not exported via chart relationship.
- ⚠️ Multi-database dashboard regression coverage remains incomplete.Steps of Reproduction ✅
1. Run the test `test_export_dashboard_cross_database_charts` in
`tests/integration_tests/dashboards/commands_tests.py` (around lines 37–124 in the current
file). Inside this test, a second dataset is created and added to the session at lines
52–59 (PR hunk lines 552–558): `second_dataset = SqlaTable(...);
db.session.add(second_dataset)` without any intervening `db.session.flush()` or
`db.session.commit()`, so `second_dataset.id` remains `None` at this point.
2. Observe that immediately after adding the dataset, the chart is instantiated at PR hunk
lines 561–568 with `datasource_id=second_dataset.id` (see same file, lines 61–69 in the
current version): `chart_from_second_db = Slice(..., datasource_id=second_dataset.id,
...)`. Because the dataset has not yet been flushed, `second_dataset.id` is still `None`,
so the new `Slice` object is constructed with `datasource_id=None`.
3. Inspect the Slice model in `superset/models/slice.py` (lines 53–59 and 97–105):
`datasource_id = Column(Integer)` and the `table` relationship is defined with
`foreign_keys=[datasource_id]` and a `primaryjoin` on `Slice.datasource_id ==
SqlaTable.id` when `datasource_type == 'table'`. Since `datasource_id` was persisted as
`NULL` for `chart_from_second_db`, `chart_from_second_db.table` will be `None` even after
`db.session.commit()` at PR hunk line 582.
4. Follow the export path: `ExportDashboardsCommand._export` in
`superset/commands/dashboard/export.py` (lines 133–139) collects `chart_ids = [chart.id
for chart in model.slices]` and calls `ExportChartsCommand(chart_ids).run(seen=seen)`. In
`superset/commands/chart/export.py` (lines 95–110), `ExportChartsCommand._export` checks
`if model.table and export_related:` and only then calls
`ExportDatasetsCommand([model.table.id]).run(seen=seen)`. Because
`chart_from_second_db.table` is `None` due to the `datasource_id=None` bug, the dataset
and database for the second DB are never exported, so the assertions in the test at PR
hunk lines 588–621 that expect the second database and dataset YAML files to be present
will fail to reflect the intended “chart linked to dataset” scenario.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/dashboards/commands_tests.py
**Line:** 558:568
**Comment:**
*Incorrect Variable Usage: The chart is created with `datasource_id` taken from `second_dataset.id` before the dataset is flushed, so this value is still `None` at construction time and the chart is not linked to the new dataset. That causes export logic to skip the second dataset/database chain and makes the test assert the wrong behavior. Flush the session before creating the chart (or set the chart’s table relationship directly) so the datasource reference is valid.
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 fixThere was a problem hiding this comment.
Good catch, fixed! Added a db.session.flush() before the chart's created so datasource_id isn't None at construction time.
| # Clean up | ||
| example_dashboard.slices.remove(chart_from_second_db) | ||
| db.session.delete(chart_from_second_db) | ||
| db.session.delete(second_dataset) | ||
| db.session.delete(second_db) | ||
| db.session.commit() |
There was a problem hiding this comment.
Suggestion: The cleanup is only executed at the end of the happy path, so any assertion failure or exception before that point leaves the temporary dashboard/chart/dataset/database records in the test DB and can make later tests flaky. Move cleanup into a finally block to guarantee teardown on all paths. [missing cleanup]
Severity Level: Major ⚠️
- ⚠️ Failing test can leave extra Database and Slice rows.
- ⚠️ Subsequent integration tests run against polluted database state.
- ⚠️ Debugging regressions complicated by residual test artifacts.Steps of Reproduction ✅
1. In `tests/integration_tests/dashboards/commands_tests.py`, inspect
`test_export_dashboard_cross_database_charts` (lines 37–124 of the current file). The test
creates a temporary `Database` (`second_db`), `SqlaTable` (`second_dataset`), and `Slice`
(`chart_from_second_db`), then appends the chart to the existing `example_dashboard` and
commits (PR hunk lines 548–582).
2. Note that cleanup is implemented only at the end of the test body (PR hunk lines
623–628): `example_dashboard.slices.remove(chart_from_second_db);
db.session.delete(chart_from_second_db); db.session.delete(second_dataset);
db.session.delete(second_db); db.session.commit()`. There is no `try`/`finally` around the
assertions at PR hunk lines 588–621, so any assertion failure (for example, if the
expected dataset file key is missing) raises `AssertionError` and prevents the cleanup
block from running.
3. Confirm that the global integration test infrastructure does not roll back database
state between tests: `SupersetTestCase.tearDown` in
`tests/integration_tests/base_tests.py` (lines 23–25) only calls `self.logout()` and does
not touch the database, and the session-scoped `setup_sample_data` fixture in
`tests/integration_tests/conftest.py` (lines 120–147) only drops tables once at the end of
the entire test session, not per test. This means any `second_db`, `second_dataset`, and
`chart_from_second_db` left behind by a failing test persist into later tests.
4. Run the test suite and introduce a temporary failure in
`test_export_dashboard_cross_database_charts` (for example by modifying one of the export
assertions at PR hunk lines 588–621 to fail). Observe that after the failure, subsequent
tests in the same session still see the additional `Database` named `"test_db_2"` and its
dataset/chart (since no rollback or cleanup ran), providing a concrete path for cross-test
contamination if other tests assume only the example database and its original dashboard
content exist.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/dashboards/commands_tests.py
**Line:** 623:628
**Comment:**
*Missing Cleanup: The cleanup is only executed at the end of the happy path, so any assertion failure or exception before that point leaves the temporary dashboard/chart/dataset/database records in the test DB and can make later tests flaky. Move cleanup into a `finally` block to guarantee teardown on all paths.
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 fixThere was a problem hiding this comment.
Same as the other cleanup comment on this test, it's wrapped in try/finally now.
There was a problem hiding this comment.
Code Review Agent Run #7add1b
Actionable Suggestions - 1
-
superset/commands/dataset/export.py - 1
- Missing seen-set update · Line 114-114
Additional Suggestions - 2
-
tests/integration_tests/dashboards/commands_tests.py - 1
-
Potential unsaved foreign key value · Line 592-592The `second_db.id` may not be assigned until after `db.session.flush()` or `db.session.commit()`. Without flushing, `database_id=second_db.id` at line 555 could be `None`, causing the dataset's foreign key to be unset. This wouldn't affect the export test's success since `model.database` relationship is set, but it violates data integrity expectations.
-
-
superset/commands/query/export.py - 1
-
Missing allow_unicode parameter · Line 106-106The `yaml.safe_dump` call on line 106 is missing `allow_unicode=True`, which is present in all other export modules (chart, dashboard, database, dataset). This inconsistency may cause YAML serialization issues with non-ASCII characters in exported database configs.
-
Review Details
-
Files reviewed - 8 · Commit Range:
71e8273..71e8273- superset/commands/chart/export.py
- superset/commands/dashboard/export.py
- superset/commands/database/export.py
- superset/commands/dataset/export.py
- superset/commands/export/models.py
- superset/commands/query/export.py
- superset/commands/theme/export.py
- tests/integration_tests/dashboards/commands_tests.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| # critical to fix the issue where databases were being | ||
| # duplicated and potentially overwritten when charts from | ||
| # different databases were exported. | ||
| if file_path not in seen: |
There was a problem hiding this comment.
After yielding the database file, file_path must be added to the seen set to prevent duplicate yields. Currently the if file_path not in seen: check will always pass on subsequent calls, causing the same database file to be yielded multiple times instead of being deduplicated.
Code Review Run #7add1b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Same as the other "seen not updated" findings here, run()'s wrapping loop is the one actually doing the dedupe, so this is already handled a level up.
There was a problem hiding this comment.
The reviewer's suggestion to update the seen set is correct. Even if the deduplication logic is handled in the run() loop, updating the seen set within this function ensures that the state remains consistent and prevents redundant processing or potential issues if this function is called independently or refactored in the future.
superset/commands/dataset/export.py
# Only yield the database file if not already seen. This is
# critical to fix the issue where databases were being
# duplicated and potentially overwritten when charts from
# different databases were exported.
if file_path not in seen:
seen.add(file_path)
yield file_path
|
Thanks for sticking with this, @Milad93R. The new Also, a few things from my last pass are still open in the current diff: |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #dba924Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
That |
github-actions
Bot
added
the
requires:rebase
|
@Milad93R heya, checking back in. This is still showing conflicts with |
This fixes issue apache#37113 where exporting dashboards with charts from different databases would result in missing charts and database files. The root cause was that each export command (Dashboard, Chart, Dataset) maintained its own isolated deduplication 'seen' set. When nested commands were called, duplicates would be incorrectly filtered out because they weren't aware of files already exported by other commands. Solution: - Modified ExportModelsCommand.run() to accept an optional shared 'seen' set - Updated all export commands to pass the shared set through the hierarchy - Ensures proper deduplication across all nested export operations - Added comprehensive test case to verify cross-database chart exports Changes: - Updated base ExportModelsCommand to support shared deduplication - Modified dashboard, chart, dataset, database, theme, and query export commands - Added test_export_dashboard_cross_database_charts to verify the fix - Maintained backward compatibility with existing exports Testing: - Added new test case that reproduces the issue and verifies the fix - Syntax validation passes for all modified files - No breaking changes to existing export functionality Fixes: apache#37113
Restore ExportChartsCommand._include_tags via try/finally so a failed chart export doesn't leave tag export disabled for later requests, fix a copy/paste NotImplementedError message in the base export command, and fix the cross-database export test so the second dataset's id is flushed before the chart references it and so a failed assertion still cleans up the temporary Database/SqlaTable/Slice rows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`payload["extra"]` can be `None` for databases whose `extra` column was never populated, and `json.loads(None)` raises `TypeError`, not `JSONDecodeError`. Catch both, matching the pattern already used elsewhere in the export commands (dashboard/export.py, theme/export.py). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ov-compatible after rebase Rebasing this PR onto master (which independently added a `run()` override on `ExportChartsCommand` for tag export, apache#42339) surfaced a signature mismatch: `ExportModelsCommand.run()` gained a `seen` param in this PR, but the chart/tag overrides didn't accept it, so mypy flagged both as incompatible with their supertype and the dashboard export command's `command.run(seen=seen)` call as invalid. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e36cc7f to
55647a8
Compare
Code Review Agent Run #c752a6Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
removed
the
requires:rebase
Co-authored-by: rusackas <evan@rusackas.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
This PR fixes issue #37113 where exporting dashboards containing charts from different databases would result in missing charts and their corresponding database YAML files in the exported ZIP.
Problem
When a dashboard contains charts from multiple databases, the export command fails to include all charts and database files. This happens because:
ExportDashboardsCommand,ExportChartsCommand,ExportDatasetsCommand) maintains its own isolatedseenset for deduplicationyield from command.run(), they create newseensetsSolution
Implemented a shared deduplication context that is passed through the entire export hierarchy:
ExportModelsCommand.run()to accept an optionalseenparameterseenset to nested commandsseensetChanges
Core Changes
superset/commands/export/models.py: Base command now supports shared deduplicationsuperset/commands/dashboard/export.py: Passes shared context to nested exportssuperset/commands/chart/export.py: Uses shared context for dataset exportssuperset/commands/dataset/export.py: Critical fix - only exports database if not seensuperset/commands/database/export.py: Supports shared contextsuperset/commands/theme/export.py: Updated for consistencysuperset/commands/query/export.py: Prevents duplicate database exportsTesting
test_export_dashboard_cross_database_chartstest case that:Backward Compatibility
seenset is provided, commands create their own (existing behavior)Testing Instructions
Related Issue
Fixes #37113
Pre-submission Checklist
Notes
This fix ensures that dashboard exports are complete and reliable, especially in environments where dashboards aggregate data from multiple sources. The shared deduplication context prevents file loss while maintaining proper deduplication.