diff --git a/wrolpi/files/lib.py b/wrolpi/files/lib.py index 196636e9..a9c6cbf9 100644 --- a/wrolpi/files/lib.py +++ b/wrolpi/files/lib.py @@ -52,7 +52,7 @@ except ImportError: logger = logger.getChild(__name__) __all__ = ['list_directories_contents', 'delete', 'split_path_stem_and_suffix', 'search_files', - 'get_mimetype', 'split_file_name_words', 'get_primary_file', 'get_file_statistics', + 'get_mimetype', 'split_file_name_words', 'get_primary_file', 'choose_primary_file', 'get_file_statistics', 'search_file_suggestion_count', 'glob_shared_stem', 'upsert_file', 'get_unique_files_by_stem', 'rename', 'delete_directory', 'handle_file_group_search_results', 'get_file_location_href', 'get_tagged_file_groups_by_ids', 'delete_file_groups', 'get_special_directories', @@ -685,6 +685,22 @@ def get_primary_file(files: Union[Tuple[pathlib.Path], Iterable[pathlib.Path]]) raise NoPrimaryFile(f'Cannot find primary file for group: {files}') +def choose_primary_file(files: Union[Tuple[pathlib.Path], Iterable[pathlib.Path]]) -> pathlib.Path: + """Primary file for a stem group. Falls back to a sorted path so every caller agrees. + + `_upsert_files` and `_upsert_file_groups` must pick the same path. A set's iteration + order is not stable; using it as the fallback would DELETE the existing tagged row + and INSERT a new untagged one. + """ + paths = [pathlib.Path(p) for p in files] + if not paths: + raise ValueError('Cannot find primary file without any files') + try: + return get_primary_file(paths) + except NoPrimaryFile: + return sorted(paths)[0] + + def _upsert_files(files: List[pathlib.Path], progress_callback: Callable[[int, int], None] = None): """Insert/update all records of the provided files. @@ -709,33 +725,18 @@ def _upsert_files(files: List[pathlib.Path], for group in grouped: # The primary file is the video/SingleFile/epub, etc. try: - try: - primary_path = get_primary_file(group) - # Multiple files in this group. - primary_path: pathlib.Path = primary_path - # The primary mimetype allows modelers to find its file_groups. - mimetype = get_mimetype(primary_path) - # The group uses a common modification_datetime so the group will be re-indexed when any of it's - # files are modified. - modification_datetime = from_timestamp(max(i.stat().st_mtime for i in group)) - size = sum(i.stat().st_size for i in group) - files_json = json.dumps(_paths_to_files_dict(group)) - stem, suffix = split_path_stem_and_suffix(primary_path) - suffix = (suffix or '').lower() or None - values[primary_path] = (modification_datetime, mimetype, size, files_json, suffix, stem) - non_primary_files.update(i for i in group if i != primary_path) - except NoPrimaryFile: - # Same stem still means one FileGroup. Pick a stable primary so - # (directory, stem) stays unique even when nothing is modelable. - primary_path = sorted(group)[0] - mimetype = get_mimetype(primary_path) - modification_datetime = from_timestamp(max(i.stat().st_mtime for i in group)) - size = sum(i.stat().st_size for i in group) - files_json = json.dumps(_paths_to_files_dict(group)) - stem, suffix = split_path_stem_and_suffix(primary_path) - suffix = (suffix or '').lower() or None - values[primary_path] = (modification_datetime, mimetype, size, files_json, suffix, stem) - non_primary_files.update(i for i in group if i != primary_path) + primary_path = choose_primary_file(group) + # The primary mimetype allows modelers to find its file_groups. + mimetype = get_mimetype(primary_path) + # The group uses a common modification_datetime so the group will be re-indexed when any of it's + # files are modified. + modification_datetime = from_timestamp(max(i.stat().st_mtime for i in group)) + size = sum(i.stat().st_size for i in group) + files_json = json.dumps(_paths_to_files_dict(group)) + stem, suffix = split_path_stem_and_suffix(primary_path) + suffix = (suffix or '').lower() or None + values[primary_path] = (modification_datetime, mimetype, size, files_json, suffix, stem) + non_primary_files.update(i for i in group if i != primary_path) except FileNotFoundError as e: # A file was deleted between discovery and upsert (temp files, WAL sidecars, etc.). refresh_logger.warning(f'File vanished during refresh, skipping group near {group[0]}', exc_info=e) diff --git a/wrolpi/files/models.py b/wrolpi/files/models.py index edc9a3c3..838f41b5 100644 --- a/wrolpi/files/models.py +++ b/wrolpi/files/models.py @@ -518,13 +518,13 @@ class FileGroup(ModelHelper, Base): @classmethod def from_paths(cls, session: Session, *paths: pathlib.Path) -> 'FileGroup': """Create a new FileGroup which contains the provided file paths.""" - from wrolpi.files.lib import get_primary_file, get_mimetype, sanitize_filename_surrogates, \ + from wrolpi.files.lib import choose_primary_file, get_mimetype, sanitize_filename_surrogates, \ split_path_stem_and_suffix # Sanitize any paths with invalid UTF-8 characters (renames files on disk if needed) paths = tuple(sanitize_filename_surrogates(p) for p in paths) - primary_path = get_primary_file(paths) if paths else None + primary_path = choose_primary_file(paths) if paths else None stem = split_path_stem_and_suffix(primary_path)[0] if primary_path else None directory = str(primary_path.parent) if primary_path else None filters = [FileGroup.primary_path.in_(list(map(str, paths)))] @@ -538,7 +538,7 @@ class FileGroup(ModelHelper, Base): if len(existing_groups) == 0: # These paths have not been used previously, create a new FileGroup. file_group = FileGroup() - primary_path = get_primary_file(paths) + primary_path = choose_primary_file(paths) file_group.primary_path = primary_path file_group.append_files(*paths) session.add(file_group) @@ -546,10 +546,10 @@ class FileGroup(ModelHelper, Base): # Found one FileGroup with these paths, no need to create a new FileGroup. file_group = existing_groups[0] file_group.append_files(*paths) - primary_path = get_primary_file(file_group.my_paths()) + primary_path = choose_primary_file(file_group.my_paths()) else: # Multiple FileGroups contain these paths as primary. - primary_path = get_primary_file(paths) + primary_path = choose_primary_file(paths) file_group: FileGroup = next(filter(lambda i: i.primary_path == primary_path, existing_groups), None) if not file_group: file_group = FileGroup.from_paths(session, primary_path) @@ -699,15 +699,33 @@ class FileGroup(ModelHelper, Base): new_name, _ = split_path_stem_and_suffix(new_primary_path, full=True) new_directory = new_primary_path.parent + new_stem, _ = split_path_stem_and_suffix(new_primary_path) + + # Unique (directory, stem): merge into the destination group before touching disk. + # Otherwise shutil.move succeeds, then commit hits IntegrityError and disk/DB disagree. + session = Session.object_session(self) + if session is not None: + existing = session.query(FileGroup).filter( + FileGroup.directory == str(new_directory), + FileGroup.stem == new_stem, + FileGroup.id != self.id, + ).one_or_none() + if existing is not None: + self.merge([existing]) + # Flush the DELETE first. Otherwise SQLAlchemy UPDATEs our stem before + # removing the other row and hits UNIQUE (directory, stem). + session.flush() # Get current files with resolved absolute paths for moving current_files = self.my_files() # Ensure that all destination files do not yet exist before move. + # After merging a same-stem group, some dest paths are the files we already own. for file in current_files: - _, suffix = split_path_stem_and_suffix(file['path']) + old_path = file['path'] + _, suffix = split_path_stem_and_suffix(old_path) new_path = pathlib.Path(f'{new_name}{suffix}') - if new_path.exists(): + if new_path.exists() and new_path != old_path: raise FileExistsError(f'Cannot move {self} to {new_path} because it already exists.') # Move physical files and collect new relative file entries @@ -717,7 +735,8 @@ class FileGroup(ModelHelper, Base): _, suffix = split_path_stem_and_suffix(old_path) new_path = pathlib.Path(f'{new_name}{suffix}') if old_path.is_file(): - shutil.move(old_path, new_path) + if old_path != new_path: + shutil.move(old_path, new_path) # Store just the filename (relative path) new_files.append({'path': new_path.name, 'mimetype': file['mimetype']}) diff --git a/wrolpi/files/test/test_lib.py b/wrolpi/files/test/test_lib.py index c3b5e6c6..a6cfc230 100644 --- a/wrolpi/files/test/test_lib.py +++ b/wrolpi/files/test/test_lib.py @@ -515,6 +515,58 @@ def test_file_group_directory_stem_is_unique(test_session, test_directory, make_ test_session.commit() +def test_choose_primary_file_is_sorted_and_set_stable(test_directory, make_files_structure): + """Unmodeled groups must pick the same primary regardless of set iteration order.""" + csv, txt = make_files_structure(['n.csv', 'n.txt']) + assert lib.choose_primary_file([txt, csv]) == csv + assert lib.choose_primary_file({txt, csv}) == csv + + +@pytest.mark.asyncio +async def test_refresh_keeps_tag_when_sidecar_added(async_client, test_session, test_directory, + make_files_structure, tag_factory, refresh_files): + """Adding a sidecar to a tagged unmodeled FileGroup must not drop the tag on refresh.""" + txt, = make_files_structure(['note.txt']) + fg = FileGroup.from_paths(test_session, txt) + test_session.commit() + tag = await tag_factory('keep') + fg.add_tag(test_session, tag.id) + test_session.commit() + fg_id = fg.id + + (test_directory / 'note.csv').write_text('a,b\n') + await refresh_files() + test_session.expire_all() + + remaining = test_session.query(FileGroup).filter_by(id=fg_id).one() + assert remaining.tag_names == ['keep'] + names = {pathlib.Path(f['path']).name for f in remaining.files} + assert names == {'note.txt', 'note.csv'} + + +@pytest.mark.asyncio +async def test_rename_into_existing_stem_merges(async_client, test_session, test_directory, make_files_structure): + """Renaming onto another FileGroup's stem merges before moving files, so disk and DB agree.""" + make_files_structure({'foo.jpg': 'x', 'bar.mp4': 'video'}) + jpg = test_directory / 'foo.jpg' + mp4 = test_directory / 'bar.mp4' + FileGroup.from_paths(test_session, jpg) + FileGroup.from_paths(test_session, mp4) + test_session.commit() + assert test_session.query(FileGroup).count() == 2 + assert {fg.primary_path.name for fg in test_session.query(FileGroup)} == {'foo.jpg', 'bar.mp4'} + + await lib.rename_file(mp4, 'foo.mp4') + test_session.expire_all() + + remaining = test_session.query(FileGroup).one() + names = {pathlib.Path(f['path']).name for f in remaining.files} + assert names == {'foo.jpg', 'foo.mp4'} + assert (test_directory / 'foo.mp4').is_file() + assert (test_directory / 'foo.jpg').is_file() + assert not (test_directory / 'bar.mp4').exists() + + @pytest.mark.asyncio async def test_dedupe_file_groups_merges_tags_onto_winner(async_client, test_session, test_directory, make_files_structure, tag_factory): diff --git a/wrolpi/files/worker.py b/wrolpi/files/worker.py index deb8af40..a0076328 100644 --- a/wrolpi/files/worker.py +++ b/wrolpi/files/worker.py @@ -33,7 +33,7 @@ from wrolpi.events import Events from wrolpi.vars import PYTEST, IS_MACOS from wrolpi.files.lib import ( split_path_stem_and_suffix, _upsert_files, get_unique_files_by_stem, glob_shared_stem, - group_files_by_stem, get_primary_file, delete_directory, apply_indexers, + group_files_by_stem, get_primary_file, choose_primary_file, delete_directory, apply_indexers, _move_file_group_files, _bulk_update_file_groups_db, MOVE_CHUNK_SIZE, _bulk_update_file_groups_reorganize, get_normalized_ignored_directories, remove_files_in_ignored_directories, ensure_file_group_stems, @@ -1273,8 +1273,8 @@ class FileWorker: try: result = await self._refresh_files_directly(paths) self._cleanup_modified_models(result.modified) - await self._upsert_file_groups(result.new + result.modified) await self._delete_file_groups(result.deleted) + await self._upsert_file_groups(result.new + result.modified) if post_processing and (result.new or result.modified or result.deleted): # Skip global modelers/indexers when the caller opts out (e.g. `lib.delete` from the # upload path defers that to the post-upload `upsert_file`) or when nothing changed. @@ -1610,17 +1610,12 @@ class FileWorker: # For modified diffs where file_group_id exists, we need to handle primary_path changes. # If the primary file changed (e.g., video deleted, leaving only poster), the ON CONFLICT # won't match, so we need to update the existing FileGroup's primary_path first. - from wrolpi.files.lib import NoPrimaryFile modified_ids_to_update = [] for diff in diffs: if diff.file_group_id and diff.fs_files: # This is a modified FileGroup - may need to update primary_path new_paths = [diff.directory / f for f in diff.fs_files] - try: - new_primary = get_primary_file(new_paths) - except NoPrimaryFile: - # No clear primary file, just use the first one - new_primary = new_paths[0] if new_paths else None + new_primary = choose_primary_file(new_paths) if new_primary: modified_ids_to_update.append((diff.file_group_id, str(new_primary)))