pallets-eco / flask-admin

Simple and extensible administrative interface framework for Flask
https://flask-admin.readthedocs.io
BSD 3-Clause "New" or "Revised" License
5.78k stars 1.57k forks source link

No changes in field history after form model population #1461

Open voronind opened 7 years ago

voronind commented 7 years ago

Use Flask-admin and SQLAlchemy. I need handling events from SQLAlchemy on editing objects and search fields that was changed in whole application not just admin part. After editing usual text field (i.e. name from 'old' to 'new') in model via Flask-admin there is no changes in history of this field. After form.populate_obj(model) in ModelView.update_model we cannot detect changes. Edited object is present in session.dirty and we gets it in before_commit event handler, but session.is_modified(model) returns False. model._sa_instance_state.attrs.name.history return History(added=(), unchanged=['new'], deleted=()) instead of History(added=['new'], unchanged=[], deleted=['old'])

mrjoes commented 7 years ago

I didn't see similar behavior - we were also tracking model changes on a SQLA level for logging purposes, but we were seeing individual field. changes

killthekitten commented 3 years ago

@voronind I'm a bit late to the party, but would the behavior change when you wrap the code in with db.session.no_autoflush:?

By default, SQLAlchemy triggers a flush on every query to the database, flushing the state and causing it to reset.

Consider this made up Flask-Admin code:

def on_model_change(self, _form, model, *args):
    if len(model.attachments) > 1:
        raise ValidationError("Too many attachments")
    print(db.inspect(model).attrs["name"].history) # => History(added=(), unchanged=['new'], deleted=())

This would issue a query to the database on the if statement, and cause a flush. You would need to decide on the strategy for autoflush: ignore it in-place, or disable globally.

If you go for a local fix, make sure it's wrapping all the code that comes before the state check:

def on_model_change(self, _form, model, *args):
    with db.session.no_autoflush:
        if len(model.attachments) > 1:
            raise ValidationError("Too many attachments")
        print(db.inspect(model).attrs["name"].history) # => History(added=['new'], unchanged=[], deleted=['old'])

Perhaps it would be a good idea to make this behavior a default in all the Flask-Admin views:

from flask_admin.contrib.sqla import ModelView

class BaseModelView(ModelView):
    def _on_model_change(self, form, model, is_created):
        """
        Adds a `no_autoflush` wrapper to every `on_model_change` handler.
        This is required to not accidentally flush the session state when
        performing validation checks.

        More details here:

        - https://github.com/flask-admin/flask-admin/issues/1461
        """

        with db.session.no_autoflush:
            super()._on_model_change(form, model, is_created)
killthekitten commented 3 years ago

@mrjoes how reliable is the _on_model_change hook? Is there a better place to turn on the no_autoflush context?