[shaohuzhang1]

refactor: simplify error message formatting in exception handling

[shaohuzhang1]

Review of the diff

1. Functional issue / fragile substring match
if "bge" in model_id: uses a plain substring check on the raw string. This is case-sensitive and unanchored, so it will:

• Miss uppercase or mixed-case IDs like BGE-M3, BGE-Large.
• False-positive on unrelated IDs that happen to contain the letters bge as part of another token (e.g. ...subgemini..., vendor prefixes, version suffixes). Low probability today, but brittle.

Suggested: normalize and use a clearer token match, e.g.

mid = model_id.lower()
if mid.startswith("bge") or "-bge" in mid:
    return ModelTypeConst.EMBEDDING

2. Ordering dependency is implicit and undocumented in code structure
The comment explains why bge follows rerank, which is good, but the correctness now relies entirely on statement order. If someone later reorders or adds another prefix check (e.g. "m3" → EMBEDDING, or moving embedding checks), bge-reranker-* could regress silently. Consider making the reranker exclusion explicit:

if "bge" in model_id and "rerank" not in model_id:
    return ModelTypeConst.EMBEDDING

This makes intent self-contained and order-independent.

3. Consistency with existing style
Other branches use any(k in model_id for k in (...)). A single-entry tuple is unnecessary; a plain in check is fine. But mixing styles may invite future inconsistencies — acceptable as-is.

4. No test coverage shown
Since this changes classification of a whole model family (bge-small, bge-m3, bge-large, and critically bge-reranker-*), add unit tests asserting at minimum:

• bge-m3 → EMBEDDING
• bge-reranker-v2-m3 → RERANKER (regression guard)
• An unrelated ID containing bge does not get misrouted if relevant.

5. Coverage gap (not caused by this diff)
bge is ForceAI/BAAI naming; other embedding families (e.g. gte, e5, m3e, jina-embed, text-embedding) aren't handled. If the goal is general robustness, consider a shared constant list rather than ad-hoc branches.

No blocking bugs — the change works for the documented cases. Main risks are the case-sensitive/unanchored match and the order-dependent logic.

[shaohuzhang1]

The change removes a translated, user-friendly message ("Verification failed, please check whether the parameters are correct: {error}") and replaces it with raw exception text str(e).

Issues / irregularities:

1. Loss of internationalization (i18n). The original used gettext(...) so the message could be localized; the new version drops that entirely. If the app supports multiple languages, this is a regression.

2. Less user-friendly error output. str(e) may expose internal validation details or produce cryptic messages (e.g., Pydantic-style "value is not a valid integer") instead of a consistent, context-aware message. This hurts UX and can leak implementation details.

3. Inconsistent error format. Other places likely still wrap errors in a friendly prefix; this creates inconsistent API error responses.

4. Potential information disclosure. Raw exception strings may reveal field names, library internals, or stack-related info—minor security/UX concern in an API response.

Suggestions:

• Keep the gettext wrapper for i18n, e.g.:

  gettext("Verification failed, please check whether the parameters are correct: {error}").format(error=str(e))

• If you want cleaner logs vs. responses, pass a generic translated message to the client and log str(e) separately at debug level.
• Ensure str(e) is safe/sanitized if it must be returned.
• If the intent was to avoid double-translation or verbose messages, consider a dedicated translated fallback like gettext("Invalid parameters") rather than raw exception text.

Minor: No functional bug introduced (exception type and code ValidCode.valid_error.value unchanged), but the behavioral change in message content is not obviously justified by the diff alone.

[shaohuzhang1]

Looking at this diff, here are the observations:

Potential Issues

1. Loss of Context / Localization Regression

The biggest concern is losing user-friendly context. The original message:

• Used gettext() → i18n/localization support
• Included a descriptive prefix: "Verification failed, please check whether the parameters are correct"
• Appended the specific error via {error}

The new version returns only str(e), which:

• Breaks i18n — raw exception strings won't be translated
• Gives the end user only a bare technical error with no guidance
• Makes it harder to distinguish "verification failed" errors from other API exceptions in logs/UI (no stable message prefix)

2. Raw Exception Messages Leaking to Clients

str(e) may expose internal details (stack fragments, field names, encoding internals like Pydantic/Django validation messages). This can be an information disclosure risk. The original phrasing at least framed it as a generic user-facing error.

3. Inconsistent Error Contract

Other branches of is_valid presumably raise AppApiException with a standard message. If one path only returns str(e), clients that pattern-match on the message prefix will behave inconsistently.

4. str(e) May Be Empty

Some exceptions (e.g., bare ValueError()) yield "". Then the exception carries an empty reason, making debugging harder than before.

Suggestions

1. Keep the localized wrapper rather than dropping it:

   gettext("Verification failed, please check whether the parameters are correct: {error}").format(error=str(e))

   If translation is genuinely unnecessary, at minimum keep a stable, documented prefix.

2. Separate user-facing vs. internal detail: Log the full repr(e) (or traceback), but return a sanitized/masked message to the client.

3. Guard against empty messages:

   error_detail = str(e) or type(e).__name__

4. Consider structured errors: pass an error code + safe message instead of free-form strings, so frontend can localize and avoid trusting server text.

5. If intentional (e.g., migrating away from gettext for error payloads), ensure this change is applied consistently across all AppApiException call sites, not just this one — otherwise you create mixed conventions.

Minor Optimization

• No performance concern here; the removal itself is cheaper, but correctness/i18n costs outweigh any micro-gain.

Bottom line: This change appears to be a regression in i18n and user experience, and potentially a minor info-disclosure risk. Recommend reverting unless it's part of a coordinated refactor with sanitization elsewhere.