Skip to content

SEP-1687: Delete the legacy Jinja SSR layer - #1318

Open
yyyyyyyan wants to merge 1 commit into
mainfrom
SEP-1687
Open

SEP-1687: Delete the legacy Jinja SSR layer#1318
yyyyyyyan wants to merge 1 commit into
mainfrom
SEP-1687

Conversation

@yyyyyyyan

@yyyyyyyan yyyyyyyan commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

SEP-1687 — the terminal ticket of Wave 3 (SEP-950). Deletes the legacy Jinja server-rendered UI, leaving the backend headless.

React parity was certified in SEP-1684, the SPA has been served by default since SEP-1685, and PMM's nginx forwards only /api, /sep_app, /files, /stream-logs and /execution-events — so nothing in the shipping topology reaches the deleted surface.

Removed

  • 15 app Jinja routers (app/sep/apps/*/routes.py), the DeprecatedJinja2Route shim, and the jinja_router field/wiring on the app framework
  • 3 shared form/AJAX routers (/inventory-api, /stop-task, /periodic)
  • The shell handlers in app/sep/main.py: GET /login, POST /login, POST /logout, GET /
  • The form-post CSRF path (validate_csrf, IsCsrfValidated, CSRFMiddleware, csrf_exempt) and the flash-message middleware
  • Cookie-based session authentication; get_current_user is now Bearer-only
  • The per-app authenticated /static mounts, StaticMount, and AuthenticatedStaticFiles
  • 98 templates and the static/ tree — carving out templates/report/result_pdf.html.j2 and static/img/percona-logo.png, both read off disk by the report-PDF renderer
  • The snippets to_form() / _to_form() / to_form_field() path and the form-element DSL behind it (app/sep/snippets/forms.py)
  • djlint, plus the js/css beautify pre-commit hooks that existed only for static/

Changed

  • All six exception handlers return JSON unconditionally. auth_provider_exception_handler no longer resolves url_for("login"), which would raise NoMatchFound once that route is gone and turn every upstream auth failure into an unhandled 500.
  • The duplicated auth aliases collapse onto the Api* pair; IsApiAuthenticated = Depends(get_current_user).
  • RequireBearerForUnsafeMethods is hoisted to the /api router so every mutating route inherits it.
  • IsCsrfValidated dropped from POST /api/apps/topology/collect, closing a live defect: a caller presenting both a Bearer token and a stale session cookie previously fell into await request.form() on a JSON body and got a 400.
  • The React snippets download moves from the /static/snippets mount to the Bearer-authenticated GET /api/apps/snippets/snippet/download, via a new useSnippetDownload hook.

Breaking changes

Three, all in changelog.d/SEP-1687.breaking.md:

  1. Every SSR route and /static/* is gone. A browser client must send Authorization: Bearer; a session cookie alone now yields 401 where it previously redirected with 303.
  2. SEP.SESSION is removed — five keys (SESSION__COOKIE_NAME, __MAX_AGE, __SAMESITE, __SECURE, __PATH) disappear from GET /api/sep/admin/settings. SESSION_REFRESH (the SPA's refreshToken cookie) is unaffected.
  3. MessagesSettings is removed with the middleware it configured — MESSAGES_SETTINGS is dropped from the settingoverride.setting_class vocabulary.

Three Alembic data migrations clear the orphaned override rows and narrow the CHECK constraint: one on sep (SESSION + MESSAGES rows), one each on tasks and inventory (MESSAGES rows). All three were applied, downgraded and re-applied locally; alembic check reports no drift on any track.

Scope taken beyond the plan

  • MessagesSettings removal was not in the plan. Deleting the messages middleware orphaned a registered settings-override class (proxy map, admin settings API, SettingClassEnum). Surfaced mid-implementation; full removal including the enum member was chosen, which is what pulls in the tasks and inventory migrations.
  • Bundled fixes forced by whole-file gates on files this PR touches: a SEP-1373 docstring cross-reference in app/sep/apps/backup_mongo/deps.py, and two redundant response_model= kwargs in app/sep/apps/topology/api_routes.py.
  • app/sep/inventory.py gained an explicit model_rebuild() loop. Service/Schema reference each other's children before those classes exist, so Pydantic defers the build; a deleted Jinja route signature was incidentally forcing the rebuild. Resolving it in the defining module fixes SystemFactsService, which cannot resolve Schema from its own namespace.
  • HTTPRedirectException deleted from app/core/exceptions.py — its last consumer (LoginRedirectException) went with this change. The plan asked for this check explicitly.

Plan deviations

Three places where the approved plan was wrong, verified against the code:

  1. SessionOptions must stay. The plan said to delete the class with the SESSION field, but SESSION_REFRESH: SessionOptions is retained by the same criterion. Only the field is removed.
  2. The plan's legacy_checksums_create_to_form trap is inverted. It claims the symbol is live via deps.py:237 inside build_checksums_payload (the payload_builder=). Line 237 was in fact inside build_checksums_task_payload — a Form()-body Jinja handler with zero consumers — while the live build_checksums_payload never calls it. Both are deleted; checksums task creation goes through build_checksums_payload, unaffected.
  3. app/sep/connectivity.py needed more than the message call removed. get_check_connectivity_flag, check_and_warn_connectivity and maybe_check_connectivity all lost their last consumers with the Jinja routers; stripping only the messages.warning(...) call would have left a "warn" helper that never warns and nothing calls.

Tested

  • 3 migrations applied → downgraded → re-applied on all three tracks; alembic check clean on sep, tasks, inventory; the SESSION deletion SQL escapes the LIKE wildcard so SESSION_REFRESH__* rows survive (covered by a comment at the SQL).
  • Route table asserted post-change: the surviving non-/api surface is exactly artifacts, files, stream-logs, execution-events, health, docs — no /static, /legacy, /login, /logout, /.
  • New tests/app/sep/apps/test_import_smoke.py imports all 15 app packages and asserts none re-exports a router — guards the __init__.py trap that would have crashed startup.
  • New tests/app/sep/routes/test_shared_route_auth.py pins 401 (not 303) on /files, /stream-logs, /execution-events for a credential-less and a cookie-only caller.
  • New TestJsonExceptionHandlers in tests/app/sep/test_main.py covers the 404/422/HTTPException-with-headers/BaseAuthProviderException paths and asserts the deleted shell routes 404.
  • Frontend: @sep/api (102), @sep/snippets (72) and the vite-proxy suite pass; pnpm -r type-check clean.
  • Backend suites run green locally: tests/app/sep/{api,routes,sync,snippets,apps/*}, tests/app/core/settings_override, test_main.py, test_deps.py, test_connectivity.py.

Not verified in one pass: the full suite. It exceeds the local time budget, so it was exercised directory-by-directory rather than as a single run. CI's python job is skipped while qa in progress is set (label-gate blocks it by design), so the first end-to-end signal arrives when QA flips the label — worth a full run before merge.

Acceptance criteria

17 of 18 ticked. The one left unchecked is "The legacy session-cookie setting and its options class are removed" — the criterion is internally contradictory: it also retains SESSION_REFRESH, which is typed by SessionOptions. Everything else in that criterion (the breaking admin-API change, the five removed keys, the sep-track data migration, SESSION_REFRESH unaffected) is satisfied.

Checklist

  • New/modified functions have type hints and rST docstrings
  • New tests added for new features or bug fixes
  • All tests pass locally in one pass — run per-directory instead; see Tested
  • Pre-commit hooks pass
  • Database migrations generated if models changed
  • User-facing changes documented
  • Configuration changes documented with examples
  • Changelog fragment added under changelog.d/

Remove the server-rendered UI that nothing reaches in the shipping topology:
15 app Jinja routers, the three shared form/AJAX routers, the login/logout/
homepage shell handlers, CSRF form validation, flash messages, cookie-based
session authentication, the per-app authenticated static mounts, and the
98 SSR templates plus their static assets. The report PDF template and its
logo are carved out and keep rendering off disk.

The six exception handlers now return JSON unconditionally; the auth-provider
handler in particular no longer resolves url_for("login"), which would raise
NoMatchFound once that route is gone.

get_current_user is Bearer-only and raises HTTPUnauthorizedException before
oauth2_scheme can surface a bare Starlette error. The duplicated auth aliases
collapse onto the Api* pair, and RequireBearerForUnsafeMethods is hoisted to
the /api router.

Also removes the legacy SEP.SESSION setting and the MessagesSettings class,
with data migrations on all three tracks clearing their orphaned override
rows and narrowing the setting_class vocabulary.
Copilot AI lite review requested due to automatic review settings August 8, 2026 08:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@github-actions github-actions Bot added python frontend app:alert_troubleshooting PR touches the alert_troubleshooting app slice app:alerts PR touches the alerts app slice app:alters PR touches the alters app slice app:archives PR touches the archives app slice app:atw PR touches the atw app slice app:backup_mongo PR touches the backup_mongo app slice app:backup_pg PR touches the backup_pg app slice app:checksums PR touches the checksums app slice app:dipper PR touches the dipper app slice app:inventory PR touches the inventory app slice app:mysql_backups PR touches the mysql_backups app slice app:report PR touches the report app slice app:snippets PR touches the snippets app slice app:tasks PR touches the tasks app slice app:topology PR touches the topology app slice svc:tasks PR touches the tasks service (app/tasks/) svc:inventory PR touches the inventory service (app/inventory/) large-diff Over 1500 changed lines, generated files discounted labels Aug 8, 2026
@yyyyyyyan yyyyyyyan added the qa in progress Someone is currently testing this PR - do not merge it label Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app:alert_troubleshooting PR touches the alert_troubleshooting app slice app:alerts PR touches the alerts app slice app:alters PR touches the alters app slice app:archives PR touches the archives app slice app:atw PR touches the atw app slice app:backup_mongo PR touches the backup_mongo app slice app:backup_pg PR touches the backup_pg app slice app:checksums PR touches the checksums app slice app:dipper PR touches the dipper app slice app:inventory PR touches the inventory app slice app:mysql_backups PR touches the mysql_backups app slice app:report PR touches the report app slice app:snippets PR touches the snippets app slice app:tasks PR touches the tasks app slice app:topology PR touches the topology app slice frontend large-diff Over 1500 changed lines, generated files discounted python qa in progress Someone is currently testing this PR - do not merge it svc:inventory PR touches the inventory service (app/inventory/) svc:tasks PR touches the tasks service (app/tasks/)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants