Make NewEntry override deleted #54
Conversation
WalkthroughAdds capability signaling methods to changes, refactors snapshot application into explicit new/revive/apply-or-skip branches, changes per-entity projection gating and save logic in the repository, and adds tests covering delete/undelete/create scenarios plus minor test formatting tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as SnapshotWorker
participant Change as Change<T> / IChange
participant Prev as PreviousSnapshot
participant SnapGen as SnapshotGenerator
Note over Worker: Process one commit change (high-level)
alt No previous snapshot
Worker->>Change: NewEntity(context)
Change-->>Worker: newEntity
Worker->>SnapGen: GenerateSnapshot(newEntity, commit)
else Previous snapshot exists
alt Prev.deleted and Change.SupportsNewEntity() == true
Note over Worker: Revival (NewEntity revives deleted)
Worker->>Change: NewEntity(previous, context)
Change-->>Worker: revivedEntity
Worker->>SnapGen: GenerateSnapshot(revivedEntity, commit)
else Change.SupportsApplyChange() == true
Note over Worker: Apply-to-existing
Worker->>Prev: copy entity
Worker->>Change: ApplyChange(copiedEntity, context)
Change-->>Worker: appliedEntity (may be marked deleted)
Worker->>SnapGen: GenerateSnapshot(appliedEntity, commit)
else
Note over Worker: Unsupported → skip change
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Files/areas to pay extra attention to:
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: 1
🧹 Nitpick comments (2)
src/SIL.Harmony/SnapshotWorker.cs (1)
107-113: Clarify the comment to cover both deleted and non-deleted cases.The fallback path is reached when the change cannot be applied to the entity, which includes:
- Existing non-deleted entity where change doesn't support ApplyChange
- Deleted entity where change supports neither NewEntity nor ApplyChange
The current comment only mentions case 1.
Consider updating the comment to be more comprehensive:
else { - // entity already exists (and is not deleted) - // and change does not support updating existing entities - // so do nothing + // change cannot be applied to entity: + // - if entity exists and is not deleted: change doesn't support ApplyChange + // - if entity is deleted: change supports neither NewEntity nor ApplyChange + // so skip without generating snapshot continue; }src/SIL.Harmony.Tests/SnapshotTests.cs (1)
193-213: Consider merging with the previous test.This test (
SupportsNewEntityAllowsUpdatingDeletedEntities) appears to duplicate the logic of the previous test (CanApplyChangeToDeletedEntityWithSupportsNewEntity). Both tests:
- Create a word with
SetWordTextChange- Delete it
- Apply another
SetWordTextChangeto the deleted entity- Assert the text is updated and
DeletedAtremains setUnless there's a subtle difference in behavior being tested, consider consolidating these into a single, well-named test to reduce maintenance burden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SIL.Harmony.Tests/SnapshotTests.cs(5 hunks)src/SIL.Harmony/Changes/Change.cs(3 hunks)src/SIL.Harmony/SnapshotWorker.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/SIL.Harmony/SnapshotWorker.cs (1)
src/SIL.Harmony/Changes/Change.cs (6)
Change(27-73)Change(29-32)SupportsNewEntity(20-20)SupportsNewEntity(66-69)SupportsApplyChange(19-19)SupportsApplyChange(61-64)
src/SIL.Harmony/Changes/Change.cs (2)
src/SIL.Harmony/Changes/CreateChange.cs (1)
CreateChange(3-9)src/SIL.Harmony/Changes/EditChange.cs (1)
EditChange(3-11)
src/SIL.Harmony.Tests/SnapshotTests.cs (6)
src/SIL.Harmony/DataModel.cs (2)
DataModel(14-371)DataModel(28-39)src/SIL.Harmony.Sample/Models/Word.cs (2)
Word(5-52)Guid(16-25)src/SIL.Harmony.Sample/Changes/SetWordTextChange.cs (1)
SetWordTextChange(11-30)src/SIL.Harmony.Sample/Changes/NewWordChange.cs (1)
NewWordChange(7-18)src/SIL.Harmony/Changes/DeleteChange.cs (1)
DeleteChange(5-15)src/SIL.Harmony/Changes/Change.cs (2)
SupportsNewEntity(20-20)SupportsNewEntity(66-69)
⏰ 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/Changes/Change.cs (3)
19-20: LGTM! Clear capability signaling interface.The new methods provide a clean way to query change capabilities without relying on runtime type checking.
49-50: LGTM! Cleaner than runtime type checking.The refactoring to use
SupportsApplyChange()improves code clarity and maintainability.
61-64: LGTM! Logic correctly identifies CreateChange limitation.The implementation correctly returns
falseforCreateChange<T>instances, which cannot apply changes to existing entities.src/SIL.Harmony/SnapshotWorker.cs (2)
85-94: LGTM! Clear separation of new entity and revival paths.The three-state logic (new, revival, update) is well-structured and the comments clearly explain each path. This will work correctly once the bug in
Change.csline 66-69 is fixed.
95-106: LGTM! ApplyChange path correctly preserves entity state.The implementation correctly:
- Copies the previous entity to preserve existing data
- Applies the change
- Handles new deletions appropriately
src/SIL.Harmony.Tests/SnapshotTests.cs (3)
24-24: LGTM! Minor style improvement.Using
varfor loop counters is more concise and consistent with modern C# conventions.
167-191: LGTM! Comprehensive test for applying changes to deleted entities.The test correctly validates that:
- Changes can be applied to deleted entities when using change types that support ApplyChange
- The deletion state is preserved (DeletedAt remains set)
- Entity data is updated appropriately
This test will pass once the bug in
Change.csline 66-69 is fixed.
215-231: LGTM! Essential test for capability signaling.This test correctly validates the
SupportsNewEntity()behavior across different change type hierarchies:
SetWordTextChange(plainChange<T>): false - cannot create new entities, should use ApplyChangeNewWordChange(CreateChange<T>): true - designed for creating new entitiesDeleteChange(EditChange<T>): false - only operates on existing entitiesThis test will pass once the critical bug in
Change.csline 66-69 is fixed.
myieye
force-pushed
the
create-changes-override-delete
branch
from
6284dfa to
0fe7a07
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/SIL.Harmony/SnapshotWorker.cs (2)
96-106: Minor: computewasDeletedfrom the copied entity for symmetry.Functionally equivalent today, but safer if
EntityIsDeletedsemantics ever diverge fromentity.DeletedAt.HasValue.- var wasDeleted = prevSnapshot.EntityIsDeleted; + var wasDeleted = entity.DeletedAt.HasValue;
107-113: Consider logging skipped changes.The skip path is silent. Add a trace/debug with commit/change ids to aid diagnosing dropped changes during sync/merge.
Would you like a small
ILogger<SnapshotWorker>injection andLogDebugcall here?src/SIL.Harmony/Changes/Change.cs (3)
19-20: Add XML docs clarifying capability semantics.Briefly document that:
- SupportsApplyChange: change can mutate an existing entity (incl. deleted).
- SupportsNewEntity: change can construct a new entity (or revive).
49-51: No-op on unsupported apply may hide misuse.Returning silently can mask bugs when callers forget to branch on
SupportsApplyChange(). Consider throwingNotSupportedExceptionhere to fail fast, or at least log.Are there external callers relying on the no-op behavior?
61-64: Prefer explicit overrides over runtime type checks.Use sealed overrides in
CreateChange<T>/EditChange<T>to define capabilities, avoidingthis is ...checks in the base.Example:
// in Change<T> - public virtual bool SupportsApplyChange() => this is not CreateChange<T>; + public virtual bool SupportsApplyChange() => true; // in CreateChange<T> + public sealed override bool SupportsApplyChange() => false; // in EditChange<T> + public sealed override bool SupportsApplyChange() => true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/SIL.Harmony.Tests/DataModelSimpleChanges.cs(1 hunks)src/SIL.Harmony.Tests/SnapshotTests.cs(4 hunks)src/SIL.Harmony/Changes/Change.cs(3 hunks)src/SIL.Harmony/SnapshotWorker.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/SIL.Harmony.Tests/SnapshotTests.cs
🧰 Additional context used
🧬 Code graph analysis (3)
src/SIL.Harmony/SnapshotWorker.cs (1)
src/SIL.Harmony/Changes/Change.cs (6)
Change(27-73)Change(29-32)SupportsNewEntity(20-20)SupportsNewEntity(66-69)SupportsApplyChange(19-19)SupportsApplyChange(61-64)
src/SIL.Harmony/Changes/Change.cs (2)
src/SIL.Harmony/Changes/CreateChange.cs (1)
CreateChange(3-9)src/SIL.Harmony/Changes/EditChange.cs (1)
EditChange(3-11)
src/SIL.Harmony.Tests/DataModelSimpleChanges.cs (4)
src/SIL.Harmony/Changes/DeleteChange.cs (1)
DeleteChange(5-15)src/SIL.Harmony.Sample/Models/Word.cs (1)
Word(5-52)src/SIL.Harmony.Sample/Changes/SetWordNoteChange.cs (1)
SetWordNoteChange(7-16)src/SIL.Harmony/Changes/Change.cs (4)
SupportsApplyChange(19-19)SupportsApplyChange(61-64)SupportsNewEntity(20-20)SupportsNewEntity(66-69)
⏰ 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 (3)
src/SIL.Harmony/SnapshotWorker.cs (1)
90-95: Revive-on-create precedence reads correctly.Preferring
NewEntity()when the last snapshot is deleted and the change can create aligns with “Create overrides delete”.src/SIL.Harmony.Tests/DataModelSimpleChanges.cs (2)
193-206: Good coverage: apply to deleted preserves deletion.This validates that edits modify fields without clearing
DeletedAt. Looks correct.
208-220: Good coverage: create overrides delete (undelete).Asserts
SupportsNewEntity()and verifiesDeletedAtcleared; matches revive behavior.Consider adding a test asserting that an
EditChangeon a non-existent entity is skipped (no snapshot) once theSnapshotWorkernull-baseline guard is added. Want me to draft it?
myieye
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/SIL.Harmony/Db/CrdtRepository.cs (2)
302-320: Project only the per-entity latest snapshot; current deletes-first pass can still project twice.Outer grouping by EntityIsDeleted causes older deletes to be projected before newer undeletes; later re-projection corrects state but adds work. Precompute the per-entity latest snapshot once and only project that one.
Minimal change sketch:
- var latestProjectByEntityId = new Dictionary<Guid, (HybridDateTime Time, Guid CommitId)>(); + // Identify the single latest snapshot per entity (time, then commit id) + var latestKeyByEntity = snapshots + .GroupBy(s => s.EntityId) + .ToDictionary( + g => g.Key, + g => { + var latest = g.DefaultOrderDescending().First(); + return (latest.Commit.HybridDateTime, latest.Commit.Id); + }); foreach (var grouping in snapshots.GroupBy(s => s.EntityIsDeleted).OrderByDescending(g => g.Key))//execute deletes first { foreach (var snapshot in grouping.DefaultOrderDescending()) { _dbContext.Add(snapshot); - if (latestProjectByEntityId.TryGetValue(snapshot.EntityId, out var latestProjected)) - { - ... - } - latestProjectByEntityId[snapshot.EntityId] = (snapshot.Commit.HybridDateTime, snapshot.Commit.Id); - - await ProjectSnapshot(snapshot); + var latest = latestKeyByEntity[snapshot.EntityId]; + if (snapshot.Commit.HybridDateTime == latest.Item1 && snapshot.Commit.Id == latest.Item2) + { + await ProjectSnapshot(snapshot); + } }Please verify with an additional unit test where two commits have identical HybridDateTime but different Commit.Id, ensuring the later id wins in projection.
Also applies to: 335-347
335-347: Extra SaveChanges is brittle; may persist unrelated tracked entities.This post-loop SaveChanges “quick fix” can save non-snapshot changes and breaks atomicity if callers expected a single commit boundary. Prefer a single, intentional save strategy (or wrap the whole method in a transaction) and remove this block when #56 lands.
Options:
- Remove this block and ensure the caller saves commits separately.
- Or gate it: only call if snapshots.Any() was false and projection mutated tracked entities (explicit flag).
src/SIL.Harmony.Tests/DeleteAndCreateTests.cs (1)
8-215: Solid coverage of delete/undelete permutations; add an equal-time tie-breaker test.Consider a test where two commits for the same entity share the same HybridDateTime, differing only by Commit.Id, to assert the later id wins in projection. This will guard the projection gating logic.
Happy to sketch that test if your test time provider can pin identical HybridDateTime values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/SIL.Harmony.Tests/DeleteAndCreateTests.cs(1 hunks)src/SIL.Harmony/Db/CrdtRepository.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/SIL.Harmony/Db/CrdtRepository.cs (3)
src/SIL.Harmony.Tests/Mocks/MockTimeProvider.cs (2)
HybridDateTime(5-5)HybridDateTime(11-17)src/SIL.Harmony/DataModel.cs (1)
Commit(102-112)src/SIL.Harmony/Commit.cs (3)
Commit(7-41)Commit(17-21)Commit(28-31)
src/SIL.Harmony.Tests/DeleteAndCreateTests.cs (4)
src/SIL.Harmony.Sample/Changes/NewWordChange.cs (1)
NewWordChange(7-18)src/SIL.Harmony/Changes/DeleteChange.cs (1)
DeleteChange(5-15)src/SIL.Harmony.Sample/Models/Word.cs (1)
Word(5-52)src/SIL.Harmony.Sample/Changes/SetWordNoteChange.cs (1)
SetWordNoteChange(7-16)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/SIL.Harmony/SnapshotWorker.cs (1)
85-89: Guard NewEntity when no previous snapshot to avoid NotSupportedException on EditChange.Call NewEntity only if supported; otherwise skip. Prevents runtime throw on malformed input.
- if (prevSnapshot is null) - { - // create brand new entity - this will (and should) throw if the change doesn't support NewEntity - entity = await commitChange.Change.NewEntity(commit, changeContext); - } + if (prevSnapshot is null) + { + // create brand new entity (only if supported) + if (!commitChange.Change.SupportsNewEntity()) + { + continue; + } + entity = await commitChange.Change.NewEntity(commit, changeContext); + }
🧹 Nitpick comments (3)
src/SIL.Harmony/SnapshotWorker.cs (1)
90-95: Don’t apply edits to a deleted baseline when the change can’t create.If the baseline is deleted and NewEntity isn’t supported, skip instead of falling through to ApplyChange.
- else if (prevSnapshot.EntityIsDeleted && commitChange.Change.SupportsNewEntity()) - { - // revive deleted entity - entity = await commitChange.Change.NewEntity(commit, changeContext); - } + else if (prevSnapshot.EntityIsDeleted) + { + if (commitChange.Change.SupportsNewEntity()) + { + // revive deleted entity + entity = await commitChange.Change.NewEntity(commit, changeContext); + } + else + { + // can't revive and shouldn't edit deleted baseline + continue; + } + }src/SIL.Harmony/Db/CrdtRepository.cs (2)
335-347: Remove the extra SaveChanges or guard it; it adds overhead and hides issues.The second try/catch SaveChanges after per-group saves is redundant and can mask ordering bugs. The comment already notes it’s temporary.
- // this extra try/catch was added as a quick way to get the NewEntityOnExistingEntityIsNoOp test to pass - // it will be removed again in a larger refactor in https://github.com/sillsdev/harmony/pull/56 - try - { - await _dbContext.SaveChangesAsync(); - } - catch (DbUpdateException e) - { - var entries = string.Join(Environment.NewLine, e.Entries.Select(entry => entry.ToString())); - var message = $"Error saving snapshots: {e.Message}{Environment.NewLine}{entries}"; - _logger.LogError(e, message); - throw new DbUpdateException(message, e); - } + // TODO: remove after PR #56; redundant extra savePlease confirm PR #56 makes this safe to drop.
308-319: Skip redundant projection when CompareKey is equal.The proposed change correctly prevents calling
ProjectSnapshotredundantly when two snapshots for the same entity have identicalCompareKeyvalues. This aligns with the intent stated in the comment: "only project the latest one."Since
ProjectSnapshotperforms database operations (add/update) and appears idempotent, the change is a safe optimization that eliminates wasteful duplicate calls within a batch. Change<to<=in the comparison at line 312.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SIL.Harmony/Changes/Change.cs(4 hunks)src/SIL.Harmony/Db/CrdtRepository.cs(2 hunks)src/SIL.Harmony/SnapshotWorker.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/SIL.Harmony/Changes/Change.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/SIL.Harmony/SnapshotWorker.cs (1)
src/SIL.Harmony/Changes/Change.cs (6)
Change(37-87)Change(39-42)SupportsNewEntity(30-30)SupportsNewEntity(80-83)SupportsApplyChange(25-25)SupportsApplyChange(75-78)
⏰ 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 (1)
src/SIL.Harmony/Db/CrdtRepository.cs (1)
302-302: Good: use full CompareKey (time, counter, id) for latest gating.This fixes equal-time tie issues and is clearer than ad-hoc comparisons.
2 participants
Resolves #55
Summary by CodeRabbit
New Features
Refactor
Tests