Conversation
…g overrides Add the database foundation for principal-based configuration overrides (user, group, role) in data-schemas. Includes schema with tenantId and tenant isolation, CRUD methods, and barrel exports.
The pre-commit hook was missing #!/bin/sh, and core.autocrlf=true was converting it to CRLF, both causing "Exec format error" on Windows. Add .gitattributes to force LF for .husky/* and *.sh files.
There was a problem hiding this comment.
Pull request overview
Introduces a new persisted “Config override” collection in packages/data-schemas to store per-principal configuration overrides (user/group/role), including schema/model/methods wiring, and adds repo-level line-ending improvements for Husky hooks.
Changes:
- Add
Configtypes export and a newconfigMongoose schema/model. - Add
createConfigMethods()with CRUD-ish helpers for config overrides and wire intocreateMethods(). - Add
pre-commitshebang and enforce LF endings via.gitattributes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-schemas/src/types/index.ts | Re-exports new config types from the types barrel. |
| packages/data-schemas/src/types/config.ts | Adds Config / IConfig type definitions for principal-scoped overrides. |
| packages/data-schemas/src/schema/index.ts | Exposes configSchema from the schema barrel. |
| packages/data-schemas/src/schema/config.ts | Defines configSchema fields/indexes and configVersion save hook. |
| packages/data-schemas/src/models/index.ts | Registers the new Config model in createModels(). |
| packages/data-schemas/src/models/config.ts | Adds createConfigModel() and applies tenant isolation plugin. |
| packages/data-schemas/src/methods/index.ts | Wires ConfigMethods into the global methods factory and types. |
| packages/data-schemas/src/methods/config.ts | Implements config lookup/upsert/delete/toggle helpers. |
| .husky/pre-commit | Adds shebang for hook execution consistency. |
| .gitattributes | Forces LF line endings for hooks/shell scripts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (this.isModified('overrides')) { | ||
| this.configVersion += 1; |
There was a problem hiding this comment.
configVersion is described as “auto-increments on overrides change”, but this pre('save') hook will also fire on the initial insert (new documents are considered modified), causing the first persisted version to become 2. Consider guarding with if (!this.isNew && this.isModified('overrides')) (or initialize configVersion to 0) to avoid an off-by-one on create.
| if (this.isModified('overrides')) { | |
| this.configVersion += 1; | |
| if (!this.isNew && this.isModified('overrides')) { | |
| this.configVersion = (this.configVersion || 1) + 1; |
| { timestamps: true }, | ||
| ); | ||
|
|
||
| configSchema.index({ principalType: 1, principalId: 1, isActive: 1, tenantId: 1 }); |
There was a problem hiding this comment.
The schema/methods assume one config doc per (principalType, principalId, tenantId) (e.g., findConfigByPrincipal() uses findOne and upsertConfig() uses findOneAndUpdate), but there’s no unique index enforcing that invariant. Concurrent upserts can create duplicates and make reads nondeterministic. Add a unique compound index (optionally partial on isActive: true if you want to allow historical/inactive rows).
| configSchema.index({ principalType: 1, principalId: 1, isActive: 1, tenantId: 1 }); | |
| configSchema.index( | |
| { principalType: 1, principalId: 1, tenantId: 1 }, | |
| { unique: true, partialFilterExpression: { isActive: true } }, | |
| ); |
| async function upsertConfig( | ||
| principalType: PrincipalType, | ||
| principalId: string | Types.ObjectId, | ||
| principalModel: PrincipalModel, | ||
| overrides: Record<string, unknown>, | ||
| priority: number, | ||
| session?: ClientSession, | ||
| ): Promise<IConfig | null> { | ||
| const Config = mongoose.models.Config as Model<IConfig>; | ||
|
|
||
| const query = { | ||
| principalType, | ||
| principalId, | ||
| }; | ||
|
|
||
| const update = { | ||
| $set: { | ||
| principalModel, | ||
| overrides, | ||
| priority, | ||
| isActive: true, | ||
| }, | ||
| }; | ||
|
|
||
| const options = { | ||
| upsert: true, | ||
| new: true, | ||
| ...(session ? { session } : {}), | ||
| }; | ||
|
|
||
| return await Config.findOneAndUpdate(query, update, options); | ||
| } |
There was a problem hiding this comment.
upsertConfig() updates overrides via findOneAndUpdate, but configVersion is only incremented by a pre('save') hook (which does not run for findOneAndUpdate). As a result, updates won’t bump configVersion, breaking cache invalidation semantics. Consider adding an $inc: { configVersion: 1 } when overrides changes, or a pre('findOneAndUpdate') hook that increments when the update modifies overrides.
| if (session) { | ||
| configQuery.session(session); | ||
| } | ||
|
|
There was a problem hiding this comment.
getApplicableConfigs() returns configs in unspecified order, but the model introduces a priority field explicitly to control merge order. Without a deterministic sort (e.g., sort({ priority: -1 }) plus a tie-breaker), callers can get inconsistent merges depending on insertion order. Sorting in this query (or documenting/enforcing sorting at the call site) would make behavior stable.
| configQuery.sort({ priority: -1, _id: 1 }); |
| export function createConfigMethods(mongoose: typeof import('mongoose')) { | ||
| async function findConfigByPrincipal( | ||
| principalType: PrincipalType, | ||
| principalId: string | Types.ObjectId, | ||
| session?: ClientSession, | ||
| ): Promise<IConfig | null> { | ||
| const Config = mongoose.models.Config as Model<IConfig>; | ||
| const query = Config.findOne({ | ||
| principalType, | ||
| principalId, | ||
| isActive: true, | ||
| }); | ||
| if (session) { | ||
| query.session(session); | ||
| } | ||
| return await query.lean(); | ||
| } | ||
|
|
||
| async function getApplicableConfigs( |
There was a problem hiding this comment.
This PR adds a new persisted config override model + methods (findConfigByPrincipal, getApplicableConfigs, upsertConfig, etc.), but there are no accompanying tests. The repo has extensive method-level tests under packages/data-schemas/src/methods/*.spec.ts; adding coverage here (especially for tenant isolation behavior, uniqueness expectations, and configVersion bumping on update) would help prevent regressions.
Add /api/admin/config endpoints for managing per-principal config overrides (user, group, role). Handlers in @librechat/api use DI pattern with section-level hasConfigCapability checks for granular access control. Supports full overrides replacement, per-field PATCH via dot-paths, field deletion, toggle active, and listing.
The path-to-regexp wildcard syntax (:fieldPath(*)) is not supported by the version used in Express. Send fieldPath in the DELETE request body instead, which also avoids URL-encoding issues with dotted paths.
Add mergeConfigOverrides utility in data-schemas for deep-merging DB config overrides into base AppConfig by priority order. Update getAppConfig to query DB for applicable configs when role/userId is provided, with short-TTL caching and a hasAnyConfigs feature flag for zero-cost when no DB configs exist. Also: add unique compound index on Config schema, pass userId from config middleware, and signal config changes from admin API handlers.
Move override resolution, caching strategy, and signalConfigChange from api/server/services/Config/app.js into packages/api/src/app/appConfigService.ts using the DI factory pattern (createAppConfigService). The JS file becomes a thin wiring layer injecting loadBaseConfig, cache, and DB dependencies.
No description provided.