MemberJunction / MemberJunction/MJ
Transaction group: an Update targeting a non-existent row silently becomes a Create
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## What
`TransactionGroupResolver.ExecuteTransactionGroup` discards the boolean from `InnerLoad`:
```ts
// packages/MJServer/src/resolvers/TransactionGroupResolver.ts, Update and Delete branches
await entity.InnerLoad(pkey); // return value discarded (lines 164, 174)
```
When the `Update` names a primary key that does not exist, that load fails and the entity is left looking brand new. The chain, verified in source:
| step | code |
|---|---|
| load fails | `baseEntity.ts` — `if (!data) { LogError(...); return false; }`, and `_everSaved` is only set to `true` further down, after this early return |
| `IsSaved` reflects it | `get IsSaved() { return this._everSaved; }` → `false` |
| provider branches on it | `databaseProviderBase.ts:1470` — `const bNewRecord = !entity.IsSaved;` |
| operation is chosen | `databaseProviderBase.ts:1481` — `entityResult.Type = bNewRecord ? 'create' : 'update';` |
So `SetMany(itemValues)` is applied to an empty entity and `Save()` **inserts** a new row instead of updating the intended one. The caller asked for an Update, got a Create, and is told it succeeded.
There is a `LogError` on the failed load, so the event is not invisible in the server log — but nothing functional sees it, and the resolver carries on.
## Why it is its own issue rather than part of #4356
#4356 fixes the adjacent defect at the same lines — `Save()`/`Delete()` returning `false` being discarded — and deliberately left this one alone. Correctly so, for two reasons its author gives:
1. **Different failure mode.** #4356's is a false success report where nothing was written. This one *writes a row that should not exist*, which needs its own reproduction and test.
2. **Fixing it is a behaviour change with a blast radius.** Any caller relying on upsert-via-Update — deliberately or accidentally — starts failing. That needs deciding on purpose, not as a side effect.
## Suggested fix
Capture the boolean and refuse the item, in the same shape #4356 established for refusals:
```ts
if (!await entity.InnerLoad(pkey)) {
refusals.push(/* index, operation, entity, "record not found" */);
break;
}
```
That reuses the refusal path #4356 adds, so the group fails whole and the operator is told which row could not be loaded — rather than discovering an unexpected insert later.
Worth checking the same pattern in the `Delete` branch (line 174) while there: a Delete against a missing row currently proceeds against an unloaded entity too.
## Before fixing
Search for callers that depend on the current upsert behaviour. If any exist, they need an explicit Create or an explicit opt-in rather than relying on a failed load.
Found during review of #4356, where it is recorded under "Still open".
Contributor guide
Research direction
Start in packages/MJServer/src/resolvers/TransactionGroupResolver.ts at the Update and Delete branches, then inspect baseEntity.ts and databaseProviderBase.ts at the cited load/save logic. Search callers for intentional upsert-via-Update and reproduce missing-key Update/Delete behavior; done means the chosen behavior is explicit, uses the established refusal path, and is covered by a regression test.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- backend, database
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 64/100