DB functions tidyup and streamlining #1169
Conversation
Added PRAGMAs for better DB performance on open. Integrated some Fake8 comments and eliminated some looping with more efficient pyton functions.
WalkthroughThe PR refactors Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant DB as server/database.py
participant SQLite
Caller->>DB: get_temp_db_connection()
activate DB
DB->>SQLite: sqlite3.connect(file or :memory:)
Note right of SQLite: PRAGMAs set:\n- journal_mode = WAL\n- synchronous = NORMAL\n- temp_store = MEMORY\n- busy_timeout\n- row_factory = Row
SQLite-->>DB: Connection (Row factory)
DB-->>Caller: Connection handle
deactivate DB
rect rgba(230,245,255,0.5)
Note over Caller,DB: Caller uses connection for operations\n(read/read_one/get_table_as_json)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/database.py (2)
85-124: Wrap schema upgrades in a single transaction to avoid partial upgradesWith isolation_level=None (autocommit), early returns on failure can leave the DB half-upgraded. Wrap initDB in an explicit transaction and rollback on error.
def initDB(self): @@ - # Add Devices fields if missing - # devFQDN - if ensure_column(self.sql, "Devices", "devFQDN", "TEXT") is False: - return # addition failed - # devParentRelType - if ensure_column(self.sql, "Devices", "devParentRelType", "TEXT") is False: - return # addition failed - # devRequireNicsOnline - if ensure_column(self.sql, "Devices", "devReqNicsOnline", "INTEGER") is False: - return # addition failed + try: + # Start transactional upgrade + self.sql_connection.execute('BEGIN IMMEDIATE;') + + # Add Devices fields if missing + if ensure_column(self.sql, "Devices", "devFQDN", "TEXT") is False: + raise RuntimeError("ensure_column(devFQDN) failed") + if ensure_column(self.sql, "Devices", "devParentRelType", "TEXT") is False: + raise RuntimeError("ensure_column(devParentRelType) failed") + if ensure_column(self.sql, "Devices", "devReqNicsOnline", "INTEGER") is False: + raise RuntimeError("ensure_column(devReqNicsOnline) failed") @@ - # commit changes - self.commitDB() + # commit changes + self.commitDB() + except Exception as e: + self.rollbackDB() + mylog('minimal', ['[Database] - initDB aborted, rolled back: ', e]) + return
184-196: Prevent TypeError when read() returns NoneIf read() returns None on error, len(rows) raises TypeError. Guard for falsy rows first.
- rows = self.read(query, *args) - if len(rows) == 1: - return rows[0] - - if len(rows) > 1: + rows = self.read(query, *args) + if not rows: + return None + if len(rows) == 1: + return rows[0] + if len(rows) > 1: mylog('verbose',[ '[Database] - Warning!: query returns multiple rows, only first row is passed on!', query, " params: ", args]) return rows[0] # empty result set return None
🧹 Nitpick comments (7)
server/database.py (7)
25-45: WAL + PRAGMAs: good; add foreign_keys and busy_timeout, and keep PRAGMAs consistent with temp connsTo enforce referential integrity and reduce lock errors, enable foreign keys and set busy_timeout here (you already do it in get_temp_db_connection). Also consider verifying WAL activation result.
def open(self): @@ - self.sql_connection = sqlite3.connect(fullDbPath, isolation_level=None) + self.sql_connection = sqlite3.connect(fullDbPath, isolation_level=None) @@ self.sql_connection.execute('pragma journal_mode=WAL;') # When synchronous is NORMAL (1), the SQLite database engine will still sync # at the most critical moments, but less often than in FULL mode. self.sql_connection.execute('PRAGMA synchronous=NORMAL;') # When temp_store is MEMORY (2) temporary tables and indices # are kept as if they were in pure in-memory databases. self.sql_connection.execute('PRAGMA temp_store=MEMORY;') + # Enforce FK constraints and avoid immediate "database is locked" errors + self.sql_connection.execute('PRAGMA foreign_keys=ON;') + self.sql_connection.execute('PRAGMA busy_timeout=5000;')Note: WAL on network-mounted filesystems can be problematic; if that’s a deployment scenario, make this configurable and fall back to DELETE journal mode.
27-31: Nit: log message nameThe log says "openDB" but the method is open(); align for consistency.
- mylog('debug', 'openDB: database already open') + mylog('debug', 'open: database already open')
70-82: Good vectorized conversion; fix log message and consider returning empty listNice list-comprehension switch. Update the log string to the function name; also prefer returning [] explicitly when DB not open for a stable return type.
- mylog('debug', 'getQueryArray: database is not open') + mylog('debug', 'get_sql_array: database is not open') - return + return []
116-121: Comment label mismatchThis section creates indexes, not views. Minor comment fix.
- # Views + # Indexes ensure_Indexes(self.sql)
168-181: Support CTEs ('WITH ...') in read(), current SELECT-only assert is too strictQueries starting with WITH will fail the assert; broaden the check. Also be aware that assert can be optimized out with -O; consider explicit checks later.
- assert query.count('?') == len(args) - assert query.upper().strip().startswith('SELECT') + assert query.count('?') == len(args) + q = query.lstrip().upper() + assert q.startswith('SELECT') or q.startswith('WITH')
215-226: Optional: align with get_sql_array using a comprehensionFunction is fine; for consistency and speed you could use a single-pass comprehension.
- arr = [] - for row in rows: - if isinstance(row, sqlite3.Row): - arr.append(list(row)) # Convert row to list - elif isinstance(row, (tuple, list)): - arr.append(list(row)) # Already iterable, just convert to list - else: - arr.append([row]) # Handle single values safely - - return arr + return [list(row) if isinstance(row, (sqlite3.Row, tuple, list)) else [row] for row in rows]
230-239: Make temp connections mirror main PRAGMAs; enforce FKs; minor consistencyAlign PRAGMAs with open(); add foreign_keys; set text_factory for parity. Also confirm every temp connection is closed (or expose a contextmanager).
def get_temp_db_connection(): @@ - conn = sqlite3.connect(fullDbPath, timeout=5, isolation_level=None) - conn.execute("PRAGMA journal_mode=WAL;") - conn.execute("PRAGMA busy_timeout=5000;") # 5s wait before giving up - conn.row_factory = sqlite3.Row + conn = sqlite3.connect(fullDbPath, timeout=5, isolation_level=None) + conn.execute("PRAGMA journal_mode=WAL;") + conn.execute("PRAGMA synchronous=NORMAL;") + conn.execute("PRAGMA temp_store=MEMORY;") + conn.execute("PRAGMA foreign_keys=ON;") + conn.execute("PRAGMA busy_timeout=5000;") # 5s wait before giving up + conn.text_factory = str + conn.row_factory = sqlite3.Row return connWould you like a small contextmanager helper to ensure connections are closed automatically?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/database.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/database.py (4)
server/logger.py (1)
mylog(86-89)server/db/db_helper.py (2)
get_table_json(183-210)json_obj(213-275)server/workflows/app_events.py (1)
AppEvent_obj(25-179)server/db/db_upgrade.py (6)
ensure_column(11-69)ensure_views(72-154)ensure_CurrentScan(211-237)ensure_Parameters(239-258)ensure_Settings(260-286)ensure_Indexes(156-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (3)
server/database.py (3)
6-11: LGTM: import cleanup and helper consolidationUnused imports removed; targeted imports from const and db_helper look correct.
54-57: LGTM: safe commit guardEarly return when DB is not open avoids exceptions.
150-159: LGTM: centralized table->JSON handling and tighter loggingDelegating to db_helper.get_table_json with minimal logging is cleaner.
|
Nice one @ingoratsdorf ! Is this ready to merge or do you need to perform additional tests? |
|
I have opted to review some of the CodeRabbot suggestions and will have a few test runs first with the modified code. |
Added some of the hand picked suggestions, including some outside of the previous changes. Some will improve documentation, some readability and some will affect performance.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
server/database.py (4)
24-31: Consider adding type hints for DB attributes.Helps static analysis and readability.
You can annotate after initialization:
self.sql: sqlite3.Cursor | None = None self.sql_connection: sqlite3.Connection | None = None
59-71: Add busy timeout and enable foreign keys; also set connect timeout.Prevents frequent “database is locked” under WAL and enforces referential integrity. Mirrors your get_temp_db_connection() behavior.
Apply:
- self.sql_connection = sqlite3.connect(fullDbPath, - isolation_level=None) + self.sql_connection = sqlite3.connect( + fullDbPath, timeout=5, isolation_level=None + ) @@ self.sql_connection.execute('pragma journal_mode=WAL;') @@ self.sql_connection.execute('PRAGMA synchronous=NORMAL;') @@ self.sql_connection.execute('PRAGMA temp_store=MEMORY;') + self.sql_connection.execute('PRAGMA busy_timeout=5000;') + self.sql_connection.execute('PRAGMA foreign_keys=ON;')Also consider applying the same PRAGMAs in get_temp_db_connection() for consistency.
143-149: DRY the repeated ensure_column checks; also satisfies Ruff TRY301/TRY003.Reduces repetition and centralizes the error message.
Apply:
- # Add Devices fields if missing - if not ensure_column(self.sql, "Devices", "devFQDN", "TEXT"): - raise RuntimeError("ensure_column(devFQDN) failed") - if not ensure_column(self.sql, "Devices", "devParentRelType", "TEXT"): - raise RuntimeError("ensure_column(devParentRelType) failed") - if not ensure_column(self.sql, "Devices", "devReqNicsOnline", "INTEGER"): - raise RuntimeError("ensure_column(devReqNicsOnline) failed") + # Add Devices fields if missing + def _ensure_devices_column(name: str, col_type: str) -> None: + if not ensure_column(self.sql, "Devices", name, col_type): + raise RuntimeError(f"ensure_column({name}) failed") + + _ensure_devices_column("devFQDN", "TEXT") + _ensure_devices_column("devParentRelType", "TEXT") + _ensure_devices_column("devReqNicsOnline", "INTEGER")
285-298: Row-to-array helper is concise; consider typing for callers.Minor: expose an alias like
RowLike = sqlite3.Row | tuple | list | Anyand typerows: Iterable[RowLike]to aid tooling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/database.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/database.py (4)
server/logger.py (1)
mylog(86-89)server/db/db_helper.py (2)
get_table_json(183-210)json_obj(213-275)server/workflows/app_events.py (1)
AppEvent_obj(25-179)server/db/db_upgrade.py (7)
ensure_column(11-69)ensure_views(72-154)ensure_CurrentScan(211-237)ensure_plugins_tables(289-384)ensure_Parameters(239-258)ensure_Settings(260-286)ensure_Indexes(156-205)
🪛 Ruff (0.12.2)
server/database.py
145-145: Abstract raise to an inner function
(TRY301)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Abstract raise to an inner function
(TRY301)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
149-149: Abstract raise to an inner function
(TRY301)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (9)
server/database.py (9)
6-14: Imports cleanup and consolidation look good.Reduced surface area and clearer intent. No issues spotted.
52-54: Idempotent open guard LGTM.Avoids duplicate connections cleanly.
94-98: Rollback docstring addition LGTM.Clearer intent; no functional change.
201-209: get_table_as_json wrapper: toned-down logging with safe fallback looks good.Keeps call sites simple; no issues.
229-232: Reduced logging verbosity on read() errors looks appropriate.Clear messages without noise. LGTM.
235-247: read_one behavior and diagnostics look good.Reasonable to return first row with a warning when multiple rows are found.
253-269: get_device_stats docstring and wrapper LGTM.Clear contract and usage.
272-281: get_all_devices docstring and wrapper LGTM.No issues.
79-87: Return-type change — safe: no callers depend on a previous return value.I searched the repo for commitDB() usages; all call sites invoke it for side-effects (no assignments, returns, or conditionals using its value), so changing commitDB() to return bool is safe.
|
It's been running for a few hours and looks stable. I'd say ready to be merged., :-) |
|
🚀 :) |
2 participants
Deleted unused imports, added more appropriate
PRAGMA synchronous=NORMALto the db open function. Tidied up some code and made added Flake8 suggestions, optimisedget_sql_array.Whitespace fixes (part of Flake8).
The change looks like it yielded some further CPU cycle savings and most of all reduces the amount of filesystem writes.
Comparison of CPU use before and after the changes:
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
New Features