mirror of
https://gitcode.com/JianFeeeee/ModelRouter.git
synced 2026-10-03 23:54:06 +00:00
fix(sources): 源编辑不再清零 proxy_url / api_key_env / timeout
bf0657b 修好了 api_key,但 upsert 仍会重写整个源,于是请求无法表达的字段
一律被重置为零值。这四个字段的后果都不是"少个配置项":
- api_key_env 丢失 ⇒ 盘上无明文密钥的源变成无凭据源,写操作返回 200,
下一次调用上游才 401。而 README 恰恰把这个特性当作卖点在宣传。
- proxy_url 丢失 ⇒ 一个走代理的上游变成直连(或反之),且完全无声。
- timeout / queue_timeout 丢失 ⇒ 退回默认 120s / 60s。
触发路径不是只有脚本:WebUI 的 saveSource() 发的 payload 只含表单上的
11 个字段,而 editSource() 表单里根本没有这 4 项 ⇒ 运维在界面上改个并发数
就会静默清掉它们。
修法用「存在性」语义而不是「空即继承」:
- 不传 → 保留已存值(部分更新的客户端要的就是这个)
- 传了 → 覆盖,包括传空串表示清空
api_key 刻意保留它原有的「空即继承」规则,不跟着改成指针:该规则已随
v1.7.6 发布,脚本依赖它;而凭据丢失比代理丢失严重得多。两个字段的失败
模式相反,所以规则相反——这一条写进了两处注释。
另一处是差一点的:config.Source 把 Timeout/QueueTimeout 标成 json:"-",
所以 reveal 接口的结构体序列化**根本不返回它们**。表单读不到 → 输入框
恒空 → 而输入框每次都回传 → 每存一次就把 timeout 清零。等于把刚修好的
丢字段换个方向又造了一个。因此 reveal 分支现在显式返回 duration 字符串。
判据:
- TestWebUIEditPayloadPreservesRoutingFields 用的是 WebUI 真实 payload
的逐字节副本,并同时断言"确实改动的字段生效",否则"什么都不写"也能过
- TestSourceEditPreservesAPIKeyEnv 单独盯 api_key_env(唯一造成凭据丢失的)
- CanBeSet / CanBeCleared 分别盯两个方向:只有"空即继承"的实现过不了
CanBeCleared(清空代理框会永远保留旧代理)
- 持久化判据**重新加载 config.yaml 并按语义比对**:300s 会被重新序列化成
5m0s,按字符串匹配是假红(我自己先踩了一次)
- TestSourcePayloadCoversEveryEditableField 用反射卡住"这一类":新增
Source 字段而没接到 API 上时立刻变红。反射查结构体而非 marshal 结果,
因为指针 + omitempty 会合法地从序列化输出里消失,那正是"未提及"信号
- 三个 UI 契约判据把 JS 侧也钉住(表单必须回传、必须从 reveal 读)
变异验证(每次都先确认 build 通过,再数红格):
1. 去掉覆盖逻辑 → 7 个判据红
2. 改成"空即继承" → CanBeCleared + ClearIsScoped 红
3. reveal 不返回 duration → TestSourceRevealExposesDurations 红
4. 表单不回传 api_key_env → TestUIEditFormRoundTrips... 红
顺带修正 /api/v1 索引:DELETE /api/keys 的路径段写的是 {name},实际是 key
本身;PUT /api/keys/{key} 实现了却没列。
全量 + vet + race 全绿;WebUI 内联脚本过 node --check。
This commit is contained in:
@ -3,6 +3,7 @@ package gateway
|
||||
import (
|
||||
"encoding/csv"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"io"
|
||||
"log"
|
||||
"net/http"
|
||||
@ -83,6 +84,121 @@ type sourcePayload struct {
|
||||
// It exists because "add one model" is the most common scripted edit and a
|
||||
// full Models list cannot be written without reading the source first.
|
||||
ModelIDs []string `json:"model_ids,omitempty"`
|
||||
|
||||
// The four fields below are POINTERS, and that is the whole point.
|
||||
//
|
||||
// POST/PUT is an upsert that rewrites the whole source, so any field the
|
||||
// payload cannot express is silently reset to its zero value. That already
|
||||
// destroyed api_key once (fixed with resolveAPIKey) and would equally
|
||||
// destroy proxy_url, api_key_env, timeout and queue_timeout.
|
||||
//
|
||||
// A plain string/duration cannot distinguish "the caller did not mention
|
||||
// this field" from "the caller asked for the empty value", and only the
|
||||
// former may inherit. So:
|
||||
//
|
||||
// nil → field absent from the request: keep the current value
|
||||
// &"x" → present: store exactly "x" (including "" to clear it)
|
||||
//
|
||||
// api_key deliberately keeps its older "empty means inherit" rule rather
|
||||
// than gaining a pointer: that rule is already published (v1.7.6) and
|
||||
// scripts depend on it. Changing it now would let a script that echoes an
|
||||
// empty api_key erase a live credential, which is the exact failure this
|
||||
// whole area exists to prevent.
|
||||
ProxyURL *string `json:"proxy_url,omitempty"`
|
||||
APIKeyEnv *string `json:"api_key_env,omitempty"`
|
||||
Timeout *string `json:"timeout,omitempty"` // duration string, e.g. "300s"
|
||||
QueueTimeout *string `json:"queue_timeout,omitempty"` // duration string, e.g. "90s"
|
||||
}
|
||||
|
||||
// optionalSourceFields carries the Source fields a partial payload must not
|
||||
// reset, each as a pointer so "absent" and "explicitly empty" stay distinct.
|
||||
//
|
||||
// nil → absent from the request: keep the current value
|
||||
// &"" → present: clear it
|
||||
// &"x" → present: store "x"
|
||||
//
|
||||
// The alternative — a plain value with "empty means inherit" — cannot express
|
||||
// "clear this field", so emptying the proxy input in the UI would silently keep
|
||||
// the old proxy. It is exactly why api_key is NOT modelled this way: for a
|
||||
// credential, failing to keep the old value is worse than failing to clear it,
|
||||
// and that rule is already published (v1.7.6). Two fields with opposite failure
|
||||
// modes therefore get opposite rules, and both are spelled out here.
|
||||
type optionalSourceFields struct {
|
||||
ProxyURL *string
|
||||
APIKeyEnv *string
|
||||
Timeout *string
|
||||
QueueTimeout *string
|
||||
}
|
||||
|
||||
// resolve overlays the four optional fields onto payload.
|
||||
//
|
||||
// A nil pointer means the request did not mention the field, so it keeps the
|
||||
// value already stored in cur (nil cur = a source being created, where the
|
||||
// payload's own zero value is correct). A non-nil pointer always wins, including
|
||||
// when it points at the empty string, which is how a UI form clears a field.
|
||||
//
|
||||
// Only these four fields are overlaid. Everything else in payload is taken as
|
||||
// sent: an upsert that inherited the whole record would make it impossible to
|
||||
// change anything.
|
||||
func (o optionalSourceFields) resolve(payload config.Source, cur *config.Source) (config.Source, error) {
|
||||
// cur is dereferenced exactly once here so the create path (nil) cannot
|
||||
// panic on the per-field lookups below.
|
||||
var storedProxyURL, storedKeyEnv string
|
||||
var storedTimeout, storedQueueTimeout time.Duration
|
||||
if cur != nil {
|
||||
storedProxyURL, storedKeyEnv = cur.ProxyURL, cur.APIKeyEnv
|
||||
storedTimeout, storedQueueTimeout = cur.Timeout, cur.QueueTimeout
|
||||
}
|
||||
pick := func(p *string, stored string) string {
|
||||
if p != nil {
|
||||
return *p
|
||||
}
|
||||
return stored
|
||||
}
|
||||
payload.ProxyURL = pick(o.ProxyURL, storedProxyURL)
|
||||
payload.APIKeyEnv = pick(o.APIKeyEnv, storedKeyEnv)
|
||||
if err := overlayDuration(&payload.Timeout, o.Timeout, "timeout", storedTimeout); err != nil {
|
||||
return payload, err
|
||||
}
|
||||
if err := overlayDuration(&payload.QueueTimeout, o.QueueTimeout, "queue_timeout", storedQueueTimeout); err != nil {
|
||||
return payload, err
|
||||
}
|
||||
return payload, nil
|
||||
}
|
||||
|
||||
// overlayDuration applies the absent/present rule to one duration field: a nil
|
||||
// pointer keeps stored, a set pointer replaces it (with "" / "0s" clearing).
|
||||
func overlayDuration(dst *time.Duration, p *string, field string, stored time.Duration) error {
|
||||
if p == nil {
|
||||
*dst = stored
|
||||
return nil
|
||||
}
|
||||
d, err := parseOptionalDuration(*p, field)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
*dst = d
|
||||
return nil
|
||||
}
|
||||
|
||||
// parseOptionalDuration parses a duration written as a Go duration string.
|
||||
// An empty string clears the field back to the config default (0), which is
|
||||
// what a UI form submitting an empty timeout box should mean. Parsing happens
|
||||
// only for fields the request actually mentions, so a typo can never surface
|
||||
// as a silent reset of something else.
|
||||
func parseOptionalDuration(v, field string) (time.Duration, error) {
|
||||
v = strings.TrimSpace(v)
|
||||
if v == "" {
|
||||
return 0, nil
|
||||
}
|
||||
d, err := time.ParseDuration(v)
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("%s %q is not a duration (use e.g. 120s, 5m, 1h)", field, v)
|
||||
}
|
||||
if d < 0 {
|
||||
return 0, fmt.Errorf("%s must be >= 0 (got %s)", field, v)
|
||||
}
|
||||
return d, nil
|
||||
}
|
||||
|
||||
// keepExistingAPIKey is the mask a client sends when it means "keep the
|
||||
@ -186,6 +302,21 @@ func (g *Gateway) handleSourcesAPI(w http.ResponseWriter, r *http.Request) {
|
||||
MaxConcurrent: p.MaxConcurrent,
|
||||
RPM: p.RPM,
|
||||
}
|
||||
// The four upsert-fragile fields are applied last: a nil pointer means
|
||||
// "not in the request", so they are inherited from the stored source
|
||||
// instead of being reset to the payload's zero value. This runs whether
|
||||
// or not the request mentioned them, because resolve() itself decides
|
||||
// per field — passing them unconditionally keeps the rule in one place.
|
||||
src, err := optionalSourceFields{
|
||||
ProxyURL: p.ProxyURL,
|
||||
APIKeyEnv: p.APIKeyEnv,
|
||||
Timeout: p.Timeout,
|
||||
QueueTimeout: p.QueueTimeout,
|
||||
}.resolve(src, g.sourceByName(p.Name))
|
||||
if err != nil {
|
||||
writeError(w, http.StatusBadRequest, "source_error", err.Error())
|
||||
return
|
||||
}
|
||||
// model_ids is additive: "add these models" is the common scripted edit
|
||||
// and it must not require reading (and echoing) the whole list back.
|
||||
// A request that omits models entirely is therefore a pure add, not a
|
||||
|
||||
Reference in New Issue
Block a user