Unify behavior of adding and updating projected entities #58
Conversation
WalkthroughRefactors antonym reference handling: introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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)
src/SIL.Harmony.Sample/Models/Word.cs (1)
55-55: Potential circular reference in ToString.Including
AntonyminToString()could cause a circular reference issue if the antonym Word'sToString()is called (e.g., when debugging or logging), since Word → Antonym → Word. Consider usingAntonym?.Idinstead ofAntonymto avoid potential stack overflow.Apply this diff:
- $"{nameof(Text)}: {Text}, {nameof(Id)}: {Id}, {nameof(Note)}: {Note}, {nameof(DeletedAt)}: {DeletedAt}, {nameof(Antonym)}: {Antonym}, {nameof(AntonymId)}: {AntonymId}, {nameof(ImageResourceId)}: {ImageResourceId}" + + $"{nameof(Text)}: {Text}, {nameof(Id)}: {Id}, {nameof(Note)}: {Note}, {nameof(DeletedAt)}: {DeletedAt}, {nameof(AntonymId)}: {AntonymId}, {nameof(ImageResourceId)}: {ImageResourceId}" +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/SIL.Harmony.Sample/Changes/AddAntonymReferenceChange.cs(1 hunks)src/SIL.Harmony.Sample/Changes/NewWordChange.cs(1 hunks)src/SIL.Harmony.Sample/CrdtSampleKernel.cs(2 hunks)src/SIL.Harmony.Sample/Models/Word.cs(4 hunks)src/SIL.Harmony.Tests/DataModelReferenceTests.cs(2 hunks)src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/SIL.Harmony.Sample/Changes/AddAntonymReferenceChange.cs (1)
src/SIL.Harmony.Sample/Models/Word.cs (2)
Guid(17-26)Word(5-58)
src/SIL.Harmony.Sample/Changes/NewWordChange.cs (1)
src/SIL.Harmony.Sample/Models/Word.cs (1)
Word(5-58)
src/SIL.Harmony.Sample/CrdtSampleKernel.cs (1)
src/SIL.Harmony.Sample/Models/Word.cs (1)
Word(5-58)
src/SIL.Harmony.Tests/DataModelReferenceTests.cs (3)
src/SIL.Harmony.Sample/Changes/AddAntonymReferenceChange.cs (1)
AddAntonymReferenceChange(7-25)src/SIL.Harmony.Sample/Models/Word.cs (2)
Word(5-58)Guid(17-26)src/SIL.Harmony.Sample/Changes/NewWordChange.cs (1)
NewWordChange(7-19)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
src/SIL.Harmony.Sample/CrdtSampleKernel.cs (1)
63-66: LGTM! Relationship configuration is correct.The one-to-one self-referencing relationship configuration for
Word.Antonymis properly structured withHasForeignKey<Word>(w => w.AntonymId)andOnDelete(DeleteBehavior.SetNull), which appropriately handles the optional nature of the antonym reference.src/SIL.Harmony.Sample/Changes/AddAntonymReferenceChange.cs (1)
7-24: LGTM! Clean implementation of conditional navigation property setting.The
SetObjectparameter effectively enables testing both behaviors:
- When
true: sets both navigation property and FK (demonstrates the bug)- When
false: sets only FK (demonstrates desired behavior)The guard against null/deleted antonyms is appropriate and prevents invalid references.
src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt (1)
192-208: LGTM! Model verification correctly reflects the relationship.The verified model accurately documents:
AntonymIdas a unique FKAntonymnavigation property- One-to-one relationship with
SetNulldelete behavior- Proper constraints and indexes
src/SIL.Harmony.Tests/DataModelReferenceTests.cs (5)
20-45: LGTM! Test correctly validates reference between existing entities.The test appropriately:
- Creates a reference between two existing entities
- Conditionally verifies navigation property in snapshots based on
includeObjectInSnapshot- Always verifies navigation property in projected entities (loaded via EF
Include)This distinction is correct: snapshots may selectively include navigation properties, while projected entities load them through EF relationship configuration.
47-81: LGTM! Critical test for demonstrating the tracking issue.This test creates a new entity and adds a forward reference in the same commit. When
includeObjectInSnapshot=true, theAddAntonymReferenceChangesets theAntonymnavigation property, which may cause EF Core to incorrectly track the referenced entity (word1) as Added, even though it already exists. This is the core issue this PR demonstrates.
83-117: LGTM! Tests reverse reference scenario effectively.This test validates the reverse case: a new entity (
word3) is created, and an existing entity (word1) is updated to reference it. WhenincludeObjectInSnapshot=true, settingword1.Antonym = word3might cause EF tracking issues ifword3is being added in the same operation.The assertions correctly verify
word1(the entity with the reference) rather thanword3.
119-152: LGTM! Tests sync-based path for the same scenario.This test validates the sync path where the new entity and forward reference are applied via
AddCommitsViaSyncrather than direct commit. This ensures the issue is demonstrated in both the direct commit path and the sync path, which may have different EF Core tracking contexts.
154-187: LGTM! Completes sync-path coverage for reverse reference.This test validates the sync path for the reverse reference scenario, completing the test matrix:
- Forward/reverse reference × direct commit/sync
- With/without navigation property (via
includeObjectInSnapshotparameter)Together, these tests comprehensively demonstrate the tracking issue across different application paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/SIL.Harmony.Sample/Changes/AddAntonymReferenceChange.cs (2)
7-11:SetObjectflag design makes sense; double‑check serialization/back‑compatUsing the
setObjectctor parameter andSetObjectproperty is a good way to control whether the navigation is populated in projections while keeping the default behavior (true) unchanged for new events. The only thing to verify is how your serializer handles this new optional parameter for existing stored changes that don’t have aSetObjectvalue: you want them to deserialize withSetObject == trueto preserve prior semantics.If you’re not already doing so, confirm your serializer uses the constructor’s optional default (
setObject = true) when the JSON omits that parameter; otherwise you may want an explicit[JsonConstructor]and/or a migration to setSetObjecttotrueon existing data.
13-24: ApplyChange flow correctly guards against deleted/missing antonyms and honorsSetObjectThe updated
ApplyChangeimplementation:
- Fetches the current
WordbyAntonymIdand skips the change if the target is missing or soft‑deleted, matching the comment and the “downward” semantics (the reference is dropped when the antonym goes away).- Always updates
AntonymIdwhile only populatingentity.AntonymwhenSetObjectis true, which is exactly what you need to toggle between “FK‑only projection” and “include navigation object in the snapshot”.This looks logically sound and aligns with
Word.RemoveReference/NewWordChangebehavior.If you want to be ultra‑explicit, you could set
entity.AntonymId = antonym.Id;instead ofAntonymId, though they should always be the same in practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SIL.Harmony.Sample/Changes/AddAntonymReferenceChange.cs(1 hunks)src/SIL.Harmony.Sample/CrdtSampleKernel.cs(2 hunks)src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: myieye
Repo: sillsdev/harmony PR: 58
File: src/SIL.Harmony.Sample/Changes/NewWordChange.cs:15-17
Timestamp: 2025-11-25T12:21:33.047Z
Learning: In SIL.Harmony.Sample: Word.AntonymId is considered a "downward" relationship, so it should be nulled when the antonym is deleted. This is different from parent references like Definition.WordId, which are "upward" and should be preserved even if the parent is deleted.
📚 Learning: 2025-11-25T12:21:33.047Z
Learnt from: myieye
Repo: sillsdev/harmony PR: 58
File: src/SIL.Harmony.Sample/Changes/NewWordChange.cs:15-17
Timestamp: 2025-11-25T12:21:33.047Z
Learning: In SIL.Harmony.Sample: Word.AntonymId is considered a "downward" relationship, so it should be nulled when the antonym is deleted. This is different from parent references like Definition.WordId, which are "upward" and should be preserved even if the parent is deleted.
Applied to files:
src/SIL.Harmony.Sample/Changes/AddAntonymReferenceChange.cssrc/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txtsrc/SIL.Harmony.Sample/CrdtSampleKernel.cs
🧬 Code graph analysis (1)
src/SIL.Harmony.Sample/Changes/AddAntonymReferenceChange.cs (4)
src/SIL.Harmony.Sample/Models/Word.cs (2)
Guid(17-26)Word(5-58)src/SIL.Harmony.Core/IObjectBase.cs (1)
Guid(16-16)src/SIL.Harmony.Sample/Changes/NewWordChange.cs (1)
ValueTask(13-18)src/SIL.Harmony/Changes/Change.cs (6)
ValueTask(18-18)ValueTask(19-19)ValueTask(49-52)ValueTask(54-54)ValueTask(55-55)ValueTask(57-73)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
src/SIL.Harmony.Sample/CrdtSampleKernel.cs (1)
60-67: Antonym self‑reference mapping looks consistent with model and delete semanticsThe
HasOne(w => w.Antonym)...HasForeignKey(w => w.AntonymId).OnDelete(DeleteBehavior.SetNull)self‑FK cleanly models an optional antonym reference, andSetNullmatches the “downward” semantics forWord.AntonymId(null the FK when the antonym is deleted). No issues from a mapping perspective.You can re-run the existing
DbContextTests.VerifyModelsnapshot test to confirm the configured relationship matches expectations.src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt (1)
189-210: Model snapshot correctly reflects the new Antonym FK and navigationThe snapshot lines for
Word(AntonymId property, Antonym navigation, SetNull FK, and AntonymId index) all match the EF configuration and theWordmodel. This keeps the verification file in sync with the mapping changes.Please re-run the model verification test to ensure the snapshot still matches the runtime model.
myieye
changed the title
It does not add to a collection.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/SIL.Harmony.Sample/Changes/SetAntonymReferenceChange.cs(1 hunks)src/SIL.Harmony.Sample/CrdtSampleKernel.cs(3 hunks)src/SIL.Harmony.Tests/DataModelReferenceTests.cs(5 hunks)src/SIL.Harmony/Db/CrdtRepository.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: myieye
Repo: sillsdev/harmony PR: 58
File: src/SIL.Harmony.Sample/Changes/NewWordChange.cs:15-17
Timestamp: 2025-11-25T12:21:33.071Z
Learning: In SIL.Harmony.Sample: Word.AntonymId is considered a "downward" relationship, so it should be nulled when the antonym is deleted. This is different from parent references like Definition.WordId, which are "upward" and should be preserved even if the parent is deleted.
📚 Learning: 2025-11-25T12:21:33.071Z
Learnt from: myieye
Repo: sillsdev/harmony PR: 58
File: src/SIL.Harmony.Sample/Changes/NewWordChange.cs:15-17
Timestamp: 2025-11-25T12:21:33.071Z
Learning: In the SIL.Harmony CRDT system: when an entity is deleted, relationships going DOWN the hierarchy are nulled, but relationships going UP the hierarchy are preserved. Deleted entities keep track of their parents, but deleted children are removed from parent references.
Applied to files:
src/SIL.Harmony/Db/CrdtRepository.cs
📚 Learning: 2025-11-25T12:21:33.071Z
Learnt from: myieye
Repo: sillsdev/harmony PR: 58
File: src/SIL.Harmony.Sample/Changes/NewWordChange.cs:15-17
Timestamp: 2025-11-25T12:21:33.071Z
Learning: In SIL.Harmony.Sample: Word.AntonymId is considered a "downward" relationship, so it should be nulled when the antonym is deleted. This is different from parent references like Definition.WordId, which are "upward" and should be preserved even if the parent is deleted.
Applied to files:
src/SIL.Harmony.Sample/Changes/SetAntonymReferenceChange.cssrc/SIL.Harmony.Tests/DataModelReferenceTests.cssrc/SIL.Harmony.Sample/CrdtSampleKernel.cs
🧬 Code graph analysis (4)
src/SIL.Harmony/Db/CrdtRepository.cs (1)
src/SIL.Harmony/Db/ObjectSnapshot.cs (2)
ObjectSnapshot(32-80)ObjectSnapshot(34-48)
src/SIL.Harmony.Sample/Changes/SetAntonymReferenceChange.cs (2)
src/SIL.Harmony/Changes/EditChange.cs (1)
EditChange(3-11)src/SIL.Harmony.Sample/Models/Word.cs (1)
Word(5-58)
src/SIL.Harmony.Tests/DataModelReferenceTests.cs (5)
src/SIL.Harmony.Sample/Changes/SetAntonymReferenceChange.cs (1)
SetAntonymReferenceChange(7-25)src/SIL.Harmony.Sample/Models/Word.cs (2)
Word(5-58)Guid(17-26)src/SIL.Harmony.Tests/Adapter/CustomObjectAdapterTests.cs (1)
Guid(108-108)src/SIL.Harmony.Tests/PersistExtraDataTests.cs (1)
Guid(30-33)src/SIL.Harmony.Sample/Models/WordTag.cs (1)
Guid(12-15)
src/SIL.Harmony.Sample/CrdtSampleKernel.cs (1)
src/SIL.Harmony.Sample/Changes/SetAntonymReferenceChange.cs (1)
SetAntonymReferenceChange(7-25)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
src/SIL.Harmony/Db/CrdtRepository.cs (1)
365-369: LGTM! This correctly addresses the Add/Update inconsistency.Using
Entry(entity).State = EntityState.Addedinstead ofAdd(entity)prevents EF Core from traversing and marking the entire object graph as Added. This resolves the tracking conflicts and UNIQUE constraint violations described in the PR objectives when navigation properties reference existing entities.src/SIL.Harmony.Sample/CrdtSampleKernel.cs (2)
46-46: Change type registration updated correctly.The rename from
AddAntonymReferenceChangetoSetAntonymReferenceChangealigns with the broader change semantics introduced in this PR.
63-66: Relationship configuration is appropriate.The
OnDelete(DeleteBehavior.SetNull)correctly implements the "downward" relationship semantics whereAntonymIdshould be nulled when the referenced antonym is deleted. This aligns with the retrieval-based validation inSetAntonymReferenceChange.ApplyChangeand the learned pattern for this codebase.src/SIL.Harmony.Sample/Changes/SetAntonymReferenceChange.cs (2)
7-11: Good design with thesetObjectparameter.The
SetObjectflag provides flexibility for consumers to control whether the navigation property is populated. Whenfalse, only the foreign key is set, which avoids the object graph tracking issues that motivated this PR.
19-23: Retrieval-based validation handles sync edge cases correctly.The early-return for deleted or missing antonyms properly handles scenarios where a referenced entity is deleted before this change is applied during sync. The conditional navigation assignment based on
SetObjectsupports both snapshot inclusion and FK-only modes.src/SIL.Harmony.Tests/DataModelReferenceTests.cs (5)
20-49: Well-structured parameterized test covering both SetObject modes.The test properly validates that the projected entity always has the navigation populated (via EF Include), while the snapshot respects the
includeObjectInSnapshotflag. Good coverage of the core Add reference behavior.
129-167: Tests the critical scenario from the PR description.This test specifically validates "adding a navigation property in the same commit that created the parent entity" - one of the failure scenarios mentioned in the PR objectives. Good coverage.
169-207: Good reverse-reference test case.Testing the case where an existing entity (
_word1Id) gets a reference to a newly created entity (word3Id) in the same commit covers an important edge case for graph tracking.
209-285: Sync-based tests properly useAddCommitsViaSync.These tests correctly simulate sync scenarios with separate commits being synced together, which exercises the code paths mentioned in the PR objectives.
287-333: Existing tests updated to useSetAntonymReferenceChange.The rename from
AddAntonymReferenceChangetoSetAntonymReferenceChangeis consistently applied to the existing retroactive delete and reference removal tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/SIL.Harmony.Tests/DataModelReferenceTests.cs (1)
247-284: Reverse reference in sync flow is covered; consider optional helper extraction.
AddEntityAndReverseReferenceInSameSyncWorkscompletes the matrix of scenarios by verifying that an existing word can safely reference a newly created antonym across a sync batch, again checking snapshot vs. projection. Given the high repetition across the new antonym tests, you could optionally extract small helpers (e.g.,AssertWordSnapshot/AssertProjectedWord) to reduce duplication, but this is purely a readability choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/SIL.Harmony.Tests/DataModelReferenceTests.cs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: myieye
Repo: sillsdev/harmony PR: 58
File: src/SIL.Harmony.Sample/Changes/NewWordChange.cs:15-17
Timestamp: 2025-11-25T12:21:33.071Z
Learning: In SIL.Harmony.Sample: Word.AntonymId is considered a "downward" relationship, so it should be nulled when the antonym is deleted. This is different from parent references like Definition.WordId, which are "upward" and should be preserved even if the parent is deleted.
📚 Learning: 2025-11-25T12:21:33.071Z
Learnt from: myieye
Repo: sillsdev/harmony PR: 58
File: src/SIL.Harmony.Sample/Changes/NewWordChange.cs:15-17
Timestamp: 2025-11-25T12:21:33.071Z
Learning: In SIL.Harmony.Sample: Word.AntonymId is considered a "downward" relationship, so it should be nulled when the antonym is deleted. This is different from parent references like Definition.WordId, which are "upward" and should be preserved even if the parent is deleted.
Applied to files:
src/SIL.Harmony.Tests/DataModelReferenceTests.cs
🧬 Code graph analysis (1)
src/SIL.Harmony.Tests/DataModelReferenceTests.cs (2)
src/SIL.Harmony.Sample/Changes/SetAntonymReferenceChange.cs (1)
SetAntonymReferenceChange(7-25)src/SIL.Harmony.Sample/Models/Word.cs (2)
Word(5-58)Guid(17-26)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
src/SIL.Harmony.Tests/DataModelReferenceTests.cs (7)
20-49: AddReferenceWorks robustly covers snapshot vs. projected behavior forSetAntonymReferenceChange.This test nicely validates both snapshot state (including the
SetObjectflag impact) and the projected EF entity behavior where the navigation is always materialized viaInclude, which directly exercises the unified add/update projection logic. No issues from a correctness perspective.
51-88: Good coverage of multiple reference updates in a single commit.
UpdateReferenceTwiceInSameCommitWorkscorrectly exercises last-write-wins semantics when twoSetAntonymReferenceChangeoperations target the same word in one commit, with bothsetObjectmodes exercised via the theory data. This should guard against regressions where earlier navigation assignments might leak through to snapshots or projections.
90-126: Sync variant correctly mirrors the multi-commit update scenario.
UpdateReferenceTwiceInSameSyncWorksnow usesAddCommitsViaSyncwith two separate commits, matching its name and ensuring that sync application order (first_word1Id, then_word2Id) yields the expected final snapshot and projected entity. This complements the single-commit test and looks correct.
128-166: Strong regression test for “add entity + reference in same commit” path.
AddEntityAndReferenceInSameCommitWorksdirectly validates the bug scenario from the PR description: a newly addedWordwith a navigation reference to an existing antonym in the same commit. Asserting both snapshot (includeObjectInSnapshoton/off) and projected entity state should catch any future reintroduction of recursiveAddbehavior on the antonym.
168-206: Reverse-reference-in-same-commit scenario is well covered.
AddEntityAndReverseReferenceInSameCommitWorksnicely covers the case where an existing word gains a reference to a newly created word within the same commit, ensuring that intra-commit resolution viaGetCurrent<Word>behaves as intended and that projections don’t attempt to add a duplicate tracked instance. Assertions look consistent with other tests.
208-245: Sync-based “add entity + reference” scenario aligns with unified projection semantics.
AddEntityAndReferenceInSameSyncWorksuses two commits added via sync to model creation followed by a reference update, and asserts both snapshot and projected states acrossincludeObjectInSnapshotvalues. This directly exercises theEntry(entity).State = Addedpath under sync, and the expectations appear correct.
289-330: Updated delete/retro tests align withSetAntonymReferenceChangeand downward-reference semantics.Switching from
AddAntonymReferenceChangetoSetAntonymReferenceChangein the delete/retroactive tests keeps their intent intact: references should be removed when the antonym is deleted, and history rewrites should prevent references to already-deleted words from being (re)applied. This matches the design thatWord.AntonymIdis a downward relationship that should be nulled on antonym deletion.
myieye
deleted the
57-unify-behavior-of-adding-and-updating-projected-entities
branch
Resolves #57
This test run demonstrated the inconsistency between add vs update: https://github.com/sillsdev/harmony/actions/runs/19669046871/job/56333060722?pr=58
The only failures were the scenarios in which the navigation property was set in the same commit and/or sync as the parent entity was created. So:
With this PR: everything works.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.