Skip to content

Commit

Permalink
chore: module scope should not require the app context
Browse files Browse the repository at this point in the history
This is a major issue that's been plaguing the backend for a long time. Currently you can't simply run a simple `from superset import models` without getting an error about the app context missing.

This DRAFT PR tries to evaluate what's getting in the way. So far I've identified:

- app.config being referenced in module scope, this is mostly easy to fix for common configuration, where we can rely on flask.current_app and avoid module scope
- dynamic/configurable model: this seems to be the core issue, where say we let people configure their EncryptedField's type
- some reliance on SecurityManager, and SecurityManager.user_model used as a relationship, I think this can be worked around using sqlalchemy's `relationship("use-a-string-reference")` call
- ???
  • Loading branch information
mistercrunch committed May 13, 2024
1 parent 356a58d commit e8b23de
Show file tree
Hide file tree
Showing 356 changed files with 1,098 additions and 1,203 deletions.
1 change: 1 addition & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
docker/**/*.sh text eol=lf
docs/static/img/erd.svg -diff
2 changes: 1 addition & 1 deletion docs/docs/contributing/development.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ gunicorn "superset.app:create_app()" -k "geventwebsocket.gunicorn.workers.Gevent
You can log anything to the browser console, including objects:

```python
from superset import app
from flask import current_app as app
app.logger.error('An exception occurred!')
app.logger.info(form_data)
```
Expand Down
2 changes: 1 addition & 1 deletion scripts/benchmark_migration.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
from sqlalchemy import create_engine, inspect
from sqlalchemy.ext.automap import automap_base

from superset import db
from superset.extensions import db
from superset.utils.mock_data import add_sample_rows

logger = logging.getLogger(__name__)
Expand Down
2 changes: 1 addition & 1 deletion scripts/erd/erd.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import click
import jinja2

from superset import db
from superset.extensions import db

GROUPINGS: dict[str, Iterable[str]] = {
"Core": [
Expand Down
26 changes: 17 additions & 9 deletions superset/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,13 @@
# under the License.
"""Package's main module!"""

from flask import current_app, Flask
from werkzeug.local import LocalProxy

from superset.app import create_app # noqa: F401
from superset.extensions import (
appbuilder, # noqa: F401
cache_manager,
db, # noqa: F401
event_logger, # noqa: F401
feature_flag_manager,
feature_flag_manager, # noqa: F401
manifest_processor,
results_backend_manager,
security_manager, # noqa: F401
Expand All @@ -37,15 +34,26 @@
# to declare "global" dependencies is to define it in extensions.py,
# then initialize it in app.create_app(). These fields will be removed
# in subsequent PRs as things are migrated towards the factory pattern
app: Flask = current_app
cache = cache_manager.cache
conf = LocalProxy(lambda: current_app.config)
get_feature_flags = feature_flag_manager.get_feature_flags
data_cache = LocalProxy(lambda: cache_manager.data_cache)
get_manifest_files = manifest_processor.get_manifest_files
is_feature_enabled = feature_flag_manager.is_feature_enabled
results_backend = LocalProxy(lambda: results_backend_manager.results_backend)
results_backend_use_msgpack = LocalProxy(
lambda: results_backend_manager.should_use_msgpack
)
data_cache = LocalProxy(lambda: cache_manager.data_cache)
thumbnail_cache = LocalProxy(lambda: cache_manager.thumbnail_cache)

__all__ = [
"appbuilder",
"cache",
"cache_manager",
"data_cache",
"event_logger",
"feature_flag_manager",
"get_manifest_files",
"manifest_processor",
"results_backend",
"results_backend_manager",
"results_backend_use_msgpack",
"thumbnail_cache",
]
7 changes: 2 additions & 5 deletions superset/advanced_data_type/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,6 @@
from superset.extensions import event_logger
from superset.views.base_api import BaseSupersetApi

config = app.config
ADVANCED_DATA_TYPES = config["ADVANCED_DATA_TYPES"]


class AdvancedDataTypeRestApi(BaseSupersetApi):
"""
Expand Down Expand Up @@ -96,7 +93,7 @@ def get(self, **kwargs: Any) -> Response:
item = kwargs["rison"]
advanced_data_type = item["type"]
values = item["values"]
addon = ADVANCED_DATA_TYPES.get(advanced_data_type)
addon = app.config["ADVANCED_DATA_TYPES"].get(advanced_data_type)
if not addon:
return self.response(
400,
Expand Down Expand Up @@ -148,4 +145,4 @@ def get_types(self) -> Response:
500:
$ref: '#/components/responses/500'
"""
return self.response(200, result=list(ADVANCED_DATA_TYPES.keys()))
return self.response(200, result=list(app.config["ADVANCED_DATA_TYPES"].keys()))
17 changes: 10 additions & 7 deletions superset/async_events/async_query_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

import jwt
import redis
from flask import Flask, Request, request, Response, session
from flask import current_app, Flask, Request, request, Response, session

from superset.utils.core import get_user_id

Expand Down Expand Up @@ -121,8 +121,7 @@ def init_app(self, app: Flask) -> None:
if config["GLOBAL_ASYNC_QUERIES_REGISTER_REQUEST_HANDLERS"]:
self.register_request_handlers(app)

# pylint: disable=import-outside-toplevel
from superset.tasks.async_queries import (
from superset.tasks.async_queries import ( # pylint: disable=import-outside-toplevel
load_chart_data_into_cache,
load_explore_json_into_cache,
)
Expand Down Expand Up @@ -191,8 +190,9 @@ def submit_explore_json_job(
force: Optional[bool] = False,
user_id: Optional[int] = None,
) -> dict[str, Any]:
# pylint: disable=import-outside-toplevel
from superset import security_manager
from superset.extensions import ( # pylint: disable=import-outside-toplevel
security_manager,
)

job_metadata = self.init_job(channel_id, user_id)
self._load_explore_json_into_cache_job.delay(
Expand All @@ -202,6 +202,7 @@ def submit_explore_json_job(
form_data,
response_type,
force,
soft_time_limit=current_app.config["SQLLAB_ASYNC_TIME_LIMIT_SEC"],
)
return job_metadata

Expand All @@ -211,8 +212,9 @@ def submit_chart_data_job(
form_data: dict[str, Any],
user_id: Optional[int] = None,
) -> dict[str, Any]:
# pylint: disable=import-outside-toplevel
from superset import security_manager
from superset.extensions import ( # pylint: disable=import-outside-toplevel
security_manager,
)

# if it's guest user, we want to pass the guest token to the celery task
# chart data cache key is calculated based on the current user
Expand All @@ -224,6 +226,7 @@ def submit_chart_data_job(
if (guest_user := security_manager.get_current_guest_user_if_guest())
else job_metadata,
form_data,
soft_time_limit=current_app.config["SQLLAB_ASYNC_TIME_LIMIT_SEC"],
)
return job_metadata

Expand Down
5 changes: 2 additions & 3 deletions superset/available_domains/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,9 @@
# under the License.
import logging

from flask import Response
from flask import current_app as app, Response
from flask_appbuilder.api import expose, protect, safe

from superset import conf
from superset.available_domains.schemas import AvailableDomainsSchema
from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP
from superset.extensions import event_logger
Expand Down Expand Up @@ -70,6 +69,6 @@ def get(self) -> Response:
$ref: '#/components/responses/403'
"""
result = self.available_domains_schema.dump(
{"domains": conf.get("SUPERSET_WEBSERVER_DOMAINS")}
{"domains": app.conf.get("SUPERSET_WEBSERVER_DOMAINS")}
)
return self.response(200, result=result)
7 changes: 3 additions & 4 deletions superset/charts/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
from werkzeug.wrappers import Response as WerkzeugResponse
from werkzeug.wsgi import FileWrapper

from superset import app, is_feature_enabled, thumbnail_cache
from superset import thumbnail_cache
from superset.charts.filters import (
ChartAllTextFilter,
ChartCertifiedFilter,
Expand Down Expand Up @@ -77,7 +77,7 @@
from superset.commands.importers.v1.utils import get_contents_from_bundle
from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
from superset.daos.chart import ChartDAO
from superset.extensions import event_logger
from superset.extensions import event_logger, feature_flag_manager
from superset.models.slice import Slice
from superset.tasks.thumbnails import cache_chart_thumbnail
from superset.tasks.utils import get_current_user
Expand All @@ -93,7 +93,6 @@
from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners

logger = logging.getLogger(__name__)
config = app.config


class ChartRestApi(BaseSupersetModelRestApi):
Expand All @@ -104,7 +103,7 @@ class ChartRestApi(BaseSupersetModelRestApi):

@before_request(only=["thumbnail", "screenshot", "cache_screenshot"])
def ensure_thumbnails_enabled(self) -> Optional[Response]:
if not is_feature_enabled("THUMBNAILS"):
if not feature_flag_manager.is_feature_enabled("THUMBNAILS"):
return self.response_404()
return None

Expand Down
7 changes: 3 additions & 4 deletions superset/charts/data/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
from flask_babel import gettext as _
from marshmallow import ValidationError

from superset import is_feature_enabled, security_manager
from superset.async_events.async_query_manager import AsyncQueryTokenException
from superset.charts.api import ChartRestApi
from superset.charts.data.query_context_cache_loader import QueryContextCacheLoader
Expand All @@ -45,7 +44,7 @@
from superset.connectors.sqla.models import BaseDatasource
from superset.daos.exceptions import DatasourceNotFound
from superset.exceptions import QueryObjectValidationError
from superset.extensions import event_logger
from superset.extensions import event_logger, feature_flag_manager, security_manager
from superset.models.sql_lab import Query
from superset.utils.core import (
create_zip,
Expand Down Expand Up @@ -164,7 +163,7 @@ def get_data(self, pk: int) -> Response:

# TODO: support CSV, SQL query and other non-JSON types
if (
is_feature_enabled("GLOBAL_ASYNC_QUERIES")
feature_flag_manager.is_feature_enabled("GLOBAL_ASYNC_QUERIES")
and query_context.result_format == ChartDataResultFormat.JSON
and query_context.result_type == ChartDataResultType.FULL
):
Expand Down Expand Up @@ -252,7 +251,7 @@ def data(self) -> Response:

# TODO: support CSV, SQL query and other non-JSON types
if (
is_feature_enabled("GLOBAL_ASYNC_QUERIES")
feature_flag_manager.is_feature_enabled("GLOBAL_ASYNC_QUERIES")
and query_context.result_format == ChartDataResultFormat.JSON
and query_context.result_type == ChartDataResultType.FULL
):
Expand Down
2 changes: 1 addition & 1 deletion superset/charts/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
from sqlalchemy.orm import aliased
from sqlalchemy.orm.query import Query

from superset import db, security_manager
from superset.connectors.sqla import models
from superset.connectors.sqla.models import SqlaTable
from superset.extensions import db, security_manager
from superset.models.core import FavStar
from superset.models.slice import Slice
from superset.utils.core import get_user_id
Expand Down
36 changes: 15 additions & 21 deletions superset/charts/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@
import inspect
from typing import Any, TYPE_CHECKING

from flask import current_app as app
from flask_babel import gettext as _
from marshmallow import EXCLUDE, fields, post_load, Schema, validate
from marshmallow.validate import Length, Range

from superset import app
from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType
from superset.db_engine_specs.base import builtin_time_grains
from superset.tags.models import TagType
Expand All @@ -41,11 +41,7 @@
from superset.common.query_context import QueryContext
from superset.common.query_context_factory import QueryContextFactory

config = app.config

#
# RISON/JSON schemas for query parameters
#
get_delete_ids_schema = {"type": "array", "items": {"type": "integer"}}

width_height_schema = {
Expand Down Expand Up @@ -603,6 +599,14 @@ class ChartDataContributionOptionsSchema(ChartDataPostProcessingOperationOptions
)


def get_time_grain_choices(): # type: ignore
return [
i
for i in {**builtin_time_grains, **app.config["TIME_GRAIN_ADDONS"]}.keys()
if i
]


class ChartDataProphetOptionsSchema(ChartDataPostProcessingOperationOptionsSchema):
"""
Prophet operation config.
Expand All @@ -615,13 +619,8 @@ class ChartDataProphetOptionsSchema(ChartDataPostProcessingOperationOptionsSchem
"[ISO 8601](https://en.wikipedia.org/wiki/ISO_8601#Durations) durations.",
"example": "P1D",
},
validate=validate.OneOf(
choices=[
i
for i in {**builtin_time_grains, **config["TIME_GRAIN_ADDONS"]}.keys()
if i
]
),
# TODO reinstate at runtime since schemas shouldn't be dynamic
# validate=validate.OneOf(choices=get_time_grain_choices),
required=True,
)
periods = fields.Integer(
Expand Down Expand Up @@ -952,14 +951,14 @@ class ChartDataExtrasSchema(Schema):
relative_start = fields.String(
metadata={
"description": "Start time for relative time deltas. "
'Default: `config["DEFAULT_RELATIVE_START_TIME"]`'
'Default: `app.config["DEFAULT_RELATIVE_START_TIME"]`'
},
validate=validate.OneOf(choices=("today", "now")),
)
relative_end = fields.String(
metadata={
"description": "End time for relative time deltas. "
'Default: `config["DEFAULT_RELATIVE_START_TIME"]`'
'Default: `app.config["DEFAULT_RELATIVE_START_TIME"]`'
},
validate=validate.OneOf(choices=("today", "now")),
)
Expand All @@ -981,13 +980,8 @@ class ChartDataExtrasSchema(Schema):
"[ISO 8601](https://en.wikipedia.org/wiki/ISO_8601#Durations) durations.",
"example": "P1D",
},
validate=validate.OneOf(
choices=[
i
for i in {**builtin_time_grains, **config["TIME_GRAIN_ADDONS"]}.keys()
if i
]
),
# TODO reinstate at runtime since schemas shouldn't be dynamic
# validate=validate.OneOf(choices=[]),
allow_none=True,
)
instant_time_comparison_range = fields.String(
Expand Down
5 changes: 3 additions & 2 deletions superset/cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,12 @@

import click
from colorama import Fore, Style
from flask import current_app as app
from flask.cli import FlaskGroup, with_appcontext

from superset import app, appbuilder, cli, security_manager
from superset import cli
from superset.cli.lib import normalize_token
from superset.extensions import db
from superset.extensions import appbuilder, db, security_manager

logger = logging.getLogger(__name__)

Expand Down
3 changes: 2 additions & 1 deletion superset/cli/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@

import click
from colorama import Fore
from flask import current_app as app
from flask.cli import with_appcontext

import superset.utils.database as database_utils
from superset import app, security_manager
from superset import security_manager

logger = logging.getLogger(__name__)

Expand Down
2 changes: 1 addition & 1 deletion superset/cli/viz_migrations.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from click_option_group import optgroup, RequiredMutuallyExclusiveOptionGroup
from flask.cli import with_appcontext

from superset import db
from superset.extensions import db


class VizType(str, Enum):
Expand Down
Loading

0 comments on commit e8b23de

Please sign in to comment.