Flask-AppBuilder icon indicating copy to clipboard operation
Flask-AppBuilder copied to clipboard

Refactor logging to use lazy interpolation

Open xuganyu96 opened this issue 1 year ago • 2 comments

While not a bug, it seems like the logging behavior of Flask-AppBuilder is not very consistent. Some logging is done with % formatting, some with .format function call, some with f-strings, and some with lazy interpolation, the last of which is pedantically the "correct" way to log.

Would it make sense to refactor all logging function calls in Flask-AppBuilder to use lazy interpolation? Here is an example of lazy interpolation already being used: https://github.com/dpgaspar/Flask-AppBuilder/blob/4f9873af0318aa0b39e290b77f6f140315bc3d37/flask_appbuilder/models/sqla/filters.py#L254

class FilterRelationManyToManyEqual(FilterRelation):
    name = lazy_gettext("Relation as Many")
    arg_name = "rel_m_m"

    def apply_item(self, query, field, value_item):
        """
        Get object by column_name and value_item, then apply filter if object exists
        Query with new filter applied
        """
        try:
            rel_obj = self.datamodel.get_related_obj(self.column_name, value_item)
        except SQLAlchemyError as exc:
            logging.warning(
                "Filter exception for %s with value %s, will not apply",
                field,
                value_item,
            )
            try:
                self.datamodel.session.rollback()
            except SQLAlchemyError:
                # on MSSQL a rollback would fail here
                pass
            raise ApplyFilterException(exception=exc)

        if rel_obj:
            return query.filter(field.contains(rel_obj))
        else:
            log.error(
                "Related object for column: %s, value: %s return Null",
                self.column_name,
                value_item,
            )

        return query

xuganyu96 avatar Jun 16 '23 03:06 xuganyu96

it would, totally agree

dpgaspar avatar Jun 22 '23 15:06 dpgaspar

A quick grep revealed the following places where log is called:

  • [x] ./flask_appbuilder/urltools.py
  • [x] ./flask_appbuilder/security/registerviews.py
  • [x] ./flask_appbuilder/security/sqla/manager.py
  • [x] ./flask_appbuilder/security/mongoengine/manager.py
  • [x] ./flask_appbuilder/security/manager.py
  • [x] ./flask_appbuilder/security/views.py
  • [x] ./flask_appbuilder/security/decorators.py
  • [x] ./flask_appbuilder/tests/test_fab_cli.py
  • [x] ./flask_appbuilder/tests/test_custom_indexview.py
  • [x] ./flask_appbuilder/tests/test_mongoengine.py
  • [x] ./flask_appbuilder/tests/test_mvc.py
  • [x] ./flask_appbuilder/baseviews.py
  • [x] ./flask_appbuilder/utils/base.py
  • [x] ./flask_appbuilder/models/sqla/interface.py
  • [x] ./flask_appbuilder/models/sqla/filters.py
  • [x] ./flask_appbuilder/models/mongoengine/interface.py
  • [x] ./flask_appbuilder/models/group.py
  • [x] ./flask_appbuilder/models/filters.py
  • [x] ./flask_appbuilder/forms.py
  • [x] ./flask_appbuilder/static/appbuilder/js/bootstrap.min.js
  • [x] ./flask_appbuilder/api/init.py
  • [x] ./flask_appbuilder/base.py
  • [x] ./flask_appbuilder/views.py
  • [x] ./bin/hash_db_password.py
  • [x] ./bin/migrate_db_0.7.py
  • [x] ./bin/migrate_db_1.3.py
  • [x] ./examples/basefilter/testdata.py
  • [x] ./examples/quicksqlviews/testdata.py
  • [x] ./examples/related_fields/testdata.py
  • [x] ./examples/user_registration/app/views.py
  • [x] ./examples/composite_keys/testdata.py
  • [x] ./examples/enums/testdata.py
  • [x] ./examples/quickcharts2/app/views.py
  • [x] ./examples/quickcharts2/build/bdist.linux-i686/egg/app/views.py
  • [x] ./examples/quickcharts2/build/lib/app/views.py
  • [x] ./examples/quickmigrate/testdata.py
  • [x] ./examples/quickcharts/app/data.py
  • [x] ./examples/crud_rest_api/testdata.py
  • [x] ./examples/extendsecurity/testdata.py
  • [x] ./examples/quickhowto3/.coverage
  • [x] ./examples/quickhowto3/migrate_db_0.7.py
  • [x] ./examples/extendsecurity2/testdata.py
  • [x] ./examples/quickhowto2/app/templates/new_base.html
  • [x] ./examples/react-rest-api/testdata.py
  • [x] ./examples/quickhowto/testdata.py

Will check off individual file as they are migrated to using lazy interpolation

xuganyu96 avatar Jun 24 '23 00:06 xuganyu96