Issue: BUG: FlaskDB incorrect implementation
Author: Andrei-Pozolotin
State: closed
Created: 2019-08-06 17:21:05

1. current implementation of `FlaskDB`
https://github.com/coleifer/peewee/blob/master/playhouse/flask_utils.py#L87
is incorrect: it stores `database` in `self` instead of the flask context

2. that prevents `FlaskDB` to be used with flask factory pattern for tests with multiple flask app instances using different per-app-instance database configurations created inside single python process

3. see:
https://flask.palletsprojects.com/en/1.1.x/patterns/appfactories/#factories-extensions
https://flask.palletsprojects.com/en/1.1.x/extensiondev/#the-extension-code


here is a minimum working version:
```
import peewee
from flask import current_app
from playhouse.sqlite_ext import SqliteExtDatabase

class PeeweeDatabase:

    def init_app(self, app) -> None:
        app.before_request(self.perform_open)
        app.teardown_request(self.perform_close)

    def perform_open(self, *args, **kwargs) -> None:
        self.database.connect()

    def perform_close(self, *args, **kwargs) -> None:
        self.database.close()

    @property
    def flask_context(self) -> Any:
        return current_app

    def produce_database(self) -> peewee.Database:
        self.logger.debug(f"produce_database")
        datafile = current_app.config['DATABASE']
        return SqliteExtDatabase(datafile)

    @property
    def database(self) -> peewee.Database:
        context = self.flask_context
        if not hasattr(context, 'peewee_database'):
            context.peewee_database = self.produce_database()
        return context.peewee_database
```


I disagree with your assessment.

You create a `FlaskDB()` instance by either

1. passing it a flask app instance
2. calling `init_app()` later with an app instance

This binds the pre/post-request hooks for the given app.

There is a 1-to-1 between `FlaskDB` instances and `Flask` instances, in other words.

I'm going to reopen this just to think about it a little more, but I'm not quite convinced that doing all kinds of magic with the app context is necessarily the right choice for this implementation.

even more simplified example:

```
import peewee
from flask import Flask, current_app
from playhouse.sqlite_ext import SqliteExtDatabase

class PeeweeDatawrap:

    def init_app(self, app:Flask) -> None:
        app.extensions['peewee_database'] = self.produce_database(app)
        app.before_request(self.perform_open)
        app.teardown_request(self.perform_close)

    def perform_open(self, *args, **kwargs) -> None:
        self.database.connect()

    def perform_close(self, *args, **kwargs) -> None:
        self.database.close()

    def produce_database(self, app:Flask) -> peewee.Database:
        datafile = app.config['DATABASE']
        return SqliteExtDatabase(datafile)

    @property
    def database(self) -> peewee.Database:
        return current_app.extensions['peewee_database']
```


with support for multiple database instances in the same flask app:
```
class PeeweeDatawrap:

    def __init__(self, data_name:str='default'):
        self.data_name = f"peewee_{data_name}"

    def init_app(self, app:Flask,) -> None:
        assert not self.data_name in app.extensions, f"duplicate entry: {self.data_name}"
        app.extensions[self.data_name] = self.produce_database(app)

    # ... snip ... #

    @property
    def database(self) -> peewee.Database:
        return current_app.extensions[self.data_name]
```

Thanks for the links and the implementation suggestions. I'm going to refactor the `FlaskDB` to more closely follow the flask extension guidelines (support for multiple apps, support for multiple databases-per-app), as you've suggested.

1. thank you.

2. regarding the name: `FlaskDB`: 
being a for-flask extension, perhaps it should be flask-centric, 
i.e. make more sense from flask point of view, say: `PeeweeFlaskPlugin`


There's an issue I realized once I started implementing some changes. Accessing the `database` relies on `current_app`. Which implies that accessing the `Model` property (which provides a base model class) will also require an app context.

This means that people will need to change their model code around and wrap everything in an app context. This would be quite disruptive for existing codebases.

Going to pass on changing the implementation for now, and if I do choose to make changes, will probably do so in a separate class that allows people to continue using `FlaskDB` without an app context.

ok, I see.


Just to get a bit more perspective I checked out the [flask-sqlachemy extension](https://flask-sqlalchemy.palletsprojects.com/en/2.x/) and the [code](https://github.com/pallets/flask-sqlalchemy/blob/master/flask_sqlalchemy/__init__.py) looks to have the same behavior as the peewee `FlaskDB` extension.

1. well, we do not want to follow low standards, do we? :-)

2. on another note, this pattern is also a major pain:
```
class Model:
   class Meta:
      database: ...
```

3. perhaps switching peewee default database injection
to flask-like `current_app` analogue would be less evil