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

feat: optimize update_from_dict #75

Merged
merged 7 commits into from
Jun 17, 2024
Merged

feat: optimize update_from_dict #75

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
681d0a4
feat: optimize update_from_dict
bdraco Jun 17, 2024
29c931e
feat: optimize update_from_dict
bdraco Jun 17, 2024
ae318ee
feat: optimize update_from_dict
bdraco Jun 17, 2024
2903ad2
feat: optimize update_from_dict
bdraco Jun 17, 2024
2784c1d
fix: handle none
bdraco Jun 17, 2024
b8f01c4
fix: handle none
bdraco Jun 17, 2024
6367dd1
fix: makes no sense to inject into dicts that will not be objects
bdraco Jun 17, 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
106 changes: 46 additions & 60 deletions src/uiprotect/data/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,68 +485,54 @@

return new_data

def _inject_api(
self,
data: dict[str, Any],
api: ProtectApiClient | None,
) -> dict[str, Any]:
data["api"] = api
unifi_objs_sets = self._get_protect_objs_set()
has_unifi_objs = bool(unifi_objs_sets)
unifi_lists_sets = self._get_protect_lists_set()
has_unifi_lists = bool(unifi_lists_sets)
unifi_dicts_sets = self._get_protect_dicts_set()
has_unifi_dicts = bool(unifi_dicts_sets)
for key, value in data.items():
if has_unifi_objs and key in unifi_objs_sets and isinstance(value, dict):
value["api"] = api
elif (
has_unifi_lists and key in unifi_lists_sets and isinstance(value, list)
):
for item in value:
if isinstance(item, dict):
item["api"] = api
elif (
has_unifi_dicts and key in unifi_dicts_sets and isinstance(value, dict)
):
for item in value.values():
if isinstance(item, dict):
item["api"] = api
def update_from_dict(cls: ProtectObject, data: dict[str, Any]) -> ProtectObject:
"""
Updates current object from a cleaned UFP JSON dict.

return data
The api client is injected into each dict for any child
UFP objects that are detected.
"""
unifi_objs = cls._get_protect_objs()
has_unifi_objs = bool(unifi_objs)
unifi_lists = cls._get_protect_lists()
has_unifi_lists = bool(unifi_lists)
unifi_dicts_sets = cls._get_protect_dicts_set()
has_unifi_dicts = bool(unifi_dicts_sets)

def update_from_dict(self: ProtectObject, data: dict[str, Any]) -> ProtectObject:
"""Updates current object from a cleaned UFP JSON dict"""
data_set = set(data)
for key in self._get_protect_objs_set().intersection(data_set):
unifi_obj: Any | None = getattr(self, key)
if unifi_obj is not None and isinstance(unifi_obj, ProtectBaseObject):
item = data.pop(key)
if item is not None:
item = unifi_obj.update_from_dict(item)
setattr(self, key, item)

data = self._inject_api(data, self._api)
unifi_lists = self._get_protect_lists()
for key in self._get_protect_lists_set().intersection(data_set):
if not isinstance(data[key], list):
continue
klass = unifi_lists[key]
new_items = []
for item in data.pop(key):
if item is not None and isinstance(item, ProtectBaseObject):
new_items.append(item)
elif isinstance(item, dict):
new_items.append(klass(**item))
setattr(self, key, new_items)

# Always injected above
del data["api"]

for key in data:
setattr(self, key, convert_unifi_data(data[key], self.__fields__[key]))

return self
api = cls._api
_fields = cls.__fields__
unifi_obj: ProtectBaseObject
if "api" in data:
del data["api"]
value: Any

for key, item in data.items():
if has_unifi_objs and key in unifi_objs and isinstance(item, dict):
item["api"] = api
unifi_obj = getattr(cls, key)
value = unifi_obj.update_from_dict(item)
elif has_unifi_lists and key in unifi_lists and isinstance(item, list):
klass = unifi_lists[key]

Check warning on line 515 in src/uiprotect/data/base.py

View check run for this annotation

Codecov / codecov/patch

src/uiprotect/data/base.py#L515 

Added line #L515 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

The line where a new instance of a class is created inside a list comprehension was not covered by tests. This is a crucial part of the functionality as it involves data transformation and API injection. It is recommended to add unit tests to cover this scenario to ensure that the creation and initialization of objects in lists are handled correctly.

Would you like assistance in writing these unit tests?

Tools
GitHub Check: codecov/patch

[warning] 515-515: src/uiprotect/data/base.py#L515
Added line #L515 was not covered by tests

value = [
klass(**i, api=api) if isinstance(i, dict) else i
for i in item
if i is not None and isinstance(i, (dict, ProtectBaseObject))
]
else:
# Inject the api if the key is in the unifi_dicts_sets
if (
has_unifi_dicts
and key in unifi_dicts_sets
and isinstance(item, dict)
):
for i in item.values():
if isinstance(i, dict):
i["api"] = api

Check warning on line 530 in src/uiprotect/data/base.py

View check run for this annotation

Codecov / codecov/patch

src/uiprotect/data/base.py#L530 

Added line #L530 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the previous comment, this line where the API client is injected into dictionary values was not covered by tests. Given that this is a key part of the API injection logic, comprehensive testing is essential to ensure that the API client is correctly and safely injected into all applicable objects.

I can help draft these unit tests or open a GitHub issue to track this task. Let me know how you would like to proceed.

Tools
GitHub Check: codecov/patch

[warning] 530-530: src/uiprotect/data/base.py#L530
Added line #L530 was not covered by tests

value = convert_unifi_data(item, _fields[key])

setattr(cls, key, value)

return cls
Copy link
Contributor

Choose a reason for hiding this comment

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

The refactored update_from_dict method integrates API injection directly into the data iteration process, which should indeed optimize performance by reducing the number of iterations. However, there are several areas that could be further improved for clarity and efficiency:

  1. The method uses getattr(cls, key) which might raise an AttributeError if key does not actually correspond to an attribute of cls. It might be safer to use getattr(cls, key, None) and check if the result is None before proceeding.
  2. The deletion of the 'api' key directly in the data dictionary could lead to side-effects if the data dictionary is used elsewhere after this method call. It might be better to work with a copy of the data.
  3. The method does not handle potential exceptions from convert_unifi_data, which might raise errors if the data is not in the expected format.

Consider adding error handling and working with a copy of the data to avoid side-effects.

+       data = data.copy()  # Work with a copy to avoid side-effects
+       try:
+               if unifi_obj is None:
+                   continue  # Skip if the attribute does not exist
+       except Exception as e:
+           _LOGGER.error("Failed to update from dict: %s", e)
+           raise

Committable suggestion was skipped due to low confidence.

Tools
GitHub Check: codecov/patch

[warning] 515-515: src/uiprotect/data/base.py#L515
Added line #L515 was not covered by tests

[warning] 530-530: src/uiprotect/data/base.py#L530
Added line #L530 was not covered by tests


def dict_with_excludes(self) -> dict[str, Any]:
"""Returns a dict of the current object without any UFP objects converted to dicts."""
Expand Down
Loading