diff --git a/.changeset/26133-import-duplicate-match-key.md b/.changeset/26133-import-duplicate-match-key.md new file mode 100644 index 00000000000..3072282daf5 --- /dev/null +++ b/.changeset/26133-import-duplicate-match-key.md @@ -0,0 +1,21 @@ +--- +'@objectstack/core': patch +'@objectstack/spec': patch +--- + +fix(core)!: an `update` / `upsert` import refuses its own rows that share a match key, before any write + +Clause-②: no (narrowing: rows of one `update` / `upsert` import that carry the same coerced `matchFields` key, which used to be written one by one, now all fail with `DUPLICATE_MATCH_KEY` and none of them is written) +Metadata-protocol: none + + + +**BREAKING** for an import that carries the same match key on two or more of its own rows. Before, an `upsert` wrote every one of them: under a key no stored record carried yet each row was created, so the object held two records for one business key and every later import of that key failed with `AMBIGUOUS_MATCH` until someone deleted the extra record by hand; under a key a stored record carried, each row updated that record in turn and the last row silently won. An `update` import did the same silent double update. + +- **The refusal.** Before any row is written, the import runner (`runImport`, behind `POST /data/:object/import`, the async import job, the connector pull and the user import) groups the rows by their match key. Every row of a key carried more than once fails with `ok: false`, `action: 'failed'`, `code: 'DUPLICATE_MATCH_KEY'`, and its `error` names every row that carries the key and the key set (`Rows 1, 2 of this import have the same match key (supplier + cert_type + cert_no), so none of them was written: keep one row per key and import again`). None of them is written. The other rows of the file import as before. +- **What counts as one key.** The key set the row is matched by (the first set of `matchFields` whose fields are all non-blank on it) with its cell values as coerced, the values the matcher queries with. So `ISO 14001` (an option label) and `ISO14001` (its value) are one key, and so are two spellings of one lookup target. A row blank on every key set, or whose key cells fail coercion, has no key and answers as before. +- **The dry run answers the same.** A `dryRun` preview reports the same `DUPLICATE_MATCH_KEY` rows the import would. +- **Every object.** An object with a unique index over the key answers the same refusal: before, its import created the first row and failed the second with `UNIQUE_VIOLATION`, while its dry run reported both as created. +- **Unchanged.** A file without repeated keys answers exactly as before, and an `insert` import, which matches nothing, still writes every row. + +**Who moves.** A caller that imports a file repeating a match key now gets those rows back as failures: keep one row per key (the one that is right) and import again. `@objectstack/spec` adds `DUPLICATE_MATCH_KEY` to the error-code ledger and to the `matchFields` contract text. diff --git a/content/docs/data-modeling/import-mappings.mdx b/content/docs/data-modeling/import-mappings.mdx index dcd14ee881f..0a1ac35ce17 100644 --- a/content/docs/data-modeling/import-mappings.mdx +++ b/content/docs/data-modeling/import-mappings.mdx @@ -278,6 +278,7 @@ fails does **not** stop the import. | `skipped` | `true` | `BLANK_MATCH_KEY` | One key set in `matchFields`, and a field of it is blank on the row. Upsert creates such a row by default; `update` skips it, and `skipBlankMatchKey: true` skips it in either mode. | | `failed` | `false` | `BLANK_MATCH_KEY` | Ordered key sets in `matchFields`, and every set has a blank field on the row. Nothing is written for that row. | | `failed` | `false` | `AMBIGUOUS_MATCH` | The match fields matched **more than one** record. Nothing is written for that row. | +| `failed` | `false` | `DUPLICATE_MATCH_KEY` | `update` or `upsert`, and another row of the same import has the same match key, compared after cell coercion (`ISO 14001` and `ISO14001` coerce to one option value). Every row of that key fails, `error` names them all, and none of them is written, on a dry run too: keep one row per key and import again. | | `failed` | `false` | a field error code | A cell could not be coerced, or the engine rejected the record. `field` names the column and `error` carries the message. | So "a row that does not match" is not one behaviour but three, chosen by `writeMode` diff --git a/content/docs/references/api/automation-api.mdx b/content/docs/references/api/automation-api.mdx index 7a8d5fa41e9..7934cf82a5b 100644 --- a/content/docs/references/api/automation-api.mdx +++ b/content/docs/references/api/automation-api.mdx @@ -354,7 +354,7 @@ const result = AutomationApiErrorCode.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +344 more>` | ✅ | The inner refusal's code in its WIRE spelling - the ADR-0112 member a record save of the same write answers in `error.code` (e.g. `UNIQUE_VIOLATION`); the closed set `ApiErrorSchema.code` parses against | +| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +345 more>` | ✅ | The inner refusal's code in its WIRE spelling - the ADR-0112 member a record save of the same write answers in `error.code` (e.g. `UNIQUE_VIOLATION`); the closed set `ApiErrorSchema.code` parses against | | **message** | `string` | ✅ | The inner refusal's own diagnostic text, verbatim, without the node and operation prefixes the run's failure text adds | ### Allowed Values: `FlowInnerRefusal.code` @@ -471,6 +471,7 @@ const result = AutomationApiErrorCode.parse(data); * `DRIVER_UNAVAILABLE` * `DUPLICATE_ARTIFACT_OBJECT_NAME` * `DUPLICATE_ARTIFACT_PACKAGE` +* `DUPLICATE_MATCH_KEY` * `DUPLICATE_REQUEST` * `DUPLICATE_SOURCE_NOT_A_BASE` * `ELIGIBILITY_UNEVALUABLE` diff --git a/content/docs/references/api/contract.mdx b/content/docs/references/api/contract.mdx index c1d6f119dfd..cfbc051a1f3 100644 --- a/content/docs/references/api/contract.mdx +++ b/content/docs/references/api/contract.mdx @@ -28,7 +28,7 @@ const result = ApiErrorSchema.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +344 more>` | ✅ | Error code (e.g. VALIDATION_ERROR; StandardErrorCode ∪ the ledger the serving side registers — ERROR_CODE_LEDGER for framework packages) | +| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +345 more>` | ✅ | Error code (e.g. VALIDATION_ERROR; StandardErrorCode ∪ the ledger the serving side registers — ERROR_CODE_LEDGER for framework packages) | | **declaredCode** | `string` | optional | The producer-declared code, verbatim, when it is not a member of the closed `code` vocabulary — the open, author-authored channel (app-specific spellings; ADR-0112) | | **message** | `string` | ✅ | Readable error message | | **userMessage** | `string` | optional | Producer-marked user-facing refusal text, verbatim. Present exactly when the producer opted in at throw time; consumers render it to end users and keep their generic substitution for anything unmarked. Status-agnostic; never replaces `message`. | @@ -152,6 +152,7 @@ const result = ApiErrorSchema.parse(data); * `DRIVER_UNAVAILABLE` * `DUPLICATE_ARTIFACT_OBJECT_NAME` * `DUPLICATE_ARTIFACT_PACKAGE` +* `DUPLICATE_MATCH_KEY` * `DUPLICATE_REQUEST` * `DUPLICATE_SOURCE_NOT_A_BASE` * `ELIGIBILITY_UNEVALUABLE` diff --git a/content/docs/references/api/error-code-ledger.mdx b/content/docs/references/api/error-code-ledger.mdx index 806298c7c96..21114ebd47d 100644 --- a/content/docs/references/api/error-code-ledger.mdx +++ b/content/docs/references/api/error-code-ledger.mdx @@ -319,6 +319,7 @@ const result = ErrorCode.parse(data); * `DRIVER_UNAVAILABLE` * `DUPLICATE_ARTIFACT_OBJECT_NAME` * `DUPLICATE_ARTIFACT_PACKAGE` +* `DUPLICATE_MATCH_KEY` * `DUPLICATE_REQUEST` * `DUPLICATE_SOURCE_NOT_A_BASE` * `ELIGIBILITY_UNEVALUABLE` diff --git a/content/docs/references/api/export.mdx b/content/docs/references/api/export.mdx index f3dcc205120..76916827fd5 100644 --- a/content/docs/references/api/export.mdx +++ b/content/docs/references/api/export.mdx @@ -53,7 +53,7 @@ const result = CreateImportJobRequestSchema.parse(data); | **mappingName** | `string` | optional | Name of a registered `mapping` metadata artifact to apply; the server resolves it (org-scoped rows first, then env-wide) and projects columns through it. Mutually exclusive with an inline `mapping` — supplying both is refused (400 CONFLICTING_MAPPING). | | **dryRun** | `boolean` | optional (default: `false`) | Validate + coerce every row without persisting. The verdict is the engine's own write-path validation, with one boundary an author should know: a preview runs NO automations. Hooks never fire in a dry run — a preview that executed user-authored side effects (mail, outbound calls, writes to other objects) would be the retired `validateOnly` defect in a new spelling. So a dry run with `runAutomations: true` can report `required` for a field a `beforeInsert` hook would populate during the real import; for hook-derived fields the real write is authoritative. | | **writeMode** | `Enum<'insert' \| 'update' \| 'upsert'>` | optional (default: `"insert"`) | insert / update / upsert semantics | -| **matchFields** | `string[] \| string[][]` | optional | Fields that identify an existing record (required for update/upsert). Either one key set (`['email']`), or an ordered list of key sets (`[['credit_code'], ['country', 'registration_no']]`) for a sheet whose rows are identified in different ways: each row is matched by the first set whose fields are all non-blank on that row, and only by that set. A row blank on every set fails with `BLANK_MATCH_KEY` instead of being created or skipped, so `skipBlankMatchKey` cannot be combined with key sets. A set that matches more than one record fails the row with `AMBIGUOUS_MATCH`, as one key set does. | +| **matchFields** | `string[] \| string[][]` | optional | Fields that identify an existing record (required for update/upsert). Either one key set (`['email']`), or an ordered list of key sets (`[['credit_code'], ['country', 'registration_no']]`) for a sheet whose rows are identified in different ways: each row is matched by the first set whose fields are all non-blank on that row, and only by that set. A row blank on every set fails with `BLANK_MATCH_KEY` instead of being created or skipped, so `skipBlankMatchKey` cannot be combined with key sets. A set that matches more than one record fails the row with `AMBIGUOUS_MATCH`, as one key set does. Rows of one import that carry the same key (the key set each row is matched by, with its cell values as coerced, so an option label and its value are one key) all fail with `DUPLICATE_MATCH_KEY`, naming every row that carries it, and none of them is written, on a dry run as on the import: keep one row per key. | | **runAutomations** | `boolean` | optional (default: `true`) | Fire triggers/hooks for each imported row. ON by default, and opting out must be explicit: automations always ran on import historically (the engine ignored this flag until this flag was honoured), so omitting the key runs them. This matches platform convention (Salesforce fires triggers on import by default). Only the system principal (an in-process caller such as a seed or migration job) may send `runAutomations: false`, because it takes the object's triggers and metadata-bound hooks, its approvals and record freezes among them, off every row: the import doors refuse it from every other caller, every signed-in user and administrator included, with `403 PERMISSION_DENIED` before any row is read. One boundary: a `dryRun` preview runs NO automations whatever this flag says. | | **treatAsHistorical** | `boolean` | optional (default: `false`) | Import as established historical facts. Two effects, both off by default so a normal import is unchanged: (1) skip the state_machine rule so mid-lifecycle rows (e.g. already-closed tickets, closed_won deals) are not rejected by initialStates; and (2) preserve the original audit timeline — keep the supplied created_at / updated_at / updated_by and author-declared business readonly fields (e.g. closed_at, resolved_by) instead of stamping-now / stripping them. Undoing a historical import mirrors (2): the captured pre-import values are restored verbatim rather than re-stamped. Only the system principal (an in-process caller such as a seed or migration job) may send `treatAsHistorical: true`: the import doors refuse it from every other caller, every signed-in user and administrator included, with `403 PERMISSION_DENIED` before any row is read. | | **trimWhitespace** | `boolean` | optional (default: `true`) | Trim leading/trailing whitespace from string cells | @@ -310,7 +310,7 @@ Type: `{ sourceField: string; targetField: string; targetLabel?: string; transfo | **mappingName** | `string` | optional | Name of a registered `mapping` metadata artifact to apply; the server resolves it (org-scoped rows first, then env-wide) and projects columns through it. Mutually exclusive with an inline `mapping` — supplying both is refused (400 CONFLICTING_MAPPING). | | **dryRun** | `boolean` | optional (default: `false`) | Validate + coerce every row without persisting. The verdict is the engine's own write-path validation, with one boundary an author should know: a preview runs NO automations. Hooks never fire in a dry run — a preview that executed user-authored side effects (mail, outbound calls, writes to other objects) would be the retired `validateOnly` defect in a new spelling. So a dry run with `runAutomations: true` can report `required` for a field a `beforeInsert` hook would populate during the real import; for hook-derived fields the real write is authoritative. | | **writeMode** | `Enum<'insert' \| 'update' \| 'upsert'>` | optional (default: `"insert"`) | insert / update / upsert semantics | -| **matchFields** | `string[] \| string[][]` | optional | Fields that identify an existing record (required for update/upsert). Either one key set (`['email']`), or an ordered list of key sets (`[['credit_code'], ['country', 'registration_no']]`) for a sheet whose rows are identified in different ways: each row is matched by the first set whose fields are all non-blank on that row, and only by that set. A row blank on every set fails with `BLANK_MATCH_KEY` instead of being created or skipped, so `skipBlankMatchKey` cannot be combined with key sets. A set that matches more than one record fails the row with `AMBIGUOUS_MATCH`, as one key set does. | +| **matchFields** | `string[] \| string[][]` | optional | Fields that identify an existing record (required for update/upsert). Either one key set (`['email']`), or an ordered list of key sets (`[['credit_code'], ['country', 'registration_no']]`) for a sheet whose rows are identified in different ways: each row is matched by the first set whose fields are all non-blank on that row, and only by that set. A row blank on every set fails with `BLANK_MATCH_KEY` instead of being created or skipped, so `skipBlankMatchKey` cannot be combined with key sets. A set that matches more than one record fails the row with `AMBIGUOUS_MATCH`, as one key set does. Rows of one import that carry the same key (the key set each row is matched by, with its cell values as coerced, so an option label and its value are one key) all fail with `DUPLICATE_MATCH_KEY`, naming every row that carries it, and none of them is written, on a dry run as on the import: keep one row per key. | | **runAutomations** | `boolean` | optional (default: `true`) | Fire triggers/hooks for each imported row. ON by default, and opting out must be explicit: automations always ran on import historically (the engine ignored this flag until this flag was honoured), so omitting the key runs them. This matches platform convention (Salesforce fires triggers on import by default). Only the system principal (an in-process caller such as a seed or migration job) may send `runAutomations: false`, because it takes the object's triggers and metadata-bound hooks, its approvals and record freezes among them, off every row: the import doors refuse it from every other caller, every signed-in user and administrator included, with `403 PERMISSION_DENIED` before any row is read. One boundary: a `dryRun` preview runs NO automations whatever this flag says. | | **treatAsHistorical** | `boolean` | optional (default: `false`) | Import as established historical facts. Two effects, both off by default so a normal import is unchanged: (1) skip the state_machine rule so mid-lifecycle rows (e.g. already-closed tickets, closed_won deals) are not rejected by initialStates; and (2) preserve the original audit timeline — keep the supplied created_at / updated_at / updated_by and author-declared business readonly fields (e.g. closed_at, resolved_by) instead of stamping-now / stripping them. Undoing a historical import mirrors (2): the captured pre-import values are restored verbatim rather than re-stamped. Only the system principal (an in-process caller such as a seed or migration job) may send `treatAsHistorical: true`: the import doors refuse it from every other caller, every signed-in user and administrator included, with `403 PERMISSION_DENIED` before any row is read. | | **trimWhitespace** | `boolean` | optional (default: `true`) | Trim leading/trailing whitespace from string cells | diff --git a/packages/core/src/utils/import-runner.ts b/packages/core/src/utils/import-runner.ts index 8b20cb7e077..07a4f8d8c6c 100644 --- a/packages/core/src/utils/import-runner.ts +++ b/packages/core/src/utils/import-runner.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { randomUUID } from 'node:crypto'; -import { coerceRow, type RefResolver, type RefMatch } from './import-coerce.js'; +import { coerceRow, type CoerceContext, type RefResolver, type RefMatch } from './import-coerce.js'; import type { ExportFieldMeta } from './import-field-meta.js'; import type { ValidationMessageTranslator } from '@objectstack/spec/system'; import { @@ -812,13 +812,15 @@ export function runImport(opts: RunImportOptions): Promise { // record, or a sentinel: 'blank' (every key set has an empty field), 'none' // (no match), 'ambiguous' (>1 match — too risky to update); and the key set // the row was matched by. - const findExisting = async ( - data: Record, - ): Promise<{ match: Record | 'blank' | 'none' | 'ambiguous'; keySet: string[] }> => { - const keySet = keySets.find((set) => set.every((f) => { + const keySetOf = (data: Record): string[] | undefined => + keySets.find((set) => set.every((f) => { const v = data[f]; return !(v === undefined || v === null || v === ''); })); + const findExisting = async ( + data: Record, + ): Promise<{ match: Record | 'blank' | 'none' | 'ambiguous'; keySet: string[] }> => { + const keySet = keySetOf(data); if (!keySet) return { match: 'blank', keySet: [] }; const filter: Record = {}; for (const f of keySet) filter[f] = data[f]; @@ -832,6 +834,71 @@ export function runImport(opts: RunImportOptions): Promise { return { match: recs[0], keySet }; }; + // One coercion context for every `coerceRow` call, so the key a row is + // grouped by below is the value its own coercion produces. + const coerceCtx: CoerceContext = { + trimWhitespace, nullValues, createMissingOptions, resolveRef, + // Cell-coercion failures land in the same row report as the engine's + // validation errors, so they speak the same language (#3957). + locale: context?.locale, translate: messageTranslator, + // [#8485] The clock an offset-free datetime cell is read in. Already + // on the resolved context beside `locale` (the localization cascade's + // `ExecutionContext.timezone`) — the SAME value the export renders + // cells in (#8373), which is what makes the round trip an inverse + // instead of a host-`TZ` lottery. Absent ⇒ UTC, as the export writes. + timezone: typeof context?.timezone === 'string' && context.timezone + ? String(context.timezone) : undefined, + }; + + /** + * [#26133] The rows of this import that share a match key, each mapped to + * the row numbers carrying that key and the key set it was read by. Asked + * once, before any row is written: every row of a shared key then fails with + * `DUPLICATE_MATCH_KEY` and none of them is written, on a dry run as on the + * write. Writing one of them would leave the rest to update it (a silent + * merge that hides which spreadsheet row was right) or, for a key no stored + * record carries yet, to create a second record that every later run + * answers `AMBIGUOUS_MATCH` for. Judged the same on every object, a unique + * index over the key included: the duplicate is a fact about the file, and + * the runner keeps no copy of the engine's uniqueness rules. + * + * A key is the key set a row is matched by ({@link keySetOf}) plus the + * values the matcher queries with, i.e. its COERCED key columns: `ISO 14001` + * (an option label) and `ISO14001` (its value) coerce to one option value, + * so they are one key. A row whose key columns fail coercion, or that is + * blank on every key set, has no key here; its own row report answers as + * before. Insert mode matches nothing, so it has no keys. + */ + const rowsSharingKey = async (): Promise> => { + const shared = new Map(); + if (writeMode === 'insert') return shared; + const keyColumns = [...new Set(keySets.flat())]; + const byKey = new Map(); + for (let i = 0; i < rows.length; i++) { + const cells: Record = {}; + for (const f of keyColumns) if (Object.prototype.hasOwnProperty.call(rows[i], f)) cells[f] = rows[i][f]; + let data: Record; + try { + const coerced = await coerceRow(cells, metaMap, coerceCtx); + if (coerced.errors.length > 0) continue; + data = coerced.data; + } catch { continue; /* the row's own pass reports the throw */ } + const keySet = keySetOf(data); + if (keySet) { + const key = JSON.stringify([keySets.indexOf(keySet), keySet.map((f) => data[f])]); + const group = byKey.get(key); + if (group) group.at.push(i); else byKey.set(key, { at: [i], keySet }); + } + if ((i + 1) % progressEvery === 0) await yieldToEventLoop(); + } + for (const { at, keySet } of byKey.values()) { + if (at.length < 2) continue; + const rowNos = at.map((i) => i + 1); + for (const i of at) shared.set(i, { rowNos, keySet }); + } + return shared; + }; + const writeCtx = { ...(context ?? {}), skipAutomations: !runAutomations, @@ -1143,27 +1210,24 @@ export function runImport(opts: RunImportOptions): Promise { }; return (async () => { + const sharingKey = await rowsSharingKey(); for (let i = 0; i < rows.length; i++) { const rowNo = i + 1; try { // 1. Coerce every cell to its storage value (+ resolve lookups). - const { data, errors, keptOptionValues } = await coerceRow(rows[i], metaMap, { - trimWhitespace, nullValues, createMissingOptions, resolveRef, - // Cell-coercion failures land in the same row report as the engine's - // validation errors, so they speak the same language (#3957). - locale: context?.locale, translate: messageTranslator, - // [#8485] The clock an offset-free datetime cell is read in. Already - // on the resolved context beside `locale` (the localization cascade's - // `ExecutionContext.timezone`) — the SAME value the export renders - // cells in (#8373), which is what makes the round trip an inverse - // instead of a host-`TZ` lottery. Absent ⇒ UTC, as the export writes. - timezone: typeof context?.timezone === 'string' && context.timezone - ? String(context.timezone) : undefined, - }); + const { data, errors, keptOptionValues } = await coerceRow(rows[i], metaMap, coerceCtx); + const shared = sharingKey.get(i); if (errors.length > 0) { const first = errors[0]; errCount++; results[i] = { row: rowNo, ok: false, action: 'failed', field: first.field, code: first.code, error: first.message }; + } else if (shared) { + // [#26133] Another row of this import carries the same key. + errCount++; + results[i] = { + row: rowNo, ok: false, action: 'failed', code: 'DUPLICATE_MATCH_KEY', + error: `Rows ${shared.rowNos.join(', ')} of this import have the same match key (${shared.keySet.join(' + ')}), so none of them was written: keep one row per key and import again`, + }; } else { // [#22183] This row's write context: the kept option values ride along. const rowCtx = writeCtxFor([keptOptionValues]); diff --git a/packages/rest/src/import-upsert-in-file-duplicate-key.test.ts b/packages/rest/src/import-upsert-in-file-duplicate-key.test.ts new file mode 100644 index 00000000000..0c83b2ee466 --- /dev/null +++ b/packages/rest/src/import-upsert-in-file-duplicate-key.test.ts @@ -0,0 +1,253 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#26133] One keyed import never writes two of its own rows for one + * `matchFields` key. Rows of the SAME import that carry the same key fail with + * `DUPLICATE_MATCH_KEY`, every one of them, naming every row that carries it, + * and none of them is written: which spreadsheet row was right is the author's + * call, never the runner's. Through the real `POST /data/:object/import` route + * over a real `SqlDriver` (better-sqlite3 `:memory:`). + * + * The card's object: a supplier certificate keyed on `supplier` (a lookup, + * resolved by name), `cert_type` (a select whose label `ISO 14001` and value + * `iso14001` both coerce to the value) and `cert_no`, with no unique index + * over the key. At the base each duplicate row was created, and from then on + * every upsert of that key answered `AMBIGUOUS_MATCH`. + * + * The key is compared AFTER cell coercion, by the key set the row is matched + * by — the value the matcher queries with — so `ISO 14001` and `ISO14001` are + * one key. A file without in-file duplicates answers exactly as before. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { SysMetadataAuditObject, SysMetadataCommitObject, SysMetadataHistoryObject, SysMetadataObject } from '@objectstack/metadata-core'; +import { RestServer } from './rest-server'; + +const SUPPLIER = { + name: 'srm_supplier_26133', label: 'Supplier', systemFields: false, + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const, label: 'Name' }, + }, +}; + +const certificate = (name: string, extra: Record = {}) => ({ + name, label: 'Certificate', systemFields: false, + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + supplier: { name: 'supplier', type: 'lookup' as const, label: 'Supplier', reference: SUPPLIER.name, displayField: 'name' }, + cert_type: { + name: 'cert_type', type: 'select' as const, label: 'Type', + options: [{ label: 'ISO 9001', value: 'iso9001' }, { label: 'ISO 14001', value: 'iso14001' }], + }, + cert_no: { name: 'cert_no', type: 'text' as const, label: 'No.' }, + note: { name: 'note', type: 'text' as const, label: 'Note' }, + }, + ...extra, +}); + +const CERT = certificate('srm_cert_26133'); + +const liveEngines: ObjectQL[] = []; +afterEach(async () => { + while (liveEngines.length) { + try { await liveEngines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +function makeRes() { + const res: any = { + write: () => true, end: () => {}, + header: () => res, + status: (code: number) => { res._status = code; return res; }, + json: (body: any) => { res._json = body; return res; }, + }; + return res; +} + +async function boot(objects: any[] = [SUPPLIER, CERT]) { + const engine = new ObjectQL(); + liveEngines.push(engine); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }), true); + await engine.init(); + for (const o of objects) engine.registry.registerObject(o); + await engine.syncSchemas(); + for (const o of [SysMetadataObject, SysMetadataHistoryObject, SysMetadataAuditObject, SysMetadataCommitObject]) { + if (!engine.registry.getObject(o.name)) engine.registry.registerObject(o as any); + } + const protocol = new ObjectStackProtocolImplementation(engine as any); + const noop = () => {}; + const server = { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; + const rest = new RestServer(server as any, protocol as any, { api: { requireAuth: false } } as any); + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + const route = rest.getRoutes().find((r: any) => r.method === 'POST' && r.path === '/api/v1/data/:object/import') as any; + expect(route).toBeDefined(); + await engine.insert(SUPPLIER.name, { id: 'sup_a', name: 'Acme' }); + await engine.insert(SUPPLIER.name, { id: 'sup_b', name: 'Beta' }); + const importInto = async (object: string, body: Record) => { + const res = makeRes(); + await route.handler({ params: { object }, body } as any, res); + expect(res._status ?? 200).toBe(200); + return res._json; + }; + return { engine, importInto }; +} + +const MAPPING = { Supplier: 'supplier', Type: 'cert_type', No: 'cert_no', Note: 'note' }; +const KEY = ['supplier', 'cert_type', 'cert_no']; +const csvOf = (...lines: string[]) => ['Supplier,Type,No,Note', ...lines].join('\n'); +const upsert = (csv: string, extra: Record = {}) => + ({ format: 'csv', csv, mapping: MAPPING, writeMode: 'upsert', matchFields: KEY, ...extra }); + +const duplicate = (row: number, rows: number[], keySet = 'supplier + cert_type + cert_no') => ({ + row, ok: false, action: 'failed', code: 'DUPLICATE_MATCH_KEY', + error: `Rows ${rows.join(', ')} of this import have the same match key (${keySet}), so none of them was written: keep one row per key and import again`, +}); + +describe('[#26133] an upsert import refuses rows of its own that share a match key', () => { + let ctx: Awaited>; + const certs = async (object = CERT.name) => + (await ctx.engine.find(object, {})) + .map((r: any) => ({ supplier: r.supplier, cert_type: r.cert_type, cert_no: r.cert_no, note: r.note ?? null })) + .sort((a: { note: string | null }, b: { note: string | null }) => String(a.note).localeCompare(String(b.note))); + beforeEach(async () => { ctx = await boot(); }); + + it('the card: two identical rows, and `ISO 14001` beside `ISO14001`, are refused whole; the rest of the file imports', async () => { + const summary = await ctx.importInto(CERT.name, upsert(csvOf( + 'Acme,ISO 9001,A-1,first', + 'Acme,ISO 9001,A-1,second', + 'Acme,ISO 14001,B-1,third', + 'Beta,ISO 9001,A-1,fourth', + 'Acme,ISO14001,B-1,fifth', + ))); + + expect(summary.results).toEqual([ + duplicate(1, [1, 2]), + duplicate(2, [1, 2]), + duplicate(3, [3, 5]), + { row: 4, ok: true, action: 'created', id: expect.any(String) }, + duplicate(5, [3, 5]), + ]); + expect(summary).toMatchObject({ total: 5, ok: 1, errors: 4, created: 1, updated: 0, skipped: 0 }); + expect(await certs()).toEqual([{ supplier: 'sup_b', cert_type: 'iso9001', cert_no: 'A-1', note: 'fourth' }]); + }); + + it('the fixed file imports, and re-running it updates instead of answering `AMBIGUOUS_MATCH`', async () => { + await ctx.importInto(CERT.name, upsert(csvOf('Acme,ISO 14001,B-1,first', 'Acme,ISO14001,B-1,second'))); + expect(await certs()).toEqual([]); + + const fixed = csvOf('Acme,ISO 14001,B-1,kept'); + const first = await ctx.importInto(CERT.name, upsert(fixed)); + expect(first.results).toEqual([{ row: 1, ok: true, action: 'created', id: expect.any(String) }]); + const again = await ctx.importInto(CERT.name, upsert(fixed)); + expect(again.results).toEqual([{ row: 1, ok: true, action: 'updated', id: first.results[0].id }]); + expect(await certs()).toEqual([{ supplier: 'sup_a', cert_type: 'iso14001', cert_no: 'B-1', note: 'kept' }]); + }); + + it('two rows naming a record that already exists are refused too: neither silently wins the update', async () => { + await ctx.engine.insert(CERT.name, { id: 'c1', supplier: 'sup_a', cert_type: 'iso9001', cert_no: 'A-1', note: 'stored' }); + const summary = await ctx.importInto(CERT.name, upsert(csvOf('Acme,ISO 9001,A-1,first', 'Acme,iso9001,A-1,second'))); + + expect(summary.results).toEqual([duplicate(1, [1, 2]), duplicate(2, [1, 2])]); + expect(await certs()).toEqual([{ supplier: 'sup_a', cert_type: 'iso9001', cert_no: 'A-1', note: 'stored' }]); + }); + + it('the dry run answers exactly what the import answers, and persists nothing', async () => { + const csv = csvOf('Acme,ISO 9001,A-1,first', 'Beta,ISO 9001,A-1,second', 'Acme,ISO 9001,A-1,third'); + const dry = await ctx.importInto(CERT.name, upsert(csv, { dryRun: true })); + expect(dry.results).toEqual([ + duplicate(1, [1, 3]), + { row: 2, ok: true, action: 'created' }, + duplicate(3, [1, 3]), + ]); + expect(await certs()).toEqual([]); + + const real = await ctx.importInto(CERT.name, upsert(csv)); + expect(real.results.map(({ id: _id, ...r }: any) => r)).toEqual(dry.results); + }); + + it('a row whose own cell fails coercion keeps its cell error; the row it would have collided with still names both', async () => { + const summary = await ctx.importInto(CERT.name, upsert(csvOf('Acme,ISO 9001,A-1,first', 'Acme,ISO 9001,A-1,second', 'Acme,ISO 27001,A-1,third'))); + expect(summary.results).toEqual([ + duplicate(1, [1, 2]), + duplicate(2, [1, 2]), + expect.objectContaining({ row: 3, ok: false, action: 'failed', field: 'cert_type', code: 'invalid_option' }), + ]); + expect(await certs()).toEqual([]); + }); + + it('update mode refuses them the same way; insert mode, which matches nothing, still writes every row', async () => { + await ctx.engine.insert(CERT.name, { id: 'c1', supplier: 'sup_a', cert_type: 'iso9001', cert_no: 'A-1', note: 'stored' }); + const csv = csvOf('Acme,ISO 9001,A-1,first', 'Acme,ISO 9001,A-1,second'); + + const update = await ctx.importInto(CERT.name, upsert(csv, { writeMode: 'update' })); + expect(update.results).toEqual([duplicate(1, [1, 2]), duplicate(2, [1, 2])]); + expect(await certs()).toEqual([{ supplier: 'sup_a', cert_type: 'iso9001', cert_no: 'A-1', note: 'stored' }]); + + const insert = await ctx.importInto(CERT.name, { format: 'csv', csv, mapping: MAPPING, writeMode: 'insert' }); + expect(insert.results).toEqual([ + { row: 1, ok: true, action: 'created', id: expect.any(String) }, + { row: 2, ok: true, action: 'created', id: expect.any(String) }, + ]); + }); + + it('ordered key sets: a key is the set a row is matched by plus its values, so rows matched by different sets never collide', async () => { + const summary = await ctx.importInto(CERT.name, { + format: 'json', writeMode: 'upsert', matchFields: [['cert_no'], ['supplier', 'note']], + rows: [ + { supplier: 'Acme', cert_type: 'ISO 9001', cert_no: 'A-1', note: 'x' }, + { supplier: 'Acme', cert_type: 'ISO 9001', note: 'A-1' }, + { supplier: 'Beta', cert_type: 'ISO 9001', note: 'y' }, + { supplier: 'Beta', cert_type: 'ISO 14001', note: 'y' }, + ], + }); + expect(summary.results).toEqual([ + { row: 1, ok: true, action: 'created', id: expect.any(String) }, + { row: 2, ok: true, action: 'created', id: expect.any(String) }, + duplicate(3, [3, 4], 'supplier + note'), + duplicate(4, [3, 4], 'supplier + note'), + ]); + }); + + it('a file without in-file duplicates answers as before: created, updated and blank-key rows', async () => { + await ctx.engine.insert(CERT.name, { id: 'c1', supplier: 'sup_a', cert_type: 'iso9001', cert_no: 'A-1', note: 'stored' }); + const summary = await ctx.importInto(CERT.name, upsert(csvOf( + 'Acme,ISO 9001,A-1,updated', + 'Acme,ISO 14001,A-1,new', + 'Acme,ISO 9001,,blank-1', + 'Acme,ISO 9001,,blank-2', + ))); + expect(summary.results).toEqual([ + { row: 1, ok: true, action: 'updated', id: 'c1' }, + { row: 2, ok: true, action: 'created', id: expect.any(String) }, + { row: 3, ok: true, action: 'created', id: expect.any(String) }, + { row: 4, ok: true, action: 'created', id: expect.any(String) }, + ]); + }); +}); + +describe('[#26133] an object with a unique index over the key', () => { + const UNIQUE_CERT = certificate('srm_cert_unique_26133', { + indexes: [{ name: 'uk_cert_26133', fields: KEY, unique: 'global' }], + }); + + // At the base this object answered `created` then `UNIQUE_VIOLATION` on the + // write, and `created`, `created` on the dry run. The seat ruled the check + // applies here too: the runner keeps no copy of the engine's uniqueness + // rules, and the dry run answers what the write answers. + it('answers the same refusal on the write and on the dry run: the key is a property of the file, not of the index', async () => { + const { engine, importInto } = await boot([SUPPLIER, UNIQUE_CERT]); + const csv = csvOf('Acme,ISO 9001,A-1,first', 'Acme,ISO9001,A-1,second'); + const dry = await importInto(UNIQUE_CERT.name, upsert(csv, { dryRun: true })); + expect(dry.results).toEqual([duplicate(1, [1, 2]), duplicate(2, [1, 2])]); + const summary = await importInto(UNIQUE_CERT.name, upsert(csv)); + expect(summary.results).toEqual([duplicate(1, [1, 2]), duplicate(2, [1, 2])]); + expect(await engine.find(UNIQUE_CERT.name, {})).toEqual([]); + }); +}); diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index d49f4f9e4c2..9a0c402568f 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -1070,6 +1070,7 @@ export const ERROR_CODE_LEDGER = { // executor in `@objectstack/service-automation` returns the same report. 'AMBIGUOUS_MATCH', // import row matched more than one record 'BLANK_MATCH_KEY', + 'DUPLICATE_MATCH_KEY', // [#26133] import row shares its match key with another row of the same import; none of them is written 'NO_MATCH', // import upsert found no record for the match key 'SUMMARY_RECOMPUTE_FAILED', 'UNSUPPORTED_TRANSFORM', diff --git a/packages/spec/src/api/export.zod.ts b/packages/spec/src/api/export.zod.ts index c2e36816b4c..7345eda9e8c 100644 --- a/packages/spec/src/api/export.zod.ts +++ b/packages/spec/src/api/export.zod.ts @@ -303,7 +303,11 @@ export const ImportRequestSchema = lazySchema(() => z.object({ 'is matched by the first set whose fields are all non-blank on that row, and only by that ' + 'set. A row blank on every set fails with `BLANK_MATCH_KEY` instead of being created or ' + 'skipped, so `skipBlankMatchKey` cannot be combined with key sets. A set that matches more ' + - 'than one record fails the row with `AMBIGUOUS_MATCH`, as one key set does.', + 'than one record fails the row with `AMBIGUOUS_MATCH`, as one key set does. Rows of one ' + + 'import that carry the same key (the key set each row is matched by, with its cell values ' + + 'as coerced, so an option label and its value are one key) all fail with ' + + '`DUPLICATE_MATCH_KEY`, naming every row that carries it, and none of them is written, on a ' + + 'dry run as on the import: keep one row per key.', ), runAutomations: z.boolean().default(true) .describe(