MemberJunction / MemberJunction/MJ

CodeGen scope model: cleanup paths treat "out of scope" as "deleted" (low priority, filed after the v5.50.0 includeSchemas review)

Open
#3,384 0 comments 0 reactions 0 assignees View on GitHub
bug priority: low
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
2d 1h
Merged PRs (30d)
323

Description

> **Updated 2026-08-05.** Re-verified against `origin/next` @ `3431f9bd72`: **all seven findings are still live and unchanged.** `sql_codegen.ts` and `manage-metadata.ts` have moved since the original audit (layered base views, #2570 smart-field-identification) but none of the audited behaviours changed; `manage-metadata.ts` line refs shift by ~9 and are corrected below. Two open PRs bear on the reachability argument — see the new note under *Why most of this is unreachable today* and the revised ordering note — and the "data-driven scoping proposal" this issue referred to in the abstract is now concrete as #3499.

## Summary

CodeGen decides what is "in scope" for a run, and several of its cleanup paths treat "out of scope" as "deleted." With `includeSchemas` now shipped in v5.50.0 (#3350), narrowing scope in the core repo is a supported configuration for the first time — and it reaches those paths.

**Priority: low, and deliberately filed that way.** This is not currently breaking anyone and there is no rush. It needs someone to deliberately narrow scope in the core repo, which only became possible in v5.50.0 and which nobody is doing yet. Everything it does is recoverable from git. It is filed because the option is newly documented, the destruction is silent while it happens, and the fixes are small — not because it is about to strike.

The audit turned up seven findings. **Three are worth doing as one small change set; four are real but not worth acting on now**, and each of those says why.

---

## Why most of this is unreachable today

Reachability depends entirely on how CodeGen is configured in the two places it runs, so it's worth stating first.

| | Core MJ repo | An installed app |
|---|---|---|
| Output directories | Core **and** non-core — MJServer resolvers, MJCoreEntities subclasses, Explorer entity forms | Only its own packages |
| `excludeSchemas` | `sys`, `staging` | `sys`, `staging`, `dbo`, **`__mj`** (+ other apps, when several are present) |
| Core output directories configured? | **Yes** | **No** |

This explains something that would otherwise look like a live bug: every app excludes the core schema, and that has always been safe — because an app configures no core output directories, the core generation passes simply never run for it. The core repo is the mirror image: it owns the core output directories and does not exclude core.

**So narrowing scope is routine and harmless in an app, and consequential only in the core repo.**

A guard along the lines of "error if core is excluded while core output directories are configured" was considered and **rejected** for exactly this reason — it would fire on every legitimate app run.

**Update 2026-08-05 — the "only a human edits scope config" premise has a pending exception.** #3487 (open) makes the Open App installer write **both** scope lists on every `mj app install` / `mj app upgrade`: it removes an app's schema from `excludeSchemas` on the default path, adds it on the `selfManagedMetadata` path, and correspondingly adds to / removes from `includeSchemas`. That does not create a new destructive case on its own — the default path *widens* scope, and its `AddIncludeSchema` deliberately never creates the key and never writes into an empty list, which is the right guard. What it changes is that the **contents** of both lists start moving without anyone editing config, and entries can be left behind: #3487's removal path does not call `RemoveIncludeSchema`, so a removed app's schema stays in the positive scope and, once the schema is dropped, becomes an entry that matches nothing. A single stale entry is harmless while other entries still match. **The relevance to this issue is item 2** — the state it describes (non-empty list matching nothing) can now arise from ordinary install/remove cycles rather than only from a typo, which raises the value of the validation fix even though it does not yet raise item 1's severity.

---

## Root cause: generation and cleanup share one entity list

CodeGen asks two different questions of the same list.

- **Generation** — "which entities should I emit?" Correctly answered by the **narrowed** set. You don't want to emit another app's entities.
- **Cleanup** — "which existing files no longer correspond to anything?" This has to be answered against **everything known**. An entity that exists but is out of scope isn't obsolete; it just isn't ours to regenerate right now.

Each destructive path picks one of those lists, and the choice looks accidental rather than deliberate — because the two live examples point in **opposite** directions:

- `cleanupOrphanedEntityDirectories` (`Angular/angular-codegen.ts:121,170`) builds its "still valid" set from the **narrowed** list, so anything out of scope is treated as deleted and its form directory is removed recursively. *Deletes too much.*
- `deleteGeneratedEntityFiles` (`Database/sql_codegen.ts:130,149`) deletes using the **unfiltered** baseline, while generation then runs on `includedEntities` (`:134`) — so an out-of-scope entity's view/stored-procedure source file is deleted and never rewritten. *Deletes source it won't rewrite.*

Opposite directions on the same decision is the evidence that neither was chosen. The generated GraphQL resolver file and the entity-subclasses file compound it: both are whole-file rewrites from the narrowed list, so out-of-scope entities are erased from them rather than left alone.

**The fix is one principle, not five patches: out of scope should mean *untouched*.**

---

## Worth fixing

### 1. Cleanup uses the wrong entity list

Set `includeSchemas` in the core repo and leave the core schema off the list (or misspell an entry — see #2), and a single run empties the generated resolver file and the entity-subclasses file and deletes every Explorer entity-form directory.

Measured on a live SQL Server database with three apps set up in it, by setting `includeSchemas` to a single app schema and running CodeGen once:

| artifact | before | after |
|---|---|---|
| `packages/MJServer/src/generated/generated.ts` | 98,184 lines · 383 resolver classes | **21 lines · 0 classes** |
| `packages/MJCoreEntities/src/generated/entity_subclasses.ts` | 117,728 lines · 383 entity classes | **10 lines · 0 classes** |
| Explorer entity-form directories | 383 | **0 — deleted** |

**769 files changed, 5 insertions, 307,039 deletions.** Every CodeGen phase reported success — including "CRUD validation passed (412 entities checked)". The run only exited non-zero at the very end, when the post-codegen build hit the emptied files. So the signal is a compile error in the wreckage, not a diagnosis, and it arrives after everything is already on disk.

The mechanism: the core passes are guarded **only** on the output directory being configured (`runCodeGen.ts:525-535`), never on entity count. `generateGraphQLServerCode([], …)` writes a header plus `export {}` over the target and returns `true`; `generateAllEntitySubClasses` has the same hole.

**In fairness on severity:** someone who narrows scope to one schema and then sees other schemas' generated files disappear is getting a harsh result, but not an inexplicable one, and it is all recoverable from git. This is not a data-loss emergency.

**Suggested fix:** separate the two decisions. Cleanup compares against all known entities rather than the narrowed set; a pass whose narrowed set is empty skips its directory instead of emptying it. No config changes, and no effect on apps — they never invoke the core passes.

### 2. An `includeSchemas` entry that matches nothing silently means "exclude everything"

This is the likelier trigger for #1.

`includeSchemas` resolves by excluding every schema *not* named in it (`Database/schema-scope.ts:72` `applyIncludeSchemaScope`). An absent or empty list short-circuits correctly and is fine. A list that is **non-empty but matches nothing** — a typo, or a value that came from an empty variable — excludes everything, silently, and #1 follows.

**Suggested fix:** error when an entry matches no schema in the database. The resolution step already has the full schema list in hand, so the check is nearly free.

### 3. The post-run CRUD validator ignores scope

Not reachable today, but cheap to fix and it fails hard.

The validator checks that every entity flagged for API inclusion has its create/update/delete routines in the database, but builds that list without applying the exclude list (`runCodeGen.ts:311` — `md.Entities.filter(e => e.IncludeInAPI)`), despite a comment saying it matches the generation baseline (`sql_codegen.ts:134` does apply the filter). On a database where out-of-scope schemas' routines were never created, it fails the whole run with a non-zero exit.

It doesn't bite today because every database we run against is migrated in full before any narrowed run happens. It is a one-line change to apply the same filter generation uses, and because the failure mode is a hard stop rather than a warning, it is cheap insurance in the same slate.

---

## Real, but we are not proposing work on them

Each is a genuine finding. Listed with the case in which it would bite, and why that doesn't justify work now.

4. Generated SQL can reference objects that were never created

Base views and cascade-delete procedures emit schema-qualified references to other schemas' objects with no scope check. **The case:** a database built from scratch with scope already narrowed, so the referenced views and procedures were never created — the generated SQL then fails at execution time. **Why skip:** every environment we have builds the full database first, so the referenced objects always exist. Fixing it properly means making SQL emission scope-aware, which is a real design change with real risk — not proportionate to a case nobody currently produces.

5. Relationship rows are written onto out-of-scope parent entities

`manageOneToManyEntityRelationships` (`Database/manage-metadata.ts:2730`) filters on the child entity's schema (`EntityID`) but not the parent's (`RelatedEntityID`). So an app entity with a foreign key into `__mj` writes a relationship row onto the **core** entity even with `__mj` excluded — and this happens on every app run today.

Worth recording because it is the **metadata-level origin of the reverse-relationship problem #3350 just fixed**: those rows are how a base app's generated types acquired references to their dependents. #3350 gates that at emit time, which is the right layer and is sufficient. **Why skip:** the rows describe a foreign key that genuinely exists, nothing currently misbehaves because of them, and filtering both sides changes what metadata gets written — medium risk for no present benefit. Filed mainly so nobody fixes this a second time at the wrong layer.

6. A dead cleanup function that would delete relationships by scope

`cleanupStaleEntityRelationships` (`Database/manage-metadata.ts:2950`) builds its "valid" set from scope-filtered data but its delete-candidate set from an unfiltered query, which would delete every relationship whose child entity is out of scope. **The case:** someone wires it up — it currently has **no callers**, and a comment at `:2811` incorrectly claims it is already called. **Why skip:** it does nothing today. If anyone touches it, filter both sides first; deleting the function outright is also reasonable.

7. A scope parameter that is accepted and never used

`checkAndRemoveMetadataForDeletedTables` (`Database/manage-metadata.ts:3014`, called from `:1398`) takes the exclude list and never reads it. Deletion is driven by whether the physical table still exists, which is the correct behaviour — exclusion has never protected metadata from pruning, by design. **The case:** someone reads the unused parameter as an oversight and "fixes" it, inverting the rule from *the table is gone* to *this schema is out of scope*, which would mass-delete metadata. **Why skip:** nothing is broken. Worth a comment, or removing the parameter, next time the file is open. **Would appreciate confirmation from a maintainer that the unused parameter is intentional.**

---

## Also worth knowing: "excluded" does not mean "untouched"

Not bugs, but they surprise people. Excluded entities still get their permissions SQL generated and executed (`sql_codegen.ts:258-273`). Some column-maintenance passes run `ALTER TABLE` across every schema that isn't excluded. Exclusion means "don't generate code for this," not "don't touch this."

---

## Recommendation

Do items **1, 2, and 3** as one patch-level change set. They share a theme, they're all small, and each fails in the safe direction — one prevents a write, one prevents a delete, one prevents a spurious hard failure. Items 4–7 belong in the record but not in a sprint.

**On ordering.** Item 1's priority is set by *who pulls the trigger*. While narrowing scope is a deliberate config edit, this is "worth doing." If scope ever starts narrowing automatically — which is what any data-driven scoping model does, e.g. auto-excluding schemas that belong to installed apps — it becomes "do this first," because the safety then rests on nobody sharing an output directory rather than on the cleanup being correct. Not an objection to that direction; ordering only.

**Update 2026-08-05 — that model is now a concrete proposal: #3499**, which derives scope from `__mj.OpenApp` and classifies schemas as mine / not-mine / unowned. It cites this issue and sequences items 1–3 ahead of itself, which is the ordering recommended above. Two notes for whoever picks this up: (a) treat that sequencing as **blocking** rather than advisory, since #3499's whole point is that scope stops being a deliberate edit; and (b) **item 4 needs re-testing under it** — item 4 was set aside specifically because every environment builds the full database before any narrowed run, and install-time scope narrowing is exactly the thing that could stop that being true.

The larger design questions — tying each output directory to the schemas it owns, and making generation for schemas outside the current package an explicit opt-in rather than a default — are genuine improvements and out of scope for a patch. Both make the scope boundary sharper, which is good, and both narrow scope automatically, which is why item 1 comes first.

---

## Verification

Originally verified against `origin/next` with #3350 merged (`ae5e8e9ea0`), released as v5.50.0. **Re-verified 2026-08-05 against `origin/next` @ `3431f9bd72` — all seven findings still present and unchanged.** `sql_codegen.ts` and `manage-metadata.ts` have since been touched by the layered-base-views work and #2570, but none of the audited behaviours changed; `angular-codegen.ts`, `schema-scope.ts` and `runCodeGen.ts` are untouched, so their line refs are unchanged. `manage-metadata.ts` refs below are corrected for the ~9-line shift.

| Item | Location |
|---|---|
| 1 — Angular cleanup uses narrowed list | `Angular/angular-codegen.ts:121,170` |
| 1 — SQL cleanup uses unfiltered list | `Database/sql_codegen.ts:130,149` (delete) vs `:134` (generate) |
| 1 — whole-file rewrites | `Misc/entity_subclasses_codegen.ts`, `Misc/graphql_server_codegen.ts` — an empty list produces a header plus `export {}` and returns success |
| 1 — core passes gated only on output dir | `runCodeGen.ts:525-535` |
| 2 — include resolution | `Database/schema-scope.ts:72` |
| 3 — validator baseline | `runCodeGen.ts:311` |
| 5 — child-only relationship filter | `Database/manage-metadata.ts:2730` |
| 6 — dead function | `Database/manage-metadata.ts:2950` (no call sites; false comment at `:2811`) |
| 7 — unused parameter | `Database/manage-metadata.ts:3014` |

The measurement in item 1 was taken on a live SQL Server database built from #3350's branch with three apps set up in it, by setting `includeSchemas` to a single app schema in the core repo configuration and running CodeGen once. Configuration evidence for the reachability table came from an app's `mj.config.cjs` (excludes `__mj`; no core output directories) and the core repo's `mj.config.cjs` (core output directories present; excludes `sys`, `staging`).

Contributor guide

Open the contributing guide

Research direction

Start with Database/schema-scope.ts and runCodeGen.ts, then trace cleanup in Angular/angular-codegen.ts and Database/sql_codegen.ts. Compare narrowed and unfiltered entity lists, and verify that out-of-scope artifacts remain untouched, unmatched includeSchemas entries fail clearly, and CRUD validation respects scope.

Written by the indexing model from the issue text.

Assessment

Tech stack
sql, typescript
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.