feat: migrate to Connect-RPC; new wire path /kv.v2.KvService/#119
Open
feat: migrate to Connect-RPC; new wire path /kv.v2.KvService/#119
Conversation
Plugin now exposes the generated kvV2connect.KvService handler instead of the legacy net/rpc + goridge codec methods. RPC() returns (path, http.Handler) so the rpc plugin's HTTP/2 mux mounts it directly at /kv.v2.KvService/. - plugin.go: RPC() any -> RPC() (string, http.Handler); import alias kvV2connect (matches proto go_package) - rpc.go: 7 methods adapted to connect.Request/Response shapes; caller ctx now flows into the OTEL span (was context.Background()); empty Storage -> CodeInvalidArgument; missing storage -> CodeNotFound; internal errors wrap errors.E(op, err) with CodeInternal - lookupStorage helper extracted to DRY the 7 identical empty/missing storage guards (was 14 lines x 7 = 98 lines of validation noise) - tests: shared h2c Connect client; goridge codec dropped - deps: api-go v6.0.0-beta.5, rpc/v6 v6.0.0-beta.4, connect v1.19.2, golang.org/x/net for h2c; goridge no longer imported in tests - lint: drop dead settings.dupl block
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR migrates the KV plugin's RPC interface from legacy ChangesConnectRPC v2 Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Apply Go 1.25 wg.Go() across the 4 test setups in kv_plugin_test.go, dropping the parallel wg.Add(1) / defer wg.Done() pairs. No behavior change; goroutine count remains the same.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #119 +/- ##
===========================================
+ Coverage 34.83% 51.04% +16.21%
===========================================
Files 3 3
Lines 267 239 -28
===========================================
+ Hits 93 122 +29
+ Misses 160 95 -65
- Partials 14 22 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- lookupStorage: switch to fmt.Errorf("%w: %s", errNoSuchStore, name)
so callers can stderr.Is(err, errNoSuchStore) — the previous
errors.Errorf("%s: %s", sentinel.Error(), name) embedded only the
message text and broke the Is-chain.
- TTL handler: clamp negative time.Until(t) durations to 0 before
emitting durationpb. An RFC3339 timestamp that has already elapsed
was previously sent as a negative duration; clients would see
nonsensical TTLs. (Carried-forward from PR #107 review comment;
natural to fix while rewriting this block.)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/kv_plugin_test.go (1)
340-359: ⚡ Quick winAdd regression tests for the new Connect error contract.
This only verifies the happy path, but this PR also changes the public error semantics in a breaking way. Please add cases for empty storage and unknown storage so
CodeInvalidArgumentandCodeNotFoundstay pinned by tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/kv_plugin_test.go` around lines 340 - 359, Add regression tests alongside kvSetTest/kvHasTest that exercise the new Connect error contract: call newKvClient().Set and .Has with an empty Storage string (using connect.NewRequest(&kvV2.KvRequest{Storage: "", Items: ...})) and assert the returned error’s code is connect.CodeInvalidArgument, and call Set/Has with a non-existent storage name like "unknown" and assert the returned error’s code is connect.CodeNotFound; use connect.CodeOf(err) (or the project’s helper to extract the Connect code) to compare against connect.CodeInvalidArgument and connect.CodeNotFound so the tests pin the public error semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rpc.go`:
- Around line 57-61: The storage handlers (e.g., st.Has) currently convert
context.Canceled and context.DeadlineExceeded into connect.CodeInternal; change
error mapping so context.Canceled -> connect.CodeCanceled and
context.DeadlineExceeded -> connect.CodeDeadlineExceeded by using a small helper
(e.g., mapContextErrorToConnectError(err) that returns the proper connect.Error)
instead of always wrapping as CodeInternal, and apply this helper in Has as well
as Set, MGet, MExpire, TTL, Delete, and Clear handlers to preserve
cancel/timeout semantics over the wire.
---
Nitpick comments:
In `@tests/kv_plugin_test.go`:
- Around line 340-359: Add regression tests alongside kvSetTest/kvHasTest that
exercise the new Connect error contract: call newKvClient().Set and .Has with an
empty Storage string (using connect.NewRequest(&kvV2.KvRequest{Storage: "",
Items: ...})) and assert the returned error’s code is
connect.CodeInvalidArgument, and call Set/Has with a non-existent storage name
like "unknown" and assert the returned error’s code is connect.CodeNotFound; use
connect.CodeOf(err) (or the project’s helper to extract the Connect code) to
compare against connect.CodeInvalidArgument and connect.CodeNotFound so the
tests pin the public error semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6739478-efbf-4612-b3be-6754b789ad43
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.golangci.ymlgo.modplugin.gorpc.gotests/go.modtests/kv_plugin_test.go
💤 Files with no reviewable changes (1)
- .golangci.yml
There was a problem hiding this comment.
Pull request overview
This PR migrates the KV plugin’s RPC surface from the legacy goridge net/rpc codec to Connect-RPC over HTTP/2, aligning the plugin with the Connect-migrated rpc/v6 plugin and the new /kv.v2.KvService/ wire path.
Changes:
- Switch plugin RPC registration to a Connect-generated HTTP handler and update
rpc.gomethod signatures to the Connect handler interface. - Update KV integration tests to use
connectrpc.com/connectwith an h2c-capable HTTP/2 client instead of goridge/netrpc. - Bump
api-goand related dependencies; clean up.golangci.ymlby removing unusedduplsettings.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
plugin.go |
Exposes the KV service as a Connect HTTP handler via the generated kvV2connect handler. |
rpc.go |
Reworks KV RPC methods to Connect request/response types, propagating caller context and mapping errors to Connect codes. |
tests/kv_plugin_test.go |
Migrates tests from goridge net/rpc to a Connect client using an h2c http2.Transport. |
go.mod |
Updates core dependency on api-go to v6.0.0-beta.5 (but currently missing a required Connect dependency). |
go.sum |
Updates sums for bumped/added transitive dependencies. |
tests/go.mod |
Adds Connect + h2c dependencies and bumps api-go/rpc for the test module. |
tests/go.sum |
Adds sums for new Connect/h2c-related dependencies used by tests. |
.golangci.yml |
Removes a dead dupl settings block. |
Comments suppressed due to low confidence (1)
go.mod:14
rpc.gonow importsconnectrpc.com/connect, but the root modulego.mod/go.sumdon’t include that module. In CI, this commonly fails withno required module provides package connectrpc.com/connect(especially when using-mod=readonly). Rungo mod tidy(at the repo root) or add the missing requirement so the main module’s dependency graph includes Connect.
require (
github.com/roadrunner-server/api-go/v6 v6.0.0-beta.5
github.com/roadrunner-server/api-plugins/v6 v6.0.0-beta.2
github.com/roadrunner-server/endure/v2 v2.6.2
github.com/roadrunner-server/errors v1.5.0
go.opentelemetry.io/otel/sdk v1.43.0
google.golang.org/protobuf v1.36.11
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- rpc struct: {storages, *TracerProvider} -> {pl *Plugin, tracer
trace.Tracer}. The tracer is now resolved once in Plugin.RPC() via
p.tracer.Tracer(tracerName), so each handler entry skips OTEL's
process-wide tracer-cache mutex/map lookup.
- storages access goes through r.pl.storages; future Plugin fields
flow through without touching rpc.
- keysOf(items) helper replaces 4 identical 4-line key-extraction
loops in Has/MGet/TTL/Delete. Each loop also avoided calling
msg.GetItems() twice per iteration.
Pre-migration the spans started from context.Background(), so storage errors were always real internal failures. Post-migration the request ctx flows through; when a client cancels or hits its deadline mid-call, storage backends return context.Canceled / context.DeadlineExceeded. Wrapping those as CodeInternal told clients "server bug" when in fact the truth is "your request was cancelled/timed out". internalErr() inspects the storage error and picks the right code: - context.Canceled -> CodeCanceled - context.DeadlineExceeded -> CodeDeadlineExceeded - otherwise -> CodeInternal Applied across all 8 internal-error sites in Has/Set/MGet/MExpire/TTL/ Delete/Clear. Also: tests/kv_plugin_test.go h2c Dialer gets a 30s Timeout so the suite can't hang on a TCP connect stall if the rpc server is down. Request body remains unbounded. Resolves coderabbitai + Copilot threads on PR #119.
Per user preference: Connect handler bodies pull each field directly from req.Msg, no local alias. Keeps the call site self-documenting.
New tests/kv_api_test.go (with a minimal in-memory config) verifies that the same kv handler mounted on the rpc plugin serves both wire formats: - TestKVHTTPApi: Connect-RPC over h2c, the protocol PHP clients use. - TestKVGRPCApi: regular gRPC (google.golang.org/grpc) on the same port. Both run a Set / Has / Delete / Has cycle and assert state changes end-to-end. The shared startKvAPIContainer helper brings up rpc + kv + memory + logger, so the suite stays Docker-free.
The kv handler is mounted once on the rpc plugin's mux but Connect-RPC hands it three wire protocols out of the box. This commit adds one test per protocol against the same in-memory storage, all running a Set / Has / Delete / Has cycle: - TestKVConnectAPI: Connect over h2c via connectrpc.com/connect. - TestKVHTTPApi: plain HTTP/1.1 POST with a protojson body and Content-Type: application/json. This is the shape PHP clients use (no Connect SDK; Guzzle/curl + json_encode). - TestKVGRPCApi: regular gRPC via google.golang.org/grpc + the gRPC-generated client. Used by PHP's gRPC extension. Helper startKvAPIContainer brings up rpc + kv + memory + logger.
api-go v6.0.0-beta.6 (tagged off api PR #71) re-generates the KvServiceHandler with WithIdempotency(IdempotencyNoSideEffects) on Has / MGet / TTL. Connect now accepts HTTP GET on those, encoding the request body in query params. - Bump api-go: v6.0.0-beta.5 -> v6.0.0-beta.6 in root + tests modules. - Replace the temporary TestKVHTTPGetRejected probe with a table-driven TestKVHTTPGetIdempotency that asserts: GET Has, MGet, TTL -> 200 OK GET Set, MExpire, Delete, Clear -> 405 Method Not Allowed Provides regression coverage for the proto idempotency annotations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
plugin.go:RPC() any→RPC() (string, http.Handler)returningkvV2connect.NewKvServiceHandler. The rpc plugin's HTTP/2 mux mounts the handler at/kv.v2.KvService/.rpc.go: 7 methods (Has,Set,MGet,MExpire,TTL,Delete,Clear) adapted to the generated Connect handler interface.ctxinstead ofcontext.Background(), so client cancellation/deadlines propagate into the storage operations.Storage→CodeInvalidArgument; unknown storage →CodeNotFound; storage errors wraperrors.E(op, err)withCodeInternal.lookupStoragehelper DRYs out the 7 identical "no storage / no such storage" guard blocks.connectrpc.com/connectwith anhttp2.Transporth2c client at127.0.0.1:6001; the goridge codec is no longer imported.connectrpc.com/connectv1.19.2,golang.org/x/netfor h2c..golangci.yml: removed deadsettings.duplblock (dupl wasn't in the enabled linters list).Breaking changes
goridgeRpc) cannot reach the kv plugin once a roadrunner build with this change is deployed./kv.v2.KvService/{Method}instead ofkv.{Method}.storageand unknown-storage errors previously surfaced as plain string errors; now returnCodeInvalidArgument/CodeNotFoundrespectively.Summary by CodeRabbit
Refactor
Chores