[SIP-24] Proposal to introduce Flask app factory pattern

[craig-rueda]

Motivation

Superset’s singletons are currently configured in the superset package’s __init__.py python module which means that the app itself (Superset) cannot be configured any other way than is described in the root package. The act of simply importing superset for other purposes, such as the cli forces the entire app to load, even though all you might need is SqlAlchemy, or some other component. A better approach is to leverage a pattern such as the “Factory Pattern” in which one or more functions can compose several components into a single “app”. What’s more is that certain portions of the app might need to be overridden in test, or for different use cases. Making the app more composable makes it easier to alter functionality as needed and customize.

Proposed Change

In order to get this done, we will need to add a few new modules, clean up superset.__init__.py and update the way views are added to FAB. This change is largely a refactor, with no change to functionality.

We should perform this migration in a few phases (likely several PRs):
1. Migrate all current direct references to app to leverage flask.current_app proxy
2. Introduce a new extensions.py in the base of the superset package whose job it will be to instantiate the Flask app, and other singletons, such as AppBuilder.
3. Move all logic currently in superset.__init__.py into a new app.py file which will define a create_app() function whose responsibility will be to call init_app() on the Flask app, along with other flask app aware objects
4. Move all calls to appbuilder.add_view_xxx() to app.py

New or Changed Public Interfaces

The biggest change will be the removal of superset.app from the global scope. Instead, we will leverage Flask’s current_app proxy in order to reference the current running app. There will be no changes to the set of dependent libraries.

New dependencies

n/a

Migration Plan and Compatibility

Documentation will likely need to be added which describes the "new way" of doing things once this is merged into master. Contributors that are accustomed to the current structure will likely need to spend a little time coming up to speed with the location of various parts.

Rejected Alternatives

n/a

[issue-label-bot]

Issue-Label Bot is automatically applying the label #enhancement to this issue, with a confidence of 0.65. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

[mistercrunch]

+1!

Note: FAB still has some intricacies around using the factory pattern, for instance, as single security manager can be used across all generated apps.

Based on SIP-0, I think you're supposed to start a [DISCUSS] thread on the mailing list that points to this issue

Also I think this should help with the looming circular dependencies we've been dealing with.

[dpgaspar]

+1

This would be an awesome improvement. FAB supports loading external security managers defined on the config, yet, any blockers found I'm happy to help.

[willbarrett]

I like this plan. I do wonder if we should get some eyes from someone like @betodealmeida or other contributors from non-Preset organizations?

[betodealmeida]

@willbarrett I'm +1 on this. I think it would really simplify development and debugging, and I see no downsides to it.

[mistercrunch]

Quick note to say that we'll need to remove all module scoped configuration references like this one:
https://github.com/apache/incubator-superset/blob/master/superset/views/schedules.py#L294

[craig-rueda]

I opened a super early WIP PR (#8418) so I could share my current direction with @john-bodley . Right now, it's looking like it will be a bit more difficult than I had perviously thought to break things into multiple "smaller" chunks.

The biggest issue I've come across is needing to change the way people start up Superset from a Flask point of view, from

FLASK_APP="superset.app"

to

FLASK_APP="superset.app:create_app()"

[john-bodley]

@craig-rueda I believe if you add the following in app.py,

@click.group(cls=FlaskGroup, create_app=create_app)
def cli() -> None:
    """
    A utility script for the Superset application.
    """

then having FLASK_APP="superset.app" should still work.

[mistercrunch]

I feel like we've made a lot of progress in this direction. Can we identify the gap here?

[craig-rueda]

Just more chipping away is needed. We're currently in a spot where we can start migrating away from referring to global extensions in various layers (ie models).

[rusackas]

@craig-rueda it looks like this needs to be put through the official DISCUSS -> VOTE process so it can be closed, if you don't mind following up here. Good news is, it looks like it has good support!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.