MemberJunction / MemberJunction/MJ

Transaction group: an Update targeting a non-existent row silently becomes a Create

Open
#4,446 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.