From 03013b6c76bb2eefe94fb74e0d66e564d706155e Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Mon, 17 Aug 2026 16:28:15 +0200 Subject: [PATCH] refactor(databases): state the databases.csv contract once The schema lived three times in this repo alone: the seed and the backup each spelled out the column list and the semicolon, and _validate_database_value existed twice under one name with different strictness - the seed checked the character set, the backup only checked for empty. A hand-edited file therefore bypassed the only real validation on its way into a dump command. baudolo.databases now holds the columns, the delimiter, the cluster marker, the validator and a read_rows() for consumers that do not want pandas. Values come back verbatim: a password may begin or end with a space, so stripping belongs to the caller that compares, never to the reader. DatabasesCsvError subclasses ValueError, so callers that predate the module keep catching what they caught. The backup's call sites switch over in the next commit, which rewrites them anyway. Co-Authored-By: Claude Opus 5 (1M context) --- src/baudolo/backup/dumps.py | 8 ++- src/baudolo/databases.py | 102 +++++++++++++++++++++++++++++++++++ src/baudolo/seed/__main__.py | 34 +++--------- tests/unit/seed/test_main.py | 21 +++----- tests/unit/test_databases.py | 84 +++++++++++++++++++++++++++++ 5 files changed, 206 insertions(+), 43 deletions(-) create mode 100644 src/baudolo/databases.py create mode 100644 tests/unit/test_databases.py diff --git a/src/baudolo/backup/dumps.py b/src/baudolo/backup/dumps.py index e373425..bac7836 100644 --- a/src/baudolo/backup/dumps.py +++ b/src/baudolo/backup/dumps.py @@ -7,6 +7,8 @@ import sys import pandas from pandas.errors import EmptyDataError +from baudolo.databases import COLUMNS, DELIMITER + from .db import backup_database from .docker import has_tool, image_id @@ -81,7 +83,7 @@ def _empty_databases_df() -> pandas.DataFrame: This allows the backup to continue without DB dumps when the CSV is missing or empty (pandas EmptyDataError). """ - return pandas.DataFrame(columns=["instance", "database", "username", "password"]) + return pandas.DataFrame(columns=list(COLUMNS)) def load_databases_df(csv_path: str) -> pandas.DataFrame: @@ -93,7 +95,9 @@ def load_databases_df(csv_path: str) -> pandas.DataFrame: - Valid CSV -> return dataframe """ try: - return pandas.read_csv(csv_path, sep=";", keep_default_na=False, dtype=str) + return pandas.read_csv( + csv_path, sep=DELIMITER, keep_default_na=False, dtype=str + ) except FileNotFoundError: print( f"WARNING: databases.csv not found: {csv_path}. Continuing without database dumps.", diff --git a/src/baudolo/databases.py b/src/baudolo/databases.py new file mode 100644 index 0000000..62833dd --- /dev/null +++ b/src/baudolo/databases.py @@ -0,0 +1,102 @@ +"""The databases.csv contract: its columns, its delimiter, and what a row means. + +``baudolo-seed`` writes the file, the backup reads it to learn which dumps to +take, and a restore consumer reads it again to replay them. Stating the schema +once keeps a column or a convention added here from being invisible to the +other two. + +Field values are handed back exactly as they stand in the file. A password may +legitimately begin or end with a space, so stripping belongs to the caller that +compares, never to the reader. +""" + +from __future__ import annotations + +import csv +import re +from typing import NamedTuple + +COLUMNS = ("instance", "database", "username", "password") +DELIMITER = ";" +CLUSTER_ROW = "*" + +_NAME_RE = re.compile(r"^[a-zA-Z0-9_][a-zA-Z0-9_-]*$") + + +class DatabasesCsvError(ValueError): + """A row does not match the contract.""" + + +class Row(NamedTuple): + """One databases.csv row, verbatim. + + ``database`` holds :data:`CLUSTER_ROW` when the whole instance is dumped. + """ + + instance: str + database: str + username: str + password: str + + @property + def is_cluster(self) -> bool: + return self.database.strip() == CLUSTER_ROW + + +def validate_database(value: str | None, *, instance: str) -> str: + """The database column of one row, or raise. + + The name reaches a shell as part of the dump command, so it is checked + where it is read as well as where it is written: a file edited by hand + never passed the seed. + + Args: + value: the raw column. + instance: named in the error, so a bad row can be found. + + Raises: + DatabasesCsvError: the column is empty, literally ``nan``, or holds + anything but letters, numbers, ``_`` and ``-``. + """ + text = (value or "").strip() + if not text: + raise DatabasesCsvError( + f"Invalid databases.csv entry for instance '{instance}': column " + f"'database' must be '{CLUSTER_ROW}' or a concrete database name " + "(not empty)." + ) + if text == CLUSTER_ROW: + return CLUSTER_ROW + if text.lower() == "nan": + raise DatabasesCsvError( + f"Invalid databases.csv entry for instance '{instance}': " + "database must not be 'nan'." + ) + if not _NAME_RE.match(text): + raise DatabasesCsvError( + f"Invalid databases.csv entry for instance '{instance}': invalid " + f"database name '{text}'. Allowed: letters, numbers, '_' and '-'." + ) + return text + + +def read_rows(csv_path: str) -> list[Row]: + """Every row of the file in file order, header skipped, blank rows dropped. + + Raises: + DatabasesCsvError: a row holds fewer columns than :data:`COLUMNS`. + """ + rows: list[Row] = [] + with open(csv_path, newline="", encoding="utf-8") as handle: + reader = csv.reader(handle, delimiter=DELIMITER) + next(reader, None) + for raw in reader: + if not any(field.strip() for field in raw): + continue + if len(raw) < len(COLUMNS): + raise DatabasesCsvError( + f"{csv_path} has a row with {len(raw)} column(s), " + f"expected {len(COLUMNS)}" + ) + rows.append(Row(*raw[: len(COLUMNS)])) + return rows diff --git a/src/baudolo/seed/__main__.py b/src/baudolo/seed/__main__.py index 3e754d4..45f15e8 100644 --- a/src/baudolo/seed/__main__.py +++ b/src/baudolo/seed/__main__.py @@ -2,38 +2,16 @@ from __future__ import annotations import argparse import os -import re import sys import pandas as pd from pandas.errors import EmptyDataError -DB_NAME_RE = re.compile(r"^[a-zA-Z0-9_][a-zA-Z0-9_-]*$") - - -def _validate_database_value(value: str | None, *, instance: str) -> str: - v = (value or "").strip() - if v == "": - raise ValueError( - f"Invalid databases.csv entry for instance '{instance}': " - "column 'database' must be '*' or a concrete database name (not empty)." - ) - if v == "*": - return "*" - if v.lower() == "nan": - raise ValueError( - f"Invalid databases.csv entry for instance '{instance}': database must not be 'nan'." - ) - if not DB_NAME_RE.match(v): - raise ValueError( - f"Invalid databases.csv entry for instance '{instance}': " - f"invalid database name '{v}'. Allowed: letters, numbers, '_' and '-'." - ) - return v +from baudolo.databases import COLUMNS, DELIMITER, validate_database def _empty_df() -> pd.DataFrame: - return pd.DataFrame(columns=["instance", "database", "username", "password"]) + return pd.DataFrame(columns=list(COLUMNS)) def check_and_add_entry( @@ -50,13 +28,13 @@ def check_and_add_entry( - database MUST be set - database MUST be '*' or a valid database name """ - database = _validate_database_value(database, instance=instance) + database = validate_database(database, instance=instance) if os.path.exists(file_path): try: df = pd.read_csv( file_path, - sep=";", + sep=DELIMITER, dtype=str, keep_default_na=False, ) @@ -77,11 +55,11 @@ def check_and_add_entry( print("Adding new entry.") new_entry = pd.DataFrame( [[instance, database, username, password]], - columns=["instance", "database", "username", "password"], + columns=list(COLUMNS), ) df = pd.concat([df, new_entry], ignore_index=True) - df.to_csv(file_path, sep=";", index=False) + df.to_csv(file_path, sep=DELIMITER, index=False) def main() -> None: diff --git a/tests/unit/seed/test_main.py b/tests/unit/seed/test_main.py index 6cf31f4..e49f6ca 100644 --- a/tests/unit/seed/test_main.py +++ b/tests/unit/seed/test_main.py @@ -17,20 +17,15 @@ class TestSeedMain(unittest.TestCase): columns=["instance", "database", "username", "password"] ) - def test_validate_database_value_rejects_empty(self) -> None: + def test_a_rejected_database_never_reaches_the_file(self) -> None: with self.assertRaises(ValueError): - seed_main._validate_database_value("", instance="x") - - def test_validate_database_value_accepts_star(self) -> None: - self.assertEqual(seed_main._validate_database_value("*", instance="x"), "*") - - def test_validate_database_value_rejects_nan(self) -> None: - with self.assertRaises(ValueError): - seed_main._validate_database_value("nan", instance="x") - - def test_validate_database_value_rejects_invalid_name(self) -> None: - with self.assertRaises(ValueError): - seed_main._validate_database_value("bad name", instance="x") + seed_main.check_and_add_entry( + file_path="/nonexistent/databases.csv", + instance="x", + database="bad name", + username="u", + password="p", + ) def _mock_df_mask_any(self, *, any_value: bool) -> MagicMock: """ diff --git a/tests/unit/test_databases.py b/tests/unit/test_databases.py new file mode 100644 index 0000000..13b0eee --- /dev/null +++ b/tests/unit/test_databases.py @@ -0,0 +1,84 @@ +"""Contract of databases.csv: the seed writes it, the backup and a restore read it.""" + +from __future__ import annotations + +import tempfile +import unittest +from pathlib import Path + +from baudolo.databases import ( + CLUSTER_ROW, + COLUMNS, + DELIMITER, + DatabasesCsvError, + Row, + read_rows, + validate_database, +) + +HEADER = DELIMITER.join(COLUMNS) + + +def _csv(*lines: str) -> str: + path = Path(tempfile.mkdtemp()) / "databases.csv" + path.write_text("\n".join((HEADER, *lines)) + "\n", encoding="utf-8") + return str(path) + + +class TestValidateDatabase(unittest.TestCase): + def test_a_concrete_name_passes(self) -> None: + self.assertEqual(validate_database("app_db-1", instance="x"), "app_db-1") + + def test_the_cluster_marker_passes(self) -> None: + self.assertEqual(validate_database(CLUSTER_ROW, instance="x"), CLUSTER_ROW) + + def test_an_empty_column_is_rejected(self) -> None: + with self.assertRaises(DatabasesCsvError): + validate_database("", instance="x") + + def test_the_string_nan_is_rejected(self) -> None: + """pandas used to hand back NaN, which wrote a nan.backup.sql.""" + with self.assertRaises(DatabasesCsvError): + validate_database("nan", instance="x") + + def test_a_name_that_could_reach_a_shell_is_rejected(self) -> None: + for hostile in ("bad name", "a;rm -rf /", "$(id)", "a`id`", "a/b"): + with self.subTest(name=hostile), self.assertRaises(DatabasesCsvError): + validate_database(hostile, instance="x") + + def test_the_error_is_a_value_error(self) -> None: + """Callers predating the shared module catch ValueError.""" + with self.assertRaises(ValueError): + validate_database("", instance="x") + + +class TestReadRows(unittest.TestCase): + def test_the_header_is_skipped(self) -> None: + rows = read_rows(_csv(f"pg{DELIMITER}app{DELIMITER}u{DELIMITER}p")) + self.assertEqual(rows, [Row("pg", "app", "u", "p")]) + + def test_a_blank_row_is_dropped(self) -> None: + rows = read_rows(_csv("", f"pg{DELIMITER}app{DELIMITER}u{DELIMITER}p", "")) + self.assertEqual(len(rows), 1) + + def test_a_short_row_is_refused(self) -> None: + with self.assertRaises(DatabasesCsvError): + read_rows(_csv(f"pg{DELIMITER}app{DELIMITER}u")) + + def test_values_arrive_verbatim(self) -> None: + """A password may legitimately begin or end with a space.""" + rows = read_rows(_csv(f"pg{DELIMITER}app{DELIMITER}u{DELIMITER} pw ")) + self.assertEqual(rows[0].password, " pw ") + + def test_a_cluster_row_knows_itself(self) -> None: + rows = read_rows( + _csv( + f"pg{DELIMITER}{CLUSTER_ROW}{DELIMITER}postgres{DELIMITER}p", + f"pg{DELIMITER}app{DELIMITER}u{DELIMITER}p", + ) + ) + self.assertEqual([row.is_cluster for row in rows], [True, False]) + + +if __name__ == "__main__": + unittest.main()