fix(daos): lazy-import SoftDeleteMixin to fix pytest collection error by aminghadersohi · Pull Request #40573 · apache/superset

aminghadersohi · 2026-06-01T04:18:58Z

SUMMARY

Commit `b2320820b4` (`feat(core): SoftDeleteMixin and restore infrastructure`) added a module-level import in `superset/daos/base.py`:

```python
from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES, SoftDeleteMixin
```

Importing `superset.models.helpers` triggers `superset/models/init.py`, which eagerly imports `superset/models/core.py`. At class-definition time, `core.py` calls `encrypted_field_factory.create(String(1024))`, raising `Exception: App not initialized yet. Please call init_app first` whenever a test file is collected before the Flask app is created.

This broke pytest collection for any test that imports from `superset.daos` before a Flask app context exists, causing CI failures in downstream consumers.

Fix (2 commits):

Lazy import + break the import chain — Move `SoftDeleteMixin` import inside `BaseDAO.delete()` where it is used at runtime (`# pylint: disable=import-outside-toplevel`). `database.py` updated to import `SKIP_VISIBILITY_FILTER_CLASSES` from `superset.daos.base` instead of `superset.models.helpers`.
DRY: move `SKIP_VISIBILITY_FILTER_CLASSES` to `superset/constants.py` — The constant was defined independently in both `superset/models/helpers.py` (line 664) and `superset/daos/base.py` (as a workaround). Canonical home is now `superset/constants.py`. `helpers.py` imports and re-exports it so all existing callers (`views/filters.py`, `commands/importers/v1/utils.py`, `security/manager.py`) remain unchanged.

BEFORE/AFTER

Before: importing `superset.daos.base` or `superset.daos.database` before `create_app()` raises `App not initialized yet`.

After: both modules import cleanly without a Flask app context. Single definition of `SKIP_VISIBILITY_FILTER_CLASSES` in `superset/constants.py`.

TESTING INSTRUCTIONS

`pytest tests/unit_tests/` — collection no longer fails with `App not initialized yet`
Functional smoke: `BaseDAO.delete()` still routes to `soft_delete` for `SoftDeleteMixin` models and `hard_delete` for others
`SKIP_VISIBILITY_FILTER_CLASSES` value is identical (`"_skip_visibility_filter_classes"`) — no behavior change to query execution options

Commit b232082 added a module-level import in superset/daos/base.py: from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES, SoftDeleteMixin Loading superset.models.helpers triggers superset/models/__init__.py which eagerly imports superset/models/core.py. At class-definition time, core.py calls encrypted_field_factory.create(String(1024)), which raises 'App not initialized yet' when imported before create_app() runs. This breaks pytest collection for any test that imports from superset.daos.base before a Flask app context exists. Fix: define SKIP_VISIBILITY_FILTER_CLASSES as a plain string constant in base.py (avoiding the helpers import entirely) and move the SoftDeleteMixin import inside the BaseDAO.delete() method body where it is used at runtime. Same pattern applied to daos/database.py.

codecov · 2026-06-01T04:22:59Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.97%. Comparing base (a621520) to head (6f04479).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #40573   +/-   ##
=======================================
  Coverage   63.97%   63.97%           
=======================================
  Files        2654     2654           
  Lines      142753   142753           
  Branches    32833    32833           
=======================================
  Hits        91325    91325           
  Misses      49870    49870           
  Partials     1558     1558

Flag	Coverage Δ
hive	`39.72% <80.00%> (-0.01%)`	⬇️
mysql	`58.42% <100.00%> (ø)`
postgres	`58.50% <100.00%> (ø)`
presto	`41.33% <80.00%> (-0.01%)`	⬇️
python	`59.99% <100.00%> (ø)`
sqlite	`58.15% <100.00%> (ø)`
unit	`100.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR fixes a pytest collection failure caused by importing superset.models.helpers at module import time from DAO modules, which can indirectly trigger encrypted-field initialization before the Flask app is set up.

Changes:

In BaseDAO, avoids importing superset.models.helpers at module scope by defining SKIP_VISIBILITY_FILTER_CLASSES locally and lazily importing SoftDeleteMixin inside BaseDAO.delete().
In DatabaseDAO, switches SKIP_VISIBILITY_FILTER_CLASSES import to come from superset.daos.base instead of superset.models.helpers.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`superset/daos/base.py`	Removes module-level dependency on `superset.models.helpers` by localizing the execution-options key constant and lazily importing `SoftDeleteMixin` at runtime.
`superset/daos/database.py`	Stops importing the execution-options key from `superset.models.helpers`, reducing eager model-package import side effects during test collection.

bito-code-review · 2026-06-01T04:32:55Z

The PR introduces a local definition of SKIP_VISIBILITY_FILTER_CLASSES in superset/daos/base.py to avoid importing superset.models.helpers at the module level. This change is made to prevent issues with pytest collection. The constant is then used in superset/daos/database.py. Adding an explicit 'keep in sync' note between these two locations would help ensure consistency and prevent future drift that could silently break the visibility-filter bypass.

…ants.py Eliminates split-truth: the constant was defined independently in both `superset/models/helpers.py` and `superset/daos/base.py`. Both modules now import from the single canonical definition in `superset/constants.py`. `helpers.py` re-exports it, so all existing callers (views/filters.py, commands/importers/v1/utils.py, security/manager.py) remain unchanged.

bito-code-review · 2026-06-01T07:33:02Z

Code Review Agent Run #e4f1d9

Actionable Suggestions - 0

Review Details

Files reviewed - 4 · Commit Range: 2eae25c..6f04479
- superset/constants.py
- superset/daos/base.py
- superset/daos/database.py
- superset/models/helpers.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

mikebridge · 2026-06-01T16:48:28Z

Sorry, yes, this looks good. I need to add a check for "can this module be imported in isolation before app init?" in one of the review agents to catch this in the future

richardfogaca

LGTM

…nical location PR apache#40573 moved SKIP_VISIBILITY_FILTER_CLASSES from superset/models/helpers.py to superset/constants.py. superset/models/helpers.py still re-exports it for backwards compat, but the canonical import path is now superset.constants. Updating the three dashboard-branch files that referenced the old path: - tests/integration_tests/dashboards/superset_factory_util.py - tests/integration_tests/dashboards/soft_delete_tests.py - tests/integration_tests/base_tests.py (lazy import inside insert_dashboard) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…l location PR apache#40573 moved SKIP_VISIBILITY_FILTER_CLASSES from superset/models/helpers.py to superset/constants.py (which is where plain string constants belong and which doesn't trigger the eager model-load chain). superset/models/helpers.py still re-exports it for backwards compat, but the canonical import path is now superset.constants. Updating to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…cal location PR apache#40573 moved SKIP_VISIBILITY_FILTER_CLASSES from superset/models/helpers.py to superset/constants.py. superset/models/helpers.py still re-exports it for backwards compat, but the canonical import path is now superset.constants. Updating the four dataset-branch files that referenced the old path: - superset/commands/dataset/importers/v1/utils.py (production code) - tests/integration_tests/dashboard_utils.py - tests/integration_tests/datasets/api_tests.py (three lazy imports inside methods) - tests/integration_tests/datasets/soft_delete_tests.py Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>