upstream handler support for model names with forward slash #298
Conversation
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
WalkthroughUpdated the upstream routing from a per-model path parameter to a single wildcard route. The proxy now parses the incoming path to resolve a model alias to a real model name, rewrites the remaining path, swaps to the appropriate process group, and forwards the request using the resolved model. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant PM as ProxyManager
participant CFG as Config
participant PG as ProcessGroup
participant U as Upstream
C->>PM: HTTP /upstream/{...path}
PM->>PM: Parse path segments, find alias prefix
alt alias found
PM->>CFG: RealModelName(alias)
CFG-->>PM: realModelName
PM->>PM: remainingPath = path after alias
PM->>PM: Rewrite request path to remainingPath
PM->>PM: swapProcessGroup(alias)
alt swap ok
PM->>PG: ProxyRequest(realModelName, writer, request)
PG->>U: Forward with rewritten path
U-->>PG: Response
PG-->>PM: Response
PM-->>C: Response
else swap error
PM-->>C: 500 Internal Server Error
end
else no alias found
PM-->>C: 400 Bad Request
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
The upstream handler would break on model IDs that contained a forward slash. Model IDs like "aaa/bbb" called at upstream/aaa/bbb would result in an error. This commit adds support for model IDs with a forward slash by iteratively searching the path for a match. Updates: #229
mostlygeek
force-pushed
the
upstream-forward-slash
branch
from
d5b079b to
6a6c6c7
Compare
![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 (4)
proxy/proxymanager.go (4)
230-230: Hook metrics middleware into /upstream route/upstream traffic should be metered like the other endpoints.
- pm.ginEngine.Any("/upstream/*upstreamPath", pm.proxyToUpstream) + pm.ginEngine.Any("/upstream/*upstreamPath", mm, pm.proxyToUpstream)
396-404: Early‑exit guard; current length check is ineffectivestrings.Split("", "/") returns [""] so len(parts) is never 0. Guard on an empty or root path instead.
- // split the upstream path by / and search for the model name - parts := strings.Split(strings.TrimSpace(upstreamPath), "/") - if len(parts) == 0 { - pm.sendErrorResponse(c, http.StatusBadRequest, "model id required in path") - return - } + // split the upstream path by / and search for the model name + upstreamPath = strings.TrimSpace(upstreamPath) + if upstreamPath == "" || upstreamPath == "/" { + pm.sendErrorResponse(c, http.StatusBadRequest, "model id required in path") + return + } + parts := strings.Split(upstreamPath, "/")
439-441: Handle ProxyRequest error consistentlyOther handlers return 500 on ProxyRequest errors; do the same here.
// rewrite the path c.Request.URL.Path = remainingPath - processGroup.ProxyRequest(realModelName, c.Writer, c.Request) + if err := processGroup.ProxyRequest(realModelName, c.Writer, c.Request); err != nil { + pm.sendErrorResponse(c, http.StatusInternalServerError, fmt.Sprintf("error proxying request: %s", err.Error())) + pm.proxyLogger.Errorf("Error Proxying Request for processGroup %s and model %s", processGroup.id, realModelName) + return + }
395-441: Add tests for slash-containing and overlapping model aliasesPlease add upstream tests to cover:
- Model alias with a slash: /upstream/openai/gpt-4o-mini/v1/chat/completions
- Overlapping aliases: ensure "aaa/bbb" wins over "aaa" (longest-prefix behavior)
Example test (adjust fixtures as needed):
t.Run("model alias with slash", func(t *testing.T) { cfg := fmt.Sprintf(` logLevel: error models: model1: cmd: %s -port ${PORT} -silent -respond model1 aliases: ["openai/gpt-4o-mini","aaa","aaa/bbb"] `, getSimpleResponderPath()) config, err := LoadConfigFromReader(strings.NewReader(cfg)) assert.NoError(t, err) proxy := New(config) defer proxy.StopProcesses(StopWaitForInflightRequest) // 1) slash alias req := httptest.NewRequest("GET", "/upstream/openai/gpt-4o-mini/v1/chat/completions", nil) rec := httptest.NewRecorder() proxy.ServeHTTP(rec, req) assert.Equal(t, http.StatusOK, rec.Code) assert.Equal(t, "model1", rec.Body.String()) // 2) longest-prefix: choose "aaa/bbb" not "aaa" req = httptest.NewRequest("GET", "/upstream/aaa/bbb/v1/chat/completions", nil) rec = httptest.NewRecorder() proxy.ServeHTTP(rec, req) assert.Equal(t, http.StatusOK, rec.Code) assert.Equal(t, "model1", rec.Body.String()) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy/proxymanager.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
proxy/proxymanager.go (3)
proxy/proxymanager_test.go (1)
TestProxyManager_Upstream(660-689)proxy/processgroup.go (1)
pg(55-83)proxy/processgroup_test.go (1)
TestProcessGroup_ProxyRequestSwapIsTrueParallel(50-91)
Comment on lines
+408
to
+425
| for i, part := range parts { | ||
| if parts[i] == "" { | ||
| continue | ||
| } | ||
|
|
||
| if searchModelName == "" { | ||
| searchModelName = part | ||
| } else { | ||
| searchModelName = searchModelName + "/" + parts[i] | ||
| } | ||
|
|
||
| if real, ok := pm.config.RealModelName(searchModelName); ok { | ||
| modelName = real | ||
| remainingPath = "/" + strings.Join(parts[i+1:], "/") | ||
| modelFound = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Prefer longest prefix match for overlapping aliases (e.g., "aaa" vs "aaa/bbb")
Breaking on the first match can misroute requests intended for a longer alias (especially with “%2F” encoded slashes decoded by Gin). Keep scanning and use the last (longest) match.
- for i, part := range parts {
- if parts[i] == "" {
+ for i, part := range parts {
+ if part == "" {
continue
}
if searchModelName == "" {
- searchModelName = part
+ searchModelName = part
} else {
- searchModelName = searchModelName + "/" + parts[i]
+ searchModelName += "/" + part
}
if real, ok := pm.config.RealModelName(searchModelName); ok {
modelName = real
remainingPath = "/" + strings.Join(parts[i+1:], "/")
modelFound = true
- break
+ // keep scanning to prefer the longest matching model alias
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for i, part := range parts { | |
| if parts[i] == "" { | |
| continue | |
| } | |
| if searchModelName == "" { | |
| searchModelName = part | |
| } else { | |
| searchModelName = searchModelName + "/" + parts[i] | |
| } | |
| if real, ok := pm.config.RealModelName(searchModelName); ok { | |
| modelName = real | |
| remainingPath = "/" + strings.Join(parts[i+1:], "/") | |
| modelFound = true | |
| break | |
| } | |
| } | |
| for i, part := range parts { | |
| if part == "" { | |
| continue | |
| } | |
| if searchModelName == "" { | |
| searchModelName = part | |
| } else { | |
| searchModelName += "/" + part | |
| } | |
| if real, ok := pm.config.RealModelName(searchModelName); ok { | |
| modelName = real | |
| remainingPath = "/" + strings.Join(parts[i+1:], "/") | |
| modelFound = true | |
| // keep scanning to prefer the longest matching model alias | |
| } | |
| } |
🤖 Prompt for AI Agents
In proxy/proxymanager.go around lines 408-425, the loop currently breaks on the
first alias match which can pick a shorter alias (e.g., "aaa") over a longer
overlapping alias ("aaa/bbb"); instead, do not break on match — record the
matched real model name and the index where it was found (or compute
remainingPath from that index) and continue scanning all parts so the final
stored match will be the longest prefix; after the loop, use the last recorded
match to set modelName, remainingPath and modelFound (or leave them unchanged if
no match).
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.
The upstream handler would break on model IDs that contained a forward slash. Model IDs like "aaa/bbb" called at upstream/aaa/bbb would result in an error. This commit adds support for model IDs with a forward slash by iteratively searching the path for a match.
Summary by CodeRabbit