Repository navigation
feat(openapi): let the adapter run on the vendored httpserver (epic #101, workstream 4, slice 2) - #128
Conversation
Adds backend selection to OpenAPIAdapter and the CLI: toolregistry-server openapi --config tools.yaml --backend httpserver fastapi stays the default while workstream 5 ports the TestClient suite; httpserver runs the app from #127 with no uvicorn, and --reload is reported as unsupported there instead of being silently dropped. create_and_run now accepts backend= and forwards it, so embedding callers can opt in the same way.
ty inferred App | FastAPI from the two constructor branches and then rejected uvicorn.run(self._app, ...) on the union. Annotate the attribute as Any; run() already narrows on self._backend.
|
CI's lint job caught a typing regression the local hook cannot see: ty inferred |
There was a problem hiding this comment.
The wiring is correct and I verified it the only way that proves the slice: in a venv with no fastapi and no uvicorn, installed with pip install -e . --no-deps (core 0.19.0 only).
$ toolregistry-server openapi --config examples/config.yaml --backend httpserver --port 8811
/tools 200, 3 tools, ETag "0"
/openapi.json 200, 5 paths (/events first), ETag "0"
POST tool 200
--tokens f 401 no header -> 200 with "Bearer sekrit" -> 401 wrong token
log "Starting OpenAPI server" ... and no uvicorn anywhere
That is the point of the flag, and it holds: the zero-dependency backend really does run with zero of those dependencies, including auth and ETag.
The default did not quietly switch either - same environment, no --backend:
ERROR | Server dependencies not installed: FastAPI is required for OpenAPI support.
Install with: pip install toolregistry-server[openapi]
Also checked: --reload on httpserver logs WARNING | --reload is a uvicorn feature; it is ignored on the httpserver backend and keeps serving; an unknown backend raises ValueError: Unknown OpenAPI backend 'gunicorn'; choose from ('fastapi', 'httpserver') from the adapter and exits via argparse from the CLI; --help lists --backend {fastapi,httpserver} with the default in the text. Suite: 407 passed, 2 skipped (401 + your 5 + the one #127 added mid-flight).
One parity leftover worth deciding on
The two backends now differ in whitespace on the two meta routes, though not in content:
/tools fastapi 1032 bytes httpserver 1070 bytes json_equal=True
/openapi.json fastapi 4020 bytes httpserver 4289 bytes json_equal=True
fast: {"tools":[{"name":"examples-add",...
http: {"tools": [{"name": "examples-add", ...
#127 gave tool results compact separators for byte parity, but /tools and /openapi.json go through the vendored JSONResponse, which uses json.dumps defaults. Nothing breaks - clients parse JSON - but the spec is 7% larger on the wire and a byte-level diff between backends will keep surprising whoever compares them next. Either give that JSONResponse the same separators=(",", ":") / ensure_ascii=False treatment or write the constraint down as deliberate.
Two inline notes, neither blocking: the untested run() dispatch (plus the untested --reload warning and the unpinned "never imports fastapi" promise - three tests that fit in one class), and the -> Any on the app property together with the silently dropped run() kwargs.
Approving - small diff, correct wiring, and the flag does exactly what it claims in an environment that cannot fall back to FastAPI.
| @@ -183,7 +215,19 @@ def run(self, *, host: str = "0.0.0.0", port: int = 8000, **kwargs) -> None: | |||
| host: Host to bind to. | |||
| port: Port to bind to. | |||
| reload: Enable auto-reload for development (default: False). | |||
There was a problem hiding this comment.
This branch has no test, and it is the one that matters. The five new tests stop at adapter construction and argparse; if run() lost this if and always went to uvicorn, all five would still pass while the actual promise of the slice - serving with no fastapi/uvicorn installed - would be broken.
Cheapest pin:
def test_run_dispatches_to_the_selected_backend(self, route_table, monkeypatch):
import uvicorn
from toolregistry_server._vendor.httpserver import App
calls = []
monkeypatch.setattr(App, "run", lambda self, **kw: calls.append(("httpserver", kw)))
monkeypatch.setattr(uvicorn, "run", lambda *a, **kw: calls.append(("uvicorn", kw)))
OpenAPIAdapter(route_table, backend="httpserver").run(port=1234)
OpenAPIAdapter(route_table).run(port=1234)
assert [name for name, _ in calls] == ["httpserver", "uvicorn"]The same App.run patch covers the --reload warning, which is also untested today: keep the lambda, wrap it with a structlog capture, and assert the warning fired for the httpserver call and not for the uvicorn one.
One more that only this backend can be wrong about: nothing asserts that the httpserver path never imports fastapi. monkeypatch.setitem(sys.modules, "fastapi", None) around OpenAPIAdapter(route_table, backend="httpserver") would pin the zero-dependency claim and would catch a future module-scope import.
| @@ -172,8 +204,8 @@ def __init__( | |||
| ) | |||
There was a problem hiding this comment.
Two small consistency points this property exposes.
-> Anyis a typing regression from-> FastAPI: callers ofadapter.applose every check, and the two backends do share a shape (run(host=..., port=...)).FastAPI | AppunderTYPE_CHECKING, or a tiny Protocol withrun, keeps it honest without importing fastapi at runtime.run()on the httpserver backend passes onlyhost/portand warns solely aboutreload. Anything else a caller passes today (log_level, ssl options) is silently dropped, where uvicorn would have honoured it - the same treatment asreload(warn and ignore) would make the backend swap loud instead of quiet.
There was a problem hiding this comment.
Approved (re-checked at 90dffce; the only delta from 91d0b21f is self._app: Any, and the suite, the mutation below and the CLI end-to-end all reproduce there). I ran both backends end to end through the CLI, not just the unit tests, and the flag does what it says. One test gap I would fix before merge, plus the current state of the #127 parity list.
End to end, through the CLI
Real tools.yaml, toolregistry-server openapi --config tools.yaml --backend <b>:
fastapi httpserver
GET /tools 200 200 (same JSON)
GET /openapi.json 200 200 (same document)
GET /docs 200 200
POST /tools/calc/evaluate "42" "42"
GET /nope 404 detail 404 detail
--tokens, no header 401 401 + WWW-Authenticate on both
--tokens, wrong token 401 401
--tokens, right token 200 200
--backend httpserver --reload logs the warning once and still serves:
WARNING | --reload is a uvicorn feature; it is ignored on the httpserver backend
--backend gunicorn exits in argparse; OpenAPIAdapter(backend="gunicorn") raises ValueError. type(adapter.app) is _vendor.httpserver.App for httpserver and FastAPI otherwise, and the response headers differ exactly as expected (no server: uvicorn).
Suite locally: 407 passed, 2 skipped (the PR says 401/8; those are environment skips).
Also: the WWW-Authenticate gap I flagged on #127 is already fixed on this base (9b1eafc), as are the 404/405 envelope and compact tool bodies. Good.
The one thing I would add before merge
The flag has exactly one behaviour - choose which server runs - and no test covers it. Mutation: in run(), if self._backend == "httpserver": -> if False:
407 passed, 2 skipped
The suite stays green while --backend httpserver silently serves uvicorn. test_httpserver_backend_builds_the_vendored_app only checks the app object; nothing checks that run() calls App.run(). One test with App.run and uvicorn.run patched, asserting which was called for each backend, closes it - and it is the assertion that makes this PR worth having.
Remaining parity gaps, now reachable by users
Re-ran the side-by-side matrix on this base. Still divergent:
- Malformed JSON: fastapi adds
input: {}andctx: {"error": "Expecting property name ..."}; httpserver has neither ({type: json_invalid, loc: [body, 1], msg: "JSON decode error"}). - Empty body: fastapi
missing/Field requiredwithinput: null; httpserverjson_invalid/JSON decode error. "No body" and "broken JSON" still collapse into one code here. Content-Type: text/plainwith a JSON body: 422 on fastapi, 200 and the tool runs on httpserver.- Not byte-identical after all.
2b7e85esays tool bodies match byte for byte, and they do;/toolsdoes not:208vs222bytes (spaced separators), same for 404/405 ({"detail": "Not Found"}vs{"detail":"Not Found"}). Cosmetic unless someone diffs bodies, but the commit message reads as global.
None of these block an opt-in flag whose default is unchanged. They should be closed or documented before workstream 5 flips the default, because after that they are the contract.
Docs
--backend appears nowhere in docs_en (ea640a4) or docs_zh (3aaa20c) - those branches are still on 0.6.0. Since this is the first user-visible switch, one paragraph saying "experimental, not the default, known differences: X, Y" would stop someone pointing production at it blind. Not a blocker for the code.
Minor: on the httpserver backend run() drops any kwarg other than reload without comment, and App.run() calls loop.add_signal_handler, so it raises if run() is ever called from a non-main thread (that is why the tests patch it). Worth a docstring line if either ever bites.
There was a problem hiding this comment.
Wiring verified end to end on a real CLI process, but the head is not mergeable: the lint job is red and it is a typing error in the new backend branch, not a flake.
error[unresolved-attribute]: Attribute `run` is not defined on `FastAPI` in union `App | FastAPI`
--> src/toolregistry_server/adapters/openapi/__init__.py:225:13
error[invalid-argument-type]: Argument to function `run` is incorrect
test (3.10–3.13), test-mcp-v1 and GitGuardian all pass; build is skipped behind the failing job. _app is annotated as the union, and each branch calls the runtime-specific method (App.run for one, uvicorn.run(self._app, ...) for the other), which ty refuses to narrow on self._backend. assert isinstance(self._app, App) in the httpserver branch and assert isinstance(self._app, FastAPI) before uvicorn.run would make it pass, as would keeping two attributes. Nothing functional is wrong — this is only the type checker.
What I ran, for the record, since the new tests only assert types and argparse and never start a server:
toolregistry-server openapi --config tools.yaml --backend httpserver --port 18099
Registered 3 tool(s) /tools 200, /openapi.json 200, /docs 200, /nope 404
POST add {2,3} -> 5 [200] (same under fastapi)
POST make_point {1,2} -> {"x":1,"y":2} [200] <- the #127 serialization fix holds through the CLI
POST make_points {4} -> [{"x":4,"y":5}] [200]
POST add [] -> 422 model_attributes_type, same detail as fastapi
GET /tools If-None-Match -> 304 GET /openapi.json If-None-Match -> 304
GET /nope -> {"detail": "Not Found"}
Auth through the CLI on the new backend: Authentication enabled with 2 token(s), no header 401, Bearer wrong 401, Bearer tok-B 200, tool POST without a token 401 {"detail": "Not authenticated"}. So tokens_path → tokens does reach create_httpserver_app — that seam is untested by the new tests and is the one place a mistake would fail silently (auth off, not an exception). One socket-level test on create_and_run(backend="httpserver", tokens_path=...) would pin it.
Also checked: default backend still runs uvicorn (server: uvicorn, Uvicorn running on ...); --backend httpserver --reload logs --reload is a uvicorn feature; it is ignored on the httpserver backend and then serves normally; --backend bogus is rejected by argparse with the choices list. Suite 407 passed / 2 skipped here (your 401 — my environment has extra optional deps installed).
Three non-blocking notes:
- The compact-JSON commit covers tool bodies but not the rest:
/toolsis 672 bytes on fastapi vs 710 here,/openapi.json3341 vs 3610, and 422 envelopes are{"detail":[...]}vs{"detail": [...]}. Parsed-equal, byte-different. If workstream 5 asserts byte equality anywhere, those three will fail. - The httpserver backend sends no
Serverheader where uvicorn sendsserver: uvicorn— an improvement, just a difference to know about. run()only looks atreloadon the httpserver branch and silently ignores every other kwarg, so a future--workersor similar would be swallowed rather than rejected.
Fixing the typing error is all I need to approve.
Milo asked for a socket-level test of the one wiring a mistake would fail silently on: create_and_run(backend="httpserver", tokens_path=...) must leave authentication switched on. The test captures the app through a patched run(), serves it, and asserts 401 without a token and 200 with.
|
The lint error you hit was fixed in 90dffce, pushed before your review landed — Your point about the seam is right, so I added the socket-level test you asked for in c8ba1c9: Thanks for the CLI walkthrough too — it matches what I measured, including the serialisation fix holding through the real process. |
|
Correction: the seam test commit is 730434e (I typo'd the hash in the previous comment). |
There was a problem hiding this comment.
Re-checked at 730434e — lint is green (8/8 including build), and the seam test is the one I was missing.
Confirmed the test is load-bearing rather than decorative: I replaced tokens=tokens with tokens=None in the httpserver branch of create_httpserver_app(...) and test_tokens_path_reaches_the_httpserver_backend fails, so it pins the wiring rather than just exercising it. It also serves the captured app on a real socket instead of inspecting app._tokens, which is the right level for this seam.
90dffce is fine as a fix. self._app: Any = None does silence ty rather than model the two types, so a future self._app.something_typo() would type-check, but both branches are exercised over sockets now and two parallel attributes would be more churn than either backend is worth at this stage.
Suite: 408 passed / 2 skipped here. My earlier request is cleared; approaching from the functional side I found nothing else.
Wires #127's app into the adapter and the CLI behind an opt-in flag, so the zero-dependency backend can be exercised end to end before it becomes the default.
OpenAPIAdapter(..., backend="fastapi"|"httpserver")—fastapiremains the default while workstream 5 ports theTestClientsuite;httpserverbuilds the app from feat(openapi): serve the routes from the vendored httpserver (epic #101, workstream 4, slice 1) #127 andrun()callsApp.run()instead of uvicorn.--backendin theopenapisubcommand, forwarded throughApp.serve_openapi→create_and_run.--reloadis a uvicorn feature: on thehttpserverbackend it logs a warning instead of being silently ignored.ValueErrorfrom the adapter and is rejected by argparse in the CLI.Tests: default/opt-in backend, wrong backend,
--reloadhandling, CLI parsing (default, explicit, invalid) and the dispatch assertion update. Full suite 401 passed, 8 skipped.Not part of this PR: making
httpserverthe default, retiring uvicorn, and porting the FastAPI-specific tests (workstreams 4-final and 5).