fix(providers): address review comments in anthropic-messages provider
- fix normalizeBaseURL edge case that incorrectly appends /v1 to URLs already containing /v1 path (e.g., https://api.example.com/v1/proxy) - remove dead code for apiBase empty check as normalizeBaseURL() always provides a default value - update test to use proper constructor instead of direct struct initialization - add detailed comments explaining the URL normalization logic Resolves review comments on PR #1284
This commit is contained in:
parent
4e52511704
commit
ddfa437f9c
2 changed files with 19 additions and 23 deletions
|
|
@ -73,10 +73,6 @@ func (p *Provider) Chat(
|
||||||
model string,
|
model string,
|
||||||
options map[string]any,
|
options map[string]any,
|
||||||
) (*LLMResponse, error) {
|
) (*LLMResponse, error) {
|
||||||
if p.apiBase == "" {
|
|
||||||
return nil, fmt.Errorf("API base not configured")
|
|
||||||
}
|
|
||||||
|
|
||||||
if p.apiKey == "" {
|
if p.apiKey == "" {
|
||||||
return nil, fmt.Errorf("API key not configured")
|
return nil, fmt.Errorf("API key not configured")
|
||||||
}
|
}
|
||||||
|
|
@ -323,19 +319,30 @@ func parseResponseBody(body []byte) (*LLMResponse, error) {
|
||||||
}
|
}
|
||||||
|
|
||||||
// normalizeBaseURL ensures the base URL is properly formatted.
|
// normalizeBaseURL ensures the base URL is properly formatted.
|
||||||
|
// It removes /v1 suffix if present (to avoid duplication) and always appends /v1.
|
||||||
|
// This handles edge cases like "https://api.example.com/v1/proxy" correctly.
|
||||||
func normalizeBaseURL(apiBase string) string {
|
func normalizeBaseURL(apiBase string) string {
|
||||||
base := strings.TrimSpace(apiBase)
|
base := strings.TrimSpace(apiBase)
|
||||||
if base == "" {
|
if base == "" {
|
||||||
return defaultBaseURL
|
return defaultBaseURL
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Remove trailing slashes
|
||||||
base = strings.TrimRight(base, "/")
|
base = strings.TrimRight(base, "/")
|
||||||
// Add /v1 if not present
|
|
||||||
if !strings.HasSuffix(base, "/v1") {
|
// Remove /v1 suffix if present (will be re-added)
|
||||||
base = base + "/v1"
|
// This prevents duplication for URLs like "https://api.example.com/v1/proxy"
|
||||||
|
if before, ok := strings.CutSuffix(base, "/v1"); ok {
|
||||||
|
base = before
|
||||||
}
|
}
|
||||||
|
|
||||||
return base
|
// Ensure we don't have an empty string after cutting
|
||||||
|
if base == "" {
|
||||||
|
return defaultBaseURL
|
||||||
|
}
|
||||||
|
|
||||||
|
// Add /v1 suffix (required by Anthropic Messages API)
|
||||||
|
return base + "/v1"
|
||||||
}
|
}
|
||||||
|
|
||||||
// Helper functions for type conversion
|
// Helper functions for type conversion
|
||||||
|
|
|
||||||
|
|
@ -413,26 +413,18 @@ func TestGetDefaultModel(t *testing.T) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Mock HTTP server test for integration testing
|
// TestProviderChatErrors tests error handling in Chat.
|
||||||
|
// Note: apiBase check removed as it's dead code - normalizeBaseURL() always provides a default.
|
||||||
func TestProviderChatErrors(t *testing.T) {
|
func TestProviderChatErrors(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
apiKey string
|
apiKey string
|
||||||
apiBase string
|
|
||||||
messages []Message
|
messages []Message
|
||||||
wantErrMsg string
|
wantErrMsg string
|
||||||
}{
|
}{
|
||||||
{
|
|
||||||
name: "missing API base",
|
|
||||||
apiKey: "test-key",
|
|
||||||
apiBase: "",
|
|
||||||
messages: []Message{{Role: "user", Content: "Test"}},
|
|
||||||
wantErrMsg: "API base not configured",
|
|
||||||
},
|
|
||||||
{
|
{
|
||||||
name: "missing API key",
|
name: "missing API key",
|
||||||
apiKey: "",
|
apiKey: "",
|
||||||
apiBase: "https://api.example.com",
|
|
||||||
messages: []Message{{Role: "user", Content: "Test"}},
|
messages: []Message{{Role: "user", Content: "Test"}},
|
||||||
wantErrMsg: "API key not configured",
|
wantErrMsg: "API key not configured",
|
||||||
},
|
},
|
||||||
|
|
@ -440,11 +432,8 @@ func TestProviderChatErrors(t *testing.T) {
|
||||||
|
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
t.Run(tt.name, func(t *testing.T) {
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
// Create provider with empty apiBase to trigger error
|
// Create provider using constructor to ensure proper initialization
|
||||||
provider := &Provider{
|
provider := NewProvider(tt.apiKey, "https://api.example.com")
|
||||||
apiKey: tt.apiKey,
|
|
||||||
apiBase: tt.apiBase,
|
|
||||||
}
|
|
||||||
|
|
||||||
_, err := provider.Chat(context.Background(), tt.messages, nil, "test-model", nil)
|
_, err := provider.Chat(context.Background(), tt.messages, nil, "test-model", nil)
|
||||||
if err == nil {
|
if err == nil {
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue