fix: allow clearing search provider API key #482
Conversation
Introduces `update_api_key` flag in save request to distinguish between intentional clearing and preserving existing key. Adds `apiKeyChanged` tracking in frontend to set this flag. Adds backend unit tests for settings controller.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
WalkthroughThis change introduces conditional API key handling for search provider configuration. A new request wrapper carries an Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend<br/>(Vue Component)
participant Backend as Backend<br/>(Controller)
participant Database as Database<br/>(DAO)
User->>Frontend: Modifies API key field
Frontend->>Frontend: Sets apiKeyChanged = true
User->>Frontend: Clicks save
Frontend->>Backend: POST /save {UpdateAPIKey: true, APIKey: "new-key"}
Backend->>Database: Query existing config
Database-->>Backend: Return current config
Backend->>Backend: Update APIKey (UpdateAPIKey = true)
Backend->>Database: Write updated config
Database-->>Backend: Confirm write
Backend-->>Frontend: Return 200 success
Frontend->>Frontend: Reload config
Frontend-->>User: Display updated settings
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/settings_test.go`:
- Around line 27-32: Replace the plain os.Setenv calls with test-scoped
environment cleanup: use t.Setenv("DB_SQLITE_PATH", tmpDir) and
t.Setenv("FC_DB_SQLITE_PATH", tmpDir) (or alternatively save original values and
defer restoring them) before calling util.GetDatabase(); keep the rest of the
test (including db.AutoMigrate(&dao.SystemSetting{})) unchanged so the
environment variables are cleaned up and won't pollute other tests.
🧹 Nitpick comments (3)
internal/controller/settings_test.go (3)
85-88: Error return values fromdao.GetJsonSettingare ignored in verification steps.While the test will likely fail on the subsequent assertion if there's an error, explicitly checking errors improves debuggability.
Proposed fix for one instance (apply similarly to others)
- dao.GetJsonSetting(db, constant.KeySearchProviderConfig, &savedCfg) + if err := dao.GetJsonSetting(db, constant.KeySearchProviderConfig, &savedCfg); err != nil { + t.Fatalf("Failed to get setting: %v", err) + }Also applies to: 103-106, 121-124, 139-142
19-143: Consider table-driven tests to reduce complexity and improve maintainability.The static analysis flagged this function for having 101 lines and cyclomatic complexity of 14. While the current structure is readable, a table-driven approach would consolidate the test cases and make it easier to add new scenarios.
Example table-driven structure
func TestSaveSearchProviderConfig(t *testing.T) { // ... setup code ... tests := []struct { name string req SearchProviderConfigRequest expectedAPIKey string }{ { name: "Initial Save with API Key", req: SearchProviderConfigRequest{ SearchProviderConfig: config.SearchProviderConfig{ Provider: "litellm", APIKey: "initial-key", APIUrl: "http://example.com", }, UpdateAPIKey: true, }, expectedAPIKey: "initial-key", }, // ... other test cases ... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { w := makeRequest(tt.req) if w.Code != http.StatusOK { t.Errorf("Expected 200, got %d", w.Code) } var savedCfg config.SearchProviderConfig if err := dao.GetJsonSetting(db, constant.KeySearchProviderConfig, &savedCfg); err != nil { t.Fatalf("Failed to get setting: %v", err) } if savedCfg.APIKey != tt.expectedAPIKey { t.Errorf("Expected %s, got %s", tt.expectedAPIKey, savedCfg.APIKey) } }) } }
29-29: Singleton database pattern could cause test isolation issues if additional tests are added.
util.GetDatabase()usessync.Oncefor initialization, so any subsequent tests in the same test file that call it will reuse the same database instance. While this file currently contains only one test, adding more tests tosettings_test.goor other test files could cause interference and flaky tests.Consider using a test-scoped database instance or resetting the singleton between tests if the test suite expands.
| os.Setenv("DB_SQLITE_PATH", tmpDir) | ||
| os.Setenv("FC_DB_SQLITE_PATH", tmpDir) | ||
| db := util.GetDatabase() | ||
| if err := db.AutoMigrate(&dao.SystemSetting{}); err != nil { | ||
| t.Fatalf("Failed to migrate: %v", err) | ||
| } |
There was a problem hiding this comment.
Environment variables should be cleaned up to avoid test pollution.
Using os.Setenv without t.Setenv (Go 1.17+) or manual cleanup with defer can pollute other tests running in parallel.
Proposed fix using t.Setenv
- os.Setenv("DB_SQLITE_PATH", tmpDir)
- os.Setenv("FC_DB_SQLITE_PATH", tmpDir)
+ t.Setenv("DB_SQLITE_PATH", tmpDir)
+ t.Setenv("FC_DB_SQLITE_PATH", tmpDir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.Setenv("DB_SQLITE_PATH", tmpDir) | |
| os.Setenv("FC_DB_SQLITE_PATH", tmpDir) | |
| db := util.GetDatabase() | |
| if err := db.AutoMigrate(&dao.SystemSetting{}); err != nil { | |
| t.Fatalf("Failed to migrate: %v", err) | |
| } | |
| t.Setenv("DB_SQLITE_PATH", tmpDir) | |
| t.Setenv("FC_DB_SQLITE_PATH", tmpDir) | |
| db := util.GetDatabase() | |
| if err := db.AutoMigrate(&dao.SystemSetting{}); err != nil { | |
| t.Fatalf("Failed to migrate: %v", err) | |
| } |
🤖 Prompt for AI Agents
In `@internal/controller/settings_test.go` around lines 27 - 32, Replace the plain
os.Setenv calls with test-scoped environment cleanup: use
t.Setenv("DB_SQLITE_PATH", tmpDir) and t.Setenv("FC_DB_SQLITE_PATH", tmpDir) (or
alternatively save original values and defer restoring them) before calling
util.GetDatabase(); keep the rest of the test (including
db.AutoMigrate(&dao.SystemSetting{})) unchanged so the environment variables are
cleaned up and won't pollute other tests.
User description
This PR fixes an issue where clearing the Search Provider API key in the settings was impossible because an empty value was interpreted as "preserve existing key".
Changes:
SaveSearchProviderConfigandCheckSearchProviderConfigto accept aSearchProviderConfigRequeststruct which includes anUpdateAPIKeyboolean.UpdateAPIKeyis false and the provided key is empty.update_api_key: truewhen modified.internal/controller/settings_test.goto verify the fix.PR created automatically by Jules for task 3120112355790831627 started by @Colin-XKL
PR Type
Bug fix
Description
Introduces
update_api_keyflag to distinguish intentional clearing from preserving existing API keysFrontend tracks API key input changes and sends flag when modified
Backend logic updated to only preserve key if flag is false and key is empty
Comprehensive unit tests added for settings controller validation
Diagram Walkthrough
File Walkthrough
index.vue
Track API key changes and send update flagweb/admin/src/views/settings/search_provider/index.vue
apiKeyChangedref to track when API key input is modified@inputevent listener on password field to setapiKeyChangedflag
handleSaveto includeupdate_api_keyflag in POST requesthandleCheckConnectionto includeupdate_api_keyflag in checkrequest
apiKeyChangedflag after successful config loadloadConfig()after successful save to refresh statesettings.go
Add update flag to distinguish key clearing intentinternal/controller/settings.go
SearchProviderConfigRequeststruct withUpdateAPIKeybooleanfield
SaveSearchProviderConfigto useSearchProviderConfigRequestinstead of direct config
CheckSearchProviderConfigto useSearchProviderConfigRequestinstead of direct config
!req.UpdateAPIKey &&req.APIKey == ""conditionsettings_test.go
Add unit tests for search provider config logicinternal/controller/settings_test.go
UpdateAPIKey=falseand key is emptyUpdateAPIKey=trueand key is emptyUpdateAPIKey=true