Skip to content

refactor: enhance model validation and loading logic in drawers #7839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wxg0103 merged 1 commit into from
Nov 11, 2026
Merged

refactor: enhance model validation and loading logic in drawers #7839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions apps/models_provider/tests/test_provide_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,58 @@ def test_model_list_returns_provider_models(self):
api_base=None,
credential=None,
)

def test_model_list_post_requires_provider_credential_fields(self):
provider = MagicMock()
provider.get_model_type_list.return_value = [{"value": "EMBEDDING"}]
provider.get_model_credential.return_value.to_form_list.return_value = [
{"field": "api_base", "label": "API URL", "required": True},
{"field": "api_key", "label": "API Key", "required": True},
]
request = SimpleNamespace(
data={
"provider": "test_provider",
"model_type": "EMBEDDING",
"credential": {"api_base": " ", "api_key": ""},
}
)

with patch("models_provider.views.provide.ModelProvideConstants", {"test_provider": provider}):
with self.assertRaises(BadRequest400) as context:
Provide.ModelList().post(request)

self.assertIn("API URL", str(context.exception.message))
self.assertIn("API Key", str(context.exception.message))
provider.get_model_list.assert_not_called()

def test_model_list_post_queries_with_provider_credential_fields(self):
provider = MagicMock()
provider.get_model_type_list.return_value = [{"value": "EMBEDDING"}]
provider.get_model_credential.return_value.to_form_list.return_value = [
{"field": "api_base", "label": "API URL", "required": True},
{"field": "api_key", "label": "API Key", "required": True},
]
provider.get_model_list.return_value = [{"name": "text-embedding-3-small"}]
request = SimpleNamespace(
data={
"provider": "test_provider",
"model_type": "EMBEDDING",
"credential": {"api_base": "https://api.example.com/v1", "api_key": "secret"},
}
)

with (
patch("models_provider.views.provide.ModelProvideConstants", {"test_provider": provider}),
patch("models_provider.views.provide.cache_base_model_list") as cache_model_list,
):
response = Provide.ModelList().post(request)

self.assertEqual(200, response.status_code)
provider.get_model_list.assert_called_once_with(
"EMBEDDING",
api_key=None,
api_base=None,
credential={"api_base": "https://api.example.com/v1", "api_key": "secret"},
allow_builtin_fallback=False,
)
cache_model_list.assert_called_once()

@shaohuzhang1 shaohuzhang1 Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a review of the added test code:

Issues / Observations

  1. Whitespace-only credential handling is implicit but unverified.
    test_model_list_post_requires_provider_credential_fields passes "api_base": " " (a space) and expects it to be treated as missing. This is fine if the production code strips values — but the test relies on that undocumented behavior. Consider also asserting a whitespace-only api_key to lock in stripping for both fields, or add a separate test if strip() isn't guaranteed.

  2. Magic strings duplicated across tests.
    "test_provider", field names, and labels are repeated. Extract constants or a shared setUp/helper to avoid drift between the two tests.

  3. Patch target string duplication.
    "models_provider.views.provide.ModelProvideConstants" appears twice. A module-level constant like PATCH_PROVIDER_CONSTANTS = "..." improves maintainability and typo safety.

  4. Weak assertion on cache call.
    cache_model_list.assert_called_once() doesn't verify arguments. Assert the payload (model list, provider, type, credentials) to ensure caching keyed correctly — a bug could cache under the wrong provider otherwise.

  5. provider.get_model_type_list.return_value is never asserted.
    It's set up but not verified. Either assert it was called with expected args, or remove the setup if the endpoint doesn't require validation here. If validation of model_type against this list exists, add a negative test for an invalid model_type.

  6. Mocking external constants vs. injectable dependency.
    Patching a module-global dict (ModelProvideConstants) couples the test to implementation detail. Consider injecting the provider registry into ModelList (constructor/class attr) for cleaner, less brittle tests.

  7. SimpleNamespace(data=...) for request.
    Works, but using Django's RequestFactory (or the actual framework's test client) would exercise middleware/serialization and catch real integration issues (e.g., multi-value form fields, non-string types).

  8. Only success/validation paths covered.
    Missing tests worth adding:

    • Unknown provider → appropriate error (404/BadRequest).
    • Invalid/missing model_type.
    • Exception raised inside provider.get_model_list → how is it propagated/wrapped?
    • Empty model list result → still cached? correct shape returned?

  9. Assertion style inconsistency.
    Mixes assertEqual(200, response.status_code) with assertIn(..., str(...)). Prefer consistent self.assertEqual(response.status_code, 200) ordering; also asserting the body structure (not just str(context.exception.message)) would be stronger (e.g., structured error codes/fields).

  10. allow_builtin_fallback=False is asserted but not tested in the alternative branch.
    Add a test where allow_builtin_fallback is passed as True in the request to cover that path.

  11. Potential naming confusion.
    test_model_list_returns_provider_models (existing) suggests GET-like behavior; new tests cover POST. Confirm the method overload is intentional, or split into separate endpoints/tests for clarity.

  12. No assertion that get_model_credential().to_form_list() is called once.
    Adding provider.get_model_credential.assert_called_once() guards against redundant instantiation.

Suggested Minimal Refactor

PROVIDER_PATCH = "models_provider.views.provide.ModelProvideConstants"

def _make_provider(self):
    provider = MagicMock()
    provider.get_model_type_list.return_value = [{"value": "EMBEDDING"}]
    provider.get_model_credential.return_value.to_form_list.return_value = [
        {"field": "api_base", "label": "API URL", "required": True},
        {"field": "api_key", "label": "API Key", "required": True},
    ]
    return provider

Then use patch(PROVIDER_PATCH, ...) consistently and assert cached payload:

cache_model_list.assert_called_once_with(
    provider="test_provider",
    model_type="EMBEDDING",
    model_list=[{"name": "text-embedding-3-small"}],
)

Overall the tests are reasonably focused; main improvements are stronger assertions on cache arguments, reducing duplication, and adding coverage for error/fallback branches.

47 changes: 42 additions & 5 deletions apps/models_provider/views/provide.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,38 @@ def _get_provider(provider):
return ModelProvideConstants[provider]


def _is_empty_credential_value(value):
return value is None or (isinstance(value, str) and not value.strip())


def _validate_model_type(provider_handler, model_type):
if not model_type:
raise BadRequest400(400, _("model_type is required"))
model_type_list = [item.get("value") for item in provider_handler.get_model_type_list()]
if model_type not in model_type_list:
raise BadRequest400(400, _("model_type is invalid"))


def _validate_required_credential(provider_handler, model_type, credential):
"""按供应商自己的凭证表单校验模型列表查询所需的必填参数。"""
model_credential = provider_handler.get_model_credential(model_type, "")
if model_credential is None:
return
missing_fields = []
for field in model_credential.to_form_list():
if not field.get("required") or not _is_empty_credential_value(credential.get(field.get("field"))):
continue
label = field.get("label")
if isinstance(label, dict):
label = label.get("label")
missing_fields.append(str(label or field.get("field")))
if missing_fields:
raise BadRequest400(
400,
_("The following fields are required: {keys}").format(keys=", ".join(missing_fields)),
)


class Provide(APIView):
authentication_classes = [TokenAuth]

Expand Down Expand Up @@ -116,11 +148,7 @@ def _filter_model_list(model_list, search):
@staticmethod
def _get_model_list(provider, model_type, api_key, api_base, credential, allow_builtin_fallback=True):
provider_handler = _get_provider(provider)
if not model_type:
raise BadRequest400(400, _("model_type is required"))
model_type_list = [item.get("value") for item in provider_handler.get_model_type_list()]
if model_type not in model_type_list:
raise BadRequest400(400, _("model_type is invalid"))
_validate_model_type(provider_handler, model_type)
if allow_builtin_fallback:
return provider_handler.get_model_list(
model_type, api_key=api_key, api_base=api_base, credential=credential
Expand Down Expand Up @@ -161,6 +189,15 @@ def post(self, request: Request):
credential = data.get("credential")
search = data.get("search")

provider_handler = _get_provider(provider)
credential = dict(credential) if isinstance(credential, dict) else {}
if api_key and not credential.get("api_key"):
credential["api_key"] = api_key
if api_base and not credential.get("api_base"):
credential["api_base"] = api_base
_validate_model_type(provider_handler, model_type)
_validate_required_credential(provider_handler, model_type, credential)

if not (api_key or credential):
# 未提供凭证：直接返回内置列表，无缓存语义。
model_list = self._get_model_list(provider, model_type, api_key, api_base, credential)
Expand Down
20 changes: 16 additions & 4 deletions ui/src/views/model/create-model/CreateModelDrawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,23 @@ function applyBaseModelOptions(provider: string, modelType: string, credentialSn
JSON.stringify(modelForm.credential) === credentialSnapshot
) {
baseModelOptions.value = options
return true
}
return false
}
const baseModelLoading = ref(false)
const credentialFields = ref<FormField[]>([])
const modelParamsForm = ref<DynamicFormField[]>([])
function fetchBaseModelOptions() {
async function fetchBaseModelOptions() {
if (!modelForm.provider || !modelForm.model_type) return
const requiredFields = credentialFields.value.filter((field) => field.required !== false).map((field) => field.field)
if (requiredFields.length) {
const valid = await dynamicsFormRef.value?.ruleFormRef?.validateField(requiredFields).then(
() => true,
() => false,
)
if (!valid) return
}
baseModelLoading.value = true
const requestProvider = modelForm.provider
const requestModelType = modelForm.model_type
Expand All @@ -109,10 +119,12 @@ function fetchBaseModelOptions() {
ProviderApi.getBaseModelList(requestProvider, requestModelType, undefined, modelForm.credential as Record<string, unknown>)
.then((options) => {
if (options?.length) {
applyBaseModelOptions(requestProvider, requestModelType, credentialSnapshot, options)
} else {
return loadBuiltin()
if (applyBaseModelOptions(requestProvider, requestModelType, credentialSnapshot, options)) {
MsgSuccess('获取成功')
}
return
}
return loadBuiltin()
})
.catch(() => loadBuiltin())
.finally(() => {
Expand Down
20 changes: 16 additions & 4 deletions ui/src/views/model/model-card/action-dropdown/edit-model-action/EditModelDrawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ function applyBaseModelOptions(provider: string, modelType: string, credentialSn
JSON.stringify(modelForm.credential) === credentialSnapshot
) {
baseModelOptions.value = options
return true
}
return false
}
const baseModelLoading = ref(false)
const credentialFields = ref<FormField[]>([])
Expand Down Expand Up @@ -93,8 +95,16 @@ function open(provider: ModelProviderItem, model: ModelItem) {
})
}

function fetchBaseModelOptions() {
async function fetchBaseModelOptions() {
if (!modelForm.provider || !modelForm.model_type) return
const requiredFields = credentialFields.value.filter((field) => field.required !== false).map((field) => field.field)
if (requiredFields.length) {
const valid = await dynamicsFormRef.value?.ruleFormRef?.validateField(requiredFields).then(
() => true,
() => false,
)
if (!valid) return
}
baseModelLoading.value = true
const requestProvider = modelForm.provider
const requestModelType = modelForm.model_type
Expand All @@ -107,10 +117,12 @@ function fetchBaseModelOptions() {
ProviderApi.getBaseModelList(requestProvider, requestModelType, undefined, modelForm.credential as Record<string, unknown>)
.then((options) => {
if (options?.length) {
applyBaseModelOptions(requestProvider, requestModelType, requestCredentialSnapshot, options)
} else {
return loadBuiltin()
if (applyBaseModelOptions(requestProvider, requestModelType, requestCredentialSnapshot, options)) {
MsgSuccess('获取成功')
}
return
}
return loadBuiltin()
})
.catch(() => loadBuiltin())
.finally(() => {
Expand Down
Loading