|
| 1 | +# Stream Viz Backend Code Review & Improvement Suggestions |
| 2 | + |
| 3 | +This document outlines findings and suggestions for improving the `stream-viz` backend codebase based on a review focusing on SOLID principles, clarity, and best practices. |
| 4 | + |
| 5 | +## Overall Structure & Design |
| 6 | + |
| 7 | +* **Good Separation:** The project follows a reasonable structure separating concerns into `api`, `core`, `utils`, and `main.py`. This promotes modularity. |
| 8 | +* **Dependency Injection:** Good use of FastAPI's dependency injection (`Depends`) for `TimeplusQueryManager` and `PerspectiveManager` in the WebSocket endpoint (`api/websocket/endpoint.py`). This improves testability and decoupling. |
| 9 | +* **Manager Classes:** The use of `TimeplusQueryManager` and `PerspectiveManager` centralizes logic for interacting with these external services, which is good. They act as facades and manage resource lifecycles (like queries and tables) using reference counting. |
| 10 | +* **Configuration:** Using Pydantic's `BaseSettings` (`config.py`) for configuration management is a standard and effective practice. |
| 11 | +* **Asyncio Usage:** The codebase heavily relies on `asyncio`, which is appropriate for I/O-bound tasks like handling WebSockets and interacting with external streaming services. |
| 12 | + |
| 13 | +## SOLID Principles & Clean Code Analysis |
| 14 | + |
| 15 | +1. **Single Responsibility Principle (SRP):** |
| 16 | + * **Mostly Good:** Classes like `WebSocketConnectionManager`, `TimeplusQueryManager`, and `PerspectiveManager` generally adhere well to SRP. |
| 17 | + * **`WebSocketMessageHandler`:** (`api/websocket/message_handler.py`) The `handle_start_query` method is quite long and complex. Consider breaking it down into smaller helper methods. |
| 18 | + * **`TimeplusQueryManager._run_query`:** This method is very long (~100 lines) and handles many tasks within the stream processing loop. Refactor into smaller, focused async methods (e.g., `_process_schema`, `_process_data_chunk`). |
| 19 | + * **`websocket_endpoint`:** (`api/websocket/endpoint.py`) The main `try...except` block is large. Some error/cleanup logic could potentially move to `WebSocketConnectionManager` or `WebSocketMessageHandler`. |
| 20 | + |
| 21 | +2. **Open/Closed Principle (OCP):** |
| 22 | + * **Message Handling:** Dispatching messages via `isinstance` is acceptable for now but consider a more scalable pattern (e.g., command pattern or dictionary mapping) if more message types are added. |
| 23 | + |
| 24 | +3. **Liskov Substitution Principle (LSP):** |
| 25 | + * **Opportunity:** If supporting alternative streaming sources or visualization backends becomes necessary, define abstract base classes/interfaces for managers (`QueryManager`, `VizManager`) to enable substitution. |
| 26 | + |
| 27 | +4. **Interface Segregation Principle (ISP):** |
| 28 | + * **Mostly Good:** Manager interfaces are generally focused. |
| 29 | + * **`TimeplusQueryManager` Callbacks:** The specific set of callbacks required by `start_or_join_query` tightly couples it to `WebSocketMessageHandler`. Consider alternatives if other consumers need different interaction patterns. |
| 30 | + |
| 31 | +5. **Dependency Inversion Principle (DIP):** |
| 32 | + * **Good (FastAPI):** FastAPI's `Depends` inverts control effectively. |
| 33 | + * **Managers & Clients:** Managers (`TimeplusQueryManager`, `PerspectiveManager`) create their underlying clients (`proton_driver`, `perspective`) directly. Injecting these clients (or interfaces) would improve testability and flexibility. |
| 34 | + |
| 35 | +## Clarity, Messiness & Specific Issues |
| 36 | + |
| 37 | +* **Error Handling:** |
| 38 | + * Extensive but complex, especially in WebSocket endpoint and message handler cleanup logic. Centralize where possible. |
| 39 | + * Unclear error propagation strategy from Timeplus callbacks in `WebSocketMessageHandler`. Decide whether errors should stop the upstream query. |
| 40 | +* **Async/Sync Mix:** |
| 41 | + * Verify potentially blocking calls (e.g., `PerspectiveManager.update_table`) and use `asyncio.to_thread` if needed. |
| 42 | + * Simplify unnecessary async wrappers around non-blocking calls in `WebSocketMessageHandler`. |
| 43 | +* **Missing Tests:** The `src/stream_viz/tests` directory is empty. This is a critical gap. |
| 44 | +* **Empty Files:** `src/stream_viz/utils/hashing.py` is empty. Implement or remove. |
| 45 | +* **Magic Strings:** Replace string literals for WebSocket message types (e.g., "table_ready") with Enums or constants for robustness. |
| 46 | +* **Resource Cleanup:** Double-check cleanup logic (`cleanup_query_resources`, `shutdown`) for edge cases and completeness. |
| 47 | +* **Perspective Client Loop:** Ensure the event loop handling in `PerspectiveManager` is robust for deployment scenarios. |
| 48 | + |
| 49 | +## High-Priority Improvement Actions |
| 50 | + |
| 51 | +1. **Implement Comprehensive Tests:** Add unit tests (mocking external deps) and integration tests. Start with managers and critical WebSocket logic. |
| 52 | +2. **Refactor Large Methods:** Break down `handle_start_query` (in `WebSocketMessageHandler`), `_run_query` (in `TimeplusQueryManager`), and `websocket_endpoint`. |
| 53 | +3. **Clarify & Standardize Error Handling:** Define clear error propagation rules (especially for callbacks) and simplify cleanup logic. |
| 54 | +4. **Address Async/Sync:** Use `asyncio.to_thread` for confirmed blocking calls. Remove unnecessary wrappers. |
| 55 | +5. **Inject Dependencies:** Inject `proton_driver` and `perspective` clients into their respective managers. |
| 56 | +6. **Populate/Remove Empty Files:** Add tests to `tests/`. Implement or remove `utils/hashing.py`. |
0 commit comments