bhetherman/spotify-voice-assistant
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Remove internal development files

Browse Source
This commit is contained in:
Chad Auld
2025-11-09 19:45:29 -07:00
parent bd221b80e8
commit d43f847e07
3 changed files with 0 additions and 778 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CACHING_IMPLEMENTATION_PLAN.md
-398
View File
@@ -1,398 +0,0 @@
# Spotify Client Caching Implementation Plan
## Problem Statement
Currently, the integration searches for the Spotify entity on **every service call**:
- Iterates through all media player entities (O(n) operation)
- Looks up entity object from component
- Accesses coordinator and client
- Repeats this 100% of the time, even though the result rarely changes
## Solution: Intelligent Caching
Cache the Spotify client reference after first successful lookup, with smart invalidation.
## Implementation Design
### 1. Cache Structure
```python
# Module-level cache (survives across service calls)
_spotify_cache = {
"client": None, # SpotifyClient instance
"entity_id": None, # Entity ID for monitoring
"last_validated": None, # Timestamp of last validation
}
```
### 2. Cache Lifecycle
```
First Call:
→ Cache Miss
→ Lookup entity (slow)
→ Store client + entity_id
→ Return client
Subsequent Calls:
→ Cache Hit
→ Validate entity still exists (fast)
→ Return cached client
Entity Removed:
→ Validation fails
→ Clear cache
→ Re-lookup on next call
```
### 3. Validation Strategy
**Fast validation** - Check if entity still exists:
```python
# Very fast - just checks if entity_id exists in state registry
if spotify_entity_id not in hass.states.async_entity_ids():
# Entity was removed, invalidate cache
_spotify_cache.clear()
```
**When to validate:**
- Option A: Every call (minimal overhead, ~0.1ms)
- Option B: Every N seconds (e.g., 60s)
- **Recommended: Option A** - Simple and fast enough
### 4. Cache Invalidation Triggers
**Automatic:**
1. Entity no longer exists (validation check)
2. Integration reload/restart (cache is module-level, cleared on reload)
**Manual (Future):**
1. Service call: `spotify_search.clear_cache`
2. HA restart (automatic)
3. Spotify integration reload (automatic via entity disappearance)
## Implementation Code
### Version 1: Simple (Recommended for v1.1.0)
```python
"""Spotify Search Integration for Home Assistant."""
import logging
from homeassistant.core import HomeAssistant, ServiceCall
from homeassistant.helpers.typing import ConfigType
_LOGGER = logging.getLogger(__name__)
DOMAIN = "spotify_search"
# Cache Spotify client to avoid repeated lookups
_spotify_cache = {
"client": None,
"entity_id": None,
}
async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool:
"""Set up the Spotify Search component."""
async def get_spotify_client():
"""Get Spotify client with caching and validation."""
# Validate cache if it exists
if _spotify_cache["client"] is not None:
# Fast check: Does entity still exist?
if _spotify_cache["entity_id"] in hass.states.async_entity_ids():
_LOGGER.debug("Using cached Spotify client")
return _spotify_cache["client"]
else:
_LOGGER.info("Cached Spotify entity no longer exists, invalidating cache")
_spotify_cache.clear()
# Cache miss or invalidated - do full lookup
_LOGGER.debug("Cache miss, performing Spotify entity lookup")
# Find Spotify media player entity
spotify_entity_id = None
for state in hass.states.async_all("media_player"):
if "spotify" in state.entity_id.lower():
spotify_entity_id = state.entity_id
break
if not spotify_entity_id:
raise LookupError("No Spotify media player entity found")
# Get entity component
entity_component = hass.data.get("entity_components", {}).get("media_player")
if not entity_component:
raise LookupError("Media player component not available")
# Find Spotify entity object
spotify_entity = None
for entity in entity_component.entities:
if entity.entity_id == spotify_entity_id:
spotify_entity = entity
break
if not spotify_entity:
raise LookupError(f"Spotify entity {spotify_entity_id} not found")
# Get client from coordinator
if not hasattr(spotify_entity, "coordinator"):
raise AttributeError("Spotify entity missing coordinator")
coordinator = spotify_entity.coordinator
if not hasattr(coordinator, "client"):
raise AttributeError("Coordinator missing client")
client = coordinator.client
# Cache for future calls
_spotify_cache["client"] = client
_spotify_cache["entity_id"] = spotify_entity_id
_LOGGER.info("Cached Spotify client for entity: %s", spotify_entity_id)
return client
async def search_spotify(call: ServiceCall):
"""Search Spotify and return the first result's URI."""
query = call.data.get("query")
search_type = call.data.get("type", "artist")
if not query:
_LOGGER.error("No query provided")
return {"error": "No query provided"}
try:
client = await get_spotify_client()
except (LookupError, AttributeError) as err:
_LOGGER.error("Failed to get Spotify client: %s", err)
return {"error": str(err)}
# ... rest of search logic unchanged ...
```
### Version 2: Time-Based (Alternative)
Add timestamp-based validation to reduce checks:
```python
import time
_spotify_cache = {
"client": None,
"entity_id": None,
"validated_at": 0,
}
VALIDATION_INTERVAL = 60 # Seconds between validations
async def get_spotify_client():
"""Get Spotify client with time-based cache validation."""
now = time.time()
if _spotify_cache["client"] is not None:
# Only validate every N seconds
if now - _spotify_cache["validated_at"] < VALIDATION_INTERVAL:
return _spotify_cache["client"]
# Time to validate
if _spotify_cache["entity_id"] in hass.states.async_entity_ids():
_spotify_cache["validated_at"] = now
return _spotify_cache["client"]
else:
# Entity gone, clear cache
_spotify_cache.clear()
# ... lookup logic ...
_spotify_cache["validated_at"] = now
```
## Cache Clearing Service (Optional)
Add manual cache clearing for troubleshooting:
```python
async def clear_cache(call: ServiceCall):
"""Clear Spotify client cache."""
if _spotify_cache["client"] is not None:
_LOGGER.info("Manually clearing Spotify client cache")
_spotify_cache.clear()
return {"success": True, "message": "Cache cleared"}
else:
return {"success": False, "message": "Cache was already empty"}
# Register both services
hass.services.async_register(DOMAIN, "search", search_spotify, supports_response="only")
hass.services.async_register(DOMAIN, "clear_cache", clear_cache, supports_response="only")
```
Add to `services.yaml`:
```yaml
clear_cache:
name: Clear Cache
description: Clear the cached Spotify client. Use this if you're experiencing issues after removing/re-adding the Spotify integration.
```
## Performance Impact
### Before Caching
```
Each service call:
- Iterate all media_player entities: ~1-5ms (depends on count)
- Lookup entity object: ~0.5ms
- Access coordinator: ~0.1ms
Total: ~1.6-5.6ms per call
```
### After Caching (V1 - Simple)
```
First call:
- Same as before: ~1.6-5.6ms
Subsequent calls:
- Check entity exists: ~0.1ms
- Return cached client: ~0.01ms
Total: ~0.11ms per call
Speedup: 15-50x faster
```
### After Caching (V2 - Time-Based)
```
First call: ~1.6-5.6ms
Calls within 60s: ~0.01ms (no validation)
Validation call: ~0.11ms
Average: ~0.01-0.02ms per call
Speedup: 80-280x faster
```
## Recommendation
### For v1.1.0: **Version 1 (Simple Validation)**
**Pros:**
- Simple implementation
- Always validates (safer)
- Validation is fast enough (~0.1ms)
- No time tracking needed
- Easy to understand
**Cons:**
- Validates on every call (tiny overhead)
### For v1.2.0+: **Version 2 (Time-Based)** - If needed
Only if profiling shows validation is a bottleneck (unlikely).
## Testing Plan
### Unit Tests (Future)
```python
async def test_cache_hit():
"""Test that second call uses cache."""
# First call
client1 = await get_spotify_client()
# Second call should use cache
client2 = await get_spotify_client()
assert client1 is client2 # Same object reference
async def test_cache_invalidation():
"""Test cache clears when entity removed."""
# Get client (cached)
client1 = await get_spotify_client()
# Remove Spotify entity
hass.states.async_remove("media_player.spotify_chad")
# Next call should re-lookup and fail
with pytest.raises(LookupError):
await get_spotify_client()
```
### Manual Testing
1. Call service twice, check logs for "Using cached Spotify client"
2. Remove Spotify integration, verify cache invalidates
3. Re-add Spotify integration, verify it re-caches
4. Restart HA, verify cache clears (new process)
## Migration Path
### v1.0.0 → v1.1.0
- Add caching (no breaking changes)
- Add cache clearing service (optional)
- Update CHANGELOG
### Code Changes Required
- Modify `async_setup()` to add `get_spotify_client()` helper
- Replace entity lookup in `search_spotify()` with `await get_spotify_client()`
- Add module-level `_spotify_cache` dict
- Optional: Add `clear_cache` service
### Lines Changed: ~30 lines
- Remove: ~15 lines (inline lookup)
- Add: ~30 lines (cached lookup function)
- Modify: ~5 lines (call new function)
**Net: ~20 line increase for 15-50x performance improvement**
## Security Considerations
**Is caching safe?**
- ✅ Client object doesn't contain credentials (OAuth tokens are in HA core)
- ✅ Module-level cache is process-scoped (isolated per HA instance)
- ✅ Cache invalidates when entity removed
- ✅ No user data in cache
- ✅ No cross-user data leakage (single-user system)
**Risks:**
- ⚠️ If Spotify integration is removed and re-added quickly, cache might use old client
- Mitigation: Validation check catches this (entity_id won't match)
## Rollout Strategy
1. **v1.1.0-beta**: Release with caching, ask users to test
2. Monitor for issues (GitHub issues)
3. If stable after 2 weeks → **v1.1.0 stable**
4. Add to CHANGELOG with performance notes
## Documentation Updates
### README.md
```markdown
## Performance
The integration caches the Spotify client reference after first use for optimal performance:
- First search: ~5ms
- Subsequent searches: ~0.1ms (50x faster)
The cache automatically invalidates if the Spotify integration is removed or reloaded.
```
### DEVELOPMENT.md
```markdown
## Caching
The integration caches the Spotify client to avoid repeated entity lookups.
To manually clear the cache:
```yaml
service: spotify_search.clear_cache
```
This is rarely needed but useful for troubleshooting.
```
## Summary
**Recommended Implementation: Version 1 (Simple Validation)**
- Cache client after first lookup
- Validate entity exists on every call (~0.1ms overhead)
- Automatic invalidation when entity removed
- Optional manual cache clearing service
- 15-50x performance improvement
- Simple, safe, effective
**Estimated effort:** 1-2 hours development + testing
**Risk level:** Low
**Performance gain:** High
CODE_REVIEW.md
-214
View File
@@ -1,214 +0,0 @@
# Code Review Summary
## Overall Assessment: **GOOD** ✅
The code is clean, functional, and secure. However, there are some improvements that would make it production-ready for broader distribution.
## Strengths ✅
### Security
- ✅ No credentials stored or handled
- ✅ Leverages existing HA Spotify OAuth securely
- ✅ No SQL injection vectors
- ✅ No shell command execution
- ✅ Safe error messages (no internal state exposure)
- ✅ No user input passed to system calls
### Code Quality
- ✅ Clean, readable Python code
- ✅ Proper async/await usage
- ✅ Good separation of concerns
- ✅ Follows Home Assistant patterns
- ✅ Minimal and focused (~116 lines)
- ✅ Type hints present
### Functionality
- ✅ Core feature works correctly
- ✅ Exact match artist search implemented
- ✅ Error handling present
- ✅ Logging implemented
- ✅ Service response support
## Issues Found ⚠️
### 1. Performance - Entity Lookup (Medium Priority)
**Current Code (Lines 26-48):**
```python
# Runs on EVERY service call
for state in hass.states.async_all("media_player"):
if "spotify" in state.entity_id.lower():
spotify_entity_id = state.entity_id
break
```
**Issue:** O(n) search through all media players on every call
**Impact:**
- Wastes CPU on repeated calls
- Could slow down with many media players
- Not significant for typical use, but inefficient
**Recommended Fix:**
- Cache the Spotify client reference after first lookup
- Invalidate cache only if entity disappears
**Severity:** Medium (works fine, but not optimal)
---
### 2. Error Handling - Bare Exception Catch (Low Priority)
**Current Code (Line 109):**
```python
except Exception as e:
_LOGGER.error(f"Error searching Spotify: {e}")
return {"error": str(e)}
```
**Issue:** Catches ALL exceptions indiscriminately
**Impact:**
- Could hide programming errors
- Makes debugging harder
- May catch errors that should crash
**Recommended Fix:**
```python
except AttributeError as err:
_LOGGER.error("Spotify API returned unexpected data: %s", err)
return {"error": "Unexpected response from Spotify"}
except Exception as err:
_LOGGER.exception("Unexpected error searching Spotify")
return {"error": "Search failed"}
```
**Severity:** Low (defensive programming is sometimes acceptable)
---
### 3. Input Validation - Missing Type Check (Low Priority)
**Current Code (Line 18):**
```python
search_type = call.data.get("type", "artist")
```
**Issue:** No validation that `search_type` is valid
**Impact:**
- Could pass invalid type to Spotify API
- Spotify API would return error anyway
- User gets less helpful error message
**Recommended Fix:**
```python
VALID_SEARCH_TYPES = {"artist", "album", "track", "playlist"}
if search_type not in VALID_SEARCH_TYPES:
return {"error": f"Invalid type. Must be: {', '.join(VALID_SEARCH_TYPES)}"}
```
**Severity:** Low (services.yaml already limits options in UI)
---
### 4. Robustness - Missing Attribute Checks (Low Priority)
**Current Code (Lines 81, 88, 102):**
```python
if artist.name.lower() == query_lower: # Could be AttributeError
exact_match = artist
```
**Issue:** Assumes API always returns expected structure
**Impact:**
- Could crash if Spotify API changes
- Rare edge case
**Recommended Fix:**
```python
if hasattr(artist, "name") and artist.name.lower() == query_lower:
exact_match = artist
```
**Severity:** Low (Spotify API is stable)
---
### 5. Code Style - F-strings in Logging (Very Low Priority)
**Current Code:**
```python
_LOGGER.debug(f"Found Spotify entity: {spotify_entity_id}")
```
**Issue:** F-strings evaluate even if log level filters the message
**Impact:**
- Tiny CPU waste when debug logging disabled
- Negligible in practice
**Best Practice:**
```python
_LOGGER.debug("Found Spotify entity: %s", spotify_entity_id)
```
**Severity:** Very Low (micro-optimization)
---
## Recommendations
### For v1.0.0 Release
**Ship as-is.** The current code is:
- Secure
- Functional
- Well-tested
- Clean and maintainable
The issues are minor optimizations and edge cases.
### For v1.1.0 (Future Enhancement)
Consider implementing:
1. **Client caching** - Most impactful performance improvement
2. **Input validation** - Better user error messages
3. **Specific exception handling** - Better debugging
### Code Review Checklist Results
| Category | Status | Notes |
|----------|--------|-------|
| **Security** | ✅ PASS | No vulnerabilities found |
| **Performance** | ⚠️ GOOD | Minor optimization opportunity (caching) |
| **Error Handling** | ⚠️ GOOD | Works but could be more specific |
| **Input Validation** | ⚠️ GOOD | UI limits input, code could validate |
| **Code Style** | ✅ PASS | Clean, readable, follows HA patterns |
| **Type Safety** | ✅ PASS | Type hints present |
| **Documentation** | ✅ PASS | Docstrings present, comments clear |
| **Testing** | ⚠️ N/A | Manual testing done, unit tests would help |
| **Robustness** | ⚠️ GOOD | Handles expected cases, edge cases possible |
## Improved Version
See `__init__.py.review` for a version with all recommendations applied:
- ✅ Client caching for performance
- ✅ Input validation for search_type
- ✅ Specific exception handling
- ✅ Defensive attribute access
- ✅ Lazy % formatting in logs
- ✅ Type hints for return values
- ✅ Using `next()` for more Pythonic exact match search
**Note:** The improved version is untested. For v1.0.0, recommend shipping the current working version.
## Final Verdict
**✅ APPROVED FOR RELEASE**
The code is production-ready for v1.0.0. The identified issues are minor optimizations and defensive programming improvements that can be addressed in future versions based on real-world usage feedback.
**Risk Level:** LOW
**Code Quality:** HIGH
**Readiness:** READY FOR PRODUCTION
custom_components/spotify_search/__init__.py.review
-166
View File
@@ -1,166 +0,0 @@
"""Spotify Search Integration for Home Assistant."""
import logging
from typing import Any
from homeassistant.core import HomeAssistant, ServiceCall
from homeassistant.config_entries import ConfigEntry
from homeassistant.helpers.typing import ConfigType
from homeassistant.exceptions import HomeAssistantError
_LOGGER = logging.getLogger(__name__)
DOMAIN = "spotify_search"
VALID_SEARCH_TYPES = {"artist", "album", "track", "playlist"}
async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool:
"""Set up the Spotify Search component."""
# Cache Spotify entity lookup for performance
spotify_entity_cache = {"entity": None, "client": None}
async def get_spotify_client():
"""Get Spotify client with caching."""
# Return cached client if available
if spotify_entity_cache["client"] is not None:
return spotify_entity_cache["client"]
# Find Spotify media player entity
spotify_entity_id = None
for state in hass.states.async_all("media_player"):
if "spotify" in state.entity_id.lower():
spotify_entity_id = state.entity_id
break
if not spotify_entity_id:
raise HomeAssistantError("No Spotify media player entity found")
_LOGGER.debug("Found Spotify entity: %s", spotify_entity_id)
# Get the Spotify integration's data through entity platform
entity_component = hass.data.get("entity_components", {}).get("media_player")
if not entity_component:
raise HomeAssistantError("Media player component not available")
# Find the Spotify entity object
spotify_entity = None
for entity in entity_component.entities:
if entity.entity_id == spotify_entity_id:
spotify_entity = entity
break
if not spotify_entity:
raise HomeAssistantError(f"Spotify entity {spotify_entity_id} not found")
# Access the Spotify client from the coordinator
if not hasattr(spotify_entity, "coordinator"):
raise HomeAssistantError("Spotify entity does not have a coordinator")
coordinator = spotify_entity.coordinator
if not hasattr(coordinator, "client"):
raise HomeAssistantError("Spotify coordinator does not have a client")
client = coordinator.client
_LOGGER.debug("Spotify client type: %s", type(client).__name__)
# Cache for future calls
spotify_entity_cache["entity"] = spotify_entity
spotify_entity_cache["client"] = client
return client
async def search_spotify(call: ServiceCall) -> dict[str, Any]:
"""Search Spotify and return the first result's URI."""
query = call.data.get("query")
search_type = call.data.get("type", "artist")
# Input validation
if not query:
_LOGGER.error("No query provided to spotify_search")
return {"error": "No query provided"}
if not isinstance(query, str):
_LOGGER.error("Query must be a string, got: %s", type(query).__name__)
return {"error": "Query must be a string"}
if search_type not in VALID_SEARCH_TYPES:
_LOGGER.error("Invalid search type: %s", search_type)
return {"error": f"Invalid type. Must be one of: {', '.join(VALID_SEARCH_TYPES)}"}
try:
client = await get_spotify_client()
except HomeAssistantError as err:
_LOGGER.error("Failed to get Spotify client: %s", err)
return {"error": str(err)}
try:
# Search Spotify using the integration's client
if search_type == "artist":
# For artists, search and use exact name matching
_LOGGER.debug("Searching for artist: %s", query)
results = await client.search(query, ["artist"], limit=10)
items_list = results.artists
if items_list:
# Check for exact match (case-insensitive)
query_lower = query.lower()
exact_match = next(
(artist for artist in items_list
if hasattr(artist, "name") and artist.name.lower() == query_lower),
None
)
# Use exact match if found, otherwise first result
selected_artist = exact_match or items_list[0]
# Defensive attribute access
if not hasattr(selected_artist, "uri") or not hasattr(selected_artist, "name"):
_LOGGER.error("Artist result missing required attributes")
return {"error": "Invalid artist data from Spotify"}
match_type = "exact match" if exact_match else "first result"
_LOGGER.info("Found Spotify artist: %s (%s) - %s",
selected_artist.name, selected_artist.uri, match_type)
return {
"uri": selected_artist.uri,
"name": selected_artist.name,
"type": "artist"
}
else:
_LOGGER.warning("No artists found for query: %s", query)
return {"error": f"No artist found for: {query}"}
else:
# For albums, tracks, playlists - use direct search
results = await client.search(query, [search_type], limit=1)
items_list = getattr(results, f"{search_type}s", None)
if items_list:
item = items_list[0]
# Defensive attribute access
if not hasattr(item, "uri") or not hasattr(item, "name"):
_LOGGER.error("%s result missing required attributes", search_type)
return {"error": f"Invalid {search_type} data from Spotify"}
_LOGGER.info("Found Spotify %s: %s (%s)", search_type, item.name, item.uri)
return {
"uri": item.uri,
"name": item.name,
"type": search_type
}
else:
_LOGGER.warning("No results found for query: %s", query)
return {"error": f"No {search_type} found for: {query}"}
except AttributeError as err:
_LOGGER.error("Spotify API returned unexpected data structure: %s", err)
return {"error": "Unexpected response from Spotify"}
except Exception as err:
_LOGGER.exception("Unexpected error searching Spotify: %s", err)
return {"error": "Search failed"}
hass.services.async_register(
DOMAIN, "search", search_spotify, supports_response="only"
)
return True