refactor(meta): split MetaModelData model into application and model - #236
Conversation
- Store mapping targets as application plus short model name, aligned with meta_model. - Update loader seed schema and reject top-level full-name model forms without a compatibility window. - Keep refBy/search/modelRef addressing as app.Model; leave Ref(xml_id) unchanged. 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: 8 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 (5)
📝 WalkthroughWalkthroughThe loader now requires separate application and short model fields. Metadata storage uses a composite application/model identity. Validation and errors preserve application context. Bootstrap data, fixtures, and related tests use the new representation. ChangesApplication/model identity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SeedRecord
participant EvolutionLoader
participant ModelData
SeedRecord->>EvolutionLoader: application and short model
EvolutionLoader->>ModelData: resolve application/model pair
ModelData-->>EvolutionLoader: model mapping
EvolutionLoader->>ModelData: persist separate application and model
Possibly related PRs
Suggested labels: 🚥 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/module/evolution/data/loader.go (1)
1291-1309: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass the application-qualified model to
detectSearchCardinality.Without the explicit first-result hint, the short
rec.Modelmakes cardinality detection fall back toManyToOne. This rejects valid multi-result searches and converts single-resultManyToManyvalues to scalars. Useapp + "." + modelNameor the equivalent qualified identity frommodel.🤖 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/evolution/data/loader.go` around lines 1291 - 1309, Update the detectSearchCardinality call in the values resolution loop to pass the application-qualified model identity, such as app plus rec.Model or the equivalent qualified model symbol, instead of the short rec.Model. Preserve the existing fieldName and raw arguments so multi-result searches and single-result ManyToMany values retain their correct cardinality.
🤖 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 `@internal/module/evolution/data/translated_seed.go`:
- Around line 201-202: The v == nil branch that constructs LoadError must
populate Application consistently with the neighboring validation errors, using
the trimmed application value from the translated seed record. Add or update a
test covering a nil translated value and assert that the resulting LoadError
retains Application.
In `@internal/module/metadata/model_data.go`:
- Around line 8-17: Update the ModelData schema and migration handling for the
new Application field so SQLite can migrate tables containing existing rows: add
a GORM default of an empty string or implement an explicit migration that safely
adds the column, then backfill existing Application values from authoritative
metadata. Keep Model unchanged and do not add it a default.
---
Outside diff comments:
In `@internal/module/evolution/data/loader.go`:
- Around line 1291-1309: Update the detectSearchCardinality call in the values
resolution loop to pass the application-qualified model identity, such as app
plus rec.Model or the equivalent qualified model symbol, instead of the short
rec.Model. Preserve the existing fieldName and raw arguments so multi-result
searches and single-result ManyToMany values retain their correct cardinality.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91e0f05c-7ec0-4c74-b765-80b501768307
📒 Files selected for processing (15)
internal/bootstrap/service/coordinator_admin_coverage_test.gointernal/module/evolution/data/loader.gointernal/module/evolution/data/loader_test.gointernal/module/evolution/data/translated_seed.gointernal/module/evolution/data/translated_seed_test.gointernal/module/lifecycle/uninstaller_model_data_test.gointernal/module/metadata/model_data.gomodules/auth/data/bootstrap.jsonmodules/auth/e2e/fixtures/smoke.jsonmodules/base/data/bootstrap.jsonmodules/base/demo/demo.jsonmodules/base/e2e/fixtures/language_format.jsonmodules/meta/e2e/fixtures/smoke.jsonmodules/meta/service/models/model_data.tsmodules/meta/service/tests/model_data_ref.test.ts
- Omit module/application in seed JSON; loader fills them from the applying module (E12). - Reject foreign xml_id namespaces and cross-app seeding; move auth e2e Company fixture to base. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/module/evolution/data/loader.go (1)
685-701: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared normalize-and-validate preflight into one helper.
planBatchRecordOrder(685-701) andplanRecordOrder(948-964) run the same sequence: callnormalizeRecordOwnership, trimModule/Name, check for a missing name, callvalidateRecordTargetModel, then check for missingValues. The only difference is howFilePath/RecordIndexare sourced (batchRecordfields vs. plainfilePath/idx). The corresponding test suites (TestPlanBatchRecordOrder_ValidationAndRefErrors,TestPlanRecordOrder_GuardsAndValidationErrors) already duplicate the same validation matrix to keep both paths in sync.Extract a small helper, for example:
func preflightRecord(rules *moduleRules, filePath string, recordIndex int, rec *record) (app, modelName string, err error) { if err := normalizeRecordOwnership(rules, filePath, recordIndex, rec); err != nil { return "", "", err } moduleName := strings.TrimSpace(rec.Module) localName := strings.TrimSpace(rec.Name) if localName == "" { return "", "", &LoadError{Kind: LoadErrorKindValidation, Code: LoadErrorCodeMissingName, FilePath: filePath, RecordIndex: recordIndex, Module: moduleName, Message: "missing name"} } app, modelName, err = validateRecordTargetModel(filePath, recordIndex, *rec) if err != nil { return "", "", err } if rec.Values == nil { return "", "", &LoadError{Kind: LoadErrorKindValidation, Code: LoadErrorCodeMissingValues, FilePath: filePath, RecordIndex: recordIndex, Module: moduleName, Name: localName, Application: app, Model: modelName, Message: "missing values"} } return app, modelName, nil }Both call sites then reduce to a single call plus their own duplicate-key check, removing the duplicated validation logic without changing behavior.
Also applies to: 948-964
🤖 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/evolution/data/loader.go` around lines 685 - 701, Extract the shared normalize-and-validate sequence from planBatchRecordOrder and planRecordOrder into a preflightRecord helper accepting rules, filePath, recordIndex, and *record, preserving all existing validation errors and returning app/modelName. Replace both call-site blocks with the helper call, retaining only each path’s own duplicate-key checks and sourcing file path/index from its existing inputs.
🤖 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/evolution/data/loader.go`:
- Around line 685-701: Extract the shared normalize-and-validate sequence from
planBatchRecordOrder and planRecordOrder into a preflightRecord helper accepting
rules, filePath, recordIndex, and *record, preserving all existing validation
errors and returning app/modelName. Replace both call-site blocks with the
helper call, retaining only each path’s own duplicate-key checks and sourcing
file path/index from its existing inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73363401-fa73-4ab3-be5d-8a206e70e666
📒 Files selected for processing (10)
internal/module/evolution/data/loader.gointernal/module/evolution/data/loader_test.gomodules/auth/data/bootstrap.jsonmodules/auth/e2e/fixtures/smoke.jsonmodules/base/data/bootstrap.jsonmodules/base/demo/demo.jsonmodules/base/e2e/fixtures/company_child.jsonmodules/base/e2e/fixtures/language_format.jsonmodules/base/package.jsonmodules/meta/e2e/fixtures/smoke.json
💤 Files with no reviewable changes (5)
- modules/base/e2e/fixtures/language_format.json
- modules/meta/e2e/fixtures/smoke.json
- modules/base/demo/demo.json
- modules/auth/data/bootstrap.json
- modules/base/data/bootstrap.json
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/module/evolution/data/loader_test.go
- Replace the short-name model column with model_id pointing at meta_model. - Keep seed JSON short model for resolve; loader writes ModelId after lookup. Co-authored-by: Cursor <cursoragent@cursor.com>
- Expect short Language model names after MetaModelData target split. - Assert language_zh_cn on company_child fixture after it left smoke.json. Co-authored-by: Cursor <cursoragent@cursor.com>
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 `@modules/meta/service/models/model_data.ts`:
- Around line 60-61: Update the ModelId field metadata in MetaModelData to
include notNull: true, matching the Go schema, while retaining the optional
property marker only for unloaded relations. Add a focused test that verifies
validation rejects a missing ModelId.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 928ad9e6-97d5-4e65-84c5-e3674d7232e7
📒 Files selected for processing (6)
internal/bootstrap/service/coordinator_admin_coverage_test.gointernal/module/evolution/data/loader.gointernal/module/evolution/data/loader_test.gointernal/module/lifecycle/uninstaller_model_data_test.gointernal/module/metadata/model_data.gomodules/meta/service/models/model_data.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/module/lifecycle/uninstaller_model_data_test.go
- internal/module/evolution/data/loader.go
- internal/module/evolution/data/loader_test.go
- Build application.Model before detectSearchCardinality so ManyToMany seeds work. - Fill Application on cycle and nil translated-map LoadErrors; sync mapping target on update. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c0405c6. Configure here.
- Exercise ownership nil guards, LoadError model display, and recordModelFull fallbacks. - Sync and fail stale application/model_id updates on existing mappings. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
User description
Summary
meta_model_datamapping target from a single full name (auth.User) intoapplication+ shortmodel(E11 / PR-EID-4).splitModel.refBy/search/modelRef/serviceRef) still uses fullapp.Model;Ref/RefOrNullsignatures staymodule.name→res_id.Test plan
go test ./internal/module/evolution/data/ ./internal/module/metadata/ ./internal/module/lifecycle/ ./internal/bootstrap/... -count=1./choysum test unit meta --be(after rebuild)Made with Cursor
PR Type
Enhancement
Description
Go core: Split
ModelDatamapping target intoApplicationand shortModelfields, updating loader validation rules.TS modules: Added
Applicationproperty toMetaModelDatamodel and updated all module JSON seed records.Testing & Compliance: Updated Go unit tests, TS unit tests, and E2E fixtures; no new files requiring SPDX headers.
File Walkthrough
8 files
Update admin seed ModelData test fixture with Application fieldUpdate seed loader tests for application and model separationUpdate translated seed tests with application and model fieldsUpdate uninstaller tests with application and short model nameUpdate auth E2E fixture records to use application and modelUpdate base language format E2E fixture record fieldsUpdate meta E2E smoke fixture to use application and modelUpdate TS unit test mock data store with Application field2 files
Validate record application and short model name during seed loadingCheck application and short model name for language translation seed2 files
Add Application field and compound index to ModelData structAdd Application field decorator to MetaModelData class3 files
Split model into application and model in auth bootstrap seedSplit model into application and model in base bootstrap seedUpdate base demo JSON record to split model fieldSummary by CodeRabbit