fix(payroll): enforce the jamkning both-dates invariant with a CHECK constraint (#2279)
* fix(payroll): enforce the jamkning both-dates invariant with a database trigger Root cause: PR #2240 made every application write path refuse a jamkning_percentage without both jamkning_valid_from and jamkning_valid_to (validateJamkning), but the rule lived only in application code. Two writes could still store the inert shape the engine never applies: (1) concurrent PATCHes, where both handlers validate a fetched snapshot and then issue an unconditional partial update, so a { jamkning_valid_to: null } that committed last left a percentage without an end date; (2) direct SQL and service-role writes, which bypass the validator entirely. Fix: migration 20260904120000 adds trg_enforce_employee_jamkning_dates, BEFORE INSERT OR UPDATE OF jamkning_percentage, jamkning_valid_from, jamkning_valid_to ON employees. It mirrors validateJamkning: a non-null percentage needs both dates, and valid_to may not precede valid_from. On INSERT it always checks; on UPDATE it checks only when one of the three columns actually changes (IS DISTINCT FROM on OLD vs NEW), so a legacy incomplete row stored before #2240 stays editable in unrelated ways, including by a route that writes the whole row back. The error is SQLSTATE 23514 with the stable prefix "JAMKNING_INCOMPLETE: " followed by the same Swedish sentence the validator produces. The function is SECURITY INVOKER with search_path pinned. No backfill: existing incomplete rows are listed by scripts/list-incomplete-jamkning.ts and decided per company. App side, jamkningIssueFromDbError in lib/salary/jamkning-rules.ts recognises the trigger rejection, and the three update paths (dashboard PATCH, v1 PATCH, MCP update_employee executor) answer it with the same 400 / VALIDATION_ERROR and sentence as the merged-state check, instead of a generic 500 / INTERNAL_ERROR. Tests: tests/pg/employees-jamkning-trigger.pg.test.ts (25 cases against real Postgres, including the interleaved two-transaction race), unit tests for the helper, and one race test per update path. Fixes #2256 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qgLgdt4mLmha1ZLFMwq1u * fix(payroll): enforce the jamkning both-dates invariant with a CHECK constraint Root cause: PR #2240 made every application write path refuse a jamkning_percentage without both jamkning_valid_from and jamkning_valid_to (validateJamkning), but the rule lived only in application code, across many writers. Two concurrent PATCHes that each validated a fetched snapshot and then wrote unconditionally could leave a percentage without an end date (the engine never applies such a beslut, so the payslip and AGI silently carry the table tax), and direct SQL or service-role writes never saw the validator at all. Fix, from first principles: the invariant is a row-level fact, so it is declared as a row-level CHECK constraint, employees_jamkning_dates_check (migration 20260904120000), added NOT VALID so the migration cannot fail on production because of rows stored incomplete before #2240. From now on every INSERT and every UPDATE of any row is checked. This replaces the trigger the issue proposed: no plpgsql function, no per-column change detection, no custom message convention, and the rule is visible in the schema. One behavioural difference from the proposal: a legacy incomplete row is refused on its next edit, related or not, until the beslut is completed (both dates) or cleared (percentage null). The application maps that rejection (SQLSTATE 23514 naming the constraint) to the validator's own Swedish sentence in the three update paths (dashboard PATCH, v1 PATCH, MCP update_employee executor), so the user is told exactly what to complete; a rejection the merged row cannot explain (a concurrent change) gets an umbrella sentence. No backfill: those rows are listed by scripts/list-incomplete-jamkning.ts and decided per company. Tests: tests/pg/employees-jamkning-check.pg.test.ts against real Postgres (constraint shape, INSERT and UPDATE rejections and acceptances, the interleaved two-transaction race, the legacy consequence), unit tests for the mapping, and race plus legacy tests per update path. The PostgREST error shape was verified against a real PostgREST: the constraint name is in `message`, `details` carries the failing row and is never forwarded. Fixes #2256 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qgLgdt4mLmha1ZLFMwq1u --------- Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Fable 5.1
Jakob Wennberg
Claude Fable 5.1
Jakob Wennberg
parent
c6ca119e73
commit
2d927349d3
@@ -8,6 +8,7 @@
|
||||
* extensions/general/mcp-server/__tests__/payroll-staged-tools.test.ts.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest'
|
||||
import { JAMKNING_ROW_INCOMPLETE } from '@/lib/salary/jamkning-rules'
|
||||
import { createQueuedMockSupabase } from '@/tests/helpers'
|
||||
import { eventBus } from '@/lib/events'
|
||||
import type { PendingOperation } from '@/types'
|
||||
@@ -945,4 +946,52 @@ describe('commitPendingOperation: update_employee', () => {
|
||||
expect(result.status).not.toBe('committed')
|
||||
expect(result.error).toMatch(/Månadslön/)
|
||||
})
|
||||
|
||||
it('answers the CHECK constraint (concurrent-update race, #2256) as a validation failure, not INTERNAL_ERROR', async () => {
|
||||
const { encryptPersonnummer } = await import('@/lib/salary/personnummer')
|
||||
const encrypted = encryptPersonnummer('190001010000')
|
||||
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim
|
||||
enqueue({
|
||||
data: {
|
||||
id: 'emp-1',
|
||||
first_name: 'Anna',
|
||||
last_name: 'Andersson',
|
||||
personnummer: encrypted,
|
||||
salary_type: 'monthly',
|
||||
monthly_salary: 35000,
|
||||
tax_table_number: 33,
|
||||
is_sidoinkomst: false,
|
||||
f_skatt_status: 'a_skatt',
|
||||
vaxa_stod_eligible: false,
|
||||
jamkning_percentage: null,
|
||||
jamkning_valid_from: null,
|
||||
jamkning_valid_to: null,
|
||||
is_active: true,
|
||||
},
|
||||
}) // fetch existing: the snapshot the merged check passes against
|
||||
enqueue({
|
||||
data: null,
|
||||
error: {
|
||||
code: '23514',
|
||||
message: 'new row for relation "employees" violates check constraint "employees_jamkning_dates_check"',
|
||||
details: 'Failing row contains (...).',
|
||||
hint: null,
|
||||
},
|
||||
}) // update: the constraint checked the row another request changed in between
|
||||
enqueue({ data: null, error: null }) // finalize
|
||||
|
||||
const op = makePendingOp({
|
||||
operation_type: 'update_employee',
|
||||
params: {
|
||||
employee_id: 'emp-1',
|
||||
patch: { jamkning_percentage: 15, jamkning_valid_from: '2026-01-01', jamkning_valid_to: '2026-12-31' },
|
||||
},
|
||||
})
|
||||
const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op)
|
||||
|
||||
expect(result.status).not.toBe('committed')
|
||||
expect(result.error).toBe(JAMKNING_ROW_INCOMPLETE)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,8 +1,11 @@
|
||||
import { describe, it, expect } from 'vitest'
|
||||
import {
|
||||
JAMKNING_CHECK_CONSTRAINT,
|
||||
JAMKNING_END_REQUIRED,
|
||||
JAMKNING_ORDER,
|
||||
JAMKNING_ROW_INCOMPLETE,
|
||||
JAMKNING_START_REQUIRED,
|
||||
jamkningIssueFromDbError,
|
||||
touchesJamkning,
|
||||
validateJamkning,
|
||||
} from '../jamkning-rules'
|
||||
@@ -105,3 +108,67 @@ describe('touchesJamkning', () => {
|
||||
expect(touchesJamkning({})).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
// The PostgREST error for a violated CHECK constraint, as observed against a
|
||||
// real PostgREST (tests/tool-pg): the constraint name is in `message` only,
|
||||
// and `details` carries the failing row, which no caller may echo.
|
||||
const CHECK_ERROR = {
|
||||
code: '23514',
|
||||
details: 'Failing row contains (...).',
|
||||
hint: null,
|
||||
message: `new row for relation "employees" violates check constraint "${JAMKNING_CHECK_CONSTRAINT}"`,
|
||||
}
|
||||
|
||||
describe('jamkningIssueFromDbError (#2256: the CHECK constraint backstop)', () => {
|
||||
it('recovers the validator sentence from the merged row (a legacy incomplete row on its next edit)', () => {
|
||||
expect(
|
||||
jamkningIssueFromDbError(CHECK_ERROR, {
|
||||
jamkning_percentage: 15,
|
||||
jamkning_valid_from: '2026-01-01',
|
||||
jamkning_valid_to: null,
|
||||
}),
|
||||
).toEqual({ field: 'jamkning_valid_to', message: JAMKNING_END_REQUIRED })
|
||||
expect(
|
||||
jamkningIssueFromDbError(CHECK_ERROR, { jamkning_percentage: 15, jamkning_valid_from: null, jamkning_valid_to: null }),
|
||||
).toEqual({ field: 'jamkning_valid_from', message: JAMKNING_START_REQUIRED })
|
||||
expect(
|
||||
jamkningIssueFromDbError(CHECK_ERROR, {
|
||||
jamkning_percentage: 15,
|
||||
jamkning_valid_from: '2026-06-01',
|
||||
jamkning_valid_to: '2026-01-31',
|
||||
}),
|
||||
).toEqual({ field: 'jamkning_valid_to', message: JAMKNING_ORDER })
|
||||
})
|
||||
|
||||
it('falls back to the umbrella sentence when the merged row is valid (a concurrent change)', () => {
|
||||
expect(
|
||||
jamkningIssueFromDbError(CHECK_ERROR, {
|
||||
jamkning_percentage: 15,
|
||||
jamkning_valid_from: '2026-01-01',
|
||||
jamkning_valid_to: '2026-12-31',
|
||||
}),
|
||||
).toEqual({ field: 'jamkning_valid_to', message: JAMKNING_ROW_INCOMPLETE })
|
||||
expect(jamkningIssueFromDbError(CHECK_ERROR)).toEqual({ field: 'jamkning_valid_to', message: JAMKNING_ROW_INCOMPLETE })
|
||||
})
|
||||
|
||||
it('accepts the node-postgres shape, which names the constraint in its own field', () => {
|
||||
expect(
|
||||
jamkningIssueFromDbError({ code: '23514', message: 'anything', constraint: JAMKNING_CHECK_CONSTRAINT }),
|
||||
).toEqual({ field: 'jamkning_valid_to', message: JAMKNING_ROW_INCOMPLETE })
|
||||
})
|
||||
|
||||
it('ignores every other error', () => {
|
||||
expect(jamkningIssueFromDbError(null)).toBeNull()
|
||||
expect(jamkningIssueFromDbError(CHECK_ERROR.message)).toBeNull()
|
||||
// Another CHECK constraint on the same table.
|
||||
expect(
|
||||
jamkningIssueFromDbError({
|
||||
code: '23514',
|
||||
message: 'new row for relation "employees" violates check constraint "employees_tax_column_check"',
|
||||
}),
|
||||
).toBeNull()
|
||||
// The constraint name under a different SQLSTATE is not the constraint.
|
||||
expect(jamkningIssueFromDbError({ code: 'P0001', message: CHECK_ERROR.message })).toBeNull()
|
||||
expect(jamkningIssueFromDbError({ code: '23514' })).toBeNull()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -19,7 +19,7 @@ import { decryptPersonnummer, maskPersonnummer } from '@/lib/salary/personnummer
|
||||
import { getCompanyEntityType } from '@/lib/company/context'
|
||||
import { isEmploymentTypeAllowedForEntity, EF_OWNER_EMPLOYMENT_ERROR } from '@/lib/salary/employment-rules'
|
||||
import { validateEmployeeBankAccount } from '@/lib/salary/payment/bank-account'
|
||||
import { touchesJamkning, validateJamkning } from '@/lib/salary/jamkning-rules'
|
||||
import { jamkningIssueFromDbError, touchesJamkning, validateJamkning, type JamkningFields } from '@/lib/salary/jamkning-rules'
|
||||
|
||||
export type EmployeeCommandResult<T> =
|
||||
| { ok: true; data: T }
|
||||
@@ -288,6 +288,19 @@ export async function updateEmployee(
|
||||
.single()
|
||||
|
||||
if (error) {
|
||||
// The CHECK constraint refused the row (#2256): either the merged-state
|
||||
// check above passed against a snapshot another request has since
|
||||
// changed, or this is a legacy incomplete row (stored before #2240) whose
|
||||
// next edit must complete or clear the beslut. Same VALIDATION_ERROR and
|
||||
// sentence as that check, derived from the merged row.
|
||||
const jamkningIssue = jamkningIssueFromDbError(error, merged as JamkningFields)
|
||||
if (jamkningIssue) {
|
||||
return {
|
||||
ok: false,
|
||||
code: 'VALIDATION_ERROR',
|
||||
details: { field: jamkningIssue.field, message: jamkningIssue.message },
|
||||
}
|
||||
}
|
||||
return { ok: false, code: 'INTERNAL_ERROR', details: { message: error.message } }
|
||||
}
|
||||
|
||||
|
||||
@@ -8,7 +8,9 @@
|
||||
* payslip and the AGI carry the table tax while the caller was told 200.
|
||||
* Every write path (web routes, v1 REST, MCP staging and executors, the Zod
|
||||
* schemas) runs the merged row through this function so that shape can no
|
||||
* longer be stored. #2058
|
||||
* longer be stored. #2058. The database declares the same rule as the CHECK
|
||||
* constraint employees_jamkning_dates_check (#2256), so a concurrent or
|
||||
* direct write cannot store it either; see jamkningIssueFromDbError below.
|
||||
*
|
||||
* Setting the percentage to null clears the beslut; the dates are then free.
|
||||
*/
|
||||
@@ -65,3 +67,48 @@ export function validateJamkning(fields: JamkningFields): JamkningIssue[] {
|
||||
export function touchesJamkning(patch: Record<string, unknown>): boolean {
|
||||
return JAMKNING_FIELDS.some((key) => key in patch)
|
||||
}
|
||||
|
||||
/**
|
||||
* The database backstop for the same invariant: CHECK constraint
|
||||
* employees_jamkning_dates_check (migration 20260904120000, #2256), the
|
||||
* rule above declared on the row itself. It is what an update sees when its
|
||||
* merged-state check passed against a snapshot another request has since
|
||||
* changed (the concurrent-PATCH race), and what ANY edit of a row stored
|
||||
* incomplete before #2240 sees: the constraint was added NOT VALID, so such
|
||||
* a row is checked on its next UPDATE and must be completed or cleared then.
|
||||
*/
|
||||
export const JAMKNING_CHECK_CONSTRAINT = 'employees_jamkning_dates_check'
|
||||
|
||||
/**
|
||||
* Sentence for a constraint rejection the merged row cannot explain: the
|
||||
* row on disk is no longer the snapshot the caller validated against.
|
||||
*/
|
||||
export const JAMKNING_ROW_INCOMPLETE =
|
||||
'Jämkningen måste ha både startdatum och slutdatum, och slutdatumet får inte ligga före startdatumet. Ladda om uppgifterna och försök igen'
|
||||
|
||||
/**
|
||||
* The issue behind a rejection by employees_jamkning_dates_check, or null
|
||||
* when the error is anything else. Matches SQLSTATE 23514 plus the
|
||||
* constraint name: Postgres puts it in the message ('new row for relation
|
||||
* "employees" violates check constraint "employees_jamkning_dates_check"'),
|
||||
* which PostgREST forwards verbatim as `message`, and node-postgres also
|
||||
* exposes it as `constraint`.
|
||||
*
|
||||
* A CHECK violation does not say which clause failed, so the sentence is
|
||||
* recovered from the row the caller meant to store (`merged`: existing row
|
||||
* plus patch). For a legacy incomplete row that is the exact validator
|
||||
* sentence, naming the missing date. When that row passes the validator the
|
||||
* rejection came from a concurrent change, and the umbrella sentence is
|
||||
* returned instead.
|
||||
*/
|
||||
export function jamkningIssueFromDbError(error: unknown, merged: JamkningFields = {}): JamkningIssue | null {
|
||||
if (typeof error !== 'object' || error === null) return null
|
||||
const { code, message, constraint } = error as { code?: unknown; message?: unknown; constraint?: unknown }
|
||||
if (code !== '23514') return null
|
||||
const named =
|
||||
constraint === JAMKNING_CHECK_CONSTRAINT ||
|
||||
(typeof message === 'string' && message.includes(`"${JAMKNING_CHECK_CONSTRAINT}"`))
|
||||
if (!named) return null
|
||||
const [issue] = validateJamkning(merged)
|
||||
return issue ?? { field: 'jamkning_valid_to', message: JAMKNING_ROW_INCOMPLETE }
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user