feat: new http foundation - #256
Conversation
|
|
||
|
|
||
| def test_registry_entries_are_unique_route_groups(): | ||
| import djehuty.route_groups as rg |
35b0253 to
4bb5e7e
Compare
| that a missing new stack degrades to legacy instead of failing. | ||
| """ | ||
|
|
||
| import djehuty.route_groups as rg |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
==========================================
+ Coverage 18.10% 19.00% +0.90%
==========================================
Files 21 24 +3
Lines 10521 10633 +112
Branches 2040 2063 +23
==========================================
+ Hits 1905 2021 +116
+ Misses 8422 8414 -8
- Partials 194 198 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e541fdb to
31d8802
Compare
There was a problem hiding this comment.
Very exciting changes and a strong foundation! Reading the http-migration documentation beforehand helped understand the code easier.
Two things to fix before merge:
- route_groups.py: the prefixes=("/api/",) claims the whole namespace.
- dispatch.py: create_app(db) sits outside the try/except, so a failure building the umbrella app takes down the legacy stack the fallback exists to protect. I've suggested a regression test alongside it.
also don't forget to rebase before next round.
| global _UWSGI_APP | ||
| if _UWSGI_APP is None: | ||
| server = main (config_file=config_file, run_internal_server=False) | ||
| from djehuty.dispatch import build_wsgi_app | ||
| _UWSGI_APP = build_wsgi_app (server, server.db, | ||
| config.web_service, config.web_service_groups) |
There was a problem hiding this comment.
Nothing checks whether main() succeeded before using its result.
| global _UWSGI_APP | |
| if _UWSGI_APP is None: | |
| server = main (config_file=config_file, run_internal_server=False) | |
| from djehuty.dispatch import build_wsgi_app | |
| _UWSGI_APP = build_wsgi_app (server, server.db, | |
| config.web_service, config.web_service_groups) | |
| global _UWSGI_APP | |
| if _UWSGI_APP is None: | |
| server = main (config_file=config_file, run_internal_server=False) | |
| if server is None: | |
| start_response('500 Internal Server Error', [('Content-Type','text/html')]) | |
| return [b"<p>djehuty failed to start. See the log for details.</p>"] | |
| from djehuty.dispatch import build_wsgi_app | |
| _UWSGI_APP = build_wsgi_app (server, server.db, | |
| config.web_service, config.web_service_groups) | |
Same failure mode pattern already done above.
| @@ -1424,5 +1508,10 @@ def application (env, start_response): | |||
| start_response('200 OK', [('Content-Type','text/html')]) | |||
| return [b"<p>Please set the <code>DJEHUTY_CONFIG_FILE</code> environment variable.</p>"] | |||
|
|
|||
| server = main (config_file=config_file, run_internal_server=False) | |||
| return server (env, start_response) | |||
| global _UWSGI_APP | |||
| if _UWSGI_APP is None: | |||
| server = main (config_file=config_file, run_internal_server=False) | |||
| from djehuty.dispatch import build_wsgi_app | |||
| _UWSGI_APP = build_wsgi_app (server, server.db, | |||
| config.web_service, config.web_service_groups) | |||
| return _UWSGI_APP (env, start_response) | |||
There was a problem hiding this comment.
make the check and set atomic by using a lock.
something like :
import threading
_UWSGI_APP = None
_UWSGI_LOCK = threading.Lock()
...
global _UWSGI_APP
if _UWSGI_APP is None: # fast path, no lock once booted
with _UWSGI_LOCK:
if _UWSGI_APP is None: # the check that actually matters
server = main (...)
_UWSGI_APP = build_wsgi_app (...)
return _UWSGI_APP (env, start_response)
with _UWSGI_LOCK only one thread at a time gets past that line; the others wait. the next thread doesn't start the process while first thread is running main which takes a bit of time.
There was a problem hiding this comment.
IMHO, Not needed here. This path only runs under uWSGI, and each worker is a separate process that boots _UWSGI_APP once, there's no shared state and no concurrent first-request race to lock against. I'll add the double-checked lock if we ever move to threaded workers.
| config.web_service_groups.update (_read_web_service_targets (groups_node, logger)) | ||
|
|
There was a problem hiding this comment.
more of a documentation gap than a bug: Config files can other config files, and read_configuration_file reads its own settings first (line 860) and only processes includes afterwards (line 1045). So if an included file also has a web-service block, it overwrites the top-level one. Last file read wins, and nothing is logged about it.
Every setting in this file already works that way, so web-service is consistent with the rest. But it's the first setting operators are told to change during an incident. Someone follows the rollback steps in docs/http-migration.md, edits the main config, restarts, and if an included file happens to set web-service too, nothing changes.
Separately, config.web_service_groups.update(...) merges not replaces, so an included file can add or change a group override but can not clear one the parent set.
suggestions:
- Point operators at the log. build_wsgi_app already logs default=... overrides=... at INFO on startup, so the effective values are recoverable. Adding "check this log line after restarting" to the rollback runbook might be enough.
- Add a note to docs/http-migration.md saying included files take precedence, so others know to check them before editing.
- Decide whether .update() is the behaviour you want for groups, or document that an include can't clear an override.
| NEW_STACK = ("djehuty.route_groups", "djehuty.application", "djehuty.dispatch") | ||
|
|
There was a problem hiding this comment.
so requires manual maintanance currently as we have to not forget to add here for each group PR. it would be best if we discover NEW_STACK by walking the package rather than listing it to make it self-maintaining, but not a big deal.
There was a problem hiding this comment.
Agreed it'd be cleaner, but leaving it explicit for now. Forgetting to add a module fails loudly and gets caught in review, and it's one line per group PR. Happy to revisit if the list grows.
| elif isinstance(node, ast.ImportFrom) and node.level == 0 and node.module: | ||
| modules.add(node.module) |
There was a problem hiding this comment.
needs fixing:
This guardrail catches only 2 of the 6 ways i tested one can write the forbidden import.
node.level == 0 skips every relative import, and for from x import y it records only x, throwing y away. I ran the current logic against each form:
OKAY import djehuty.web.wsgi
OKAY from djehuty.web.wsgi import WebServer
MISSED from djehuty.web import wsgi
MISSED from .web import wsgi
MISSED from .web.wsgi import WebServer
The third line is most important as that import style pops up in our documentation.
Matters, because every later group PR will be reviewed on the assumption this test works!
suggestion:
- create new function such as
def _imported_modules(tree: ast.AST, package: str) -> set: """Every module the file imports: absolute, relative, and from-imported names.""" pass
c058a02 to
451630f
Compare
|
thanks for the review @641e16 |
641e16
left a comment
There was a problem hiding this comment.
Thank you for the changes! Looks good to me :)
The only minor things I see that may be worth checking out:
- uv.lock maybe needs regenerating with the pinned uv?
- for packaging: spec.in, guix.scm.in, requirements.txt, pyproject.toml.in still have no fastapi? but we are moving away from them, right?
- djehuty-example-config.xml:4 -> doc/http-migration.md should be docs/; doc/ is the LaTeX manual.
Also, You could add the three new modules to the include list under [tool.ruff] and run just format. The code is already well formatted so it would be nice to add it already as part of the formatted files.
| <maintenance-mode>0</maintenance-mode> | ||
| <!-- New HTTP stack switch (see doc/http-migration.md). default is new|legacy; | ||
| list a functional group under <groups> to pin it, e.g. | ||
| <groups><api-v2>legacy</api-v2></groups>. The docs (/api/) are always new. --> |
There was a problem hiding this comment.
one quick fix: this needs to be updated to mention the three URLs not the directory prefix.
I think we need to fix it in the release process, because it happens after every release. I will investigate it, maybe we need a issue for it.
Right, leaving those as-is since we're moving away from them; pyproject.toml is the source of truth for the runtime deps. |
641e16
left a comment
There was a problem hiding this comment.
The changes look good! I approve.
Add docs/http-migration.md: how we ship the FastAPI stack on refact/new-http group by group, with a per-group new/legacy switch so any group can be flipped back to legacy in production via config + restart. Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Add fastapi, pydantic, a2wsgi and python-multipart as runtime dependencies, and httpx as a dev dependency for Starlette's TestClient. No code uses these yet. Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
- config: web_service (default "new"|"legacy") + web_service_groups overrides. - ui.py: read_web_service_configuration parses the flat value and the object form (default + groups), JSON and XML, with "api-service" as an alias. run_simple now serves the dispatcher. - dispatch.py: WebServiceDispatcher routes per route_groups.target_for_path; build_wsgi_app wraps the umbrella app + legacy and falls back to legacy if the new stack cannot be imported. Never imports djehuty.web.wsgi. - example configs: document the web-service block (default new, empty groups; each group PR adds its own line). Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Route-group matching and resolution (incl. the api-docs group and always_new staying new when everything is disabled), config parsing (flat and object, JSON and XML, api-service alias), dispatcher routing and legacy fallback, the umbrella serving /api/docs, and a guardrail that no new-stack module imports the legacy WSGI app. Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Strip whitespace, lowercase, and warn on unrecognized values so a typo or indented config no longer silently stays on the wrong target. Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Co-authored-by: vic <110847019+641e16@users.noreply.github.com>
Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
613eed8 to
36f1b4e
Compare
Summary
Foundation for the phased Werkzeug to FastAPI HTTP migration (see
docs/http-migration.md). It adds the three pieces every later group PR builds on: aroute-group registry (which group owns a path), an umbrella FastAPI app that mounts
the new routers, and a WSGI dispatcher that sends each request to the new stack or to
the legacy
wsgi.pybased on a per-groupweb-serviceconfig switch. The point ofthe switch: any migrated group can be flipped back to legacy in production with a
one-word config change and a restart — no rebuild, no hotfix.
This PR migrates no routes. No groups are registered yet, so every request is served
by legacy exactly as before. The only new surface is the API reference under
/api/(
/api/docs,/api/redoc,/api/openapi.json), served by the new stack; legacy hasno
/api/routes.wsgi.pyis untouched.Changes
docs/http-migration.md,mkdocs.yml: the migration plan — principles (AS-IS,reversible per group), route groups, rollout/rollback, and how legacy is removed.
pyproject.toml,uv.lock: runtime depsfastapi,pydantic,a2wsgi,python-multipart;httpxadded to the dev group for testing.src/djehuty/route_groups.py: route-group registry; resolves a path to new orlegacy. Registers only
api-docs(always_new).src/djehuty/application.py: umbrella FastAPI app (create_app); future grouprouters mount here.
src/djehuty/dispatch.py:WebServiceDispatcher+build_wsgi_app; falls back tolegacy-only if the new stack cannot be imported; never imports
djehuty.web.wsgi.src/djehuty/web/config/runtime.py:web_servicedefault andweb_service_groupsoverrides.src/djehuty/web/ui.py: parses theweb-serviceblock (JSON and XML, flat andobject form,
api-servicealias); both entry points — the internal server and theuWSGI
application— now serve the dispatcher.etc/djehuty/*.json|.xml: document theweb-serviceblock in the example configs.tests/unit/: six new files covering the registry, umbrella app, dispatcher,config parsing, the uWSGI entry point, and an isolation test asserting no new-stack
module imports
wsgi.py.Approval Checklist
Issue Reference (optional - PRs may not be associated with an issue)
Closes #255
Screenshots (optional)
N/A — a screenshot of
/api/docscould be added to show the always-on API reference.Notes (optional)
web-serviceconfig is optional: with no block,defaultisnew, and sinceunregistered paths always resolve to legacy, behaviour is unchanged.
contract tests), each independently toggleable between new and legacy.
Replaces #242 (base retargeted from
new-httptomainafter the API contract suite merged; GitHub's native-stacking lock prevented editing the base in place, so this is a fresh PR with the same branch).