Jelajahi Sumber

docs: add connectrpc-removal exploration notes

Federico Feliziani 2 bulan lalu
induk
melakukan
0ba7b1d922
1 mengubah file dengan 182 tambahan dan 0 penghapusan
  1. 182 0
      docs/no-pyqwest.md

+ 182 - 0
docs/no-pyqwest.md

@@ -0,0 +1,182 @@
+# Replacing `connectrpc` (and its `pyqwest` dependency) with a hand-rolled Connect client
+
+Exploration notes for `chattolib-zo8b`. Goal: determine whether chattolib can
+drop the `connectrpc` package — and with it the transitive `pyqwest` — by
+implementing the small slice of the Connect protocol it actually uses.
+
+## TL;DR recommendation
+
+**Feasible, and small.** Everything chattolib needs from `connectrpc` reduces
+to one async method, `execute_unary`, plus a trivial binary codec and a JSON
+error-envelope parser. The whole thing is ~150 lines. The real cost is not the
+runtime — it's **code generation**: chattolib's 24 `*_connect.py` stubs are
+emitted by `protoc-gen-connect-python`, which is coupled to `connectrpc`'s
+types. We either (a) keep the stubs but repoint their base class, or (b) write
+a tiny codegen that emits only the async client.
+
+## What chattolib actually uses from connectrpc
+
+Of the 8 `connectrpc` submodules the generated stubs import
+(`client`, `code`, `compression`, `errors`, `interceptor`, `method`,
+`request`, `server`), chattolib's runtime path touches only:
+
+| connectrpc piece | Used by chattolib? | Notes |
+|---|---|---|
+| `client.ConnectClient` (async) | **Yes** — 23 `*ServiceClient` classes subclass it | Only the async client. The `*ClientSync`, `*ASGIApplication`, `*WSGIApplication`, and `Protocol` classes in each stub are **dead weight** chattolib never imports. |
+| `client.execute_unary(...)` | **Yes** — the one method every stub calls | POSTs to `/{service}/{Method}`, encodes with the codec, decodes the response, maps errors. |
+| `compat.google_protobuf_binary_codec` | **Yes** | Trivial: `encode = SerializeToString`, `decode = cls.FromString`. ~10 lines. |
+| `errors.ConnectError` | **Yes** | Chattolib wraps it into `ChattoConnectError`. |
+| `code.Code` | **Yes** | The gRPC/Connect status-code enum. |
+| `method.MethodInfo` / `IdempotencyLevel` | **Yes** (data only) | Just carries `name`, `service_name`, input/output classes. |
+| `request.Headers` / `RequestContext` | Indirectly | Chattolib passes `headers=` (an auth bearer) into each call. |
+| `compression`, `interceptor`, `server.*` | **No** | Chattolib never compresses, never intercepts, never serves. |
+
+So the *entire* runtime surface is: **one async unary call + a binary codec +
+an error parser.**
+
+## The wire protocol (what `execute_unary` does)
+
+From `connectrpc/_client_async.py::_send_request_unary`:
+
+1. `url = f"{address}/{service_name}/{Method}"` (e.g.
+   `/api/connect/chatto.api.v1.RoomService/JoinRoom`)
+2. `body = codec.encode(request)` → `request.SerializeToString()`
+3. `POST url` with headers (chattolib adds `Authorization: Bearer <token>`),
+   `Content-Type: application/proto`
+4. On `200`: `return codec.decode(resp.content, output_cls)` →
+   `output_cls.FromString(body)`
+5. On non-200: parse the JSON error envelope
+   `{"code": "...", "message": "...", "details": [...]}` into a
+   `ConnectError(code, message, details)`; fall back to an HTTP-status→code map
+   if the body isn't JSON.
+6. Timeouts → `DEADLINE_EXCEEDED`; network errors → `UNAVAILABLE`.
+
+That's the whole protocol for unary Connect. (Chatto uses unary-only; there are
+no client/server/bidi streams in the Chatto API surface chattolib wraps.)
+
+## The two real decisions
+
+### 1. The codec is a non-issue
+`ProtoBinaryCodec` is `SerializeToString`/`FromString`. Trivial to copy.
+
+### 2. Code generation is the actual work
+The 24 stubs are generated by `protoc-gen-connect-python` and import
+`connectrpc.{client,code,compression,errors,interceptor,method,request,server}`.
+Two paths:
+
+- **(a) Repoint, don't regenerate.** Keep the existing `*_connect.py` files but
+  make their base class a chattolib-owned `ConnectClient` that *also* exposes
+  `execute_unary`. The stubs' `MethodInfo(...)` calls would need our own
+  `MethodInfo`/`IdempotencyLevel` shims. Pro: no codegen change. Con: we still
+  carry 24 files that import names we now have to provide, and every future
+  proto regen re-introduces `connectrpc` imports.
+
+- **(b) Tiny custom codegen.** Write a small `protoc` plugin (or a Python
+  post-processor over the `.proto`/`_pb2` files) that emits *only* the async
+  client class, importing from `chattolib._connect`. Pro: clean, no
+  `connectrpc` anywhere, stubs shrink ~4x (drop sync/ASGI/WSGI/Protocol).
+  Con: it's a new thing to maintain and to run on every proto regen.
+
+Option (b) is the cleaner end state and matches "chattolib owns its transport."
+Option (a) is the lower-effort first step and a fine way to de-risk.
+
+## What we'd drop / keep
+
+- **Drop:** `connectrpc` (and therefore `pyqwest`) from `pyproject.toml`.
+- **Keep:** `protobuf` (the messages), `websockets` (realtime, unaffected).
+- **Net:** dependency tree goes from `connectrpc → {protobuf-py, pyqwest}` to
+  just `protobuf`. `pyqwest` (a Rust-backed HTTP client) disappears entirely.
+
+## Risks
+
+- **Correctness on the error path.** The JSON error envelope + `details`
+  (protobuf `Any`, base64) + HTTP-status fallback is the fiddly bit. Must be
+  tested against real server errors, not just happy paths.
+- **`use_get` / compression / interceptors.** Chattolib never uses them, so we
+  can simply not implement them. (Confirm no stub call passes `use_get=True`
+  in a way chattolib relies on — it doesn't; chattolib always POSTs.)
+- **Future Chatto changes.** If Chatto ever adds a streaming RPC, the
+  hand-rolled client would need a streaming path. Today: none.
+- **We own the protocol now.** No more upstream `connectrpc` fixes/patches.
+  Acceptable given how small the surface is.
+
+## Prototype plan (next step)
+
+1. `src/chattolib/_connect.py`: `Codec`, `MethodInfo`, `Code` (re-export or
+   minimal enum), `ConnectError`, and `class ConnectClient` with
+   `async execute_unary(...)` doing the 6 steps above over `httpx`.
+2. Point the 23 `*ServiceClient` base classes at it (option a) — or regenerate
+   (option b).
+3. Re-run the full test suite + a live smoke test against the preview server.
+4. If green: drop `connectrpc` from `pyproject.toml`.
+
+---
+
+## Result (spike outcome)
+
+**Feasible and done — as a working prototype in this worktree.** The
+hand-rolled client is green against the live server with `connectrpc` and
+`pyqwest` physically uninstalled.
+
+### What was built (all in this worktree, throwaway until you approve)
+
+- **`src/chattolib/_connect.py`** (~180 lines) — the whole replacement:
+  - `Code` enum (the 16 Connect/gRPC status codes)
+  - `ConnectError` (`.code`, `.message`, `.details`, `.status_code`; `str()` is
+    the message only, matching `connectrpc` so `translate_connect_error` is
+    unchanged)
+  - `MethodInfo` (name, service_name, input, output, idempotency_level)
+  - `_BinaryCodec` (`SerializeToString` / `FromString`) +
+    `google_protobuf_binary_codec()`
+  - `ConnectClient` with `async execute_unary(...)` over `httpx`:
+    POST `/{service}/{Method}`, `Content-Type: application/proto`, 200 → decode,
+    non-200 → parse the JSON error envelope (with an HTTP-status→code fallback).
+- **`scripts/regen_connect.py`** — the tiny codegen. Reads each existing
+  `*_connect.py` and rewrites it to keep **only** the async `*ServiceClient`
+  (dropping the `Protocol`/`*Sync`/`*ASGIApplication`/`*WSGIApplication`/
+  `*ClientSync` classes chattolib never used), strip all `connectrpc` imports,
+  repoint the base class + `MethodInfo`/`IdempotencyLevel` at `chattolib._connect`,
+  and trim the `timeout_ms`/`use_get` params our client doesn't take.
+- **`src/chattolib/_transport.py`** — `Code`/`ConnectError`/
+  `google_protobuf_binary_codec` now imported from `chattolib._connect`.
+- **`src/chattolib/client.py`** — the `ConnectError` import in `_rpc` repointed.
+- **`pyproject.toml`** — `connectrpc` removed from `dependencies` (now just
+  `protobuf`; `httpx` is already gone from the bot-only change).
+
+### Verification
+
+- Full suite: **64 passed, 2 skipped** (the 2 are the bot-key-gated live tests).
+- `ruff check` + `mypy` clean.
+- **Live** against `next.preview.chatto.run` with `connectrpc`/`pyqwest`
+  uninstalled: `get_server`, `me`, `list_rooms`, `post_message` (a write), and
+  the error path (`not_found`) all work.
+
+### The real trade-off (the actual recommendation)
+
+The runtime is small and the win is real: **the dependency tree loses both
+`connectrpc` and `pyqwest`** (a Rust-backed HTTP client), leaving just
+`protobuf` + `websockets`. In exchange:
+
+1. **We own the Connect protocol now.** ~180 lines, well-tested here, but no
+   more upstream patches. The surface is small (unary only) so this is
+   low-risk — *as long as Chatto stays unary-only*. If Chatto ever adds a
+   streaming RPC, this client needs a streaming path.
+2. **Codegen is a new maintenance item.** `regen_connect.py` is a
+   regex-based post-processor over the `connectrpc`-generated files, not a
+   true `protoc` plugin. It works for the current 24 stubs, but on each proto
+   regen you'd run `generate_pb.sh` (which still uses `protoc-gen-connect-python`
+   to *produce* the stubs) **then** `regen_connect.py` to strip them down. So
+   the `connectrpc` *code generator* is still in the build loop even though the
+   *runtime* is gone. A fully clean end state would replace that generator too —
+   a larger lift.
+3. **The stubs are now "DO NOT EDIT" in a new sense.** They're generated, but by
+   our script; the header says so.
+
+### Recommendation
+
+**Go, with a caveat.** The runtime replacement is clearly worth it if dropping
+`pyqwest` is the goal — it's ~180 lines, fully tested, and verified live. The
+one thing to decide before merging: do you want to (a) ship this as-is (runtime
+freedom, but `generate_pb.sh` still shells out to the connectrpc codegen), or
+(b) also write a from-scratch `protoc` plugin so the connectrpc generator is
+gone from the build entirely? (a) is this worktree; (b) is a follow-on.