From 14017d43bb2eb949f3c5a85df5e14cd73c53ce8b Mon Sep 17 00:00:00 2001 From: Roland Date: Thu, 17 Aug 2023 18:46:42 -0600 Subject: [PATCH] Creating Video/Archive get_by_id/find_by_id methods. find_by_id will raise an exception when not found, get_by_id will not. --- modules/archive/lib.py | 4 +-- modules/archive/models.py | 20 +++++++++++++++ modules/videos/channel/lib.py | 10 +++----- modules/videos/channel/test/test_api.py | 4 ++- modules/videos/conftest.py | 2 +- modules/videos/downloader.py | 4 +-- modules/videos/lib.py | 2 +- modules/videos/models.py | 34 +++++++++++++++++++++---- modules/videos/test/test_api.py | 8 +++--- modules/videos/test/test_video.py | 4 +-- modules/videos/video/lib.py | 13 ++-------- wrolpi/common.py | 8 +++++- wrolpi/files/downloader.py | 2 +- wrolpi/files/models.py | 2 +- wrolpi/test/test_tags.py | 4 +-- 15 files changed, 78 insertions(+), 43 deletions(-) diff --git a/modules/archive/lib.py b/modules/archive/lib.py index bdc450e1..cf92fd58 100644 --- a/modules/archive/lib.py +++ b/modules/archive/lib.py @@ -304,9 +304,7 @@ def get_title_from_html(html: str, url: str = None) -> str: @optional_session def get_archive(session, archive_id: int) -> Archive: """Get an Archive.""" - archive = session.query(Archive).filter_by(id=archive_id).one_or_none() - if not archive: - raise UnknownArchive(f'Could not find archive with id: {archive_id}') + archive = Archive.find_by_id(archive_id, session=session) return archive diff --git a/modules/archive/models.py b/modules/archive/models.py index 669be3e1..d9e3e860 100644 --- a/modules/archive/models.py +++ b/modules/archive/models.py @@ -11,6 +11,8 @@ from sqlalchemy.orm.collections import InstrumentedList from wrolpi.common import ModelHelper, Base, logger from wrolpi.dates import TZDateTime +from wrolpi.db import optional_session +from wrolpi.errors import UnknownArchive from wrolpi.files.models import FileGroup from wrolpi.media_path import MediaPathType from wrolpi.tags import Tag, TagFile @@ -43,6 +45,24 @@ class Archive(Base, ModelHelper): f'domain={self.domain.domain}>' return f'' + @staticmethod + @optional_session + def get_by_id(id_: int, session: Session = None) -> Optional[Base]: + """Attempt to find an Archive with the provided id. Returns None if it cannot be found.""" + archive = session.query(Archive).filter_by(id=id_).one_or_none() + return archive + + @staticmethod + @optional_session + def find_by_id(id_: int, session: Session = None) -> Base: + """Find an Archive with the provided id, raises an exception when no Archive is found. + + @raise UnknownArchive: if the Archive can not be found""" + archive = Archive.get_by_id(id_, session=session) + if not archive: + raise UnknownArchive(f'Cannot find Archive with id {id_}') + return archive + def my_paths(self, *mimetypes: str) -> List[pathlib.Path]: return self.file_group.my_paths(*mimetypes) diff --git a/modules/videos/channel/lib.py b/modules/videos/channel/lib.py index d32e50b3..70eef27b 100644 --- a/modules/videos/channel/lib.py +++ b/modules/videos/channel/lib.py @@ -172,17 +172,13 @@ def create_channel(session: Session, data: schema.ChannelPostRequest, return_dic @run_after(save_channels_config) -@optional_session +@optional_session(commit=True) def delete_channel(session: Session, *, channel_id: int): - try: - channel = session.query(Channel).filter_by(id=channel_id).one() - channel_dict = channel.dict() - except NoResultFound: - raise UnknownChannel() + channel = Channel.find_by_id(channel_id, session=session) + channel_dict = channel.dict() channel.delete_with_videos() - session.commit() return channel_dict diff --git a/modules/videos/channel/test/test_api.py b/modules/videos/channel/test/test_api.py index 2de72b04..15f551dd 100644 --- a/modules/videos/channel/test/test_api.py +++ b/modules/videos/channel/test/test_api.py @@ -38,7 +38,9 @@ def test_get_video(test_client, test_session, simple_channel, video_factory): # Test that a 404 is returned when no video exists _, response = test_client.get('/api/videos/video/10') assert response.status_code == HTTPStatus.NOT_FOUND, response.json - assert response.json == {'code': 'UNKNOWN_VIDEO', 'summary': 'The video could not be found.', 'error': ''} + assert response.json == {'code': 'UNKNOWN_VIDEO', + 'error': 'Cannot find Video with id 10', + 'summary': 'The video could not be found.'} # Get the video info we inserted _, response = test_client.get('/api/videos/video/1') diff --git a/modules/videos/conftest.py b/modules/videos/conftest.py index cc4739a7..eb3b13dc 100644 --- a/modules/videos/conftest.py +++ b/modules/videos/conftest.py @@ -81,7 +81,7 @@ def simple_video(test_session, test_directory, simple_channel, video_file) -> Vi video = Video.from_paths(test_session, video_path) video.channel = simple_channel test_session.commit() - video = test_session.query(Video).filter_by(id=video.id).one() + video = Video.find_by_id(video.id, session=test_session) return video diff --git a/modules/videos/downloader.py b/modules/videos/downloader.py index 03db5dc4..b42e11de 100755 --- a/modules/videos/downloader.py +++ b/modules/videos/downloader.py @@ -505,9 +505,7 @@ def update_channel_catalog(channel: Channel, info: dict): def _find_all_missing_videos(channel_id: id) -> List[Tuple]: """Get all Video entries which don't have the required media files (i.e. hasn't been downloaded).""" with get_db_session() as session: - channel: Channel = Channel.get_by_id(channel_id) - if not channel: - raise UnknownChannel(f'Cannot find channel with id {channel_id}') + channel: Channel = Channel.find_by_id(channel_id) def resolve_url(entry: dict) -> str: parsed = urlparse(entry.get('webpage_url') or entry['url']) diff --git a/modules/videos/lib.py b/modules/videos/lib.py index 966284c7..49387be7 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -475,7 +475,7 @@ def import_channels_config(): # Favorite in the config is the name of the video_path. Add the channel directory onto this # video_path, so we can match the complete path for the Video. video_path = channel_dir / video_path - file_group: FileGroup = FileGroup.find_by_path(video_path, session) + file_group: FileGroup = FileGroup.get_by_path(video_path, session) if file_group and file_group.tag_files: # Video is already tagged or favorited. pass diff --git a/modules/videos/models.py b/modules/videos/models.py index 636ed5d1..111659f1 100644 --- a/modules/videos/models.py +++ b/modules/videos/models.py @@ -8,7 +8,7 @@ from sqlalchemy import Column, Integer, String, Boolean, JSON, Date, ARRAY, Fore from sqlalchemy.orm import relationship, Session, deferred from sqlalchemy.orm.collections import InstrumentedList -from modules.videos.errors import UnknownVideo +from modules.videos.errors import UnknownVideo, UnknownChannel from wrolpi.captions import read_captions from wrolpi.common import Base, ModelHelper, logger, get_media_directory, background_task from wrolpi.dates import now, TZDateTime @@ -192,8 +192,8 @@ class Video(ModelHelper, Base): break # Fetch the videos by id, if they exist. - previous_video = session.query(Video).filter_by(id=previous_id).one() if previous_id else None - next_video = session.query(Video).filter_by(id=next_id).one() if next_id else None + previous_video = Video.find_by_id(previous_id, session) if previous_id else None + next_video = Video.find_by_id(next_id, session) if next_id else None return previous_video, next_video @@ -285,7 +285,7 @@ class Video(ModelHelper, Base): @staticmethod @optional_session - def find_by_path(path, session: Session) -> Optional['Video']: + def get_by_path(path, session: Session) -> Optional['Video']: video = session.query(Video) \ .join(FileGroup, FileGroup.id == Video.file_group_id) \ .filter(FileGroup.primary_path == path).one_or_none() @@ -293,10 +293,22 @@ class Video(ModelHelper, Base): @staticmethod @optional_session - def find_by_id(id_: int, session: Session) -> Optional['Video']: + def get_by_id(id_: int, session: Session = None) -> Optional['Video']: + """Attempt to find a Video with the provided id. Returns None if it cannot be found.""" video = session.query(Video).filter(Video.id == id_).one_or_none() return video + @staticmethod + @optional_session + def find_by_id(id_: int, session: Session = None) -> 'Video': + """Find a Video with the provided id, raises an exception if it cannot be found. + + @raise UnknownVideo: if the Video can not be found""" + video = Video.get_by_id(id_, session) + if not video: + raise UnknownVideo(f'Cannot find Video with id {id_}') + return video + def add_tag(self, tag_or_tag_name: Union[Tag, str]) -> TagFile: tag = Tag.find_by_name(tag_or_tag_name) if isinstance(tag_or_tag_name, str) else tag_or_tag_name return self.file_group.add_tag(tag) @@ -535,5 +547,17 @@ class Channel(ModelHelper, Base): @staticmethod @optional_session def get_by_id(id_: int, session: Session = None) -> Optional['Channel']: + """Attempt to find a Channel with the provided id. Returns None if it cannot be found.""" channel = session.query(Channel).filter_by(id=id_).one_or_none() return channel + + @staticmethod + @optional_session + def find_by_id(id_: int, session: Session = None) -> 'Channel': + """Find a Channel with the provided id, raises an exception when no Channel is found. + + @raise UnknownChannel: if the channel can not be found""" + channel = Channel.get_by_id(id_, session=session) + if not channel: + raise UnknownChannel(f'Cannot find channel with id {id_}') + return channel diff --git a/modules/videos/test/test_api.py b/modules/videos/test/test_api.py index a24abd1d..047e52a4 100644 --- a/modules/videos/test/test_api.py +++ b/modules/videos/test/test_api.py @@ -104,7 +104,7 @@ def test_refresh_videos(test_client, test_session, test_directory, simple_channe video1_video_path = str(video1.video_path) video1.poster_path.unlink() test_client.post('/api/files/refresh') - video1 = Video.find_by_path(video1_video_path, test_session) + video1 = Video.get_by_path(video1_video_path, test_session) assert not video1.poster_path @@ -136,9 +136,9 @@ def test_channels_with_videos(test_session, test_client, test_directory, channel assert test_session.query(Video).count() == 3, 'Did not find correct amount of video files.' assert {i[0] for i in test_session.query(Channel.name)} == {'channel1', 'channel2'}, 'Channels were changed.' - vid1 = Video.find_by_path(vid1_path) - vid2 = Video.find_by_path(vid2_path) - vid3 = Video.find_by_path(vid3_path) + vid1 = Video.get_by_path(vid1_path) + vid2 = Video.get_by_path(vid2_path) + vid3 = Video.get_by_path(vid3_path) assert vid1.channel == channel1 assert vid2.channel == channel2 diff --git a/modules/videos/test/test_video.py b/modules/videos/test/test_video.py index 286df545..545cd974 100644 --- a/modules/videos/test/test_video.py +++ b/modules/videos/test/test_video.py @@ -79,8 +79,8 @@ async def test_video_channel_refresh(test_session, test_directory, channel_facto assert test_session.query(Video).count() == 2 # The video is associated with `simple_channel` because its file is in the Channel's directory. - video1: Video = Video.find_by_path(video1_path, test_session) - video2: Video = Video.find_by_path(video2_path, test_session) + video1: Video = Video.get_by_path(video1_path, test_session) + video2: Video = Video.get_by_path(video2_path, test_session) assert video1.channel == channel assert not video2.channel assert video1.__json__()['video']['channel'] diff --git a/modules/videos/video/lib.py b/modules/videos/video/lib.py index ee6f1887..1de77d98 100644 --- a/modules/videos/video/lib.py +++ b/modules/videos/video/lib.py @@ -1,32 +1,23 @@ from typing import Tuple, Optional, List, Dict from sqlalchemy.orm import Session -from sqlalchemy.orm.exc import NoResultFound from modules.videos.models import Video from wrolpi.common import run_after, logger from wrolpi.db import get_db_session, optional_session -from ..errors import UnknownVideo from wrolpi.files.lib import handle_file_group_search_results, tag_names_to_file_group_sub_select +from ..errors import UnknownVideo from ..lib import save_channels_config logger.getChild(__name__) -def get_video(session: Session, video_id: int) -> Video: - try: - video = session.query(Video).filter_by(id=video_id).one() - return video - except NoResultFound: - raise UnknownVideo() - - def get_video_for_app(video_id: int) -> Tuple[dict, Optional[dict], Optional[dict]]: """ Get a Video, with it's prev/next videos. Mark the Video as viewed. """ with get_db_session(commit=True) as session: - video = get_video(session, video_id) + video = Video.find_by_id(video_id, session=session) video.set_viewed() previous_video, next_video = video.get_surrounding_videos() diff --git a/wrolpi/common.py b/wrolpi/common.py index e6cc7e56..69fa0ed9 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -155,7 +155,7 @@ class ModelHelper: raise NotImplementedError('This model has not defined this method.') @staticmethod - def find_by_path(path, session) -> Base: + def get_by_path(path, session) -> Base: raise NotImplementedError('This model has not defined this method.') @staticmethod @@ -164,6 +164,12 @@ class ModelHelper: @staticmethod def get_by_id(id_: int, session: Session = None) -> Optional[Base]: + """Attempts to get a model instance by its id. Returns None if no instance can be found.""" + raise NotImplementedError('This model has not defined this method.') + + @staticmethod + def find_by_id(id_: int, session: Session = None) -> Base: + """Get a model instance by its id, raise an error if no instance is found.""" raise NotImplementedError('This model has not defined this method.') def flush(self): diff --git a/wrolpi/files/downloader.py b/wrolpi/files/downloader.py index 5967931b..c5001b2f 100644 --- a/wrolpi/files/downloader.py +++ b/wrolpi/files/downloader.py @@ -77,7 +77,7 @@ async def save_and_tag(file: pathlib.Path, tag_names: List[str] = None): tag_names = tag_names or list() with get_db_session(commit=True) as session: - file_group = FileGroup.find_by_path(file) + file_group = FileGroup.get_by_path(file) if not file_group: file_group = FileGroup.from_paths(session, file) session.commit() diff --git a/wrolpi/files/models.py b/wrolpi/files/models.py index d1fa37a0..66b700e1 100644 --- a/wrolpi/files/models.py +++ b/wrolpi/files/models.py @@ -260,7 +260,7 @@ class FileGroup(ModelHelper, Base): @staticmethod @optional_session - def find_by_path(path, session) -> Optional['FileGroup']: + def get_by_path(path, session) -> Optional['FileGroup']: file_group = session.query(FileGroup).filter(FileGroup.primary_path == str(path)).one_or_none() return file_group diff --git a/wrolpi/test/test_tags.py b/wrolpi/test/test_tags.py index f2c53762..c1446313 100644 --- a/wrolpi/test/test_tags.py +++ b/wrolpi/test/test_tags.py @@ -67,8 +67,8 @@ async def test_tags_file_group(test_session, make_files_structure, tag_factory, async def test_tags_config_(test_session, test_directory, tag_factory, example_pdf, video_file, test_tags_config): """Test that the config is updated when a FileGroup is tagged.""" await files_lib.refresh_files() - pdf: FileGroup = FileGroup.find_by_path(example_pdf, test_session) - video: FileGroup = FileGroup.find_by_path(video_file, test_session) + pdf: FileGroup = FileGroup.get_by_path(example_pdf, test_session) + video: FileGroup = FileGroup.get_by_path(video_file, test_session) tag1 = tag_factory() tag2 = tag_factory() test_session.commit()