Add plugin route overrides, and drop the platform pdk re-exports - #2961
Add plugin route overrides, and drop the platform pdk re-exports#2961malinthaprasan wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds deferred core route registration through a shared router interface. It adds plugin route overrides, response capture helpers, startup validation, external-plugin forwarding, and updates handler registration signatures. ChangesCore routing and plugin route overrides
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@platform-api/pdk/override.go`:
- Around line 139-160: Update WriteCaptured to clear each destination header key
before adding its captured values, ensuring captured headers replace existing
upstream values rather than append duplicates. Preserve the existing
Content-Length exclusion and status/body handling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 047bb071-e076-4a83-819e-fe588316c3f2
📒 Files selected for processing (31)
platform-api/internal/handler/api.goplatform-api/internal/handler/api_deployment.goplatform-api/internal/handler/api_key.goplatform-api/internal/handler/apikey_user.goplatform-api/internal/handler/application.goplatform-api/internal/handler/auth_login.goplatform-api/internal/handler/gateway.goplatform-api/internal/handler/gateway_internal.goplatform-api/internal/handler/llm.goplatform-api/internal/handler/llm_apikey.goplatform-api/internal/handler/llm_deployment.goplatform-api/internal/handler/llm_proxy_apikey.goplatform-api/internal/handler/mcp.goplatform-api/internal/handler/mcp_deployment.goplatform-api/internal/handler/organization.goplatform-api/internal/handler/project.goplatform-api/internal/handler/secret.goplatform-api/internal/handler/subscription_handler.goplatform-api/internal/handler/subscription_plan_handler.goplatform-api/internal/handler/websocket.goplatform-api/internal/plugin/plugin.goplatform-api/internal/router/router.goplatform-api/internal/router/router_test.goplatform-api/internal/server/external_plugin.goplatform-api/internal/server/overrides.goplatform-api/internal/server/overrides_test.goplatform-api/internal/server/plugins.goplatform-api/internal/server/server.goplatform-api/pdk/override.goplatform-api/pdk/override_test.goplatform-api/platform/override.go
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
WriteCaptured merged the captured headers into the real ResponseWriter with Header.Add, which appends to any value already present under the same key. A decorator that set a header before passing core's response through therefore emitted both values -- two Content-Type lines for a field RFC 9110 defines as a singleton. The existing test could not see it, because Header.Get returns only the first value. Delete each captured key before adding its values, so a captured header replaces what the decorator set instead of stacking on top of it. Set-Cookie is excepted: multiple cookies are legitimate, and a blanket delete would discard the decorator's own. Headers under keys core never touched are untouched, since Del on an absent key is a no-op.
aeb66ac to
de748fe
Compare
Purpose
A wrapper embedding platform-api as a library can add routes and chain middleware, but cannot change what an existing core endpoint returns. The only options are forking the handler or shadowing the route, and
ServeMuxpanics on a duplicate pattern.#2831
Goals
Let a plugin decorate one existing core route: its
Wrapreceives the original core handler asnext.Approach
Six commits, each buildable on its own:
internal/router— aRouterinterface (*http.ServeMuxsatisfies it) and aRecorderthat records registrations instead of serving them. Core handlers'RegisterRoutesnow takerouter.Router; thepdk.Plugin/plugin.Plugincontracts keep*http.ServeMux, so no plugin or wrapper changes.pdk/override.go—RouteOverride,RouteOverrideProvider, and the capture helpersInvoke/WriteCaptured.installCoreRoutesvalidates them and installs each recorded route on the mux, wrapped where claimed.platformre-exports nothing frompdk. See Breaking changes in the PDK below.RouteDecorator— a defined type offunc(http.Handler) http.Handlerto provide the handler funtion to override the default handler.Constraints:
Breaking changes in the PDK
platformno longer re-exports anything frompdk. It exports onlyNew,App,Run, and theWith*options; every contract type has one name, inpdk.Seven symbols are removed, all previously released on
main:platform.Pluginpdk.Pluginplatform.Depspdk.Depsplatform.Middlewarepdk.Middlewareplatform.ChainPositionpdk.ChainPositionplatform.PositionedMiddlewarepdk.PositionedMiddlewareplatform.BeforePlatformChainpdk.BeforePlatformChainplatform.AfterPlatformChainpdk.AfterPlatformChainUser stories
Documentation
N/A — extension surface only, no user-facing API change. The contract is documented in the doc comments on
pdk.RouteOverride,Invoke, andWriteCaptured.Automation tests
internal/router100.0% statement coverage,pdk96.8%,installCoreRoutes100.0%. Cover ordering and deferred errors in the recorder, capture/write semantics, and every startup-failure path (unknown pattern, duplicate claims, nilWrap, empty pattern, plugin/core collision). Two assert the header-replacement fix onHeader().Values()rather thanGet()—Getreturns only the first value, so it cannot see a duplicate. Two more pinRouteDecoratoras a type distinct fromMiddleware, while confirming a plain func literal still assigns toWrap.405preserved, and a bad pattern refusing startup.Security checks
go vetclean.An override cannot widen access. Required scopes are keyed by OpenAPI path/method and are untouched, so a decorated route keeps the requirement it had. A decorator cannot re-route a request either — the handler for the pattern is already selected before
Wrapruns, so rewriting the path changes nothing about what executes. The contract documents that a decorator must read the organization from request context, never from request input. Every malformed or unmatched override aborts startup rather than being skipped.Samples
Related PRs
None.
Test environment
Go 1.26.5, macOS.
go build ./...,go build ./cmd/main.go,go vet, and the test suite all clean; each of the first three commits was checked out and built independently. The reference wrapper (apip-cloud-platform-api) was also smoke-built against this branch through its.localsymlinks —go build,go vet,go testclean — since nothing in this repo compiles againstpdkfrom outside the module.