From 0c19bf5d43fa355dbe024f0ad2eb269283fcb93b Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 22 Jul 2026 01:52:04 -0400 Subject: [PATCH 1/2] Library-appropriate configuration: no argv parsing, config discovery Porting the configuration redesign from LMFDB/lmfdb#7069 to psycodict itself, ahead of the 1.0 release. Three checkout-era behaviors were wrong for a general-purpose library: - Configuration() auto-detected "running as a script" and then parsed the host program's command line; an option psycodict did not recognize meant argparse printed psycodict's usage and raised SystemExit. readargs now defaults to False everywhere -- the command line belongs to the host program unless it explicitly hands it over. - The configuration file defaulted to ./config.ini and was created there on first use, sprinkling config files into whatever directory Python first ran in. With no explicit location the file is now discovered: $PSYCODICT_CONFIG, then ./config.ini if it already exists, then ~/.psycodict/config.ini -- and only the latter two explicit locations are auto-created, never the working directory. The secrets file defaults to secrets.ini next to the resolved configuration file. - The default slow_queries.log landed in the working directory; the default name now resolves to a logs directory next to the configuration file (created eagerly, since PostgresDatabase attaches its FileHandler at construction). Any explicitly configured value is used verbatim. Also fixes postgresql_dbname, the one option whose default ignored the defaults dictionary. Downstream is unaffected: LMFDB and seminars both subclass Configuration with their own parser, their own config_file default and explicit readargs, so none of the changed defaults reach them; the log redirect is additionally gated to the builtin parser. The downstream CI jobs verify this mechanically. Co-Authored-By: Claude Fable 5 --- psycodict/config.py | 105 ++++++++++++++++++++++++++---- tests/test_config.py | 151 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 236 insertions(+), 20 deletions(-) diff --git a/psycodict/config.py b/psycodict/config.py index d2a9c6a..383d79a 100644 --- a/psycodict/config.py +++ b/psycodict/config.py @@ -18,6 +18,38 @@ def strbool(s): raise ValueError(s) +def psycodict_home(): + """ + The directory used for psycodict's own files (the configuration file and + the slow-query log) when no explicit location is given: ``~/.psycodict``. + """ + return os.path.join(os.path.expanduser("~"), ".psycodict") + + +def find_config_file(): + """ + The path of the configuration file when none is specified explicitly. + + This is the first of: + + - the ``PSYCODICT_CONFIG`` environment variable; + - ``config.ini`` in the current directory, if it exists; + - ``config.ini`` in ``~/.psycodict``. + + A missing configuration file is created at the resolved location. The + current-directory candidate requires the file to already exist, so + nothing is ever created in the working directory: a fresh setup gets its + configuration under ``~/.psycodict`` (or wherever ``PSYCODICT_CONFIG`` + points). + """ + path = os.environ.get("PSYCODICT_CONFIG") + if path: + return os.path.abspath(os.path.expanduser(path)) + if os.path.exists("config.ini"): + return os.path.abspath("config.ini") + return os.path.join(psycodict_home(), "config.ini") + + class Configuration(): """ This configuration object merges input from the command line and a configuration file. @@ -30,21 +62,32 @@ class Configuration(): - ``parser`` -- an argparse.ArgumentParser instance. If not provided, a default will be created. - ``defaults`` -- a dictionary with default values for the created argument parser. Only used if a parser is not specified. The keys used are: - - ``config_file`` -- the filename for the configuration file + - ``config_file`` -- the filename for the configuration file. If not + given, it is discovered: the ``PSYCODICT_CONFIG`` environment + variable, then ``config.ini`` in the current directory if it exists, + then ``~/.psycodict/config.ini`` (created on first use); see + :func:`find_config_file`. + - ``secrets_file`` -- the filename for the secrets file, whose values + override the configuration file. If not given, ``secrets.ini`` next + to the configuration file. - ``logging_slowcutoff`` -- a float, giving the threshold above which a slow-query warning will be logged - - ``logging_slowlogfile`` -- a filename where slow-query warnings are printed + - ``logging_slowlogfile`` -- a filename where slow-query warnings are + printed. The default value ``slow_queries.log`` is placed in a + ``logs`` directory next to the configuration file; any other value + is used verbatim. - ``postgresql_host`` -- the hostname for the database - ``postgresql_port`` -- an integer, the port to use when connecting to the database - ``postgresql_user`` -- the username when connecting to the database - ``postgresql_password`` -- the password for connecting to the database - - ``writeargstofile`` - a boolean, if config file doesn't exist, it determines if command line arguments are written to the config file instead of the default arguments - - ``readargs`` - a boolean, if determines if command line arguments are read + - ``postgresql_dbname`` -- the name of the database to connect to + - ``writeargstofile`` -- a boolean, if config file doesn't exist, it determines if command line arguments are written to the config file instead of the default arguments + - ``readargs`` -- a boolean (default False), determining whether command + line arguments are read. Leave this off in libraries and applications + with their own command line; pass True in a script whose command line + psycodict should parse. """ - def __init__(self, parser=None, defaults={}, writeargstofile=False, readargs=None): - if readargs is None: - import __main__ as main - # if a file was ran - readargs = hasattr(main, '__file__') + def __init__(self, parser=None, defaults={}, writeargstofile=False, readargs=False): + builtin_parser = parser is None if parser is None: parser = argparse.ArgumentParser(description="Default psycodict parser") @@ -54,16 +97,17 @@ def __init__(self, parser=None, defaults={}, writeargstofile=False, readargs=Non "--config-file", dest="config_file", metavar="FILE", - help="configuration file [default: %(default)s]", - default=defaults.get("config_file", "config.ini"), + help="configuration file [default: $PSYCODICT_CONFIG, " + "./config.ini if present, else ~/.psycodict/config.ini]", + default=defaults.get("config_file"), ) parser.add_argument( "-s", "--secrets-file", dest="secrets_file", metavar="SECRETS", - help="secrets file [default: %(default)s]", - default=defaults.get("secrets_file", "secrets.ini"), + help="secrets file [default: secrets.ini next to the configuration file]", + default=defaults.get("secrets_file"), ) logginggroup = parser.add_argument_group("Logging options:") @@ -123,7 +167,7 @@ def __init__(self, parser=None, defaults={}, writeargstofile=False, readargs=Non dest="postgresql_dbname", metavar="DBNAME", help="PostgreSQL database name [default: %(default)s]", - default="lmfdb", + default=defaults.get("postgresql_dbname", "lmfdb"), ) def sec_opt(key): @@ -141,6 +185,19 @@ def sec_opt(key): # only read config file args = parser.parse_args([]) + # Resolve the file locations. An explicit location (from the + # defaults dictionary or the command line) is used verbatim; with the + # builtin parser both default to None, triggering the discovery in + # find_config_file -- so a missing configuration file is created + # under ~/.psycodict (or $PSYCODICT_CONFIG), never in the working + # directory. + if args.config_file is None: + args.config_file = find_config_file() + if args.secrets_file is None: + args.secrets_file = os.path.join( + os.path.dirname(os.path.abspath(args.config_file)), "secrets.ini" + ) + args_dict = vars(args) default_arguments_dict = vars(parser.parse_args([])) @@ -179,6 +236,11 @@ def sec_opt(key): for opt, val in options.items(): _cfgp.set(sec, opt, str(val)) + # the resolved location may sit in a directory that does not + # exist yet (a fresh ~/.psycodict in particular) + confdir = os.path.dirname(args.config_file) + if confdir: + os.makedirs(confdir, exist_ok=True) with open(args.config_file, "w") as configfile: _cfgp.write(configfile) @@ -238,6 +300,21 @@ def get(section, key): if key not in default_arguments_dict: self.extra_options[key] = val + if builtin_parser: + # The default value of slowlogfile used to be created in the + # working directory; place it in a logs directory next to the + # configuration file instead. Any other value is used verbatim. + # The directory must exist before PostgresDatabase attaches its + # FileHandler, so it is created here. (Callers supplying their + # own parser own their logging semantics; they are not touched.) + logopts = self.options["logging"] + if logopts.get("slowlogfile") == "slow_queries.log": + logdir = os.path.join( + os.path.dirname(os.path.abspath(args.config_file)), "logs" + ) + os.makedirs(logdir, exist_ok=True) + logopts["slowlogfile"] = os.path.join(logdir, "slow_queries.log") + def get_postgresql_default(self): res = dict(self.default_args["postgresql"]) res["port"] = int(res["port"]) diff --git a/tests/test_config.py b/tests/test_config.py index 8f1eef9..f335852 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -284,15 +284,17 @@ def test_command_line_values_beat_the_config_file(paths, monkeypatch): assert config.options["postgresql"]["user"] == "fileuser" -@pytest.mark.parametrize("has_file,expected", [(True, "cli.example.com"), (False, "localhost")]) -def test_readargs_defaults_to_whether_main_is_a_script(paths, monkeypatch, has_file, expected): +def test_readargs_defaults_to_false_even_in_a_script(paths, monkeypatch): + # The command line belongs to the host program: psycodict must not parse + # it unless asked. It used to auto-detect "running as a script" (via + # __main__.__file__) and then argparse would reject the host's own + # options with a SystemExit; pin that both are gone. main = types.ModuleType("fake_main") - if has_file: - main.__file__ = "script.py" + main.__file__ = "script.py" monkeypatch.setitem(sys.modules, "__main__", main) - monkeypatch.setattr(sys, "argv", ["prog", "--postgresql-host", "cli.example.com"]) + monkeypatch.setattr(sys, "argv", ["prog", "--no-such-option", "--postgresql-host", "cli.example.com"]) config = Configuration(defaults=paths) - assert config.options["postgresql"]["host"] == expected + assert config.options["postgresql"]["host"] == "localhost" def test_extra_options_holds_the_arguments_not_stored_in_the_file(paths): @@ -303,6 +305,143 @@ def test_extra_options_holds_the_arguments_not_stored_in_the_file(paths): } +# --------------------------------------------------------------------------- +# file discovery +# --------------------------------------------------------------------------- + + +@pytest.fixture +def isolated(tmp_path, monkeypatch): + """ + An isolated environment for the discovery tests: an empty working + directory, a throwaway HOME, and no PSYCODICT_CONFIG. + + Returns the fake home directory (so ``~/.psycodict`` resolves under it). + """ + cwd = tmp_path / "cwd" + home = tmp_path / "home" + cwd.mkdir() + home.mkdir() + monkeypatch.chdir(cwd) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.delenv("PSYCODICT_CONFIG", raising=False) + return home + + +def test_discovery_falls_back_to_the_psycodict_home(isolated): + config = Configuration(readargs=False) + created = isolated / ".psycodict" / "config.ini" + assert created.exists() + assert config.extra_options["config_file"] == str(created) + assert config.options["postgresql"]["host"] == "localhost" + # ... and nothing was created in the working directory + assert not os.path.exists("config.ini") + + +def test_discovery_uses_an_existing_cwd_config(isolated): + write_ini("config.ini", {"postgresql": {"host": "cwdhost"}}) + config = Configuration(readargs=False) + assert config.options["postgresql"]["host"] == "cwdhost" + assert config.extra_options["config_file"] == os.path.abspath("config.ini") + # the fallback location is not touched + assert not (isolated / ".psycodict").exists() + + +def test_discovery_env_var_beats_the_cwd_config(isolated, tmp_path, monkeypatch): + write_ini("config.ini", {"postgresql": {"host": "cwdhost"}}) + target = tmp_path / "elsewhere" / "psycodict.ini" + monkeypatch.setenv("PSYCODICT_CONFIG", str(target)) + config = Configuration(readargs=False) + # created at the environment location, with the defaults + assert target.exists() + assert config.extra_options["config_file"] == str(target) + assert config.options["postgresql"]["host"] == "localhost" + + +def test_an_explicit_config_file_beats_the_environment(isolated, tmp_path, monkeypatch, paths): + monkeypatch.setenv("PSYCODICT_CONFIG", str(tmp_path / "env.ini")) + config = Configuration(defaults=paths, readargs=False) + assert config.extra_options["config_file"] == paths["config_file"] + assert not (tmp_path / "env.ini").exists() + + +def test_the_default_secrets_file_sits_next_to_the_config_file(isolated): + confdir = isolated / ".psycodict" + confdir.mkdir() + write_ini(str(confdir / "secrets.ini"), {"postgresql": {"password": "s3cret"}}) + config = Configuration(readargs=False) + assert config.extra_options["secrets_file"] == str(confdir / "secrets.ini") + assert config.options["postgresql"]["password"] == "s3cret" + + +# --------------------------------------------------------------------------- +# the slow-query log location +# --------------------------------------------------------------------------- + + +def test_the_default_slowlogfile_lands_in_logs_next_to_the_config(isolated): + config = Configuration(readargs=False) + logdir = isolated / ".psycodict" / "logs" + assert config.options["logging"]["slowlogfile"] == str(logdir / "slow_queries.log") + # PostgresDatabase attaches a FileHandler immediately, so the directory + # must already exist + assert logdir.is_dir() + # the file itself records the default name; the redirect is applied when + # the configuration is read, so editing the file still works + written = read_ini(str(isolated / ".psycodict" / "config.ini")) + assert written.get("logging", "slowlogfile") == "slow_queries.log" + + +def test_an_explicit_slowlogfile_is_respected(isolated, tmp_path): + target = str(tmp_path / "my_slow.log") + config = Configuration( + defaults={"logging_slowlogfile": target}, readargs=False + ) + assert config.options["logging"]["slowlogfile"] == target + + +def test_a_slowlogfile_from_the_config_file_is_respected(isolated, tmp_path): + target = str(tmp_path / "configured.log") + write_ini("config.ini", {"logging": {"slowlogfile": target}}) + config = Configuration(readargs=False) + assert config.options["logging"]["slowlogfile"] == target + assert not os.path.exists("logs") + + +def test_the_default_name_in_a_config_file_is_also_redirected(isolated): + # An existing file that kept the default name gets the same treatment as + # a fresh one: the name is treated as "unconfigured", not as a request + # for a file of that name in the working directory. + write_ini("config.ini", {"logging": {"slowlogfile": "slow_queries.log"}}) + config = Configuration(readargs=False) + expected = os.path.join(os.path.abspath("logs"), "slow_queries.log") + assert config.options["logging"]["slowlogfile"] == expected + + +def test_a_supplied_parser_gets_no_log_redirect(isolated, paths): + # Callers with their own parser own their logging semantics. + parser = make_parser(paths) + parser.add_argument("--slowlogfile", dest="logging_slowlogfile", + default="slow_queries.log") + config = Configuration(parser=parser, readargs=False) + assert config.options["logging"]["slowlogfile"] == "slow_queries.log" + + +# --------------------------------------------------------------------------- +# postgresql_dbname +# --------------------------------------------------------------------------- + + +def test_postgresql_dbname_default_is_honoured(paths): + # dbname used to be the one option whose default ignored the defaults + # dictionary. + config = Configuration( + defaults=dict(paths, postgresql_dbname="mydb"), readargs=False + ) + assert config.options["postgresql"]["dbname"] == "mydb" + assert read_ini(paths["config_file"]).get("postgresql", "dbname") == "mydb" + + # --------------------------------------------------------------------------- # a supplied parser # --------------------------------------------------------------------------- From 9a97eadf20abaef9340c577263be8eddc43ebd52 Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 22 Jul 2026 02:36:31 -0400 Subject: [PATCH 2/2] Survive homeless containers and read-only config directories Two review findings, both reproduced: - os.path.expanduser("~") returns the literal ~ when HOME is unset and the uid has no passwd entry (a common container setup), so the home fallback created ./~/.psycodict in the working directory -- exactly the pollution the discovery exists to prevent. psycodict_home() now requires the expansion to be absolute and otherwise raises a RuntimeError pointing at PSYCODICT_CONFIG. A discoverable configuration (env var or ./config.ini) never consults the home directory, so homeless containers with a configured location work. - The slow-log redirect eagerly created /logs, so merely reading a configuration file in a directory without write access (a system-managed /etc/psycodict/config.ini, say) raised PermissionError. The default log directory is now the first usable of /logs and ~/.psycodict/logs -- probed by creating them, checked writable -- and when neither is usable the plain default name is kept (the historical cwd behavior), so reading a configuration never requires write access beside it. Co-Authored-By: Claude Fable 5 --- psycodict/config.py | 70 ++++++++++++++++++++++++++++++++++++-------- tests/test_config.py | 70 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 13 deletions(-) diff --git a/psycodict/config.py b/psycodict/config.py index 383d79a..7cda7f2 100644 --- a/psycodict/config.py +++ b/psycodict/config.py @@ -22,8 +22,21 @@ def psycodict_home(): """ The directory used for psycodict's own files (the configuration file and the slow-query log) when no explicit location is given: ``~/.psycodict``. + + Raises ``RuntimeError`` when no home directory can be determined -- for + example a container running with ``HOME`` unset and a uid that has no + passwd entry, where ``expanduser`` returns the literal ``~``. Refusing + loudly beats quietly creating a directory named ``~`` in the working + directory. """ - return os.path.join(os.path.expanduser("~"), ".psycodict") + home = os.path.expanduser("~") + if not os.path.isabs(home): + raise RuntimeError( + "Cannot determine a home directory for psycodict's configuration; " + "set the PSYCODICT_CONFIG environment variable (or pass " + "config_file) to choose a location for the configuration file" + ) + return os.path.join(home, ".psycodict") def find_config_file(): @@ -40,7 +53,8 @@ def find_config_file(): current-directory candidate requires the file to already exist, so nothing is ever created in the working directory: a fresh setup gets its configuration under ``~/.psycodict`` (or wherever ``PSYCODICT_CONFIG`` - points). + points). When no home directory can be determined either, this raises + ``RuntimeError`` (see :func:`psycodict_home`). """ path = os.environ.get("PSYCODICT_CONFIG") if path: @@ -73,8 +87,9 @@ class Configuration(): - ``logging_slowcutoff`` -- a float, giving the threshold above which a slow-query warning will be logged - ``logging_slowlogfile`` -- a filename where slow-query warnings are printed. The default value ``slow_queries.log`` is placed in a - ``logs`` directory next to the configuration file; any other value - is used verbatim. + ``logs`` directory next to the configuration file, falling back to + ``~/.psycodict/logs`` when that location is not writable; any other + value is used verbatim. - ``postgresql_host`` -- the hostname for the database - ``postgresql_port`` -- an integer, the port to use when connecting to the database - ``postgresql_user`` -- the username when connecting to the database @@ -303,17 +318,46 @@ def get(section, key): if builtin_parser: # The default value of slowlogfile used to be created in the # working directory; place it in a logs directory next to the - # configuration file instead. Any other value is used verbatim. - # The directory must exist before PostgresDatabase attaches its - # FileHandler, so it is created here. (Callers supplying their - # own parser own their logging semantics; they are not touched.) + # configuration file instead, falling back to ~/.psycodict/logs + # when that location cannot be written (a system-managed + # configuration in a read-only directory, say -- reading a + # configuration must not require write access beside it). Any + # other value is used verbatim. (Callers supplying their own + # parser own their logging semantics; they are not touched.) logopts = self.options["logging"] if logopts.get("slowlogfile") == "slow_queries.log": - logdir = os.path.join( - os.path.dirname(os.path.abspath(args.config_file)), "logs" - ) - os.makedirs(logdir, exist_ok=True) - logopts["slowlogfile"] = os.path.join(logdir, "slow_queries.log") + logdir = self._default_log_dir(args.config_file) + if logdir is not None: + logopts["slowlogfile"] = os.path.join(logdir, "slow_queries.log") + + @staticmethod + def _default_log_dir(config_file): + """ + The directory for the default slow-query log: the first of + ``/logs`` and ``~/.psycodict/logs`` that exists or can be + created, and is writable. + + The directory must exist and be writable before PostgresDatabase + attaches its FileHandler, so the candidates are probed by creating + them. Returns None when neither candidate is usable (the plain + default name is then kept, preserving the historical behavior of + writing in the working directory). + """ + candidates = [ + os.path.join(os.path.dirname(os.path.abspath(config_file)), "logs") + ] + try: + candidates.append(os.path.join(psycodict_home(), "logs")) + except RuntimeError: + pass + for candidate in candidates: + try: + os.makedirs(candidate, exist_ok=True) + except OSError: + continue + if os.access(candidate, os.W_OK | os.X_OK): + return candidate + return None def get_postgresql_default(self): res = dict(self.default_args["postgresql"]) diff --git a/tests/test_config.py b/tests/test_config.py index f335852..b039c02 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -427,6 +427,76 @@ def test_a_supplied_parser_gets_no_log_redirect(isolated, paths): assert config.options["logging"]["slowlogfile"] == "slow_queries.log" +needs_permissions = pytest.mark.skipif( + hasattr(os, "geteuid") and os.geteuid() == 0, + reason="root ignores directory permissions", +) + + +@pytest.fixture +def read_only_config(isolated, tmp_path, monkeypatch): + """ + A readable configuration file in a directory without write access, the + shape of a system-managed /etc/psycodict/config.ini. Restores the + permissions afterward so tmp_path can be cleaned up. + """ + confdir = tmp_path / "etc" + confdir.mkdir() + write_ini(str(confdir / "config.ini"), {"postgresql": {"host": "etchost"}}) + confdir.chmod(0o555) + monkeypatch.setenv("PSYCODICT_CONFIG", str(confdir / "config.ini")) + yield confdir + confdir.chmod(0o755) + + +@needs_permissions +def test_a_config_in_a_read_only_directory_is_readable(isolated, read_only_config): + # Reading a configuration must not require write access beside it; the + # default slow log falls back to ~/.psycodict/logs. + config = Configuration(readargs=False) + assert config.options["postgresql"]["host"] == "etchost" + expected = os.path.join(str(isolated), ".psycodict", "logs", "slow_queries.log") + assert config.options["logging"]["slowlogfile"] == expected + assert os.path.isdir(os.path.dirname(expected)) + + +@needs_permissions +def test_no_usable_log_directory_keeps_the_plain_name(isolated, read_only_config, monkeypatch): + # With the config directory read-only and no home either, the redirect + # gives up and the historical cwd-relative default survives. + monkeypatch.setattr(os.path, "expanduser", lambda path: path) + config = Configuration(readargs=False) + assert config.options["logging"]["slowlogfile"] == "slow_queries.log" + + +# --------------------------------------------------------------------------- +# missing home directory +# --------------------------------------------------------------------------- + + +def test_no_home_directory_is_a_clear_error_not_a_tilde_directory(isolated, monkeypatch): + # In a container with HOME unset and a uid without a passwd entry, + # expanduser returns the literal ~. The home fallback must refuse + # loudly instead of creating ./~/.psycodict in the working directory. + monkeypatch.setattr(os.path, "expanduser", lambda path: path) + with pytest.raises(RuntimeError, match="PSYCODICT_CONFIG"): + Configuration(readargs=False) + assert not os.path.exists("~") + + +def test_a_cwd_config_needs_no_home_directory(isolated, monkeypatch): + # The error is confined to the home fallback: with a discoverable + # configuration the homeless container works fine (and the log lands + # next to the config, not in a literal ~). + write_ini("config.ini", {"postgresql": {"host": "cwdhost"}}) + monkeypatch.setattr(os.path, "expanduser", lambda path: path) + config = Configuration(readargs=False) + assert config.options["postgresql"]["host"] == "cwdhost" + expected = os.path.join(os.path.abspath("logs"), "slow_queries.log") + assert config.options["logging"]["slowlogfile"] == expected + assert not os.path.exists("~") + + # --------------------------------------------------------------------------- # postgresql_dbname # ---------------------------------------------------------------------------