Allow getting objects of type via IChangeContext #41
Conversation
WalkthroughA new asynchronous method for retrieving objects by type and JSON type name was introduced in the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Method
participant SetTagChange as SetTagChange
participant Context as IChangeContext
participant Tag as Tag Entity
Test->>SetTagChange: NewEntity(commit, context)
SetTagChange->>Context: GetObjectsOfType<Tag>("Tag")
Context-->>SetTagChange: Async enumerable of tags
SetTagChange->>SetTagChange: Check for existing tag with same text
SetTagChange->>Tag: Create new Tag (set DeletedAt if duplicate)
SetTagChange-->>Test: Return new Tag
Test->>SetTagChange: ApplyChange(entity, context)
SetTagChange->>Context: GetObjectsOfType<Tag>("Tag")
Context-->>SetTagChange: Async enumerable of tags
SetTagChange->>SetTagChange: Check for other tag with same text
SetTagChange->>Tag: Update DeletedAt if duplicate, update text
SetTagChange-->>Test: Complete
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/Changes/SetTagChange.cs (1)
11-20: Convert to async and handle duplicate tag namesThe method now correctly uses the new
GetObjectsOfType<T>method to check for duplicate tag names, and sets theDeletedAtproperty accordingly.Fix the extra space in the ternary operator:
- DeletedAt = tagExists ? commit.DateTime : null + DeletedAt = tagExists ? commit.DateTime : null
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/SIL.Harmony.Core/IChangeContext.cs(1 hunks)src/SIL.Harmony.Sample/Changes/SetTagChange.cs(1 hunks)src/SIL.Harmony.Tests/DataModelReferenceTests.cs(1 hunks)src/SIL.Harmony/Changes/ChangeContext.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/SIL.Harmony.Core/IChangeContext.cs (2)
src/SIL.Harmony.Tests/ObjectBaseTestingHelpers.cs (1)
T(5-8)src/SIL.Harmony/SyncHelper.cs (1)
T(80-86)
🔇 Additional comments (3)
src/SIL.Harmony.Core/IChangeContext.cs (1)
17-17: Excellent addition to the interface!The new
GetObjectsOfType<T>method follows the same pattern as the existingGetObjectsReferencingmethod. The type constraintwhere T : classis appropriate since we're dealing with entity objects, and the parameterjsonTypeNameallows for precise type filtering beyond what the generic parameter alone would provide.src/SIL.Harmony/Changes/ChangeContext.cs (1)
29-34: Well-implemented method!The implementation is clean and efficient:
- It uses
GetSnapshotsWherewith a predicate that correctly handles both the deletion status and type name filtering- It projects the results using
Selectto get theDbObjectproperty- It uses
OfType<T>()to ensure type safetyThis provides a solid foundation for enforcing unique constraints as mentioned in the PR objectives.
src/SIL.Harmony.Sample/Changes/SetTagChange.cs (1)
23-32: Consider the order of operations when handling duplicate tagsThe method correctly checks for duplicate tag names and marks the entity as deleted when needed. However, the text is still updated even if the entity is marked as deleted.
Is it intentional to update the
Textproperty after the entity is marked as deleted? Consider whether it makes more sense to update the text first and then check for duplicates, or to return early after marking the entity as deleted:public override async ValueTask ApplyChange(Tag entity, IChangeContext context) { if (entity.Text == Text) return; + entity.Text = Text; var tagExists = await context.GetObjectsOfType<Tag>(nameof(Tag)).AnyAsync(t => t.Id != EntityId && t.Text == Text); if (tagExists) { entity.DeletedAt = context.Commit.DateTime; } - entity.Text = Text; }or
public override async ValueTask ApplyChange(Tag entity, IChangeContext context) { if (entity.Text == Text) return; var tagExists = await context.GetObjectsOfType<Tag>(nameof(Tag)).AnyAsync(t => t.Id != EntityId && t.Text == Text); if (tagExists) { entity.DeletedAt = context.Commit.DateTime; + entity.Text = Text; + return; } entity.Text = Text; }
this enables changes to better enforce constraints when running to avoid conflicts with unique constraints
Summary by CodeRabbit