[shaohuzhang1]

refactor: update string filtering to be case-insensitive for improved search functionality
refactor: add client parameter handling to prevent sending sensitive config in request body

[shaohuzhang1]

Here's my review of the code:

Irregularities / Potential Issues

1. Missing test for actual success path: Neither credential test covers the happy path (valid credentials returning True). Only failure cases are tested.

2. Brittle assertion on exception message verbatim: test_validation_propagates_provider_error_message asserts the exact string "Error code: 500 - {...}" is in .message. If the production code re-wraps or reformats the error, this breaks. Consider asserting on a stable substring like "internal_error" instead.

3. Unused import: QianfanVisionModel and QianfanChatModel are both used, but verify MagicMock usage — it's fine. However max_tokens=1 / temperature=0.95 are arbitrary; document why if meaningful, else simplify.

4. Hard-coded fake API key prefix sk-test: Fine for mocks, but ensure no network calls actually occur. new_instance could theoretically trigger validation/network I/O — confirm it doesn't, otherwise tests are not hermetic despite SimpleTestCase.

5. Untested behavior in test_client_params_are_not_sent_in_request_body: The test verifies params are absent from extra_body and payload, but doesn't verify they land where expected on the underlying client beyond attribute checks. Attribute model.request_timeout existing doesn't guarantee it was passed to the OpenAI client constructor.

6. MagicMock provider.get_model returns a mock whose .invoke raises — good, but the model's other attributes (e.g., name) may be accessed by is_valid before invoke; confirm the mock covers those accesses (currently any attribute auto-resolves to a MagicMock, which may mask bugs).

7. Duplicated setup across tests: _mock_provider + QianfanLLMModelCredential() + argument tuple is repeated. Consider a helper or setUp.

8. subTest loop only covers construction args identical for both models — fine, but add a comment that vision/chat share the same client-config contract intentionally.

Optimization Suggestions

• Add a positive-path test (invoke succeeds → is_valid returns True).
• Parametrize the two credential tests with subTest over raise_exception True/False to reduce duplication.
• Assert AppApiException.message contains the original exception text via assertIn("internal_error", ...) rather than the full raw string.
• Verify request params are forwarded to the client (e.g., assert against the constructed OpenAI client) to prove they're "applied," not just absent from the body.
• Consider asserting payload does not unexpectedly contain other internal keys (snapshot-style) to catch regressions broadly.

Overall the tests are reasonable and focused; main gaps are the missing success case and over-specific error-message assertions.

[shaohuzhang1]

Review of the Diff

1. __contains → __icontains (lines ~245, ~400, ~533)

Good change.

• Better UX: case-insensitive search is usually what users expect.
• Caveat: icontains on large tables can be slower and may bypass certain DB indexes (e.g., PostgreSQL LIKE index only helps with text_pattern_ops/pg_trgm). If these querysets are large or hit hot paths, consider a trigram index or full-text search.

2. Removing redundant f-string (f'attachment; filename="data.xlsx"' → plain string)

Correct. There were no interpolation placeholders, so the f prefix was dead code. No behavior change.

3. is_resource_filter = False added after workspace_manage branch (line ~409)

Potentially behavior-changing — verify intended semantics.

Compare with the sibling method around line ~533:

if is_workspace_manage(auth, workspace_id):
    queryset = queryset.filter(workspace_id=workspace_id)
    is_resource_filter = False   # <-- only here in original
elif is_extends_workspace_manage(auth, workspace_id):
    ...

The patched block now also sets is_resource_filter = False for the workspace-manager path. Two concerns:

1. Is this a bug fix or an accidental change? If is_resource_filter controls whether further permission filtering is applied downstream, setting it to False means workspace managers now skip that extra filtering. That is likely the intent (managers shouldn't be restricted by per-resource permissions), but it should be confirmed against tests.
2. Consistency: The sibling method at ~533 already does this. So this change makes them consistent — good, but worth double-checking there isn't another branch (e.g., is_extends_workspace_manage) missing the same treatment.
3. Naming clarity: is_resource_filter is ambiguous. Consider something like skip_resource_permission_filter.

4. Other observations (not in diff but visible)

• Content-Type mismatch: content_type="application/vnd.ms-excel" is the MIME type for .xls, but the file is written as data.xlsx. For modern Excel use:

  content_type = "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"

  Also add UTF-8 filename encoding via filename*=UTF-8''... if non-ASCII names may be used later.

• Duplication: The export method is copy-pasted three times. Extract a shared helper like _build_excel_response(rows).

• QuerySet(Application) — assuming this is a custom wrapper; fine, just noting.

5. Minor

• In the removed f-string, workbook.save(response) is unchanged — fine.
• Consider adding a test asserting case-insensitivity for each get_queryset, since icontains relies on DB collation and can differ across backends (SQLite vs MySQL vs Postgres).

Summary

Change	Verdict
icontains	✅ Recommended
Remove useless f prefix	✅ Correct
is_resource_filter = False	⚠️ Verify intent + add a test
Content-Type / duplication	💡 Optimize separately

[shaohuzhang1]

Here are the issues and suggestions for the code:

Potential Issues:

1. Inconsistent return behavior: The function now can return False (when raise_exception=False) or implicitly return True at the end, but in earlier paths it may raise exceptions. This is fine, but ensure callers handle both cases.

2. Exception re-raise logic order:

   • If e is AppApiException, it re-raises as-is.
   • Otherwise, if raise_exception=True, it wraps in a new AppApiException.
   • However, the original exception loses its traceback context. Consider using raise ... from e to preserve the chain:

     raise AppApiException(...) from e

3. Swallowing exceptions silently: When raise_exception=False, the exception is logged (with exc_info=True) but conversion to return False hides details. If callers rely only on boolean result, this is fine, but no error message reaches them.

4. Logging already happens before branching: Since maxkb_logger.error(...) runs for all cases, wrapped exceptions will be double-logged at the caller level. Acceptable, but note it.

5. is_valid returns True unconditionally at the end — this means any exception path with raise_exception=False returns False, while success returns True. Just confirm this matches the intended contract (could there be other validation failures that should also return False instead of raising?).

Optimization Suggestions:

1. Preserve the exception chain:

   raise AppApiException(...) from e

2. Consider validating the type of raise_exception early (it's assumed boolean). Minor.

3. The error message template uses .format(error=str(e)) — fine, but ensure gettext string has matching {error} placeholder in all translations.

4. If AppApiException is a subclass of a general exception used elsewhere, ensure the isinstance check doesn't accidentally catch unrelated subclasses you want to wrap.

5. Consider whether return False should also happen when raise_exception=False for the AppApiException branch — currently it always re-raises AppApiException regardless of raise_exception. Confirm that's intentional (likely yes, since it's a domain-specific error).

Summary: Main fix is adding from e to preserve traceback; otherwise the logic change looks reasonable assuming intended semantics.