feat(meta): add meta_raw_* schema and E2 effective merge - #244
Conversation
- Introduce declaration-layer Raw* entities and register them in meta.Entities for AutoMigrate. - Extract MergeSameNameModelsByExtensionChain into pkg/meta and wire codegen to the shared E2 implementation. - Add EnsureDualStoreTables / MigrateIMDCatalogToDualStore helpers for wipe/test backfill with stable tip ids. 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: 38 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)
📝 WalkthroughWalkthroughAdds raw metadata entities, extension-chain effective-model merging, and IMD-to-dual-store migration. The migration copies declaration trees, recomputes effective projections, clears legacy ChangesDual-store metadata catalog
Estimated code review effort: 5 (Critical) | ~90 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:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
pkg/meta/effective_merge.go (1)
189-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated field mapping risks silent column drops.
rawFieldAsFieldhere andrawFieldFromFieldinpkg/meta/dual_store_migrate.go(lines 254-296) map the same ~35Fieldcolumns in opposite directions.rawServiceAsServiceandrawServiceFromServiceduplicate theServicecolumns the same way. When a column is added toFieldorRawField, both mappers must be updated, and an omission fails silently: the column is dropped from the raw copy or from the effective projection with no error.Two options reduce the hazard:
- Extract the shared declaration columns into an embedded struct that both
FieldandRawFieldembed, so the mapping becomes a single assignment.- Add a reflection-based test that asserts the exported field sets of
FieldandRawFieldmatch, so a new column fails the build until both mappers are updated.🤖 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 `@pkg/meta/effective_merge.go` around lines 189 - 230, Reduce drift between rawFieldAsField and rawFieldFromField, and likewise rawServiceAsService and rawServiceFromService, by adding reflection-based tests that compare the exported field sets of each raw/effective type pair. Ensure the tests fail when a field exists on only one side, forcing both conversion mappers to be updated for new columns.pkg/meta/dual_store_migrate_test.go (1)
123-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a soft-delete case to the uniqueness test.
This case inserts a second live row and asserts a unique violation. It does not cover the soft-deleted row, which is where the index name
uidx_meta_model_app_name_aliveand the doc comment inpkg/meta/meta_model.golines 11-14 diverge from the DDL. Add a case that soft-deletes the effectivePartnerrow and then inserts a new live row with the same(application, name). Assert the intended behavior. With the current non-partial index that insert fails, which pins the semantics either way. The root cause is described onpkg/meta/dual_store_migrate.golines 468-478.🤖 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 `@pkg/meta/dual_store_migrate_test.go` around lines 123 - 132, Extend the uniqueness coverage in the existing test around the duplicate Partner insertion by soft-deleting the effective live Partner row, then attempting to create another live row with the same (application, name). Assert the intended post-soft-delete behavior, preserving the existing duplicate-live-row assertion and using the current index semantics to pin the result.pkg/meta/effective_merge_test.go (1)
13-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for service merging.
The tests cover field union, raw conversion, cycles,
selectionAddrejection, and tie-breaking. No test covers theServicesbranch atpkg/meta/effective_merge.golines 120-130, which uses last-write-wins semantics that differ from the field path. Add a case where a base row and an extension row declare a service with the sameName, and assert that the extension row's service replaces the base one and that distinct service names are unioned.🤖 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 `@pkg/meta/effective_merge_test.go` around lines 13 - 143, Add a test alongside TestMergeSameNameModelsByExtensionChain_PartnerStyleUnion that creates base and extension models with same-name services plus distinct service names, then calls MergeSameNameModelsByExtensionChain. Assert the extension service replaces the base service with the same Name while services unique to either model remain present, covering the Services merge branch’s last-write-wins behavior.pkg/meta/dual_store_migrate.go (2)
54-77: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffLoad and insert in batches.
Line 55-66 loads every
meta_modelrow with six nested preloads into one slice.copyModelTreeToRawthen issues oneCreateper model, field, service, parameter, type parameter, decorator, and argument. For a catalog with thousands of models the peak memory holds the entire metadata tree, and the write count is one round-trip per row.Page the source query with
FindInBatches, and group the child writes withCreateInBatches. The same applies toRecomputeAllEffectiveFromRawat lines 89-96 and topersistEffectiveProjection.🤖 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 `@pkg/meta/dual_store_migrate.go` around lines 54 - 77, Update the migration flow around the source query and copyModelTreeToRaw to process models with FindInBatches instead of loading the entire preloaded tree, and batch each corresponding child insertion with CreateInBatches. Apply the same batching approach to RecomputeAllEffectiveFromRaw and persistEffectiveProjection, preserving existing preload relationships and migration behavior while avoiding one write round-trip per row.
98-110: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTrack the tip row by pointer, not by id lookup.
findRawByID(groups[key], prev)rescans the whole group on every row, so tip selection is O(n²) per(application, name)group. The lookup exists only to recover a*RawModelthat the loop already held.Two further details:
- A raw row with an empty
Id.StringsetstipIDs[key]to"". The next iteration readsprev == ""and replaces the tip unconditionally, bypassing therawIsNewerTipcomparison.- Tip selection orders by
CreatedAtthenId, whileMergeSameNameModelsByExtensionChainpicks canonical scalars by extends-depth, thenUpdatedAt, thenId. The effective row can therefore reuse the id of one raw row while carrying the scalars of another. The doc comment at line 37 states this is intended; a short note here would record why the two orderings differ.Holding the tip pointer removes both the quadratic scan and the empty-id edge case, and lets
findRawByIDbe deleted.♻️ Proposed refactor
groups := map[string][]*RawModel{} - tipIDs := map[string]string{} + tips := map[string]*RawModel{} for _, raw := range raws { if raw == nil { continue } key := logicalModelKey(raw.Application, raw.Name) groups[key] = append(groups[key], raw) - prev := tipIDs[key] - if prev == "" || rawIsNewerTip(raw, findRawByID(groups[key], prev)) { - tipIDs[key] = raw.Id.String + if rawIsNewerTip(raw, tips[key]) { + tips[key] = raw } }Then read the id at the projection site:
var effID string if tip := tips[key]; tip != nil && tip.Id.Valid { effID = tip.Id.String } if effID == "" { effID = xid.New().String() }🤖 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 `@pkg/meta/dual_store_migrate.go` around lines 98 - 110, Update tip selection in the migration flow to track the current *RawModel in a tips map keyed by logical model key, comparing each row directly with the stored pointer via rawIsNewerTip; do not use raw Id.String as the sentinel or call findRawByID. At the projection site, derive effID from the selected tip only when tip.Id is valid, otherwise generate a new xid, and add a brief note explaining that tip ordering intentionally differs from canonical scalar selection in MergeSameNameModelsByExtensionChain; remove findRawByID once unused.
🤖 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 `@pkg/meta/dual_store_migrate.go`:
- Around line 112-115: Update the comments near clearEffectiveShapeTrees and the
ordering note around the effective-table cleanup to state that the effective
tables are truncated, not soft-deleted. Extend the RecomputeAllEffectiveFromRaw
doc comment to warn that it destroys the entire effective catalog and rebuilds
only (application, name) groups present in meta_raw_model, so rows absent from
the raw store cannot be recovered.
- Around line 38-80: Make MigrateIMDCatalogToDualStore run all validation,
copying, and recomputation work inside a single db.Transaction, but invoke
EnsureDualStoreTables and its AutoMigrate DDL before opening that transaction.
Also update RecomputeAllEffectiveFromRaw to execute its clear-and-rebuild
workflow transactionally, while moving ensureEffectiveAppNameUniqueIndex outside
the transaction so DDL is not performed within it.
- Around line 166-184: Update the migration flow around copyModelTreeToRaw to
explicitly copy only live rows or use unscoped queries and preserve DeletedAt
consistently. Before copying, preflight duplicate (Path, ModuleId) keys,
counting existing raw rows with Unscoped so soft-deleted rows reserve unique
keys. Wrap the duplicate validation, raw-row copying, and recompute steps in one
database transaction so any failure rolls back all changes.
- Around line 468-478: Update ensureEffectiveAppNameUniqueIndex in
pkg/meta/dual_store_migrate.go:468-478 to drop/recreate
uidx_meta_model_app_name_alive as a partial unique index with WHERE deleted_at
IS NULL, and rename sql to stmt. Keep the index definition consistent in
pkg/meta/meta_model.go:10-14 and modules/meta/service/models/model.ts:39-43.
Extend pkg/meta/dual_store_migrate_test.go:123-132 to verify a soft-deleted row
permits reuse while duplicate live (application, name) rows remain rejected.
- Around line 401-429: Update RecomputeAllEffectiveFromRaw and
persistEffectiveProjection to load and project raw project, field, and service
decorators plus arguments into the effective records before
clearEffectiveShapeTrees removes existing rows. Clone these associations with
the new effective IDs and parent references, preserving the metadata expected by
backend builds rather than nil-ing or omitting them.
In `@pkg/meta/effective_merge.go`:
- Around line 19-24: Remove the len(models)==0/1 early-return fast paths in the
effective merge entry point and route every non-empty input through the existing
merge loop. Preserve the nil result for empty input while ensuring single-model
merges perform FieldHasSelectionAdd validation and return a copy rather than the
caller’s *Model, matching multi-model behavior.
In `@pkg/meta/meta_raw_model.go`:
- Around line 15-34: Update RawModel’s ModuleId/path uniqueness around the model
definition and copyModelTreeToRaw flow so moduleless rows cannot share the same
Path despite nullable ModuleId values. Choose an appropriate fix by requiring a
module owner, migrating legacy null ModuleId rows, or enforcing null-safe
uniqueness, and add coverage proving duplicate moduleless RawModel paths are
rejected.
---
Nitpick comments:
In `@pkg/meta/dual_store_migrate_test.go`:
- Around line 123-132: Extend the uniqueness coverage in the existing test
around the duplicate Partner insertion by soft-deleting the effective live
Partner row, then attempting to create another live row with the same
(application, name). Assert the intended post-soft-delete behavior, preserving
the existing duplicate-live-row assertion and using the current index semantics
to pin the result.
In `@pkg/meta/dual_store_migrate.go`:
- Around line 54-77: Update the migration flow around the source query and
copyModelTreeToRaw to process models with FindInBatches instead of loading the
entire preloaded tree, and batch each corresponding child insertion with
CreateInBatches. Apply the same batching approach to
RecomputeAllEffectiveFromRaw and persistEffectiveProjection, preserving existing
preload relationships and migration behavior while avoiding one write round-trip
per row.
- Around line 98-110: Update tip selection in the migration flow to track the
current *RawModel in a tips map keyed by logical model key, comparing each row
directly with the stored pointer via rawIsNewerTip; do not use raw Id.String as
the sentinel or call findRawByID. At the projection site, derive effID from the
selected tip only when tip.Id is valid, otherwise generate a new xid, and add a
brief note explaining that tip ordering intentionally differs from canonical
scalar selection in MergeSameNameModelsByExtensionChain; remove findRawByID once
unused.
In `@pkg/meta/effective_merge_test.go`:
- Around line 13-143: Add a test alongside
TestMergeSameNameModelsByExtensionChain_PartnerStyleUnion that creates base and
extension models with same-name services plus distinct service names, then calls
MergeSameNameModelsByExtensionChain. Assert the extension service replaces the
base service with the same Name while services unique to either model remain
present, covering the Services merge branch’s last-write-wins behavior.
In `@pkg/meta/effective_merge.go`:
- Around line 189-230: Reduce drift between rawFieldAsField and
rawFieldFromField, and likewise rawServiceAsService and rawServiceFromService,
by adding reflection-based tests that compare the exported field sets of each
raw/effective type pair. Ensure the tests fail when a field exists on only one
side, forcing both conversion mappers to be updated for new columns.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31a0d51f-8bf9-4fc0-b4a2-177a5c38d39e
📒 Files selected for processing (16)
internal/module/artifact/generate/generator.gomodules/meta/service/models/model.tspkg/meta/dual_store_migrate.gopkg/meta/dual_store_migrate_test.gopkg/meta/effective_merge.gopkg/meta/effective_merge_test.gopkg/meta/meta_model.gopkg/meta/meta_raw_argument.gopkg/meta/meta_raw_decorator.gopkg/meta/meta_raw_field.gopkg/meta/meta_raw_model.gopkg/meta/meta_raw_parameter.gopkg/meta/meta_raw_service.gopkg/meta/meta_raw_typeparameter.gopkg/meta/model.gopkg/meta/model_test.go
- Wrap IMD→dual-store migrate/recompute in a DB transaction and document hard-delete rebuild semantics. - Preload and project decorator/argument trees into effective rows; require ModuleId and preflight (module,path) duplicates. - Use a partial unique index on live (application, name); unify single-model merge validation/copy with the multi-row path. Co-authored-by: Cursor <cursoragent@cursor.com>
Review triage (applied in follow-up commit)
|
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)
pkg/meta/dual_store_migrate.go (1)
556-579: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake
ensureEffectiveAppNameUniqueIndexidempotent for MySQL. MySQL rejectsDROP INDEX IF EXISTSand requiresON meta_model. It does not supportCREATE UNIQUE INDEX IF NOT EXISTS. Since line 558 discards the drop error, line 571 fails with a duplicate key name on every laterRecomputeAllEffectiveFromRawcall. UseDROP INDEX <name> ON meta_modeland ignore only the missing-index error, or queryinformation_schema.STATISTICSfirst.🤖 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 `@pkg/meta/dual_store_migrate.go` around lines 556 - 579, Update ensureEffectiveAppNameUniqueIndex for MySQL so index removal uses the required DROP INDEX <name> ON meta_model syntax and treats only a missing-index result as ignorable; preserve other drop errors and return them. Keep the existing partial-index handling for SQLite/Postgres and ensure repeated RecomputeAllEffectiveFromRaw calls do not fail with duplicate index-name errors.
🧹 Nitpick comments (1)
pkg/meta/dual_store_migrate_test.go (1)
161-166: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the decorator argument payload.
This test only checks the number of decorator arguments. A migration that changes
Argument.TypeorArgument.Valuewill still pass. Assert the seeded values to protect the declaration payload.Proposed test assertion
if len(nameField.Decorators[0].Arguments) != 1 { t.Fatalf("expected decorator argument preserved, got %#v", nameField.Decorators[0].Arguments) } +arg := nameField.Decorators[0].Arguments[0] +if arg == nil || arg.Type != "object" || arg.Value != `{"type":"varchar"}` { + t.Fatalf("expected decorator argument payload preserved, got %#v", arg) +}🤖 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 `@pkg/meta/dual_store_migrate_test.go` around lines 161 - 166, Strengthen the assertions in the migration test around nameField.Decorators[0].Arguments by validating the seeded Argument.Type and Argument.Value, not only the argument count. Preserve the existing decorator and argument-presence checks while failing with the actual argument details when either payload field changes.
🤖 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 `@pkg/meta/dual_store_migrate.go`:
- Around line 169-171: The call to ensureEffectiveAppNameUniqueIndex at line 169
inside recomputeAllEffectiveFromRawTx violates the documented contract (lines
40-41) that DDL operations run outside transactions, since DDL causes implicit
commits on MySQL and breaks transaction atomicity. Remove the
ensureEffectiveAppNameUniqueIndex call from inside
recomputeAllEffectiveFromRawTx and instead invoke
ensureEffectiveAppNameUniqueIndex from the exported wrapper functions that call
recomputeAllEffectiveFromRawTx, placing it after the transaction completes.
---
Outside diff comments:
In `@pkg/meta/dual_store_migrate.go`:
- Around line 556-579: Update ensureEffectiveAppNameUniqueIndex for MySQL so
index removal uses the required DROP INDEX <name> ON meta_model syntax and
treats only a missing-index result as ignorable; preserve other drop errors and
return them. Keep the existing partial-index handling for SQLite/Postgres and
ensure repeated RecomputeAllEffectiveFromRaw calls do not fail with duplicate
index-name errors.
---
Nitpick comments:
In `@pkg/meta/dual_store_migrate_test.go`:
- Around line 161-166: Strengthen the assertions in the migration test around
nameField.Decorators[0].Arguments by validating the seeded Argument.Type and
Argument.Value, not only the argument count. Preserve the existing decorator and
argument-presence checks while failing with the actual argument details when
either payload field changes.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c00c45a-4fe5-4b42-b56a-481e103a4a43
📒 Files selected for processing (5)
internal/module/artifact/generate/generator_test.gopkg/meta/dual_store_migrate.gopkg/meta/dual_store_migrate_test.gopkg/meta/effective_merge.gopkg/meta/effective_merge_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/meta/effective_merge.go
- Run ensureEffectiveAppNameUniqueIndex after migrate/recompute commit so MySQL DDL cannot implicitly commit mid-TX. - Make MySQL unique-index ensure idempotent via HasIndex/DropIndex before CREATE. - Assert migrated decorator argument Type/Value in the dual-store migrate test. Co-authored-by: Cursor <cursoragent@cursor.com>
Review triage (review)
|
|
@cursor review |
- Skip recompute when migrate finds no live IMD sources so soft-deleted effective rows are not hard-wiped. - Omit synthetic service parameter this from effective merge/persist, matching codegen readers. Co-authored-by: Cursor <cursoragent@cursor.com>
Review triage (Bugbot review)
|
- Cover meta_raw_field Set/GetResolvedSpec including injectable marshal failures. - Expand E2 merge unit cases for nil/empty/service/conflict/tie-break paths. - Add dual-store migrate/recompute coverage for happy trees, dialect index DDL, and error hooks. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov follow-upBrought these files to 100% statement coverage (
|
|
@cursor review |
Codecov follow-upBrought these files to 100% statement coverage (
|
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 9a506bd. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/meta/dual_store_migrate_test.go (1)
303-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert that the raw side keeps the
thisparameter.The test proves the effective projection omits
this. It does not prove the raw declaration still holds it. That pair is the dual-store invariant: raw preserves the declaration, effective omits the synthetic parameter. A regression that droppedthisduring the copy intometa_raw_parameterwould still pass this test.♻️ Suggested addition
params := eff[0].Services[0].Parameters if len(params) != 1 || params[0].Name != "vals" { t.Fatalf("expected only vals on effective service, got %#v", params) } + var rawParams []*RawParameter + if err := db.Where("service_id = ?", svc.Id).Find(&rawParams).Error; err != nil { + t.Fatalf("load raw params: %v", err) + } + if len(rawParams) != 2 { + t.Fatalf("expected raw service to retain this and vals, got %#v", rawParams) + } }🤖 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 `@pkg/meta/dual_store_migrate_test.go` around lines 303 - 317, Extend the test after the effective projection assertions to load the raw service and its parameters, then assert that the raw declaration retains the synthetic “this” parameter alongside “vals”. Keep the existing effective-side assertion unchanged so the test verifies both sides of the dual-store invariant.pkg/meta/dual_store_migrate_coverage_test.go (4)
430-573: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsolidate the six near-identical failure cases.
Each sub-case repeats the same four steps: open a database, ensure tables, drop one raw table, and call
copyModelTreeToRaw. A table-driven loop over(name, dropTable, srcModel)removes most of this boilerplate and makes a missing case obvious.Two related points:
- Line 437 contains a leftover working note. Remove it or replace it with a statement of intent.
- This function name is nearly identical to
TestCopyModelTreeToRaw_FieldAndServiceDecoratorFailuresat Line 893. Rename one of them, for example toTestCopyModelTreeToRaw_ChildTableFailuresandTestCopyModelTreeToRaw_NestedDecoratorFailures.🤖 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 `@pkg/meta/dual_store_migrate_coverage_test.go` around lines 430 - 573, Refactor TestCopyModelTreeToRaw_FieldServiceDecoratorFailures into a table-driven loop covering the six raw-table failure cases, with each case defining its name, table-drop operation, and source Model while preserving the expected copy error assertions. Remove the leftover working note near the first case or replace it with a concise intent comment, and rename this test and the similarly named test near the later decorator cases to distinct names such as TestCopyModelTreeToRaw_ChildTableFailures and TestCopyModelTreeToRaw_NestedDecoratorFailures.
671-679: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo
persistDecoratorTreecases accept any outcome. Both sites callpersistDecoratorTreeand then tolerate success or failure, logging instead of asserting. Each block raises statement coverage without pinning behavior, so a regression in argument persistence passes. The outcome is deterministic in both cases, because the test controls whethermeta_argumentexists and SQLite foreign keys are off in this helper.
pkg/meta/dual_store_migrate_coverage_test.go#L671-L679:Argumentwas dropped at Line 670, so replace the emptyifbody and theelse if/t.Logfchain witht.Fatalonerr == nil. The emptyifbody also triggers staticcheck SA9003.pkg/meta/dual_store_migrate_coverage_test.go#L966-L973:meta_argumentstill exists at this point, so asserterr == nil, or remove the call and keep only the deterministic drop-table case at Lines 974-979.🤖 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 `@pkg/meta/dual_store_migrate_coverage_test.go` around lines 671 - 679, Make both persistDecoratorTree test sites assert deterministic outcomes instead of accepting and logging either result. At pkg/meta/dual_store_migrate_coverage_test.go lines 671-679, replace the empty if/else chain with t.Fatal when err is nil, since the dropped Argument requires failure. At pkg/meta/dual_store_migrate_coverage_test.go lines 966-973, assert err is nil while meta_argument still exists, or remove that call and retain the deterministic drop-table case.
686-711: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote the fidelity limit of name-only dialect faking.
namedDialectoroverridesName()only. The statements still run on SQLite. So these cases prove branch selection, not that the Postgres and MySQL DDL is valid on those engines. If the MySQL branch emits engine-specific DDL, add a dialect-tagged integration test, or assert the generated SQL text instead of only the returned error.Mutating
db.Dialectoron an open*gorm.DBalso persists for the rest of the test. That is contained here because each case reopens a database, but a comment stating the intent would help future edits.🤖 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 `@pkg/meta/dual_store_migrate_coverage_test.go` around lines 686 - 711, Clarify TestEnsureEffectiveAppNameUniqueIndex_Dialects that namedDialector only exercises dialect branch selection while SQL still executes against SQLite, and document that each database is reopened to isolate persistent db.Dialector mutation. To validate engine-specific MySQL or PostgreSQL DDL, add dialect-tagged integration coverage or assert generated SQL rather than relying only on returned errors.
202-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the copied field tree.
The query preloads
Fields.Decoratorsbut the test does not assert oneff[0].Fields. Add an assertion for the migratedNamefield and itsFielddecorator to make the preload meaningful.🤖 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 `@pkg/meta/dual_store_migrate_coverage_test.go` around lines 202 - 224, Extend the assertions after loading eff in the migration test to validate the copied field tree: assert the expected migrated Name field exists and that it has the expected Field decorator. Use eff[0].Fields and its decorators, alongside the existing model and service assertions.
🤖 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 `@pkg/meta/dual_store_migrate.go`:
- Around line 595-601: The index migration block in the dual-store migration
currently ignores the error from dropping effectiveAppNameUniqueIndex. Capture
and return the db.Exec DROP INDEX error immediately, before constructing or
executing the CREATE UNIQUE INDEX statement, so the replacement is not attempted
when removal fails.
---
Nitpick comments:
In `@pkg/meta/dual_store_migrate_coverage_test.go`:
- Around line 430-573: Refactor
TestCopyModelTreeToRaw_FieldServiceDecoratorFailures into a table-driven loop
covering the six raw-table failure cases, with each case defining its name,
table-drop operation, and source Model while preserving the expected copy error
assertions. Remove the leftover working note near the first case or replace it
with a concise intent comment, and rename this test and the similarly named test
near the later decorator cases to distinct names such as
TestCopyModelTreeToRaw_ChildTableFailures and
TestCopyModelTreeToRaw_NestedDecoratorFailures.
- Around line 671-679: Make both persistDecoratorTree test sites assert
deterministic outcomes instead of accepting and logging either result. At
pkg/meta/dual_store_migrate_coverage_test.go lines 671-679, replace the empty
if/else chain with t.Fatal when err is nil, since the dropped Argument requires
failure. At pkg/meta/dual_store_migrate_coverage_test.go lines 966-973, assert
err is nil while meta_argument still exists, or remove that call and retain the
deterministic drop-table case.
- Around line 686-711: Clarify TestEnsureEffectiveAppNameUniqueIndex_Dialects
that namedDialector only exercises dialect branch selection while SQL still
executes against SQLite, and document that each database is reopened to isolate
persistent db.Dialector mutation. To validate engine-specific MySQL or
PostgreSQL DDL, add dialect-tagged integration coverage or assert generated SQL
rather than relying only on returned errors.
- Around line 202-224: Extend the assertions after loading eff in the migration
test to validate the copied field tree: assert the expected migrated Name field
exists and that it has the expected Field decorator. Use eff[0].Fields and its
decorators, alongside the existing model and service assertions.
In `@pkg/meta/dual_store_migrate_test.go`:
- Around line 303-317: Extend the test after the effective projection assertions
to load the raw service and its parameters, then assert that the raw declaration
retains the synthetic “this” parameter alongside “vals”. Keep the existing
effective-side assertion unchanged so the test verifies both sides of the
dual-store invariant.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56a09dc8-816d-4323-9498-975bf3157ae6
📒 Files selected for processing (8)
pkg/meta/dual_store_migrate.gopkg/meta/dual_store_migrate_coverage_test.gopkg/meta/dual_store_migrate_test.gopkg/meta/effective_merge.gopkg/meta/effective_merge_coverage_test.gopkg/meta/effective_merge_test.gopkg/meta/meta_raw_field.gopkg/meta/meta_raw_field_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/meta/effective_merge.go
- pkg/meta/meta_raw_field.go
- Return sqlite/postgres DROP INDEX failures before recreating the partial unique index. - Assert raw retains this while effective omits it; strengthen migrate/persist coverage assertions. - Table-drive child-table copy failures and clarify dialect-branch test limits. Co-authored-by: Cursor <cursoragent@cursor.com>
Review triage (review)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/meta/dual_store_migrate.go`:
- Around line 595-598: Update the index migration flow around
effectiveAppNameUniqueIndex so the existing uniqueness constraint remains
available if replacement CREATE UNIQUE INDEX fails: create the replacement under
a temporary name before removing the old index, or implement an equivalent
atomic swap/restore path, then finalize the intended index name. Add a
failure-injection test that forces creation to fail and verifies the original
unique index still protects live (application, name) rows.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d013234b-b595-4d54-a7a7-71d685fdc6c2
📒 Files selected for processing (3)
pkg/meta/dual_store_migrate.gopkg/meta/dual_store_migrate_coverage_test.gopkg/meta/dual_store_migrate_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/meta/dual_store_migrate_test.go
- Create the replacement (application, name) unique index under a temp name before dropping the final index. - Keep the temp index if final recreate fails, and cover the failure path with injection tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Review triage (review)
|
1 participant
User description
Summary
meta_raw_*GORM entities (RawModel/RawField/ …) and register them inmeta.Entities()so AutoMigrate creates empty raw tables alongside existingmeta_model*(EDS13 naming).MergeSameNameModelsByExtensionChain/MergeEffectiveModel) intopkg/meta; codegen now calls the shared implementation (gold-standard parity with prior generator tests).EnsureDualStoreTablesandMigrateIMDCatalogToDualStoreto copy IMD rows into raw, recompute one effective row per(application, name)(reuse tip id), and create a unique index on live effective keys.Out of scope (EDS-2+): Persist/Uninstall rewire away from
materializeEffectiveModels, Auth/loader reader migration, ModelData tip retirement, TS Raw facades.Upgrade note: Do not run
MigrateIMDCatalogToDualStoreon a live DB that will keep writing IMD rows intometa_modeluntil EDS-2 switches Persist to raw. Empty raw tables from AutoMigrate are safe.Test plan
go test ./pkg/meta/ ./internal/module/artifact/generate/ -count=1meta_raw_*tables exist and install still writes IMDmeta_modeluntil EDS-2Made with Cursor
PR Type
Enhancement, Tests
Description
Go Core: Introduce declaration-layer
meta_raw_*GORM entities with LGPL-3.0 SPDX headers and register them inmeta.Entities().Go Core: Extract E2 effective merge logic (
MergeSameNameModelsByExtensionChain/MergeEffectiveModel) intopkg/metaand update generator to call shared package.Go Core: Add
EnsureDualStoreTablesandMigrateIMDCatalogToDualStoremigration helpers alongside Go unit test suites.TypeScript Module: Update JSDoc documentation in
modules/meta/service/models/model.tsfor E2 effective projection and legacyModuleId.File Walkthrough
1 files
Delegate model extension chain merge to pkg/meta10 files
Implement dual-store table creation and IMD catalog migrationExtract E2 effective model merge logic to pkg/metaDefine RawArgument entity schema with SPDX headersDefine RawDecorator entity schema with SPDX headersDefine RawField entity schema with SPDX headersDefine RawModel entity schema with SPDX headersDefine RawParameter entity schema with SPDX headersDefine RawService entity schema with SPDX headersDefine RawTypeParameter entity schema with SPDX headersRegister dual-store raw entities in metadata entity registry3 files
Add Go unit tests for dual-store migration helpersAdd Go unit tests for extension chain model mergingUpdate metadata entity count and table name test assertions2 files
Document effective projection behavior and ModuleId field usageUpdate TS model JSDoc comments for dual-store effective representationSummary by CodeRabbit
New Features
Bug Fixes
Documentation