Skip to content

fix: When retrieving the model list with multiple keys, select the first enabled one. - #2260

Merged
Calcium-Ion merged 1 commit into
QuantumNous:mainfrom
seefs001:fix/multi-key-fetch-models
Nov 20, 2025
Merged

fix: When retrieving the model list with multiple keys, select the first enabled one.#2260
Calcium-Ion merged 1 commit into
QuantumNous:mainfrom
seefs001:fix/multi-key-fetch-models

Conversation

@seefs001

@seefs001 seefs001 commented Nov 20, 2025

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Bug Fixes
    • Improved key retrieval and validation with enhanced error handling. The system now properly manages key selection and provides clearer error responses when key retrieval fails.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Nov 20, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Refactored channel key retrieval in controller/channel.go to use the centralized GetNextEnabledKey() method instead of direct access, with added error handling to return JSON error responses when key retrieval fails.

Changes

Cohort / File(s) Summary
Key retrieval refactor
controller/channel.go
Replaced direct channel.Key extraction with channel.GetNextEnabledKey() method call; added error handling to return JSON failure response on retrieval error; removed previous string splitting logic; whitespace trimmed from retrieved key.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Single file modification with straightforward refactoring pattern
  • Error handling addition is minimal and follows standard practice
  • Primary focus: verify that GetNextEnabledKey() method signature matches expectations and error case handling is appropriate

Possibly related PRs

Poem

🐰 A key retrieval quest, once wild and free,

Now tamed through methods, centralized to be!

With errors caught and handled with care,

The channel hops forth with confidence rare! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: replacing direct key extraction with a call to GetNextEnabledKey() to select the first enabled key when multiple keys are available.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
controller/channel.go (1)

206-216: Multi-key selection via GetNextEnabledKey looks correct; consider a couple of hardening tweaks

The switch to channel.GetNextEnabledKey() plus trimming the returned key achieves the PR goal: multi-key channels will use the first enabled key instead of blindly taking the first line, while single-key channels should behave as before. Error handling with a JSON {success:false, message: ...} response is consistent with nearby code paths.

Two small, non-blocking suggestions:

  1. Guard against unexpected empty keys
    If GetNextEnabledKey can ever return "" with a nil error (e.g., future refactor), you’d end up sending Authorization: Bearer upstream. A cheap defensive check here avoids a confusing 401 from the provider:
    key = strings.TrimSpace(key)
  • if key == "" {
  •  c.JSON(http.StatusOK, gin.H{
    
  •      "success": false,
    
  •      "message": "获取渠道密钥失败: 没有可用的密钥",
    
  •  })
    
  •  return
    
  • }
    
    
  1. Ensure error messages don’t leak key material
    Please double‑check that apiErr.Error() from GetNextEnabledKey never embeds the raw key or a large prefix of it. If it might, consider wrapping it in a more generic message here instead of interpolating the full error text.

These are polish items; the core behavior change looks good and aligns with the multi‑key design in the rest of this file.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b899122 and 33b1fad.

📒 Files selected for processing (1)
  • controller/channel.go (1 hunks)

@Calcium-Ion
Calcium-Ion merged commit ef06472 into QuantumNous:main Nov 20, 2025
1 check passed
ennnnny pushed a commit to ennnnny/new-api that referenced this pull request Mar 17, 2026
…-models

fix: When retrieving the model list with multiple keys, select the first enabled one.
@coderabbitai coderabbitai Bot mentioned this pull request May 21, 2026
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants