diff --git a/modules/archive/test/test_api.py b/modules/archive/test/test_api.py index fb728fa8..c7dad355 100644 --- a/modules/archive/test/test_api.py +++ b/modules/archive/test/test_api.py @@ -604,6 +604,8 @@ async def test_get_archive_loads_history_eagerly(test_session, archive_factory, test_session.commit() async def get(archive): + url = f'/api/archive/{archive.file_group_id}?skip_viewed=true' + test_session.expire_all() # Each request starts from the same Session state. statements = [] def count(conn, cursor, statement, *_): @@ -611,7 +613,7 @@ async def test_get_archive_loads_history_eagerly(test_session, archive_factory, event.listen(Engine, 'before_cursor_execute', count) try: - request, response = await async_client.get(f'/api/archive/{archive.file_group_id}?skip_viewed=true') + request, response = await async_client.get(url) finally: event.remove(Engine, 'before_cursor_execute', count) assert response.status_code == HTTPStatus.OK, response.json diff --git a/modules/repos/lib.py b/modules/repos/lib.py index f6d6b663..ecbfa40a 100644 --- a/modules/repos/lib.py +++ b/modules/repos/lib.py @@ -738,6 +738,8 @@ def _set_repository_download(session: Session, repo: Repository, frequency: int, (`frequency` is NEVER) has none; with `clone`, it is cloned once if its clone is missing.""" if not frequency: if download := repo.download: + # A deleted object stays in loaded collections; remove it so the Repo reads as never updated. + repo.collection.downloads.remove(download) session.delete(download) if clone and not (repo.directory / '.git').exists(): request_repository_update(session, repo) diff --git a/modules/videos/channel/api.py b/modules/videos/channel/api.py index 50a52023..b1c1ff7b 100644 --- a/modules/videos/channel/api.py +++ b/modules/videos/channel/api.py @@ -37,7 +37,6 @@ async def get_channels(request: Request): def channel_get(request: Request, channel_id: int = None): session = request.ctx.session channel = lib.get_channel(session, channel_id=channel_id, return_dict=True) - channel.pop('info_json') return json_response({'channel': channel}) diff --git a/modules/videos/channel/lib.py b/modules/videos/channel/lib.py index c514c5da..dad68f6a 100644 --- a/modules/videos/channel/lib.py +++ b/modules/videos/channel/lib.py @@ -3,7 +3,7 @@ from pathlib import Path from typing import List, Optional, Union from sqlalchemy import or_, func, desc -from sqlalchemy.orm import Query, Session, joinedload +from sqlalchemy.orm import Query, Session from wrolpi import flags from wrolpi.collections import Collection @@ -71,9 +71,11 @@ def get_channel(session: Session, *, channel_id: int = None, source_id: str = No """ channel: COD = None # noqa # Try to find the channel by the most reliable methods first. - # Always eagerly load Collection to ensure name/directory/tag_id are available + # Load what `Channel.dict()` reads: the Collection (name/directory/tag) and its Downloads. Overwrite any + # copy already in the Session so the Channel reflects the database. + query = session.query(Channel).options(*Channel.json_options()).populate_existing() if channel_id: - channel = session.query(Channel).options(joinedload(Channel.collection)).filter_by(id=channel_id).one_or_none() + channel = query.filter_by(id=channel_id).one_or_none() if not channel and directory: directory = Path(directory) if not directory.is_absolute(): @@ -85,31 +87,27 @@ def get_channel(session: Session, *, channel_id: int = None, source_id: str = No media_dir = get_media_directory() current = directory while current != media_dir and current != current.parent: - channel = session.query(Channel).options(joinedload(Channel.collection)).join(Collection).filter( - Collection.directory == str(current)).one_or_none() + channel = query.join(Collection).filter(Collection.directory == str(current)).one_or_none() if channel: break current = current.parent if not channel and source_id: - channel = session.query(Channel).options(joinedload(Channel.collection)).filter_by( - source_id=source_id).one_or_none() + channel = query.filter_by(source_id=source_id).one_or_none() if not channel and url: - channel = session.query(Channel).options(joinedload(Channel.collection)).filter_by(url=url).one_or_none() + channel = query.filter_by(url=url).one_or_none() if not channel and url: # Try to extract YouTube channel ID from the URL and match by source_id. from modules.videos.common import get_youtube_channel_id yt_channel_id = get_youtube_channel_id(url) if yt_channel_id: - channel = session.query(Channel).options(joinedload(Channel.collection)).filter_by( - source_id=yt_channel_id).one_or_none() + channel = query.filter_by(source_id=yt_channel_id).one_or_none() if not channel and name: - channel = session.query(Channel).join(Collection).filter(Collection.name == name).one_or_none() + channel = query.join(Collection).filter(Collection.name == name).one_or_none() if not channel: raise UnknownChannel(f'No channel matches {channel_id=} {source_id=} {url=} {directory=}') logger.debug(f'Found {channel=} using {channel_id=} {source_id=} {url=} {directory=}') - session.refresh(channel) if return_dict: statistics = channel.get_statistics() channel = channel.dict() @@ -179,12 +177,13 @@ def create_channel(session: Session, data: schema.ChannelPostRequest, return_dic name=data.name, kind='channel', directory=directory, + downloads=[], # A new Collection has none; nothing to load. ) session.add(collection) session.flush([collection]) # Create Channel linked to Collection - channel = Channel(collection_id=collection.id) + channel = Channel(collection=collection) session.add(channel) session.flush([channel]) # Apply the changes now that we've OK'd them @@ -199,7 +198,7 @@ def create_channel(session: Session, data: schema.ChannelPostRequest, return_dic def delete_channel(session: Session, *, channel_id: int): check_channels_config_imported() - channel = Channel.find_by_id(session, channel_id) + channel = get_channel(session, channel_id=channel_id, return_dict=False) channel_dict = channel.dict() channel.delete_with_videos() diff --git a/modules/videos/channel/test/test_api.py b/modules/videos/channel/test/test_api.py index 578a5a85..c9652875 100644 --- a/modules/videos/channel/test/test_api.py +++ b/modules/videos/channel/test/test_api.py @@ -317,6 +317,8 @@ async def test_channel_by_id(test_session, async_client, simple_channel, simple_ channel = response.json['channel'] # needs_reorganization should be included for UI to display reorganization status assert 'needs_reorganization' in channel + # The yt-dlp catalog is large and deferred; it is never loaded for a response. + assert 'info_json' not in channel @pytest.mark.asyncio diff --git a/modules/videos/channel/test/test_lib.py b/modules/videos/channel/test/test_lib.py index 4797f380..987d22d1 100644 --- a/modules/videos/channel/test/test_lib.py +++ b/modules/videos/channel/test/test_lib.py @@ -115,14 +115,14 @@ async def test_channels_no_url(async_client, test_session, test_directory, test_ directory=str(channel1_directory), ) channel1.url = '' - lib.create_channel(test_session, data=channel1) + lib.create_channel(test_session, data=channel1, return_dict=False) channel2 = schema.ChannelPostRequest( name='bar', directory=str(channel2_directory), url='' ) - lib.create_channel(test_session, data=channel2) + lib.create_channel(test_session, data=channel2, return_dict=False) channels = list(test_session.query(Channel)) assert len(channels) == 2 diff --git a/modules/videos/downloader.py b/modules/videos/downloader.py index fe4f36ca..d5996d45 100755 --- a/modules/videos/downloader.py +++ b/modules/videos/downloader.py @@ -712,7 +712,6 @@ class ChannelDownloader(Downloader, ABC): with get_db_session(commit=True) as session: channel = get_or_create_channel(session, channel_source_id, channel_url, name, channel_tag_name, destination=prepared.destination) - channel.dict() # get all attributes while we have the session. if channel and channel.id: channel_id = channel.id collection_id = channel.collection_id @@ -1469,8 +1468,6 @@ def get_or_create_channel(session: Session, source_id: str = None, url: str = No """ try: channel = get_channel(session, source_id=source_id, url=url, name=name, return_dict=False) - # Get all properties while we have the session. - channel.dict() return channel except UnknownChannel: pass @@ -1481,7 +1478,6 @@ def get_or_create_channel(session: Session, source_id: str = None, url: str = No if destination: try: channel = get_channel(session, directory=destination, return_dict=False) - channel.dict() return channel except UnknownChannel: pass @@ -1498,7 +1494,6 @@ def get_or_create_channel(session: Session, source_id: str = None, url: str = No # Reuse the Channel that already owns this directory instead of creating a conflicting one. try: channel = get_channel(session, directory=channel_directory, return_dict=False) - channel.dict() return channel except UnknownChannel: pass @@ -1519,9 +1514,6 @@ def get_or_create_channel(session: Session, source_id: str = None, url: str = No f'Cannot create channel {name!r}: conflicts with an existing channel') from e # Create the directory now that the channel is approved. channel_directory.mkdir(exist_ok=True) - # Get all properties while we have the session. - channel.dict() - return channel diff --git a/modules/videos/models.py b/modules/videos/models.py index 20130ade..6c9552c7 100644 --- a/modules/videos/models.py +++ b/modules/videos/models.py @@ -632,6 +632,9 @@ class Channel(ModelHelper, Base): # Joined: a Channel's name, directory and Tag live on its Collection. collection = relationship('Collection', foreign_keys=[collection_id], lazy='joined') + # The yt-dlp catalog is large and no response reads it. + dict_exclude = ('info_json',) + @property def downloads(self) -> list: """Get recurring downloads from associated Collection. Once-downloads are excluded because they are not @@ -966,6 +969,7 @@ class Channel(ModelHelper, Base): ) return d + @serializer def dict(self, with_statistics: bool = False, with_downloads: bool = True) -> dict: d = super(Channel, self).dict() # Add Collection-delegated properties diff --git a/wrolpi/collections/models.py b/wrolpi/collections/models.py index a423e4f0..a18829c9 100644 --- a/wrolpi/collections/models.py +++ b/wrolpi/collections/models.py @@ -284,7 +284,8 @@ class Collection(ModelHelper, Base): ) if reset_attempts: download.attempts = 0 - download.collection_id = self.id + # Through the relationship, so `self.downloads` stays current if it is loaded. + download.collection = self return download @staticmethod @@ -931,6 +932,7 @@ class Collection(ModelHelper, Base): return collection raise UnknownCollection(f'Cannot find Collection with directory {path}') + @serializer def dict(self) -> dict: """Return dictionary representation.""" d = super(Collection, self).dict() @@ -1272,6 +1274,7 @@ class CollectionItem(ModelHelper, Base): FileGroup's Tags, or its Zim.""" return joinedload(CollectionItem.file_group).selectinload(FileGroup.tag_files), joinedload(CollectionItem.zim) + @serializer def dict(self) -> dict: """Return dictionary representation.""" d = super(CollectionItem, self).dict() diff --git a/wrolpi/common.py b/wrolpi/common.py index 9d6f1b2e..febbd83b 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -340,8 +340,12 @@ Base = declarative_base() class ModelHelper: + # Columns `dict()` leaves out, such as large deferred columns that no response needs. + dict_exclude = () + def dict(self, *_, **__) -> dict: - d = {i.name: getattr(self, i.name) for i in self.__table__.columns} # noqa + d = {i.name: getattr(self, i.name) for i in self.__table__.columns # noqa + if i.name not in self.dict_exclude} return d @classmethod diff --git a/wrolpi/conftest.py b/wrolpi/conftest.py index d3990bb5..43f27fd9 100644 --- a/wrolpi/conftest.py +++ b/wrolpi/conftest.py @@ -45,7 +45,7 @@ from wrolpi.common import logger, await_background_tasks as await_background_tas from wrolpi.common import set_test_media_directory, Base, set_test_config from wrolpi.contexts import attach_shared_contexts, initialize_configs_contexts from wrolpi.dates import set_test_now -from wrolpi.db import create_wrolpi_engine, RequestSession +from wrolpi.db import create_wrolpi_engine, RequestDB, RequestSession, REQUEST_SESSION_OPTIONS from wrolpi.downloader import DownloadContext, DownloadManager, DownloadResult, Download, Downloader, \ downloads_manager_config_context, download_cache_config_context from wrolpi.errors import UnrecoverableDownloadError @@ -90,9 +90,25 @@ def test_session(test_directory) -> Generator[Session, Any, None]: """Get the testing db""" return test_engine, session + def fake_get_request_session(): + """A request shares the testing db, but commits like a `RequestSession`: without expiring. The default + returns once the handler is done (`background_task_listener`, `finish`), for the background code which + shares the session.""" + session.expire_on_commit = REQUEST_SESSION_OPTIONS['expire_on_commit'] + return test_engine, session + + finish = RequestDB.finish + + def fake_finish(self, ok: bool): + try: + finish(self, ok) + finally: + session.expire_on_commit = True + try: with mock.patch('wrolpi.db._get_db_session', fake_get_db_session), \ - mock.patch('wrolpi.db._get_request_session', fake_get_db_session): + mock.patch('wrolpi.db._get_request_session', fake_get_request_session), \ + mock.patch.object(RequestDB, 'finish', fake_finish): yield session finally: session.rollback() @@ -193,6 +209,9 @@ ROUTES_ATTACHED = False @api_app.on_response async def background_task_listener(request, response): """Wait for all background tasks to finish before returning API response while testing.""" + if session := getattr(request.ctx, 'session', None): + # The handler is done. Background work shares the test's Session, and expires on commit like production. + session.expire_on_commit = True await await_switches_() diff --git a/wrolpi/db.py b/wrolpi/db.py index e9a95e7b..cf1ef34b 100644 --- a/wrolpi/db.py +++ b/wrolpi/db.py @@ -204,6 +204,12 @@ def create_wrolpi_engine(target: Union[str, pathlib.Path]) -> sqlalchemy.engine. return engine +# Options for a request's Session. Committing does not expire loaded objects: the handler serializes them after +# its commit without a query (see `no_db_access`), and an async Session cannot reload them at all. A handler that +# changes data outside the Session (raw SQL, triggers, another connection) must `session.refresh()` what it reads +# next. Background Sessions (`get_db_session`) keep SQLAlchemy's default and reload expired objects after a commit. +REQUEST_SESSION_OPTIONS = dict(expire_on_commit=False) + _engine_lock = threading.Lock() _engine: sqlalchemy.engine.Engine = None _session_maker: sessionmaker = None @@ -259,7 +265,7 @@ class RequestSession(Session): it. `close()` releases the connection.""" def __init__(self, engine: sqlalchemy.engine.Engine, **kwargs): - super().__init__(**kwargs) + super().__init__(**{**REQUEST_SESSION_OPTIONS, **kwargs}) self._request_engine = engine self._request_connection = None diff --git a/wrolpi/test/test_db.py b/wrolpi/test/test_db.py index c6ac2edf..c859da12 100644 --- a/wrolpi/test/test_db.py +++ b/wrolpi/test/test_db.py @@ -262,6 +262,25 @@ def test_request_session_keeps_one_connection_until_closed(test_session): session.close() +def test_only_request_sessions_keep_objects_after_commit(test_session): + """A request serializes what it committed without a query (an async Session could not query at all). Background + Sessions reload objects after a commit, because they change data outside the Session (raw SQL, triggers).""" + test_session.add(Tag(name='one', color='#000000')) + test_session.commit() + with production_like_sessions(test_session) as maker: + request_session = RequestSession(maker.kw['bind']) + tag = request_session.query(Tag).one() + request_session.commit() + with no_db_access('reading after a commit'): + assert tag.name == 'one' + request_session.close() + + with get_db_session() as session: + tag = session.query(Tag).one() + session.commit() + assert sqlalchemy.inspect(tag).expired + + @pytest.mark.allow_unexpected_queries def test_no_db_access_refuses_queries_during_tests(test_session): """Serializing must not query: the data must already be loaded."""