encode / apistar

The Web API toolkit. 🛠
https://docs.apistar.com
BSD 3-Clause "New" or "Revised" License
5.57k stars 411 forks source link

Is there any "right" way to integrate SQLAlchemy in the new versions of apistar (>= 0.4.0) #546

Closed rjusher closed 6 years ago

rjusher commented 6 years ago

Some time ago I started a project based on the microservices architecture it was based on the version 0.3.9 of apistar.

At that version the way to incorporate SQLAlchemy was something like the following:

class SQLAlchemyConnection(object):
    def execute(self, *args, **kwargs):
        pass

@contextlib.contextmanager
def get_connection(backend: SQLAlchemyBackend) -> typing.Generator[
    SQLAlchemyConnection, None, None]:
    conn = backend.engine.connect()

    try:
        yield conn
    finally:
        conn.close()

Right now I was trying to upgrade apistar to the latest version and I understand there are some breaking changes but the only functionality I currently need is SQLAlchemy connectivity. I found the following project:

https://github.com/PeRDy/apistar-sqlalchemy

Which resolves the connection closing with event_hooks.

class SQLAlchemySessionComponent(Component):
    def __init__(self, url: str) -> None:
        """
        Configure a new database backend.
        :param url: SQLAlchemy database url.
        """
        self.engine = create_engine(url)
        database.Session.configure(bind=self.engine)
        logger.info('SQLAlchemy connection created')
        logger.debug('Engine connection to %s', url)

    def resolve(self) -> Session:
        return database.Session()

class SQLAlchemyTransactionHook:
    def on_response(self, response: http.Response, session: Session, exc: Exception) -> http.Response:
        if exc is None:
            session.commit()
            logger.debug('Commit')
            self.remove_session()
        else:
            session.rollback()
            logger.debug('Rollback')
            self.remove_session()

        return response

I am not sure if it is a good way of solving it, because there are some endpoints in my api which don't require the SQLAlchemy connection but the way event_hooks works ( or what I understand of them) is that it creates a connection for every request even if I don't need it because it hooks to the request lifecycle, but the previous way only created the connection when it was injected which to me seems "better". What I mean by it creates it no matter what endpoint it is, is that all the event_hooks functions get called for any given request but if the connection wasn't requested to the injector for the function it gets instantiated for the event_hook hence creating a connection that gets closed immediately after, with out executing anything against the database. I am not a SQLAlchemy expert so I don't know if it matters if the connection object connects or if SQLAlchemy handles the resource so the connect function doesn't consume CPU resources but my intuition is that it does.

So how can I solve my SQLAlchemy connectivity problem?

Thanks.

danigosa commented 6 years ago

Best way to do it (although IMHO not very elegant) is to do the solution that does @audiolion in apistar-jwt: https://github.com/audiolion/apistar-jwt/blob/master/apistar_jwt/decorators.py that is create a decorator that adds necessary information in the function object itself, something like:

def disable_db(fn):  # or a better name...
    fn.sql = False
    return fn

Then in the component:

def resolve(self, route: http.Route) -> Session:
        if getattr(route.handler, 'sql', True):
               database.Session()
        return None

Then your hook:

def on_response(self, response: http.Response, session: Session, exc: Exception) -> http.Response:
        if session is None:
             return   # No SQL enabled
        # It's got a Session ongoing
        if exc is None:
            session.commit()
            logger.debug('Commit')
            self.remove_session()
        else:
            session.rollback()
            logger.debug('Rollback')
            self.remove_session()
        # no need to return response

Finally:

@disable_db
def your_handler(...)
       # This won't have an SQL session dangling nor won't try to cleanup resources in the hook
tomchristie commented 6 years ago

I'd suggest not using auto-closed sessions as a component - the flow control becomes non-obvious. Basically no other frameworks are taking this tack, and I think it was a mistake to push so much towards components in the first place.

pslacerda commented 6 years ago

@rjusher maybe you forgot to use scoped_session.

@danigosa your solution need first to fix #490, anyway #574 seems a lot less hackish.

@tomchristie Django autocloses database sessions automatically so I suppose that it is a very usual approach.

tomchristie commented 6 years ago

Django autocloses database sessions automatically so I suppose that it is a very usual approach.

Yes, but with (essentially) middleware, like ~every other Python framework out there. We should just push towards the same style.

pslacerda commented 6 years ago

I'm assuming that the following is pretty much a middleware, I guess specially because run setup from outer components until inner components, run the function, and then run teardown from inner to outer (where outer=left and inner=right parameters).

def resolve():
    setup()
    yield component
    teardown()

I guess that you don't wat to put this on APIStar because every use case possible to do with setup/yield/teardown is also possible to do with hooks. Is it?

On the other side to implement this in 0.5x requires an hook with two functions, an decorator, a component and some boilerplate to fully replace a single and concise component. This yields much more code, and number of bugs and code size are very proportional.

rjusher commented 6 years ago

Thanks to all.

@tomchristie so the best solution is to open/close the connection in the response function and never lose sight of the life of the connection.

Thanks.