server/apiinternal: use generated DRPC gateway routes#168665
Conversation
|
😎 Merged successfully - details. |
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
|
Remove
|
|
shubhamdhama
force-pushed
the
auto-generate-httproute-info
branch
from
1bde011 to
604e7a5
Compare
| apiutil.WriteHTTPError(ctx, w, req, err) | ||
| return | ||
| } | ||
| if vars := mux.Vars(req); len(vars) > 0 { |
There was a problem hiding this comment.
This constraint seems to be new in this PR. What changed?
There was a problem hiding this comment.
It's simply guarding against calling decodePathVars when we don't need to call it to avoid allocating a map. It can be moved within decodePathVars or avoided completely but overall not a behavior change
There was a problem hiding this comment.
I've removed it btw as it was unnecessary optimization.
| return status.Errorf(codes.InvalidArgument, "failed to decode request body: %v", err) | ||
| reqType := fnType.In(1).Elem() | ||
| return func(w http.ResponseWriter, req *http.Request) { | ||
| newReq := reflect.New(reqType) |
There was a problem hiding this comment.
Can you add comments around the reflection calls so the code will be easy to follow/read?
Replace the manually maintained HTTP route tables (~100 entries across three files) with auto-generated route data from `protoc-gen-go-drpc`. The generator reads `google.api.http` proto annotations and emits `DRPC<Service>GatewayRoutes` functions that return `[]drpc.HTTPRoute`, so adding a new HTTP-annotated RPC no longer requires a corresponding change in `apiinternal`. This also simplifies the package: the three satellite files (`api_internal_status.go`, `api_internal_admin.go`, `api_internal_ts.go`) and the per-service struct fields are no longer needed. Route registration now happens inline in the constructor from generated data. Fixes: cockroachdb#153047 Epic: none Release note: None Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shubhamdhama
force-pushed
the
auto-generate-httproute-info
branch
from
604e7a5 to
8e71487
Compare
cthumuluru-crdb
left a comment
cthumuluru-crdb
left a comment
There was a problem hiding this comment.
I'm assuming existing tests are sufficient since this is a refactor. If not, please add additional tests. Otherwise the PR looks good to me.
|
TFTR! /trunk merge |
|
Detected infrastructure failure on trunk-merge branch (matched: self-hosted runner lost communication with the server). Automatically resubmitting to merge queue (attempt 1 of 2). (run link) |
|
/trunk merge |
3 participants
Replace the manually maintained HTTP route tables (~100 entries across
three files) with auto-generated route data from
protoc-gen-go-drpc.The generator reads
google.api.httpproto annotations and emitsDRPC<Service>GatewayRoutesfunctions that return[]drpc.HTTPRoute,so adding a new HTTP-annotated RPC no longer requires a corresponding
change in
apiinternal.This also simplifies the package: the three satellite files
(
api_internal_status.go,api_internal_admin.go,api_internal_ts.go)and the per-service struct fields are no longer needed. Route registration
now happens inline in the constructor from generated data.
Fixes: #153047
Epic: none
Release note: None