Skip to content

Conversation

@hahn-kev
Copy link
Collaborator

@hahn-kev hahn-kev commented Apr 23, 2025

related to sillsdev/languageforge-lexbox#1615

WordTags are linking objects between a word and a tag. There is a constraint that there can only be one row with the same tag and word id. If a sync comes in that creates a word tag which already exists, but the commit precedes the local word tag we run into this constraint violation.

Summary by CodeRabbit

  • Bug Fixes

    • Improved duplicate prevention to ensure that duplicate tag relationships are not created, even if a duplicate entry is inserted with an earlier version.
  • Tests

    • Added a new test to verify that the system prevents duplicate tag relationships when a duplicate is created before the current version.
  • Refactor

    • Updated internal class structures and constructors for better maintainability and serialization support.
    • Enhanced snapshot processing logic to handle deleted entities more robustly.

@hahn-kev hahn-kev marked this pull request as draft April 23, 2025 09:35
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Apr 23, 2025

Walkthrough

This update refactors the TagWordChange class to use explicit constructors, including one marked with [JsonConstructor] for deserialization, and moves property initialization into these constructors. A new unit test is introduced to verify correct handling of duplicate tag-word relationships, especially when duplicates are inserted retroactively. Additionally, the snapshot processing logic in the repository is revised to group and process snapshots by deletion status, ensuring deleted entities are handled first and saving changes after each group. The early-exit condition in the ProjectSnapshot method for deleted root snapshots is removed.

Changes

File(s) Change Summary
src/SIL.Harmony.Sample/Changes/TagWordChange.cs Refactored TagWordChange to use explicit constructors, added [JsonConstructor] overload, moved property initialization into constructors, and added necessary namespace import.
src/SIL.Harmony.Tests/SnapshotTests.cs Added a new test method to verify duplicate prevention when a duplicate is created before the current version.
src/SIL.Harmony/Db/CrdtRepository.cs Modified snapshot processing to group by deletion status and process deleted entities first; moved save and error handling inside the group loop; removed early return for deleted root snapshots in ProjectSnapshot.

Sequence Diagram(s)

sequenceDiagram
    participant Test as SnapshotTests
    participant Repo as CrdtRepository
    participant Db as DbContext

    Test->>Repo: AddSnapshots(snapshots)
    loop For each group by deletion status (deleted first)
        Repo->>Db: Add snapshots in group
        Repo->>Repo: ProjectSnapshot for new EntityIds
        Repo->>Db: SaveChangesAsync()
        alt Exception occurs
            Repo->>Repo: Handle exception
        end
    end
Loading

Possibly related PRs

Poem

In the code garden where snapshots bloom,
Constructors now have extra room.
Duplicates chased from every nook,
With tests to ensure a tidy look.
Snapshots grouped, deletions first—
Harmony’s logic, well-rehearsed!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3145188 and c45f3bb.

📒 Files selected for processing (3)
  • src/SIL.Harmony.Sample/Changes/TagWordChange.cs (1 hunks)
  • src/SIL.Harmony.Tests/SnapshotTests.cs (1 hunks)
  • src/SIL.Harmony/Db/CrdtRepository.cs (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/SIL.Harmony.Sample/Changes/TagWordChange.cs (1)
src/SIL.Harmony/Changes/CreateChange.cs (1)
  • CreateChange (3-9)
🔇 Additional comments (5)
src/SIL.Harmony.Tests/SnapshotTests.cs (1)

125-142: Good test coverage for duplicate prevention.

This test effectively validates that the system correctly handles duplicate tag-word relationships even when they're inserted retroactively with an earlier timestamp. It directly addresses the constraint violation issue mentioned in the PR objectives, ensuring that duplicates are properly prevented regardless of insertion order.

src/SIL.Harmony/Db/CrdtRepository.cs (2)

259-281: Well-structured ordering for snapshot processing.

The modified code now groups snapshots by deletion status and processes deleted entities first, which effectively prevents constraint violations during synchronization. Saving changes after each group is also a good approach to isolate potential errors.

One minor suggestion:

-foreach (var grouping in snapshots.GroupBy(s => s.EntityIsDeleted).OrderByDescending(g => g.Key))//execute deletes first
+foreach (var grouping in snapshots.GroupBy(s => s.EntityIsDeleted).OrderByDescending(g => g.Key)) // Process deletes first to prevent constraint violations during sync

287-287: Improved handling of root deleted snapshots.

Removing the early-exit condition for deleted root snapshots ensures proper processing of all snapshot types, supporting the enhanced duplicate prevention mechanism.

src/SIL.Harmony.Sample/Changes/TagWordChange.cs (2)

1-2: Added JsonSerializer support.

Adding the System.Text.Json.Serialization namespace enables proper JSON serialization/deserialization, which is essential for handling sync operations.


8-24: Improved class design with explicit constructors.

Refactoring from a primary constructor to explicit constructors with a dedicated [JsonConstructor] improves serialization support while maintaining the same functionality. The readonly properties ensure immutability after initialization, which is a good practice for change objects.

This change directly addresses the PR objective of fixing constraint violations during sync by ensuring proper deserialization of TagWordChange objects, particularly when handling retroactive duplicate entries.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Group and process snapshots by deletion status to ensure deletes are prioritized during entity synchronization.
@hahn-kev hahn-kev marked this pull request as ready for review April 24, 2025 03:33
@hahn-kev hahn-kev requested a review from myieye April 24, 2025 03:33
Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@hahn-kev hahn-kev merged commit 4af7d83 into main Apr 26, 2025
5 checks passed
@hahn-kev hahn-kev deleted the constraint-order-dependent-changes branch April 26, 2025 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants