fix(meta): resolve MetaModelData ModelId to newest IMD tip - #243
Conversation
- Add ResolveMetaModelTip (created_at DESC, id DESC) and use it in loader model resolves. - Rebind MetaModelData.ModelId onto surviving tips when uninstall soft-deletes IMD MetaModel rows. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change makes model selection deterministic by choosing the newest matching record. ChangesMeta model resolution and cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- Drop the ResolveMetaModelTip helper and resolve IMD tips inline with Order(created_at DESC, id DESC). - Project MetaModelData.ModelId as an @SqlCompute tip over live meta_model rows instead of the stored FK. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/improve |
|
@cursor review |
PR Code Suggestions ✨Latest suggestions up to b215ae4
Previous suggestionsSuggestions up to commit 5d045ff
|
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b215ae4. Configure here.
- Exercise rebindMetaModelDataTips edge branches and cleanModels error returns for IMD tip rebinding. - Cover applyRecord model-resolve failure and MetaModelData.sqlModelId tip projection. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/module/lifecycle/uninstaller_model_data_test.go (1)
332-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLeaked failure-injection triggers in
internal/module/lifecycle/uninstaller_model_data_test.go. Both new tests create a SQLiteBEFORE UPDATE OF model_idtrigger onmeta_model_datato force a rebind failure, and neither drops it. The shared root cause is a missing cleanup step for the fixture trigger. The leak couples subtest order insideTestRebindMetaModelDataTipsBranches, and it breaks later tests ifnewLifecycleCommitTestScopereturns a shared database.
internal/module/lifecycle/uninstaller_model_data_test.go#L332-L346: addt.Cleanupthat runsDROP TRIGGER IF EXISTS block_meta_model_data_rebindafter theCREATE TRIGGERsucceeds.internal/module/lifecycle/uninstaller_model_data_test.go#L396-L415: addt.Cleanupthat runsDROP TRIGGER IF EXISTS block_meta_model_data_rebind_cleanafter theCREATE TRIGGERsucceeds.🤖 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 `@internal/module/lifecycle/uninstaller_model_data_test.go` around lines 332 - 346, The failure-injection triggers created in TestRebindMetaModelDataTipsBranches are not cleaned up. At internal/module/lifecycle/uninstaller_model_data_test.go lines 332-346, register t.Cleanup after creating block_meta_model_data_rebind to drop it with DROP TRIGGER IF EXISTS; do the same at lines 396-415 for block_meta_model_data_rebind_clean.modules/meta/service/tests/model_data_ref.test.ts (1)
97-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the SQL fragment semantics.
Replace the truthiness check with an assertion on
out.toOperationNode().sqlFragments.join('')that coversm.deleted_at is null,order by m.created_at desc, m.id desc, andlimit 1. Keep thecolCallsassertion for the correlated columns.🤖 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 `@modules/meta/service/tests/model_data_ref.test.ts` around lines 97 - 102, Update the test around proto.sqlModelId to assert the generated SQL semantics via out.toOperationNode().sqlFragments.join('') instead of only checking truthiness; verify it contains m.deleted_at is null, order by m.created_at desc, m.id desc, and limit 1, while preserving the existing colCalls assertion.
🤖 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.
Nitpick comments:
In `@internal/module/lifecycle/uninstaller_model_data_test.go`:
- Around line 332-346: The failure-injection triggers created in
TestRebindMetaModelDataTipsBranches are not cleaned up. At
internal/module/lifecycle/uninstaller_model_data_test.go lines 332-346, register
t.Cleanup after creating block_meta_model_data_rebind to drop it with DROP
TRIGGER IF EXISTS; do the same at lines 396-415 for
block_meta_model_data_rebind_clean.
In `@modules/meta/service/tests/model_data_ref.test.ts`:
- Around line 97-102: Update the test around proto.sqlModelId to assert the
generated SQL semantics via out.toOperationNode().sqlFragments.join('') instead
of only checking truthiness; verify it contains m.deleted_at is null, order by
m.created_at desc, m.id desc, and limit 1, while preserving the existing
colCalls assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32503b81-78fc-4643-b507-7f49bac9ff24
📒 Files selected for processing (3)
internal/module/evolution/data/loader_test.gointernal/module/lifecycle/uninstaller_model_data_test.gomodules/meta/service/tests/model_data_ref.test.ts
- Drop failure-injection SQLite triggers via t.Cleanup after rebind error cases. - Assert MetaModelData.sqlModelId tip SQL fragments instead of a truthiness check. Co-authored-by: Cursor <cursoragent@cursor.com>
User description
Summary
meta_modelrows with the same(application, name).meta.ResolveMetaModelTip(created_at DESC, id DESC) and route loader resolves through it (includingmodelRef/ search / seed apply).meta_model_data.model_idvalues that pointed at victims onto the surviving tip (when one remains).Test plan
go test ./pkg/meta/ -run ResolveMetaModelTipgo test ./internal/module/lifecycle/ -run 'CleanModels|MetaModelData|Rebind'go test ./internal/module/evolution/data/go test ./internal/module/lifecycle/ ./pkg/meta/ ./internal/module/metadata/Made with Cursor
PR Type
Bug fix, Enhancement, Tests
Description
Add
meta.ResolveMetaModelTipto pick the newestmeta_modeltip bycreated_at DESC, id DESC.Update Go core seed data loader and model resolutions to use model tips for extended models.
Rebind
MetaModelData.ModelIdrecords to surviving model tips when uninstalling extension modules.Add Go unit tests for tip resolution and uninstall rebinding; new files include required SPDX headers.
File Walkthrough
model_tip.go
Add ResolveMetaModelTip helper functionpkg/meta/model_tip.go
ResolveMetaModelTipto fetch the latestmeta_modelrowordered by
created_at DESC, id DESCexcludeIDsoption to filter out models being soft-deletedmodel_tip_test.go
Add unit tests for ResolveMetaModelTippkg/meta/model_tip_test.go
created_attimestamp andidtie-breaking
uninstaller_model_data_test.go
Add unit test for model tip rebinding during module uninstallationinternal/module/lifecycle/uninstaller_model_data_test.go
TestModuleUninstallerCleanModelsRebindsMetaModelDataTipto testMetaModelDatarebinding when extension modules are removedloader.go
Update data loader to resolve model references via ResolveMetaModelTipinternal/module/evolution/data/loader.go
applyRecord,resolveSearchModel,detectFieldCardinality, andresolveModelRefto useResolveMetaModelTiptranslated_seed.go
Use ResolveMetaModelTip in language code checkinternal/module/evolution/data/translated_seed.go
languageCodeExiststo lookup base Language model usingResolveMetaModelTipuninstaller.go
Rebind MetaModelData model references on module uninstallationinternal/module/lifecycle/uninstaller.go
uninstallation
rebindMetaModelDataTipsto rebind orphanedMetaModelData.ModelIdvalues to remaining model tipsSummary by CodeRabbit