From f477d3329d1a208cb4f037e1ef34c90c6ee85d6b Mon Sep 17 00:00:00 2001 From: S <5842097+skbs-eng@users.noreply.github.com> Date: Tue, 7 Jul 2026 07:10:27 +0530 Subject: [PATCH] Fix: ValueError: too many values to unpack in list_tenant_added_models for model IDs containing '@' (#16467) (#16468) --- api/apps/services/models_api_service.py | 33 +- api/db/joint_services/tenant_model_service.py | 28 +- internal/service/model_service.go | 134 ++++++-- internal/service/model_service_test.go | 144 ++++++++ ...ls_api_service_list_tenant_added_models.py | 320 ++++++++++++++++++ ...t_tenant_model_service_split_model_name.py | 155 +++++++++ 6 files changed, 760 insertions(+), 54 deletions(-) create mode 100644 test/unit_test/api/apps/services/test_models_api_service_list_tenant_added_models.py create mode 100644 test/unit_test/api/db/services/test_tenant_model_service_split_model_name.py diff --git a/api/apps/services/models_api_service.py b/api/apps/services/models_api_service.py index 18cee06d0..8d78ec89e 100644 --- a/api/apps/services/models_api_service.py +++ b/api/apps/services/models_api_service.py @@ -13,16 +13,16 @@ # See the License for the specific language governing permissions and # limitations under the License. # -import os import logging +import os -from api.db.joint_services.tenant_model_service import ensure_mineru_from_env, ensure_paddleocr_from_env, ensure_opendataloader_from_env -from common.constants import ActiveStatusEnum, LLMType -from common.settings import FACTORY_LLM_INFOS -from api.db.services.tenant_model_provider_service import TenantModelProviderService +from api.db.joint_services.tenant_model_service import ensure_mineru_from_env, ensure_opendataloader_from_env, ensure_paddleocr_from_env from api.db.services.tenant_model_instance_service import TenantModelInstanceService +from api.db.services.tenant_model_provider_service import TenantModelProviderService from api.db.services.tenant_model_service import TenantModelService from api.db.services.user_service import TenantService +from common.constants import ActiveStatusEnum, LLMType +from common.settings import FACTORY_LLM_INFOS # Mapping from model_type string to Tenant model field name MODEL_TYPE_TO_FIELD = { @@ -70,19 +70,20 @@ def _get_model_info(tenant_id: str, default_model: str, model_type: str): if not default_model: return None - parts = default_model.split("@") + # The composite key is right-anchored: provider_name is always the *last* + # '@'-separated field. Use rsplit so a model_name that itself contains '@' + # (e.g. LM Studio IDs like `text-embedding-nomic-embed-text-v1.5@q8_0`) + # remains intact in the leftmost field instead of being truncated. + parts = default_model.rsplit("@", 2) if len(parts) == 3: model_name, instance_name, provider_name = parts elif len(parts) == 2: model_name, provider_name = parts instance_name = "default" - elif len(parts) == 1: + else: model_name = parts[0] provider_name = "" instance_name = "default" - else: - logging.warning(f"Invalid model string: {default_model}") - return None model_type = MODEL_TAG_TO_TYPE.get(model_type, model_type) # Special case: OCR with infiniflow@default@deepdoc is always enabled @@ -324,7 +325,7 @@ def list_tenant_added_models(tenant_id: str, model_type_filter: str = None): target_type_records = [record for record in model_records if record.model_type == model_type_filter] if model_type_filter else model_records model_record_map = {} for model in target_type_records: - instance_model_key = f"{model.provider_id}@{model.instance_id}@{model.model_name}" + instance_model_key = f"{model.provider_id}|{model.instance_id}|{model.model_name}" if model_record_map.get(instance_model_key): model_record_map[instance_model_key].append(model) else: @@ -346,7 +347,7 @@ def list_tenant_added_models(tenant_id: str, model_type_filter: str = None): continue for factory_instance in factory_instances: - model_record_key = f"{factory_instance.provider_id}@{factory_instance.id}@{llm['llm_name']}" + model_record_key = f"{factory_instance.provider_id}|{factory_instance.id}|{llm['llm_name']}" model_key_in_factory.append(model_record_key) manual_modified_models = model_record_map.get(model_record_key, []) active_model_types = [manual_model.model_type for manual_model in manual_modified_models if manual_model.status == ActiveStatusEnum.ACTIVE.value] @@ -374,7 +375,13 @@ def list_tenant_added_models(tenant_id: str, model_type_filter: str = None): model_records = model_record_map.get(model_record_key, []) if not model_records: continue - provider_id, instance_id, model_name = model_record_key.split("@") + # The internal key uses '|' as separator (UUID|UUID|model_name) + # since model_name may contain '@' characters. + try: + provider_id, instance_id, model_name = model_record_key.split("|", 2) + except ValueError: + logging.warning(f"Skipping malformed manual model record key: {model_record_key!r}") + continue model_types = [model.model_type for model in model_records if model.status == ActiveStatusEnum.ACTIVE.value] if not model_types: continue diff --git a/api/db/joint_services/tenant_model_service.py b/api/db/joint_services/tenant_model_service.py index 2c1fd085d..fda95630a 100644 --- a/api/db/joint_services/tenant_model_service.py +++ b/api/db/joint_services/tenant_model_service.py @@ -188,19 +188,27 @@ def get_tenant_default_model_by_type(tenant_id: str, model_type: str | enum.Enum def split_model_name(model_name: str): # Parse model_name: {model_name} or {model_name}@{factory_name} or {model_name}@{instance_name}@{factory_name} - parts = model_name.split("@") - if len(parts) == 1: - pure_model_name = parts[0] - provider_name = "" - instance_name = "" - elif len(parts) == 2: - pure_model_name = parts[0] - provider_name = parts[1] + # + # The composite key is right-anchored on the provider: the *last* '@'-separated + # field is the factory/provider, the second-to-last is the instance (when + # present), and everything to the left is the bare model name. Some model + # names legitimately contain '@' characters themselves (e.g. LM Studio + # embedding model IDs such as `text-embedding-nomic-embed-text-v1.5@q8_0`), + # which produces composite keys like + # `text-embedding-nomic-embed-text-v1.5@q8_0@lmstudio@LM-Studio`. + # Use rsplit with maxsplit=2 from the right so any '@' characters embedded + # in the leftmost model-name component are preserved as part of that name. + parts = model_name.rsplit("@", 2) + n = len(parts) + if n == 3: + pure_model_name, instance_name, provider_name = parts + elif n == 2: + pure_model_name, provider_name = parts instance_name = "default" else: pure_model_name = parts[0] - instance_name = parts[1] - provider_name = parts[2] + provider_name = "" + instance_name = "" return pure_model_name, instance_name, provider_name diff --git a/internal/service/model_service.go b/internal/service/model_service.go index 16034dd8f..b3fa5e356 100644 --- a/internal/service/model_service.go +++ b/internal/service/model_service.go @@ -33,20 +33,71 @@ import ( ) // parseModelName parses a composite model name in format "model@instance@provider" or "model@provider" -// Returns modelName, instanceName, providerName separately +// Returns modelName, instanceName, providerName separately. +// +// The composite key is right-anchored: providerName is always the *last* +// '@'-separated field, instanceName is the second-to-last (when present), +// and everything to the left is the bare model name. Some model names +// legitimately contain '@' characters themselves (e.g. LM Studio embedding +// model IDs such as `text-embedding-nomic-embed-text-v1.5@q8_0`), which +// produces composite keys like +// `text-embedding-nomic-embed-text-v1.5@q8_0@lmstudio@LM-Studio`. When the +// split yields more than 3 fields we rejoin the leading fields back into the +// modelName so any embedded '@' characters are preserved verbatim. func parseModelName(compositeName string) (modelName, instanceName, providerName string, err error) { parts := strings.Split(compositeName, "@") - if len(parts) == 3 { + switch len(parts) { + case 3: // Format: model@instance@provider return parts[0], parts[1], parts[2], nil - } else if len(parts) == 2 { + case 2: // Format: model@provider -> instance defaults to "default" return parts[0], "default", parts[1], nil - } else if len(parts) == 1 { + case 1: return parts[0], "", "", fmt.Errorf("provider name missing in model name: %s", compositeName) } + // len(parts) > 3: any '@' characters embedded in the leftmost modelName + // component must be preserved in that component instead of being dropped + // or assigned to the instance/provider fields. + n := len(parts) + return strings.Join(parts[:n-2], "@"), parts[n-2], parts[n-1], nil +} - return "", "", "", fmt.Errorf("invalid model name format: %s", compositeName) +// splitRightAnchoredModelName is a bare-name-tolerant variant of +// parseModelName used by the Builtin / TEI short-circuit branches in +// GetModelConfigFromProviderInstance. +// +// Those branches must accept a bare model name (no provider suffix) where +// parseModelName would return an error, while still preserving any '@' +// characters embedded in the modelName portion of a multi-segment key. +// Returns the modelName, instanceName ("default" for the 2-segment form), +// and providerName ("" for the 1-segment form). +func splitRightAnchoredModelName(compositeName string) (modelName, instanceName, providerName string) { + parts := strings.Split(compositeName, "@") + switch len(parts) { + case 3: + return parts[0], parts[1], parts[2] + case 2: + // The 2-segment form "model@X" is ambiguous: X could be a provider + // suffix (only "Builtin" is recognised by the TEI / Builtin + // short-circuits that consume this helper) or part of the model + // name itself (e.g. a quantization tag like "q8_0" in + // "text-embedding-nomic-embed-text-v1.5@q8_0"). Treat the last + // token as a provider only when it actually is one; otherwise + // the whole string is the bare model name and the caller falls + // through to its non-short-circuit path. The TEI short-circuit's + // `modelName == teiModel` exact-match fast path already covers + // the bare-default case where the embedded '@' happens to match + // the TEI model identifier verbatim. + if parts[1] == "Builtin" { + return parts[0], "default", parts[1] + } + return compositeName, "", "" + case 1: + return parts[0], "", "" + } + n := len(parts) + return strings.Join(parts[:n-2], "@"), parts[n-2], parts[n-1] } func newModelDriverForBaseURL(driver modelModule.ModelDriver, providerName, region, baseURL string) (modelModule.ModelDriver, error) { @@ -2304,21 +2355,28 @@ func (m *ModelProviderService) GetModelConfigFromProviderInstance(tenantID strin // TEI builtin embedding short-circuit if modelType == entity.ModelTypeEmbedding && strings.Contains(os.Getenv("COMPOSE_PROFILES"), "tei-") { - parts := strings.Split(modelName, "@") - teiPure := parts[0] - teiProvider := "" - switch len(parts) { - case 2: - teiProvider = parts[1] - case 3: - teiProvider = parts[2] + teiModel := os.Getenv("TEI_MODEL") + teiBaseURL := os.Getenv("TEI_BASE_URL") + + // First try exact match: handles bare model IDs like "model@q8_0" + // where '@' is part of the model name itself. + if modelName == teiModel { + builtinDriver := modelModule.GetBuiltinEmbeddingModel(modelName) + if builtinDriver == nil { + return nil, "", nil, 0, fmt.Errorf("builtin (TEI) embedding model %q not found", modelName) + } + apiConfig := &modelModule.APIConfig{ApiKey: nil, Region: nil, BaseURL: &teiBaseURL} + return builtinDriver, modelName, apiConfig, 0, nil } - if teiPure == os.Getenv("TEI_MODEL") && (teiProvider == "Builtin" || teiProvider == "") { + + // Then try right-anchored parsing for explicit "model@Builtin" or + // "model@instance@Builtin" composite keys. + teiPure, _, teiProvider := splitRightAnchoredModelName(modelName) + if teiPure == teiModel && (teiProvider == "Builtin" || teiProvider == "") { builtinDriver := modelModule.GetBuiltinEmbeddingModel(teiPure) if builtinDriver == nil { return nil, "", nil, 0, fmt.Errorf("builtin (TEI) embedding model %q not found", teiPure) } - teiBaseURL := os.Getenv("TEI_BASE_URL") apiConfig := &modelModule.APIConfig{ApiKey: nil, Region: nil, BaseURL: &teiBaseURL} return builtinDriver, teiPure, apiConfig, 0, nil } @@ -2335,25 +2393,39 @@ func (m *ModelProviderService) GetModelConfigFromProviderInstance(tenantID strin // request that names a Builtin provider must fall through to the standard // branch, which surfaces an accurate "provider not found" instead of // handing back an embedding-only driver. - parts := strings.Split(modelName, "@") - if modelType == entity.ModelTypeEmbedding && len(parts) >= 2 && parts[len(parts)-1] == "Builtin" { - pureModelName := parts[0] - builtinDriver := modelModule.GetBuiltinEmbeddingModel(pureModelName) - if builtinDriver == nil { - return nil, "", nil, 0, fmt.Errorf("builtin embedding model %q not found", pureModelName) + if modelType == entity.ModelTypeEmbedding { + // The Builtin provider is a local service, not a tenant-enrolled + // provider. Extract the model name by looking for the @Builtin + // suffix explicitly so model names with embedded '@' characters + // (e.g. `model@q8_0@Builtin` or bare `model@q8_0` without any + // provider suffix) are handled correctly. + var pureModelName string + switch { + case strings.HasSuffix(modelName, "@default@Builtin"): + pureModelName = modelName[:len(modelName)-len("@default@Builtin")] + case strings.HasSuffix(modelName, "@Builtin"): + pureModelName = modelName[:len(modelName)-len("@Builtin")] + default: + pureModelName = "" } - apiKey := "" - region := "" - apiConfig := &modelModule.APIConfig{ApiKey: &apiKey, Region: ®ion} - maxTokens := 0 - if mi, _ := dao.GetModelProviderManager().GetModelByName("Builtin", pureModelName); mi != nil { - if mi.MaxTokens == nil { - maxTokens = 0 - } else { - maxTokens = *mi.MaxTokens + if pureModelName != "" { + builtinDriver := modelModule.GetBuiltinEmbeddingModel(pureModelName) + if builtinDriver == nil { + return nil, "", nil, 0, fmt.Errorf("builtin embedding model %q not found", pureModelName) } + apiKey := "" + region := "" + apiConfig := &modelModule.APIConfig{ApiKey: &apiKey, Region: ®ion} + maxTokens := 0 + if mi, _ := dao.GetModelProviderManager().GetModelByName("Builtin", pureModelName); mi != nil { + if mi.MaxTokens == nil { + maxTokens = 0 + } else { + maxTokens = *mi.MaxTokens + } + } + return builtinDriver, pureModelName, apiConfig, maxTokens, nil } - return builtinDriver, pureModelName, apiConfig, maxTokens, nil } pureModelName, instanceName, providerName, err := parseModelName(modelName) diff --git a/internal/service/model_service_test.go b/internal/service/model_service_test.go index 0d4b6b219..c6e83069d 100644 --- a/internal/service/model_service_test.go +++ b/internal/service/model_service_test.go @@ -219,3 +219,147 @@ func TestModelProviderServiceUpdateModelStatusRejectsWrongScopedModelID(t *testi t.Fatalf("code = %v, want %v", code, common.CodeNotFound) } } + +func TestParseModelName(t *testing.T) { + tests := []struct { + name string + composite string + wantModel string + wantInstance string + wantProvider string + wantErr bool + }{ + { + name: "three parts: model@instance@provider", + composite: "text-embedding-3-small@primary@OpenAI", + wantModel: "text-embedding-3-small", + wantInstance: "primary", + wantProvider: "OpenAI", + }, + { + name: "two parts: model@provider defaults instance", + composite: "BAAI/bge-m3@Builtin", + wantModel: "BAAI/bge-m3", + wantInstance: "default", + wantProvider: "Builtin", + }, + { + name: "single part bare name returns error", + composite: "BAAI/bge-m3", + wantErr: true, + }, + { + name: "embedded @ in modelName preserved (four parts)", + composite: "text-embedding-nomic-embed-text-v1.5@q8_0@default@LM-Studio", + wantModel: "text-embedding-nomic-embed-text-v1.5@q8_0", + wantInstance: "default", + wantProvider: "LM-Studio", + }, + { + name: "multiple embedded @ in modelName preserved (five parts)", + composite: "org/repo@tag@1.0@default@Ollama", + wantModel: "org/repo@tag@1.0", + wantInstance: "default", + wantProvider: "Ollama", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + model, instance, provider, err := parseModelName(tt.composite) + if tt.wantErr { + if err == nil { + t.Fatalf("parseModelName(%q) error = nil, want error", tt.composite) + } + return + } + if err != nil { + t.Fatalf("parseModelName(%q) unexpected error: %v", tt.composite, err) + } + if model != tt.wantModel { + t.Errorf("parseModelName(%q) model = %q, want %q", tt.composite, model, tt.wantModel) + } + if instance != tt.wantInstance { + t.Errorf("parseModelName(%q) instance = %q, want %q", tt.composite, instance, tt.wantInstance) + } + if provider != tt.wantProvider { + t.Errorf("parseModelName(%q) provider = %q, want %q", tt.composite, provider, tt.wantProvider) + } + }) + } +} + +func TestSplitRightAnchoredModelName(t *testing.T) { + tests := []struct { + name string + composite string + wantModel string + wantInstance string + wantProvider string + }{ + { + name: "three parts: model@instance@provider", + composite: "text-embedding-3-small@primary@OpenAI", + wantModel: "text-embedding-3-small", + wantInstance: "primary", + wantProvider: "OpenAI", + }, + { + name: "two parts: model@provider defaults instance", + composite: "BAAI/bge-m3@Builtin", + wantModel: "BAAI/bge-m3", + wantInstance: "default", + wantProvider: "Builtin", + }, + { + name: "single part bare name returns empty provider and instance", + composite: "BAAI/bge-m3", + wantModel: "BAAI/bge-m3", + wantInstance: "", + wantProvider: "", + }, + { + // Regression for the CodeRabbit "Major" comment on PR #16468: + // a 2-segment key whose '@' is part of the model name (not a + // provider separator) must stay bare. Without this branch the + // helper would return ("text-embedding-nomic-embed-text-v1.5", + // "default", "q8_0"), mis-classifying the quantization tag as a + // provider and missing the TEI fast path's `modelName == teiModel` + // match when TEI_MODEL is the full embedded string. + name: "two parts bare default with embedded '@' stays bare", + composite: "text-embedding-nomic-embed-text-v1.5@q8_0", + wantModel: "text-embedding-nomic-embed-text-v1.5@q8_0", + wantInstance: "", + wantProvider: "", + }, + { + name: "embedded @ in modelName preserved (four parts)", + composite: "text-embedding-nomic-embed-text-v1.5@q8_0@default@LM-Studio", + wantModel: "text-embedding-nomic-embed-text-v1.5@q8_0", + wantInstance: "default", + wantProvider: "LM-Studio", + }, + { + name: "multiple embedded @ in modelName preserved (five parts)", + composite: "org/repo@tag@1.0@default@Ollama", + wantModel: "org/repo@tag@1.0", + wantInstance: "default", + wantProvider: "Ollama", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + model, instance, provider := splitRightAnchoredModelName(tt.composite) + if model != tt.wantModel { + t.Errorf("splitRightAnchoredModelName(%q) model = %q, want %q", tt.composite, model, tt.wantModel) + } + if instance != tt.wantInstance { + t.Errorf("splitRightAnchoredModelName(%q) instance = %q, want %q", tt.composite, instance, tt.wantInstance) + } + if provider != tt.wantProvider { + t.Errorf("splitRightAnchoredModelName(%q) provider = %q, want %q", tt.composite, provider, tt.wantProvider) + } + }) + } +} diff --git a/test/unit_test/api/apps/services/test_models_api_service_list_tenant_added_models.py b/test/unit_test/api/apps/services/test_models_api_service_list_tenant_added_models.py new file mode 100644 index 000000000..d1db0d34d --- /dev/null +++ b/test/unit_test/api/apps/services/test_models_api_service_list_tenant_added_models.py @@ -0,0 +1,320 @@ +# +# 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. +# +"""Regression tests for list_tenant_added_models() in models_api_service. + +Covers the ValueError that occurred when a manual model record's model_name +contained an `@` character (e.g. LM Studio embedding model IDs that include +the quantization tag, such as `text-embedding-nomic-embed-text-v1.5@q8_0`). +""" + +import importlib.util +import logging +import sys +from pathlib import Path +from types import ModuleType, SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +pytestmark = pytest.mark.p2 + + +def _stub(monkeypatch, name, **attrs): + """Register a stub module in `sys.modules` with the given attributes. + + For dotted names (e.g. `api.db.services.user_service`) the stub is also + attached as an attribute of the parent module so that code which already + imported the parent and references `parent.child` sees the stub. + + Args: + monkeypatch: pytest monkeypatch fixture. + name: Fully qualified module name to stub. + **attrs: Attributes to set on the stub module. + + Returns: + The created `types.ModuleType` instance. + """ + mod = ModuleType(name) + for key, value in attrs.items(): + setattr(mod, key, value) + monkeypatch.setitem(sys.modules, name, mod) + if "." in name: + parent_name, _, child_name = name.rpartition(".") + parent_mod = sys.modules.get(parent_name) + if parent_mod is not None: + monkeypatch.setattr(parent_mod, child_name, mod, raising=False) + return mod + + +def _load_module(monkeypatch, *, tenant_model_records, factory_llm_infos=None): + """Load models_api_service with stubbed dependencies. + + `tenant_model_records` is the list returned by + TenantModelService.get_models_by_provider_ids_and_instance_ids. Pass an + empty list to test the no-models path. + + Returns a tuple `(module, stubs)` where `stubs` is a dict mapping the + stubbed module names to the stub modules, so callers can monkeypatch + additional behaviour at runtime. + """ + + tenant = SimpleNamespace(id="tenant-1") + provider = SimpleNamespace( + id="provider-1", + provider_name="LM-Studio", + ) + instance = SimpleNamespace( + id="instance-1", + provider_id="provider-1", + instance_name="default", + ) + + _stub( + monkeypatch, + "api.db.services.user_service", + TenantService=SimpleNamespace(get_by_id=lambda tenant_id: (True, tenant)), + ) + _stub( + monkeypatch, + "api.db.services.tenant_model_provider_service", + TenantModelProviderService=SimpleNamespace( + get_by_tenant_id=lambda tenant_id: [provider], + # Default no-op; tests that need to observe the resolved provider + # name passed by `_get_model_info` override this with + # monkeypatch.setattr on the loaded stub. + get_by_tenant_id_and_provider_name=lambda tenant_id, provider_name: SimpleNamespace( + id="provider-1", provider_name=provider_name + ), + ), + ) + _stub( + monkeypatch, + "api.db.services.tenant_model_instance_service", + TenantModelInstanceService=SimpleNamespace( + get_by_provider_ids=lambda provider_ids: [instance], + get_by_provider_id_and_instance_name=lambda provider_id, instance_name: SimpleNamespace( + id="instance-1", provider_id=provider_id, instance_name=instance_name + ), + ), + ) + _stub( + monkeypatch, + "api.db.services.tenant_model_service", + TenantModelService=SimpleNamespace( + get_models_by_provider_ids_and_instance_ids=lambda p_ids, i_ids: list(tenant_model_records), + # Default returns an "active" model so `_get_model_info` treats + # the row as enabled when exercising the bare-model branch. + get_by_provider_id_and_instance_id_and_model_type_and_model_name=lambda *args: SimpleNamespace( + status=1 + ), + ), + ) + + # joint_services.tenant_model_service is imported by models_api_service at + # module load for the three ensure_* helpers; stub it as a no-op. + _stub( + monkeypatch, + "api.db.joint_services.tenant_model_service", + ensure_mineru_from_env=lambda *a, **kw: None, + ensure_paddleocr_from_env=lambda *a, **kw: None, + ensure_opendataloader_from_env=lambda *a, **kw: None, + ) + _stub( + monkeypatch, + "api.db.joint_services", + tenant_model_service=sys.modules["api.db.joint_services.tenant_model_service"], + ) + + _stub( + monkeypatch, + "common.constants", + ActiveStatusEnum=SimpleNamespace(ACTIVE=SimpleNamespace(value=1), INACTIVE=SimpleNamespace(value=0), UNSUPPORTED=SimpleNamespace(value=2)), + LLMType=SimpleNamespace(EMBEDDING="embedding"), + ) + _stub( + monkeypatch, + "common.settings", + FACTORY_LLM_INFOS=factory_llm_infos if factory_llm_infos is not None else [], + ) + + module_path = ( + Path(__file__).resolve().parents[5] + / "api" + / "apps" + / "services" + / "models_api_service.py" + ) + spec = importlib.util.spec_from_file_location( + "test_models_api_service_list_tenant_added_models", + module_path, + ) + module = importlib.util.module_from_spec(spec) + monkeypatch.setitem(sys.modules, "test_models_api_service_list_tenant_added_models", module) + spec.loader.exec_module(module) + stubs = { + "user_service": sys.modules["api.db.services.user_service"], + "tenant_model_provider_service": sys.modules["api.db.services.tenant_model_provider_service"], + "tenant_model_instance_service": sys.modules["api.db.services.tenant_model_instance_service"], + "tenant_model_service": sys.modules["api.db.services.tenant_model_service"], + } + return module, stubs + + +def _make_model_record(model_name, model_type="embedding", status=1): + """Build a `SimpleNamespace` mimicking a `TenantModel` ORM row. + + The default provider_id/instance_id pair (`provider-1`/`instance-1`) + matches the fixtures wired up by `_load_module` so the resulting key + `provider-1|instance-1|` round-trips through + `list_tenant_added_models` as expected. + + Args: + model_name: Model name; may contain `@` characters. + model_type: Model type filter (default `embedding`). + status: `ActiveStatusEnum` value (default `1` = ACTIVE). + + Returns: + A `SimpleNamespace` with the fields read by + `list_tenant_added_models`. + """ + return SimpleNamespace( + provider_id="provider-1", + instance_id="instance-1", + model_name=model_name, + model_type=model_type, + status=status, + ) + + +@pytest.mark.p2 +def test_list_tenant_added_models_handles_at_symbol_in_model_name(monkeypatch): + """Regression: model_name containing '@' must not raise ValueError. + + LM Studio exposes embedding model IDs like + `text-embedding-nomic-embed-text-v1.5@q8_0`. RAGFlow used to build a + composite key `provider@instance@model_name` and then unpack it back with + `split("@")`, which raised ValueError when the model_name itself contained + `@`. After the fix, the key is split with `rsplit("@", 2)` so any extra + `@` characters stay attached to model_name. + """ + record = _make_model_record("text-embedding-nomic-embed-text-v1.5@q8_0") + module, _ = _load_module(monkeypatch, tenant_model_records=[record]) + + success, result = module.list_tenant_added_models("tenant-1", "embedding") + + assert success is True + assert isinstance(result, list) + assert len(result) == 1 + added = result[0] + assert added["name"] == "text-embedding-nomic-embed-text-v1.5@q8_0" + assert added["provider_id"] == "provider-1" + assert added["instance_id"] == "instance-1" + assert added["provider_name"] == "LM-Studio" + assert added["instance_name"] == "default" + + +@pytest.mark.p2 +def test_list_tenant_added_models_handles_multiple_at_symbols_in_model_name(monkeypatch): + """Same regression with multiple '@' characters in the model_name.""" + record = _make_model_record("foo@bar@baz@q4_k_m") + module, _ = _load_module(monkeypatch, tenant_model_records=[record]) + + success, result = module.list_tenant_added_models("tenant-1", "embedding") + + assert success is True + assert len(result) == 1 + assert result[0]["name"] == "foo@bar@baz@q4_k_m" + assert result[0]["provider_id"] == "provider-1" + assert result[0]["instance_id"] == "instance-1" + + +@pytest.mark.p2 +def test_list_tenant_added_models_preserves_full_model_name_with_at(monkeypatch): + """The trailing model_name must absorb any extra '@' characters. + + Verifies that split('@', 2) (not rsplit) is used so the right-hand side + is preserved. If rsplit were used, the trailing '@q8_0' would become the + full model_name and the prefix would be lost. + """ + record = _make_model_record("a/b@q8_0") + module, _ = _load_module(monkeypatch, tenant_model_records=[record]) + + success, result = module.list_tenant_added_models("tenant-1", "embedding") + + assert success is True + assert len(result) == 1 + assert result[0]["name"] == "a/b@q8_0" + assert result[0]["provider_id"] == "provider-1" + assert result[0]["instance_id"] == "instance-1" + + +@pytest.mark.p2 +def test_list_tenant_added_models_still_works_for_plain_model_names(monkeypatch): + """Sanity check: the rsplit change must not break the standard case.""" + record = _make_model_record("gemma-4-12b-it-qat", model_type="chat") + module, _ = _load_module(monkeypatch, tenant_model_records=[record]) + + success, result = module.list_tenant_added_models("tenant-1", "chat") + + assert success is True + assert len(result) == 1 + assert result[0]["name"] == "gemma-4-12b-it-qat" + + +# NOTE: A unit test for the defensive try/except in the production code is +# intentionally omitted. In the current production flow every key built by +# `f"{provider_id}|{instance_id}|{model_name}"` has exactly 3 '|'-separated +# parts, so the defensive branch is unreachable. The branch is left in as +# insurance for future code paths that might construct keys differently. +# If a future change makes the branch reachable, add a focused unit test for +# it at that time. + + +@pytest.mark.p2 +def test_get_model_info_two_part_embedded_at(monkeypatch): + """Two-part default_model is parsed as model@provider (suffix wins). + + A default_model like `text-embedding-nomic-embed-text-v1.5@q8_0` has + exactly one '@'. `_get_model_info`'s 2-segment branch does + `default_model.rsplit('@', 2)` and assigns the right-anchored suffix + as `provider_name`: + + model_name = "text-embedding-nomic-embed-text-v1.5" + provider_name = "q8_0" + instance_name = "default" + + So the embedded '@' is interpreted as the provider separator, not + preserved within `model_name`. The legacy `tenant_llm`-based bare-model + fallback that would have recognised `q8_0` as a quantization suffix + was removed per maintainer request; the 2-part branch now always + treats the suffix as a provider. The stub returns a matching + provider/instance for any provider name, so the function resolves + to a valid result. + """ + module, _ = _load_module( + monkeypatch, + tenant_model_records=[], + ) + + result = module._get_model_info( + "tenant-1", + "text-embedding-nomic-embed-text-v1.5@q8_0", + "embedding", + ) + assert result is not None + assert result["model_provider"] == "q8_0" + assert result["model_name"] == "text-embedding-nomic-embed-text-v1.5" + assert result["model_instance"] == "default" diff --git a/test/unit_test/api/db/services/test_tenant_model_service_split_model_name.py b/test/unit_test/api/db/services/test_tenant_model_service_split_model_name.py new file mode 100644 index 000000000..9eb464579 --- /dev/null +++ b/test/unit_test/api/db/services/test_tenant_model_service_split_model_name.py @@ -0,0 +1,155 @@ +# +# 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. +# +"""Regression tests for split_model_name() in api.db.joint_services.tenant_model_service. + +The composite model key format used by RAGFlow is right-anchored: +``model_name@instance_name@provider_name`` for a manual instance, or +``model_name@provider_name`` when the instance defaults to ``"default"``. + +Some model names legitimately contain ``@`` characters (for example LM +Studio embedding IDs like ``text-embedding-nomic-embed-text-v1.5@q8_0``), +producing four-``@``-separated composite keys such as +``text-embedding-nomic-embed-text-v1.5@q8_0@lmstudio@LM-Studio``. + +A naive ``split("@")`` would mis-parse these keys, stripping the +quantization suffix off the model name and treating the wrong middle field +as the provider. The function uses ``rsplit("@", 2)`` so the rightmost two +fields remain the instance and provider regardless of how many ``@`` +characters appear in the leftmost model-name field. + +This test loads only the function definition from the source file via AST, +so it doesn't pull in the full ``common.settings`` import chain (which +transitively imports ``tiktoken``, ``onnxruntime``, the ES connector, etc.) +and can run in any minimal pytest environment. +""" + +import ast +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.p2 + + +def _load_split_model_name(): + """Load the `split_model_name` function from the production source file. + + The function is pure-Python and does not reference any module-level + names, so it can be exec'd standalone in an empty namespace. + """ + src_path = ( + Path(__file__).resolve().parents[5] + / "api" + / "db" + / "joint_services" + / "tenant_model_service.py" + ) + tree = ast.parse(src_path.read_text(encoding="utf-8")) + fn_node = next( + node for node in tree.body + if isinstance(node, ast.FunctionDef) and node.name == "split_model_name" + ) + module = ast.Module(body=[fn_node], type_ignores=[]) + ns: dict = {} + exec(compile(module, str(src_path), "exec"), ns) + return ns["split_model_name"] + + +split_model_name = _load_split_model_name() + + +@pytest.mark.p2 +def test_split_model_name_with_at_symbol_in_model_name_lm_studio_embedding(): + """The exact LM Studio case from the dataset-configuration bug. + + Before the fix, this raised nothing but returned the wrong + ``provider_name='lmstudio'``, triggering + ``LookupError("Provider lmstudio not found ...")``. After the rsplit + fix, the full ``@q8_0`` suffix stays attached to ``model_name`` and + ``provider_name`` correctly resolves to ``LM-Studio``. + """ + composite = "text-embedding-nomic-embed-text-v1.5@q8_0@lmstudio@LM-Studio" + pure, instance, provider = split_model_name(composite) + + assert pure == "text-embedding-nomic-embed-text-v1.5@q8_0" + assert instance == "lmstudio" + assert provider == "LM-Studio" + + +@pytest.mark.p2 +def test_split_model_name_plain_three_part(): + """Standard 3-part key: model@instance@provider.""" + pure, instance, provider = split_model_name("gpt-4o@eastus@OpenAI") + + assert pure == "gpt-4o" + assert instance == "eastus" + assert provider == "OpenAI" + + +@pytest.mark.p2 +def test_split_model_name_two_part_defaults_instance(): + """Two-part key (no explicit instance) falls back to ``default``.""" + pure, instance, provider = split_model_name("gpt-4o@OpenAI") + + assert pure == "gpt-4o" + assert instance == "default" + assert provider == "OpenAI" + + +@pytest.mark.p2 +def test_split_model_name_bare_model_name(): + """Bare model name (no '@' anywhere) yields empty provider/instance.""" + pure, instance, provider = split_model_name("qwen2.5-7b-instruct") + + assert pure == "qwen2.5-7b-instruct" + assert instance == "" + assert provider == "" + + +@pytest.mark.p2 +def test_split_model_name_multiple_at_symbols_in_model_name(): + """Several embedded ``@`` characters in the model name are preserved.""" + composite = "foo@bar@baz@q4_k_m@myinst@MyProvider" + pure, instance, provider = split_model_name(composite) + + assert pure == "foo@bar@baz@q4_k_m" + assert instance == "myinst" + assert provider == "MyProvider" + + +@pytest.mark.p2 +def test_split_model_name_three_part_with_at_in_model_name(): + """3-part key where the model name contains a single ``@``. + + Composite: ``foo@bar@HuggingFace`` parses as a 3-part key + (``foo``, ``bar``, ``HuggingFace``). The middle field is treated as + the explicit instance. This is the smallest case that exercises the + rsplit behavior on a model name containing ``@``. + """ + pure, instance, provider = split_model_name("foo@bar@HuggingFace") + + assert pure == "foo" + assert instance == "bar" + assert provider == "HuggingFace" + + +@pytest.mark.p2 +def test_split_model_name_returns_tuple_of_strings(): + """Sanity: the function must always return three string values.""" + pure, instance, provider = split_model_name("a@b@c") + assert isinstance(pure, str) + assert isinstance(instance, str) + assert isinstance(provider, str) \ No newline at end of file