Add per-client external link controls#5650
Conversation
There was a problem hiding this comment.
Well-structured feature with solid Go test coverage. There is one blocking correctness issue in how pre-existing rows interact with the new expiry filter, and the i18n rollout is incomplete (a documented hard requirement). The four inline comments below carry the details.
Blocking
internal/sub/external_config.go:59— the newexpiry_timepredicate is not NULL-tolerant, so external-link rows that existed before this migration (backfilled as NULL) are dropped from every subscription.- i18n — the five new
pages.clients.*keys are added only toen-USandzh-CN; the other 11 locale files lack them.
Suggestions
internal/database/model/model.go:714—expiry_time/last_fetch_atomit thedefault:0tag used for every other integer column.internal/sub/external_subscription.go:52— fetch state is written to the DB on every cache hit, turning the subscription read path into a write.
To verify before merge
- Confirm
make genreproducesfrontend/public/openapi.jsonwith no further diff; the file is generated fromendpoints.ts, not hand-maintained. - The
go test ./.../npm run buildruns listed in the PR description.
This review was generated automatically; a maintainer may follow up.
| now := time.Now().UnixMilli() | ||
| if err := db.Where("client_id IN ?", clientIds). | ||
| Where("(enable IS NULL OR enable = ?)", true). | ||
| Where("(expiry_time = 0 OR expiry_time > ?)", now). |
There was a problem hiding this comment.
blocking: The enable predicate above is NULL-tolerant (enable IS NULL OR enable = ?), but this expiry_time predicate is not. The expiry_time column is added without default:0 (unlike every other integer column in the schema), so on existing databases AutoMigrate adds it nullable and rows created before this migration are backfilled as NULL. Under SQL three-valued logic expiry_time = 0 OR expiry_time > ? is false for NULL, so every pre-existing external link silently disappears from all client subscriptions after upgrade. Make the predicate NULL-tolerant (and/or add a backfill mirroring normalizeClientExternalLinkEnable).
| Where("(expiry_time = 0 OR expiry_time > ?)", now). | |
| Where("(expiry_time IS NULL OR expiry_time = 0 OR expiry_time > ?)", now). |
| Value string `json:"value" gorm:"column:value"` | ||
| Remark string `json:"remark" gorm:"column:remark"` | ||
| Enable *bool `json:"enable" gorm:"column:enable;default:true"` | ||
| ExpiryTime int64 `json:"expiryTime" gorm:"column:expiry_time"` |
There was a problem hiding this comment.
suggestion: Every other integer column that needs a defined value carries gorm:"...;default:0" (for example SortOrder, Reset, Port, Priority). expiry_time and last_fetch_at omit it, so AutoMigrate adds them as nullable columns and pre-existing rows are backfilled as NULL rather than 0. Adding the default is the cleaner root-cause fix for the expiry-filter issue flagged in external_config.go.
| ExpiryTime int64 `json:"expiryTime" gorm:"column:expiry_time"` | |
| ExpiryTime int64 `json:"expiryTime" gorm:"column:expiry_time;default:0"` |
Consider the same ;default:0 on last_fetch_at (line 716).
| cached, ok := subscriptionCache.m[rawURL] | ||
| subscriptionCache.Unlock() | ||
| if ok && time.Since(cached.fetchedAt) < subscriptionCacheTTL { | ||
| updateExternalSubscriptionFetchState(rowID, cached.fetchedAt, nil) |
There was a problem hiding this comment.
suggestion: updateExternalSubscriptionFetchState issues a DB UPDATE here on every cache hit, repeatedly writing the same cached.fetchedAt. Because subscriptions are polled frequently and one request can carry several subscription rows, this converts the previously read-only subscription-serving path into a write path; on SQLite writes serialize, so it adds lock contention under load. Consider recording fetch state only when it actually changes — for example drop the call on the cache-hit branch and keep the writes on the real-fetch success/error branches.
| "addExternalSubscription": "Add External Subscription", | ||
| "noExternalLinks": "No external links yet.", | ||
| "noExternalSubscriptions": "No external subscriptions yet.", | ||
| "namePrefix": "Name prefix", |
There was a problem hiding this comment.
blocking: These five keys (namePrefix, lastFetchAt, lastFetchError, neverFetched, leaveBlankToNeverExpire) are added under pages.clients here and in zh-CN.json, but the other 11 files in internal/web/translation/ do not define them under pages.clients. Per the project hard rules a new English key must be added to every locale JSON. Missing keys fall back to en-US so the build still passes, but non-English/Chinese users see English text. Add the translated keys to ar-EG, es-ES, fa-IR, id-ID, ja-JP, pt-BR, ru-RU, tr-TR, uk-UA, vi-VN, and zh-TW. Note leaveBlankToNeverExpire already exists in those files but only under pages.inbounds, so the pages.clients lookup still misses.
The subscription render path issued an UPDATE even on cache hits (write amplification on a hot path); now writes only on a real fetch or error. Adds the 5 new keys to the remaining 11 locales and trims added Go comments. Merges current main.
Summary
Adds per-client controls for external links and external subscriptions without changing WireGuard client binding architecture.
Notes
This is intentionally split from the earlier WireGuard client-binding PR. It does not change the main WireGuard clients[] model on current main.
Verification