From e178c81bb4b9b8737e32897e8874897b04cceb86 Mon Sep 17 00:00:00 2001 From: Hunnyboy1217 <110440428+hunnyboy1217@users.noreply.github.com> Date: Wed, 17 Jun 2026 03:47:27 -0700 Subject: [PATCH] refactor(go-models): harden Ollama ListModels and route through ParseListModel (#15853) (#15955) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What problem does this PR solve? Part of #15853 (provider model-list refactor). Refactors **Ollama** `ListModels` onto the shared `ParseListModel` pattern and fixes two correctness issues: - **Endpoint:** switch the models suffix from `api/ps` (only currently-running models) to `api/tags` (all installed models) — the latter is what a model picker should show. - **Parsing:** Ollama returns `{"models":[{"name","model"}]}`, a non-OpenAI shape. Decode it into a typed struct, map the names into `ModelList`, then enrich through `ParseListModel`. This removes the previous unchecked type assertions (`result["models"].([]interface{})` / `.(map[string]interface{})` / `.(string)`) that **panicked** when the body was missing the `models` array or any field, and adds a fallback to the `model` field when `name` is blank. - Drops the no-op GET request body and a dead base-URL reassignment. #### Drive-by fix Shared gitee_test.go `DSModelList` -> `ModelList` compile fix (renamed in #15900) so the models test package builds; auto-resolves against the sibling #15853 PRs. ### Type of change - [x] Bug Fix (non-breaking change which fixes an issue) - [x] Refactoring --- internal/entity/models/ollama.go | 49 ++++++++------- internal/entity/models/ollama_test.go | 91 +++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 23 deletions(-) create mode 100644 internal/entity/models/ollama_test.go diff --git a/internal/entity/models/ollama.go b/internal/entity/models/ollama.go index 86bc80b91a..1facb4e5b2 100644 --- a/internal/entity/models/ollama.go +++ b/internal/entity/models/ollama.go @@ -460,30 +460,20 @@ func (o *OllamaModel) ParseFile(modelName *string, content []byte, url *string, func (o *OllamaModel) ListModels(apiConfig *APIConfig) ([]ListModelResponse, error) { - resolvedBaseURL, err := o.baseModel.GetBaseURL(apiConfig) + baseURL, err := o.baseModel.GetBaseURL(apiConfig) if err != nil { return nil, err } - baseURL := resolvedBaseURL if baseURL == "" { - baseURL = resolvedBaseURL - } - if baseURL == "" { - return nil, fmt.Errorf("missing base URL: please configure the local access address for Ollama (e.g., http://127.0.0.1:11434/v1)") + return nil, fmt.Errorf("missing base URL: please configure the local access address for Ollama (e.g., http://127.0.0.1:11434)") } - url := fmt.Sprintf("%s/%s", baseURL, o.baseModel.URLSuffix.Models) - reqBody := map[string]interface{}{} - - jsonData, err := json.Marshal(reqBody) - if err != nil { - return nil, fmt.Errorf("failed to marshal request: %w", err) - } + url := fmt.Sprintf("%s/%s", strings.TrimSuffix(baseURL, "/"), o.baseModel.URLSuffix.Models) ctx, cancel := context.WithTimeout(context.Background(), nonStreamCallTimeout) defer cancel() - req, err := http.NewRequestWithContext(ctx, "GET", url, bytes.NewBuffer(jsonData)) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return nil, fmt.Errorf("failed to create request: %w", err) } @@ -505,21 +495,34 @@ func (o *OllamaModel) ListModels(apiConfig *APIConfig) ([]ListModelResponse, err return nil, fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, string(body)) } - // Parse response - var result map[string]interface{} + // Ollama's /api/tags returns {"models":[{"name":...,"model":...}]}, a shape + // that differs from the OpenAI list. Decode it into a local struct, map the + // names into ModelList, then enrich through the shared ParseListModel helper + // (issue #15853). Using a typed struct also avoids the previous unchecked + // type assertions, which panicked when "models" was absent or malformed. + var result struct { + Models []struct { + Name string `json:"name"` + Model string `json:"model"` + } `json:"models"` + } if err = json.Unmarshal(body, &result); err != nil { return nil, fmt.Errorf("failed to parse response: %w", err) } - // convert result["data"] to []map[string]interface{} - models := make([]ListModelResponse, 0) - for _, model := range result["models"].([]interface{}) { - modelMap := model.(map[string]interface{}) - modelName := modelMap["name"].(string) - models = append(models, ListModelResponse{Name: modelName}) + modelList := ModelList{Object: "list"} + for _, m := range result.Models { + name := strings.TrimSpace(m.Name) + if name == "" { + name = strings.TrimSpace(m.Model) + } + if name == "" { + continue + } + modelList.Models = append(modelList.Models, DSModel{ID: name}) } - return models, nil + return ParseListModel(modelList), nil } func (o *OllamaModel) Balance(apiConfig *APIConfig) (map[string]interface{}, error) { diff --git a/internal/entity/models/ollama_test.go b/internal/entity/models/ollama_test.go new file mode 100644 index 0000000000..cdfc2066de --- /dev/null +++ b/internal/entity/models/ollama_test.go @@ -0,0 +1,91 @@ +// +// Copyright 2026 The InfiniFlow Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +package models + +import ( + "io" + "net/http" + "net/http/httptest" + "testing" +) + +func newOllamaForListModelsTest(baseURL string) *OllamaModel { + return NewOllamaModel(map[string]string{"default": baseURL}, URLSuffix{Models: "api/tags"}) +} + +func TestOllamaListModels(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + t.Errorf("method=%s, want GET", r.Method) + } + if r.URL.Path != "/api/tags" { + t.Errorf("path=%s, want /api/tags", r.URL.Path) + } + // Ollama's /api/tags response shape (name + model fields). + _, _ = io.WriteString(w, `{"models":[{"name":"llama3:latest","model":"llama3:latest"},{"name":"qwen3:8b","model":"qwen3:8b"}]}`) + })) + defer srv.Close() + + models, err := newOllamaForListModelsTest(srv.URL).ListModels(&APIConfig{}) + if err != nil { + t.Fatalf("ListModels: %v", err) + } + if len(models) != 2 { + t.Fatalf("len(models)=%d, want 2", len(models)) + } + if models[0].Name != "llama3:latest" || models[1].Name != "qwen3:8b" { + t.Fatalf("names=%v, want [llama3:latest qwen3:8b]", []string{models[0].Name, models[1].Name}) + } +} + +func TestOllamaListModelsFallsBackToModelField(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Some entries may carry only the "model" field; it should be used as the name. + _, _ = io.WriteString(w, `{"models":[{"model":"phi3:mini"},{"name":""},{"name":" "}]}`) + })) + defer srv.Close() + + models, err := newOllamaForListModelsTest(srv.URL).ListModels(&APIConfig{}) + if err != nil { + t.Fatalf("ListModels: %v", err) + } + if len(models) != 1 { + t.Fatalf("len(models)=%d, want 1 (blank names skipped)", len(models)) + } + if models[0].Name != "phi3:mini" { + t.Fatalf("Name=%q, want phi3:mini", models[0].Name) + } +} + +func TestOllamaListModelsRejectsHTTPError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = io.WriteString(w, `boom`) + })) + defer srv.Close() + + if _, err := newOllamaForListModelsTest(srv.URL).ListModels(&APIConfig{}); err == nil { + t.Fatal("ListModels: expected error for HTTP 500, got nil") + } +} + +func TestOllamaListModelsRequiresBaseURL(t *testing.T) { + m := NewOllamaModel(map[string]string{}, URLSuffix{Models: "api/tags"}) + if _, err := m.ListModels(&APIConfig{}); err == nil { + t.Fatal("ListModels: expected error for missing base URL, got nil") + } +}