v15 database schema migration to deprioritize unsupported attributes#1766
v15 database schema migration to deprioritize unsupported attributes#1766
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1766 +/- ##
=======================================
Coverage 99.52% 99.52%
=======================================
Files 63 63
Lines 12715 12731 +16
=======================================
+ Hits 12655 12671 +16
Misses 60 60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces database schema v15 and a corresponding migration to correct a v14 migration regression where “unsupported attribute” entries incorrectly took precedence over cached attribute values, breaking devices that had the same attribute present in both tables pre-v14.
Changes:
- Bump DB schema version to v15 and add schema_v15.sql.
- Update migration table-copy logic to use explicit column lists (avoids issues with generated columns).
- Add a v15 data fix-up to restore cached values from v13 when v14 incorrectly marked them unsupported, plus a quirk helper to ensure constant attributes are never treated as unsupported (with tests).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| zigpy/quirks/init.py | Prevent constant attributes from being reported as unsupported. |
| zigpy/appdb_schemas/schema_v15.sql | Add schema v15 (table definitions for v15). |
| zigpy/appdb.py | Bump DB version, add v15 migration, and improve table migration to avoid generated-column inserts. |
| tests/test_quirks.py | Add test coverage for constant attributes never being unsupported. |
| tests/test_appdb_migration.py | Update migration tests for new table-copy strategy and add v15 regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ce over unsupported
0371c18 to
0d3e0dd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/test_appdb_migration.py:636
- This assignment to 'test_data_migration_ambiguous_attributes' is unnecessary as it is redefined before this value is used.
async def test_data_migration_ambiguous_attributes(tmp_path):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # The v14 migration incorrectly gave unsupported attributes priority over cached | ||
| # values when merging the two tables. Restore cached values from v13 for any | ||
| # attribute that was marked unsupported in v14 but had a value in v13. | ||
| if not await self._table_exists("attributes_cache_v13"): | ||
| return | ||
|
|
||
| # Delete unsupported v15 rows that have cached values in v13 | ||
| await self.execute( | ||
| """ | ||
| DELETE FROM attributes_cache_v15 | ||
| WHERE status = :unsupported | ||
| AND EXISTS ( | ||
| SELECT 1 | ||
| FROM attributes_cache_v13 c13 | ||
| WHERE c13.ieee = attributes_cache_v15.ieee | ||
| AND c13.endpoint_id = attributes_cache_v15.endpoint_id | ||
| AND c13.cluster_type = attributes_cache_v15.cluster_type | ||
| AND c13.cluster_id = attributes_cache_v15.cluster_id | ||
| AND c13.attr_id = attributes_cache_v15.attr_id | ||
| ) |
There was a problem hiding this comment.
The v15 restore step deletes any row with status = UNSUPPORTED_ATTRIBUTE as long as a matching (ieee, endpoint_id, cluster_type, cluster_id, attr_id) exists in attributes_cache_v13. Since unsupported rows can also be written at runtime in v14 (with a real last_updated), this migration can incorrectly flip legitimately-unsupported attributes back to SUCCESS based on stale v13 cache data. Consider narrowing the DELETE/restore scope to only the rows created by the v14 migration bug (e.g., manufacturer_code = UNMIGRATED_MANUFACTURER_CODE and last_updated equal to the epoch timestamp used in _migrate_to_v14, and/or requiring presence in unsupported_attributes_v13).
There was a problem hiding this comment.
The suggestions mostly miss the point of this PR. However, I think the last_updated = 0 check would actually work to only filter out potentially inaccurate unsupported attributes from the migration, but not ones correctly added "live on v14". Whether this is worth doing, I'm not sure about.
The v14 schema migration prioritized unsupported attribute table entries over those in the attribute cache. This breaks devices that had entries in both tables. We can perform a new migration to
v15and peek at thev13tables (if they exist) to reprioritize them after the fact.