Update entry search records if morph type tokens change#2378
Conversation
github-actions
Bot
added
the
💻 FW Lite
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughEntry search regeneration now uses a static rebuild helper, and save-changes interception now routes MorphType token changes to a full table regeneration while keeping incremental updates for other entry changes. ChangesEntry search maintenance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/FwLite/LcmCrdt/FullTextSearch/EntrySearchService.cs (1)
284-301: 🗄️ Data Integrity & Integration | 🔴 CriticalMove regeneration to after commit and remove explicit transaction.
Invoking
RegenerateEntrySearchTablewithinSavingChangesAsyncguarantees stale data because the database reads forWritingSystemsOrderedandMorphTypes(marked.AsNoTracking()inLcmCrdtDbContext.cs) execute before the pending changes are committed. Consequently, the regeneration uses outdated token mappings. Additionally, callingBeginTransactionAsyncinside the SaveChanges pipeline conflicts with EF Core's internal transaction management, risking runtime failures.The logic must be moved to
SavedChangesAsyncor handled in a background worker that runs only after the transaction commits, ensuring all tables reflect the new state before reading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/FwLite/LcmCrdt/FullTextSearch/EntrySearchService.cs` around lines 284 - 301, `RegenerateEntrySearchTable` is running too early and should not open its own transaction inside the SaveChanges flow. Move the call out of `SavingChangesAsync` into `SavedChangesAsync` (or an equivalent post-commit background path) so `WritingSystemsOrdered` and `MorphTypes` are read after the commit and reflect the latest state. Also remove the explicit `BeginTransactionAsync`/`CommitAsync` handling from `RegenerateEntrySearchTable`, since EF Core already manages the save transaction and nested transaction usage can fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/FwLite/LcmCrdt/FullTextSearch/UpdateEntrySearchTableInterceptor.cs`:
- Around line 60-68: SavingChangesAsync in UpdateEntrySearchTableInterceptor is
triggering RegenerateEntrySearchTable(dbContext) before EF Core has committed
MorphType, entry, and writing-system changes, so the rebuild reads stale data
and can drop same-save updates. Move the full regeneration out of
SavingChangesAsync and defer it until after the commit (for example via
SavedChangesAsync or equivalent post-commit handling), preserving the
changedMorphTypes decision so EntrySearchService.RegenerateEntrySearchTable runs
only on committed database state and includes the latest newWritingSystems,
toUpdate, and toRemove values.
---
Outside diff comments:
In `@backend/FwLite/LcmCrdt/FullTextSearch/EntrySearchService.cs`:
- Around line 284-301: `RegenerateEntrySearchTable` is running too early and
should not open its own transaction inside the SaveChanges flow. Move the call
out of `SavingChangesAsync` into `SavedChangesAsync` (or an equivalent
post-commit background path) so `WritingSystemsOrdered` and `MorphTypes` are
read after the commit and reflect the latest state. Also remove the explicit
`BeginTransactionAsync`/`CommitAsync` handling from
`RegenerateEntrySearchTable`, since EF Core already manages the save transaction
and nested transaction usage can fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9376e548-8f2b-4223-81fa-2723d03fbaa8
📒 Files selected for processing (2)
backend/FwLite/LcmCrdt/FullTextSearch/EntrySearchService.csbackend/FwLite/LcmCrdt/FullTextSearch/UpdateEntrySearchTableInterceptor.cs
Also fixed two bugs: 1. SQLite doesn't allow nested transactions: RegenerateEntrySearchTable needed to check if we were already in a transaction before starting one. 2. By moving the entry table regeneration into a SavedChangesAsync event handler, we can ensure that the MorphType changes have already been applied to the database and will be read correctly.
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
I'm generally happy with this. 👍
But, I'm expecting my test suggestions to uncover one missing case 😉
|
CodeRabbit is suggesting adding |
@rmunn Yes, it's quite likely, but I think that should still be re-determined by the changeset. If it's necessary the next time, then your change checking code will turn the flag back on. I think resetting the flag in failure situations makes sense. |
Changing the MorphType of an entry successfully regenerates its entry search record.
|
New test case added in 6eedb36 to cover the first scenario @myieye metnioned ("An entry's morph-type is changed so that its morph-tokens are different"). Passes; it's covered by the interceptor because the interceptor is updating the headword for any change to an entry, not merely looking to see whether its lexeme or citation forms changed. So this is already working. Still, it's good to have it in there so that we don't accidentally break that functionality during an attempted optimization. Now going to work on the second test, updating many things at once. |
Regenerated search table entry successfully gets new morph type prefix.
I guess what I had in mind probably requires using the raw DbContext. That's maybe the only way to batch all 3 changes into a single I suppose that's not a particularly realistic scenario, but it would still be nice to be confident that we don't have any loop holes in our change detection. |
There was a problem hiding this comment.
One comment to fix.
Also, I'd personally still like one more test with multiple changes applied directly to db-context entities, so they actually get persisted at the same time.
Actually, batches of changes in a single dbcontext.SaveChanges() call are not entirely unrealistic, because batching stuff is one pretty interesting optimization idea for the whole CRDT side of a fw-headless sync.
Just to make sure everything still works when multiple changes are added to the DbContext all at once, let's add two more tests making those changes directly in the DbContext instead of via the API. That will be a pretty good simulation of syncing a bunch of changes, without needing to set up an entire sync environment.
Added two more tests in commit 88ffb3b doing that, one changing the entry first and then the morph type, and the other changing the morph type first and then the entry. Those should both have the same effect, and in fact I'd be shocked if one failed while the other one passed. But you never know what weirdness we might manage to cause by getting something wrong in a save-changes hook, so I figured best to be sure about it. The new tests pass when run locally. Once they also pass in CI, I'll merge the branch. |
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
Looks good! I'd like to see the order of operations change in the interceptor.
Both add or remove a postfix token. In theory we should also test a prefix token, but there's no real reason to do that because it's pretty much impossible that we would change prefix token handling yet forget to do the same thing to postfix tokens.
If morph-type prefix or postfix tokens are ever altered, then we would be skipping the entry changes, so we might as well skip looking through the list for writing system and entry changes anyway.
| if (maybeDbContext is not LcmCrdtDbContext dbContext) return; | ||
| // Morph types with changes to prefix or postfix tokens will require updated entry search records | ||
| // (Note that morph types can't be added or deleted, so we only need to catch changes, which will be rare) | ||
| var changedMorphTypes = dbContext.ChangeTracker.Entries<MorphType>() |
There was a problem hiding this comment.
do we actually need a list of morph types? or can we just .Where(...).Any() to avoid allocating a list just to see if it's empty or not.
There was a problem hiding this comment.
You know what, that's correct. I had allocated a list because I was going through and finding entries that referred to those morph types in order to only change those entries. But when it (surprisingly) turned out to be faster to simply delete and recreate the search table, I didn't need the list anymore.
I'll change that to a simple .Any() query and then merge this.
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
one small suggestion, otherwise I'm happy with it
3 participants
Fixes #2213.
I chose to hook into the existing interceptor rather than creating a new one. That way if entries and morph types change in the same "transaction" (or the EF equivalent of a transaction), we can avoid duplicating the search record updates for entries that changed (and we can use the changed entries, rather than their original values, when calculating the updated headwords with the new morph types — if we used a separate interceptor then we wouldn't be able to guarantee that these two steps ran in the desired order).
P.S. This is #2246 recreated on top of current
develop.