From 3ea8eb81f60f8202f246bc2e1ebef6c34d76b27e Mon Sep 17 00:00:00 2001
From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com>
Date: Sun, 11 Oct 2026 16:47:32 -0600
Subject: [PATCH] Model reprs read only loaded state (#819)

A repr runs inside log calls. Reading a relationship or an expired column there runs a query, and
on an object whose session is gone it raises DetachedInstanceError instead of logging. Archive's
repr also stat'ed its SingleFile file on disk.

`wrolpi.common.loaded()` follows attributes through state that is already loaded (primary keys
from the identity key) and never runs SQL. Every model repr uses it; with everything loaded the
output is unchanged, except Video and Archive now show the FileGroup's primary path.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
---
 modules/archive/models.py      | 14 +++----
 modules/docs/models.py         |  9 +++--
 modules/repos/models.py        |  5 ++-
 modules/videos/models.py       | 14 +++----
 modules/zim/models.py          | 25 ++++++------
 wrolpi/collections/models.py   |  9 +++--
 wrolpi/common.py               | 25 ++++++++++++
 wrolpi/downloader.py           | 15 +++----
 wrolpi/files/models.py         |  9 +++--
 wrolpi/tags.py                 | 10 +++--
 wrolpi/test/test_model_repr.py | 71 ++++++++++++++++++++++++++++++++++
 11 files changed, 154 insertions(+), 52 deletions(-)
 create mode 100644 wrolpi/test/test_model_repr.py

diff --git a/modules/archive/models.py b/modules/archive/models.py
index 0931f1a6..4296dea6 100644
--- a/modules/archive/models.py
+++ b/modules/archive/models.py
@@ -9,8 +9,8 @@ from sqlalchemy.orm import Mapped, relationship, Session, joinedload
 
 from wrolpi import dates
 from wrolpi.collections import Collection
-from wrolpi.common import ModelHelper, Base, logger, get_title_from_html, get_wrolpi_config, get_media_directory, \
-    strip_surrogates
+from wrolpi.common import loaded, ModelHelper, Base, logger, get_title_from_html, get_wrolpi_config, \
+    get_media_directory, strip_surrogates
 from wrolpi.dates import now
 from wrolpi.db import serializer
 from wrolpi.errors import UnknownArchive
@@ -60,11 +60,11 @@ class Archive(Base, ModelHelper):
         return d
 
     def __repr__(self):
-        domain_name = self.domain if self.collection else None
-        if domain_name:
-            return f'<Archive id={self.id} url={self.file_group.url} singlefile={repr(str(self.singlefile_path))} ' \
-                   f'domain={domain_name}>'
-        return f'<Archive id={self.id} url={self.file_group.url} singlefile={repr(str(self.singlefile_path))}>'
+        id_, url = loaded(self, 'id'), loaded(self, 'file_group', 'url')
+        path = repr(str(loaded(self, 'file_group', 'primary_path')))
+        if domain_name := loaded(self, 'collection', 'name'):
+            return f'<Archive id={id_} url={url} path={path} domain={domain_name}>'
+        return f'<Archive id={id_} url={url} path={path}>'
 
     @property
     def domain(self) -> str | None:
diff --git a/modules/docs/models.py b/modules/docs/models.py
index 84cdc562..793782fc 100644
--- a/modules/docs/models.py
+++ b/modules/docs/models.py
@@ -4,7 +4,7 @@ from typing import Optional
 from sqlalchemy import Column, Integer, BigInteger, ForeignKey, String, Text, Index, or_
 from sqlalchemy.orm import Mapped, relationship, Session, joinedload
 
-from wrolpi.common import ModelHelper, Base
+from wrolpi.common import loaded, ModelHelper, Base
 from wrolpi.db import get_db_session, serializer
 from wrolpi.files.models import FileGroup
 
@@ -62,8 +62,8 @@ class Doc(ModelHelper, Base):
                             passive_deletes=True)
 
     def __repr__(self):
-        path = str(self.file_group.primary_path) if self.file_group else 'None'
-        return f'<Doc id={self.id} path={repr(path)} file_group_id={self.file_group_id}>'
+        path = str(loaded(self, 'file_group', 'primary_path'))
+        return f'<Doc id={loaded(self, "id")} path={repr(path)} file_group_id={loaded(self, "file_group_id")}>'
 
     @staticmethod
     def json_options() -> tuple:
@@ -166,4 +166,5 @@ class DocSection(ModelHelper, Base):
     doc = relationship('Doc', back_populates='sections')
 
     def __repr__(self):
-        return f'<DocSection id={self.id} doc_id={self.doc_id} kind={self.kind} ordinal={self.ordinal}>'
+        return f'<DocSection id={loaded(self, "id")} doc_id={loaded(self, "doc_id")} kind={loaded(self, "kind")} ' \
+               f'ordinal={loaded(self, "ordinal")}>'
diff --git a/modules/repos/models.py b/modules/repos/models.py
index 5022f8dc..3c3c9ad7 100644
--- a/modules/repos/models.py
+++ b/modules/repos/models.py
@@ -5,7 +5,7 @@ from sqlalchemy import Column, Integer, String, ForeignKey, BigInteger, Text, Bo
 from sqlalchemy.orm import relationship, Session, joinedload, selectinload, deferred
 
 from wrolpi.collections.models import Collection
-from wrolpi.common import Base, ModelHelper, get_relative_to_media_directory
+from wrolpi.common import loaded, Base, ModelHelper, get_relative_to_media_directory
 from wrolpi.dates import TZDateTime
 from wrolpi.db import serializer
 from wrolpi.downloader import Download
@@ -57,7 +57,8 @@ class Repository(ModelHelper, Base):
                                 uselist=False)
 
     def __repr__(self):
-        return f'<Repository id={self.id} url={self.url} directory={self.directory}>'
+        return f'<Repository id={loaded(self, "id")} url={loaded(self, "url")} ' \
+               f'directory={loaded(self, "collection", "directory")}>'
 
     @property
     def name(self) -> Optional[str]:
diff --git a/modules/videos/models.py b/modules/videos/models.py
index 60f31c61..3637cec8 100644
--- a/modules/videos/models.py
+++ b/modules/videos/models.py
@@ -8,7 +8,7 @@ from sqlalchemy.orm import Mapped, relationship, Session, deferred, undefer, joi
 
 from modules.videos.errors import UnknownVideo, UnknownChannel
 from wrolpi.captions import read_captions, read_captions_with_timestamps
-from wrolpi.common import Base, ModelHelper, logger, get_media_directory, get_relative_to_media_directory, \
+from wrolpi.common import loaded, Base, ModelHelper, logger, get_media_directory, get_relative_to_media_directory, \
     background_task
 from wrolpi.db import get_db_curs, get_db_session, session_curs, serializer
 from wrolpi.collections.models import Collection
@@ -53,11 +53,10 @@ class Video(ModelHelper, Base):
     file_group: Mapped[FileGroup] = relationship('FileGroup')
 
     def __repr__(self):
-        v = None
-        if self.video_path:
-            v = repr(str(self.video_path.relative_to(get_media_directory())))
-        return f'<Video id={self.id} title={repr(self.file_group.title)} path={v} channel={self.channel_id} ' \
-               f'source_id={repr(self.source_id)}>'
+        title = loaded(self, 'file_group', 'title')
+        path = repr(str(loaded(self, 'file_group', 'primary_path')))
+        return f'<Video id={loaded(self, "id")} title={repr(title)} path={path} channel={loaded(self, "channel_id")} ' \
+               f'source_id={repr(loaded(self, "source_id"))}>'
 
     @staticmethod
     def json_options() -> tuple:
@@ -665,7 +664,8 @@ class Channel(ModelHelper, Base):
         return [d for d in self.collection.downloads if d.frequency] if self.collection else []
 
     def __repr__(self):
-        return f'<Channel id={self.id} name={repr(self.name)} url={self.url} directory={self.directory}>'
+        return f'<Channel id={loaded(self, "id")} name={repr(loaded(self, "collection", "name"))} ' \
+               f'url={loaded(self, "url")} directory={loaded(self, "collection", "directory")}>'
 
     @property
     def name(self) -> str | None:
diff --git a/modules/zim/models.py b/modules/zim/models.py
index 9489fdd9..441d5829 100644
--- a/modules/zim/models.py
+++ b/modules/zim/models.py
@@ -12,7 +12,7 @@ from sqlalchemy.orm.exc import NoResultFound  # noqa
 
 from modules.zim.errors import UnknownZimEntry, UnknownZimTagEntry, UnknownZim
 from wrolpi import dates, tags
-from wrolpi.common import Base, logger, get_relative_to_media_directory, ModelHelper
+from wrolpi.common import loaded, Base, logger, get_relative_to_media_directory, ModelHelper
 from wrolpi.dates import TZDateTime
 from wrolpi.db import session_curs, serializer
 from wrolpi.downloader import Download, download_manager
@@ -78,7 +78,8 @@ class Zim(Base, ModelHelper):
     auto_search = Column(Boolean, default=True)
 
     def __repr__(self):
-        return f'<Zim id={self.id} file_group_id={self.file_group_id} path={self.path}>'
+        return f'<Zim id={loaded(self, "id")} file_group_id={loaded(self, "file_group_id")} ' \
+               f'path={loaded(self, "path")}>'
 
     @staticmethod
     def json_options() -> tuple:
@@ -366,13 +367,11 @@ class TagZimEntry(Base):
     created_at: Mapped[datetime] = Column(TZDateTime, default=dates.now)
 
     def __repr__(self):
-        tag = self.tag_id
-        if self.tag:
-            tag = self.tag.name
-        zim = self.zim_id
-        if self.zim:
-            zim = get_relative_to_media_directory(self.zim.path)
-        return f'<TagZimEntry {tag=} zim={str(zim)} zim_entry={self.zim_entry}>'
+        tag = loaded(self, 'tag', 'name') or loaded(self, 'tag_id')
+        zim = loaded(self, 'zim_id')
+        if zim_path := loaded(self, 'zim', 'path'):
+            zim = get_relative_to_media_directory(zim_path)
+        return f'<TagZimEntry {tag=} zim={str(zim)} zim_entry={loaded(self, "zim_entry")}>'
 
     @staticmethod
     def get_by_primary_keys(tag_id: int, zim_id: int, zim_entry: str, session: Session):
@@ -391,10 +390,10 @@ class ZimSubscription(Base):
     download: Mapped[Download] = relationship('Download', primaryjoin='ZimSubscription.download_id==Download.id')
 
     def __repr__(self):
-        name = self.name
-        language = self.language
-        download_id = self.download_id
-        return f'<ZimSubscription id={self.id} {name=} {language=} {download_id=}>'
+        name = loaded(self, 'name')
+        language = loaded(self, 'language')
+        download_id = loaded(self, 'download_id')
+        return f'<ZimSubscription id={loaded(self, "id")} {name=} {language=} {download_id=}>'
 
     @staticmethod
     def json_options() -> tuple:
diff --git a/wrolpi/collections/models.py b/wrolpi/collections/models.py
index a18829c9..cb6041a3 100644
--- a/wrolpi/collections/models.py
+++ b/wrolpi/collections/models.py
@@ -10,7 +10,7 @@ from sqlalchemy import Column, Integer, String, ForeignKey, Text, DateTime, Inde
 from sqlalchemy.orm import Mapped, relationship, Session, joinedload, selectinload
 
 from wrolpi import flags
-from wrolpi.common import Base, ModelHelper, logger, get_media_directory, get_relative_to_media_directory, \
+from wrolpi.common import loaded, Base, ModelHelper, logger, get_media_directory, get_relative_to_media_directory, \
     unique_by_predicate, TRACE_LEVEL
 from wrolpi.db import serializer
 from wrolpi.downloader import Download, save_downloads_config
@@ -141,7 +141,8 @@ class Collection(ModelHelper, Base):
     )
 
     def __repr__(self):
-        return f'<Collection id={self.id} name={repr(self.name)} directory={self.directory} items={self.item_count}>'
+        return f'<Collection id={loaded(self, "id")} name={repr(loaded(self, "name"))} ' \
+               f'directory={loaded(self, "directory")} items={loaded(self, "item_count")}>'
 
     @property
     def tag_name(self) -> str | None:
@@ -1265,8 +1266,8 @@ class CollectionItem(ModelHelper, Base):
     )
 
     def __repr__(self):
-        return (f'<CollectionItem collection={self.collection_id} kind={self.item_kind} '
-                f'position={self.position}>')
+        return (f'<CollectionItem collection={loaded(self, "collection_id")} kind={loaded(self, "item_kind")} '
+                f'position={loaded(self, "position")}>')
 
     @staticmethod
     def json_options() -> tuple:
diff --git a/wrolpi/common.py b/wrolpi/common.py
index febbd83b..58beaf6c 100644
--- a/wrolpi/common.py
+++ b/wrolpi/common.py
@@ -35,6 +35,7 @@ import yaml
 from aiohttp import ClientResponse, ClientSession
 from bs4 import BeautifulSoup
 from selenium import webdriver
+from sqlalchemy import inspect as sa_inspect
 from sqlalchemy import types
 from sqlalchemy.orm import declarative_base
 from sqlalchemy.orm import Session
@@ -338,6 +339,30 @@ __all__ = [
 Base = declarative_base()
 
 
+def loaded(obj, *path: str):
+    """Follow `path` (attribute names) from `obj` through state that is already loaded: never runs SQL.
+
+    For `__repr__`, which runs inside log calls on objects that may be expired or detached.  A primary key
+    comes from the identity key, which survives both.  Anything not loaded is None.
+
+    >>> loaded(video, 'file_group', 'title')
+    """
+    for name in path:
+        if obj is None:
+            return None
+        if name in obj.__dict__:
+            obj = obj.__dict__[name]
+            continue
+        state = sa_inspect(obj, raiseerr=False)
+        value = None
+        if state is not None and state.identity:
+            keys = [state.mapper.get_property_by_column(i).key for i in state.mapper.primary_key]
+            if name in keys:
+                value = state.identity[keys.index(name)]
+        obj = value
+    return obj
+
+
 class ModelHelper:
 
     # Columns `dict()` leaves out, such as large deferred columns that no response needs.
diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py
index 256d18b1..ff6a5cf8 100644
--- a/wrolpi/downloader.py
+++ b/wrolpi/downloader.py
@@ -31,7 +31,7 @@ from sqlalchemy.orm import Mapped, Session, relationship
 from wrolpi import flags
 from wrolpi.api_utils import api_app, perpetual_signal
 from wrolpi.cmd import which, run_command, CommandResult
-from wrolpi.common import Base, ModelHelper, logger, wrol_mode_check, zig_zag, ConfigFile, \
+from wrolpi.common import loaded, Base, ModelHelper, logger, wrol_mode_check, zig_zag, ConfigFile, \
     wrol_mode_enabled, background_task, get_absolute_media_path, timer, aiohttp_get, \
     get_download_info, trim_file_name, get_wrolpi_config, TRACE_LEVEL, normalize_domain
 from wrolpi.dates import TZDateTime, now, Seconds
@@ -273,12 +273,13 @@ class Download(ModelHelper, Base):  # noqa
                               back_populates='downloads')
 
     def __repr__(self):
-        if self.next_download or self.frequency:
-            return f'<Download id={self.id} status={self.status} url={repr(self.url)} ' \
-                   f'next_download={repr(self.next_download)} frequency={self.frequency} attempts={self.attempts} ' \
-                   f'error={bool(self.error)}>'
-        return f'<Download id={self.id} status={self.status} url={repr(self.url)} attempts={self.attempts} ' \
-               f'error={bool(self.error)}>'
+        id_, status, url, attempts = (loaded(self, i) for i in ('id', 'status', 'url', 'attempts'))
+        next_download, frequency, error = (loaded(self, i) for i in ('next_download', 'frequency', 'error'))
+        if next_download or frequency:
+            return f'<Download id={id_} status={status} url={repr(url)} ' \
+                   f'next_download={repr(next_download)} frequency={frequency} attempts={attempts} ' \
+                   f'error={bool(error)}>'
+        return f'<Download id={id_} status={status} url={repr(url)} attempts={attempts} error={bool(error)}>'
 
     @serializer
     def __json__(self) -> dict:
diff --git a/wrolpi/files/models.py b/wrolpi/files/models.py
index d056ec8e..b73ff63b 100644
--- a/wrolpi/files/models.py
+++ b/wrolpi/files/models.py
@@ -10,7 +10,7 @@ from sqlalchemy import Column, String, BigInteger, Boolean, event, Float, Index,
 from sqlalchemy import types
 from sqlalchemy.orm import Mapped, deferred, relationship, Session, selectinload
 
-from wrolpi.common import Base, ModelHelper, logger, recursive_map, get_media_directory, \
+from wrolpi.common import loaded, Base, ModelHelper, logger, recursive_map, get_media_directory, \
     get_relative_to_media_directory, unique_by_predicate, replace_file
 from wrolpi.dates import TZDateTime, now, from_timestamp, strptime_ms, strftime
 from wrolpi.db import get_db_session, serializer
@@ -136,8 +136,9 @@ class FileGroup(ModelHelper, Base):
     d_text = deferred(Column(String))
 
     def __repr__(self):
-        m = f'model={self.model}' if self.model else f'mimetype={self.mimetype}'
-        return f'<FileGroup id={self.id} {m} primary_path={repr(str(self.primary_path))}>'
+        model = loaded(self, 'model')
+        m = f'model={model}' if model else f'mimetype={loaded(self, "mimetype")}'
+        return f'<FileGroup id={loaded(self, "id")} {m} primary_path={repr(str(loaded(self, "primary_path")))}>'
 
     @staticmethod
     def json_options() -> tuple:
@@ -877,7 +878,7 @@ class Directory(ModelHelper, Base):
         return d
 
     def __repr__(self):
-        return f'<Directory path={repr(str(self.path))}>'
+        return f'<Directory path={repr(str(loaded(self, "path")))}>'
 
 
 class WrolpiKV(Base):
diff --git a/wrolpi/tags.py b/wrolpi/tags.py
index 63c6173e..62469b09 100644
--- a/wrolpi/tags.py
+++ b/wrolpi/tags.py
@@ -11,7 +11,7 @@ from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm import Mapped, relationship, Session
 
 from wrolpi import dates, flags
-from wrolpi.common import ModelHelper, Base, logger, ConfigFile, get_media_directory, background_task, \
+from wrolpi.common import loaded, ModelHelper, Base, logger, ConfigFile, get_media_directory, background_task, \
     get_relative_to_media_directory, is_valid_hex_color, walk, INVALID_FILE_CHARS, get_wrolpi_config
 from wrolpi.dates import TZDateTime
 from wrolpi.db import get_db_session, named_placeholders, session_curs, serializer
@@ -42,7 +42,9 @@ class TagFile(ModelHelper, Base):
     file_group = relationship('FileGroup', back_populates='tag_files')
 
     def __repr__(self):
-        return f'<TagFile tag={self.tag.name} file_group={self.file_group.primary_path}>'
+        tag = loaded(self, 'tag', 'name') or loaded(self, 'tag_id')
+        file_group = loaded(self, 'file_group', 'primary_path') or loaded(self, 'file_group_id')
+        return f'<TagFile tag={tag} file_group={file_group}>'
 
     @staticmethod
     def get_by_primary_keys(session: Session, file_group_id: int, tag_id: int) -> Optional['TagFile']:
@@ -83,8 +85,8 @@ class Tag(ModelHelper, Base):
     # Note: Channel relationship removed - Channels now access Tags through Collection
 
     def __repr__(self):
-        name = self.name
-        color = self.color
+        name = loaded(self, 'name')
+        color = loaded(self, 'color')
         return f'<Tag {name=} {color=}>'
 
     @serializer
diff --git a/wrolpi/test/test_model_repr.py b/wrolpi/test/test_model_repr.py
new file mode 100644
index 00000000..5ef7f91c
--- /dev/null
+++ b/wrolpi/test/test_model_repr.py
@@ -0,0 +1,71 @@
+import pytest
+
+from modules.docs.conftest import doc_factory  # noqa: F401  The docs module's fixture.
+from modules.repos.models import Repository
+from modules.zim.models import TagZimEntry
+from wrolpi.collections import Collection
+from wrolpi.db import no_db_access
+from wrolpi.downloader import Download
+
+
+@pytest.fixture
+def every_model(async_client, test_session, video_factory, channel_factory, archive_factory, doc_factory, zim_factory,
+                tag_factory):
+    """One saved instance of each model that has a `__repr__`, plus the related rows its repr describes."""
+
+    async def _():
+        tag = await tag_factory('one')
+        channel = channel_factory()
+        video = video_factory(channel_id=channel.id, title='a video', tag_names=['one'])
+        archive = archive_factory('example.com', 'https://example.com/page')
+        doc = doc_factory()
+        zim = zim_factory('test zim')
+        tag_zim_entry = TagZimEntry(tag=tag, zim=zim, zim_entry='home')
+        repository = Repository(url='https://example.com/owner/repo.git',
+                                collection=Collection(kind='repo', name='repo'))
+        download = Download(url='https://example.com/feed', downloader='rss', frequency=86400,
+                            collection=channel.collection)
+        test_session.add_all([tag_zim_entry, repository, download])
+        test_session.commit()
+        models = [tag, channel, channel.collection, download, video, video.file_group,
+                  *video.file_group.tag_files, archive, archive.collection, doc, zim, tag_zim_entry, repository]
+        return {type(i).__name__: i for i in models}
+
+    return _
+
+
+@pytest.mark.asyncio
+async def test_repr_runs_no_sql(test_session, every_model):
+    """A repr reads only what is loaded.  Reprs run inside log calls, where a query (or a refresh after commit)
+    is a hidden round trip, and on an object whose session is gone it raises instead of logging."""
+    models = await every_model()
+    assert {'Tag', 'TagFile', 'Channel', 'Collection', 'Download', 'Video', 'FileGroup', 'Archive', 'Doc', 'Zim',
+            'TagZimEntry', 'Repository'} <= set(models)
+
+    test_session.expire_all()  # As after a commit in a background session.
+    with no_db_access('repr'):
+        reprs = {name: repr(model) for name, model in models.items()}
+    assert all(i.startswith(f'<{name}') for name, i in reprs.items()), reprs
+
+    test_session.expunge_all()  # Detached and expired: the session is gone.
+    for name, model in models.items():
+        assert repr(model).startswith(f'<{name}'), name
+
+
+@pytest.mark.asyncio
+async def test_repr_describes_loaded_objects(test_session, every_model):
+    """With everything loaded, a repr still shows the related names and paths."""
+    models = await every_model()
+    # Load what each repr describes, as the code that logs these objects would have.
+    video, tag_file, archive = models['Video'], models['TagFile'], models['Archive']
+    assert video.file_group.title and tag_file.tag.name and archive.file_group.url and archive.collection.name
+    assert models['Doc'].file_group.primary_path and models['TagZimEntry'].tag.name
+    assert models['Channel'].collection.name
+
+    assert "title='a video'" in repr(models['Video'])
+    assert 'tag=one' in repr(models['TagFile'])
+    assert 'url=https://example.com/page' in repr(models['Archive'])
+    assert 'domain=example.com' in repr(models['Archive'])
+    assert "path='" in repr(models['Doc']) and 'None' not in repr(models['Doc'])
+    assert "tag='one'" in repr(models['TagZimEntry'])
+    assert models['Channel'].name in repr(models['Channel'])