diff --git a/.claude/skills/fix-issue/findings/mdl-other/2026-10-30-source-catalog-rebuild-redescribes-every-document-after-each-save.json b/.claude/skills/fix-issue/findings/mdl-other/2026-10-30-source-catalog-rebuild-redescribes-every-document-after-each-save.json new file mode 100644 index 000000000..61271dc7c --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-other/2026-10-30-source-catalog-rebuild-redescribes-every-document-after-each-save.json @@ -0,0 +1 @@ +{"area":"mdl/catalog","date":"2026-10-30","symptom":"On a project with a source index, every catalog rebuild — `refresh catalog`, and the implicit ones `search` / `show references` start after any save — described every document again: 38 s on a 4,753-unit app on macOS, hours on Windows before ako/mxcli#1789, repeated after every edit in an agent loop","cause":"By design a refresh rebuilds at least the cached level (#1081, `cmd_catalog.go`), and `buildSource` had no memory of what it had described before, so the cheapest rebuild that could still be saved was the most expensive one","file":"`mdl/catalog/builder_source_reuse.go` (basis tables `source_units` / `source_elements`, `planSourceReuse`, `sourceDependents`, `sourceBasis`), `mdl/catalog/builder_source.go`, `modelsdk/mpr/reader.go` (`UnitContentHashes`), `mdl/executor/cmd_catalog.go` (`sourceFingerprint`, `catalogForce`)","insight":"**A row is reusable only if everything its describe read is unchanged, and the describe reads more than its own unit.** Three dependencies turned up, each found by a step of the equality test (incremental vs forced full build, row for row, after each kind of edit): other documents named in the refs (a page prints its data-source flow's return entity: `-- Context: $currentObject (M.ProbeA)`); the unit's container, which is a Unit-table column and not in the bytes, so a move into an existing folder kept the stale `folder` line; and the describer itself (fingerprint = executable path, size, mtime). **The equality test needs a control that breaks the dependency logic**: with `sourceDependents` stubbed to return nothing the first version of the test still passed, because none of its edits changed a dependent's text; the page-context step is what made it fail. Anything the plan cannot follow (a changed unit that is not a described document: folder, module, settings, Java action) falls back to describing everything, so reuse is an optimisation of the full build and never a different answer. FactoryManagement: one attribute added, plain refresh 11.2 s vs 38.2 s, 3,525 of 3,718 rows reused, byte-identical to `force`","refs":["ako/mxcli#1790","ako/mxcli#1789","mendixlabs/mxcli#1081"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index fc5801b32..5313377b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Rebuilding a catalog that has a source index describes only what changed** (ako/mxcli#1790) — once a project had a source index (`refresh catalog full source`), every later rebuild described every document again. That includes the rebuilds `search` and `show references` start after each save. On a 4,753-unit app that was 38 s per rebuild, and much longer on Windows before #1789. A rebuild now re-describes the documents whose unit changed (an entity's unit is its module's domain model) and the documents whose references name one of them. The rest are carried over from the cache. After adding one attribute, the same app rebuilds in 11 s: 3,525 rows reused, 193 described, and the index is byte-identical to a forced full rebuild. A change outside the described documents (a folder, a module, the project settings, a Java action), a cache written by another mxcli build, and `force` still describe everything. The cache records each unit's content hash and container, so a document moved into another folder is described again too. - **Commands on an MPR v2 project open only the files they need** (ako/mxcli#1791) — on a 4,753-unit app, `show modules` opened 4,511 unit files, `describe microflow` 2,996 and `describe page` 3,176, in every process. The commands read every folder to build module paths and every microflow to name or find one, and `show modules` decoded every page, snippet, flow, constant and Java action just to count them. Each open is cheap on macOS and is scanned by Defender on Windows, so this added time to every command an agent runs. The unit index in `.mxcli/` now also holds each unit's name and `Excluded` flag, keyed by content hash like its type, so it never goes stale. Folders and microflows are named from the index, a microflow is found by name and read by ID, and `show modules` counts from unit types. The same commands now open 509, 93 and 274 files, with output identical across three projects and all 229 page describes. The index moves to `unit-index-2.idx`, so an older mxcli on the same project keeps its own `unit-types.idx` instead of the two rebuilding each other's. - **`refresh catalog full source` reads each unit file once instead of millions of times** (ako/mxcli#1789) — the source index describes every document, and each microflow describe found its microflow, and every microflow it calls, by listing every microflow of the project from disk. On a 4,753-unit MPR v2 app one refresh opened 7,532,649 `.mxunit` files: 5.5 minutes on macOS, and 90 minutes or more on Windows, where Defender scans every file open. The catalog build now keeps the units it reads in memory, and a describe reads its microflow and the microflows it calls by ID. The same refresh opens 4,753 files and takes 31 s, and its source text is byte-identical. The rebuilds that `show references` and `search` run on a project with a source index are the same build, so they speed up too. - **Moving a Java or JavaScript action to another module takes its source along** — `move java action A.X to B` moved the document and left `javasource/a/actions/X.java` behind (likewise a JavaScript action's `.js`). The build still succeeded, but the app called a generated stub that throws "Java action was not implemented", and `describe` printed a placeholder body that re-executing would have stored. The source now moves to the target module's directory, with a Java source's `package` line rewritten exactly as mxbuild regenerates it; an occupied destination refuses the move before anything changes. diff --git a/docs-site/src/tools/refresh-catalog.md b/docs-site/src/tools/refresh-catalog.md index 573399bfd..b94798624 100644 --- a/docs-site/src/tools/refresh-catalog.md +++ b/docs-site/src/tools/refresh-catalog.md @@ -71,6 +71,8 @@ mxcli -p app.mpr -c "REFRESH CATALOG FULL FORCE" The catalog is cached in `.mxcli/catalog.db` next to the MPR file. On subsequent refreshes, mxcli checks whether the project has changed and only rebuilds if necessary (unless `FORCE` is specified). +A catalog with the MDL source index (`REFRESH CATALOG FULL SOURCE`) keeps it on every later rebuild, including the ones `SEARCH` and `SHOW REFERENCES` start when the project has changed. Those rebuilds describe again only the documents that changed, and the documents that reference them. Every other row is carried over from the cache, so a rebuild after one edit costs about as much as `REFRESH CATALOG FULL`. Everything is described again when something outside the described documents changes (a folder, a module, the project settings, a Java action), when another mxcli build wrote the cache, and with `FORCE`. + ## When to Refresh - **After opening a project** -- Run `REFRESH CATALOG` to enable queries diff --git a/mdl/backend/modelsdk/units.go b/mdl/backend/modelsdk/units.go index b494c3887..b3fd74078 100644 --- a/mdl/backend/modelsdk/units.go +++ b/mdl/backend/modelsdk/units.go @@ -92,6 +92,13 @@ func (b *Backend) ListFolders() ([]*types.FolderInfo, error) { return out, nil } +// UnitContentHashes returns a hash of every unit's current contents, keyed by +// unit ID, so a caller can tell which units changed since it last looked (the +// catalog's incremental source index, ako/mxcli#1790). +func (b *Backend) UnitContentHashes() (map[string]string, error) { + return b.reader.UnitContentHashes() +} + // AddRawUnit inserts a new unit verbatim. // // Exposed on the backend because the marketplace transplant copies a module's diff --git a/mdl/catalog/builder.go b/mdl/catalog/builder.go index 422984f45..4afce78d0 100644 --- a/mdl/catalog/builder.go +++ b/mdl/catalog/builder.go @@ -96,6 +96,12 @@ type Builder struct { resolution float64 // Leiden resolution for the graph-analysis pass describeFunc DescribeFunc + // prevSource is the last source build's rows and basis, and + // sourceFingerprint the mxcli build writing this one: together they let + // unchanged documents keep their rows (builder_source_reuse.go). + prevSource *PreviousSource + sourceFingerprint string + // The project's DefaultLanguageCode, resolved once per build by // defaultLanguage(). Texts (enumeration value captions today) are read in // this language: see language.go. diff --git a/mdl/catalog/builder_source.go b/mdl/catalog/builder_source.go index 1edbcbd0b..786636d0e 100644 --- a/mdl/catalog/builder_source.go +++ b/mdl/catalog/builder_source.go @@ -87,7 +87,9 @@ func (b *Builder) buildSource() error { // Phase 1: Collect all items to describe (fast — uses cached lists) var items []sourceItem - // Entities + // Entities. An entity is described from its module's domain model, the + // unit whose hash says whether it changed. + entityUnit := map[string]string{} dms, err := b.cachedDomainModels() if err == nil { for _, dm := range dms { @@ -95,6 +97,7 @@ func (b *Builder) buildSource() error { moduleName := b.hierarchy.getModuleName(moduleID) for _, ent := range dm.Entities { items = append(items, sourceItem{SourceEntity, moduleName + "." + ent.Name, moduleName, string(ent.ID)}) + entityUnit[string(ent.ID)] = string(dm.ID) } } } @@ -204,11 +207,26 @@ func (b *Builder) buildSource() error { return nil } + // Carry over the rows whose basis has not changed (#1790); describe the + // rest. Without unit hashes nothing is reused and nothing is recorded. + units := make([]string, len(items)) + for i, it := range items { + units[i] = sourceUnitOf(entityUnit, it) + } + hashes := b.sourceBasis() + plan := b.planSourceReuse(items, units, hashes) + toDescribe := make([]sourceItem, 0, len(items)) + for i, it := range items { + if _, ok := plan.reused[i]; !ok { + toDescribe = append(toDescribe, it) + } + } + // Phase 2: Generate MDL source in parallel. This is the slow phase on large // projects (one describe per document), so report progress periodically — // from a single ticker goroutine, the only caller of report() during the // run, so the progress sink isn't written concurrently. - total := len(items) + total := len(toDescribe) stop := make(chan struct{}) var done atomic.Int64 go func() { @@ -224,7 +242,7 @@ func (b *Builder) buildSource() error { } }() - results, failures := runDescribes(items, b.describeFunc, + results, failures := runDescribes(toDescribe, b.describeFunc, max(min(runtime.NumCPU(), 8), 1), func(n int) { done.Store(int64(n)) }) @@ -248,7 +266,18 @@ func (b *Builder) buildSource() error { stmt.Exec(res.item.qn, res.item.objType, res.text, res.item.moduleName, res.item.id) count++ } + for i, row := range plan.reused { + it := items[i] + stmt.Exec(it.qn, it.objType, row.text, it.moduleName, it.id) + count++ + } + if err := b.persistSourceBasis(items, units, hashes); err != nil { + return err + } + if len(plan.reused) > 0 { + b.report(sourceReuseSummary(len(plan.reused), len(toDescribe)), len(toDescribe)) + } b.report("source", count) // Report what produced no row. Silence here is what let #912 sit unnoticed: diff --git a/mdl/catalog/builder_source_reuse.go b/mdl/catalog/builder_source_reuse.go new file mode 100644 index 000000000..be3bc174b --- /dev/null +++ b/mdl/catalog/builder_source_reuse.go @@ -0,0 +1,331 @@ +// SPDX-License-Identifier: Apache-2.0 + +package catalog + +import ( + "database/sql" + "fmt" +) + +// Incremental CATALOG.SOURCE (ako/mxcli#1790). +// +// A project with a source index rebuilds it whenever the catalog goes stale, +// which is every save, and every rebuild described every document again. Most +// of them have not changed. A source build now records what it described from +// (each unit's content hash, and which document came from which unit), and the +// next build carries a document's row over when nothing its text depends on +// has changed: +// +// - its own unit's bytes are unchanged (an entity's unit is its module's +// domain model, so any change there re-describes all its entities); +// - no document it references changed: a describe resolves names against +// other documents (a called flow's return type, a data source's entity), +// so a document whose refs name a changed one is described again; +// - nothing else in the project changed. A change to any unit that is not +// a described document — a folder, a module, the project settings, a +// Java action — can move names or change what describes print, and is +// not followed through the refs, so it makes the build describe +// everything, as it always did; +// - the same mxcli wrote the previous rows (the fingerprint): another +// version may describe differently. +// +// Anything the plan cannot vouch for is described again; reuse is only ever +// an optimisation of a full rebuild, never a different answer. + +// MetaSourceFingerprint records which mxcli build wrote the source rows. +const MetaSourceFingerprint = "source_fingerprint" + +// unitHasher is the optional reader capability incremental source needs: a +// hash of every unit's current contents, keyed by unit ID. +type unitHasher interface { + UnitContentHashes() (map[string]string, error) +} + +// PreviousSource is what the last source build recorded. +type PreviousSource struct { + rows map[string]previousRow // by ElementId + unitHashes map[string]string // unit ID -> content hash, every unit + described map[string]bool // unit IDs that held a described document + unitMembers map[string][]string // unit ID -> qualified names described from it +} + +type previousRow struct { + qn, objType, text, module string +} + +// LoadPreviousSource reads the basis of the source index cached at path, or +// returns nil and why it cannot be reused. +func LoadPreviousSource(path, fingerprint string) (*PreviousSource, string) { + if path == "" || fingerprint == "" { + return nil, "no cache to reuse" + } + db, err := sql.Open("sqlite", "file:"+path+"?mode=ro") + if err != nil { + return nil, err.Error() + } + defer db.Close() + + var mode, fp string + _ = db.QueryRow(`SELECT Value FROM catalog_meta WHERE Key = ?`, MetaBuildMode).Scan(&mode) + _ = db.QueryRow(`SELECT Value FROM catalog_meta WHERE Key = ?`, MetaSourceFingerprint).Scan(&fp) + if mode != "source" { + return nil, "the cache has no source index" + } + if fp != fingerprint { + return nil, "the source index was written by another mxcli build" + } + + p := &PreviousSource{ + rows: map[string]previousRow{}, + unitHashes: map[string]string{}, + described: map[string]bool{}, + unitMembers: map[string][]string{}, + } + units, err := db.Query(`SELECT UnitId, Hash, Described FROM source_units`) + if err != nil { + return nil, "the cache records no source basis" + } + for units.Next() { + var id, hash string + var described int + if err := units.Scan(&id, &hash, &described); err != nil { + units.Close() + return nil, err.Error() + } + p.unitHashes[id] = hash + p.described[id] = described != 0 + } + units.Close() + if len(p.unitHashes) == 0 { + return nil, "the cache records no source basis" + } + + elems, err := db.Query(`SELECT UnitId, QualifiedName FROM source_elements`) + if err != nil { + return nil, err.Error() + } + for elems.Next() { + var unit, qn string + if err := elems.Scan(&unit, &qn); err != nil { + elems.Close() + return nil, err.Error() + } + p.unitMembers[unit] = append(p.unitMembers[unit], qn) + } + elems.Close() + + rows, err := db.Query(`SELECT ElementId, QualifiedName, ObjectType, SourceText, ModuleName FROM source`) + if err != nil { + return nil, err.Error() + } + defer rows.Close() + for rows.Next() { + var id, qn, objType, text, module sql.NullString + if err := rows.Scan(&id, &qn, &objType, &text, &module); err != nil { + return nil, err.Error() + } + if id.String == "" { + continue + } + p.rows[id.String] = previousRow{qn.String, objType.String, text.String, module.String} + } + return p, "" +} + +// SetPreviousSource gives the build the previous source index to reuse from. +func (b *Builder) SetPreviousSource(p *PreviousSource) { + b.prevSource = p +} + +// SetSourceFingerprint identifies the mxcli build that writes the source rows. +func (b *Builder) SetSourceFingerprint(fp string) { + b.sourceFingerprint = fp +} + +// sourceBasis is what each unit's describe depends on of the unit itself: +// its content hash and its container. The container is a column of the Unit +// table, not part of the bytes, so moving a document into another folder +// leaves its content hash as it was — and a describe prints the folder. Nil +// when the reader cannot hash units, which reuses and records nothing. +func (b *Builder) sourceBasis() map[string]string { + h, ok := b.reader.(unitHasher) + if !ok { + return nil + } + hashes, err := h.UnitContentHashes() + if err != nil { + return nil + } + units, err := b.reader.ListUnits() + if err != nil { + return nil + } + for _, u := range units { + id := string(u.ID) + if hash, ok := hashes[id]; ok { + hashes[id] = hash + "|" + string(u.ContainerID) + } + } + return hashes +} + +// sourceReusePlan says, per item, whether its previous row can be carried +// over; reused[i] is that row's text. +type sourceReusePlan struct { + reused map[int]previousRow + reason string // why nothing was reused, when nothing was +} + +// planSourceReuse decides which items keep their previous row. units maps +// each item to the unit it is described from; hashes is every unit's content +// hash now. +func (b *Builder) planSourceReuse(items []sourceItem, units []string, hashes map[string]string) sourceReusePlan { + prev := b.prevSource + if prev == nil { + return sourceReusePlan{reason: "no previous source index"} + } + if hashes == nil { + return sourceReusePlan{reason: "unit hashes unavailable"} + } + + itemUnit := map[string]bool{} + for _, u := range units { + itemUnit[u] = true + } + changed := map[string]bool{} + for id, h := range hashes { + if prev.unitHashes[id] != h { + changed[id] = true + } + } + for id := range prev.unitHashes { + if _, ok := hashes[id]; !ok { + changed[id] = true + } + } + for id := range changed { + if !itemUnit[id] && !prev.described[id] { + return sourceReusePlan{reason: "a unit other than a described document changed"} + } + } + + // Names a describe may have resolved against: every document described + // from a changed unit, now and before (a rename or a delete). + var names []string + for i, it := range items { + if changed[units[i]] { + names = append(names, it.qn) + } + } + for id := range changed { + names = append(names, prev.unitMembers[id]...) + } + dependents, err := b.sourceDependents(names) + if err != nil { + return sourceReusePlan{reason: "references unavailable: " + err.Error()} + } + + plan := sourceReusePlan{reused: map[int]previousRow{}} + for i, it := range items { + if changed[units[i]] || dependents[it.qn] { + continue + } + row, ok := prev.rows[it.id] + if !ok || row.qn != it.qn || row.objType != it.objType || row.module != it.moduleName { + continue + } + plan.reused[i] = row + } + return plan +} + +// sourceDependents is the qualified names of the documents whose refs name +// one of names, or a member of one (`M.Entity.Attribute` under `M.Entity`). +// The refs are this build's, written by buildReferences before buildSource. +func (b *Builder) sourceDependents(names []string) (map[string]bool, error) { + out := map[string]bool{} + if len(names) == 0 { + return out, nil + } + stmt, err := b.tx.Prepare(`SELECT DISTINCT SourceName FROM refs + WHERE TargetName = ? OR substr(TargetName, 1, length(?) + 1) = ? || '.'`) + if err != nil { + return nil, err + } + defer stmt.Close() + seen := map[string]bool{} + for _, n := range names { + if n == "" || seen[n] { + continue + } + seen[n] = true + rows, err := stmt.Query(n, n, n) + if err != nil { + return nil, err + } + for rows.Next() { + var src string + if err := rows.Scan(&src); err != nil { + rows.Close() + return nil, err + } + out[src] = true + } + rows.Close() + } + return out, nil +} + +// persistSourceBasis records what this build described from, so the next one +// can tell what changed. +func (b *Builder) persistSourceBasis(items []sourceItem, units []string, hashes map[string]string) error { + if hashes == nil || b.sourceFingerprint == "" { + return nil + } + described := map[string]bool{} + for _, u := range units { + described[u] = true + } + ustmt, err := b.tx.Prepare(`INSERT OR REPLACE INTO source_units (UnitId, Hash, Described) VALUES (?, ?, ?)`) + if err != nil { + return err + } + defer ustmt.Close() + for id, h := range hashes { + d := 0 + if described[id] { + d = 1 + } + if _, err := ustmt.Exec(id, h, d); err != nil { + return err + } + } + estmt, err := b.tx.Prepare(`INSERT INTO source_elements (ElementId, UnitId, QualifiedName, ObjectType) VALUES (?, ?, ?, ?)`) + if err != nil { + return err + } + defer estmt.Close() + for i, it := range items { + if _, err := estmt.Exec(it.id, units[i], it.qn, it.objType); err != nil { + return err + } + } + _, err = b.tx.Exec(`INSERT OR REPLACE INTO catalog_meta (Key, Value) VALUES (?, ?)`, MetaSourceFingerprint, b.sourceFingerprint) + return err +} + +// sourceReuseSummary is the progress line for a build that reused rows. +func sourceReuseSummary(reused, described int) string { + return fmt.Sprintf("source: %d reused, %d described", reused, described) +} + +// sourceUnitOf maps entity element IDs to their domain model's unit; every +// other source document is its own unit. +func sourceUnitOf(entityUnit map[string]string, it sourceItem) string { + if it.objType == SourceEntity { + if u, ok := entityUnit[it.id]; ok { + return u + } + } + return it.id +} diff --git a/mdl/catalog/catalog.go b/mdl/catalog/catalog.go index 2c79a33d0..257867d75 100644 --- a/mdl/catalog/catalog.go +++ b/mdl/catalog/catalog.go @@ -446,7 +446,7 @@ func (c *Catalog) migrateIfSchemaMismatch() error { for _, key := range []string{ MetaMprPath, MetaMprModTime, MetaMendixVersion, MetaBuildMode, MetaBuildTime, MetaBuildDuration, - MetaGraphAnalysis, MetaGraphResolution, + MetaGraphAnalysis, MetaGraphResolution, MetaSourceFingerprint, } { if _, err := c.db.Exec(`DELETE FROM catalog_meta WHERE Key = ?`, key); err != nil { return fmt.Errorf("clearing cache-info key %s: %w", key, err) diff --git a/mdl/catalog/tables.go b/mdl/catalog/tables.go index 28113f3ec..9e00de0cf 100644 --- a/mdl/catalog/tables.go +++ b/mdl/catalog/tables.go @@ -1521,6 +1521,23 @@ func (c *Catalog) createTables() error { ModuleName )`, + // What the source rows were described from: every unit's content + // hash at the time, whether it held a described document, and which + // document came from which unit. The next source build compares and + // re-describes only what changed (builder_source_reuse.go, #1790). A + // cache without them reuses nothing, so they need no schema bump. + `CREATE TABLE IF NOT EXISTS source_units ( + UnitId TEXT PRIMARY KEY, + Hash TEXT NOT NULL, + Described INTEGER NOT NULL + )`, + `CREATE TABLE IF NOT EXISTS source_elements ( + ElementId TEXT NOT NULL, + UnitId TEXT NOT NULL, + QualifiedName TEXT NOT NULL, + ObjectType TEXT NOT NULL + )`, + `CREATE VIRTUAL TABLE IF NOT EXISTS source USING fts5( QualifiedName, ObjectType, diff --git a/mdl/executor/catalog_source_incremental_pedapp_test.go b/mdl/executor/catalog_source_incremental_pedapp_test.go new file mode 100644 index 000000000..1c470b17e --- /dev/null +++ b/mdl/executor/catalog_source_incremental_pedapp_test.go @@ -0,0 +1,189 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "context" + "fmt" + "regexp" + "sort" + "strconv" + "strings" + "testing" +) + +// ako/mxcli#1790: a project with a source index rebuilt all of it whenever the +// catalog went stale — every save — describing every document again. The +// build now carries a document's row over when nothing its text depends on +// changed. Reuse is only safe if it never changes the answer, so every step +// below compares the incremental build with a forced full one, row for row. + +// sourceBuild builds a source catalog, incremental unless force, and returns +// its rows (ElementId|QualifiedName|ObjectType -> text) and the build output. +func sourceBuild(t *testing.T, exec *Executor, force bool) (map[string]string, string) { + t.Helper() + ctx := exec.newExecContext(context.Background()) + var out bytes.Buffer + ctx.Output = &out + ctx.catalogForce = force + if err := buildCatalog(ctx, true, true, false, 0); err != nil { + t.Fatalf("build: %v\n%s", err, out.String()) + } + defer func() { ctx.Catalog.Close(); ctx.Catalog = nil }() + res, err := ctx.Catalog.Query(`SELECT ElementId, QualifiedName, ObjectType, SourceText FROM source`) + if err != nil { + t.Fatalf("query source: %v", err) + } + rows := map[string]string{} + for _, r := range res.Rows { + key := fmt.Sprintf("%v|%v|%v", r[0], r[1], r[2]) + if _, dup := rows[key]; dup { + t.Errorf("source row %s written twice", key) + } + rows[key] = fmt.Sprint(r[3]) + } + return rows, out.String() +} + +var reusedRe = regexp.MustCompile(`source: (\d+) reused, (\d+) described`) + +// reusedCount is how many rows the build carried over, from its progress line. +func reusedCount(out string) int { + m := reusedRe.FindStringSubmatch(out) + if m == nil { + return 0 + } + n, _ := strconv.Atoi(m[1]) + return n +} + +func diffSourceRows(t *testing.T, step string, got, want map[string]string) { + t.Helper() + var keys []string + for k := range want { + keys = append(keys, k) + } + for k := range got { + if _, ok := want[k]; !ok { + keys = append(keys, k) + } + } + sort.Strings(keys) + bad := 0 + for _, k := range keys { + g, gok := got[k] + w, wok := want[k] + switch { + case !gok: + t.Errorf("%s: the incremental build has no row for %s", step, k) + case !wok: + t.Errorf("%s: the incremental build kept a row the full build does not have: %s", step, k) + case g != w: + t.Errorf("%s: %s differs from the full build:\n--- incremental\n%s\n--- full\n%s", step, k, g, w) + default: + continue + } + if bad++; bad == 5 { + t.Fatalf("%s: stopping after %d differences", step, bad) + } + } +} + +func TestSourceCatalog_IncrementalBuildEqualsFullBuild(t *testing.T) { + exec, out, _ := openPedAppCopy(t) + if err := agreeExec(t, exec, redeclareChain(4, "create")); err != nil { + t.Fatalf("setup: %v\n%s", err, out.String()) + } + // A page whose describe prints what another document says: the context + // entity of a microflow data source is the flow's return type. + if err := agreeExec(t, exec, `mdl 1; +create persistent entity MyFirstModule.ProbeA (Name: String(50)); +create persistent entity MyFirstModule.ProbeB (Name: String(50)); +create microflow MyFirstModule.GetProbe () returns MyFirstModule.ProbeA as $A +begin + $A = create MyFirstModule.ProbeA (Name = 'a'); + return $A; +end; +create page MyFirstModule.ProbePage (Title: 'P', Layout: Atlas_Core.Atlas_Default) { + dataview dataView1 (DataSource: microflow MyFirstModule.GetProbe) { + textbox txtName (Attribute: Name) + } +}; +`); err != nil { + t.Fatalf("setup page: %v\n%s", err, out.String()) + } + base, _ := sourceBuild(t, exec, true) + if len(base) < 50 { + t.Fatalf("control: the fixture's source index has %d rows", len(base)) + } + + // Each step edits the project the way an agent does between two + // rebuilds, and names what the incremental build must not get wrong. + steps := []struct { + name, mdl string + }{ + // Unchanged project: everything is reused, and that must change nothing. + {"no change", ""}, + // One flow's own bytes change. + {"edit a flow", strings.Replace(redeclareChain(2, "create or modify"), "length($Title) + $Prev", "length($Title) * 2 + $Prev", 1)}, + // A callee's signature changes: its callers' describes resolve it. + {"callee returns another type", `mdl 1; +create or modify microflow MyFirstModule.MF_Chain001 ($Title: String) returns Boolean as $B +begin + declare $B Boolean = length($Title) > 3; + return $B; +end; +`}, + // The page's own unit is untouched, and its text still changes: the + // data source flow now returns another entity. + {"data source flow returns another entity", `mdl 1; +create or modify microflow MyFirstModule.GetProbe () returns MyFirstModule.ProbeB as $B +begin + $B = create MyFirstModule.ProbeB (Name = 'b'); + return $B; +end; +`}, + // A new document, and a document that calls it. + {"add a flow", `mdl 1; +create microflow MyFirstModule.MF_Added ($Title: String) returns Integer as $N +begin + $N = call microflow MyFirstModule.MF_Chain004(Title = $Title); + return $N; +end; +`}, + // A rename rewrites the callers too; the old name's row must go. + {"rename a flow", "rename microflow MyFirstModule.MF_Chain004 to MF_Chain004b;"}, + // A domain model change re-describes its entities and what uses them. + {"alter an entity", "alter entity FeedbackModule.Feedback add attribute IncrementalProbe: String;"}, + // A new folder is a unit no describe covers: everything is described. + {"move a flow to a new folder", "move microflow MyFirstModule.MF_Chain001 to folder 'Probes';"}, + // A move into an existing folder changes no unit's bytes, only its + // container, and the describe prints the folder. + {"move a flow into that folder", "move microflow MyFirstModule.MF_Chain002 to folder 'Probes';"}, + // A deleted document's row must not survive. + {"drop a flow", "drop microflow MyFirstModule.MF_Added;"}, + } + reusedAny := false + for _, s := range steps { + if s.mdl != "" { + if err := agreeExec(t, exec, s.mdl); err != nil { + t.Fatalf("%s: %v\n%s", s.name, err, out.String()) + } + } + got, buildOut := sourceBuild(t, exec, false) + n := reusedCount(buildOut) + if n > 0 { + reusedAny = true + } + if s.name == "no change" && n != len(base) { + t.Errorf("no change: reused %d of %d rows, want all\n%s", n, len(base), buildOut) + } + want, _ := sourceBuild(t, exec, true) + diffSourceRows(t, s.name, got, want) + t.Logf("%s: reused %d of %d rows", s.name, n, len(want)) + } + if !reusedAny { + t.Fatalf("control: no build reused a row, so the comparison proves nothing about reuse") + } +} diff --git a/mdl/executor/cmd_catalog.go b/mdl/executor/cmd_catalog.go index 26d39e6ce..3c1e1d9cf 100644 --- a/mdl/executor/cmd_catalog.go +++ b/mdl/executor/cmd_catalog.go @@ -529,6 +529,16 @@ func buildCatalog(ctx *ExecContext, full, isSource, communities bool, resolution builder.SetDescribeFunc(func(objectType string, qualifiedName string, id string) (string, error) { return captureDescribeParallel(ctx, objectType, qualifiedName, model.ID(id)) }) + // Rows of documents nothing has changed under are carried over from + // the cached index, which every save makes stale (#1790). FORCE + // describes everything, as does a cache another mxcli build wrote. + fp := sourceFingerprint() + builder.SetSourceFingerprint(fp) + if !ctx.catalogForce { + if prev, _ := catalog.LoadPreviousSource(getCachePath(ctx), fp); prev != nil { + builder.SetPreviousSource(prev) + } + } } // Supply built-in widget definitions (hand-crafted COMBOBOX, GALLERY, // DATAGRID, filter widgets, …) for the widget_definitions catalog table. @@ -705,6 +715,9 @@ func execRefreshCatalogStmt(ctx *ExecContext, stmt *ast.RefreshCatalogStmt) erro ctx.Catalog = nil } + ctx.catalogForce = stmt.Force + defer func() { ctx.catalogForce = false }() + // Handle background mode — clone ctx so the goroutine doesn't race // with the main dispatch loop (which may syncBack and mutate fields). if stmt.Background { @@ -756,6 +769,23 @@ func execRefreshCatalogStmt(ctx *ExecContext, stmt *ast.RefreshCatalogStmt) erro return buildCatalog(ctx, full, source, stmt.Communities, stmt.Resolution) } +// sourceFingerprint identifies the running mxcli build, so source rows are +// only reused by the build that described them: another version may describe +// the same document differently. The executable's path, size and modification +// time change with every build, released or local, without needing version +// stamping. "" (no executable to stat) reuses nothing. +func sourceFingerprint() string { + exe, err := os.Executable() + if err != nil { + return "" + } + fi, err := os.Stat(exe) + if err != nil { + return "" + } + return fmt.Sprintf("%s|%d|%d", exe, fi.Size(), fi.ModTime().UnixNano()) +} + // execRefreshCatalog handles REFRESH CATALOG [FULL] command (legacy signature). func execRefreshCatalog(ctx *ExecContext, full bool) error { return execRefreshCatalogStmt(ctx, &ast.RefreshCatalogStmt{Full: full, Source: false}) diff --git a/mdl/executor/exec_context.go b/mdl/executor/exec_context.go index 5bc008795..edbf61afb 100644 --- a/mdl/executor/exec_context.go +++ b/mdl/executor/exec_context.go @@ -178,6 +178,10 @@ type ExecContext struct { // "by name", which is every interactive DESCRIBE. describeID model.ID + // catalogForce is REFRESH CATALOG … FORCE: the build describes every + // document again instead of carrying unchanged rows over (#1790). + catalogForce bool + // lastWriteStats is the storage write watermark as of the previous // ReportMutation call (or of this context's construction, i.e. the start of // the statement). Its only use is telling "Modified X" from "X was already diff --git a/modelsdk/mpr/reader.go b/modelsdk/mpr/reader.go index 57fc3e754..b252e23af 100644 --- a/modelsdk/mpr/reader.go +++ b/modelsdk/mpr/reader.go @@ -236,6 +236,55 @@ func (r *Reader) ListUnitHashes() (map[string]string, error) { return result, nil } +// UnitContentHashes returns a hash of every unit's current contents, keyed by +// unit ID: what changed between two calls is the units whose hash differs. On +// MPR v2 it is the Unit table's ContentsHash, which every v2 writer keeps in +// step with the file, so no file is opened; a unit without one is hashed from +// its file. On v1 it is computed from the blob, because mxcli wrote v1 units +// without updating their hash before ako/mxcli#1159, and a stale hash would +// say an edited unit is unchanged. Units held in memory are hashed as held. +func (r *Reader) UnitContentHashes() (map[string]string, error) { + query := `SELECT UnitID, COALESCE(ContentsHash, '') FROM Unit` + if r.version != MPRVersionV2 { + query = `SELECT UnitID, Contents FROM Unit` + } + rows, err := r.db.Query(query) + if err != nil { + return nil, fmt.Errorf("query unit hashes: %w", err) + } + defer rows.Close() + result := make(map[string]string) + var missing []string + for rows.Next() { + var blob, value []byte + if err := rows.Scan(&blob, &value); err != nil { + return nil, fmt.Errorf("scan unit hash: %w", err) + } + id := blobToUUID(blob) + switch held, ok := r.overlaid(id); { + case ok: + result[id] = contentsHash(held) + case r.version != MPRVersionV2: + result[id] = contentsHash(value) + case len(value) == 0: + missing = append(missing, id) + default: + result[id] = string(value) + } + } + if err := rows.Err(); err != nil { + return nil, err + } + for _, id := range missing { + contents, err := r.readMprContents(id) + if err != nil { + continue + } + result[id] = contentsHash(contents) + } + return result, nil +} + // Close closes the reader and releases resources. func (r *Reader) Close() error { r.v2TypesMu.Lock() diff --git a/modelsdk/mpr/reader_unit_content_hashes_test.go b/modelsdk/mpr/reader_unit_content_hashes_test.go new file mode 100644 index 000000000..0f722c52e --- /dev/null +++ b/modelsdk/mpr/reader_unit_content_hashes_test.go @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import "testing" + +// ako/mxcli#1790: the incremental source index decides what changed from +// UnitContentHashes, so a hash must follow the contents. On MPR v1 the stored +// ContentsHash cannot be trusted — mxcli wrote v1 units without updating it +// before ako/mxcli#1159 — and the hash is computed from the blob. +func TestUnitContentHashes_FollowTheContents(t *testing.T) { + t.Run("v1", func(t *testing.T) { + f := newTypedListingFixture(t, []string{"Microflows$Microflow", "Forms$Page"}) + id := v1UnitID(1) + // A stale stored hash, as an older mxcli left it. + if _, err := f.db.Exec(`UPDATE Unit SET ContentsHash = 'stale' WHERE UnitID = ?`, uuidToBlob(id)); err != nil { + t.Fatal(err) + } + before, err := f.r.UnitContentHashes() + if err != nil { + t.Fatal(err) + } + if len(before) != 2 { + t.Fatalf("hashed %d units, want 2", len(before)) + } + key := blobToUUID(uuidToBlob(id)) + if before[key] == "stale" { + t.Fatalf("the v1 hash is the stored column, not the contents") + } + if _, err := f.db.Exec(`UPDATE Unit SET Contents = ? WHERE UnitID = ?`, + v1UnitContents(t, 1, "Microflows$Microflow", 100), uuidToBlob(id)); err != nil { + t.Fatal(err) + } + after, err := f.r.UnitContentHashes() + if err != nil { + t.Fatal(err) + } + if after[key] == before[key] { + t.Errorf("an edited v1 unit kept its hash %q", after[key]) + } + }) + t.Run("v2", func(t *testing.T) { + p := newV2Project(t) + id := p.add(t, 1, "Microflows$Microflow") + p.add(t, 2, "Forms$Page") + r := p.reader() + before, err := r.UnitContentHashes() + if err != nil { + t.Fatal(err) + } + if len(before) != 2 { + t.Fatalf("hashed %d units, want 2", len(before)) + } + if reads := r.UnitFileReads(); reads != 0 { + t.Errorf("hashing a v2 project opened %d unit files; the Unit table has the hashes", reads) + } + key := blobToUUID(uuidToBlob(id)) + p.rewrite(t, id, typedDoc(t, "Microflows$Microflow", "Renamed")) + after, err := r.UnitContentHashes() + if err != nil { + t.Fatal(err) + } + if after[key] == before[key] { + t.Errorf("a rewritten v2 unit kept its hash %q", after[key]) + } + }) +}