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()