[FIX] common: close database connections, fix helpers, isolate the test suite

odev/_version.py

@@ -22,4 +22,4 @@
 # or merged change.
 # ------------------------------------------------------------------------------
 
-__version__ = "4.31.1"
+__version__ = "4.31.2"

odev/common/connectors/postgres.py

@@ -314,27 +314,31 @@ def create_table(self, table: str, columns: Mapping[str, str]) -> bool:
         return bool(self.query(f"CREATE TABLE IF NOT EXISTS {table} ({sql_columns})"))
 
     def columns_exist(self, table: str, columns: list[str]) -> list[str]:
-        """Check whether a column exists in a table.
+        """List the columns missing from a table, among the ones requested.
 
         :param table: The name of the table to check.
-        :param columns: The name of the column to check.
-        :return: Whether the column exists.
+        :param columns: The names of the columns to look for.
+        :return: The requested columns that do not exist in the table, in the order they were requested.
         :rtype: list[str]
         """
         if not isinstance(columns, list):
             raise TypeError("Columns should be a list of strings")
 
+        if not columns:
+            return []
+
         results = self.query(
             f"""
             SELECT column_name
             FROM information_schema.columns
-            WHERE table_name = '{table}' AND column_name IN ({",".join([f"'{c}'" for c in columns])})
+            WHERE table_name = '{table}' AND column_name IN ({",".join([f"'{column}'" for column in columns])})
             """,
         )
 
-        if results and isinstance(results, list) and columns:
-            return [c for c in columns if c not in [r[0] for r in results]]
-        return []
+        # An empty result means none of the requested columns exist, not that none are missing.
+        existing = {result[0] for result in results} if isinstance(results, list) else set()
+
+        return [column for column in columns if column not in existing]
 
     def create_column(self, table: str, column: str, attributes: str) -> bool:
         """Create a column in a table.

odev/common/databases/local.py

@@ -23,6 +23,7 @@
 from zipfile import ZipFile
 
 from packaging.version import Version
+from psycopg2 import OperationalError
 
 from odev.common import bash, progress, string
 from odev.common.connectors import GitConnector, GitWorktree, PostgresConnector
@@ -60,9 +61,6 @@
 class LocalDatabase(PostgresConnectorMixin, Database):
     """Class for manipulating PostgreSQL (local) databases."""
 
-    connector: PostgresConnector | None = None
-    """The PostgreSQL connector of the database."""
-
     _whitelisted: bool = False
     """Whether the database is whitelisted and should not be removed automatically."""
 
@@ -101,11 +99,11 @@ def __init__(self, name: str):
             self.whitelisted = info is not None and info.whitelisted
 
     def __enter__(self):
-        self.connector = self.psql(self.name).__enter__()  # type: ignore [assignment]
+        self._enter_connector(self.name)
         return self
 
     def __exit__(self, *args):
-        self.psql(self.name).__exit__(*args)
+        self._exit_connector(*args)
 
     @property
     def rpc_port(self):
@@ -116,8 +114,20 @@ def is_odoo(self) -> bool:
         if not self.exists:
             return False
 
-        with self:
-            return self.table_exists("ir_module_module")
+        try:
+            with self:
+                return self.table_exists("ir_module_module")
+
+        except OperationalError:
+            # Any process can drop a database at any time, including between the check above and this
+            # connection. Listing databases inspects each of them in turn and must not fail because one
+            # went away in the meantime; anything else is a genuine connection error. The check has to
+            # reach the server rather than the cache, which is what said the database was still there.
+            with self.psql() as psql, psql.nocache():
+                if psql.database_exists(self.name):
+                    raise
+
+            return False
 
     @property
     def venv(self) -> PythonEnv:
@@ -637,7 +647,9 @@ def signal_handler_progress(
 
         tracker.stop()
 
-        if self.connector is not None:
+        # The attribute holds the connector class until a block connects for the first time, so the check
+        # is on the type rather than on `None`, as in `drop`.
+        if isinstance(self.connector, PostgresConnector):
             self.connector.invalidate_cache()
 
     def _replace_filestore(self, source_dir: Path) -> None:

odev/common/mixins/connectors/postgres.py

@@ -10,6 +10,38 @@ class PostgresConnectorMixin(ConnectorMixin):
     _connector_class: type[PostgresConnector] = PostgresConnector  # type: ignore [assignment]
     connector: PostgresConnector  # type: ignore [assignment]
 
+    _connection_depth: int = 0
+    """Number of nested blocks currently sharing the connector."""
+
     def psql(self, name: str = "postgres") -> PostgresConnector:
         """Return a PostgreSQL connector to the selected database."""
         return self._connector_class(name)
+
+    def _enter_connector(self, name: str) -> PostgresConnector:
+        """Connect to a database, or join the connection an enclosing block already opened.
+
+        `ensure_connected` runs every database method inside its own block, and those blocks nest, so
+        opening a connection per block would mean a new PostgreSQL backend for each call.
+
+        :param name: The name of the database to connect to.
+        :return: The connector the caller should use.
+        :rtype: PostgresConnector
+        """
+        # A held connection that was closed in the meantime is replaced rather than reused: `drop`
+        # disconnects explicitly, and the datastore holds its own connection across such a call. At depth
+        # zero the check is skipped, `connector` still being the connector class rather than an instance.
+        if not self._connection_depth or not self.connector.connected:
+            # Narrows what the base mixin types as a `Connector`, deliberately: everything reached through
+            # this mixin talks to PostgreSQL, and its callers query rather than use the base interface.
+            self.connector = self.psql(name).__enter__()  # pyright: ignore[reportIncompatibleVariableOverride]
+
+        self._connection_depth += 1
+
+        return self.connector
+
+    def _exit_connector(self, *args) -> None:
+        """Close the connection once the outermost block sharing it is done with it."""
+        self._connection_depth -= 1
+
+        if not self._connection_depth:
+            self.connector.__exit__(*args)

odev/common/odev.py

@@ -25,7 +25,6 @@
     Any,
     ClassVar,
     Generic,
-    Literal,
     NamedTuple,
     TypedDict,
     cast,
@@ -197,17 +196,21 @@ class Odev(Generic[CommandType]):
     _command_stack: list[CommandType] = []
     """Stack of current commands being executed. Last command in list is the one currently running."""
 
-    def __init__(self, test: bool = False):
+    def __init__(self, test: bool = False, name: str | None = None):
         """Initialize the framework.
 
         :param test: Whether the framework is being initialized for testing purposes
+        :param name: Namespace of the framework, overriding the one inferred from the test mode
         """
         self.start_time = monotonic()
         """Time when the framework was started."""
 
         self.in_test_mode = test
         """Whether the framework is in testing mode."""
 
+        self._name = name
+        """Namespace explicitly assigned to this instance, if any."""
+
         self.commands = CommandRegistry(self)
         """Collection of existing commands, imported on demand."""
 
@@ -225,8 +228,15 @@ def git(self) -> GitConnector:
         return GitConnector(f"{self.path.parent.name}/{self.path.name}", self.path)
 
     @property
-    def name(self) -> Literal["odev", "odev-test"]:
-        """Name of the framework."""
+    def name(self) -> str:
+        """Name of the framework, and the namespace of everything it owns.
+
+        The configuration file and the datastore database are both named after it, so an instance given an
+        explicit name works on its own resources rather than on the ones of the user.
+        """
+        if self._name is not None:
+            return self._name
+
         return "odev" if not self.in_test_mode else "odev-test"
 
     @property

odev/common/postgres.py

@@ -22,24 +22,32 @@ class PostgresDatabase(PostgresConnectorMixin):
     connector: PostgresConnector
     """Instance of the connector to the database engine."""
 
-    tables: MutableMapping[str, "PostgresTable"] = {}
-    """Mapping of tables in the database."""
-
     def __init__(self, name: str):
         """Initialize the database."""
         super().__init__()
 
         self.name: str = name
         """The name of the database."""
 
+        self.tables: MutableMapping[str, PostgresTable] = {}
+        """Mapping of tables in the database, keyed by table name."""
+
         self.prepare_database()
 
     def __enter__(self):
-        self.connector = self._connector_class(self.name).__enter__()
+        self._enter_connector(self.name)
         return self
 
     def __exit__(self, *args):
-        self._connector_class(self.name).__exit__(*args)
+        self._exit_connector(*args)
+
+    def hold_connection(self) -> None:
+        """Keep the connection open for the lifetime of this object rather than for that of a block.
+
+        Meant for a database read often enough that reconnecting for each operation is wasteful, and
+        long-lived enough that holding a backend for it is not.
+        """
+        self._enter_connector(self.name)
 
     def __repr__(self):
         """Return the representation of the database."""

odev/common/store/datastore.py

@@ -18,6 +18,12 @@ class DataStore(PostgresDatabase):
 
     def __init__(self, name: str = "odev"):
         super().__init__(name)
+
+        # Every command reads the store, and it lives as long as the process does: its connection is
+        # opened once here and held, rather than reopened for each of the reads a single run makes.
+        # Holding it from this point also covers the tables prepared below.
+        self.hold_connection()
+
         self.databases = DatabaseStore(self)
         self.history = HistoryStore(self)
         self.secrets = SecretStore(self)

odev/common/string.py

@@ -93,11 +93,13 @@ def dedent(text: str, dedent: int = 0) -> str:
 def min_indent(text: str) -> int:
     """Return the smallest indentation in a text.
 
+    A text without any non-blank line has no indentation to speak of and returns zero.
+
     :param text: The text to get the minimum indentation from.
     :return: The minimum indentation of the text.
     :rtype: int
     """
-    return min(len(line) - len(line.lstrip()) for line in text.splitlines() if line.strip())
+    return min((len(line) - len(line.lstrip()) for line in text.splitlines() if line.strip()), default=0)
 
 
 def bytes_size(size: int | float) -> str:
@@ -281,18 +283,27 @@ def ago(date: datetime.datetime) -> str:
 def quote(string: str, dirty_only: bool = False, force_single: bool = False) -> str:
     """Quote a string.
 
+    The quote character is chosen so that it does not appear in the string: a string containing single
+    quotes is wrapped in double quotes, and the other way around.
+
+    **Warning** This helper picks a delimiter, it does not escape. A string containing both quote
+    characters, or containing the delimiter imposed by `force_single`, cannot be represented and comes
+    back with an unbalanced delimiter. Do not use it to interpolate untrusted input into SQL or shell
+    commands; use `shlex.quote` or query parameters instead.
+
     :param string: The string to quote.
     :param dirty_only: Do not quote strings that have no quotes to begin with.
     :param force_single: Force single quotes.
     :return: The quoted string.
     :rtype: str
     """
-    index = max(string.find(char) for char in ("'", '"'))
+    contains_single = "'" in string
+    contains_double = '"' in string
 
-    if dirty_only and index == -1:
+    if dirty_only and not contains_single and not contains_double:
         return string
 
-    double = not force_single and (index == -1 or string[index] == "'")
+    double = not force_single and (contains_single or not contains_double)
     return f'"{string}"' if double else f"'{string}'"
 
 

odev/common/version.py

@@ -86,7 +86,9 @@ def __repr__(self) -> str:
 
     def __bool__(self) -> bool:
         """Return True if the version is not empty."""
-        return bool(self.major or self.minor or self.module or self.master)
+        # `module` is padded to `MIN_VERSION_LENGTH` and is therefore never an empty tuple:
+        # it has to be tested on its values rather than on its own truthiness.
+        return bool(self.major or self.minor or any(self.module) or self.master)
 
     @property
     def major(self) -> int:
@@ -130,7 +132,8 @@ def _cmpkey(master: bool, major: int, minor: int, module: tuple, enterprise: boo
     # Saas versions should sort after non-saas versions
     _saas = int(saas)
 
-    # Master versions should sort before non-master versions
+    # Master is the development version and therefore sorts after every numbered version, hence it comes
+    # first in the key so that it outweighs the major number.
     _master = int(master)
 
     return _master, major, minor, _module, enterprise, _saas

odev/setup/symlink.py

@@ -15,6 +15,27 @@
 
 PRIORITY = 10
 
+LOCAL_BIN_PATH = Path("~/.local/bin").expanduser()
+"""Directory holding the executables of the current user."""
+
+SYSTEM_BIN_PATH = Path("/usr/local/bin")
+"""Directory holding the executables of every user, used when the local one is not in `PATH`."""
+
+
+# --- Values -------------------------------------------------------------------
+
+
+def link_path() -> Path:
+    """Return the path the `odev` command should be linked from.
+
+    :return: The link to create, in the local binaries directory of the user when it is in `PATH`, in the
+        system-wide one otherwise.
+    :rtype: Path
+    """
+    known_env_paths = getenv("PATH", "").split(":")
+
+    return (LOCAL_BIN_PATH if str(LOCAL_BIN_PATH) in known_env_paths else SYSTEM_BIN_PATH) / "odev"
+
 
 # --- Setup --------------------------------------------------------------------
 
@@ -25,26 +46,24 @@ def setup(odev: Odev) -> None:
     :param config: Odev configuration
     """
     main_path = Path(__file__).parents[2] / "odev.sh"
-    known_env_paths = getenv("PATH", "").split(":")
-    local_path = Path("~/.local/bin").expanduser()
-    link_path = (local_path if str(local_path) in known_env_paths else Path("/usr/local/bin")) / "odev"
+    command_path = link_path()
 
-    if not link_path.parent.exists():
-        logger.debug(f"Directory {link_path.parent} does not exist, creating it")
-        link_path.parent.mkdir(parents=True)
+    if not command_path.parent.exists():
+        logger.debug(f"Directory {command_path.parent} does not exist, creating it")
+        command_path.parent.mkdir(parents=True)
 
-    if link_path.exists() or link_path.is_symlink():
-        logger.warning(f"Symlink path {link_path} already exists, this is used to run odev as a shell command")
+    if command_path.exists() or command_path.is_symlink():
+        logger.warning(f"Symlink path {command_path} already exists, this is used to run odev as a shell command")
 
         if console.confirm("Would you like to overwrite it?"):
-            logger.debug(f"Removing symlink path {link_path}")
-            bash.execute(f"rm {link_path}", sudo=True)
+            logger.debug(f"Removing symlink path {command_path}")
+            bash.execute(f"rm {command_path}", sudo=True)
 
-    if not link_path.exists() and not link_path.is_symlink():
-        logger.debug(f"Creating symlink from {link_path} to {main_path}")
-        bash.execute(f"ln -s {main_path} {link_path}", sudo=True)
+    if not command_path.exists() and not command_path.is_symlink():
+        logger.debug(f"Creating symlink from {command_path} to {main_path}")
+        bash.execute(f"ln -s {main_path} {command_path}", sudo=True)
         logger.info("Symlink created")
 
-    for path in (main_path, link_path):
+    for path in (main_path, command_path):
         logger.debug(f"Checking execute permissions for {path}")
         path.chmod(path.stat().st_mode | stat.S_IXUSR)