mirror of
https://gitcode.com/JianFeeeee/ModelRouter.git
synced 2026-11-07 09:47:23 +00:00
fix(discovery): 裸 host 也必须有回退候选
补 image_endpoint 那一轮的测试暴露了一个覆盖缺口：候选构造是
if !hasVersion { add(raw, "/v1/models", 0) }
else { add(raw, "/models", 0) }
裸 host（https://api.vendor.com）因此只生成**一个**候选，没有任何回退。
catalog 挂在裸 /models 上的厂商会被报成不可达。
为什么一直没发现：所有测试用的 base 都以 /v1 结尾。生产里唯一已配
kind:image 的 siliconflow（base_url https://api.siliconflow.cn）它的
/v1/models 恰好能应答——**一个能工作的巧合把这个洞盖住了**。
现在两个 catalog 路径无条件都试（偏好只决定「多个都成功时报告哪个」，
不决定「试哪些」）。同时修正优先级：base 已带版本段时第一候选必须是
相对的 /models，否则会先试 …/v1/v1/models——一个必然 404 的 URL，白花
一个来回，还让错误信息以无意义的 URL 开头。
判据同步改了：原本断言 len(cands)==2，那是**在测量当前的列表而不是
覆盖性质**，去重修一次漂一次。现在断言「该覆盖的 URL 都在」
（base+"/v1/models" 与 base+"/models"），实现怎么变都不会漂。
新增 discover_endpoint_test.go：
- 裸 base 不带版本段时 image_endpoint 必须留空，否则会写坏今天正常工作的
siliconflow 配置
- 裸 host + catalog 在 /v1 响应时 endpoint 必须是 /v1/chat/completions
（这正是我第一次把测试期望写反的地方：catalog 路径是上游要不要 /v1 的
唯一证据）
- 两种 base 形态下最终拼出的 chat/image URL 逐字断言，且都不含 /v1/v1
This commit is contained in:
root
JianFeeeee
@ -187,21 +187,31 @@ func discoveryCandidates(raw string) []discoveryCandidate {
|
||||
stripped := trimVersionSuffix(raw)
|
||||
hasVersion := stripped != raw
|
||||
|
||||
// Preference 0: the URL exactly as typed, catalog at the canonical path.
|
||||
// When the typed base already ends in "/v1" this is "…/v1/v1/models", which
|
||||
// is precisely the doubled form that 404s — so the canonical path is only
|
||||
// offered against a base that does not already carry a version.
|
||||
if !hasVersion {
|
||||
add(raw, discoveryModelsPath, 0)
|
||||
} else {
|
||||
// Base already has "/v1"; ask for the bare "/models" relative to it.
|
||||
add(raw, "/models", 0)
|
||||
}
|
||||
// Preference 1: host root + canonical versioned path.
|
||||
add(stripped, discoveryModelsPath, 1)
|
||||
// Preference 2: host root + bare path (vendors that imply the version).
|
||||
// Both catalog paths are always tried, whichever form the operator typed.
|
||||
//
|
||||
// The earlier version offered "/models" only when the base already carried a
|
||||
// version segment. That left a bare host ("https://api.vendor.com") with a
|
||||
// SINGLE candidate and therefore no fallback at all — so a vendor serving
|
||||
// its catalog at the bare "/models" was reported as unreachable. Every
|
||||
// test used a "/v1"-suffixed base and so never saw it; siliconflow's
|
||||
// "/v1/models" happens to answer, which is why the gap survived review.
|
||||
//
|
||||
// Preference only decides WHICH URL is reported when several work, never
|
||||
// which ones are attempted.
|
||||
//
|
||||
// Order matters: a base that already ends in a version segment must be
|
||||
// paired with the RELATIVE "/models" first, because pairing it with
|
||||
// "/v1/models" produces "…/v1/v1/models" — a URL that is 404 by construction.
|
||||
// Trying it first would spend a round trip on a certain miss and, worse,
|
||||
// make the reported "tried" list lead with nonsense.
|
||||
if hasVersion {
|
||||
add(raw, "/models", 0)
|
||||
add(raw, discoveryModelsPath, 1) // the doubled form; kept only as a last resort
|
||||
add(stripped, "/models", 2)
|
||||
add(stripped, discoveryModelsPath, 3)
|
||||
} else {
|
||||
add(raw, discoveryModelsPath, 0)
|
||||
add(raw, "/models", 1)
|
||||
}
|
||||
|
||||
// Stable insertion sort by preference; add() above is not ordered.
|
||||
|
||||
153
internal/gateway/discover_endpoint_test.go
Normal file
153
internal/gateway/discover_endpoint_test.go
Normal file
@ -0,0 +1,153 @@
|
||||
package gateway
|
||||
|
||||
import (
|
||||
"context"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestComposeImageEndpoint_NoVersionBaseLeavesDefault is the counterpart of the
|
||||
// /v1 case: when the base carries no version segment, discovery must emit "" so
|
||||
// the gateway's own default "/v1/images/generations" composes.
|
||||
//
|
||||
// This is the shape every currently-working source uses — siliconflow, the only
|
||||
// source in production with kind:image, has base_url
|
||||
// "https://api.siliconflow.cn" — so writing a non-empty value here would break
|
||||
// exactly the configuration that is known to work today.
|
||||
func TestComposeImageEndpoint_NoVersionBaseLeavesDefault(t *testing.T) {
|
||||
if got := composeImageEndpoint("https://api.siliconflow.cn"); got != "" {
|
||||
t.Errorf("composeImageEndpoint = %q, want \"\" so the built-in default composes", got)
|
||||
}
|
||||
// And the composition it implies must be the vendor's real path.
|
||||
base := "https://api.siliconflow.cn"
|
||||
composed := base + "/v1/images/generations"
|
||||
if !strings.HasSuffix(composed, "/v1/images/generations") {
|
||||
t.Errorf("composed URL %q lost the image path", composed)
|
||||
}
|
||||
}
|
||||
|
||||
// TestDiscovery_BareHostNeedsVersionOnEndpointWhenCatalogIsVersioned
|
||||
// distinguishes two cases that look identical from the outside.
|
||||
//
|
||||
// bare host + catalog at "/models" → chat is "/chat/completions" (the
|
||||
// adapter default, no endpoint written)
|
||||
// bare host + catalog at "/v1/models" → chat is "/v1/chat/completions",
|
||||
// so an endpoint MUST be written
|
||||
//
|
||||
// The catalog path is the only evidence of whether the upstream wants the
|
||||
// version segment, and getting this backwards sends every chat request to a
|
||||
// path the vendor does not serve.
|
||||
func TestDiscovery_BareHostNeedsVersionOnEndpointWhenCatalogIsVersioned(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
catalogPath string
|
||||
wantEndpoint string
|
||||
wantImage string
|
||||
}{
|
||||
{
|
||||
name: "catalog at the bare /models needs no endpoint",
|
||||
catalogPath: "/models",
|
||||
wantEndpoint: "",
|
||||
wantImage: "",
|
||||
},
|
||||
{
|
||||
name: "catalog at /v1/models requires /v1 on the chat endpoint",
|
||||
catalogPath: "/v1/models",
|
||||
wantEndpoint: "/v1/chat/completions",
|
||||
wantImage: "",
|
||||
},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path != tc.catalogPath {
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
return
|
||||
}
|
||||
_, _ = w.Write([]byte(`{"data":[{"id":"m1"},{"id":"m2"}]}`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
res, err := DiscoverModels(context.Background(), srv.URL, "k")
|
||||
if err != nil {
|
||||
t.Fatalf("discovery failed: %v", err)
|
||||
}
|
||||
if res.BaseURL != srv.URL {
|
||||
t.Errorf("base = %q, want %q", res.BaseURL, srv.URL)
|
||||
}
|
||||
if res.Endpoint != tc.wantEndpoint {
|
||||
t.Errorf("endpoint = %q, want %q", res.Endpoint, tc.wantEndpoint)
|
||||
}
|
||||
if res.ImageEndpoint != tc.wantImage {
|
||||
t.Errorf("image_endpoint = %q, want %q", res.ImageEndpoint, tc.wantImage)
|
||||
}
|
||||
// Whatever it wrote, the chat URL must reach the same version
|
||||
// prefix the catalog was served under.
|
||||
ep := res.Endpoint
|
||||
if ep == "" {
|
||||
ep = "/chat/completions"
|
||||
}
|
||||
if got := res.BaseURL + ep; strings.Contains(got, "/v1/v1") {
|
||||
t.Errorf("chat URL %q has a doubled version", got)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestComposeEndpoints_BothFormsProduceWorkingURLs is the end-to-end
|
||||
// invariant across the two shapes, stated as the URLs the gateway will build.
|
||||
//
|
||||
// A base with a version and no image_endpoint produces "/v1/v1/…" — the exact
|
||||
// 404 observed before composeImageEndpoint existed. A base without one and a
|
||||
// non-empty image_endpoint produces a path missing the version. Both are
|
||||
// silent, expensive mistakes, so they are asserted literally.
|
||||
func TestComposeEndpoints_BothFormsProduceWorkingURLs(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
base string
|
||||
wantChat string
|
||||
wantImage string
|
||||
}{
|
||||
{
|
||||
name: "bare host: adapter and image defaults compose",
|
||||
base: "https://h",
|
||||
wantChat: "https://h/chat/completions",
|
||||
// openai's adapter endpoint is /chat/completions (verified in
|
||||
// internal/lua/adapters/openai.lua: adapter.endpoint).
|
||||
wantImage: "https://h/v1/images/generations",
|
||||
},
|
||||
{
|
||||
name: "versioned base: the version moves onto both endpoints",
|
||||
base: "https://h/v1",
|
||||
wantChat: "https://h/v1/chat/completions",
|
||||
wantImage: "https://h/v1/images/generations",
|
||||
},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
ep := composeEndpoint("/models", tc.base, tc.base)
|
||||
if ep == "" {
|
||||
ep = "/chat/completions" // the adapter default
|
||||
}
|
||||
iep := composeImageEndpoint(tc.base)
|
||||
if iep == "" {
|
||||
iep = "/v1/images/generations" // the gateway default
|
||||
}
|
||||
gotChat := tc.base + ep
|
||||
gotImage := tc.base + iep
|
||||
if gotChat != tc.wantChat {
|
||||
t.Errorf("chat URL = %q, want %q", gotChat, tc.wantChat)
|
||||
}
|
||||
if gotImage != tc.wantImage {
|
||||
t.Errorf("image URL = %q, want %q", gotImage, tc.wantImage)
|
||||
}
|
||||
for _, u := range []string{gotChat, gotImage} {
|
||||
if strings.Contains(u, "/v1/v1") {
|
||||
t.Errorf("%q has a doubled version segment", u)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@ -310,9 +310,6 @@ func TestDiscoveryCandidates_DedupesAndPrefersTyped(t *testing.T) {
|
||||
if len(cands) == 0 {
|
||||
t.Fatal("no candidates")
|
||||
}
|
||||
if cands[0].modelsPath != "/models" || cands[0].baseURL != "https://h/v1" {
|
||||
t.Errorf("first candidate = %+v, want the typed base with a bare /models path", cands[0])
|
||||
}
|
||||
seen := map[string]bool{}
|
||||
for _, c := range cands {
|
||||
// Dedup is on the requested URL, so that is the key to assert on.
|
||||
@ -322,17 +319,22 @@ func TestDiscoveryCandidates_DedupesAndPrefersTyped(t *testing.T) {
|
||||
}
|
||||
seen[k] = true
|
||||
}
|
||||
// The versioned spelling of the typed URL must be reachable: on the real
|
||||
// amr-link endpoint that is what answers, and it is the preferred candidate.
|
||||
if !seen["https://h/v1/models"] {
|
||||
t.Error("the typed base with a bare /models path must be tried; it is the first candidate and the one that answers on the real amr-link endpoint")
|
||||
t.Error("the typed base with a versioned catalog path must be tried; that is what answers on amr-link")
|
||||
}
|
||||
// Both catalog forms must be attempted for a bare host too. Offering only
|
||||
// "/v1/models" there left a vendor serving a bare "/models" looking
|
||||
// unreachable.
|
||||
if !seen["https://h/models"] {
|
||||
t.Error("host root with a bare /models path was never tried; vendors that imply the version would be missed")
|
||||
t.Error("host root with a bare /models path was never tried")
|
||||
}
|
||||
// The two candidate PAIRS that both spell "https://h/v1/models" collapse to
|
||||
// one request. That is the point of deduping on the final URL: listing the
|
||||
// same address twice in an error message reads as two attempts.
|
||||
if seen["https://h|v1/models"] && seen["https://h/models"] {
|
||||
t.Error("both spellings of the versioned URL are present; one is redundant")
|
||||
// The first candidate is preferred, so its URL is what gets reported when
|
||||
// several answer.
|
||||
if cands[0].baseURL+cands[0].modelsPath != "https://h/v1/models" {
|
||||
t.Errorf("first candidate requests %q, want the canonical versioned form",
|
||||
cands[0].baseURL+cands[0].modelsPath)
|
||||
}
|
||||
// The bare "/models" form must exist too — some gateways serve it there.
|
||||
hasBare := false
|
||||
|
||||
Reference in New Issue
Block a user