flask-sqlalchemy icon indicating copy to clipboard operation
flask-sqlalchemy copied to clipboard

after_create event is called multiple times for a single MetaData

Open mrcljx opened this issue 4 years ago • 3 comments

Expected Behavior

When calling db.create_all(), the after_create event should only be dispatched once for a sqlalchemy.MetaData object when using multiple binds.

Workaround-ish: There should be a way (extra keyword maybe?) to detect for which current bind_key the event is called so that the listener can decide whether to run. The only way to do this right now is via kw["tables"][0].info.get("bind_key") which won't work if there aren't any tables, yet.

Actual Behavior

It's dispatched multiple times (once per bind).

Environment

  • Python version: 3.6
  • Flask-SQLAlchemy version: 2.4
  • SQLAlchemy version: 1.3.13

mrcljx avatar Jun 05 '20 10:06 mrcljx

I think this is handled inside SQLAlchemy itself. In the short term, something like your workaround looks like it may work?

Surely after_create would make the tables you need to get the bind key? Or, could you catch the exception raised when they don't exist, to skip your logic in the cases when it's not needed/won't run?

CoburnJoe avatar Jun 05 '20 11:06 CoburnJoe

I think this is handled inside SQLAlchemy itself.

I might be wrong here, but my understanding is that multi-db support via bind-keys is added by flask_sqlalchemy as documented here. The deliberate re-dispatch happens because of the for-loop in _execute_for_all_tables in flask_sqlalchemy/__init__.py:

        for bind in binds:
            extra = {}
            if not skip_tables:
                tables = self.get_tables_for_bind(bind)
                extra['tables'] = tables
            op = getattr(self.Model.metadata, operation)
            op(bind=self.get_engine(app, bind), **extra)

Surely after_create would make the tables you need to get the bind key?

Here's an example where this doesn't work well: The sqlalchemy_utils library has a create_materialized_view helper, which will listen for after_create to then create the materialized view. What I've been seeing is that because of the double event dispatch, the library will create views unintentionally in both databases. It ultimately fails because the tables, that the view references, don't exist in the second database.

Or, could you catch the exception raised when they don't exist, to skip your logic in the cases when it's not needed/won't run?

I'm worried that I'd make that catch too broad (especially because the exceptions will be low-level DB exceptions), swallowing an exception which shouldn't have been.

In the short term, something like your workaround looks like it may work?

I'll probably monkey-patch flask_sqlalchemy.SQLAlchemy.get_engine to setattr(engine, "bind_key", bind) - this seems to be more resilient. Is that maybe something that I could create a PR for and try to merge upstream?

mrcljx avatar Jun 05 '20 12:06 mrcljx

You're quite correct - sorry for my misunderstanding :)

I think a PR would be the best way to resolve this, but I think there are some considerations. We need to figure out if this is a desirable behaviour or just an oversight. I think it there may be many use-cases whereby multiple events are desirable.

Perhaps @mitsuhiko or @davidism can advise further.

CoburnJoe avatar Jun 05 '20 12:06 CoburnJoe

#1087 uses a separate metadata per bind, so this should no longer be an issue.

davidism avatar Sep 18 '22 17:09 davidism