fix(categories): add unique index on family_id and name - #2981
fix(categories): add unique index on family_id and name#2981bittensorrider wants to merge 7 commits into
Conversation
find_or_create_by! race rescues only work when the DB enforces uniqueness. Deduplicate existing collisions, then add the unique index so parallel sync/import jobs cannot create duplicate category names per family. Closes we-promise#929 Co-authored-by: Cursor <cursoragent@cursor.com>
|
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:
📝 WalkthroughWalkthroughThe migration consolidates duplicate categories by family and name, remaps related records, removes duplicates, and adds a unique database index. Tests cover budget associations, import mappings, index restoration, and database-level uniqueness. ChangesCategory uniqueness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Migration
participant Categories
participant RelatedRecords
participant Database
Migration->>Categories: Build duplicate-category map
Migration->>RelatedRecords: Reparent and remap references
Migration->>Categories: Delete duplicate categories
Migration->>Database: Create unique index on family_id and name
Database-->>Migration: Enforce uniqueness
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a595147844
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
db/migrate/20260808120000_add_unique_index_on_categories_family_id_and_name.rb (1)
24-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse two-space indentation for
dedupe_categories!.Move
def dedupe_categories!to the same class indentation asdef up. De-indent its body by two spaces.As per coding guidelines, Ruby code should use 2-space indent.
🤖 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 `@db/migrate/20260808120000_add_unique_index_on_categories_family_id_and_name.rb` around lines 24 - 25, Adjust the indentation of the private method dedupe_categories! to match the class-level indentation of up, and de-indent the entire method body by two spaces while preserving its implementation.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
`@db/migrate/20260808120000_add_unique_index_on_categories_family_id_and_name.rb`:
- Around line 64-73: Update the migration’s duplicate-row deletion flow using
category_dedupe_map so each collision reconciles the duplicate
budget_categories.budgeted_spending into the keeper row before deletion,
following an explicit preservation policy. Ensure the policy handles differing
amounts without changing the budget total, and add migration coverage for
colliding rows with different spending values.
- Around line 82-88: Extend the category deduplication remapping around the
existing import_mappings UPDATE to also update import_source_mappings targets,
replacing duplicate category IDs with category_dedupe_map.keeper_id where
target_type is 'Category'. Run this remapping before the duplicate categories
are deleted, preserving valid non-Category mappings.
---
Nitpick comments:
In
`@db/migrate/20260808120000_add_unique_index_on_categories_family_id_and_name.rb`:
- Around line 24-25: Adjust the indentation of the private method
dedupe_categories! to match the class-level indentation of up, and de-indent the
entire method body by two spaces while preserving its implementation.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59f3de77-71fb-4589-8f54-784b90d7a1ed
📒 Files selected for processing (3)
db/migrate/20260808120000_add_unique_index_on_categories_family_id_and_name.rbdb/schema.rbtest/models/category_test.rb
Match Category::Merger by summing duplicate budget_categories amounts into the keeper row before deleting (budget_id, category_id) collisions. Co-authored-by: Cursor <cursoragent@cursor.com>
Document the additive collision policy and add migration tests for differing keeper/duplicate amounts plus non-colliding reassignment. Co-authored-by: Cursor <cursoragent@cursor.com>
Update target_id via category_dedupe_map before deleting duplicate categories so session import mappings stay valid; leave non-Category targets untouched. Co-authored-by: Cursor <cursoragent@cursor.com>
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Verdict: Well thought-out migration overall, but there's a plausible data-migration failure edge case in the budget_categories reassignment step.
db/migrate/20260808120000_add_unique_index_on_categories_family_id_and_name.rb(finalUPDATE budget_categories SET category_id = map.keeper_id ...block, the "reassign remaining non-colliding duplicate rows" step): if a family has two or more duplicate-named categories that each have abudget_categoriesrow for the same budget, and the keeper category has no row for that budget, neither duplicate collides with an existing keeper row, so both are skipped by the earlier "sum into keeper" and "delete colliding" steps. The final blanket reassignment then tries to give both rows(budget_id, category_id=keeper_id)in a singleUPDATE, which violates the very unique index this migration is adding (index_budget_categories_on_budget_id_and_category_id) and would fail the migration for that family. The test suite only covers the single-duplicate-per-budget case, so this wouldn't be caught. Consider extending the "sum into keeper" logic to also aggregate duplicate-vs-duplicate collisions (not just duplicate-vs-keeper) before the final reassignment.- Everything else — the dedup ordering (oldest row kept via
created_at ASC, id ASC), remapping oftransactions, subcategoryparent_id,import_mappings/import_source_mappings, andrule_actions/rule_conditions— looks correct, and the migration test coverage for the budget_categories sum/reassign/import-mapping paths is solid (Minitest, no RSpec, per convention).
Generated by Claude Code
When several duplicate-named categories each have a budget_categories row for the same budget and the keeper has none, the final UPDATE would violate the (budget_id, category_id) unique index. Sum extras into one survivor and delete the rest first. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@jjmata Good catch — that multi-duplicate / no-keeper-row path would have failed the migration on Fixed by collapsing duplicate-vs-duplicate |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/migrations/add_unique_index_on_categories_family_id_and_name_test.rb`:
- Around line 70-104: Add an assertion immediately after `@migration.up` in the
test “collapses multiple duplicate budget_categories for the same budget when
keeper has none” that verifies the unique index has been restored, using the
existing index-checking helper or established schema assertion. Keep
ensure_unique_index! only as cleanup and preserve the existing deletion and
consolidation assertions.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52c1a21a-23c7-43a4-a426-e2bc965c3929
📒 Files selected for processing (2)
db/migrate/20260808120000_add_unique_index_on_categories_family_id_and_name.rbtest/migrations/add_unique_index_on_categories_family_id_and_name_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- db/migrate/20260808120000_add_unique_index_on_categories_family_id_and_name.rb
|
Bot triage: this is a DB-level unique constraint per convention, and the migration is safe on data that already has duplicates — it dedupes The one real edge case — two-or-more duplicate categories sharing a Generated by Claude Code |
Resolve db/schema.rb version conflict by keeping main's latest migration stamp (2026_08_09_000000) while retaining the categories unique index from this branch. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks @sure-design — agreed on the merge readiness (including the multi-dup Resolved the |
|
Update: new commit since the last triage ( Generated by Claude Code |
3 participants
Summary
Closes #929:
Family#investment_contributions_category(and otherfind_or_create_by!category paths) rescueActiveRecord::RecordNotUnique, but the DB only indexedcategories.family_id, so races could still insert duplicates.(family_id, name)collisions (keeping the oldest row) and reassignstransactions,budget_categories, subcategoryparent_id,import_mappings, and rule action/condition values before deleting extrasindex_categories_on_family_id_and_nameRecordNotUniquewhen validations are bypassedTest plan
bin/rails db:migrateon a DB that may already have duplicate category namesbin/rails test test/models/category_test.rbCloses #929
Summary by CodeRabbit
Bug Fixes
Data Integrity
Tests