Serialize complex ZCL attribute types for database persistence#1792
Serialize complex ZCL attribute types for database persistence#1792TheJulianJES wants to merge 9 commits intozigpy:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Zigpy’s SQLite persistence of ZCL attribute cache entries by adding a serialization/deserialization layer for complex ZCL attribute value types that SQLite can’t store directly (e.g., LVList[...]). This addresses persistence failures seen with some custom/manufacturer clusters (notably ubisys-style array/list attributes) across restarts.
Changes:
- Add
_serialize_for_db()to convert non-SQLite-native attribute values intobytesvia.serialize()before storing. - Add
_deserialize_from_db()to restore serializedbytesinto typed ZCL values using the attribute definition’s type on DB load. - Add tests covering native passthrough, complex type round-tripping, and persistence for a custom cluster attribute.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
zigpy/appdb.py |
Serializes complex attribute values for SQLite storage and deserializes them on cache load. |
tests/test_appdb.py |
Adds unit/integration tests validating serialization behavior and DB round-trip for complex types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if hasattr(value, "serialize"): | ||
| return value.serialize() | ||
|
|
||
| raise ValueError( |
There was a problem hiding this comment.
I think it would be fine to log exceptions when we can't save something to the DB now? (not just in debug)
It shouldn't happen, except maybe because of the issue where we have quirks return datetime objects for attributes defined as CharacterString? Backlog issue:
There was a problem hiding this comment.
Yeah, these errors would now no longer be confined to the debug log level. It's generally good IMO, but as long as we still have this fundamentally broken behavior for these quirks, we may want to consider just raising a custom SerializationError, which we can then only log at debug level again – just catching all ValueErrors might be a bit too much(?)
diff --git a/zigpy/appdb.py b/zigpy/appdb.py
index e86a5dc9..efcb15d9 100644
--- a/zigpy/appdb.py
+++ b/zigpy/appdb.py
@@ -113,6 +113,10 @@ def aiosqlite_connect(
_SQLITE_TYPES = (type(None), int, float, str, bytes)
+class SerializationError(Exception):
+ """Raised when an attribute value cannot be serialized for database storage."""
+
+
def _serialize_for_db(value: Any) -> None | int | float | str | bytes:
"""Convert a ZCL attribute value to a type SQLite can store natively."""
if isinstance(value, _SQLITE_TYPES):
@@ -121,7 +125,7 @@ def _serialize_for_db(value: Any) -> None | int | float | str | bytes:
if hasattr(value, "serialize"):
return value.serialize()
- raise ValueError(
+ raise SerializationError(
f"Cannot persist attribute value of type {type(value).__name__!r} to the "
f"database: {value!r}"
)
@@ -229,7 +233,7 @@ class PersistingListener(zigpy.util.CatchingTaskMixin):
assert handler
try:
await handler(*args)
- except sqlite3.Error as exc:
+ except (sqlite3.Error, SerializationError) as exc:
LOGGER.debug(
"Error handling '%s' event with %s params: %s",
cb_name,That kind of keeps the old behavior. We should later revert that if we no longer have any known non-matching types in attribute cache (compared to attribute definition type).
| attrid, | ||
| manufacturer_code, | ||
| Status.SUCCESS, | ||
| cache_item.value, | ||
| _serialize_for_db(cache_item.value), | ||
| cache_item.last_updated.timestamp(), |
There was a problem hiding this comment.
Yeah, this is what I mentioned in the PR description. We could re-add that but it's not really used at all at the moment, as this never runs for quirked devices (40ac143).
|
The only test that's failing is this: zigpy/tests/test_appdb_migration.py Lines 705 to 726 in 7d1ea41
We could prevent deserialization if |
|
Awesome! What do you think about completely skipping the implicit storage done by sqlite and always serializing types, including integers and strings? |
|
That would be the fourth open PR with a migration from v14 to v15 😄 But hmm... I'm not sure. It would make the serialization/deserialization added here less special. That's nice but it's also not complicated code to exclude some types. When peeking into a DB for debugging purposes, you'd also get slightly less readable values. And there's some DB tests that would need to be updated to use the serialized values (likely both queries and their output). No big issues though. I think it would be fine..? It does feels a bit like using a DB and then just storing JSON in it (ignoring our backups 😅). But yeah, I don't have a real opinion here. Both ideas seem fine. |
2a81410 to
6949152
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1792 +/- ##
=======================================
Coverage 99.51% 99.51%
=======================================
Files 63 63
Lines 12792 12808 +16
=======================================
+ Hits 12730 12746 +16
Misses 62 62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think that's a good approach. The only cases where it would matter what objects make it into SQLite are:
|
|
Those are good points. From some quick tests, they should be non-issues.
Also, we were apparently storing Lines 113 to 117 in 5cb5429 Lines 860 to 870 in 5cb5429 Technically, we could also use Though currently, With this PR, we will debug log "Failed to deserialize cached value for type CharacterString, using raw bytes" for those legacy values (and just leave them untouched). I think this is fine? Draft of a possible patch where `_deserialize_from_db` deals with this for all strings, but with no DB migration (CLICK TO EXPAND)
diff --git a/zigpy/appdb.py b/zigpy/appdb.py
index e86a5dc9..f2dad413 100644
--- a/zigpy/appdb.py
+++ b/zigpy/appdb.py
@@ -142,6 +142,10 @@ def _deserialize_from_db(value: Any, attr_type: type | None) -> Any:
try:
result, _ = attr_type.deserialize(value) # type: ignore[attr-defined]
except (ValueError, KeyError):
+ # Legacy bytes values for string types: decode as UTF-8 with null stripping
+ if issubclass(attr_type, str):
+ return value.split(b"\x00", 1)[0].decode("utf-8", errors="replace")
+
LOGGER.debug(
"Failed to deserialize cached value for type %s, using raw bytes",
attr_type,
@@ -151,13 +155,6 @@ def _deserialize_from_db(value: Any, attr_type: type | None) -> Any:
return result
-def decode_str_attribute(value: str | bytes) -> str:
- if isinstance(value, str):
- return value
-
- return value.split(b"\x00", 1)[0].decode("utf-8")
-
-
class PersistingListener(zigpy.util.CatchingTaskMixin):
def __init__(
self,
@@ -888,27 +885,29 @@ class PersistingListener(zigpy.util.CatchingTaskMixin):
continue
- if row.status == Status.SUCCESS:
- cluster._attr_cache.set_value(
- attr_def,
- _deserialize_from_db(row.value, attr_def.type),
- last_updated=datetime.fromtimestamp(row.last_updated, UTC),
- )
- else:
+ if row.status != Status.SUCCESS:
cluster._attr_cache.mark_unsupported(attr_def)
continue
+ value = _deserialize_from_db(row.value, attr_def.type)
+
+ cluster._attr_cache.set_value(
+ attr_def,
+ value,
+ last_updated=datetime.fromtimestamp(row.last_updated, UTC),
+ )
+
# Populate the device's manufacturer and model attributes
if (
row.cluster_id == Basic.cluster_id
and attr_def == Basic.AttributeDefs.manufacturer
):
- dev.manufacturer = decode_str_attribute(row.value)
+ dev.manufacturer = value
elif (
row.cluster_id == Basic.cluster_id
and attr_def == Basic.AttributeDefs.model
):
- dev.model = decode_str_attribute(row.value)
+ dev.model = value
@staticmethod
def _resolve_unmigrated_attribute( |
Background
This is somewhat needed for Ubisys devices that we re-interview (LD6). We (obviously) clear the whole attribute cache, including local attributes, so we need to actually read the device-specific attribute and parse that after the (re-)interview.
This already works fine at runtime. Between HA restarts, we rely on the local attribute, so we don't technically need this for that, but we log an error about failing to save this when reading the attribute. We could also just ignore those errors and not log them...?
EDIT: Actually, we already seem to only log these errors at debug level, so we don't need to push through with this PR now if we don't like it.
For this solution, see the summary below. The code changes are relatively small.
AI summary
LVList[LVBytes]) could not be persisted to the SQLite database, raisingsqlite3.ProgrammingError: type 'AnonymousLVList' is not supported. This affected custom clusters like the ubisys0xFC00cluster that define array-typed attributes.None,int,float,str,bytes) are now serialized tobytesvia their.serialize()method before storage.bytesvalues are deserialized back into their original typed form using the attribute definition's type, socluster.get(attr)returns the correct type after restart.Note: pre-existing type fidelity gap
There is a pre-existing issue (not introduced by this PR) where the type of simple attribute values is not preserved across restarts. For example, a
uint16_tattribute read live returns at.uint16_tinstance, but after DB reload it comes back as a plain Pythonint, because SQLite stores both identically as INTEGER. This PR does not attempt to fix that — it only addresses the crash for types SQLite cannot store at all.Note
We could also guard the the
_save_attribute_cachecall that happens for all attributes when zigpy is shutting down, so that it doesn't fail completely, but only for attributes that can't be serialized (of which there should be none..?).However, because of the code below, we never call
_save_attribute_cachefor quirked devices anyway, purely relying on the runtime/event attributes updates for DB persistence. Since non-quirked devices using pure ZCL attributes mostly/only use "simple" types that can be saved directly into the DB, we don't really need the extra guard?Either way, we could revert this commit to add that guard: 40ac143
Code preventing quirked devices from calling
_save_attribute_cache:zigpy/zigpy/appdb.py
Lines 408 to 420 in 7d1ea41