MemberJunction / MemberJunction/MJ

next is red: two un-allowlisted global-provider references fail MultiProviderCompliance

Open
#4,284 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
2d 1h
Merged PRs (30d)
323

Description

**Found by** the PR gauntlet on #4275 (`fix/4260-new-user-roles-default`), 2026-09-07. Not caused by that PR: both offending lines are byte-identical to `f2ec156daf31670b42d7704e994686b09daf14df` (`origin/next`) and neither file is in that PR's diff.

### Where
`packages/CodeGenLib/src/Database/sql_codegen.ts:2065` — `const md = new Metadata();`
`packages/MetadataSync/src/services/PushService.ts:974` — `const host = Metadata.Provider as unknown as DatabaseProviderBase;`

### What happens
`@memberjunction/global`'s `MultiProviderCompliance.test.ts` scans `packages/` for `new Metadata()` / `Metadata.Provider` references that are neither routed through a provider nor marked `// global-provider-ok:`. It currently fails:
```
AssertionError: Found 2 non-allowlisted global-provider reference(s):
CodeGenLib/src/Database/sql_codegen.ts:2065: const md = new Metadata();
MetadataSync/src/services/PushService.ts:974: const host = Metadata.Provider as unknown as DatabaseProviderBase;
```
Both lines arrived on `next` in `8d880cc6fe` ("feat(geo,sync): writable-geo write path, isolated parallel providers, skipGeoCoding").

### Also failing from the same commit
\`@memberjunction/codegen-lib\` fails \`BaseViewEmissionGoldenMaster.test.ts\` GM-VIEW-10 and GM-VIEW-10b — the **geo virtual columns** block, whose emitting code \`8d880cc6fe\` rewrote (+51/−14 in \`sql_codegen.ts\`) — plus 6 \`evaluateAndHoldDriftRow\` cases in the materialized-result drift suite. Sibling cases in the same files pass, so these are shape assertions that the geo change moved without updating. Fix them in the same pass.

### Why it matters
Two things, and the second is arguably the bigger one.

First, the rule this test enforces: in multi-provider scenarios — parallel client connections, transaction-isolated server requests — a global-provider reference silently uses the *wrong* provider. `PushService` is a sync path and `sql_codegen` runs under CodeGen; both are places where "which provider" is a real question. Each site needs either a fix or an explicit `// global-provider-ok:` reason.

Second, **CI did not catch this.** `origin/next` HEAD is failing `Run unit tests`, `Unit tests (shard 4/6)` and `Source guards` — yet in the shard-4/6 log this specific assertion prints `✓ … 32 passed`. A local run of the same test on the same tree fails. The most plausible explanation is a turbo cache replay of a result computed before `8d880cc6fe` landed, i.e. the cache key for this task does not include the sources the test scans. If so, this gate is currently blind for every future violation too.

### Repro
```bash
git checkout next && git pull
cd ~/Projects/mj-dev && pnpm --filter @memberjunction/global run test
# Observed: MultiProviderCompliance.test.ts fails with the two references above.
```

### Evidence
```bash
gh api repos/MemberJunction/MJ/commits/f2ec156daf31670b42d7704e994686b09daf14df/check-runs \
--jq '.check_runs[] | select(.conclusion=="failure") | .name'
# => Run unit tests / Unit tests (shard 4/6) / Source guards
```

### Suggested fix
Two separate changes; do both.
1. At each call site, either take the provider explicitly (`this.ProviderToUse` inside a class, or a `provider ?? new Metadata()` parameter in a helper — the `??` form is recognised by the scanner) or append `// global-provider-ok: ` where the global really is correct. `sql_codegen` is codegen-time and may well be legitimately global; `PushService` is a sync path and probably is not.
2. Investigate the cache replay: make the scanned sources part of `@memberjunction/global`'s test task inputs in `turbo.json`, so the task cannot report a stale pass after an unrelated package changes.

### Definition of done
- [ ] A failing test that reproduces it, then green — for part 1 this is the existing `MultiProviderCompliance` test going green honestly
- [ ] `origin/next` is green on `Unit tests (shard 4/6)`, `Run unit tests` and `Source guards`
- [ ] Part 2 demonstrated: touching a scanned file invalidates the cache entry for that test task
- [ ] Existing suite and gates green; no changed expectation in an existing test

### Verify by
`pnpm --filter @memberjunction/global run test` passing locally on a fresh `next`, and a CI run on `next` where the shard executes (not replays) the test.

Contributor guide

Open the contributing guide

Research direction

Start by running pnpm --filter @memberjunction/global run test and inspect MultiProviderCompliance.test.ts for the two references in packages/CodeGenLib/src/Database/sql_codegen.ts and packages/MetadataSync/src/services/PushService.ts. Then review BaseViewEmissionGoldenMaster.test.ts, the materialized-result drift cases, and turbo.json for the related failures and cache inputs. Done means the provider checks, geo assertions, drift cases, and CI cache invalidation all pass without changing existing expectations.

Written by the indexing model from the issue text.

Assessment

Tech stack
github-actions, typescript
Domain
build-system, ci-cd, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.