diff --git a/pymkv/MKVFile.py b/pymkv/MKVFile.py index 31f20f5..50fa4cc 100644 --- a/pymkv/MKVFile.py +++ b/pymkv/MKVFile.py @@ -657,11 +657,10 @@ def add_track(self, track: str | MKVTrack, new_file: bool = True) -> None: >>> mkv.add_track(track) # doctest: +SKIP """ if isinstance(track, str): - new_track = MKVTrack( - track, - mkvmerge_path=self.mkvmerge_path, - existing_info=self._info_json, - ) + # `track` is an arbitrary path, so self._info_json (the mkvmerge output of the file + # this MKVFile was built from) does not describe it. Passing it here would make the + # new track report metadata of track 0 of the original file and skip verification. + new_track = MKVTrack(track, mkvmerge_path=self.mkvmerge_path) self._extracted_from_add_track(new_track, new_file) elif isinstance(track, MKVTrack): self._extracted_from_add_track(track, new_file) diff --git a/pymkv/MKVTrack.py b/pymkv/MKVTrack.py index e6706bb..14e445e 100644 --- a/pymkv/MKVTrack.py +++ b/pymkv/MKVTrack.py @@ -66,7 +66,7 @@ from pymkv.models import MkvMergeOutput from pymkv.TypeTrack import get_track_extension from pymkv.utils import prepare_mkvtoolnix_path -from pymkv.Verifications import checking_file_path, get_file_info, verify_supported +from pymkv.Verifications import checking_file_path, get_file_info, verify_mkvmerge if TYPE_CHECKING: from collections.abc import Iterable @@ -189,6 +189,10 @@ def __init__( # noqa: PLR0913 self.mkvmerge_path = prepare_mkvtoolnix_path(mkvmerge_path) # Ensure _info_json is always MkvMergeOutput if possible, or None self._info_json: MkvMergeOutput | None = None + # Path that _info_json describes. None means "not probed yet", which also covers + # caller-supplied existing_info: it is trusted for the first file_path assignment + # and invalidated as soon as the path changes to something else. + self._info_path: str | None = None if existing_info: if isinstance(existing_info, dict): # Convert legacy dict to Struct for strict usage @@ -261,20 +265,40 @@ def file_path(self, file_path: str) -> None: This method checks if the provided file path is valid and supported by mkvmerge. If the file is valid, it sets the file path and resets the track_id to 0. - If existing_info is already set, skip the verify_supported check since the file - was already verified. + + The mkvmerge output is probed once and cached, so the track_id setter does not + have to run mkvmerge a second time. Pointing the track at a different file drops + that cache, so the track always describes the file it currently points at. + + When existing_info was supplied, it is trusted for the first assignment and no + probe happens at all. It stops being trusted as soon as the path changes. Args: file_path (str): The path to the file containing the track. Raises: ValueError: If the file is not a valid Matroska file or is not supported. + FileNotFoundError: If mkvmerge is not available at mkvmerge_path. """ fp = checking_file_path(file_path) - if not self._info_json and not verify_supported(fp, mkvmerge_path=self.mkvmerge_path): - msg = f"The file '{file_path}' is not a valid Matroska file or is not supported." - raise ValueError(msg) + if self._info_path is not None and fp != self._info_path: + # Pointing at a different file, whatever we cached describes the old one. + self._info_json = None + if self._info_json is None: + # verify_supported() would run mkvmerge -J and throw the result away, leaving + # the track_id setter to run it a second time. Probe once and keep the output. + # checking_file_path() above already validated the path, so only the mkvmerge + # binary still needs checking, and that lookup is cached. + if not verify_mkvmerge(self.mkvmerge_path): + msg = "mkvmerge is not at the specified path, add it there or change the mkvmerge_path property" + raise FileNotFoundError(msg) + info = get_file_info(fp, self.mkvmerge_path, check_path=False) + if not info.container.supported: + msg = f"The file '{file_path}' is not a valid Matroska file or is not supported." + raise ValueError(msg) + self._info_json = info self._file_path = fp + self._info_path = fp self.track_id = 0 @property diff --git a/tests/.DS_Store b/tests/.DS_Store new file mode 100644 index 0000000..5008ddf Binary files /dev/null and b/tests/.DS_Store differ diff --git a/tests/test_actions_track.py b/tests/test_actions_track.py index ff558f2..ef6e26c 100644 --- a/tests/test_actions_track.py +++ b/tests/test_actions_track.py @@ -218,7 +218,6 @@ def test_track_init_legacy_dict(dummy_mkv: Path) -> None: def test_track_repr(dummy_mkv: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) rep = repr(t) @@ -234,7 +233,6 @@ def test_track_pts_property(dummy_mkv: Path) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) # pts is 0 by default @@ -246,7 +244,6 @@ def test_track_pts_property(dummy_mkv: Path) -> None: def test_track_tags_setter(dummy_mkv: Path, tmp_path: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) @@ -270,20 +267,16 @@ def test_track_tags_setter(dummy_mkv: Path, tmp_path: Path, single_video_info: M def test_track_file_path_setter_verification_failure(tmp_path: Path) -> None: - # Test that verify_supported failure raises ValueError + """An empty file is reported unsupported by mkvmerge, so the setter must reject it.""" invalid = tmp_path / "invalid.mkv" invalid.touch() - with ( - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=False), - pytest.raises(ValueError, match="not a valid Matroska file"), - ): + with pytest.raises(ValueError, match="not a valid Matroska file"): MKVTrack(str(invalid)) def test_extract_silent_and_path(dummy_mkv: Path, tmp_path: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) output = tmp_path / "extracted" @@ -305,7 +298,6 @@ def test_extract_silent_and_path(dummy_mkv: Path, tmp_path: Path, single_video_i def test_file_id_setter_invalid_type(dummy_mkv: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) with pytest.raises(ValueError, match="file_id must be an integer"): @@ -315,7 +307,6 @@ def test_file_id_setter_invalid_type(dummy_mkv: Path, single_video_info: MkvMerg def test_track_id_setter_out_of_range(dummy_mkv: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) with pytest.raises(IndexError, match="track index out of range"): @@ -325,7 +316,6 @@ def test_track_id_setter_out_of_range(dummy_mkv: Path, single_video_info: MkvMer def test_language_setter_invalid(dummy_mkv: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) with pytest.raises( @@ -338,7 +328,6 @@ def test_language_setter_invalid(dummy_mkv: Path, single_video_info: MkvMergeOut def test_extract_track_name_fallback(dummy_mkv: Path, tmp_path: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) @@ -355,7 +344,6 @@ def test_extract_track_name_fallback(dummy_mkv: Path, tmp_path: Path, single_vid def test_extract_timestamps_silent(dummy_mkv: Path, tmp_path: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) @@ -370,7 +358,6 @@ def test_extract_timestamps_silent(dummy_mkv: Path, tmp_path: Path, single_video def test_extract_timestamps_default_path(dummy_mkv: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): t = MKVTrack(str(dummy_mkv)) @@ -378,3 +365,27 @@ def test_extract_timestamps_default_path(dummy_mkv: Path, single_video_info: Mkv result = t.extract_timestamps(silent=True) assert str(dummy_mkv) in result assert "timestamps" in result + + +def test_file_path_reassignment_refreshes_track_info( + get_path_test_file_two: Path, + get_path_test_srt: Path, +) -> None: + """Pointing a track at another file must describe that file, not the previous one.""" + track = MKVTrack(str(get_path_test_srt)) + assert track.track_type == "subtitles" + + track.file_path = str(get_path_test_file_two) + + assert track.track_type == "video" + assert track.track_codec == MKVTrack(str(get_path_test_file_two)).track_codec + + +def test_file_path_reassignment_verifies_new_file(get_path_test_srt: Path, tmp_path: Path) -> None: + """Verification must run again for the new path, not be skipped by the cached info.""" + track = MKVTrack(str(get_path_test_srt)) + invalid = tmp_path / "invalid.mkv" + invalid.touch() + + with pytest.raises(ValueError, match="not a valid Matroska file"): + track.file_path = str(invalid) diff --git a/tests/test_create_mkv.py b/tests/test_create_mkv.py index c71307e..1610840 100644 --- a/tests/test_create_mkv.py +++ b/tests/test_create_mkv.py @@ -162,7 +162,6 @@ def test_order_tracks_by_file_id_keyerror(single_video_info: MkvMergeOutput) -> with ( patch.object(sys.modules["pymkv.MKVTrack"], "checking_file_path", side_effect=lambda x: x), patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): mkv = MKVFile() t1 = MKVTrack("file1.mkv") @@ -299,3 +298,34 @@ def test_add_track_with_string(get_path_test_file: Path) -> None: mkv = MKVFile() mkv.add_track(str(get_path_test_file)) assert len(mkv.tracks) >= 1 + + +def test_add_track_with_string_reads_its_own_file( + get_path_test_file: Path, + get_path_test_srt: Path, +) -> None: + """A track added by path must describe that path, not the file MKVFile was built from.""" + mkv = MKVFile(str(get_path_test_file)) + original_count = len(mkv.tracks) + + mkv.add_track(str(get_path_test_srt)) + + assert len(mkv.tracks) == original_count + 1 + added = mkv.tracks[-1] + assert added.file_path == str(get_path_test_srt) + assert added.track_type == "subtitles" + assert added.track_codec == MKVTrack(str(get_path_test_srt)).track_codec + + +def test_add_track_with_string_rejects_unsupported_file( + get_path_test_file: Path, + get_base_path: Path, +) -> None: + """Verification must still run for tracks added by path to a populated MKVFile.""" + bogus = get_base_path / "not_a_media_file.txt" + bogus.write_text("definitely not media") + + mkv = MKVFile(str(get_path_test_file)) + + with pytest.raises(ValueError, match="not a valid Matroska file or is not supported"): + mkv.add_track(str(bogus)) diff --git a/tests/test_language_loss_fix.py b/tests/test_language_loss_fix.py index f0b47ca..680e39d 100644 --- a/tests/test_language_loss_fix.py +++ b/tests/test_language_loss_fix.py @@ -7,7 +7,6 @@ def test_language_loss_fix() -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info") as mock_info, - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), patch.object(sys.modules["pymkv.MKVTrack"], "checking_file_path", side_effect=lambda x: x), ): mock_info.return_value.tracks = [MagicMock(id=0, codec="V_MS/VFW/FOURCC", type="video", start_pts=0)] @@ -35,7 +34,6 @@ def test_language_loss_fix() -> None: def test_track_options_command_generation() -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info") as mock_info, - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), patch.object(sys.modules["pymkv.MKVFile"], "verify_mkvmerge", return_value=True), patch.object(sys.modules["pymkv.MKVTrack"], "checking_file_path", side_effect=lambda x: x), ): @@ -66,7 +64,6 @@ def test_track_options_skips_unnormalizable_language_ietf() -> None: # the same case, keeping the read- and write-side APIs consistent. with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info") as mock_info, - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), patch.object(sys.modules["pymkv.MKVFile"], "verify_mkvmerge", return_value=True), patch.object(sys.modules["pymkv.MKVTrack"], "checking_file_path", side_effect=lambda x: x), ): @@ -95,7 +92,6 @@ def test_track_options_emits_und_sentinel_language_ietf() -> None: # tag with ``und`` (rather than silently passing the source through). with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info") as mock_info, - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), patch.object(sys.modules["pymkv.MKVFile"], "verify_mkvmerge", return_value=True), patch.object(sys.modules["pymkv.MKVTrack"], "checking_file_path", side_effect=lambda x: x), ): @@ -123,7 +119,6 @@ def test_track_options_und_ietf_does_not_override_language() -> None: # and write APIs disagreeing about the same track. with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info") as mock_info, - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), patch.object(sys.modules["pymkv.MKVFile"], "verify_mkvmerge", return_value=True), patch.object(sys.modules["pymkv.MKVTrack"], "checking_file_path", side_effect=lambda x: x), ): @@ -151,7 +146,6 @@ def test_track_options_und_subtag_ietf_does_not_override_language() -> None: # ``language``. The write side must not emit the IETF tag in that case. with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info") as mock_info, - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), patch.object(sys.modules["pymkv.MKVFile"], "verify_mkvmerge", return_value=True), patch.object(sys.modules["pymkv.MKVTrack"], "checking_file_path", side_effect=lambda x: x), ): @@ -182,7 +176,6 @@ def test_track_options_name_ietf_does_not_emit_and_reads_consistently() -> None: # ``--language`` at all. with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info") as mock_info, - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), patch.object(sys.modules["pymkv.MKVFile"], "verify_mkvmerge", return_value=True), patch.object(sys.modules["pymkv.MKVTrack"], "checking_file_path", side_effect=lambda x: x), ): diff --git a/tests/test_mkvtrack_language.py b/tests/test_mkvtrack_language.py index 39ed54e..88dea28 100644 --- a/tests/test_mkvtrack_language.py +++ b/tests/test_mkvtrack_language.py @@ -49,7 +49,6 @@ def _make_track(dummy_mkv: Path, info: MkvMergeOutput) -> MKVTrack: """Build an MKVTrack against a dummy file path with mocked file probing.""" with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): return MKVTrack(str(dummy_mkv)) @@ -137,7 +136,6 @@ def test_language_setter_empty_string_raises(dummy_mkv: Path, single_video_info: def test_language_setter_constructor_canonicalizes(dummy_mkv: Path, single_video_info: MkvMergeOutput) -> None: with ( patch.object(sys.modules["pymkv.MKVTrack"], "get_file_info", return_value=single_video_info), - patch.object(sys.modules["pymkv.MKVTrack"], "verify_supported", return_value=True), ): track = MKVTrack(str(dummy_mkv), language="fra") assert track.language == "fre"