changed saving system to sqlite#7
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request migrates the Dragon Riders game's save system from JSON-based files to a SQLite database, adding encryption, integrity verification via HMAC hashes, and automatic migration support for existing saves. The change enables better scalability for games with hundreds or thousands of dragons by allowing selective loading rather than deserializing the entire save file at once.
Key Changes:
- Introduces SQLiteSaveSystem class with encrypted database storage, compression, and integrity verification
- Implements automatic migration from JSON saves to SQLite with fallback support
- Adds backup utilities and migration verification tools for safe data transitions
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
| src/save_system_sqlite.py | Core SQLite save system implementation with encryption (Fernet), HMAC integrity checks, and database schema for dragons, game state, eggs, and inventory |
| src/utils/save_migration.py | Migration utilities for backing up save files, verifying migration success, and cleaning up old backups |
| src/game_state.py | Integration changes to support dual save systems with automatic migration from JSON to SQLite on first load |
| tests/test_save_system_sqlite.py | Comprehensive unit tests for SQLite save system covering CRUD operations, encryption, integrity verification, and edge cases |
| docs/SAVE_SYSTEM.md | Complete documentation of the new save system architecture, security features, migration process, API reference, and troubleshooting guide |
Comments suppressed due to low confidence (1)
src/game_state.py:572
- When loading from SQLite (the normal case at line 550), the code at lines 567-572 is reached and
from_dictis called again on the same data. This means the data is loaded twice - once when we load from SQLite and reach this point. While not breaking, this is inefficient. The code should return after successfully loading from SQLite, similar to how the migration path returns early at line 565. Consider restructuring to return early after the SQLite load succeeds.
if data:
self.from_dict(data)
# Add starter items if inventory is empty (for existing saves)
if not self.inventory:
self._add_starter_items()
return True
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Create indexes for performance | ||
| cursor.execute(""" | ||
| CREATE INDEX IF NOT EXISTS idx_dragons_type ON dragons(dragon_type) | ||
| """) | ||
| cursor.execute(""" | ||
| CREATE INDEX IF NOT EXISTS idx_dragons_stage ON dragons(stage) | ||
| """) | ||
| cursor.execute(""" | ||
| CREATE INDEX IF NOT EXISTS idx_dragons_created ON dragons(created_at) | ||
| """) |
There was a problem hiding this comment.
The indexes are created with IF NOT EXISTS, which is good. However, creating indexes immediately during initialization means every time the save system is instantiated, these CREATE INDEX statements are executed. While they're no-ops if indexes exist, they still require parsing and checking. Consider either: (1) checking the metadata table for a flag indicating indexes are created, or (2) accepting this minor overhead since instantiation isn't frequent.
| def save_dragon(self, dragon_data: Dict[str, Any], dragon_id: Optional[int] = None) -> int: | ||
| """Save a single dragon to database. | ||
|
|
||
| Args: | ||
| dragon_data: Dragon data dictionary | ||
| dragon_id: Existing dragon ID to update, or None to create new | ||
|
|
||
| Returns: | ||
| Dragon ID (row id) | ||
| """ | ||
| conn = self._get_connection() | ||
| cursor = conn.cursor() | ||
|
|
||
| # Extract non-sensitive data for indexing | ||
| dragon_type = dragon_data.get("dragon_type", "unknown") | ||
| name = dragon_data.get("name", "Dragon") | ||
| stage = dragon_data.get("stage", "egg") | ||
|
|
||
| # Encrypt sensitive data | ||
| encrypted_data = self._encrypt_data(dragon_data) | ||
| data_hash = self._compute_hash(dragon_data) | ||
| current_time = time.time() | ||
|
|
||
| if dragon_id is None: | ||
| # Insert new dragon | ||
| cursor.execute(""" | ||
| INSERT INTO dragons (dragon_type, name, stage, encrypted_data, data_hash, created_at, updated_at) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?) | ||
| """, (dragon_type, name, stage, encrypted_data, data_hash, current_time, current_time)) | ||
| dragon_id = cursor.lastrowid | ||
| else: | ||
| # Update existing dragon | ||
| cursor.execute(""" | ||
| UPDATE dragons | ||
| SET dragon_type = ?, name = ?, stage = ?, encrypted_data = ?, data_hash = ?, updated_at = ? | ||
| WHERE id = ? | ||
| """, (dragon_type, name, stage, encrypted_data, data_hash, current_time, dragon_id)) | ||
|
|
||
| conn.commit() | ||
| return dragon_id |
There was a problem hiding this comment.
The save_dragon method returns an integer dragon_id on success, but doesn't have a clear error indicator. If the save fails silently due to an exception being caught internally, it might return None or an invalid ID. The caller must check if the returned ID is valid. Consider either: (1) raising exceptions on failure rather than catching them, (2) returning a tuple (success: bool, dragon_id: Optional[int]), or (3) documenting clearly that the return value may be None on failure and callers must check for this.
| print("Migration successful! Future saves will use SQLite.") | ||
| # Optionally keep JSON as backup or delete it | ||
| # self.save_system_json.delete() | ||
| return success |
There was a problem hiding this comment.
If migration fails (success is False at line 560), the function returns False without loading any data. This means the player loses access to their save file until they manually intervene. The JSON data was loaded successfully but then failed to migrate. Consider: (1) falling back to using the JSON save system if migration fails, (2) keeping the game state in the already-loaded JSON data even if migration fails, or (3) at minimum, providing a clear error message to the user about what happened and how to recover.
| return success | |
| else: | |
| print("Migration to SQLite failed! Continuing to use JSON save system. Your progress is safe, but future saves will use the old format.") | |
| self.use_sqlite = False | |
| self.from_dict(data) | |
| # Add starter items if inventory is empty (for existing saves) | |
| if not self.inventory: | |
| self._add_starter_items() | |
| return True |
| # Secret key for HMAC - obfuscated to make casual editing harder | ||
| # In production, this could be more sophisticated | ||
| _SECRET = b"DragonRiders_SQLite_Integrity_Key_v1_2024_Secure" | ||
|
|
||
| # Encryption key - derived from secret (must be 32 bytes for Fernet) | ||
| _ENCRYPTION_KEY = base64.urlsafe_b64encode(hashlib.sha256(_SECRET).digest()) |
There was a problem hiding this comment.
The secret key is hardcoded in the source code with a comment acknowledging it's only obfuscation. While the comment at line 18 mentions "In production, this could be more sophisticated," this is in the production code. For a game with anti-cheat concerns, consider: (1) deriving the key from multiple sources (machine ID, game files, etc.), (2) using different keys per installation, or (3) at minimum, documenting clearly in SAVE_SYSTEM.md that this is client-side security only and determined users can extract the key.
| self._close_connection() | ||
|
|
||
| if Path(self.db_file).exists(): | ||
| Path(self.db_file).unlink() | ||
|
|
||
| # Reinitialize empty database | ||
| self._initialize_database() | ||
| return True |
There was a problem hiding this comment.
The delete_all method closes the connection, deletes the file, then reinitializes. However, if the connection wasn't closed properly (e.g., due to an exception), the file deletion might fail on Windows due to file locks. Additionally, if reinitialization fails after deletion, the save system is left in a broken state with no database file and no connection. Consider: (1) wrapping the deletion in a try-finally to ensure reinitialization happens, (2) checking if the connection exists before closing, or (3) creating a backup before deletion.
| json_system = SaveSystem(json_file) | ||
| sqlite_system = SQLiteSaveSystem(db_file) |
There was a problem hiding this comment.
Creating SaveSystem and SQLiteSaveSystem instances in this function could interfere with any existing game state instances. If the game is running when this verification is called, it could create conflicting database connections or file locks. Consider accepting save system instances as parameters or adding warnings about calling this while the game is not running.
| - **Algorithm**: Fernet (AES-128-CBC + HMAC-SHA256) | ||
| - **Key Size**: 256 bits (derived from secret) | ||
| - **IV**: Unique per encryption (handled by Fernet) | ||
| - **Padding**: PKCS7 padding (handled by Fernet) | ||
|
|
There was a problem hiding this comment.
The documentation states "AES-128-CBC" but Fernet actually uses AES-128 in CBC mode with a specific structure that includes a version, timestamp, IV, ciphertext, and HMAC. The documentation should be more precise about the encryption algorithm or simply refer to it as "Fernet encryption" without specifying the underlying details, which could change between versions of the cryptography library.
| - **Algorithm**: Fernet (AES-128-CBC + HMAC-SHA256) | |
| - **Key Size**: 256 bits (derived from secret) | |
| - **IV**: Unique per encryption (handled by Fernet) | |
| - **Padding**: PKCS7 padding (handled by Fernet) | |
| - **Algorithm**: Fernet encryption (see cryptography library documentation) | |
| - **Key Size**: 256 bits (Fernet key, derived from secret) | |
| - **IV and Padding**: Handled internally by Fernet |
| for i, dragon in enumerate(self.dragons): | ||
| dragon_dict = dragon.to_dict() | ||
| # Preserve database ID if it exists | ||
| if hasattr(dragon, '_db_id'): |
There was a problem hiding this comment.
The code uses hasattr(dragon, '_db_id') to check for a private attribute that's dynamically added to Dragon objects. This coupling between GameState and the save system implementation is fragile. If a Dragon is created through any other means, it won't have this attribute. Consider either: (1) adding _db_id as an optional field in the Dragon class itself, (2) maintaining a separate mapping of dragons to database IDs in GameState, or (3) using the dragon's position in the list as an implicit ID.
| if hasattr(dragon, '_db_id'): | |
| if dragon._db_id is not None: |
| import random | ||
| import time | ||
| from typing import List, Dict, Any, Optional | ||
| from pathlib import Path |
There was a problem hiding this comment.
Import of 'Path' is not used.
| from pathlib import Path |
| import os | ||
| import tempfile | ||
| import time | ||
| from pathlib import Path |
There was a problem hiding this comment.
Import of 'Path' is not used.
| from pathlib import Path |
No description provided.