Ensure senses are created before adding complex-form-components #1945
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughIntroduces “without references” add/replace APIs and updates diffing logic to create entries and referenced children before full updates. Adjusts EntrySync to stage reference syncing, adds Entry.WithEntryRefsFrom, and updates tests, including a new scenario syncing a complex-form component that references a newly created sense. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNone found. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
💻 FW Lite
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
backend/FwLite/MiniLcm/Models/Entry.cs (1)
85-88: Avoid list aliasing in WithEntryRefsFrom to prevent unintended mutationsReturning the same list instances from
fromcan lead to aliasing between objects used as "before" and "after" in later diffs. Safer to copy the lists (shallow copy of components is sufficient; you already have.Copy()for deep copy consistency).Apply this diff:
- public Entry WithEntryRefsFrom(Entry from) - { - return this with { Components = from.Components, ComplexForms = from.ComplexForms }; - } + public Entry WithEntryRefsFrom(Entry from) + { + // copy to avoid aliasing with the source instance + return this with + { + Components = [.. from.Components.Select(c => c.Copy())], + ComplexForms = [.. from.ComplexForms.Select(c => c.Copy())] + }; + }backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs (1)
228-263: Optionally parameterize entry ordering to harden against ordering variationRight now the test fixes the entry with the new sense at the end. Since DiffAddThenUpdate’s stage-1 pass processes entries in
afterorder, consider a[Theory]variant that flips theafterarray order to prove invariance.backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs (1)
109-119: Tighten comment and variable naming for clarity in ReplaceWithoutReferencesAndGetMinor clarity nit: the comment references the old method name, and the variable name can better convey its role as the "before" state for the final replace, carrying only refs from the original before.
Apply this diff:
- { - //same as AddAndGet, but for already existing entries, because they + { + // same as AddWithoutReferencesAndGet, but for already existing entries, because they //might have new entities (e.g. senses) in their hierarchy that other entries reference var beforeEntryWithoutEntryRefs = beforeEntry.WithoutEntryRefs(); var afterEntryWithoutEntryRefs = afterEntry.WithoutEntryRefs(); var changes = await Sync(beforeEntryWithoutEntryRefs, afterEntryWithoutEntryRefs, api); //We've synced everything except the refs - var updatedBeforeEntry = afterEntry.WithEntryRefsFrom(beforeEntry); - return (changes, updatedBeforeEntry); + var beforeForRefs = afterEntry.WithEntryRefsFrom(beforeEntry); + return (changes, beforeForRefs); }backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs (2)
13-22: Consider XML docs to describe stage-1 semantics and expectationsA brief summary on both methods explaining they perform the “without cross-entity references” phase and must return the object representing the stage-1 “before” state for the subsequent Replace would help future implementers avoid subtle ordering/aliasing bugs.
68-79: Clarify comments to reflect the two-stage processThe comments can be more explicit that this loop performs stage 1 (non-reference creations) and defers stage 2 (full update with references).
Apply this diff:
- // ensure all children that might be referenced are created + // Stage 1: ensure non-reference children (e.g., senses) are created var (changed, replacedEntry) = await diffApi.ReplaceWithoutReferencesAndGet(beforeEntry, afterEntry); changes += changed; - postAddUpdates.Add((replacedEntry, afterEntry)); // defer full update + postAddUpdates.Add((replacedEntry, afterEntry)); // Stage 2: defer full update (incl. cross-entry refs) } else { - // create new entry with children that might be referenced + // Stage 1: create new entry with non-reference children (e.g., senses) var (change, created) = await diffApi.AddWithoutReferencesAndGet(afterEntry); changes += change; - postAddUpdates.Add((created, afterEntry)); // defer full update + postAddUpdates.Add((created, afterEntry)); // Stage 2: defer full update (incl. cross-entry refs) }backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs (1)
259-273: Reword the explanatory comment to match the actual sequencingThe current comment suggests “new entries must be created first,” but the verified call order shows stage-1 Replace on existing items can safely run before stage-1 Add of new items because references are deferred to stage 2.
Apply this diff:
- //this order is required because new entries (and/or new senses etc.) must be created first - //updated entries might reference the newEntry (or a new sense) and so must be updated after the new entry is created. - //the order of the "simple" Replace calls is unimportant. + // The key requirement is that all stage-1 operations (without cross-entry refs) complete before any stage-2 Replace. + // In our flow, stage-1 Replace can run before stage-1 Add because references are deferred to stage 2. + // The order of the final "simple" Replace calls is unimportant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs(1 hunks)backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs(3 hunks)backend/FwLite/MiniLcm/Models/Entry.cs(1 hunks)backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs(2 hunks)backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-27T09:24:39.507Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable<IChange>) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Applied to files:
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.csbackend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs
🧬 Code Graph Analysis (5)
backend/FwLite/MiniLcm/Models/Entry.cs (1)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
Entry(648-677)
backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs (3)
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs (15)
Task(12-12)Task(13-17)Task(18-22)Task(23-23)Task(24-24)Task(38-38)Task(39-39)Task(40-40)Task(41-41)Task(54-94)Task(96-123)Task(125-176)Task(259-264)Guid(30-33)Guid(43-46)backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs (3)
Guid(181-185)Guid(224-228)EntrySync(8-306)backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs (3)
Guid(36-45)ComplexFormComponent(7-22)ComplexFormComponent(53-66)
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs (2)
backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs (6)
Task(158-171)Task(281-285)Task(287-291)Task(293-297)Task(299-303)Task(305-309)backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs (8)
Task(10-15)Task(17-35)Task(37-46)Task(48-57)Task(59-66)Task(68-75)Task(77-83)Task(100-107)
backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs (2)
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs (13)
Task(12-12)Task(13-17)Task(18-22)Task(23-23)Task(24-24)Task(38-38)Task(39-39)Task(40-40)Task(41-41)Task(54-94)Task(96-123)Task(125-176)Task(259-264)backend/FwLite/MiniLcm/Models/Entry.cs (3)
Entry(43-69)Entry(80-83)Entry(85-88)
backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs (2)
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs (14)
Task(12-12)Task(13-17)Task(18-22)Task(23-23)Task(24-24)Task(38-38)Task(39-39)Task(40-40)Task(41-41)Task(54-94)Task(96-123)Task(125-176)Task(259-264)DiffCollection(49-242)backend/FwLite/MiniLcm/Models/Entry.cs (3)
Entry(43-69)Entry(80-83)Entry(85-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Analyze (csharp)
- GitHub Check: frontend-component-unit-tests
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend
🔇 Additional comments (6)
backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs (1)
228-263: Excellent targeted test validating FK-safe two-phase syncThis test covers the exact failure mode (component referencing a newly created sense) and demonstrates the two-stage flow preventing FK violations. Assertions look correct with strict ordering and exclusions.
backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs (1)
100-107: Two-phase creation (without references) is correct and aligns with FK constraintsCreating entries (including senses, example sentences, etc.) without cross-entry refs first is the right approach to avoid FK failures when adding complex-form components.
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs (1)
13-22: New stage-1 hooks (Add/ReplaceWithoutReferencesAndGet) are a good extension pointDefault implementations are sensible fallbacks; they enable specialized two-phase behavior where needed (e.g., entries/senses) without forcing it on all call sites.
backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs (3)
234-236: Updated expectation for Add path looks correctVerifying AddWithoutReferencesAndGet followed by Replace(created, after) matches the new two-phase contract.
253-256: Updated expectation for Replace path looks correctEnsuring ReplaceWithoutReferencesAndGet precedes the final Replace aligns with the “creation-before-update” sequence.
299-310: FakeDiffApi overrides reflect the new API surface and sequencingThe test double changes correctly capture and verify call order for stage-1 methods.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
myieye
deleted the
1944-project-sync-blocked-foreign-key-constraint-failed
branch
2 participants
Fixes #1944