Skip to content

SEP-1705: Add BaseManager.exists() and convert existence checks - #1251

Open
olucasandrade wants to merge 7 commits into
mainfrom
SEP-1705
Open

SEP-1705: Add BaseManager.exists() and convert existence checks#1251
olucasandrade wants to merge 7 commits into
mainfrom
SEP-1705

Conversation

@olucasandrade

@olucasandrade olucasandrade commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add BaseManager.exists() that emits a short-circuiting SELECT EXISTS (...) with the same filter signature as count(), built through _filter_query.
  • Convert the settings-override delete and app-drain finalize existence checks from count(...) to exists().
  • Delete TaskHistoryLogManager.exists_for_task and retarget its callers, tests, and docstring cross-references to exists(session, task_history_id=...).

Closes / implements SEP-1705. Related: SEP-1689 / PR #1235 review thread; blocks SEP-1707 only by landing first on the same statement.

Tested

  • N/A for primary path — internal manager API refactor with no user-visible UI change
  • Optional regression: as admin, DELETE a NOT_OVERRIDABLE settings key with no override row and confirm 409 is unchanged
  • Optional: disable an app with in-flight tasks and confirm DISABLING stays until tasks finish

Checklist

  • New/modified functions have type hints and rST docstrings
  • New tests added for new features or bug fixes
  • All tests pass locally (make test) — targeted gate: 264 passed, 1 skipped
  • Pre-commit hooks pass (make run-pre-commit) — ran on commit
  • Database migrations generated if models changed (make makemigrations) (N/A)
  • User-facing changes documented (README, inline help, UI text) (N/A)
  • Configuration changes documented with examples (N/A)
  • Changelog fragment added under changelog.d/ (N/A — internal-only refactor)

Route existence-by-count and the bespoke TaskHistoryLogManager.exists_for_task
helpers through a shared short-circuiting SELECT EXISTS query.
@olucasandrade olucasandrade self-assigned this Aug 3, 2026
@github-actions github-actions Bot added the python label Aug 3, 2026
@olucasandrade
olucasandrade marked this pull request as ready for review August 4, 2026 14:28
Copilot AI review requested due to automatic review settings August 4, 2026 14:28
@olucasandrade olucasandrade added the qa passed Tests for this PR are completed and successful. label Aug 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a reusable BaseManager.exists() helper for short-circuit existence checks across the FastAPI/SQLModel data layer, then updates a few hot paths (settings override delete, app drain finalize, and task-history log presence checks) to use it instead of count(...) > 0 or a bespoke manager method.

Changes:

  • Add BaseManager.exists() implemented via _filter_query(...) and SELECT EXISTS (...).
  • Replace select existence checks (count(...) > 0, app drain finalize) with exists(...).
  • Remove TaskHistoryLogManager.exists_for_task and retarget call sites/tests to TaskHistoryLogManager.exists(session, task_history_id=...).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
app/core/db/crud.py Adds BaseManager.exists() using _filter_query and SELECT EXISTS (...).
app/core/settings_override/api/routes.py Switches override-row existence detection from count(...) > 0 to exists(...).
app/sep/app_drain.py Converts the “any running tasks for app” check from count(...) to exists(...).
app/tasks/crud.py Removes TaskHistoryLogManager.exists_for_task and updates related doc references; cleans up now-unused import.
app/tasks/routes.py Updates task history has_logs computation to use TaskHistoryLogManager.exists(...).
app/tasks/logs/log_reader.py Switches chunk-presence detection from exists_for_task to exists(...).
tests/app/core/db/test_crud.py Adds focused unit tests covering BaseManager.exists() behavior and filter handling.
tests/app/tasks/test_routes.py Updates assertions to use TaskHistoryLogManager.exists(...).
tests/app/tasks/connectivity/test_service.py Updates log-row existence assertions to exists(...).
tests/app/tasks/connectivity/test_routes.py Updates log-row existence assertions to exists(...) for HTTP-level tests.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  app/core/db
  crud.py
  app/core/settings_override/api
  routes.py 1094
  app/sep
  app_drain.py
  inventory.py
  app/tasks
  crud.py
  routes.py 473
  app/tasks/logs
  log_reader.py
Project Total  

This report was generated by python-coverage-comment-action

@olucasandrade

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

@olucasandrade

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

Co-authored-by: olucasandrade <64823667+olucasandrade@users.noreply.github.com>

Copilot AI commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved in commit a1a14dc5 by merging latest main into SEP-1705 and fixing the conflict in /home/runner/work/SEP/SEP/app/tasks/crud.py while preserving the BaseManager.exists(...) migration.

@github-actions github-actions Bot added the svc:tasks PR touches the tasks service (app/tasks/) label Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python qa passed Tests for this PR are completed and successful. svc:tasks PR touches the tasks service (app/tasks/)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants