Open
Conversation
…guoruqiang/new-api into feature/topup-auto-switch-group
Contributor
WalkthroughImplements a payment auto-switch group feature: new API and option normalization for auto-switch rules, persistent payment_setting batch updates, top-up completion flow that applies auto-switch group logic, subscription group-resolution changes, frontend rule editor/UI, and comprehensive tests for auto-switch behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Admin
participant Frontend as Frontend UI
participant API as Controller
participant Model as Model Layer
participant PaymentSetting as PaymentSetting<br/>(Operation Config)
participant DB as Database
User->>Frontend: Configure auto-switch rules<br/>(base group, thresholds)
Frontend->>Frontend: Validate rules & convert<br/>display unit → USD
User->>Frontend: Submit configuration
Frontend->>API: PUT /api/option/payment_auto_switch_group
API->>API: Normalize & validate request<br/>(base group, rules, thresholds)
API->>Model: UpdateOptions(map of<br/>payment_setting.* keys)
Model->>DB: Transaction: Upsert all<br/>option key/values
DB-->>Model: Options saved
Model->>PaymentSetting: applyPaymentSettingOptionValues
PaymentSetting->>PaymentSetting: UpdatePaymentSetting(mutator)<br/>- Lock & update struct<br/>- Normalize base_group<br/>- Return deep copy
Model->>Model: Update in-memory OptionMap for payment_setting.* keys
Model-->>API: Success
API-->>Frontend: 200 OK
sequenceDiagram
participant User as User Performs<br/>Top-up
participant API as Controller
participant Model as Model Layer
participant PaymentSetting as PaymentSetting<br/>(Config)
participant DB as Database
participant UserModel as User Model
User->>API: Submit top-up (Epay)
API->>Model: RechargeEpay(tradeNo)
Model->>Model: completeTopUpByTradeNo()<br/>- Lock row (FOR UPDATE)
Model->>DB: Mark top-up as Success
Model->>DB: Update User quota/balance
Model->>PaymentSetting: GetPaymentSetting()
PaymentSetting-->>Model: Auto-switch config & rules
Model->>Model: NormalizeTopUpValueUSD()
Model->>Model: GetUserSuccessfulTopupTotalUSDTx()
Model->>Model: Match total vs rules → target group
alt in controlled chain and no active subscription override
Model->>UserModel: Update User.Group = target group
Model->>DB: Save User changes
end
Model-->>API: CompleteTopUp returns
API->>API: Update cache if group changed
API-->>User: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
setting/operation_setting/payment_setting.go (1)
43-60: Extract the deep-copy logic into one helper.The clone path is duplicated in both accessors. If
PaymentSettinggains another slice/map field later, it is easy to update one copy site and miss the other, which would quietly reintroduce shared mutable state.♻️ Suggested refactor
+func clonePaymentSetting(src PaymentSetting) PaymentSetting { + cloned := src + if src.AmountOptions != nil { + cloned.AmountOptions = append([]int(nil), src.AmountOptions...) + } + if src.AmountDiscount != nil { + cloned.AmountDiscount = make(map[int]float64, len(src.AmountDiscount)) + for amount, discount := range src.AmountDiscount { + cloned.AmountDiscount[amount] = discount + } + } + if src.AutoSwitchGroupRules != nil { + cloned.AutoSwitchGroupRules = append([]PaymentAutoSwitchGroupRule(nil), src.AutoSwitchGroupRules...) + } + return cloned +} + func GetPaymentSetting() PaymentSetting { paymentSettingRWMutex.RLock() defer paymentSettingRWMutex.RUnlock() - - copiedSetting := paymentSetting - if paymentSetting.AmountOptions != nil { - copiedSetting.AmountOptions = append([]int(nil), paymentSetting.AmountOptions...) - } - if paymentSetting.AmountDiscount != nil { - copiedSetting.AmountDiscount = make(map[int]float64, len(paymentSetting.AmountDiscount)) - for amount, discount := range paymentSetting.AmountDiscount { - copiedSetting.AmountDiscount[amount] = discount - } - } - if paymentSetting.AutoSwitchGroupRules != nil { - copiedSetting.AutoSwitchGroupRules = append([]PaymentAutoSwitchGroupRule(nil), paymentSetting.AutoSwitchGroupRules...) - } - return copiedSetting + return clonePaymentSetting(paymentSetting) }Also applies to: 78-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setting/operation_setting/payment_setting.go` around lines 43 - 60, Extract the deep-copy code into a single helper function (e.g., ClonePaymentSetting or copyPaymentSetting) that accepts a PaymentSetting and returns a fully deep-copied PaymentSetting; move the existing slice/map copy logic (AmountOptions, AmountDiscount, AutoSwitchGroupRules and any future slice/map fields) into that helper, then update GetPaymentSetting to call the helper while retaining the paymentSettingRWMutex.RLock/RUnlock; do the same for the other accessor that duplicates this logic so both call the new helper to avoid divergent copy implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/option.go`:
- Around line 221-247: The current DB.Transaction block (DB.Transaction) commits
before in-memory validation and application (updateOptionMap and
applyPaymentSettingOptionValues), which can leave partial updates if validation
fails; move the validation/application logic inside the same transaction so the
whole multi-key update is atomic: perform updateOptionMap checks and
collect/validate paymentSettingOptionValues within the transaction callback
(alongside FirstOrCreate/Save for Option), and call
applyPaymentSettingOptionValues from inside that transaction so any error causes
tx rollback and is returned to the caller; ensure
applyPaymentSettingOptionValues returns/propagates errors so DB.Transaction can
roll back.
- Around line 260-272: The code stores the raw input for
auto_switch_group_base_group into common.OptionMap instead of the normalized
value produced via UpdatePaymentSetting; update the flow so the normalized
base-group is written back to common.OptionMap (and ideally normalized before
persisting to DB) by retrieving the normalized value from the PaymentSetting
after operation_setting.UpdatePaymentSetting (or from
paymentSettingConfigMap/UpdateConfigFromMap result) and assigning that
normalized string to common.OptionMap["auto_switch_group_base_group"] instead of
the original raw value; adjust code around
operation_setting.UpdatePaymentSetting, paymentSettingConfigMap,
UpdateConfigFromMap, and common.OptionMap to ensure storage, runtime and UI all
use the same normalized value.
In `@model/topup.go`:
- Around line 616-617: Replace the inconsistent logger call in the RecordLog
invocation: currently it uses logger.LogQuota(...) but other recharge paths use
logger.FormatQuota(...); update the RecordLog call that references topUp.UserId,
LogTypeTopup and topUp.Money to call logger.FormatQuota(quotaToAdd) instead of
logger.LogQuota(quotaToAdd) so the log formatting is consistent with the other
recharge flows.
- Around line 293-315: The transaction only locks the top-up row and can race
when multiple top-ups for the same user commit concurrently; before calling
completeTopUpTx(tx, &topUp, userUpdates) acquire a per-user lock to serialize
completions for that user (e.g. a DB advisory lock or SELECT ... FOR UPDATE on
the users row) using the user ID from topUp, hold the lock for the duration of
the transaction, then call completeTopUpTx and release the lock at transaction
end; update the code around the transaction block that references topUp, tx,
userUpdatesBuilder and completeTopUpTx to acquire and release this per-user lock
so recomputation of cumulative tiers and writes to users.group are serialized.
- Around line 294-295: The SELECT using a FOR UPDATE lock
(tx.Set("gorm:query_option", "FOR UPDATE").Where(...).First(&topUp)) is
incompatible with SQLite; guard it by checking common.UsingSQLite and only apply
the FOR UPDATE option when not using SQLite (i.e., if !common.UsingSQLite {
tx.Set("gorm:query_option", "FOR UPDATE")... } ), otherwise perform the plain
SELECT and rely on the existing manual rollback/serial approach used in
model/checkin.go; apply the same pattern to the similar usages in
model/subscription.go, model/user.go, and model/redemption.go to ensure
cross-database compatibility.
In `@setting/operation_setting/payment_setting.go`:
- Line 25: paymentSetting is subject to a data race because ExportAllConfigs →
ConfigToMap uses reflection to read struct fields without locking
paymentSettingRWMutex while UpdatePaymentSetting writes to AmountDiscount and
AutoSwitchGroupRules; also deep-copy logic is duplicated in GetPaymentSetting
and UpdatePaymentSetting. Fix by centralizing safe copying and ensuring
reflections read the protected copy: add a helper function (e.g.,
clonePaymentSetting or copyPaymentSetting) that takes the lock, makes a deep
copy of paymentSetting (including proper deep copies of AmountDiscount and
AutoSwitchGroupRules), and returns it; replace the duplicated deep-copy code in
GetPaymentSetting and UpdatePaymentSetting with calls to this helper; change
ExportAllConfigs/ConfigToMap to call GetPaymentSetting or clonePaymentSetting
(so reflection operates on an immutable copy) instead of directly reflecting
over paymentSetting.
In `@web/src/components/settings/PaymentSetting.jsx`:
- Around line 109-115: The JSON.parse of item.value that assigns to
newInputs['AutoSwitchGroupRules'] may return non-array values; after parsing
inside the try block (the code that sets newInputs['AutoSwitchGroupRules']),
validate the parsed result with Array.isArray and only assign it if true,
otherwise set newInputs['AutoSwitchGroupRules'] = []; keep the existing catch to
handle parse errors but add the Array.isArray check for the parsed value to
prevent non-array objects from being stored.
In `@web/src/i18n/locales/fr.json`:
- Around line 158-171: Add the missing French translation for the new error key
used in SettingsPaymentGateway.jsx: add an entry for the Chinese key
"分组加载失败，请刷新后重试" to web/src/i18n/locales/fr.json with an appropriate French
message (e.g., "Échec du chargement des groupes, veuillez actualiser et
réessayer") so t('分组加载失败，请刷新后重试') returns a French string instead of falling
back to Chinese.
In `@web/src/pages/Setting/Payment/SettingsPaymentGateway.jsx`:
- Around line 339-349: handleFormChange currently normalizes inputs into React
state only, leaving the Semi Form store stale; update the form store with the
same normalizedValues when a change is accepted. Inside handleFormChange (after
computing normalizedValues and confirming compareObjects(prev,
normalizedValues).length !== 0), call the Semi Form API to set the form values
(e.g., formApi.setValues or formRef.current.setValues) with normalizedValues so
the form store and React state stay in sync, taking care to only call the form
setter when values actually changed to avoid infinite loops.
---
Nitpick comments:
In `@setting/operation_setting/payment_setting.go`:
- Around line 43-60: Extract the deep-copy code into a single helper function
(e.g., ClonePaymentSetting or copyPaymentSetting) that accepts a PaymentSetting
and returns a fully deep-copied PaymentSetting; move the existing slice/map copy
logic (AmountOptions, AmountDiscount, AutoSwitchGroupRules and any future
slice/map fields) into that helper, then update GetPaymentSetting to call the
helper while retaining the paymentSettingRWMutex.RLock/RUnlock; do the same for
the other accessor that duplicates this logic so both call the new helper to
avoid divergent copy implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e558997a-a6bd-4544-ac15-047bb355905c
📒 Files selected for processing (17)
controller/option.gocontroller/topup.gomodel/option.gomodel/subscription.gomodel/topup.gomodel/topup_auto_switch_test.gorouter/api-router.gosetting/operation_setting/payment_setting.goweb/src/components/settings/PaymentSetting.jsxweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh-CN.jsonweb/src/i18n/locales/zh-TW.jsonweb/src/pages/Setting/Payment/SettingsPaymentGateway.jsx
Comment on lines
+221
to
+247
| if err := DB.Transaction(func(tx *gorm.DB) error { | ||
| for _, key := range keys { | ||
| option := Option{Key: key} | ||
| if err := tx.FirstOrCreate(&option, Option{Key: key}).Error; err != nil { | ||
| return err | ||
| } | ||
| option.Value = optionValues[key] | ||
| if err := tx.Save(&option).Error; err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| }); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| paymentSettingOptionValues := make(map[string]string) | ||
| for _, key := range keys { | ||
| if strings.HasPrefix(key, "payment_setting.") { | ||
| paymentSettingOptionValues[key] = optionValues[key] | ||
| continue | ||
| } | ||
| if err := updateOptionMap(key, optionValues[key]); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return applyPaymentSettingOptionValues(paymentSettingOptionValues) |
Contributor
There was a problem hiding this comment.
UpdateOptions can return an error after the batch is already committed.
Line 221 commits the transaction before Lines 243-247 validate and apply the values in memory. If a later key is malformed, the caller gets a failure after some rows are already saved and earlier globals may already be mutated, so multi-key updates lose all-or-nothing semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/option.go` around lines 221 - 247, The current DB.Transaction block
(DB.Transaction) commits before in-memory validation and application
(updateOptionMap and applyPaymentSettingOptionValues), which can leave partial
updates if validation fails; move the validation/application logic inside the
same transaction so the whole multi-key update is atomic: perform
updateOptionMap checks and collect/validate paymentSettingOptionValues within
the transaction callback (alongside FirstOrCreate/Save for Option), and call
applyPaymentSettingOptionValues from inside that transaction so any error causes
tx rollback and is returned to the caller; ensure
applyPaymentSettingOptionValues returns/propagates errors so DB.Transaction can
roll back.
Comment on lines
+260
to
272
| var paymentSettingErr error | ||
| operation_setting.UpdatePaymentSetting(func(setting *operation_setting.PaymentSetting) { | ||
| paymentSettingErr = config.UpdateConfigFromMap(setting, paymentSettingConfigMap) | ||
| }) | ||
| if paymentSettingErr != nil { | ||
| return paymentSettingErr | ||
| } | ||
|
|
||
| common.OptionMapRWMutex.Lock() | ||
| defer common.OptionMapRWMutex.Unlock() | ||
| for key, value := range optionValues { | ||
| common.OptionMap[key] = value | ||
| } |
Contributor
There was a problem hiding this comment.
Write back the normalized base-group value, not the raw input.
Lines 261-263 normalize auto_switch_group_base_group through UpdatePaymentSetting, but Line 271 still stores the pre-normalized string in common.OptionMap. Saving "" or whitespace makes runtime behavior fall back to "default" while the config API/UI reads back blank, so operators see a different value from the one actually enforced.
🛠️ Focused fix
- var paymentSettingErr error
- operation_setting.UpdatePaymentSetting(func(setting *operation_setting.PaymentSetting) {
+ var paymentSettingErr error
+ normalizedPaymentSetting := operation_setting.UpdatePaymentSetting(func(setting *operation_setting.PaymentSetting) {
paymentSettingErr = config.UpdateConfigFromMap(setting, paymentSettingConfigMap)
})
if paymentSettingErr != nil {
return paymentSettingErr
}
@@
common.OptionMapRWMutex.Lock()
defer common.OptionMapRWMutex.Unlock()
for key, value := range optionValues {
+ if key == "payment_setting.auto_switch_group_base_group" {
+ common.OptionMap[key] = normalizedPaymentSetting.AutoSwitchGroupBaseGroup
+ continue
+ }
common.OptionMap[key] = value
}At minimum, mirror the normalized value into OptionMap; ideally normalize before the DB write too so storage, runtime, and UI stay consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/option.go` around lines 260 - 272, The code stores the raw input for
auto_switch_group_base_group into common.OptionMap instead of the normalized
value produced via UpdatePaymentSetting; update the flow so the normalized
base-group is written back to common.OptionMap (and ideally normalized before
persisting to DB) by retrieving the normalized value from the PaymentSetting
after operation_setting.UpdatePaymentSetting (or from
paymentSettingConfigMap/UpdateConfigFromMap result) and assigning that
normalized string to common.OptionMap["auto_switch_group_base_group"] instead of
the original raw value; adjust code around
operation_setting.UpdatePaymentSetting, paymentSettingConfigMap,
UpdateConfigFromMap, and common.OptionMap to ensure storage, runtime and UI all
use the same normalized value.
Comment on lines
+293
to
315
| err := DB.Transaction(func(tx *gorm.DB) error { | ||
| if err := tx.Set("gorm:query_option", "FOR UPDATE").Where(refCol+" = ?", tradeNo).First(&topUp).Error; err != nil { | ||
| return errors.New("充值订单不存在") | ||
| } | ||
|
|
||
| if topUp.Status == common.TopUpStatusSuccess { | ||
| return nil | ||
| } | ||
| if topUp.Status != common.TopUpStatusPending { | ||
| return errors.New("充值订单状态错误") | ||
| } | ||
|
|
||
| topUp.CompleteTime = common.GetTimestamp() | ||
| topUp.Status = common.TopUpStatusSuccess | ||
| err = tx.Save(topUp).Error | ||
| if err != nil { | ||
| return err | ||
| userUpdates := map[string]interface{}{} | ||
| if userUpdatesBuilder != nil { | ||
| updates, err := userUpdatesBuilder(&topUp, tx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| userUpdates = updates | ||
| } | ||
|
|
||
| quota = topUp.Money * common.QuotaPerUnit | ||
| err = tx.Model(&User{}).Where("id = ?", topUp.UserId).Updates(map[string]interface{}{"stripe_customer": customerId, "quota": gorm.Expr("quota + ?", quota)}).Error | ||
| var err error | ||
| switchedGroup, err = completeTopUpTx(tx, &topUp, userUpdates) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
Serialize completions per user before recomputing cumulative tiers.
This transaction only locks the current top-up row. If two different top-ups for the same user finish at the same time, each aggregation can miss the other's uncommitted success and both writes can leave the user at the lower matched group. Add a per-user lock/mutex before completeTopUpTx() recomputes the total and updates users.group.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/topup.go` around lines 293 - 315, The transaction only locks the top-up
row and can race when multiple top-ups for the same user commit concurrently;
before calling completeTopUpTx(tx, &topUp, userUpdates) acquire a per-user lock
to serialize completions for that user (e.g. a DB advisory lock or SELECT ...
FOR UPDATE on the users row) using the user ID from topUp, hold the lock for the
duration of the transaction, then call completeTopUpTx and release the lock at
transaction end; update the code around the transaction block that references
topUp, tx, userUpdatesBuilder and completeTopUpTx to acquire and release this
per-user lock so recomputation of cumulative tiers and writes to users.group are
serialized.
Comment on lines
+294
to
295
| if err := tx.Set("gorm:query_option", "FOR UPDATE").Where(refCol+" = ?", tradeNo).First(&topUp).Error; err != nil { | ||
| return errors.New("充值订单不存在") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'gorm:query_option|FOR UPDATE|UsingSQLite' model/topup.go modelRepository: QuantumNous/new-api
Length of output: 7178
Add SQLite compatibility guard for FOR UPDATE lock.
FOR UPDATE is not supported in SQLite. The code at line 294 will fail at runtime when using SQLite. Add a check like if !common.UsingSQLite to either skip locking for SQLite or use an alternative mechanism (e.g., sequential operations with manual rollback as done in model/checkin.go).
Note: Similar unsupported FOR UPDATE patterns exist in model/subscription.go, model/user.go, and model/redemption.go — consider a comprehensive fix across all model files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/topup.go` around lines 294 - 295, The SELECT using a FOR UPDATE lock
(tx.Set("gorm:query_option", "FOR UPDATE").Where(...).First(&topUp)) is
incompatible with SQLite; guard it by checking common.UsingSQLite and only apply
the FOR UPDATE option when not using SQLite (i.e., if !common.UsingSQLite {
tx.Set("gorm:query_option", "FOR UPDATE")... } ), otherwise perform the plain
SELECT and rely on the existing manual rollback/serial approach used in
model/checkin.go; apply the same pattern to the similar usages in
model/subscription.go, model/user.go, and model/redemption.go to ensure
cross-database compatibility.
Comment on lines
+616
to
+617
| RecordLog(topUp.UserId, LogTypeTopup, fmt.Sprintf("使用在线充值成功，充值金额: %v，支付金额：%f", logger.LogQuota(quotaToAdd), topUp.Money)) | ||
| return topUp, true, nil |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'func (LogQuota|FormatQuota)|\bLogQuota\(|\bFormatQuota\('Repository: QuantumNous/new-api
Length of output: 4458
Align logging function with other recharge paths in this file.
Line 616 uses logger.LogQuota(...) while other recharge operations in model/topup.go (lines 350, 519, 587) use logger.FormatQuota(...). Both functions are exported, but align this call with the established pattern for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/topup.go` around lines 616 - 617, Replace the inconsistent logger call
in the RecordLog invocation: currently it uses logger.LogQuota(...) but other
recharge paths use logger.FormatQuota(...); update the RecordLog call that
references topUp.UserId, LogTypeTopup and topUp.Money to call
logger.FormatQuota(quotaToAdd) instead of logger.LogQuota(quotaToAdd) so the log
formatting is consistent with the other recharge flows.
| AutoSwitchGroupRules []PaymentAutoSwitchGroupRule `json:"auto_switch_group_rules"` | ||
| } | ||
|
|
||
| var paymentSettingRWMutex sync.RWMutex |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether payment_setting is ever accessed outside the wrapper helpers.
# Any direct registry read/write path is a mutex bypass and needs review.
rg -n -C2 --type=go 'Register\("payment_setting"|Get\("payment_setting"\)|ExportAllConfigs\('Repository: QuantumNous/new-api
Length of output: 931
🏁 Script executed:
#!/bin/bash
# Examine config.go to understand how Register and ExportAllConfigs work
fd -e go "config.go" | xargs wc -l | sort -nRepository: QuantumNous/new-api
Length of output: 291
🏁 Script executed:
# Find and examine the Register method in config.go
rg -A 15 "func \(cm \*ConfigManager\) Register" --type=goRepository: QuantumNous/new-api
Length of output: 858
🏁 Script executed:
# Find and examine the ExportAllConfigs method implementation
rg -A 20 "func \(cm \*ConfigManager\) ExportAllConfigs" --type=goRepository: QuantumNous/new-api
Length of output: 939
🏁 Script executed:
# Check for direct field accesses to paymentSetting fields (AmountDiscount, AutoSwitchGroupRules)
rg -n "AmountDiscount|AutoSwitchGroupRules" --type=go | head -20Repository: QuantumNous/new-api
Length of output: 2456
🏁 Script executed:
# Find ConfigToMap implementation
rg -A 15 "func ConfigToMap" --type=goRepository: QuantumNous/new-api
Length of output: 912
🏁 Script executed:
# Examine the context of model/topup.go:88
sed -n '80,95p' model/topup.goRepository: QuantumNous/new-api
Length of output: 641
🏁 Script executed:
# Check if model/topup.go has any synchronization around these accesses
rg -B 5 -A 5 "paymentSetting.AutoSwitchGroupRules" model/topup.goRepository: QuantumNous/new-api
Length of output: 895
🏁 Script executed:
# Find all callers of buildPaymentAutoSwitchGroupChainSet
rg -B 10 "buildPaymentAutoSwitchGroupChainSet" --type=go | head -40Repository: QuantumNous/new-api
Length of output: 1433
🏁 Script executed:
# Find the private configToMap implementation
rg -A 30 "func configToMap" --type=go | head -50Repository: QuantumNous/new-api
Length of output: 1429
The mutex doesn't fully protect paymentSetting from concurrent access.
While GetPaymentSetting() and UpdatePaymentSetting() are protected by paymentSettingRWMutex, the struct is also accessed via ExportAllConfigs() → ConfigToMap() which uses reflection to read struct fields directly without holding the mutex. This creates a data race: concurrent ConfigToMap reflection reads can race against UpdatePaymentSetting writes to AmountDiscount and AutoSwitchGroupRules.
Additionally, the deep-copy logic for these two fields is duplicated in both GetPaymentSetting() (lines 51–58) and UpdatePaymentSetting() (lines 82–89), increasing maintenance risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@setting/operation_setting/payment_setting.go` at line 25, paymentSetting is
subject to a data race because ExportAllConfigs → ConfigToMap uses reflection to
read struct fields without locking paymentSettingRWMutex while
UpdatePaymentSetting writes to AmountDiscount and AutoSwitchGroupRules; also
deep-copy logic is duplicated in GetPaymentSetting and UpdatePaymentSetting. Fix
by centralizing safe copying and ensuring reflections read the protected copy:
add a helper function (e.g., clonePaymentSetting or copyPaymentSetting) that
takes the lock, makes a deep copy of paymentSetting (including proper deep
copies of AmountDiscount and AutoSwitchGroupRules), and returns it; replace the
duplicated deep-copy code in GetPaymentSetting and UpdatePaymentSetting with
calls to this helper; change ExportAllConfigs/ConfigToMap to call
GetPaymentSetting or clonePaymentSetting (so reflection operates on an immutable
copy) instead of directly reflecting over paymentSetting.
Comment on lines
+109
to
+115
| try { | ||
| newInputs['AutoSwitchGroupRules'] = JSON.parse( | ||
| item.value || '[]', | ||
| ); | ||
| } catch (error) { | ||
| newInputs['AutoSwitchGroupRules'] = []; | ||
| } |
Contributor
There was a problem hiding this comment.
為 auto_switch_group_rules 增加陣列型別校驗。
Line 110 目前只做 JSON.parse，若值是合法但非陣列 JSON（如物件），會把非陣列寫入 AutoSwitchGroupRules，可能導致後續規則渲染/遍歷異常。
🔧 Proposed fix
case 'payment_setting.auto_switch_group_rules':
try {
- newInputs['AutoSwitchGroupRules'] = JSON.parse(
- item.value || '[]',
- );
+ const parsed = JSON.parse(item.value || '[]');
+ newInputs['AutoSwitchGroupRules'] = Array.isArray(parsed)
+ ? parsed
+ : [];
} catch (error) {
newInputs['AutoSwitchGroupRules'] = [];
}
break;📝 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.
Suggested change
| try { | |
| newInputs['AutoSwitchGroupRules'] = JSON.parse( | |
| item.value || '[]', | |
| ); | |
| } catch (error) { | |
| newInputs['AutoSwitchGroupRules'] = []; | |
| } | |
| try { | |
| const parsed = JSON.parse(item.value || '[]'); | |
| newInputs['AutoSwitchGroupRules'] = Array.isArray(parsed) | |
| ? parsed | |
| : []; | |
| } catch (error) { | |
| newInputs['AutoSwitchGroupRules'] = []; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/settings/PaymentSetting.jsx` around lines 109 - 115, The
JSON.parse of item.value that assigns to newInputs['AutoSwitchGroupRules'] may
return non-array values; after parsing inside the try block (the code that sets
newInputs['AutoSwitchGroupRules']), validate the parsed result with
Array.isArray and only assign it if true, otherwise set
newInputs['AutoSwitchGroupRules'] = []; keep the existing catch to handle parse
errors but add the Array.isArray check for the parsed value to prevent non-array
objects from being stored.
Comment on lines
339
to
+349
| const handleFormChange = (values) => { | ||
| setInputs(values); | ||
| setInputs((prev) => { | ||
| const normalizedValues = normalizeAutoSwitchGroupInputs({ | ||
| ...prev, | ||
| ...values, | ||
| }); | ||
| if (compareObjects(prev, normalizedValues).length === 0) { | ||
| return prev; | ||
| } | ||
| return normalizedValues; | ||
| }); |
Contributor
There was a problem hiding this comment.
Sync normalized values back into the form store.
normalizeAutoSwitchGroupInputs() only updates React state. Semi Form still keeps the old AutoSwitchGroupOnlyNewTopups value, so turning auto-switch off and then back on can resurrect a stale true and save the wrong mode.
🩹 One way to keep the form store and React state aligned
const handleFormChange = (values) => {
- setInputs((prev) => {
- const normalizedValues = normalizeAutoSwitchGroupInputs({
- ...prev,
- ...values,
- });
- if (compareObjects(prev, normalizedValues).length === 0) {
- return prev;
- }
- return normalizedValues;
- });
+ const mergedValues = {
+ ...inputs,
+ ...values,
+ };
+ const normalizedValues = normalizeAutoSwitchGroupInputs(mergedValues);
+ if (compareObjects(mergedValues, normalizedValues).length !== 0) {
+ formApiRef.current?.setValues(normalizedValues);
+ }
+ setInputs((prev) =>
+ compareObjects(prev, normalizedValues).length === 0
+ ? prev
+ : normalizedValues,
+ );
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/Setting/Payment/SettingsPaymentGateway.jsx` around lines 339 -
349, handleFormChange currently normalizes inputs into React state only, leaving
the Semi Form store stale; update the form store with the same normalizedValues
when a change is accepted. Inside handleFormChange (after computing
normalizedValues and confirming compareObjects(prev, normalizedValues).length
!== 0), call the Semi Form API to set the form values (e.g., formApi.setValues
or formRef.current.setValues) with normalizedValues so the form store and React
state stay in sync, taking care to only call the form setter when values
actually changed to avoid infinite loops.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
model/option.go (2)
281-292:⚠️ Potential issue | 🟠 MajorMirror normalized base-group into
OptionMap, not raw input.
UpdatePaymentSettingnormalizesauto_switch_group_base_group, butOptionMapstill stores the original raw value. This keeps API/UI-visible value inconsistent with runtime-enforced value.Focused fix
var paymentSettingErr error -operation_setting.UpdatePaymentSetting(func(setting *operation_setting.PaymentSetting) { +normalizedPaymentSetting := operation_setting.UpdatePaymentSetting(func(setting *operation_setting.PaymentSetting) { paymentSettingErr = config.UpdateConfigFromMap(setting, paymentSettingConfigMap) }) if paymentSettingErr != nil { return paymentSettingErr } common.OptionMapRWMutex.Lock() defer common.OptionMapRWMutex.Unlock() for key, value := range optionValues { + if key == "payment_setting.auto_switch_group_base_group" { + common.OptionMap[key] = normalizedPaymentSetting.AutoSwitchGroupBaseGroup + continue + } common.OptionMap[key] = value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/option.go` around lines 281 - 292, Update OptionMap to store the normalized auto_switch_group_base_group value instead of the raw input: inside the UpdatePaymentSetting closure (the func(setting *operation_setting.PaymentSetting) passed to operation_setting.UpdatePaymentSetting), capture the normalized field (e.g., setting.AutoSwitchGroupBaseGroup) into a local variable; after the closure completes and before writing optionValues into common.OptionMap (the loop using common.OptionMapRWMutex), overwrite optionValues["auto_switch_group_base_group"] with that captured normalized value so the value written into common.OptionMap matches the runtime-normalized payment setting.
229-253:⚠️ Potential issue | 🟠 Major
UpdateOptionscan still fail after DB commit (partial success path).The transaction commits before
updateOptionMap/applyPaymentSettingOptionValuesrun. If either later step fails, the caller receives an error after persistence has already succeeded.Suggested direction
- if err := DB.Transaction(func(tx *gorm.DB) error { + return DB.Transaction(func(tx *gorm.DB) error { for _, key := range keys { option := Option{Key: key} if err := tx.FirstOrCreate(&option, Option{Key: key}).Error; err != nil { return err } option.Value = optionValues[key] if err := tx.Save(&option).Error; err != nil { return err } } - return nil - }); err != nil { - return err - } - - for _, key := range keys { - if strings.HasPrefix(key, "payment_setting.") { - continue - } - if err := updateOptionMap(key, optionValues[key]); err != nil { - return err - } - } - return applyPaymentSettingOptionValues(paymentSettingOptionValues) + for _, key := range keys { + if strings.HasPrefix(key, "payment_setting.") { + continue + } + if err := updateOptionMap(key, optionValues[key]); err != nil { + return err + } + } + return applyPaymentSettingOptionValues(paymentSettingOptionValues) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/option.go` around lines 229 - 253, The current flow commits DB changes inside DB.Transaction then runs updateOptionMap and applyPaymentSettingOptionValues, causing possible partial success; to fix, perform the map update and payment-setting application inside the same DB.Transaction closure so any failure returns an error and triggers rollback: update the Transaction lambda that iterates keys (using Option, optionValues, paymentSettingOptionValues, keys) to call updateOptionMap(key, ...) for non-"payment_setting." keys and to call applyPaymentSettingOptionValues(...) before returning nil; if updateOptionMap or applyPaymentSettingOptionValues are non-DB operations, refactor them to return errors and make them safe to call inside the transaction (or add TX-aware variants) so failures propagate and the transaction can rollback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/option.go`:
- Around line 225-227: The current validatePaymentSettingOptionValues is too
permissive because it calls config.UpdateConfigFromMap which skips unknown
keys/parse errors; change validatePaymentSettingOptionValues to perform strict
decoding into the exact expected config struct (e.g., marshal the option map to
JSON/YAML and unmarshal with a decoder that DisallowUnknownFields or use a
strict decoder/validator) and return an error on any unknown keys or parse
failures instead of silently accepting them; replace the permissive
UpdateConfigFromMap usage with this strict decode+validate flow and apply the
same strict approach to the other similar validators referenced in the review.
---
Duplicate comments:
In `@model/option.go`:
- Around line 281-292: Update OptionMap to store the normalized
auto_switch_group_base_group value instead of the raw input: inside the
UpdatePaymentSetting closure (the func(setting
*operation_setting.PaymentSetting) passed to
operation_setting.UpdatePaymentSetting), capture the normalized field (e.g.,
setting.AutoSwitchGroupBaseGroup) into a local variable; after the closure
completes and before writing optionValues into common.OptionMap (the loop using
common.OptionMapRWMutex), overwrite optionValues["auto_switch_group_base_group"]
with that captured normalized value so the value written into common.OptionMap
matches the runtime-normalized payment setting.
- Around line 229-253: The current flow commits DB changes inside DB.Transaction
then runs updateOptionMap and applyPaymentSettingOptionValues, causing possible
partial success; to fix, perform the map update and payment-setting application
inside the same DB.Transaction closure so any failure returns an error and
triggers rollback: update the Transaction lambda that iterates keys (using
Option, optionValues, paymentSettingOptionValues, keys) to call
updateOptionMap(key, ...) for non-"payment_setting." keys and to call
applyPaymentSettingOptionValues(...) before returning nil; if updateOptionMap or
applyPaymentSettingOptionValues are non-DB operations, refactor them to return
errors and make them safe to call inside the transaction (or add TX-aware
variants) so failures propagate and the transaction can rollback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cc63373-16a9-4340-b0f4-0a00a4dcd81c
📒 Files selected for processing (2)
model/option.goweb/src/i18n/locales/fr.json
✅ Files skipped from review due to trivial changes (1)
- web/src/i18n/locales/fr.json
Comment on lines
+225
to
+227
| if err := validatePaymentSettingOptionValues(paymentSettingOptionValues); err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
Current payment-setting “validation” is effectively non-strict.
validatePaymentSettingOptionValues relies on config.UpdateConfigFromMap, which is permissive (unknown keys / parse failures are skipped). Invalid values can pass validation and still be written to DB, causing persisted config to diverge from effective runtime config.
Suggested strict validation shape
+import "fmt"
...
func validatePaymentSettingOptionValues(optionValues map[string]string) error {
if len(optionValues) == 0 {
return nil
}
paymentSettingConfigMap := make(map[string]string, len(optionValues))
for key, value := range optionValues {
paymentSettingConfigMap[strings.TrimPrefix(key, "payment_setting.")] = value
}
- paymentSetting := operation_setting.GetPaymentSetting()
- return config.UpdateConfigFromMap(&paymentSetting, paymentSettingConfigMap)
+ for key, value := range paymentSettingConfigMap {
+ v := strings.TrimSpace(value)
+ switch key {
+ case "auto_switch_group_enabled", "auto_switch_group_only_new_topups":
+ if _, err := strconv.ParseBool(v); err != nil {
+ return fmt.Errorf("invalid %s: %w", key, err)
+ }
+ case "auto_switch_group_enabled_from":
+ if _, err := strconv.ParseInt(v, 10, 64); err != nil {
+ return fmt.Errorf("invalid %s: %w", key, err)
+ }
+ case "auto_switch_group_base_group":
+ // normalized later; accept empty/whitespace.
+ case "auto_switch_group_rules":
+ var rules []operation_setting.PaymentAutoSwitchGroupRule
+ if err := common.UnmarshalJsonStr(v, &rules); err != nil {
+ return fmt.Errorf("invalid %s: %w", key, err)
+ }
+ default:
+ return fmt.Errorf("unsupported payment setting key: %s", key)
+ }
+ }
+ return nil
}Also applies to: 256-268, 280-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/option.go` around lines 225 - 227, The current
validatePaymentSettingOptionValues is too permissive because it calls
config.UpdateConfigFromMap which skips unknown keys/parse errors; change
validatePaymentSettingOptionValues to perform strict decoding into the exact
expected config struct (e.g., marshal the option map to JSON/YAML and unmarshal
with a decoder that DisallowUnknownFields or use a strict decoder/validator) and
return an error on any unknown keys or parse failures instead of silently
accepting them; replace the permissive UpdateConfigFromMap usage with this
strict decode+validate flow and apply the same strict approach to the other
similar validators referenced in the review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
变更说明
payment_setting.auto_switch_group_enabledpayment_setting.auto_switch_group_rulespayment_setting.auto_switch_group_only_new_topupspayment_setting.auto_switch_group_base_groupupgrade_group逻辑。变更类型
关联任务
验证摘要
当前测试规则
1 USD -> user110 USD -> vip100 USD -> svip已完成验证
10 用户回归矩阵：
覆盖
default/user1/vip/svip初始分组、无历史充值、有历史充值、跨阈值累计、Stripe 累计、Creem + Epay 混合累计、重复补单幂等，以及 active subscription 用户再次普通充值。结果：Passed 10/10，Failed 0/10。
边界与配置校验：
覆盖重复阈值、非法分组、启用状态下空规则、
failed/expired历史订单不参与累计、订阅失效后的回退、active subscription 保护、关闭自动切换、单规则配置、单规则未达阈值、未命中任何阈值。结果：Passed 10/10，Failed 0/10。
补充验证：
覆盖“仅统计新充值”、基础分组约束、金额边界、状态流转、幂等、负向校验，以及订阅优先级与回退。
重点验证：
1 / 10 / 100 USD精确命中、failed/expired订单不能补成成功、同一订单并发重复完成、active subscription 不会被普通充值覆盖、subscription 失效后会回退到正确分组、链外分组不会被自动切组覆盖。结论
upgrade_group不会被普通充值覆盖。Summary by CodeRabbit
New Features
Bug Fixes