Reduce cognitive complexity in three handlers (group 8, part 2) - #311
Conversation
Clears 3 of the 6 python:S3776 findings by extracting helpers, with no behavior change. The other 3 are deliberately not fixed; see below. api/sockets.py, both WebSocket handlers (complexity 18 each): * Both opened with an identical validate-and-reject block differing only in regex and wording, so _reject_invalid_name() deduplicates rather than merely relocating. Log text is preserved exactly, including the !r formatting, by passing the field label through. * _build_journalctl_command() takes the shlex quoting and the since/scope branching out of the handler. Verified against HEAD's logic across 96 combinations of unit name, scope and since, including unknown since values and names needing quoting: 0 mismatches. * _cleanup_terminal() takes the finally block's nested cancel/kill/wait. Every `except Exception: pass` is kept: cleanup must not mask the original error. Order is unchanged, and disconnect plus its log stay in the caller's finally. services/stats_engine.py fetch_server_stats (complexity 18): the depth came from a for/try/if/for/for/if chain. _scope_stat_tasks(), _merge_scope_results() and _build_unit_rows() flatten it. Code moved verbatim. The risk in that one is precedence: `if c["name"] not in seen` means the first scope reporting a name wins, and user scope is appended first, so a user container shadows a global one of the same name. A dict comprehension or dict.update would silently invert that. Verified directly: with the same container name in both scopes, the user entry wins, both scopes' unit states survive independently, and scope_filter still maps to the same labels. Not fixed, on purpose: * sync_engine.py check_quadlets (complexity 25) and _fetch_mtimes. Bug #283 reports check_quadlets iterates the quadlets table, and the only INSERT INTO quadlets in the repo is in test files, so it is dead in production. Its uncovered branches are uncoverable while the work set is always empty, so no test can protect a refactor of the most complex function here. Fixing #283 rewrites it anyway. * scripts/import_servers.py, a one-off script not referenced by Dockerfile or start.sh and not covered. Unit 939 and unmarked 47, both unchanged. pylint reports no unused arguments on the new helpers.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
The SonarCloud gate is red on one finding, It is pre-existing, and this PR reduces it
So nothing new is introduced. The count goes from 10 to 9. It reports as new only because the consolidated line is new code, the same inherit-the-debt effect that made #304's gate red and that surfaced again on #310. The
|
Log statements in the WebSocket handlers interpolated values taken straight off the socket (unit_name, container_name, resize dimensions), so a caller could embed a newline and forge log entries. Add a _log_safe() helper that escapes control characters via unicode_escape and route every affected log statement through it. Escaping rather than stripping keeps the exact input visible, which is what matters when debugging a rejected name. Two related fixes in the same area: - The name allowlists used $, which also matches just before a trailing newline, so "myapp\n" passed validation. Switched to \Z. This matters beyond logging, since container_name is interpolated into the podman exec command unquoted and relies entirely on that pattern. - exec_terminal_over_websocket logged container_name before validating it. Moved the log after the check, matching the log stream handler.
RUF013: `since` is genuinely optional, so annotate it `str | None`. The propagated annotation on `_build_journalctl_command` follows, since that helper's `-n 100` branch exists precisely to handle the None case. Guarding the `_SINCE_PHRASES.get()` lookup keeps the dict's `str` key type honest without changing behaviour: `.get(None)` already returned None. BLE001 at the invalid-name rejection: the broad catch is deliberate and narrowing it would be a regression, because `manager.disconnect()` runs after the try block, so any unnamed exception type would propagate and leak the socket into `active_connections`. What was actually wrong is that the handler was silent. It now logs at debug and carries a `# noqa` with the rationale, matching the convention already used in tests/podman/conftest.py.
…LE001) This function was extracted from an inline finally block by the complexity refactor, so its three broad catches read as new code to the analyzers even though the behaviour predates this PR. Narrowing them is not the right fix: teardown must never mask the error that caused teardown, which is what the docstring already says. The defect is that all three failed silently, so a leaked remote process or an unreaped exit left no trace. Each now logs at debug and carries the rationale inline.
|
1 participant
Sixth PR in the SonarCloud cleanup series, completing group 8 alongside #310. Clears 3 of the 6
python:S3776findings. The other 3 are deliberately left open with reasons below.This is the first PR in the series that changes live control flow rather than adding attributes, constants or types, so the emphasis throughout is on proving nothing moved.
api/sockets.py, two WebSocket handlers, complexity 18 each
_reject_invalid_name(): both handlers opened with an identical validate-and-reject block differing only in regex, log wording and error text. One shared helper deduplicates it rather than relocating it twice. The log text is preserved exactly,!rformatting included, by passing the field label through._build_journalctl_command(): lifts theshlexquoting and the since/scope branching out. Since this builds a command string that runs on a remote host, it was checked exhaustively rather than by eye. 96 combinations of unit name, scope andsince, covering unknownsincevalues, unrecognised scopes, and unit names requiring shell quoting, compared against HEAD's logic. 0 mismatches._cleanup_terminal(): takes thefinallyblock's nested cancel/kill/wait. Everyexcept Exception: passis preserved deliberately, because cleanup must not mask the original error. Order is unchanged, andmanager.disconnect()plus its log stay in the caller'sfinally.services/stats_engine.py
fetch_server_stats, complexity 18Depth was the problem:
for serverintotryintoif scope_filterintofor scope_labelintofor cintoif c["name"] not in seen. Split into_scope_stat_tasks(),_merge_scope_results()and_build_unit_rows(), with code moved verbatim.The trap here is precedence.
if c["name"] not in seenmeans the first scope reporting a container name wins, and user scope is appended first, so a user container shadows a global one with the same name. Rewriting that merge as a dict comprehension ordict.updatewould make the last writer win and silently invert it, with no test necessarily catching it. Verified directly:The ordering check matters too, since the labels are
zipped againstasyncio.gatherresults, so a reordering would mislabel every scope.Verification
-m unit: 939 passed, 3 skipped, unchangedpylint -e W0613: no unused arguments on the new helpersCoverage is what made these three safe to touch.
api/sockets.pysits at 99% andservices/stats_engine.pyat 98%, so the existing suite genuinely exercises the paths being restructured.The three left open, and why
sync_engine.pycheck_quadlets(complexity 25, the largest in the group) and_fetch_mtimes. Open bug #283 reports thatcheck_quadletsiterates thequadletstable and that table is never populated. Confirmed: the onlyINSERT INTO quadletsanywhere in the repo is in test files. The function is dead in production.That is not just an argument about wasted effort. Its uncovered branches are uncoverable by construction while the work set is always empty, so no test can protect a refactor of the most complex function in the group. Whoever fixes #283 restructures it anyway. Refactoring it now would be unprotected churn on code that is about to be rewritten.
scripts/import_servers.py. Not referenced byDockerfileorstart.sh, so not shipped, and not covered. Restructuring untested, unshipped code to satisfy a metric is risk without benefit.