address PR review: log sjson error and add unit tests

- Log a warning instead of silently ignoring sjson.SetBytes errors in the TranslateRequest fallback path
  - Add registry_test.go with tests covering the fallback model normalization and verifying registered transforms take precedence
This commit is contained in:
Longwu Ou
2026-03-18 12:43:40 -04:00
parent e1e9fc43c1
commit 1e27990561
2 changed files with 96 additions and 1 deletions

View File

@@ -4,6 +4,7 @@ import (
"context"
"sync"
log "github.com/sirupsen/logrus"
"github.com/tidwall/gjson"
"github.com/tidwall/sjson"
)
@@ -55,7 +56,9 @@ func (r *Registry) TranslateRequest(from, to Format, model string, rawJSON []byt
}
}
if model != "" && gjson.GetBytes(rawJSON, "model").String() != model {
if updated, err := sjson.SetBytes(rawJSON, "model", model); err == nil {
if updated, err := sjson.SetBytes(rawJSON, "model", model); err != nil {
log.Warnf("translator: failed to normalize model in request fallback: %v", err)
} else {
return updated
}
}

View File

@@ -0,0 +1,92 @@
package translator
import (
"testing"
"github.com/tidwall/gjson"
)
func TestTranslateRequest_FallbackNormalizesModel(t *testing.T) {
r := NewRegistry()
tests := []struct {
name string
model string
payload string
wantModel string
wantUnchanged bool
}{
{
name: "prefixed model is rewritten",
model: "gpt-5-mini",
payload: `{"model":"copilot/gpt-5-mini","input":"ping"}`,
wantModel: "gpt-5-mini",
},
{
name: "matching model is left unchanged",
model: "gpt-5-mini",
payload: `{"model":"gpt-5-mini","input":"ping"}`,
wantModel: "gpt-5-mini",
wantUnchanged: true,
},
{
name: "empty model leaves payload unchanged",
model: "",
payload: `{"model":"copilot/gpt-5-mini","input":"ping"}`,
wantModel: "copilot/gpt-5-mini",
wantUnchanged: true,
},
{
name: "deeply prefixed model is rewritten",
model: "gpt-5.3-codex",
payload: `{"model":"team/gpt-5.3-codex","stream":true}`,
wantModel: "gpt-5.3-codex",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
input := []byte(tt.payload)
got := r.TranslateRequest(Format("a"), Format("b"), tt.model, input, false)
gotModel := gjson.GetBytes(got, "model").String()
if gotModel != tt.wantModel {
t.Errorf("model = %q, want %q", gotModel, tt.wantModel)
}
if tt.wantUnchanged && string(got) != tt.payload {
t.Errorf("payload was modified when it should not have been:\ngot: %s\nwant: %s", got, tt.payload)
}
// Verify other fields are preserved.
for _, key := range []string{"input", "stream"} {
orig := gjson.Get(tt.payload, key)
if !orig.Exists() {
continue
}
after := gjson.GetBytes(got, key)
if orig.Raw != after.Raw {
t.Errorf("field %q changed: got %s, want %s", key, after.Raw, orig.Raw)
}
}
})
}
}
func TestTranslateRequest_RegisteredTransformTakesPrecedence(t *testing.T) {
r := NewRegistry()
from := Format("openai-response")
to := Format("openai-response")
r.Register(from, to, func(model string, rawJSON []byte, stream bool) []byte {
return []byte(`{"model":"from-transform"}`)
}, ResponseTransform{})
input := []byte(`{"model":"copilot/gpt-5-mini","input":"ping"}`)
got := r.TranslateRequest(from, to, "gpt-5-mini", input, false)
gotModel := gjson.GetBytes(got, "model").String()
if gotModel != "from-transform" {
t.Errorf("expected registered transform to take precedence, got model = %q", gotModel)
}
}