diff --git a/pymkv/MKVAttachment.py b/pymkv/MKVAttachment.py index 00ae256..8f6c81c 100644 --- a/pymkv/MKVAttachment.py +++ b/pymkv/MKVAttachment.py @@ -56,6 +56,9 @@ class MKVAttachment: attach_once : bool Determines if the attachment should be added to all split files or only the first. Default is False, which will attach to all files. + size : int | None + The size of the attachment in bytes, as reported by the file it was read from. It is ``None`` for an + attachment built from a local path, since that attachment is not part of an MKV yet. """ def __init__( @@ -73,6 +76,7 @@ def __init__( self._attach_once = attach_once self._source_id: int | None = None self._source_file: str | None = None + self._size: int | None = None def __repr__(self) -> str: """ @@ -90,10 +94,16 @@ def __repr__(self) -> str: def file_path(self) -> str: """str: The path to the attachment file. + For an attachment read out of an MKV this is the containing file, and the path cannot be changed. + Replace such an attachment with :meth:`~pymkv.MKVFile.remove_attachment` followed by + :meth:`~pymkv.MKVFile.add_attachment`. + Raises ------ FileNotFoundError Raised if `file_path` does not exist. + ValueError + Raised when changing the path of an attachment that was read from a file. """ return self._file_path @@ -109,17 +119,38 @@ def file_path(self, file_path: str) -> None: ------ FileNotFoundError If the specified file does not exist. + ValueError + If this attachment was read from a file and `file_path` names a different one. Returns ------- None """ + current = getattr(self, "_file_path", None) + if current is not None and str(file_path) == current: + # Byte-identical path. Nothing to validate, and nothing learned about the file has gone stale, + # so this stays a no-op even if the file has since been deleted. + return fp = Path(file_path).expanduser() if not fp.is_file(): msg = f'"{fp}" does not exist' raise FileNotFoundError(msg) + if current is not None and Path(current).is_file() and fp.samefile(current): + # Same file spelled differently, e.g. after the caller normalized the path. + return + if getattr(self, "_source_id", None) is not None: + # This attachment is a reference into an MKV, not a local file. Repointing it used to look like + # it worked while AttachmentOptions skipped it, so the old embedded payload was muxed instead. + msg = ( + f"attachment {self._name!r} (id {self._source_id}) was read from " + f'"{current}", so its path cannot be changed. ' + "Remove this attachment and add a new MKVAttachment for the replacement instead." + ) + raise ValueError(msg) self._mime_type = guess_type(fp)[0] self._name = None + # The name and size described the attachment this object used to point at. + self._size = None self._file_path = str(fp) @property @@ -219,11 +250,43 @@ def source_id(self, source_id: int | None) -> None: """ Set the ID of the attachment from the source file. + Pointing at a different attachment invalidates the size, which described the previous one. + Parameters: source_id (int | None): The ID to set for the attachment in the source file. """ + if source_id != self._source_id: + self._size = None self._source_id = source_id + @property + def size(self) -> int | None: + """ + Get the size of the attachment in bytes. + + The value comes from the file the attachment was read from, not from + :attr:`~pymkv.MKVAttachment.file_path`, which points at the containing MKV for an attachment read + out of one. + + It stays None for an attachment built from a local path, including after + :meth:`~pymkv.MKVFile.mux`: this object is never refreshed from the output. Load the muxed file into + a new :class:`~pymkv.MKVFile` to read the sizes it ended up with. + + Returns: + int | None: The size in bytes, or None for an attachment that was not read from a file. + """ + return self._size + + @size.setter + def size(self, size: int | None) -> None: + """ + Set the size of the attachment in bytes. + + Parameters: + size (int | None): The size in bytes. + """ + self._size = size + @property def source_file(self) -> str | None: """ @@ -239,7 +302,11 @@ def source_file(self, source_file: str | None) -> None: """ Set the path to the source file containing the attachment. + Pointing at a different file invalidates the size, which described the previous one. + Parameters: source_file (str | None): The path to set for the source file. """ + if source_file != self._source_file: + self._size = None self._source_file = source_file diff --git a/pymkv/MKVFile.py b/pymkv/MKVFile.py index 50fa4cc..da4adc3 100644 --- a/pymkv/MKVFile.py +++ b/pymkv/MKVFile.py @@ -246,6 +246,7 @@ def __init__( new_attachment.mime_type = mime_type new_attachment.source_id = attachment_id new_attachment.source_file = file_path + new_attachment.size = attachment.size self.attachments.append(new_attachment) # split options diff --git a/pymkv/models.py b/pymkv/models.py index 3d56799..2b22505 100644 --- a/pymkv/models.py +++ b/pymkv/models.py @@ -158,13 +158,16 @@ class AttachmentInfo(msgspec.Struct): The ID of the attachment. properties : AttachmentProperties The properties of the attachment. + size : int | None + The size of the attachment in bytes. None when mkvmerge did not report one — mkvmerge refuses to + attach an empty file, so a real attachment is never 0 bytes and a missing value must not be read as one. """ id: int file_name: str | None = None content_type: str | None = None description: str | None = None - size: int = 0 + size: int | None = None properties: AttachmentProperties = msgspec.field(default_factory=AttachmentProperties) diff --git a/tests/test_mkv_file_attachments.py b/tests/test_mkv_file_attachments.py index 264c93d..5279d08 100644 --- a/tests/test_mkv_file_attachments.py +++ b/tests/test_mkv_file_attachments.py @@ -1,3 +1,4 @@ +import subprocess from pathlib import Path import msgspec @@ -9,6 +10,9 @@ ATTACHMENT_COUNT_1 = 1 ATTACHMENT_COUNT_2 = 2 ATTACHMENT_COUNT_3 = 3 +SMALL_ATTACHMENT_SIZE = 24 +LARGE_ATTACHMENT_SIZE = 4096 +REPLACEMENT_SIZE = 1234 def test_get_attachment(temp_file: str) -> None: @@ -174,3 +178,211 @@ def test_attachments_preserved_after_mux(temp_file: str, tmp_path: Path, get_pat assert len(output_mkv.attachments) >= ATTACHMENT_COUNT_2, ( f"Expected at least 2 attachments, found {output_mkv.attachments}" ) + + +def _mkv_with_two_attachments(source: Path, tmp_path: Path) -> tuple[str, int, int]: + """Mux two attachments of deliberately different sizes and return the path plus both sizes.""" + small = tmp_path / "small.txt" + small.write_bytes(b"x" * SMALL_ATTACHMENT_SIZE) + large = tmp_path / "large.txt" + large.write_bytes(b"y" * LARGE_ATTACHMENT_SIZE) + + mkv = MKVFile(str(source)) + mkv.add_attachment(MKVAttachment(str(small))) + mkv.add_attachment(MKVAttachment(str(large))) + output = str(tmp_path / "with_attachments.mkv") + mkv.mux(output, silent=True) + return output, SMALL_ATTACHMENT_SIZE, LARGE_ATTACHMENT_SIZE + + +def test_attachment_size_read_from_source(get_path_test_file: Path, tmp_path: Path) -> None: + """Each attachment reports its own payload size, not the size of the MKV holding it.""" + output, small_size, large_size = _mkv_with_two_attachments(get_path_test_file, tmp_path) + container_size = Path(output).stat().st_size + + attachments = MKVFile(output).attachments + + assert [a.size for a in attachments] == [small_size, large_size] + # file_path points at the container, so a size taken from it would be this instead. + assert container_size not in [a.size for a in attachments] + + +def test_attachment_size_is_none_before_muxing(temp_file: str) -> None: + """An attachment built from a local path is not in an MKV yet, so it has no reported size.""" + assert MKVAttachment(temp_file).size is None + + +def test_attachment_size_survives_mux(get_path_test_file: Path, tmp_path: Path) -> None: + output, small_size, large_size = _mkv_with_two_attachments(get_path_test_file, tmp_path) + + mkv = MKVFile(output) + remuxed = str(tmp_path / "remuxed.mkv") + mkv.mux(remuxed, silent=True) + + assert [a.size for a in MKVFile(remuxed).attachments] == [small_size, large_size] + + +def test_attachment_read_from_file_rejects_a_new_path(get_path_test_file: Path, tmp_path: Path) -> None: + """Repointing an embedded attachment used to mux the old payload silently, so it is refused.""" + output, _, _ = _mkv_with_two_attachments(get_path_test_file, tmp_path) + attachment = MKVFile(output).attachments[0] + other = tmp_path / "other.txt" + other.write_bytes(b"z" * LARGE_ATTACHMENT_SIZE) + + with pytest.raises(ValueError, match="cannot be changed"): + attachment.file_path = str(other) + + assert attachment.size == SMALL_ATTACHMENT_SIZE + + +def test_replacing_an_attachment_by_removing_and_adding(get_path_test_file: Path, tmp_path: Path) -> None: + """The supported way to swap an attachment: remove it, add a new one, and the bytes follow.""" + output, _, _ = _mkv_with_two_attachments(get_path_test_file, tmp_path) + replacement = tmp_path / "replacement.bin" + replacement.write_bytes(b"r" * REPLACEMENT_SIZE) + + mkv = MKVFile(output) + mkv.remove_attachment(0) + mkv.add_attachment(MKVAttachment(str(replacement))) + repointed = str(tmp_path / "replaced.mkv") + mkv.mux(repointed, silent=True) + + result = MKVFile(repointed) + by_name = {a.name: a.size for a in result.attachments} + assert by_name.get("replacement.bin") == REPLACEMENT_SIZE + assert "small.txt" not in by_name + + extracted = tmp_path / "extracted.bin" + attachment_id = next(a.source_id for a in result.attachments if a.name == "replacement.bin") + subprocess.run( # noqa: S603 + ["mkvextract", repointed, "attachments", f"{attachment_id}:{extracted}"], # noqa: S607 + check=True, + capture_output=True, + ) + assert extracted.read_bytes() == replacement.read_bytes() + + +def test_local_attachment_path_can_still_change(temp_file: str, tmp_path: Path) -> None: + """An attachment that is not tied to an MKV is still free to point somewhere else.""" + attachment = MKVAttachment(temp_file) + other = tmp_path / "other.bin" + other.write_bytes(b"o" * REPLACEMENT_SIZE) + + attachment.file_path = str(other) + + assert attachment.file_path == str(other) + assert attachment.size is None + + +def test_attachment_size_invalidated_when_source_id_changes(get_path_test_file: Path, tmp_path: Path) -> None: + """source_id selects which embedded attachment is used, so the old size must not survive it.""" + output, _, _ = _mkv_with_two_attachments(get_path_test_file, tmp_path) + attachment = MKVFile(output).attachments[0] + assert attachment.size == SMALL_ATTACHMENT_SIZE + + attachment.source_id = 2 + + assert attachment.size is None + + +def test_attachment_metadata_kept_when_file_path_reassigned_to_itself( + get_path_test_file: Path, + tmp_path: Path, +) -> None: + """Assigning the same path is not a mutation and must not discard what was read from the source.""" + output, _, _ = _mkv_with_two_attachments(get_path_test_file, tmp_path) + attachment = MKVFile(output).attachments[0] + + attachment.file_path = attachment.file_path + + assert attachment.size == SMALL_ATTACHMENT_SIZE + assert attachment.source_id is not None + assert attachment.name == "small.txt" + + +def test_attachment_sizes_with_duplicate_names_and_removal(get_path_test_file: Path, tmp_path: Path) -> None: + """Sizes must follow the attachment, not its name or list index, and survive mkvmerge reindexing.""" + first = tmp_path / "dup" / "шрифт.ttf" + first.parent.mkdir() + first.write_bytes(b"a" * SMALL_ATTACHMENT_SIZE) + second = tmp_path / "шрифт.ttf" + second.write_bytes(b"b" * LARGE_ATTACHMENT_SIZE) + + mkv = MKVFile(str(get_path_test_file)) + mkv.add_attachment(MKVAttachment(str(first))) + mkv.add_attachment(MKVAttachment(str(second))) + output = str(tmp_path / "dupes.mkv") + mkv.mux(output, silent=True) + + loaded = MKVFile(output) + assert [a.name for a in loaded.attachments] == ["шрифт.ttf", "шрифт.ttf"] + assert [a.size for a in loaded.attachments] == [SMALL_ATTACHMENT_SIZE, LARGE_ATTACHMENT_SIZE] + + loaded.remove_attachment(0) + remuxed = str(tmp_path / "dupes_trimmed.mkv") + loaded.mux(remuxed, silent=True) + + remaining = MKVFile(remuxed).attachments + assert [a.size for a in remaining] == [LARGE_ATTACHMENT_SIZE] + + +def test_attachment_size_after_dropping_source_attachments(get_path_test_file: Path, tmp_path: Path) -> None: + """Dropping the inherited attachments and adding a new one leaves only the new size.""" + output, _, _ = _mkv_with_two_attachments(get_path_test_file, tmp_path) + fresh = tmp_path / "fresh.bin" + fresh.write_bytes(b"f" * 777) + + mkv = MKVFile(output) + mkv.remove_all_attachments() + mkv.no_attachments() + mkv.add_attachment(MKVAttachment(str(fresh))) + trimmed = str(tmp_path / "only_fresh.mkv") + mkv.mux(trimmed, silent=True) + + attachments = MKVFile(trimmed).attachments + assert [(a.name, a.size) for a in attachments] == [("fresh.bin", 777)] + + +def test_attachment_path_may_be_reassigned_in_another_spelling(get_path_test_file: Path, tmp_path: Path) -> None: + """A different spelling of the same file is not a change, so it must not be refused.""" + output, _, _ = _mkv_with_two_attachments(get_path_test_file, tmp_path) + attachment = MKVFile(output).attachments[0] + # Path keeps ".." segments verbatim, so this is a different string naming the same file. + detour = tmp_path / "detour" + detour.mkdir() + spelled_differently = str(detour / ".." / Path(output).name) + assert spelled_differently != output + + attachment.file_path = spelled_differently + + assert attachment.size == SMALL_ATTACHMENT_SIZE + assert attachment.name == "small.txt" + assert attachment.source_id is not None + + +def test_attachment_path_reassignment_survives_a_deleted_source(get_path_test_file: Path, tmp_path: Path) -> None: + """An identical path is a no-op even when the container is gone, rather than FileNotFoundError.""" + output, _, _ = _mkv_with_two_attachments(get_path_test_file, tmp_path) + attachment = MKVFile(output).attachments[0] + Path(output).unlink() + + attachment.file_path = attachment.file_path + + assert attachment.size == SMALL_ATTACHMENT_SIZE + + +def test_attachment_repoint_error_identifies_the_attachment(get_path_test_file: Path, tmp_path: Path) -> None: + """Iterating attachments and catching the error must show which one refused.""" + output, _, _ = _mkv_with_two_attachments(get_path_test_file, tmp_path) + other = tmp_path / "other.bin" + other.write_bytes(b"o" * REPLACEMENT_SIZE) + + messages = [] + for attachment in MKVFile(output).attachments: + with pytest.raises(ValueError, match="cannot be changed") as excinfo: + attachment.file_path = str(other) + messages.append(str(excinfo.value)) + + assert "small.txt" in messages[0] + assert "large.txt" in messages[1] + assert messages[0] != messages[1] diff --git a/tests/test_models.py b/tests/test_models.py index d448118..5286860 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -60,7 +60,8 @@ def test_attachment_info_defaults() -> None: assert ai.file_name is None assert ai.content_type is None assert ai.description is None - assert ai.size == 0 + # A missing size stays unknown. mkvmerge refuses to attach an empty file, so 0 would be a fabricated value. + assert ai.size is None assert isinstance(ai.properties, AttachmentProperties)