Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flask App factory PR #1 #8418

Merged
merged 48 commits into from
Nov 20, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
56e6c1a
First cut at app factory
craig-rueda Oct 15, 2019
21e3494
Setting things back to master
craig-rueda Oct 18, 2019
be37d24
Working with new FLASK_APP
craig-rueda Oct 18, 2019
9d31fff
Merge branch 'master' into app_factory
craig-rueda Oct 18, 2019
259a307
Still need to refactor Celery
craig-rueda Oct 21, 2019
9f8fe89
Merge branch 'master' into app_factory
craig-rueda Oct 25, 2019
89bba87
CLI mostly working
craig-rueda Oct 28, 2019
fb9cf33
Working on unit tests
craig-rueda Oct 28, 2019
01aafb5
Moving cli stuff around a bit
craig-rueda Oct 28, 2019
9bf6333
Merge branch 'master' into app_factory
craig-rueda Nov 6, 2019
db25d4d
Removing get in config
craig-rueda Nov 6, 2019
0ab9f0a
Defaulting test config
craig-rueda Nov 6, 2019
921c285
Adding flask-testing
craig-rueda Nov 7, 2019
095529f
flask-testing casing
craig-rueda Nov 8, 2019
428655b
resultsbackend property bug
craig-rueda Nov 8, 2019
97e4cda
Fixing up cli
craig-rueda Nov 8, 2019
9f686cd
Quick fix for KV api
craig-rueda Nov 8, 2019
f1318d5
Working on save slice
craig-rueda Nov 8, 2019
947622b
Fixed core_tests
craig-rueda Nov 9, 2019
91a39f5
Fixed utils_tests
craig-rueda Nov 9, 2019
f675ad4
Most tests working - still need to dig into remaining app_context iss…
craig-rueda Nov 13, 2019
78990d1
All tests passing locally - need to update code comments
craig-rueda Nov 13, 2019
7e37cef
Fixing dashboard tests again
craig-rueda Nov 13, 2019
3b95e96
Blacking
craig-rueda Nov 13, 2019
7f1dadc
Sorting imports
craig-rueda Nov 13, 2019
01607c4
linting
craig-rueda Nov 13, 2019
3a66b07
removing envvar mangling
craig-rueda Nov 13, 2019
5764936
Merge remote-tracking branch 'remotes/origin/master' into app_factory
craig-rueda Nov 14, 2019
74cd1bf
blacking
craig-rueda Nov 14, 2019
8d53bd7
Fixing unit tests
craig-rueda Nov 14, 2019
037cb2c
isorting
craig-rueda Nov 14, 2019
3c0d7da
licensing
craig-rueda Nov 14, 2019
c3c480d
fixing mysql tests
craig-rueda Nov 15, 2019
90a25f2
fixing cypress?
craig-rueda Nov 15, 2019
d8e74a2
fixing .flaskenv
craig-rueda Nov 15, 2019
55d5c27
fixing test app_ctx
craig-rueda Nov 15, 2019
92e7922
fixing cypress
craig-rueda Nov 15, 2019
798d723
Merge remote-tracking branch 'remotes/upstream/master' into app_factory
craig-rueda Nov 18, 2019
b943d68
moving manifest processor around
craig-rueda Nov 18, 2019
ab7a69c
moving results backend manager around
craig-rueda Nov 18, 2019
f72438a
Cleaning up __init__ a bit more
craig-rueda Nov 19, 2019
b30ff82
Addressing PR comments
craig-rueda Nov 19, 2019
b333044
Addressing PR comments
craig-rueda Nov 19, 2019
a9ecc7a
Blacking
craig-rueda Nov 19, 2019
ccd0338
Fixes for running celery worker
craig-rueda Nov 19, 2019
03a3e4a
Tuning isort
craig-rueda Nov 19, 2019
cf0dac2
Merge remote-tracking branch 'remotes/origin/master' into app_factory
craig-rueda Nov 19, 2019
d2f4ada
Blacking
craig-rueda Nov 19, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions requirements-dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
black==19.3b0
coverage==4.5.3
flask-cors==3.0.7
flask-testing==0.7.1
ipdb==0.12
isort==4.3.21
mypy==0.670
Expand Down
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ def get_git_sha():
extras_require={
"bigquery": ["pybigquery>=0.4.10", "pandas_gbq>=0.10.0"],
"cors": ["flask-cors>=2.0.0"],
"flask-testing": ["Flask-Testing==0.7.1"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. Can we lowercase this like the other packages. Package names are case insensitive.

"gsheets": ["gsheetsdb>=0.1.9"],
"hive": ["pyhive[hive]>=0.6.1", "tableschema", "thrift>=0.11.0, <1.0.0"],
"mysql": ["mysqlclient==1.4.2.post1"],
Expand Down
34 changes: 17 additions & 17 deletions superset/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def post_init(self) -> None:
pass

def configure_celery(self) -> None:
celery_app.config_from_object(self.config.get("CELERY_CONFIG"))
celery_app.config_from_object(self.config["CELERY_CONFIG"])
celery_app.set_default()

def init_views(self) -> None:
Copy link
Member

@john-bodley john-bodley Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we simply make all our views blueprints and register them accordingly? This may simplify things in the future and/or reduce the cyclical dependency issues we often run into. We (Airbnb) have found the model of using blueprints for everything has worked quite successfully and the Flask app really just glues everything together in a somewhat lightweight fashion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, definitely. I want to pull up all occurrences of things like appbuilder.add_view() to this method. Ideally, the views themselves wouldn't register themselves, but rather this init logic would compose the views we're interested in.

Again, this PR was intended to be the first step in chipping away at this pattern migration. Thoughts on going for it?? @mistercrunch ??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that FAB's add_view creates a blueprint for each call

Copy link
Member

@dpgaspar dpgaspar Oct 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yap, it's ModelView, ModelRestApi class is a blueprint and it's actually created on add_view

Expand All @@ -112,7 +112,7 @@ def init_app_in_ctx(self) -> None:

# Hook that provides administrators a handle on the Flask APP
# after initialization
flask_app_mutator = self.config.get("FLASK_APP_MUTATOR")
flask_app_mutator = self.config["FLASK_APP_MUTATOR"]
if flask_app_mutator:
flask_app_mutator(self.flask_app)

Expand Down Expand Up @@ -155,8 +155,8 @@ def setup_event_logger(self):

def configure_data_sources(self):
# Registering sources
module_datasource_map = self.config.get("DEFAULT_MODULE_DS_MAP")
module_datasource_map.update(self.config.get("ADDITIONAL_MODULE_DS_MAP"))
module_datasource_map = self.config["DEFAULT_MODULE_DS_MAP"]
module_datasource_map.update(self.config["ADDITIONAL_MODULE_DS_MAP"])
ConnectorRegistry.register_sources(module_datasource_map)

def configure_cache(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the cookie-cutter example one could have a method which registers all extensions.

Expand All @@ -167,10 +167,10 @@ def configure_feature_flags(self):
feature_flag_manager.init_app(self.flask_app)

def configure_fab(self):
if self.config.get("SILENCE_FAB"):
if self.config["SILENCE_FAB"]:
logging.getLogger("flask_appbuilder").setLevel(logging.ERROR)

custom_sm = self.config.get("CUSTOM_SECURITY_MANAGER") or SupersetSecurityManager
custom_sm = self.config["CUSTOM_SECURITY_MANAGER"] or SupersetSecurityManager
if not issubclass(custom_sm, SupersetSecurityManager):
raise Exception(
"""Your CUSTOM_SECURITY_MANAGER must now extend SupersetSecurityManager,
Expand All @@ -185,17 +185,17 @@ def configure_fab(self):
appbuilder.init_app(self.flask_app, db.session)

def configure_middlewares(self):
if self.config.get("ENABLE_CORS"):
if self.config["ENABLE_CORS"]:
from flask_cors import CORS

CORS(self.flask_app, **self.config.get("CORS_OPTIONS"))
CORS(self.flask_app, **self.config["CORS_OPTIONS"])

if self.config.get("ENABLE_PROXY_FIX"):
if self.config["ENABLE_PROXY_FIX"]:
from werkzeug.middleware.proxy_fix import ProxyFix

self.flask_app.wsgi_app = ProxyFix(self.flask_app.wsgi_app, **self.config.get("PROXY_FIX_CONFIG"))
self.flask_app.wsgi_app = ProxyFix(self.flask_app.wsgi_app, **self.config["PROXY_FIX_CONFIG"])

if self.config.get("ENABLE_CHUNK_ENCODING"):
if self.config["ENABLE_CHUNK_ENCODING"]:

class ChunkedEncodingFix(object):
def __init__(self, app):
Expand All @@ -210,17 +210,17 @@ def __call__(self, environ, start_response):

self.flask_app.wsgi_app = ChunkedEncodingFix(self.flask_app.wsgi_app)

if self.config.get("UPLOAD_FOLDER"):
if self.config["UPLOAD_FOLDER"]:
try:
os.makedirs(self.config.get("UPLOAD_FOLDER"))
os.makedirs(self.config["UPLOAD_FOLDER"])
except OSError:
pass

for middleware in self.config.get("ADDITIONAL_MIDDLEWARE"):
for middleware in self.config["ADDITIONAL_MIDDLEWARE"]:
self.flask_app.wsgi_app = middleware(self.flask_app.wsgi_app)

# Flask-Compress
if self.config.get("ENABLE_FLASK_COMPRESS"):
if self.config["ENABLE_FLASK_COMPRESS"]:
Compress(self.flask_app)

if self.config["TALISMAN_ENABLED"]:
Expand All @@ -240,14 +240,14 @@ def setup_db(self):
migrate.init_app(self.flask_app, db=db, directory=APP_DIR + "/migrations")

def configure_wtf(self):
if self.config.get("WTF_CSRF_ENABLED"):
if self.config["WTF_CSRF_ENABLED"]:
csrf = CSRFProtect(self.flask_app)
csrf_exempt_list = self.config.get("WTF_CSRF_EXEMPT_LIST", [])
craig-rueda marked this conversation as resolved.
Show resolved Hide resolved
for ex in csrf_exempt_list:
csrf.exempt(ex)

def register_blueprints(self):
for bp in self.config.get("BLUEPRINTS"):
for bp in self.config["BLUEPRINTS"]:
try:
logger.info("Registering blueprint: '{}'".format(bp.name))
self.flask_app.register_blueprint(bp)
Expand Down
7 changes: 6 additions & 1 deletion tests/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,10 @@
TODO: Clean this up! The current pattern of accessing app props on package init means
that we need to ensure the creation of our Flask app BEFORE any tests load
"""
from os import environ
environ.setdefault("SUPERSET_CONFIG", "tests.superset_test_config")

from superset.app import create_app
create_app().app_context().push()
app = create_app()
# ctx = app.test_request_context()
# ctx.push()
15 changes: 11 additions & 4 deletions tests/base_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,29 @@

import pandas as pd
from flask_appbuilder.security.sqla import models as ab_models
from flask_testing import TestCase

from superset import app, db, security_manager
from superset.app import create_app
from superset.connectors.druid.models import DruidCluster, DruidDatasource
from superset.connectors.sqla.models import SqlaTable
from superset.models import core as models
from superset.models.core import Database
from superset.utils.core import get_example_database

BASE_DIR = app.config["BASE_DIR"]


class SupersetTestCase(unittest.TestCase):
class SupersetTestCase(TestCase):
def __init__(self, *args, **kwargs):
super(SupersetTestCase, self).__init__(*args, **kwargs)
self.client = app.test_client()
self.maxDiff = None
self.BASE_DIR = ""

def create_app(self):
from tests import app
return app

def setUp(self) -> None:
self.BASE_DIR = app.config["BASE_DIR"]

@classmethod
def create_druid_test_objects(cls):
Expand Down
9 changes: 3 additions & 6 deletions tests/core_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,13 @@ class CoreTests(SupersetTestCase):
def __init__(self, *args, **kwargs):
super(CoreTests, self).__init__(*args, **kwargs)

@classmethod
def setUpClass(cls):
cls.table_ids = {
tbl.table_name: tbl.id for tbl in (db.session.query(SqlaTable).all())
}

def setUp(self):
db.session.query(Query).delete()
db.session.query(models.DatasourceAccessRequest).delete()
db.session.query(models.Log).delete()
self.table_ids = {
tbl.table_name: tbl.id for tbl in (db.session.query(SqlaTable).all())
}

def tearDown(self):
db.session.query(Query).delete()
Expand Down