Fix two bugs found during test-matrix expansion#1331
Merged
Conversation
Bug 1: database.reset_data() crashes on a fresh SQLite file
--------------------------------------------------------------
reset_data() was the only function in modules/database.py that issued
a DELETE without first calling CREATE TABLE IF NOT EXISTS. On a fresh
config directory (e.g. in CI or after a clean install) this raises:
sqlite3.OperationalError: no such table: section_data
Fix: add cursor.execute(persisted_section_table_create()) before the
DELETE, matching the pattern used by every other helper in the module.
Bug 2: validate_github route crashes when token is invalid
----------------------------------------------------------
validate_github_server() (in modules/validations.py) returns a
(jsonify(payload), 400) tuple when the GitHub token is rejected.
The validate_github() route called result.get_json() directly without
unpacking the tuple first, crashing with:
AttributeError: 'tuple' object has no attribute 'get_json'
Root cause: an inconsistency introduced when the routes were written.
The validate_radarr and validate_sonarr routes already handled both
forms with 'if isinstance(result, tuple): result, status_code = result'.
The other five routes (omdb, github, tmdb, mdblist, notifiarr) did not.
Fix: add a module-level helper _unpack_validation_result(result) that
normalises both forms to (flask_response, status_code|None). All seven
affected routes now use it, so the class of bug cannot recur regardless
of what a validator chooses to return:
result, status_code = _unpack_validation_result(validations.validate_xxx_server(data))
if result.get_json().get('valid'):
return jsonify(result.get_json())
return jsonify(result.get_json()), status_code or 400
The sentinel None for 'plain response' makes 'None or 400 = 400'
correct for all error cases (the original hardcoded behaviour for the
five routes that previously didn't handle tuples).
Tests
-----
New file tests/test_bug_fixes.py (+16 tests):
Bug 1: reset_data() on empty db (no section arg), reset_data() on
empty db (with section arg), removes existing rows, section-scoped
delete does not disturb sibling sections.
Bug 2: validate_github returns 400 (not 500) when validator returns
tuple, 200 on plain success, 400 on plain error.
_unpack_validation_result unit tests: plain response, tuple, status
code coerced to int.
Parametrized check that all 7 routes (radarr, sonarr, omdb, github,
tmdb, mdblist, notifiarr) survive a tuple return and produce 400.
817 tests pass (801 existing + 16 new). ruff clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two bugs surfaced while writing the test-matrix expansion PR (#1330). Both are fixed here with regression tests.
Bug 1 —
database.reset_data()crashes on a fresh SQLite fileWhat happened
reset_data()was the only function inmodules/database.pythat issued aDELETEwithout first runningCREATE TABLE IF NOT EXISTS. On a fresh config directory (clean install, CI, isolated test fixture) this raises:Every other helper in the module (
save_section_data,retrieve_section_data,retrieve_all_section_data, etc.) follows the pattern:reset_dataskipped the first line.Fix
One line; matches the established pattern.
Bug 2 —
validate_githubroute crashes when the GitHub token is invalidWhat happened
validate_github_server()inmodules/validations.pyreturns either a plain Flask response or a(jsonify(payload), 400)tuple depending on the HTTP outcome:The
validate_githubroute calledresult.get_json()directly without unpacking the tuple first:This crashes with
AttributeError: 'tuple' object has no attribute 'get_json'every time a GitHub token is rejected.Root cause: inconsistent route pattern
The
validate_radarrandvalidate_sonarrroutes already handled both forms:The other five routes (
omdb,github,tmdb,mdblist,notifiarr) did not.Fix
A new module-level helper normalises both forms:
All seven routes now use it:
The
Nonesentinel ensuresNone or 400 = 400— the correct fallback for plain-response errors — while an explicit 400 from a tuple passes straight through.Tests —
tests/test_bug_fixes.py(+16 tests)Bug 1 (4 tests)
reset_data()does not crash on a totally fresh SQLite file (no section arg)reset_data(name, section=...)does not crash on a fresh SQLite filereset_data()removes existing rowsreset_data(name, section=X)leaves other sections untouchedBug 2 (6 tests)
validate_githubreturns 400 (not 500) when validator returns a tuple ← the crashvalidate_githubreturns 200 on successvalidate_githubreturns 400 on plain-response error_unpack_validation_resulthandles plain response (returnsNonesentinel)_unpack_validation_resulthandles tuple_unpack_validation_resultcoerces status code tointHardening (6 tests — parametrized)
CI
ruff checkclean on all 3 changed files