MemberJunction / MemberJunction/MJ
Audit the first-pk-ok annotations: at least two document a defect as intentional design
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## Summary
#4304 added **166 `// first-pk-ok:` annotations across 73 files**, enforced by `packages/MJCore/src/__tests__/PrimaryKeyCompliance.test.ts`. The gate checks that an annotation *exists*, not that its stated reason is true.
At least two of them document a **defect as intentional design**. Both concern polymorphic `*RecordID` columns, and both assert that such a column "stores one key value" — which is the specific claim #4321's audit disproves.
## Case 1 — proven wrong, with tests
`SQLServerDataProvider.BuildSoftLinkDependencySQL` (and its PostgreSQL twin):
```js
const quotes = entity.FirstPrimaryKey.NeedsQuotes ? "'" : ''; // first-pk-ok: a soft-link column stores one bare key value; matched against the first key value below by design
```
Both clauses are false, and #4321 demonstrates it with mutation-checked tests:
- A soft-link `RecordID` column stores the **field-prefixed** form (`ID|`), not a bare value — #4321's defect **B3**.
- The quoting derives from the **holder's** primary key type and is then applied to both the discriminator and the payload literal, emitting malformed SQL for an integer-keyed holder — defect **B4**.
So "by design" describes two bugs. This particular line disappears when #4321 merges, since that PR deletes it — no fix needed here for this instance.
## Case 2 — same shape, still live
`packages/MJCoreEntities/src/custom/ResourcePermissions/ResourcePermissionEngine.ts:197`:
```js
PrimaryKeyFieldName: entity.FirstPrimaryKey.Name // first-pk-ok: ResourcePermission.ResourceRecordID stores one key value, so resource entities are single-column by contract
```
`ResourceRecordID` is a polymorphic payload column — it has a sibling discriminator `ResourceTypeID`, used together in filters like `ResourceTypeID='…' AND ResourceRecordID='…'`. And it is written in **at least three different encodings**:
| Site | What it writes |
|---|---|
| `shared/src/lib/base-resource-component.ts:425` | `PrimaryKey.ToURLSegment()` — the **prefixed** form |
| `conversations/.../share-modal.component.ts:178` | `this.conversation.ID` — a **bare** value |
| `explorer-core/.../dashboard-resource.component.ts:660` | compares against `'DataExplorer'` — **not a record id at all** |
So "stores one key value … single-column by contract" is not a contract; it is one of several things that column holds. This is precisely the estate #4321's audit characterised: **90 write sites to polymorphic payload columns, 2 canonical, five distinct encodings.**
## Why this is worth a sweep rather than two spot fixes
The annotations were evidently written by reading each call site and inferring intent. That is exactly the process that mistakes a long-standing bug for a decision — and the annotation format then makes the claim look authoritative and gate-approved, so the next reader trusts it rather than re-deriving it. The gate cannot help: it verifies presence, not truth.
**63 of the 166** annotations make an encoding, quoting, or "by design" claim (as opposed to a structural one like "this is an FK target, which is single-column"). Those are the ones that can conceal a defect; the purely structural ones are far more likely to be sound.
## Suggested scope
Audit the 63 encoding/quoting-claim annotations, not all 166. For each, check the claim against what the column actually holds rather than against what the surrounding code assumes. The polymorphic `*RecordID` columns are the highest-yield subset — #4321's audit already enumerated their real write sites.
Consider also whether an annotation asserting an *encoding* belongs in this mechanism at all. The gate exists to stop `FirstPrimaryKey` being used where a composite key is possible; claims about what a column stores are a different kind of assertion and are not verified by anything.
cc @AN-BC (author of #4304), @MS-BC (author of #4321, whose audit supplies the evidence)
Contributor guide
Research direction
Start with packages/MJCore/src/__tests__/PrimaryKeyCompliance.test.ts and the 63 encoding or quoting annotations, prioritizing ResourcePermissionEngine.ts:197 and the listed write sites. Compare each annotation with the values its column actually stores and identify claims that are unsupported or describe defects as intentional. Done means the targeted annotations have been audited and the issue of whether encoding claims belong in this gate has a documented resolution.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, postgresql, sql, typescript
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100