feat: add support for new download method in Protect 4.x #249
Conversation
WalkthroughThe changes introduce new methods for video handling in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/uiprotect/api.py (2)
1507-1507: Typo in error message: "support" should be "supported"In the error message, correct "This method is only support" to "This method is only supported".
Apply this diff to fix the typo:
- "This method is only support from Unifi Protect version >= 4.0.0." + "This method is only supported from Unifi Protect version >= 4.0.0."
1583-1583: Typo in docstring: "wtih" should be "with"In the docstring, fix the typo "wtih" to "with".
Apply this diff:
- Providing the `fps` parameter creates a "timelapse" export wtih the given FPS + Providing the `fps` parameter creates a "timelapse" export with the given FPS
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/uiprotect/api.py (6 hunks)
- src/uiprotect/data/types.py (1 hunks)
🔇 Additional comments (2)
src/uiprotect/data/types.py (1)
623-627: Consider documenting the distinction between VideoExportType and RecordingTypeThe new
VideoExportTypeenum appears to duplicate values from the existingRecordingTypeenum:
VideoExportType.TIMELAPSEvsRecordingType.TIMELAPSEVideoExportType.ROTATINGvsRecordingType.CONTINUOUS(note: uses "rotating" value)Please add a docstring to clarify:
- The specific purpose of
VideoExportType- How it differs from
RecordingType- Why we need separate enums for seemingly similar concepts
This will help future maintainers understand the distinction between these types.
Let's verify the usage context of both enums:
src/uiprotect/api.py (1)
1532-1532: Verify the use oftimeout=0in the requestSetting
timeout=0disables the timeout for the request, which may cause the client to hang indefinitely if the server becomes unresponsive. Ensure that this behavior is intentional and won't lead to unwanted consequences.If disabling the timeout is necessary due to potentially long download times, consider adding a comment explaining the reasoning behind this choice for future maintainability.
|
I forgot to run mypy through this so please bare with me |
bdraco
changed the title
src/uiprotect/api.py
Outdated
| path = "video/prepare" | ||
|
|
||
| return await self.api_request( | ||
| path, | ||
| params=params, | ||
| raise_exception=True, | ||
| ) |
There was a problem hiding this comment.
| path = "video/prepare" | |
| return await self.api_request( | |
| path, | |
| params=params, | |
| raise_exception=True, | |
| ) | |
| return await self.api_request( | |
| "video/prepare", | |
| params=params, | |
| raise_exception=True, | |
| ) |
Unnecessary assignment to temp variable
Codecov ReportAttention: Patch coverage is
|
src/uiprotect/api.py
Outdated
|
|
||
| r = await self.request( | ||
| "get", | ||
| f"{self.api_path}{path}", |
There was a problem hiding this comment.
| f"{self.api_path}{path}", | |
| f"{self.api_path}video/download", |
src/uiprotect/api.py
Outdated
| path = "video/download" | ||
|
|
There was a problem hiding this comment.
| path = "video/download" |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
src/uiprotect/api.py (1)
1612-1624: Adjust Lens Parameter Handling for ClarityIn the
prepare_camera_videomethod, the handling ofchannel_indexwhen it equals3is a bit opaque. It's setting thelensparameter to2, but this magic number may confuse future developers. Consider defining a constant or adding a comment to clarify this behavior.Add a comment or define a constant:
if channel_index == 3: + # For channel_index 3, set lens parameter to 2 (e.g., package camera lens) params.update({"lens": 2}) else: params.update({"channel": channel_index})🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1612-1614: src/uiprotect/api.py#L1612-L1614
Added lines #L1612 - L1614 were not covered by tests
[warning] 1616-1616: src/uiprotect/api.py#L1616
Added line #L1616 was not covered by tests
[warning] 1618-1621: src/uiprotect/api.py#L1618-L1621
Added lines #L1618 - L1621 were not covered by tests
[warning] 1623-1623: src/uiprotect/api.py#L1623
Added line #L1623 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/uiprotect/api.py (5 hunks)
- src/uiprotect/data/init.py (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/uiprotect/api.py
[warning] 1509-1510: src/uiprotect/api.py#L1509-L1510
Added lines #L1509 - L1510 were not covered by tests
[warning] 1514-1514: src/uiprotect/api.py#L1514
Added line #L1514 was not covered by tests
[warning] 1516-1516: src/uiprotect/api.py#L1516
Added line #L1516 was not covered by tests
[warning] 1521-1521: src/uiprotect/api.py#L1521
Added line #L1521 was not covered by tests
[warning] 1526-1526: src/uiprotect/api.py#L1526
Added line #L1526 was not covered by tests
[warning] 1532-1532: src/uiprotect/api.py#L1532
Added line #L1532 was not covered by tests
[warning] 1539-1540: src/uiprotect/api.py#L1539-L1540
Added lines #L1539 - L1540 were not covered by tests
[warning] 1542-1546: src/uiprotect/api.py#L1542-L1546
Added lines #L1542 - L1546 were not covered by tests
[warning] 1548-1548: src/uiprotect/api.py#L1548
Added line #L1548 was not covered by tests
[warning] 1550-1550: src/uiprotect/api.py#L1550
Added line #L1550 was not covered by tests
[warning] 1556-1557: src/uiprotect/api.py#L1556-L1557
Added lines #L1556 - L1557 were not covered by tests
[warning] 1562-1562: src/uiprotect/api.py#L1562
Added line #L1562 was not covered by tests
[warning] 1566-1567: src/uiprotect/api.py#L1566-L1567
Added lines #L1566 - L1567 were not covered by tests
[warning] 1593-1594: src/uiprotect/api.py#L1593-L1594
Added lines #L1593 - L1594 were not covered by tests
[warning] 1598-1599: src/uiprotect/api.py#L1598-L1599
Added lines #L1598 - L1599 were not covered by tests
[warning] 1601-1601: src/uiprotect/api.py#L1601
Added line #L1601 was not covered by tests
[warning] 1607-1608: src/uiprotect/api.py#L1607-L1608
Added lines #L1607 - L1608 were not covered by tests
[warning] 1610-1610: src/uiprotect/api.py#L1610
Added line #L1610 was not covered by tests
[warning] 1612-1614: src/uiprotect/api.py#L1612-L1614
Added lines #L1612 - L1614 were not covered by tests
[warning] 1616-1616: src/uiprotect/api.py#L1616
Added line #L1616 was not covered by tests
[warning] 1618-1621: src/uiprotect/api.py#L1618-L1621
Added lines #L1618 - L1621 were not covered by tests
[warning] 1623-1623: src/uiprotect/api.py#L1623
Added line #L1623 was not covered by tests
[warning] 1625-1625: src/uiprotect/api.py#L1625
Added line #L1625 was not covered by tests
[warning] 1627-1627: src/uiprotect/api.py#L1627
Added line #L1627 was not covered by tests
[warning] 1670-1670: src/uiprotect/api.py#L1670
Added line #L1670 was not covered by tests
[warning] 1737-1737: src/uiprotect/api.py#L1737
Added line #L1737 was not covered by tests
[warning] 1747-1748: src/uiprotect/api.py#L1747-L1748
Added lines #L1747 - L1748 were not covered by tests
[warning] 1751-1751: src/uiprotect/api.py#L1751
Added line #L1751 was not covered by tests
[warning] 1753-1753: src/uiprotect/api.py#L1753
Added line #L1753 was not covered by tests
🔇 Additional comments (4)
src/uiprotect/data/__init__.py (3)
69-69: LGTM! Import addition maintains alphabetical order.The VideoExportType import is correctly placed in alphabetical order within the types import group.
149-149: LGTM! Export addition maintains alphabetical order.The VideoExportType export is correctly placed in alphabetical order within the all list.
69-69: Verify the VideoExportType implementation.Let's verify the implementation of VideoExportType enum that's being imported and exported.
Also applies to: 149-149
✅ Verification successful
Let me check the implementation details of VideoExportType by looking at its definition.
VideoExportType implementation is properly defined and used
The VideoExportType is a well-defined enum that:
- Inherits from str and enum.Enum for string-based enumeration
- Includes ValuesEnumMixin for additional functionality
- Has two valid states: TIMELAPSE and ROTATING
- Is correctly used in api.py for type parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the VideoExportType enum implementation and its usage # Test 1: Check the enum definition echo "Checking VideoExportType definition..." ast-grep --pattern 'class VideoExportType' -A 10 # Test 2: Check for any usage of VideoExportType echo "Checking VideoExportType usage..." rg "VideoExportType" --type pyLength of output: 704
Script:
#!/bin/bash # Get the full implementation of VideoExportType enum echo "VideoExportType implementation:" rg -A 10 "class VideoExportType" src/uiprotect/data/types.pyLength of output: 354
src/uiprotect/api.py (1)
1490-1557: Verify Unit Test Coverage for New MethodsThe new methods
download_camera_videoandprepare_camera_videolack unit tests, as indicated by the static analysis hints. To maintain code quality and ensure future reliability, please add unit tests covering these methods.Would you like assistance in generating unit tests for these methods or should I open a GitHub issue to track this task?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1509-1510: src/uiprotect/api.py#L1509-L1510
Added lines #L1509 - L1510 were not covered by tests
[warning] 1514-1514: src/uiprotect/api.py#L1514
Added line #L1514 was not covered by tests
[warning] 1516-1516: src/uiprotect/api.py#L1516
Added line #L1516 was not covered by tests
[warning] 1521-1521: src/uiprotect/api.py#L1521
Added line #L1521 was not covered by tests
[warning] 1526-1526: src/uiprotect/api.py#L1526
Added line #L1526 was not covered by tests
[warning] 1532-1532: src/uiprotect/api.py#L1532
Added line #L1532 was not covered by tests
[warning] 1539-1540: src/uiprotect/api.py#L1539-L1540
Added lines #L1539 - L1540 were not covered by tests
[warning] 1542-1546: src/uiprotect/api.py#L1542-L1546
Added lines #L1542 - L1546 were not covered by tests
[warning] 1548-1548: src/uiprotect/api.py#L1548
Added line #L1548 was not covered by tests
[warning] 1550-1550: src/uiprotect/api.py#L1550
Added line #L1550 was not covered by tests
[warning] 1556-1557: src/uiprotect/api.py#L1556-L1557
Added lines #L1556 - L1557 were not covered by tests
src/uiprotect/api.py
Outdated
| 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. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deprecation Warning for get_camera_video Method
The get_camera_video method is deprecated, but users might not notice this. To help users transition to the new methods, consider adding a DeprecationWarning.
Apply this diff to include a deprecation warning:
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.0.0 or later, please use
`prepare_camera_video()` and `download_camera_video()` instead.
"""
+ import warnings
+ warnings.warn(
+ "get_camera_video() is deprecated and will be removed in a future release. "
+ "Use prepare_camera_video() and download_camera_video() instead.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
if self.bootstrap.nvr.version >= Version("4.0.0"):
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,Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Thats probably a good idea, but we can't do late imports in async code because it does blocking I/O
@coderabbitai remember we can't do late imports in async code because it does blocking I/O
There was a problem hiding this comment.
@bdraco, thanks for the reminder about late imports in async code. To avoid blocking I/O, we can move the import warnings statement to the top of the module. Here's the updated diff:
+import warnings
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.0.0 or later, please use
`prepare_camera_video()` and `download_camera_video()` instead.
"""
warnings.warn(
"get_camera_video() is deprecated and will be removed in a future release. "
"Use `prepare_camera_video()` and `download_camera_video()` instead.",
DeprecationWarning,
stacklevel=2,
)
if self.bootstrap.nvr.version >= Version("4.0.0"):
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,Let me know if this works for you.
✏️ Learnings added
Learnt from: bdraco
PR: uilibs/uiprotect#249
File: src/uiprotect/api.py:1716-1735
Timestamp: 2024-10-27T22:01:02.393Z
Learning: In async functions in `src/uiprotect/api.py`, avoid late imports because they cause blocking I/O. All import statements should be at the top of the module.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@coderabbitai please produce a committable suggestion
There was a problem hiding this comment.
@bdraco, certainly! Here's the updated code with the DeprecationWarning:
At the top of src/uiprotect/api.py, add:
import warningsIn the get_camera_video method, add the warning:
warnings.warn(
"get_camera_video() is deprecated and will be removed in a future release. "
"Use `prepare_camera_video()` and `download_camera_video()` instead.",
DeprecationWarning,
stacklevel=2,
)The updated method would look like this:
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.0.0 or later, please use
`prepare_camera_video()` and `download_camera_video()` instead.
"""
warnings.warn(
"get_camera_video() is deprecated and will be removed in a future release. "
"Use `prepare_camera_video()` and `download_camera_video()` instead.",
DeprecationWarning,
stacklevel=2,
)
if self.bootstrap.nvr.version >= Version("4.0.0"):
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,
)
# Rest of the method...This will issue a DeprecationWarning when get_camera_video is called, helping users transition to the new methods.
There was a problem hiding this comment.
Do we really want to depreciate this? I just took the code from the old PR, but there is nothing wrong with calling this function per se.
I'm kinda dubious about doing this behind the scene switcheroo in the first place. I don't want to introduce some unexpected bug into downstream dependencies
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Generally looks good. Needs some tests to make sure the http requests are sending the expected payloads |
Adds export_camera_video() which is the exact implementation of what get_camera_video() used to be. Adds prepare_camera_video() which is the new implementation used by Unifi Protect 4, and the recommended way to get videos. Adds download_camera_video() which is the follow call that needs to be made after prepare_camera_video() Rewrites get_camera_video() to automatically choose between export or prepare/download to maintain backwards compatability. Signed-off-by: cyberpower678 <[email protected]>
|
I will add some tests in tomorrow as its getting late here now. I have one remaining question about this whole change. Do we really want to replace the old mechanism with the new one silently based on version? I worry if there is some unexpected differences in this new mechansim it will affect downstream consumers. |
Assuming the old methods still work then it makes more sense to add this all as new instead of replacing the old mechanism with the new one silently. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/uiprotect/api.py (1)
1555-1563: Enhance error messages for better debugging.While the validation logic is correct, the error messages could be more descriptive to help identify the specific validation failure.
Apply this diff to improve error messages:
try: camera = self._bootstrap.cameras[camera_id] # type: ignore[union-attr] camera.channels[channel_index] except (IndexError, AttributeError, KeyError) as e: - raise BadRequest(f"Invalid input: {e}") from e + if isinstance(e, KeyError): + raise BadRequest(f"Camera not found: {camera_id}") from e + elif isinstance(e, IndexError): + raise BadRequest(f"Invalid channel index {channel_index} for camera {camera_id}") from e + else: + raise BadRequest(f"Invalid camera or channel: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/uiprotect/api.py (4 hunks)
- src/uiprotect/data/init.py (2 hunks)
- src/uiprotect/data/types.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/uiprotect/data/init.py
- src/uiprotect/data/types.py
🧰 Additional context used
📓 Learnings (1)
src/uiprotect/api.py (1)
Learnt from: bdraco PR: uilibs/uiprotect#249 File: src/uiprotect/api.py:1490-1512 Timestamp: 2024-10-27T22:01:21.967Z Learning: In the `download_camera_video` method in `src/uiprotect/api.py`, prefer using built-in exceptions like `ValueError` over custom exceptions unless there is a compelling reason.
🔇 Additional comments (4)
src/uiprotect/api.py (4)
92-93: LGTM! Well-defined version constant.The constant is appropriately defined and documented, clearly indicating the version where the new download mechanism was introduced.
1488-1510: LGTM! Well-structured method signature and version validation.The method is well-documented and includes proper version validation to ensure compatibility.
1565-1613: LGTM! Well-implemented video preparation method.The method is well-structured with proper parameter handling and clear documentation.
1662-1664: LGTM! Proper integration of VideoExportType.The changes correctly implement timelapse export functionality using the VideoExportType enum.
|
I am trying to write tests, but the built in sample data is pre v4.0.0, so I tried to generate my own data and get the following error trying to run one of the existing tests: There is so much going on here I am struggling to see the wood for the trees. Is this something you have seen before? |
|
Sorry I don’t remember how that’s intended to work. Some discovery will be needed |
|
Just checking, is this an action on myself or you? |
I'm likely not going to be able to do that any time soon so you'll probably get to it before I can |
|
I was wondering if you have had a chance to look into why the tests don't work with data from the latest unifi releases? I am unable to test any tests I write till this is resolved because this PR relies on checking that the version is new enough. |
|
I haven't had a large enough time block free to look at it yet. End of the year is crazy for me. It usually calms down right after the holidays |
Description of change
Adds export_camera_video() which is the exact implementation of what get_camera_video() used to be.
Adds prepare_camera_video() which is the new implementation used by Unifi Protect 4, and the recommended way to get videos.
Adds download_camera_video() which is the follow call that needs to be made after prepare_camera_video()
Rewrites get_camera_video() to automatically choose between export or prepare/download to maintain backwards compatability.
Once we are happy with the functionality, I will add tests & tidy up the commit history with proper messages.
Pull-Request Checklist
mainbranchFixes #0000Summary by CodeRabbit
New Features
Bug Fixes