Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add support for the new download mechanism used in Unifi OS 4 #38

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
+195 −9
Open

feat: add support for the new download mechanism used in Unifi OS 4 #38

Changes from 15 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
b0c5171
Update api.py
cyberpower678 Jun 11, 2024
6edcb19
chore(pre-commit.ci): auto fixes
pre-commit-ci[bot] Jun 11, 2024
dda6f72
Fix code rabbit suggestions and lint errors
cyberpower678 Jun 11, 2024
9f48751
Update api.py
cyberpower678 Jun 11, 2024
98274fa
chore(pre-commit.ci): auto fixes
pre-commit-ci[bot] Jun 11, 2024
c08044e
Merge branch 'main' into main
bdraco Jun 12, 2024
23c3bfe
Merge branch 'main' into main
cyberpower678 Jun 12, 2024
e8b1c00
Merge branch 'main' into main
bdraco Jun 12, 2024
1c09589
Merge branch 'main' into main
bdraco Jun 13, 2024
02a4dca
Merge branch 'main' into main
bdraco Jun 13, 2024
cfc5f35
Merge branch 'main' into main
bdraco Jun 14, 2024
ba488c1
Merge branch 'main' into main
bdraco Jun 17, 2024
5351838
Merge branch 'main' into main
bdraco Jun 17, 2024
3e0096e
Merge branch 'main' into main
bdraco Jun 17, 2024
28faec8
Merge branch 'main' into main
bdraco Jun 21, 2024
02ab2ea
Merge branch 'main' into main
bdraco Jun 23, 2024
31a6eef
Merge branch 'main' into main
bdraco Jun 23, 2024
8aa92bb
Merge branch 'main' into main
bdraco Jun 25, 2024
694064a
Merge branch 'main' into main
bdraco Jul 2, 2024
692a550
Merge branch 'main' into main
bdraco Jul 16, 2024
a8a31e4
Merge branch 'main' into main
bdraco Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
204 changes: 195 additions & 9 deletions src/uiprotect/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1432,7 +1432,134 @@ async def _stream_response(
if progress_callback is not None:
await progress_callback(step, current, total)

async def get_camera_video(
async def download_camera_video(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests for the new methods

self,
camera_id: str,
filename: str,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
) -> bytes:
"""
Downloads a prepared MP4 video from a given camera.

This is the newer implementation of retrieving video from Unifi Protect.
You need to supply the filename returned from prepare_camera_video().

It is recommended to provide an output file or progress callback for larger
video clips, otherwise the full video must be downloaded to memory before
being written.
"""
Comment on lines +1483 to +1501
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enhance error handling and optimize memory usage in download_camera_video.

  1. Memory Usage: Consider always writing to a temporary file if output_file is not provided to avoid high memory consumption.
  2. Error Handling: Add error handling to manage potential failures in network requests or file operations.
  3. Test Coverage: The function lacks test coverage for several lines, as indicated by static analysis.

Here's a proposed improvement for handling memory usage:

import tempfile
from pathlib import Path

async def download_camera_video(
    self,
    camera_id: str,
    filename: str,
    output_file: Path | None = None,
    iterator_callback: IteratorCallback | None = None,
    progress_callback: ProgressCallback | None = None,
    chunk_size: int = 65536,
) -> bytes:
    if output_file is None:
        temp_file = Path(tempfile.mkstemp()[1])
        await self._download_to_file(camera_id, filename, temp_file, iterator_callback, progress_callback, chunk_size)
        with open(temp_file, 'rb') as file:
            return file.read()
    else:
        await self._download_to_file(camera_id, filename, output_file, iterator_callback, progress_callback, chunk_size)
    return None

async def _download_to_file(
    self,
    camera_id: str,
    filename: str,
    output_file: Path,
    iterator_callback: IteratorCallback | None,
    progress_callback: ProgressCallback | None,
    chunk_size: int
) -> None:
    # Existing logic for downloading to a file

path = "video/download"

params = {
"camera": camera_id,
"filename": filename,
}

if (
iterator_callback is None
and progress_callback is None
and output_file is None
):
return await self.api_request_raw(
path,
params=params,
raise_exception=False,
)
Comment on lines +1483 to +1518
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential memory usage issue when downloading large video files

The method download_camera_video allows downloading large video files directly into memory if neither iterator_callback, progress_callback, nor output_file are provided. This can lead to high memory usage which might cause performance issues or crashes, especially for large video files.

- if (iterator_callback is None and progress_callback is None and output_file is None):
+ if output_file is None:
+     temp_file = Path(tempfile.mkstemp()[1])
+     result = await self.download_camera_video(
+         camera_id=camera_id,
+         filename=filename,
+         output_file=temp_file,
+         iterator_callback=iterator_callback,
+         progress_callback=progress_callback,
+         chunk_size=chunk_size,
+     )
+     with open(temp_file, 'rb') as file:
+         return file.read()
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def download_camera_video(
self,
camera_id: str,
filename: str,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
) -> bytes:
"""
Downloads a prepared MP4 video from a given camera.
This is the newer implementation of retrieving video from Unifi Protect.
You need to supply the filename returned from prepare_camera_video().
It is recommended to provide an output file or progress callback for larger
video clips, otherwise the full video must be downloaded to memory before
being written.
"""
path = "video/download"
params = {
"camera": camera_id,
"filename": filename,
}
if (
iterator_callback is None
and progress_callback is None
and output_file is None
):
return await self.api_request_raw(
path,
params=params,
raise_exception=False,
)
async def download_camera_video(
self,
camera_id: str,
filename: str,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
) -> bytes:
"""
Downloads a prepared MP4 video from a given camera.
This is the newer implementation of retrieving video from Unifi Protect.
You need to supply the filename returned from prepare_camera_video().
It is recommended to provide an output file or progress callback for larger
video clips, otherwise the full video must be downloaded to memory before
being written.
"""
path = "video/download"
params = {
"camera": camera_id,
"filename": filename,
}
if output_file is None:
temp_file = Path(tempfile.mkstemp()[1])
result = await self.download_camera_video(
camera_id=camera_id,
filename=filename,
output_file=temp_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
)
with open(temp_file, 'rb') as file:
return file.read()
Tools
GitHub Check: codecov/patch

[warning] 1499-1499: src/uiprotect/api.py#L1499
Added line #L1499 was not covered by tests

[warning] 1501-1501: src/uiprotect/api.py#L1501
Added line #L1501 was not covered by tests

[warning] 1511-1511: src/uiprotect/api.py#L1511
Added line #L1511 was not covered by tests


r = await self._os.request(
"get",
urljoin(self.api_path, path),
auto_close=False,
timeout=0,
params=params,
)
if output_file is not None:
async with aiofiles.open(output_file, "wb") as output:

async def callback(total: int, chunk: bytes | None) -> None:
if iterator_callback is not None:
await iterator_callback(total, chunk)
if chunk is not None:
await output.write(chunk)

await self._stream_response(r, chunk_size, callback, progress_callback)
else:
await self._stream_response(
r,
chunk_size,
iterator_callback,
progress_callback,
)
r.close()
return None
Comment on lines +1483 to +1545
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize memory usage and error handling in download_camera_video.

  1. Memory Usage: When neither iterator_callback, progress_callback, nor output_file are provided, the method downloads the entire video into memory. This could lead to high memory usage, especially for large video files. Consider always writing to a temporary file if output_file is not provided to avoid high memory consumption.

  2. Error Handling: The method lacks explicit error handling for network requests and file operations, which could lead to unhandled exceptions. Consider adding error handling to manage potential failures in network requests or file operations.

  3. Resource Management: Ensure that the response object r is always properly closed, even in the case of exceptions. This can be achieved by using a context manager or ensuring r.close() is called in a finally block.

async def download_camera_video(
    self,
    camera_id: str,
    filename: str,
    output_file: Path | None = None,
    iterator_callback: IteratorCallback | None = None,
    progress_callback: ProgressCallback | None = None,
    chunk_size: int = 65536,
) -> bytes | None:
    path = "video/download"
    params = {
        "camera": camera_id,
        "filename": filename,
    }

    if (
        iterator_callback is None
        and progress_callback is None
        and output_file is None
    ):
        return await self.api_request_raw(
            path,
            params=params,
            raise_exception=False,
        )

    try:
        r = await self._os.request(
            "get",
            urljoin(self.api_path, path),
            auto_close=False,
            timeout=0,
            params=params,
        )
        if output_file is not None:
            async with aiofiles.open(output_file, "wb") as output:
                async def callback(total: int, chunk: bytes | None) -> None:
                    if iterator_callback is not None:
                        await iterator_callback(total, chunk)
                    if chunk is not None:
                        await output.write(chunk)
                await self._stream_response(r, chunk_size, callback, progress_callback)
        else:
            await self._stream_response(
                r,
                chunk_size,
                iterator_callback,
                progress_callback,
            )
        r.close()
    except Exception as e:
        _LOGGER.error(f"Failed to download video: {str(e)}")
        raise
    return None
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def download_camera_video(
self,
camera_id: str,
filename: str,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
) -> bytes:
"""
Downloads a prepared MP4 video from a given camera.
This is the newer implementation of retrieving video from Unifi Protect.
You need to supply the filename returned from prepare_camera_video().
It is recommended to provide an output file or progress callback for larger
video clips, otherwise the full video must be downloaded to memory before
being written.
"""
path = "video/download"
params = {
"camera": camera_id,
"filename": filename,
}
if (
iterator_callback is None
and progress_callback is None
and output_file is None
):
return await self.api_request_raw(
path,
params=params,
raise_exception=False,
)
r = await self._os.request(
"get",
urljoin(self.api_path, path),
auto_close=False,
timeout=0,
params=params,
)
if output_file is not None:
async with aiofiles.open(output_file, "wb") as output:
async def callback(total: int, chunk: bytes | None) -> None:
if iterator_callback is not None:
await iterator_callback(total, chunk)
if chunk is not None:
await output.write(chunk)
await self._stream_response(r, chunk_size, callback, progress_callback)
else:
await self._stream_response(
r,
chunk_size,
iterator_callback,
progress_callback,
)
r.close()
return None
async def download_camera_video(
self,
camera_id: str,
filename: str,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
) -> bytes | None:
path = "video/download"
params = {
"camera": camera_id,
"filename": filename,
}
if (
iterator_callback is None
and progress_callback is None
and output_file is None
):
return await self.api_request_raw(
path,
params=params,
raise_exception=False,
)
try:
r = await self._os.request(
"get",
urljoin(self.api_path, path),
auto_close=False,
timeout=0,
params=params,
)
if output_file is not None:
async with aiofiles.open(output_file, "wb") as output:
async def callback(total: int, chunk: bytes | None) -> None:
if iterator_callback is not None:
await iterator_callback(total, chunk)
if chunk is not None:
await output.write(chunk)
await self._stream_response(r, chunk_size, callback, progress_callback)
else:
await self._stream_response(
r,
chunk_size,
iterator_callback,
progress_callback,
)
r.close()
except Exception as e:
_LOGGER.error(f"Failed to download video: {str(e)}")
raise
return None


async def prepare_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
fps: int | None = None,
filename: str | None = None,
) -> list[Any] | dict[str, Any] | None:
"""
Prepares MP4 video from a given camera at a specific time.

This is the newer implementation of retrieving video from Unifi Protect.
When using this method, it should be followed up with download_camera_video().

Start/End of video export are approximate. It may be +/- a few seconds.

Providing the `fps` parameter creates a "timelapse" export wtih the given FPS
value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x
(fps=20), and 600x (fps=40).

You will receive a filename and an expiry time in seconds.
"""
if validate_channel_id and self._bootstrap is not None:
try:
camera = self._bootstrap.cameras[camera_id]
camera.channels[channel_index]
except IndexError as e:
raise BadRequest from e

Comment on lines +1547 to +1577
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enhance error handling in prepare_camera_video method.

The method catches IndexError to handle invalid channel_index but does not account for other potential issues such as KeyError if camera_id is not valid. Improve the error handling to cover these cases.

-            except IndexError as e:
-                raise BadRequest from e
+            except (IndexError, KeyError) as e:
+                raise BadRequest(f"Invalid input: {str(e)}") from e
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def prepare_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
fps: int | None = None,
filename: str | None = None,
) -> list[Any] | dict[str, Any] | None:
"""
Prepares MP4 video from a given camera at a specific time.
This is the newer implementation of retrieving video from Unifi Protect.
When using this method, it should be followed up with download_camera_video().
Start/End of video export are approximate. It may be +/- a few seconds.
Providing the `fps` parameter creates a "timelapse" export wtih the given FPS
value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x
(fps=20), and 600x (fps=40).
You will receive a filename and an expiry time in seconds.
"""
if validate_channel_id and self._bootstrap is not None:
try:
camera = self._bootstrap.cameras[camera_id]
camera.channels[channel_index]
except IndexError as e:
raise BadRequest from e
if validate_channel_id and self._bootstrap is not None:
try:
camera = self._bootstrap.cameras[camera_id]
camera.channels[channel_index]
except (IndexError, KeyError) as e:
raise BadRequest(f"Invalid input: {str(e)}") from e

params = {
"camera": camera_id,
"start": to_js_time(start),
"end": to_js_time(end),
}

if channel_index == 3:
params.update({"lens": 2})
else:
params.update({"channel": channel_index})

if fps is not None and fps > 0:
params["fps"] = fps
params["type"] = "timelapse"
else:
params["type"] = "rotating"

if not filename:
start_str = start.strftime("%m-%d-%Y, %H.%M.%S %Z")
end_str = end.strftime("%m-%d-%Y, %H.%M.%S %Z")
filename = f"{camera_id} {start_str} - {end_str}.mp4"

params["filename"] = filename

path = "video/prepare"

return await self.api_request(
path,
params=params,
raise_exception=True,
)
Comment on lines +1547 to +1608
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enhance error handling and optimize fps handling in prepare_camera_video.

  1. Error Handling: Add checks to ensure that camera and camera.channels are valid before accessing them to prevent AttributeError or KeyError.
  2. Optimizing fps Handling: Avoid unnecessary dictionary operations by checking if fps is not None before setting params['type'].
  3. Test Coverage: The function lacks test coverage for several lines, as indicated by static analysis.

Here's a proposed improvement for error handling:

if validate_channel_id and self._bootstrap is not None:
    try:
        camera = self._bootstrap.cameras[camera_id]
        if channel_index >= len(camera.channels):
            raise IndexError("Channel index out of range")
    except (KeyError, AttributeError, IndexError) as e:
        raise BadRequest(f"Invalid input: {str(e)}") from e
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def prepare_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
fps: int | None = None,
filename: str | None = None,
) -> list[Any] | dict[str, Any] | None:
"""
Prepares MP4 video from a given camera at a specific time.
This is the newer implementation of retrieving video from Unifi Protect.
When using this method, it should be followed up with download_camera_video().
Start/End of video export are approximate. It may be +/- a few seconds.
Providing the `fps` parameter creates a "timelapse" export wtih the given FPS
value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x
(fps=20), and 600x (fps=40).
You will receive a filename and an expiry time in seconds.
"""
if validate_channel_id and self._bootstrap is not None:
try:
camera = self._bootstrap.cameras[camera_id]
camera.channels[channel_index]
except IndexError as e:
raise BadRequest from e
params = {
"camera": camera_id,
"start": to_js_time(start),
"end": to_js_time(end),
}
if channel_index == 3:
params.update({"lens": 2})
else:
params.update({"channel": channel_index})
if fps is not None and fps > 0:
params["fps"] = fps
params["type"] = "timelapse"
else:
params["type"] = "rotating"
if not filename:
start_str = start.strftime("%m-%d-%Y, %H.%M.%S %Z")
end_str = end.strftime("%m-%d-%Y, %H.%M.%S %Z")
filename = f"{camera_id} {start_str} - {end_str}.mp4"
params["filename"] = filename
path = "video/prepare"
return await self.api_request(
path,
params=params,
raise_exception=True,
)
async def prepare_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
fps: int | None = None,
filename: str | None = None,
) -> list[Any] | dict[str, Any] | None:
"""
Prepares MP4 video from a given camera at a specific time.
This is the newer implementation of retrieving video from Unifi Protect.
When using this method, it should be followed up with download_camera_video().
Start/End of video export are approximate. It may be +/- a few seconds.
Providing the `fps` parameter creates a "timelapse" export wtih the given FPS
value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x
(fps=20), and 600x (fps=40).
You will receive a filename and an expiry time in seconds.
"""
if validate_channel_id and self._bootstrap is not None:
try:
camera = self._bootstrap.cameras[camera_id]
if channel_index >= len(camera.channels):
raise IndexError("Channel index out of range")
except (KeyError, AttributeError, IndexError) as e:
raise BadRequest(f"Invalid input: {str(e)}") from e
params = {
"camera": camera_id,
"start": to_js_time(start),
"end": to_js_time(end),
}
if channel_index == 3:
params.update({"lens": 2})
else:
params.update({"channel": channel_index})
if fps is not None and fps > 0:
params["fps"] = fps
params["type"] = "timelapse"
else:
params["type"] = "rotating"
if not filename:
start_str = start.strftime("%m-%d-%Y, %H.%M.%S %Z")
end_str = end.strftime("%m-%d-%Y, %H.%M.%S %Z")
filename = f"{camera_id} {start_str} - {end_str}.mp4"
params["filename"] = filename
path = "video/prepare"
return await self.api_request(
path,
params=params,
raise_exception=True,
)
Tools
GitHub Check: codecov/patch

[warning] 1575-1576: src/uiprotect/api.py#L1575-L1576
Added lines #L1575 - L1576 were not covered by tests

[warning] 1585-1585: src/uiprotect/api.py#L1585
Added line #L1585 was not covered by tests

[warning] 1590-1591: src/uiprotect/api.py#L1590-L1591
Added lines #L1590 - L1591 were not covered by tests


async def export_camera_video(
self,
camera_id: str,
start: datetime,
Expand All @@ -1444,13 +1571,13 @@ async def get_camera_video(
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
fps: int | None = None,
) -> bytes | None:
) -> bytes:
"""
Exports MP4 video from a given camera at a specific time.

Start/End of video export are approximate. It may be +/- a few seconds.

It is recommended to provide a output file or progress callback for larger
It is recommended to provide an output file or progress callback for larger
video clips, otherwise the full video must be downloaded to memory before
being written.

Expand All @@ -1459,8 +1586,8 @@ async def get_camera_video(
(fps=20), and 600x (fps=40).
"""
if validate_channel_id and self._bootstrap is not None:
camera = self._bootstrap.cameras[camera_id]
try:
camera = self._bootstrap.cameras[camera_id]
camera.channels[channel_index]
except IndexError as e:
raise BadRequest from e
Expand All @@ -1471,16 +1598,17 @@ async def get_camera_video(
"end": to_js_time(end),
}

if fps is not None:
params["fps"] = fps
params["type"] = "timelapse"

if channel_index == 3:
params.update({"lens": 2})
else:
params.update({"channel": channel_index})

if fps is not None and fps > 0:
params["fps"] = fps
params["type"] = "timelapse"

path = "video/export"

if (
iterator_callback is None
and progress_callback is None
Expand All @@ -1492,7 +1620,7 @@ async def get_camera_video(
raise_exception=False,
)

r = await self.request(
r = await self._os.request(
"get",
urljoin(self.api_path, path),
auto_close=False,
Expand All @@ -1519,6 +1647,64 @@ async def callback(total: int, chunk: bytes | None) -> None:
r.close()
return None

async def get_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
fps: int | None = None,
filename: str | None = None,
) -> bytes:
"""
Deprecated: maintained for backwards compatibility.

If you are using Unifi Protect 4 or later, please use
prepare_camera_video() and download_camera_video() instead.
"""
try:
prepare_response = await self.prepare_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
fps=fps,
filename=filename,
)

if isinstance(prepare_response, dict):
download_filename = prepare_response["fileName"]
else:
raise Exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid using a broad exception raise in a try block to control flow


return await self.download_camera_video(
camera_id=camera_id,
filename=download_filename,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid a broad exception catch and only catch exceptions that would confirm the need to fallback to the other method as it can unexpectedly hide bugs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was quite intentional, as it's supposed to fall back on the previous video/export on any kind of error, including using it on a pre-4 Protect installation whose responses I can't predict since I have no platform to test it on. The get_camera_video() needs to be deprecated in favor of either using the export_camera_video method, which is functionally identical, or the new prepare/download_camera_video methods used in Protect 4.

Copy link
Member

@bdraco bdraco Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better for it to fail than to have undefined behavior since if it falls back without warning the problem will never get fixed, and when the deprecated function is removed we will find out the hard way when it breaks which will be harder to fix.

Please narrow the exception catch to only exceptions that are expected that should trigger the fallback.

https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/broad-exception-caught.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs a test to simulate a failure so we know under which conditions it should fallback to the old api

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should also log a message when it falls back so we know it's happening so we have a chance to fix whatever is causing the failure

return await self.export_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
fps=fps,
)
Comment on lines +1698 to +1754
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrow down exceptions and add logging in get_camera_video.

  1. Exception Handling: Narrow down the exceptions to those that are expected to occur, such as BadRequest or NvrError. This prevents masking other important exceptions.
  2. Logging: Add logging to capture fallback scenarios for better debugging.
  3. Test Coverage: The function lacks test coverage for certain lines, as indicated by static analysis.

Here's a proposed improvement:

try:
    prepare_response = await self.prepare_camera_video(
        camera_id=camera_id,
        start=start,
        end=end,
        channel_index=channel_index,
        validate_channel_id=validate_channel_id,
        fps=fps,
        filename=filename,
    )

    if isinstance(prepare_response, dict):
        download_filename = prepare_response["fileName"]
    else:
        raise BadRequest("Invalid response from prepare_camera_video")

    return await self.download_camera_video(
        camera_id=camera_id,
        filename=download_filename,
        output_file=output_file,
        iterator_callback=iterator_callback,
        progress_callback=progress_callback,
        chunk_size=chunk_size,
    )
except (BadRequest, NvrError) as e:
    _LOGGER.error(f"Error during video retrieval: {str(e)}")
    return await self.export_camera_video(
        camera_id=camera_id,
        start=start,
        end=end,
        channel_index=channel_index,
        validate_channel_id=validate_channel_id,
        output_file=output_file,
        iterator_callback=iterator_callback,
        progress_callback=progress_callback,
        chunk_size=chunk_size,
        fps=fps,
    )
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def get_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
fps: int | None = None,
filename: str | None = None,
) -> bytes:
"""
Deprecated: maintained for backwards compatibility.
If you are using Unifi Protect 4 or later, please use
prepare_camera_video() and download_camera_video() instead.
"""
try:
prepare_response = await self.prepare_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
fps=fps,
filename=filename,
)
if isinstance(prepare_response, dict):
download_filename = prepare_response["fileName"]
else:
raise Exception
return await self.download_camera_video(
camera_id=camera_id,
filename=download_filename,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
)
except Exception:
return await self.export_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
fps=fps,
)
async def get_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
fps: int | None = None,
filename: str | None = None,
) -> bytes:
"""
Deprecated: maintained for backwards compatibility.
If you are using Unifi Protect 4 or later, please use
prepare_camera_video() and download_camera_video() instead.
"""
try:
prepare_response = await self.prepare_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
fps=fps,
filename=filename,
)
if isinstance(prepare_response, dict):
download_filename = prepare_response["fileName"]
else:
raise BadRequest("Invalid response from prepare_camera_video")
return await self.download_camera_video(
camera_id=camera_id,
filename=download_filename,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
)
except (BadRequest, NvrError) as e:
_LOGGER.error(f"Error during video retrieval: {str(e)}")
return await self.export_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
fps=fps,
)
Tools
GitHub Check: codecov/patch

[warning] 1732-1732: src/uiprotect/api.py#L1732
Added line #L1732 was not covered by tests

[warning] 1734-1734: src/uiprotect/api.py#L1734
Added line #L1734 was not covered by tests


Comment on lines +1698 to +1755
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrow down exceptions and add logging in get_camera_video.

The method catches a broad Exception, which could mask other important exceptions that should be handled specifically. Narrow down the exceptions to those that are expected to occur. Additionally, add logging to capture fallback scenarios for better debugging.

async def get_camera_video(
    self,
    camera_id: str,
    start: datetime,
    end: datetime,
    channel_index: int = 0,
    validate_channel_id: bool = True,
    output_file: Path | None = None,
    iterator_callback: IteratorCallback | None = None,
    progress_callback: ProgressCallback | None = None,
    chunk_size: int = 65536,
    fps: int | None = None,
    filename: str | None = None,
) -> bytes | None:
    """
    Deprecated: maintained for backwards compatibility.

    If you are using Unifi Protect 4 or later, please use
    prepare_camera_video() and download_camera_video() instead.
    """
    try:
        prepare_response = await self.prepare_camera_video(
            camera_id=camera_id,
            start=start,
            end=end,
            channel_index=channel_index,
            validate_channel_id=validate_channel_id,
            fps=fps,
            filename=filename,
        )

        if isinstance(prepare_response, dict):
            download_filename = prepare_response["fileName"]
        else:
            raise BadRequest("Invalid response from prepare_camera_video")

        return await self.download_camera_video(
            camera_id=camera_id,
            filename=download_filename,
            output_file=output_file,
            iterator_callback=iterator_callback,
            progress_callback=progress_callback,
            chunk_size=chunk_size,
        )
    except (BadRequest, NvrError) as e:
        _LOGGER.error(f"Error during video retrieval: {str(e)}")
        return await self.export_camera_video(
            camera_id=camera_id,
            start=start,
            end=end,
            channel_index=channel_index,
            validate_channel_id=validate_channel_id,
            output_file=output_file,
            iterator_callback=iterator_callback,
            progress_callback=progress_callback,
            chunk_size=chunk_size,
            fps=fps,
        )
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def get_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
fps: int | None = None,
filename: str | None = None,
) -> bytes:
"""
Deprecated: maintained for backwards compatibility.
If you are using Unifi Protect 4 or later, please use
prepare_camera_video() and download_camera_video() instead.
"""
try:
prepare_response = await self.prepare_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
fps=fps,
filename=filename,
)
if isinstance(prepare_response, dict):
download_filename = prepare_response["fileName"]
else:
raise Exception
return await self.download_camera_video(
camera_id=camera_id,
filename=download_filename,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
)
except Exception:
return await self.export_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
fps=fps,
)
async def get_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
fps: int | None = None,
filename: str | None = None,
) -> bytes | None:
"""
Deprecated: maintained for backwards compatibility.
If you are using Unifi Protect 4 or later, please use
prepare_camera_video() and download_camera_video() instead.
"""
try:
prepare_response = await self.prepare_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
fps=fps,
filename=filename,
)
if isinstance(prepare_response, dict):
download_filename = prepare_response["fileName"]
else:
raise BadRequest("Invalid response from prepare_camera_video")
return await self.download_camera_video(
camera_id=camera_id,
filename=download_filename,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
)
except (BadRequest, NvrError) as e:
_LOGGER.error(f"Error during video retrieval: {str(e)}")
return await self.export_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
fps=fps,
)

async def _get_image_with_retry(
self,
path: str,
Expand Down
Loading