From 3df9823a5db148e2345fb643bd252baf53ce3659 Mon Sep 17 00:00:00 2001 From: Alex Burke Date: Sat, 6 Sep 2025 16:13:59 +0200 Subject: [PATCH] Avoid linting failures by unconditionally defining a logger property. Recent changes to the linting checks have resulted in any changes in and around configuration fail in CI with the complaint that logger is not defined. In trying to understand what was happening it was found that some amount of confusion was occurring having both logger and logger_obj properties. Attempt to fix this by: 1) unconditionally defining both properties 2) always setting both properties 3) determining the type of logger being assigned and set the internal properties as appropriate Expanding on the latter, loggers are almost always set as assignment to .logger, but this was not always being passed the same kind of object. At times this was a bare logger (i.e. info(), .debug() etc) but sometimes it was something with .reopen() which would then simply be thrown away and thus reload() would not actually work. Fix this by detecting a .reopen() method and correctly referencing such an object. --- mig/shared/configuration.py | 57 +++++++++++++++---- mig/shared/logger.py | 17 ++++++ .../mig_shared_configuration--new.json | 2 - tests/test_mig_shared_configuration.py | 6 +- 4 files changed, 67 insertions(+), 15 deletions(-) diff --git a/mig/shared/configuration.py b/mig/shared/configuration.py index 91a24549a..cd5637020 100644 --- a/mig/shared/configuration.py +++ b/mig/shared/configuration.py @@ -37,6 +37,7 @@ import copy import datetime import functools +import inspect import os import pwd import re @@ -67,7 +68,7 @@ generic_valid_days, DEFAULT_USER_ID_FORMAT, valid_user_id_formats, \ valid_filter_methods, default_twofactor_auth_apps, \ mig_conf_section_dirname - from mig.shared.logger import Logger, SYSLOG_GDP + from mig.shared.logger import Logger, BareLoggerAdapter, SYSLOG_GDP from mig.shared.htmlgen import menu_items, vgrid_items from mig.shared.fileio import read_file, load_json, write_file except ImportError as ioe: @@ -693,7 +694,6 @@ def get(self, *args, **kwargs): 'logfile': '', 'loglevel': '', 'logger_obj': None, - 'logger': None, 'gdp_logger_obj': None, 'gdp_logger': None, 'auth_logger_obj': None, @@ -757,6 +757,13 @@ def __init__(self, config_file, verbose=False, skip_log=False, # Explicitly init a few helpers hot-plugged and used in ways where it's # less obvious if they are always guaranteed to already be initialized. self.default_page = None + + # internal state + self._loaded = False + + # logging related + self.logger_obj = None + self._logger = None self.auth_logger_obj = None self.gdp_logger_obj = None @@ -770,6 +777,34 @@ def __init__(self, config_file, verbose=False, skip_log=False, disable_auth_log=disable_auth_log, _config_file=config_file) + @property + def logger(self): + assert self._logger, "logging attempt prior to logger availability" + return self._logger + + @logger.setter + def logger(self, logger): + """Setter method that correctly sets logger related properties.""" + + # attempt to determine what type of objetc we were given - this logic + # exists to deal with some fallout from having both logger_obj and + # logger properties and which of them should be set + + if inspect.ismethod(getattr(logger, 'reopen', None)): + # we have a logger_obj, not a plain logger + # record it that way to ensure it could be corrctly reopened where + # otherwise refefences to the object that has it may be lost and it + # may not occur + self.logger_obj = logger + self._logger = logger.logger + elif inspect.ismethod(getattr(logger, 'info', None)): + # we have a bare logger object based on the sanity check + self._logger = logger + self.logger_obj = BareLoggerAdapter(logger) + else: + raise AssertionError("attempted assignment of unsupported logger") + + def reload_config(self, verbose, skip_log=False, disable_auth_log=False, _config_file=None): """Re-read and parse configuration file. Optional skip_log arg @@ -786,12 +821,6 @@ def reload_config(self, verbose, skip_log=False, disable_auth_log=False, _config_file = _config_file or self.config_file assert _config_file is not None - try: - if self.logger: - self.logger.info('reloading configuration and reopening log') - except: - pass - try: config_file_is_path = os.path.isfile(_config_file) except TypeError: @@ -841,13 +870,17 @@ def reload_config(self, verbose, skip_log=False, disable_auth_log=False, # reopen or initialize logger - if self.logger_obj: + if self._loaded: + self.logger.info('reloading configuration and reopening log') self.logger_obj.reopen() else: - self.logger_obj = Logger(self.loglevel, logfile=self.log_path) + self.logger = Logger(self.loglevel, logfile=self.log_path) + self.logger.info('loading configuration and opening log') + + # record that the object has been populated + self._loaded = True - logger = self.logger_obj.logger - self.logger = logger + logger = self.logger # print "logger initialized (level " + logger_obj.loglevel() + ")" # logger.debug("logger initialized") diff --git a/mig/shared/logger.py b/mig/shared/logger.py index 5b949fca0..a2244f511 100644 --- a/mig/shared/logger.py +++ b/mig/shared/logger.py @@ -67,6 +67,23 @@ def _name_to_format(name): return formats[name] +class BareLoggerAdapter: + """Small wrapper to adapt an arbitrary bare logger to the MiG Logger API""" + + def __init__(self, logger): + self._logger = logger + + @property + def logger(self): + return self._logger + + def reopen(self): + pass + + def shutdown(self): + pass + + class SysLogLibHandler(logging.Handler): """A logging handler that emits messages to syslog.syslog.""" diff --git a/tests/fixture/mig_shared_configuration--new.json b/tests/fixture/mig_shared_configuration--new.json index 2d4edb02c..54b3d9673 100644 --- a/tests/fixture/mig_shared_configuration--new.json +++ b/tests/fixture/mig_shared_configuration--new.json @@ -104,8 +104,6 @@ ], "log_dir": "", "logfile": "", - "logger": null, - "logger_obj": null, "loglevel": "", "lrmstypes": [], "mig_code_base": "", diff --git a/tests/test_mig_shared_configuration.py b/tests/test_mig_shared_configuration.py index 7c9aef06e..655739fcd 100644 --- a/tests/test_mig_shared_configuration.py +++ b/tests/test_mig_shared_configuration.py @@ -34,6 +34,7 @@ from tests.support import MigTestCase, TEST_DATA_DIR, PY2, testmain, \ fixturefile from mig.shared.configuration import Configuration +from mig.shared.logger import null_logger def _is_method(value): @@ -42,7 +43,7 @@ def _is_method(value): def _to_dict(obj): return {k: v for k, v in inspect.getmembers(obj) - if not (k.startswith('__') or _is_method(v))} + if not (k.startswith('_') or k.startswith('logger') or _is_method(v))} class MigSharedConfiguration(MigTestCase): @@ -320,6 +321,9 @@ def test_default_object(self): 'mig_shared_configuration--new', fixture_format='json') configuration = Configuration(None) + # attach a null logger to sidestep the useful logger before available + # assertion which would otherwise blow when the object is inspected + configuration.logger = null_logger("test_configuration") # TODO: the following work-around default values set for these on the # instance that no longer make total sense but fiddling with them # is better as a follow-up.