feat: add unique constraint migration on customer table name field - #4284
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a unique index on governance customer names with a deduplication migration; updates governance config merge to match customers/teams by name when config IDs are empty and adopt DB IDs; ensures created customers, teams, and virtual keys receive generated UUIDs when missing. ChangesCustomer Name Uniqueness and Config Reconciliation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/migrations.go`:
- Around line 9968-9971: The Rollback closure for this migration currently drops
an index but cannot revert the irreversible data rewrite of duplicate customer
names; update the Rollback function (the Rollback: func(tx *gorm.DB) error
closure that references idxName and ctx) to explicitly return a non-rollbackable
error (e.g., fmt.Errorf or errors.New) with a clear message explaining the
irreversible rewrite of customer names, or alternatively implement and document
a true reversal strategy that restores original customer name state before
returning nil; ensure imports are added if you use fmt/errors and keep the error
message descriptive about the duplicate-customer-name rewrite.
- Around line 9951-9958: The rename loop can produce candidate strings longer
than the DB column; truncate the base name before appending the "-<suffix>" so
candidate fits the column max length and uniqueness checks still work. In the
block that builds candidate from c.Name, compute a safe base (truncate c.Name to
ensure len(base)+len("-")+len(str(suffix)) <= CUSTOMER_NAME_MAX_LEN), use that
truncated base when generating candidate and when checking/marking
taken[candidate], and then call tx.Model(&tables.TableCustomer{}).Where("id =
?", c.ID).Update("name", candidate).Error with the truncated candidate.
- Line 9963: The migration currently uses tx.Exec to run "CREATE UNIQUE INDEX
..." which runs inside the migration transaction and can block writes; instead,
detect the Postgres dialect in migrations.go and create the index outside the
transaction using "CREATE UNIQUE INDEX CONCURRENTLY " + idxName + " ON
governance_customers (name)" (i.e., do not use tx; run it on the DB connection
after the transaction is committed/rolled back), and only use the non-concurrent
form for non-Postgres dialects. Reference the existing tx.Exec call and
idxName/governance_customers to locate where to change the behavior.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2077-2079: The ID equality check currently treats empty strings as
valid matches causing cross-row merges; update the idMatch computation in the
blocks using existingCustomer and newCustomer so it requires a non-empty ID
(e.g., idMatch only true when existingCustomer.ID != "" && existingCustomer.ID
== newCustomer.ID), and apply the same guard to the duplicate occurrence around
the lines referencing idMatch/nameMatch (the second occurrence using the same
existingCustomer/newCustomer variables). Ensure nameMatch logic remains
unchanged so name-based matching still only applies when existingCustomer.ID is
empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17e32fd0-8d6e-4897-97bd-857c732e81fb
📒 Files selected for processing (3)
framework/configstore/migrations.goframework/configstore/tables/customer.gotransports/bifrost-http/lib/config.go
Confidence Score: 3/5Not safe to merge as-is: the customer hash computed before ID adoption causes repeated spurious updates on every config reload for name-matched entries, and the team name-match loop can attach a team to the wrong customer when two customers share a team name. Two independent correctness defects exist in the config merge path. First, GenerateCustomerHash is called with an empty ID before the name-match adopts the DB UUID, so the hash will never equal the DB-stored hash and every reload triggers an unnecessary update. Second, team name-matching does not filter by CustomerID, so a same-named team under a different customer can be incorrectly matched and its primary key adopted. transports/bifrost-http/lib/config.go — both the customer hash ordering and the team name-match scoping issues are in the merge loops here. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
3f14f4e to
95b2262
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2069-2075: The name-based reconciliation condition is checking the
wrong struct's ID; change the fallback from existingCustomer.ID == "" to
newCustomer.ID == "" so that when the config entry lacks an ID we match by name
and then adopt the DB ID (update configData.Governance.Customers[i].ID =
existingCustomer.ID). Apply the same fix for teams (replace existingTeam.ID ==
"" with newTeam.ID == "" where nameMatch is computed) so name-only entries
reconcile instead of creating duplicates.
- Around line 1163-1164: processAuthoritativeProvider currently converts
validation failures into warnings but syncAuthoritativeProvidersInStore still
writes whatever is in authoritativeProviders to the DB and prunes prior state;
change the flow so invalid providers are not persisted or used to prune existing
configs. Modify processAuthoritativeProvider to return an explicit error or a
validity boolean for each provider (referencing processAuthoritativeProvider,
providerName, providerCfgInFile, existingCfg, exists, authoritativeProviders)
and update syncAuthoritativeProvidersInStore to skip entries that are invalid
(do not write them and do not remove the previous working provider), only
persisting and pruning based on providers that passed validation. Ensure
explicit error handling is preserved (propagate or check errors) rather than
swallowing validation failures.
- Around line 2751-2753: When constructing governance entries, set
team/virtualKey IDs before appending to the in-memory slices so the in-memory
config and DB use the same UUIDs: when building teamsToAdd and virtualKeysToAdd
(the code that currently checks team.ID == "" and calls uuid.NewString()),
assign IDs at creation time and propagate them back into GovernanceConfig.Teams
/ GovernanceConfig.VirtualKeys (or mutate the source elements before append),
and reuse that same ID-generation helper in createGovernanceConfigInStore and
the bootstrap path to ensure reads/reconciliation see the DB-assigned IDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a193ddfa-7b13-4567-a816-6eeab0f3b903
📒 Files selected for processing (3)
framework/configstore/migrations.goframework/configstore/tables/customer.gotransports/bifrost-http/lib/config.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2069-2075: The name-based reconciliation condition is checking the
wrong struct's ID; change the fallback from existingCustomer.ID == "" to
newCustomer.ID == "" so that when the config entry lacks an ID we match by name
and then adopt the DB ID (update configData.Governance.Customers[i].ID =
existingCustomer.ID). Apply the same fix for teams (replace existingTeam.ID ==
"" with newTeam.ID == "" where nameMatch is computed) so name-only entries
reconcile instead of creating duplicates.
- Around line 1163-1164: processAuthoritativeProvider currently converts
validation failures into warnings but syncAuthoritativeProvidersInStore still
writes whatever is in authoritativeProviders to the DB and prunes prior state;
change the flow so invalid providers are not persisted or used to prune existing
configs. Modify processAuthoritativeProvider to return an explicit error or a
validity boolean for each provider (referencing processAuthoritativeProvider,
providerName, providerCfgInFile, existingCfg, exists, authoritativeProviders)
and update syncAuthoritativeProvidersInStore to skip entries that are invalid
(do not write them and do not remove the previous working provider), only
persisting and pruning based on providers that passed validation. Ensure
explicit error handling is preserved (propagate or check errors) rather than
swallowing validation failures.
- Around line 2751-2753: When constructing governance entries, set
team/virtualKey IDs before appending to the in-memory slices so the in-memory
config and DB use the same UUIDs: when building teamsToAdd and virtualKeysToAdd
(the code that currently checks team.ID == "" and calls uuid.NewString()),
assign IDs at creation time and propagate them back into GovernanceConfig.Teams
/ GovernanceConfig.VirtualKeys (or mutate the source elements before append),
and reuse that same ID-generation helper in createGovernanceConfigInStore and
the bootstrap path to ensure reads/reconciliation see the DB-assigned IDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a193ddfa-7b13-4567-a816-6eeab0f3b903
📒 Files selected for processing (3)
framework/configstore/migrations.goframework/configstore/tables/customer.gotransports/bifrost-http/lib/config.go
🛑 Comments failed to post (3)
transports/bifrost-http/lib/config.go (3)
1163-1164:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't persist invalid providers in authoritative mode.
processAuthoritativeProvidernow turns provider validation failures into warnings, butsyncAuthoritativeProvidersInStorestill writes that config to the DB and prunes the previous state. Insource_of_truth=config.jsonmode, one bad alias/custom-provider edit can now replace a working provider config with a broken one instead of being rejected.Suggested fix
- processAuthoritativeProvider(providerName, providerCfgInFile, existingCfg, exists, authoritativeProviders) + if err := processAuthoritativeProvider(providerName, providerCfgInFile, existingCfg, exists, authoritativeProviders); err != nil { + logger.Warn("failed to process provider %s: %v", providerName, err) + continue + }-func processAuthoritativeProvider( +func processAuthoritativeProvider( providerName string, providerCfgInFile configstore.ProviderConfig, existingCfg configstore.ProviderConfig, exists bool, providers map[schemas.ModelProvider]configstore.ProviderConfig, -) { +) error { provider := schemas.ModelProvider(strings.ToLower(providerName)) if err := ValidateCustomProvider(providerCfgInFile, provider); err != nil { - logger.Warn("invalid custom provider config for %s (writing through): %v", provider, err) + return err } ... if err := providerKeyInFile.Aliases.Validate(baseProvider); err != nil { - logger.Warn("invalid aliases for key %q in provider %s (writing through): %v", providerKeyInFile.Name, provider, err) + return fmt.Errorf("invalid aliases for key %q in provider %s: %w", providerKeyInFile.Name, provider, err) } } ... providers[provider] = providerCfgInFile + return nil }As per coding guidelines, Go changes here should preserve explicit error handling; swallowing provider validation errors changes authoritative sync from reject/skip to write-through persistence.
Also applies to: 1277-1290
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 1163 - 1164, processAuthoritativeProvider currently converts validation failures into warnings but syncAuthoritativeProvidersInStore still writes whatever is in authoritativeProviders to the DB and prunes prior state; change the flow so invalid providers are not persisted or used to prune existing configs. Modify processAuthoritativeProvider to return an explicit error or a validity boolean for each provider (referencing processAuthoritativeProvider, providerName, providerCfgInFile, existingCfg, exists, authoritativeProviders) and update syncAuthoritativeProvidersInStore to skip entries that are invalid (do not write them and do not remove the previous working provider), only persisting and pruning based on providers that passed validation. Ensure explicit error handling is preserved (propagate or check errors) rather than swallowing validation failures.Source: Coding guidelines
2069-2075:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winName-based reconciliation checks the wrong ID.
The fallback is gated on
existingCustomer.ID == ""/existingTeam.ID == "", but the intended behavior here is to match when the file entry has no ID and then adopt the existing DB ID. As written, normal DB rows with generated IDs never hit this path, so a config customer/team declared by name only falls through to create instead of reconcile. For customers, that now collides with the new unique-name constraint.Suggested fix
- nameMatch := existingCustomer.ID == "" && existingCustomer.Name == newCustomer.Name + nameMatch := newCustomer.ID == "" && existingCustomer.Name == newCustomer.Name ... - nameMatch := existingTeam.ID == "" && existingTeam.Name == newTeam.Name + nameMatch := newTeam.ID == "" && existingTeam.Name == newTeam.NameAlso applies to: 2106-2112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2069 - 2075, The name-based reconciliation condition is checking the wrong struct's ID; change the fallback from existingCustomer.ID == "" to newCustomer.ID == "" so that when the config entry lacks an ID we match by name and then adopt the DB ID (update configData.Governance.Customers[i].ID = existingCustomer.ID). Apply the same fix for teams (replace existingTeam.ID == "" with newTeam.ID == "" where nameMatch is computed) so name-only entries reconcile instead of creating duplicates.
2751-2753:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGenerate governance UUIDs before merging them into in-memory state.
These IDs are assigned after
config.GovernanceConfig.Teams/VirtualKeyswere already appended at Lines 2346-2347, so the DB row gets a UUID but the merged in-memory config still carries"". That leaves API reads and later reconciliation working against stale IDs. Teams also still miss the same fallback increateGovernanceConfigInStoreat Line 3315.Generate the ID when building
teamsToAdd/virtualKeysToAddor copy it back into the source slices before the append, then reuse the same helper in the bootstrap path.Also applies to: 2797-2799
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2751 - 2753, When constructing governance entries, set team/virtualKey IDs before appending to the in-memory slices so the in-memory config and DB use the same UUIDs: when building teamsToAdd and virtualKeysToAdd (the code that currently checks team.ID == "" and calls uuid.NewString()), assign IDs at creation time and propagate them back into GovernanceConfig.Teams / GovernanceConfig.VirtualKeys (or mutate the source elements before append), and reuse that same ID-generation helper in createGovernanceConfigInStore and the bootstrap path to ensure reads/reconciliation see the DB-assigned IDs.
95b2262 to
da53f1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
framework/configstore/migrations.go (2)
9968-9970:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark this migration as non-rollbackable.
Rollback only drops the index, but Line 9957 permanently rewrites duplicate customer names. Returning success here misrepresents the downgrade semantics and hides irreversible data loss. Return an explicit non-rollbackable error instead.
As per coding guidelines: “If a migration cannot be rolled back, explicitly flag it as non-rollbackable.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/configstore/migrations.go` around lines 9968 - 9970, The migration's Rollback function currently executes tx.Exec("DROP INDEX IF EXISTS " + idxName) (see the Rollback func and idxName usage) but the migration is irreversible due to the duplicate-customer-name rewrite earlier; replace the current Rollback implementation so it explicitly returns a non-rollbackable error (e.g. the project's standard non-rollback sentinel) instead of attempting to drop the index or returning success, and ensure the migration metadata reflects that it is non-rollbackable.Source: Coding guidelines
9924-9964:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCreate the Postgres index outside the migration transaction.
Line 9924 runs this through
RunSingleMigration(ctx, nil, db, ...), so Line 9963 executes inside the default migration transaction. On Postgres,CREATE UNIQUE INDEXthere can block concurrent writes togovernance_customersfor the full index build. Split the Postgres path into a non-transactional step and useCREATE UNIQUE INDEX CONCURRENTLYafter the dedupe commit.As per coding guidelines: “When migrations are added or changed, verify they avoid deadlocks on large tables and create indexes concurrently.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/configstore/migrations.go` around lines 9924 - 9964, The migration currently runs CREATE UNIQUE INDEX inside the transaction in RunSingleMigration (migration ID "add_customer_name_unique_constraint") via tx.Exec, which can block writes on large Postgres tables; split this into two steps: keep the dedupe/rename logic (the code using customers, taken, firstSeen and the tx.Model(...).Update calls) in the existing migration so it runs inside the transaction, then add a follow-up migration (new ID e.g. "add_customer_name_unique_constraint_index") that runs outside any transaction and executes CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS using the global db (not tx) and the same idxName so the index build is non-blocking; ensure the new migration uses db.Exec (or whatever mechanism your migration runner provides to run non-transactional SQL) and uses CONCURRENTLY instead of plain CREATE UNIQUE INDEX.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/tables/customer.go`:
- Line 12: Add application-layer validation to reject empty customer names
before DB writes: in the CreateCustomer and UpdateCustomer functions in rdb.go
(or the API handler that calls them), check the Customer.Name (or the struct
field represented by Name in customer.go) and return a validation error if
strings.TrimSpace(name) == "" so empty or whitespace-only names are not written
to the DB; keep the gorm tag unchanged but ensure both create and update code
paths enforce this check and surface a clear error to callers.
- Line 12: The migration and error handling must be aligned for case-insensitive
uniqueness: update migrationAddCustomerNameUniqueConstraint to deduplicate using
a lowercased key (e.g., GROUP BY LOWER(name) or dedupe by strings.ToLower) and
create a case-insensitive unique index (e.g., UNIQUE INDEX ... ON (LOWER(name)))
instead of a plain UNIQUE ON (name); add normalization on writes (e.g., a
BeforeSave hook or normalize in the create path to TrimSpace and strings.ToLower
the Name field defined in the Customer struct) so existing/future records match
the index, and enhance parseGormError to include the offending customer name
value (use the normalized input value or parse it from the DB error payload) in
its unique-violation message.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2069-2075: The schema must allow empty IDs because the
reconciliation code assigns existingCustomer.ID into
configData.Governance.Customers[i].ID (and likewise for
configData.Governance.Teams[].ID), so update transports/config.schema.json to
make governance.customers[].id and governance.teams[].id optional (remove "id"
from the item's required list or mark the property as nullable/optional) and
adjust any examples/docs/tests that assert those fields are required so the
input this code reconciles will validate.
- Around line 2069-2075: When iterating configData.Governance.Customers, if
newCustomer.ID == "" and neither idMatch nor nameMatch is true, generate/adopt
an ID on configData.Governance.Customers[i] before appending to customersToAdd
or rebuilding config.GovernanceConfig.Customers; specifically, ensure that the
code that currently sets ID only on nameMatch is extended so that for unmatched
empty-ID rows you either (a) call CreateCustomer (or whatever synthesizes the
ID) and assign the returned ID back into configData.Governance.Customers[i], or
(b) generate a stable UUID client-side and assign it, then append that customer
to customersToAdd so the in-memory config reflects the real/synthesized ID prior
to persisting.
- Around line 2750-2753: The loop over teamsToAdd is mutating the range copy so
generated IDs are not written back; change the iteration to index or pointer
form (e.g., for i := range teamsToAdd { if teamsToAdd[i].ID == "" {
teamsToAdd[i].ID = uuid.NewString() } }) so the new ID is stored into the slice
element and thus visible when you append to config.GovernanceConfig.Teams; also
add the same guard in createGovernanceConfigInStore to ensure any newly created
governance team records get an ID when first-boot.
---
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 9968-9970: The migration's Rollback function currently executes
tx.Exec("DROP INDEX IF EXISTS " + idxName) (see the Rollback func and idxName
usage) but the migration is irreversible due to the duplicate-customer-name
rewrite earlier; replace the current Rollback implementation so it explicitly
returns a non-rollbackable error (e.g. the project's standard non-rollback
sentinel) instead of attempting to drop the index or returning success, and
ensure the migration metadata reflects that it is non-rollbackable.
- Around line 9924-9964: The migration currently runs CREATE UNIQUE INDEX inside
the transaction in RunSingleMigration (migration ID
"add_customer_name_unique_constraint") via tx.Exec, which can block writes on
large Postgres tables; split this into two steps: keep the dedupe/rename logic
(the code using customers, taken, firstSeen and the tx.Model(...).Update calls)
in the existing migration so it runs inside the transaction, then add a
follow-up migration (new ID e.g. "add_customer_name_unique_constraint_index")
that runs outside any transaction and executes CREATE UNIQUE INDEX CONCURRENTLY
IF NOT EXISTS using the global db (not tx) and the same idxName so the index
build is non-blocking; ensure the new migration uses db.Exec (or whatever
mechanism your migration runner provides to run non-transactional SQL) and uses
CONCURRENTLY instead of plain CREATE UNIQUE INDEX.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0435ac80-c217-4e6b-9bfd-ded7891f18d2
📒 Files selected for processing (3)
framework/configstore/migrations.goframework/configstore/tables/customer.gotransports/bifrost-http/lib/config.go
da53f1a to
7b59a58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
transports/bifrost-http/lib/config.go (3)
2069-2071:⚠️ Potential issue | 🟠 MajorRequire a non-empty incoming ID before declaring an ID match.
Line 2069 and Line 2106 still make
idMatchtrue for"" == "". If a legacy DB row or malformed file entry has an empty ID, this can merge the wrong customer/team before the name-based adoption logic runs.💡 Proposed fix
- idMatch := existingCustomer.ID == newCustomer.ID + idMatch := newCustomer.ID != "" && existingCustomer.ID == newCustomer.ID nameMatch := newCustomer.ID == "" && existingCustomer.Name == newCustomer.Name- idMatch := existingTeam.ID == newTeam.ID + idMatch := newTeam.ID != "" && existingTeam.ID == newTeam.ID nameMatch := newTeam.ID == "" && existingTeam.Name == newTeam.NameAlso applies to: 2106-2108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2069 - 2071, The idMatch calculation is currently true when both IDs are empty, allowing unintended merges; update the idMatch logic to require a non-empty incoming ID by changing the condition to check newCustomer.ID != "" && existingCustomer.ID == newCustomer.ID (and leave nameMatch as-is), and apply the same fix to the analogous idMatch at lines 2106-2108 so both places only treat an ID match when the incoming ID is non-empty (use the existingCustomer and newCustomer identifiers to locate and update the checks).
2670-2672:⚠️ Potential issue | 🟠 MajorGenerate IDs before these rows are copied into
GovernanceConfig.By the time these loops run,
customersToAdd/teamsToAddhave already been copied intoconfig.GovernanceConfigat Lines 2345-2346. So customer IDs written here never reach the live config, and the team path additionally mutates only the range copy. The same gap still exists increateGovernanceConfigInStore, so first-boot empty-ID customers/teams also stay inconsistent with the DB until restart.Also applies to: 2754-2756
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2670 - 2672, customersToAdd and teamsToAdd IDs are being set after those slices are copied into config.GovernanceConfig (and similarly in createGovernanceConfigInStore), so the assigned customer.ID / team.ID values never propagate to the live config or DB; move ID generation earlier: iterate customersToAdd and teamsToAdd before copying them into config.GovernanceConfig (and also before persisting in createGovernanceConfigInStore) and assign uuid.NewString() for any empty ID, ensuring you mutate the original slices (customersToAdd, teamsToAdd) so the populated IDs are included when you assign config.GovernanceConfig and write to the store.
2069-2075:⚠️ Potential issue | 🟠 MajorAlign the schema with this new empty-ID reconciliation path.
These branches now depend on
governance.customers[].id/governance.teams[].idbeing optional, but the referenced schema still requires both. That leaves the intended config shape rejected by schema-driven tooling and still logged as invalid byValidateConfigSchema()during startup.As per coding guidelines,
transports/config.schema.jsonis the source of truth, and the referenced schema contents still requireidfor both collections.Also applies to: 2106-2112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2069 - 2075, The schema currently requires governance.customers[].id and governance.teams[].id but the code in config.go (e.g., configData.Governance.Customers[...] reconciliation and similar for Governance.Teams) allows empty IDs and populates them from DB; update transports/config.schema.json to make those id properties optional (remove them from the "required" arrays for the customers and teams item schemas or mark them as nullable/optional) so ValidateConfigSchema() accepts the empty-ID shape, keeping descriptive schema docs and types intact.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/tables/customer.go`:
- Line 12: The migration function migrationAddCustomerNameUniqueConstraint must
not attempt to create a CONCURRENTLY index inside a transaction; refactor it so
the deduplication/cleanup step still runs inside the transaction but the CREATE
UNIQUE INDEX (with CONCURRENTLY and final name idx_governance_customers_name)
runs outside the transaction—e.g., split into two steps or two migrations and
call RunSingleMigration for the index creation with
&migrator.Options{UseTransaction:false} (or perform the index Exec on the non-tx
DB), and ensure the rollback/cleanup references the same final index name
(idx_governance_customers_name) so the up/down are aligned.
---
Duplicate comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2069-2071: The idMatch calculation is currently true when both IDs
are empty, allowing unintended merges; update the idMatch logic to require a
non-empty incoming ID by changing the condition to check newCustomer.ID != "" &&
existingCustomer.ID == newCustomer.ID (and leave nameMatch as-is), and apply the
same fix to the analogous idMatch at lines 2106-2108 so both places only treat
an ID match when the incoming ID is non-empty (use the existingCustomer and
newCustomer identifiers to locate and update the checks).
- Around line 2670-2672: customersToAdd and teamsToAdd IDs are being set after
those slices are copied into config.GovernanceConfig (and similarly in
createGovernanceConfigInStore), so the assigned customer.ID / team.ID values
never propagate to the live config or DB; move ID generation earlier: iterate
customersToAdd and teamsToAdd before copying them into config.GovernanceConfig
(and also before persisting in createGovernanceConfigInStore) and assign
uuid.NewString() for any empty ID, ensuring you mutate the original slices
(customersToAdd, teamsToAdd) so the populated IDs are included when you assign
config.GovernanceConfig and write to the store.
- Around line 2069-2075: The schema currently requires governance.customers[].id
and governance.teams[].id but the code in config.go (e.g.,
configData.Governance.Customers[...] reconciliation and similar for
Governance.Teams) allows empty IDs and populates them from DB; update
transports/config.schema.json to make those id properties optional (remove them
from the "required" arrays for the customers and teams item schemas or mark them
as nullable/optional) so ValidateConfigSchema() accepts the empty-ID shape,
keeping descriptive schema docs and types intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b94b68bf-c854-4c92-9d47-9b4e8f61e4d8
📒 Files selected for processing (3)
framework/configstore/migrations.goframework/configstore/tables/customer.gotransports/bifrost-http/lib/config.go
7b59a58 to
4fc319f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
transports/bifrost-http/lib/config.go (3)
2069-2075:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the schema contract for optional governance IDs.
This code now accepts
governance.customers[].idandgovernance.teams[].idas omitted inputs and backfills them, but the referenced schema still requires bothidandname. That leaves schema validation and schema-driven tooling rejecting the exact config this reconciliation path is meant to support.As per coding guidelines,
transports/config.schema.jsonis the source of truth, and the referenced schema contents still require bothidandnamefor governance customers and teams.Also applies to: 2090-2092, 2109-2115, 2130-2132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2069 - 2075, Update the JSON schema so governance.customers[].id and governance.teams[].id are optional (not required) to match the reconciliation code that backfills missing IDs; specifically modify transports/config.schema.json to remove "id" from the required list (or mark it as optional) for the Customer and Team object definitions used by Governance, ensure the schema still requires "name", and run/adjust any schema-driven tooling or validators to accept configs where governance.customers[].id and governance.teams[].id are omitted so the code paths around configData.Governance.Customers (where ID is adopted from existingCustomer.ID) and the analogous Teams logic no longer fail schema validation.Source: Coding guidelines
2090-2092:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror this missing-ID normalization into the bootstrap path.
These UUID assignments only run inside
mergeGovernanceConfig. When the store has no governance rows,loadGovernanceConfiggoes straight tocreateGovernanceConfigInStore, so file-defined customers/teams with empty IDs still bypass this normalization. For customers, that path copies intocustomerRowbeforeCreateCustomer, so the live config can retainID == ""even if the store layer synthesizes one.Also applies to: 2130-2132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2090 - 2092, The bootstrap path that calls loadGovernanceConfig/createGovernanceConfigInStore currently skips normalizing empty IDs, so file-defined customers/teams can be inserted with ID == "". Update the bootstrap flow to mirror the mergeGovernanceConfig normalization: before copying into customerRow or teamRow and before calling CreateCustomer/CreateTeam, detect empty ID fields for each configData.Governance.Customers and configData.Governance.Teams and assign uuid.NewString() where missing (same logic used around mergeGovernanceConfig). Ensure the same ID-fix is applied at the locations currently mentioned (the customer copy before CreateCustomer and the analogous team handling at the 2130-2132 area) so runtime rows never get empty IDs.
2069-2071:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire a non-empty config ID before taking the ID-match branch.
Line 2069 and Line 2109 still treat
"" == ""as a valid ID match. An empty-ID customer/team from config can therefore bind to the first existing row with an empty ID comparison, bypassing the intended name-based reconciliation and updating the wrong record.💡 Minimal fix
- idMatch := existingCustomer.ID == newCustomer.ID + idMatch := newCustomer.ID != "" && existingCustomer.ID == newCustomer.ID nameMatch := newCustomer.ID == "" && existingCustomer.Name == newCustomer.Name- idMatch := existingTeam.ID == newTeam.ID + idMatch := newTeam.ID != "" && existingTeam.ID == newTeam.ID nameMatch := newTeam.ID == "" && existingTeam.Name == newTeam.NameAlso applies to: 2109-2111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2069 - 2071, The idMatch logic currently allows empty IDs to match ("" == ""), so change the idMatch computation to require a non-empty newCustomer.ID (e.g., idMatch := newCustomer.ID != "" && existingCustomer.ID == newCustomer.ID) and keep nameMatch as the fallback for when newCustomer.ID == "" (nameMatch := newCustomer.ID == "" && existingCustomer.Name == newCustomer.Name); apply the identical change to the other location that computes idMatch/nameMatch so empty-ID configs don't accidentally match by ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/tables/customer.go`:
- Line 12: The Name field's GORM tag uses an unnamed uniqueIndex which lets GORM
choose the physical index name and can break migration up/down parity; update
the struct field tag for Name (the Name string field in the Customer model) to
specify the explicit index name (for example change `uniqueIndex` to
`uniqueIndex:idx_governance_customers_name`) so the generated index name is
stable and matches your migration contract.
---
Duplicate comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2069-2075: Update the JSON schema so governance.customers[].id and
governance.teams[].id are optional (not required) to match the reconciliation
code that backfills missing IDs; specifically modify
transports/config.schema.json to remove "id" from the required list (or mark it
as optional) for the Customer and Team object definitions used by Governance,
ensure the schema still requires "name", and run/adjust any schema-driven
tooling or validators to accept configs where governance.customers[].id and
governance.teams[].id are omitted so the code paths around
configData.Governance.Customers (where ID is adopted from existingCustomer.ID)
and the analogous Teams logic no longer fail schema validation.
- Around line 2090-2092: The bootstrap path that calls
loadGovernanceConfig/createGovernanceConfigInStore currently skips normalizing
empty IDs, so file-defined customers/teams can be inserted with ID == "". Update
the bootstrap flow to mirror the mergeGovernanceConfig normalization: before
copying into customerRow or teamRow and before calling
CreateCustomer/CreateTeam, detect empty ID fields for each
configData.Governance.Customers and configData.Governance.Teams and assign
uuid.NewString() where missing (same logic used around mergeGovernanceConfig).
Ensure the same ID-fix is applied at the locations currently mentioned (the
customer copy before CreateCustomer and the analogous team handling at the
2130-2132 area) so runtime rows never get empty IDs.
- Around line 2069-2071: The idMatch logic currently allows empty IDs to match
("" == ""), so change the idMatch computation to require a non-empty
newCustomer.ID (e.g., idMatch := newCustomer.ID != "" && existingCustomer.ID ==
newCustomer.ID) and keep nameMatch as the fallback for when newCustomer.ID == ""
(nameMatch := newCustomer.ID == "" && existingCustomer.Name ==
newCustomer.Name); apply the identical change to the other location that
computes idMatch/nameMatch so empty-ID configs don't accidentally match by ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f654c41-4906-4e3e-9983-399946f3b921
📒 Files selected for processing (3)
framework/configstore/migrations.goframework/configstore/tables/customer.gotransports/bifrost-http/lib/config.go
4fc319f to
55b4025
Compare
55b4025 to
e4f9467
Compare
Merge activity
|
…4284) ## Summary Enforces uniqueness on the `governance_customers.name` column. Previously, duplicate customer names could exist in the database, which caused config-sync to create new rows instead of updating existing ones when customers were identified by name rather than ID. ## Changes - Added a `uniqueIndex` constraint to `TableCustomer.Name` so the schema enforces name uniqueness going forward. - Added a database migration (`migrationAddCustomerNameUniqueConstraint`) that deduplicates any existing rows before creating the unique index. Duplicate names are resolved by appending `-1`, `-2`, etc. to later occurrences (ordered by `created_at ASC, id ASC`), ensuring the earliest-created record always retains the original name. - Updated `mergeGovernanceConfig` to match customers and teams by name (in addition to ID) when the config file entry has no ID set. When a name match is found, the DB record's ID is adopted into the in-memory config so subsequent updates target the correct primary key rather than inserting a new row. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... ./transports/bifrost-http/... ``` 1. Seed the database with two `governance_customers` rows sharing the same name. 2. Run the service to trigger migrations and verify the second row is renamed to `<name>-1` and the unique index is created on `governance_customers(name)`. 3. Define a customer in the config file without an `id` field and confirm that reloading the config updates the existing DB record rather than inserting a duplicate. ## Breaking changes - [x] Yes - [ ] No Any existing duplicate customer names in the database will be automatically renamed during the migration. Operators should audit renamed customers after upgrading if downstream systems reference customer names directly. ## Related issues ## Security considerations None beyond standard data integrity guarantees. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Customer names are now enforced unique in the database. * Existing duplicate customer names are deterministically auto-renamed with numeric suffixes to prevent conflicts. * Configuration imports match and preserve existing customers and teams by ID or name, reducing unintended duplicates. * Customers, teams, and virtual keys with missing IDs now receive generated IDs so creations and imports succeed. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary
Enforces uniqueness on the
governance_customers.namecolumn. Previously, duplicate customer names could exist in the database, which caused config-sync to create new rows instead of updating existing ones when customers were identified by name rather than ID.Changes
uniqueIndexconstraint toTableCustomer.Nameso the schema enforces name uniqueness going forward.migrationAddCustomerNameUniqueConstraint) that deduplicates any existing rows before creating the unique index. Duplicate names are resolved by appending-1,-2, etc. to later occurrences (ordered bycreated_at ASC, id ASC), ensuring the earliest-created record always retains the original name.mergeGovernanceConfigto match customers and teams by name (in addition to ID) when the config file entry has no ID set. When a name match is found, the DB record's ID is adopted into the in-memory config so subsequent updates target the correct primary key rather than inserting a new row.Type of change
Affected areas
How to test
go test ./framework/configstore/... ./transports/bifrost-http/...governance_customersrows sharing the same name.<name>-1and the unique index is created ongovernance_customers(name).idfield and confirm that reloading the config updates the existing DB record rather than inserting a duplicate.Breaking changes
Any existing duplicate customer names in the database will be automatically renamed during the migration. Operators should audit renamed customers after upgrading if downstream systems reference customer names directly.
Related issues
Security considerations
None beyond standard data integrity guarantees.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit