diff --git a/modules/videos/lib.py b/modules/videos/lib.py index 6d34bd91..c5dd210f 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -216,15 +216,20 @@ def convert_or_generate_poster(video: Video) -> Tuple[Optional[pathlib.Path], Op if old != new and new.exists(): # Destination JPEG already exists (it may have the wrong format). + video.file_group.unlink_tag_directory_links(old) old.unlink() old = new if not is_valid_poster(old): - # Poster is not valid, convert it and place it in the new location. + # Poster is not valid, convert it and place it in the new location. Conversion removes `old`, so its + # Tags Directory links are found now and removed only once the conversion succeeds. + old_links = video.file_group.get_tag_directory_links(old) if old != new else [] try: convert_image(old, new) if not new.is_file() or not new.stat().st_size: raise FileNotFoundError(f'Failed to convert poster: {new}') + for link in old_links: + link.unlink(missing_ok=True) logger.info(f'Converted invalid poster {repr(str(old))} to {repr(str(new))}') return new, None except Exception as e: diff --git a/modules/videos/models.py b/modules/videos/models.py index 3637cec8..8e75bbe2 100644 --- a/modules/videos/models.py +++ b/modules/videos/models.py @@ -515,6 +515,8 @@ class Video(ModelHelper, Base): def delete_video(video_: Video): """Delete a Video with any of its tags.""" + # Untagging forgets which links are this Video's, so remove them first. + video_.file_group.unlink_tag_directory_links() for tag_name_ in video_.file_group.tag_names: video_.file_group.untag(session, tag_name_) video_.delete() diff --git a/modules/videos/test/test_lib.py b/modules/videos/test/test_lib.py index 2fd3bd6c..b37d7a51 100644 --- a/modules/videos/test/test_lib.py +++ b/modules/videos/test/test_lib.py @@ -593,3 +593,37 @@ def test_download_settings_codec_validation(): with pytest.raises(ValidationError): DownloadSettings(audio_codecs=['wma']) + + +@pytest.mark.asyncio +async def test_convert_poster_removes_tags_directory_link(async_client, test_session, test_directory, video_factory, + tag_factory, await_switches): + """Converting a tagged Video's poster removes the old poster's Tags Directory link.""" + tag = await tag_factory() + video = video_factory(with_video_file=True, with_poster_ext='webp', tag_names=[tag.name]) + test_session.commit() + await await_switches() + webp_link = test_directory / 'tags' / tag.name / video.poster_path.name + assert webp_link.is_file() + + new_poster, _ = lib.convert_or_generate_poster(video) + + assert new_poster.suffix == '.jpg' + assert not webp_link.exists(), 'The converted poster left its link behind' + + +@pytest.mark.asyncio +async def test_valid_poster_keeps_tags_directory_link(async_client, test_session, test_directory, video_factory, + tag_factory, await_switches): + """A valid JPEG poster that is not named .jpg is not converted, so its Tags Directory link is kept.""" + tag = await tag_factory() + video = video_factory(with_video_file=True, with_poster_ext='jpeg', tag_names=[tag.name]) + test_session.commit() + await await_switches() + poster_link = test_directory / 'tags' / tag.name / video.poster_path.name + assert poster_link.is_file() + + lib.convert_or_generate_poster(video) + + assert video.poster_path.is_file() + assert poster_link.is_file() and poster_link.samefile(video.poster_path) diff --git a/modules/videos/test/test_video.py b/modules/videos/test/test_video.py index 93277ac1..5ccbee9e 100644 --- a/modules/videos/test/test_video.py +++ b/modules/videos/test/test_video.py @@ -240,6 +240,33 @@ async def test_delete_duplicate_video_tagged_duplicate(async_client, test_sessio assert video.file_group.tag_names == [tag.name, ] +@pytest.mark.asyncio +async def test_delete_duplicate_video_leaves_no_tags_directory_orphans(async_client, test_session, test_directory, + channel_factory, video_factory, tag_factory, + await_switches): + """Deleting a tagged duplicate Video removes its Tags Directory links; only links to existing files remain.""" + channel = channel_factory(name='Channel Name') + video_path = channel.directory / f'{channel.name}_20000101_ABC123456_The video title.mp4' + tag = await tag_factory() + for title in (f'{channel.name}_20000101_ABC123456_The video title', + f'{channel.name}_20000101_ABC123456_The video title '): + video = video_factory(channel_id=channel.id, title=title, upload_date=datetime(2000, 1, 1, tzinfo=timezone.utc), + source_id='ABC123456', with_video_file=True, with_info_json=True, tag_names=[tag.name]) + video.file_group.url = 'https://example.com/video' + test_session.commit() + await await_switches() + tags_directory = test_directory / 'tags' + assert len([i for i in tags_directory.rglob('*.mp4')]) == 2 + + assert await Video.delete_duplicate_videos(test_session, 'https://example.com/video', 'ABC123456', video_path) + test_session.commit() + await await_switches() + + links = [i for i in tags_directory.rglob('*') if i.is_file() and i.name != 'README.txt'] + assert links, 'The kept Video is still linked' + assert all(i.stat().st_nlink > 1 for i in links), 'A link was left behind with no source file' + + @pytest.mark.asyncio async def test_delete_duplicate_video_smaller_copy_at_destination(async_client, test_session, channel_factory, video_factory): diff --git a/wrolpi/files/lib.py b/wrolpi/files/lib.py index da05b252..dce501cb 100644 --- a/wrolpi/files/lib.py +++ b/wrolpi/files/lib.py @@ -427,6 +427,16 @@ async def delete(*paths: Union[str, pathlib.Path], force: bool = False, post_pro return [i.__json__() for i in tagged_file_groups] had_tagged = bool(tagged_file_groups) + # Remove the Tags Directory links of the files about to be deleted, while they can still be matched to + # their sources. A deleted directory takes every file of a FileGroup inside it. + directories = [i for i in paths if i.is_dir()] + files = [i for i in paths if i.is_file()] + for file_group in tagged_file_groups: + if any(i == file_group.directory or i in file_group.directory.parents for i in directories): + file_group.unlink_tag_directory_links() + elif files: + file_group.unlink_tag_directory_links(*files) + for path in paths: ignored_directories = get_wrolpi_config().ignored_directories if ignored_directories and str(path) in ignored_directories: diff --git a/wrolpi/files/models.py b/wrolpi/files/models.py index b73ff63b..2d9955b5 100644 --- a/wrolpi/files/models.py +++ b/wrolpi/files/models.py @@ -19,7 +19,7 @@ from wrolpi.errors import FileGroupIsTagged, UnknownFile from wrolpi.files import indexers from wrolpi.media_path import MediaPathType from wrolpi.files.progress import save_recently_viewed_config -from wrolpi.tags import Tag, TagFile, save_tags_config, sync_tags_directory +from wrolpi.tags import Tag, TagFile, save_tags_config, sync_tags_directory, get_tags_directory from wrolpi.vars import PYTEST logger = logger.getChild(__name__) @@ -494,6 +494,7 @@ class FileGroup(ModelHelper, Base): if self.tag_files and not force: raise FileGroupIsTagged(f'Cannot delete {self} because it has tags') + self.unlink_tag_directory_links() for path in self.my_paths(): path.unlink(missing_ok=True) @@ -835,6 +836,32 @@ class FileGroup(ModelHelper, Base): self.do_index() self.indexed = True + def get_tag_directory_links(self, *paths: pathlib.Path) -> List[pathlib.Path]: + """Return this FileGroup's links in the Tags Directory (only those of `paths`, if provided). + + A link is only returned if it is the same file as its source, so the sources must still exist.""" + if not self.tag_files: + return [] + tags_directory = get_tags_directory() + links = [] + for file, link in self.get_tag_directory_paths_map().items(): + if paths and file not in paths: + continue + link = tags_directory / link + try: + if link.samefile(file): + links.append(link) + except FileNotFoundError: + pass + return links + + def unlink_tag_directory_links(self, *paths: pathlib.Path): + """Delete this FileGroup's links in the Tags Directory (only those of `paths`, if provided). + + Call this before the source files are removed. A file that is not this FileGroup's link is never deleted.""" + for link in self.get_tag_directory_links(*paths): + link.unlink(missing_ok=True) + def get_tag_directory_paths_map(self) -> dict[pathlib.Path, str]: """Return all links that should exist for this FileGroup in the Tags Directory.""" tag_names = self.tag_names diff --git a/wrolpi/test/test_tags.py b/wrolpi/test/test_tags.py index 04f9e77a..32affa09 100644 --- a/wrolpi/test/test_tags.py +++ b/wrolpi/test/test_tags.py @@ -680,6 +680,77 @@ async def test_tags_directory_all_files_deleted(test_session, test_directory, ta assert poster_link.is_file(), 'Poster link should be preserved when all sources deleted' +@pytest.mark.asyncio +async def test_tags_directory_tagged_file_group_deleted(async_client, test_session, test_directory, tag_factory, + video_bytes, image_bytes_factory, await_switches): + """Deleting a tagged FileGroup removes its Tags Directory links, but never a file that is not one of them.""" + video_file = test_directory / 'video.mp4' + poster_file = test_directory / 'video.jpg' + video_file.write_bytes(video_bytes) + poster_file.write_bytes(image_bytes_factory()) + fg = FileGroup.from_paths(test_session, video_file, poster_file) + test_session.add(fg) + test_session.commit() + + tag = await tag_factory('favorite') + fg.add_tag(test_session, tag.id) + test_session.commit() + await await_switches() + + video_link = test_directory / 'tags' / tag.name / 'video.mp4' + poster_link = test_directory / 'tags' / tag.name / 'video.jpg' + assert video_link.is_file() and poster_link.is_file() + # The poster link was replaced with a different file; it is not this FileGroup's link. + poster_link.unlink() + poster_link.write_bytes(b'not a link') + + body = dict(file_group_ids=[fg.id], force=True) + request, response = await async_client.post('/api/files/delete_groups', content=json.dumps(body)) + assert response.status_code == HTTPStatus.NO_CONTENT + await await_switches() + + assert not video_file.exists() and not poster_file.exists() + assert not video_link.exists(), 'The link to the deleted file should be removed' + assert poster_link.read_bytes() == b'not a link', 'A file that is not a link to the FileGroup is kept' + + +@pytest.mark.asyncio +@pytest.mark.parametrize('delete_directory', [False, True]) +async def test_tags_directory_tagged_path_deleted(async_client, test_session, test_directory, tag_factory, + video_bytes, image_bytes_factory, await_switches, delete_directory): + """Force-deleting a tagged file (or its directory) by path removes the deleted files' Tags Directory links.""" + directory = test_directory / 'videos' + directory.mkdir() + video_file = directory / 'video.mp4' + poster_file = directory / 'video.jpg' + video_file.write_bytes(video_bytes) + poster_file.write_bytes(image_bytes_factory()) + fg = FileGroup.from_paths(test_session, video_file, poster_file) + test_session.add(fg) + test_session.commit() + + tag = await tag_factory('favorite') + fg.add_tag(test_session, tag.id) + test_session.commit() + await await_switches() + + video_link = test_directory / 'tags' / tag.name / 'video.mp4' + poster_link = test_directory / 'tags' / tag.name / 'video.jpg' + assert video_link.is_file() and poster_link.is_file() + + # Delete the whole directory, or only the poster. + body = dict(paths=['videos' if delete_directory else 'videos/video.jpg'], force=True) + request, response = await async_client.post('/api/files/delete', content=json.dumps(body)) + assert response.status_code == HTTPStatus.NO_CONTENT + await await_switches() + + assert not poster_link.exists(), 'The link to the deleted file should be removed' + if delete_directory: + assert not video_link.exists(), 'The link to the deleted file should be removed' + else: + assert video_link.is_file() and video_link.samefile(video_file), 'The link of a kept file is kept' + + @pytest.mark.asyncio async def test_tags_directory_disabled(test_session, test_directory, tag_factory, video_factory, await_switches): """Test that Tag Directory is NOT synchronized when tags_directory setting is disabled."""