EnxDev commented on PR #43017:
URL: https://github.com/apache/superset/pull/43017#issuecomment-5339757729
## EnxDev's Review Agent β apache/superset#43017 Β· HEAD a44bae3
**comment** β behavior-preserving perf refactor; the file set and UUID
substitution are unchanged for all realistic inputs. Two robustness/test gaps
below.
Audited as a refactor: walked both `_file_content` and `_export` against
`origin/master` for observable differences. The per-target
`ExportDatasetsCommand([id]).run()` duplicates were already discarded by the
`seen` filename dedup in `ExportModelsCommand.run()`, so the emitted bundle is
identical β only the ordering of `datasets/*` and `databases/*` entries inside
the zip changes, which the importer doesn't depend on. CI is green across
`unit-tests`, `test-postgres`/`test-mysql`/`test-sqlite`, `pre-commit`, and
`codecov/patch`.
### π‘ Should-fix
- **`superset/commands/dashboard/export.py:327`** β `datasets_by_id` is
keyed by `dataset.id` (int), but every lookup uses the raw value from
`json_metadata`. `find_by_id(dataset_id)` compared in SQL, so a string
`datasetId` (`"5"`) resolved on MySQL/SQLite through type coercion;
`datasets_by_id.get("5")` misses, so `datasetUuid` is silently dropped and the
display-control path logs a bogus "missing dataset" warning. The same input on
Postgres is worse: `find_by_ids` wraps `SQLAlchemyError` into
`DAOFindFailedError` and raises, where `find_by_id` caught `StatementError` and
returned `None`, so a single malformed id now aborts the whole dashboard export
instead of skipping one target. Not hypothetical typing β
`superset/tasks/native_filter_cache.py:53` declares this field `int | str`.
Normalize on collection (coerce to `int`, skipping values that don't coerce) so
both sides of the map agree.
- **`superset/commands/dashboard/export.py:462-467`** β the `find_by_ids`
pre-filter here is load-bearing and untested. `ExportModelsCommand.validate()`
raises `DatasetNotFoundError` when `len(self._models) != len(self.model_ids)`,
so passing `referenced_dataset_ids` straight through would make any dashboard
with a dangling filter dataset reference fail its entire export.
`test_export_batches_dataset_export_across_targets` returns both datasets, so
nothing guards it. **regression test:** metadata referencing ids `{1, 2, 3}`
with `find_by_ids` returning only 1 and 2 β assert `ExportDatasetsCommand` is
constructed with exactly `{1, 2}` and the export completes without raising. A
one-line comment saying why the lookup isn't redundant with `validate()`'s own
`find_by_ids` would stop the next reader from "simplifying" it away.
### π΅ Nits
- `superset/commands/dashboard/export.py:312-355` β `payload.get("metadata",
{})` now appears four times in `_file_content`. Binding it once at the top also
removes the `AttributeError` when `json_metadata` decodes to `null` (the key
exists with value `None`, so the default never applies).
- The codecov patch figure of 36% comes from the carried-forward integration
flags β the `unit` flag reports `<ΓΈ>` for these lines. The new unit tests do
cover them; worth noting so the red badge doesn't stall the PR.
### π Praise
- `superset/commands/dashboard/export.py:462` β resolving ids first and
passing only the found ones preserves the skip-dangling-reference semantics
that a naive batch would have broken into a hard `DatasetNotFoundError`. Easy
detail to miss.
- `tests/unit_tests/commands/dashboard/export_test.py` β asserting
`find_by_id.assert_not_called()` alongside the UUID substitutions makes these
real N+1 guards rather than smoke tests, and the overlapping-id fixtures
exercise the dedup that motivates the change.
<!-- enxdev-review-agent:a44bae3 -->
_Reviewed by EnxDev's Review Agent β @EnxDev Β· HEAD a44bae3._
--
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]
