diff --git a/app/api/salary/employees/[id]/__tests__/route.test.ts b/app/api/salary/employees/[id]/__tests__/route.test.ts index b3d67674..384ac754 100644 --- a/app/api/salary/employees/[id]/__tests__/route.test.ts +++ b/app/api/salary/employees/[id]/__tests__/route.test.ts @@ -18,6 +18,7 @@ * which loosens the Zod schema to prove the defence lives in the route. */ import { describe, it, expect, vi, beforeEach } from 'vitest' +import { JAMKNING_ROW_INCOMPLETE } from '@/lib/salary/jamkning-rules' import { NextResponse } from 'next/server' import { createQueuedMockSupabase, createMockRequest, parseJsonResponse } from '@/tests/helpers' @@ -73,13 +74,19 @@ const EXISTING_ROW = { * to `.update()`, and resolves the update chain with the merged row. `captured.updates` * staying null is the assertion that no write was attempted at all. */ -function employeeSupabase(existing: Record = { ...EXISTING_ROW }) { +function employeeSupabase( + existing: Record = { ...EXISTING_ROW }, + updateError: { code: string; message: string } | null = null, +) { const captured: { updates: Record | null } = { updates: null } function chainFor(state: { isUpdate: boolean }): unknown { const handler: ProxyHandler = { get(_target, prop) { if (prop === 'then') { + if (state.isUpdate && updateError) { + return (resolve: (v: unknown) => void) => resolve({ data: null, error: updateError }) + } const data = state.isUpdate ? { ...existing, ...(captured.updates ?? {}) } : existing return (resolve: (v: unknown) => void) => resolve({ data, error: null }) } @@ -223,8 +230,8 @@ describe('jämkning on PATCH /api/salary/employees/[id]', () => { const JAMKNING_END_REQUIRED = 'Jämkningens slutdatum måste anges när jämkningsprocent sätts' const JAMKNING_ORDER = 'Jämkningens slutdatum måste vara efter startdatumet' - function useRow(existing: Record) { - const mock = employeeSupabase(existing) + function useRow(existing: Record, updateError: { code: string; message: string } | null = null) { + const mock = employeeSupabase(existing, updateError) requireAuthMock.mockResolvedValue({ user: { id: 'user-1' }, supabase: mock.supabase }) return mock.captured } @@ -351,4 +358,64 @@ describe('jämkning on PATCH /api/salary/employees/[id]', () => { expect(response.status).toBe(200) expect(captured.updates).toEqual({ first_name: 'Ny' }) }) + + // The PostgREST error for employees_jamkning_dates_check (#2256), as + // observed against a real PostgREST: the constraint name is in `message`, + // `details` carries the failing row and must never reach the response. + const CHECK_CONSTRAINT_ERROR = { + code: '23514', + message: 'new row for relation "employees" violates check constraint "employees_jamkning_dates_check"', + details: 'Failing row contains (...).', + hint: null, + } + + it('400 with the umbrella sentence when the CHECK constraint catches the race (#2256)', async () => { + // The snapshot this handler validated against was empty, so nulling only + // the end date is a no-op to the merged check... except another request + // stored a complete beslut in between. The constraint is the only thing + // that sees the real row: it must come back as a 400, not a 500, and the + // merged row cannot explain it, so the umbrella sentence is used. + const captured = useRow( + { ...EXISTING_ROW, jamkning_percentage: null, jamkning_valid_from: null, jamkning_valid_to: null }, + CHECK_CONSTRAINT_ERROR, + ) + + const response = await PATCH(patchRequest({ jamkning_valid_to: null }), params) + const { status, body } = await parseJsonResponse<{ error: string }>(response) + + expect(status).toBe(400) + expect(body.error).toBe(JAMKNING_ROW_INCOMPLETE) + expect(JSON.stringify(body)).not.toContain('Failing row') + expect(captured.updates).toEqual({ jamkning_valid_to: null }) + }) + + it('400 naming the missing date when a legacy incomplete row is edited in an unrelated column (NOT VALID trade-off)', async () => { + // The route lets the unrelated edit through (touched gate), the + // constraint refuses the row on its next UPDATE: the user is told exactly + // what to complete, with the validator's own sentence. + const captured = useRow( + { ...EXISTING_ROW, jamkning_percentage: 15, jamkning_valid_from: '2026-01-01', jamkning_valid_to: null }, + CHECK_CONSTRAINT_ERROR, + ) + + const response = await PATCH(patchRequest({ first_name: 'Ny' }), params) + const { status, body } = await parseJsonResponse<{ error: string }>(response) + + expect(status).toBe(400) + expect(body.error).toBe(JAMKNING_END_REQUIRED) + expect(captured.updates).toEqual({ first_name: 'Ny' }) + }) + + it('500 through the generic mapper for any other database error on the update', async () => { + useRow( + { ...EXISTING_ROW, jamkning_percentage: null, jamkning_valid_from: null, jamkning_valid_to: null }, + { code: '23514', message: 'new row for relation "employees" violates check constraint "employees_tax_column_check"' }, + ) + + const response = await PATCH(patchRequest({ first_name: 'Ny' }), params) + const { status, body } = await parseJsonResponse<{ error: string }>(response) + + expect(status).toBe(500) + expect(body.error).not.toContain('JAMKNING') + }) }) diff --git a/app/api/salary/employees/[id]/route.ts b/app/api/salary/employees/[id]/route.ts index f22f2e45..a8500016 100644 --- a/app/api/salary/employees/[id]/route.ts +++ b/app/api/salary/employees/[id]/route.ts @@ -7,7 +7,7 @@ import { getCompanyEntityType } from '@/lib/company/context' import { encryptPersonnummer, extractLast4, maskEmployeeForResponse, validatePersonnummer } from '@/lib/salary/personnummer' 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 } from '@/lib/salary/jamkning-rules' import { getErrorMessage as getUserErrorMessage } from '@/lib/errors/get-error-message' ensureInitialized() @@ -180,6 +180,15 @@ export const PATCH = withRouteContext<{ params: Promise<{ id: string }> }>( if (error.code === '23505') { return NextResponse.json({ error: 'En anställd med detta personnummer finns redan' }, { status: 409 }) } + // 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 400 and + // sentence as that check, derived from the merged row. + const jamkningIssue = jamkningIssueFromDbError(error, merged) + if (jamkningIssue) { + return NextResponse.json({ error: jamkningIssue.message }, { status: 400 }) + } return NextResponse.json({ error: getUserErrorMessage(error) }, { status: 500 }) } diff --git a/app/api/v1/companies/[companyId]/employees/[id]/route.ts b/app/api/v1/companies/[companyId]/employees/[id]/route.ts index f56dc6af..f0e6b748 100644 --- a/app/api/v1/companies/[companyId]/employees/[id]/route.ts +++ b/app/api/v1/companies/[companyId]/employees/[id]/route.ts @@ -27,7 +27,15 @@ import { readV1JsonBody } from '@/lib/api/v1/body' import { UpdateEmployeeSchema } from '@/lib/api/schemas' import { maskPersonnummer } from '@/lib/api/v1/mask-personnummer' import { decryptPersonnummer } from '@/lib/salary/personnummer' -import { JAMKNING_ORDER, touchesJamkning, validateJamkning, type JamkningFields } from '@/lib/salary/jamkning-rules' +import { + JAMKNING_END_REQUIRED, + JAMKNING_START_REQUIRED, + jamkningIssueFromDbError, + touchesJamkning, + validateJamkning, + type JamkningFields, + type JamkningIssue, +} from '@/lib/salary/jamkning-rules' const EmploymentType = z.enum(['employee', 'company_owner', 'board_member']) const SalaryType = z.enum(['monthly', 'hourly']) @@ -349,19 +357,20 @@ export const PATCH = withApiV1<{ params: Promise<{ companyId: string; id: string // jamkning field: a legacy row with inconsistent jamkning_* state must // not block unrelated updates (fixing it requires touching those very // fields). #2058 + const jamkningIssueDetails = (issue: JamkningIssue) => ({ + field: issue.field, + message: + issue.message === JAMKNING_START_REQUIRED || issue.message === JAMKNING_END_REQUIRED + ? `${issue.message}. Skicka även \`${issue.field}\` i samma PATCH.` + : `${issue.message}.`, + }) + const mergedJamkning = { ...(existing as Record), ...updates } as JamkningFields if (touchesJamkning(updates)) { - const mergedJamkning = { ...(existing as Record), ...updates } as JamkningFields const [issue] = validateJamkning(mergedJamkning) if (issue) { return v1ErrorResponseFromCode('VALIDATION_ERROR', ctx.log, { requestId: ctx.requestId, - details: { - field: issue.field, - message: - issue.message === JAMKNING_ORDER - ? `${issue.message}.` - : `${issue.message}. Skicka även \`${issue.field}\` i samma PATCH.`, - }, + details: jamkningIssueDetails(issue), }) } } @@ -395,6 +404,18 @@ export const PATCH = withApiV1<{ params: Promise<{ companyId: string; id: string .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 details as that check, derived from the merged row. + const jamkningIssue = jamkningIssueFromDbError(error, mergedJamkning) + if (jamkningIssue) { + return v1ErrorResponseFromCode('VALIDATION_ERROR', ctx.log, { + requestId: ctx.requestId, + details: jamkningIssueDetails(jamkningIssue), + }) + } return v1ErrorResponse(error, ctx.log, { requestId: ctx.requestId }) } diff --git a/app/api/v1/companies/[companyId]/employees/__tests__/route.test.ts b/app/api/v1/companies/[companyId]/employees/__tests__/route.test.ts index 184519be..0fb461df 100644 --- a/app/api/v1/companies/[companyId]/employees/__tests__/route.test.ts +++ b/app/api/v1/companies/[companyId]/employees/__tests__/route.test.ts @@ -8,6 +8,7 @@ */ import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' +import { JAMKNING_END_REQUIRED, JAMKNING_ROW_INCOMPLETE } from '@/lib/salary/jamkning-rules' beforeAll(() => { if (process.env.NODE_ENV !== 'test') { @@ -907,6 +908,89 @@ describe('PATCH /api/v1/companies/:companyId/employees/:id', () => { expect(body.error.details.field).toBe('jamkning_valid_to') }) + // The PostgREST error for employees_jamkning_dates_check (#2256), as + // observed against a real PostgREST: the constraint name is in `message`, + // `details` carries the failing row and must never reach the response. + const CHECK_CONSTRAINT_ERROR = { + code: '23514', + message: 'new row for relation "employees" violates check constraint "employees_jamkning_dates_check"', + details: 'Failing row contains (...).', + hint: null, + } + + it('answers the CHECK constraint (concurrent-PATCH race, #2256) as the same VALIDATION_ERROR', async () => { + // The snapshot was empty, so a percentage with both dates passes the + // merged check; another request changed the row in between and the + // constraint refused the write. The merged row cannot explain it, so the + // umbrella sentence is used. + mockServiceClient.mockReturnValue( + makeFlexibleSupabase({ + company_members: { data: { company_id: COMPANY_ID, role: 'owner' }, error: null }, + employees: [ + { data: SAMPLE_EMPLOYEE, error: null }, + { data: null, error: CHECK_CONSTRAINT_ERROR }, + ], + idempotency_keys: { data: null, error: null }, + }), + ) + + const res = await updateEmployee( + makeRequest(`https://x.test/api/v1/companies/${COMPANY_ID}/employees/${EMPLOYEE_ID}`, { + method: 'PATCH', + body: JSON.stringify({ + jamkning_percentage: 15, + jamkning_valid_from: '2026-01-01', + jamkning_valid_to: '2026-12-31', + }), + }), + detailParams(COMPANY_ID, EMPLOYEE_ID), + ) + + expect(res.status).toBe(400) + const body = await res.json() + expect(body.error.code).toBe('VALIDATION_ERROR') + expect(body.error.details.field).toBe('jamkning_valid_to') + expect(body.error.details.message).toBe(`${JAMKNING_ROW_INCOMPLETE}.`) + expect(JSON.stringify(body)).not.toContain('violates check constraint') + expect(JSON.stringify(body)).not.toContain('Failing row') + }) + + it('names the missing date when a legacy incomplete row is edited in an unrelated column (NOT VALID trade-off)', async () => { + // The route lets the unrelated edit through (touched gate); the + // constraint refuses the row on its next UPDATE, and the merged row + // explains exactly what to complete. + const legacy = { + ...SAMPLE_EMPLOYEE, + jamkning_percentage: 15, + jamkning_valid_from: '2026-01-01', + jamkning_valid_to: null, + } + mockServiceClient.mockReturnValue( + makeFlexibleSupabase({ + company_members: { data: { company_id: COMPANY_ID, role: 'owner' }, error: null }, + employees: [ + { data: legacy, error: null }, + { data: null, error: CHECK_CONSTRAINT_ERROR }, + ], + idempotency_keys: { data: null, error: null }, + }), + ) + + const res = await updateEmployee( + makeRequest(`https://x.test/api/v1/companies/${COMPANY_ID}/employees/${EMPLOYEE_ID}`, { + method: 'PATCH', + body: JSON.stringify({ monthly_salary: 38000 }), + }), + detailParams(COMPANY_ID, EMPLOYEE_ID), + ) + + expect(res.status).toBe(400) + const body = await res.json() + expect(body.error.code).toBe('VALIDATION_ERROR') + expect(body.error.details.field).toBe('jamkning_valid_to') + expect(body.error.details.message).toBe(`${JAMKNING_END_REQUIRED}. Skicka även \`jamkning_valid_to\` i samma PATCH.`) + }) + it('leaves a legacy row without valid_to editable in unrelated ways (touched gate)', async () => { const legacy = { ...SAMPLE_EMPLOYEE, diff --git a/lib/pending-operations/__tests__/payroll-executors.test.ts b/lib/pending-operations/__tests__/payroll-executors.test.ts index 53c26be8..1be2b561 100644 --- a/lib/pending-operations/__tests__/payroll-executors.test.ts +++ b/lib/pending-operations/__tests__/payroll-executors.test.ts @@ -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) + }) }) diff --git a/lib/salary/__tests__/jamkning-rules.test.ts b/lib/salary/__tests__/jamkning-rules.test.ts index 28ebe6eb..6102e8e2 100644 --- a/lib/salary/__tests__/jamkning-rules.test.ts +++ b/lib/salary/__tests__/jamkning-rules.test.ts @@ -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() + }) +}) diff --git a/lib/salary/employee-commands.ts b/lib/salary/employee-commands.ts index c5d280d0..e269b614 100644 --- a/lib/salary/employee-commands.ts +++ b/lib/salary/employee-commands.ts @@ -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 = | { 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 } } } diff --git a/lib/salary/jamkning-rules.ts b/lib/salary/jamkning-rules.ts index b4726ec0..3e03111e 100644 --- a/lib/salary/jamkning-rules.ts +++ b/lib/salary/jamkning-rules.ts @@ -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): 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 } +} diff --git a/supabase/migrations/20260904120000_employees_jamkning_dates_check.sql b/supabase/migrations/20260904120000_employees_jamkning_dates_check.sql new file mode 100644 index 00000000..e686be38 --- /dev/null +++ b/supabase/migrations/20260904120000_employees_jamkning_dates_check.sql @@ -0,0 +1,57 @@ +-- Enforce the jämkning both-dates invariant in the database (#2256). +-- +-- A jämkningsbeslut (Skatteverket beslut om ändrad beräkning av skatteavdrag) +-- on an employee is a percentage with a validity window. The calculation +-- engine (isJamkningValid in lib/salary/calculation-engine.ts) applies the +-- beslut only when BOTH jamkning_valid_from and jamkning_valid_to are set and +-- the payment date falls inside them. A percentage stored without an end date +-- is therefore inert: the payslip and the AGI carry the table tax while the +-- stored beslut says otherwise, and nobody is told. +-- +-- PR #2240 closed that shape on every application write path through +-- lib/salary/jamkning-rules.ts (validateJamkning), but the rule lived only in +-- application code, across many writers: two PATCH handlers that validate a +-- fetched snapshot and then write unconditionally can interleave so that the +-- last one leaves a percentage without an end date, and direct SQL or +-- service-role writes never see the validator at all. +-- +-- The invariant is a row-level fact, so it is declared as a row-level CHECK +-- constraint, the same rule validateJamkning applies: +-- +-- * a non-null jamkning_percentage requires both dates; +-- * when both dates are present, jamkning_valid_to may not precede +-- jamkning_valid_from (also when no percentage is set, as the validator +-- and the Zod update schema do); +-- * a null percentage clears the beslut and leaves the dates free. +-- +-- NOT VALID: the constraint is not checked against rows that already exist, +-- so this migration cannot fail on production because of the incomplete +-- rows stored before #2240. From now on every INSERT and every UPDATE of any +-- row is checked, whatever columns the UPDATE names. Consequence for a +-- legacy incomplete row: its next edit, related or not, is refused with +-- SQLSTATE 23514 until the beslut is completed (both dates) or cleared +-- (percentage null); the application maps that to the validator's Swedish +-- sentence (jamkningIssueFromDbError in lib/salary/jamkning-rules.ts). Those +-- rows are listed read-only by scripts/list-incomplete-jamkning.ts and +-- decided per company. No backfill here: this migration never rewrites data. +-- +-- Not validated later on purpose: VALIDATE CONSTRAINT would fail while any +-- legacy row remains, and the constraint already protects every new write. + +ALTER TABLE public.employees + ADD CONSTRAINT employees_jamkning_dates_check CHECK ( + ( + jamkning_percentage IS NULL + OR (jamkning_valid_from IS NOT NULL AND jamkning_valid_to IS NOT NULL) + ) + AND ( + jamkning_valid_from IS NULL + OR jamkning_valid_to IS NULL + OR jamkning_valid_to >= jamkning_valid_from + ) + ) NOT VALID; + +COMMENT ON CONSTRAINT employees_jamkning_dates_check ON public.employees IS + 'Jämkning both-dates invariant (#2256): a non-null jamkning_percentage needs both jamkning_valid_from and jamkning_valid_to, and valid_to may not precede valid_from. Mirrors validateJamkning in lib/salary/jamkning-rules.ts. Added NOT VALID: rows stored before this constraint are checked on their next UPDATE, not backfilled.'; + +NOTIFY pgrst, 'reload schema'; diff --git a/tests/pg/employees-jamkning-check.pg.test.ts b/tests/pg/employees-jamkning-check.pg.test.ts new file mode 100644 index 00000000..4e93c7af --- /dev/null +++ b/tests/pg/employees-jamkning-check.pg.test.ts @@ -0,0 +1,360 @@ +/** + * pg-real tests for 20260904120000_employees_jamkning_dates_check.sql + * (#2256: the jämkning both-dates invariant declared as a CHECK constraint). + * + * The application validator (lib/salary/jamkning-rules.ts, PR #2240) refuses + * a jamkning_percentage without both validity dates on every write path, but + * a concurrent PATCH that validated a stale snapshot, or a direct SQL / + * service-role write, could still store one. The constraint is the backstop: + * the same rule, declared on the row, checked on every INSERT and UPDATE. + * + * Verifies: + * - constraint shape: CHECK on employees, added NOT VALID, commented + * - INSERT: percentage without valid_to / valid_from rejected; valid_to + * before valid_from rejected; percentage with both dates accepted; no + * percentage with stray dates accepted + * - UPDATE: nulling valid_to while the percentage stays rejected; valid_to + * before valid_from rejected; clearing the beslut accepted + * - the concurrent-PATCH race from the issue, with the second statement + * blocked on the first transaction's row lock, fails for the second + * - the legacy consequence of NOT VALID: a row stored incomplete before the + * constraint is refused on its next edit, related or not, until the + * beslut is completed or cleared + * - the error is SQLSTATE 23514 naming the constraint, which is what the + * application keys on (jamkningIssueFromDbError) + */ +import { describe, it, expect } from 'vitest' +import { randomUUID } from 'node:crypto' +import { getClient, getPool } from './setup' +import { insertAuthUser, insertCompany, insertCompanyMember } from './fixtures' + +const CONSTRAINT = 'employees_jamkning_dates_check' + +interface PgError extends Error { + code?: string + constraint?: string +} + +interface Jamkning { + percentage: number | null + from: string | null + to: string | null +} + +async function seedCompany(): Promise<{ userId: string; companyId: string }> { + const userId = await insertAuthUser() + const companyId = await insertCompany({ createdBy: userId }) + await insertCompanyMember({ companyId, userId, role: 'owner' }) + return { userId, companyId } +} + +function insertSql(id: string, companyId: string, userId: string, j: Jamkning) { + return { + text: `INSERT INTO public.employees + (id, company_id, user_id, first_name, last_name, personnummer, personnummer_last4, + employment_start, jamkning_percentage, jamkning_valid_from, jamkning_valid_to) + VALUES ($1, $2, $3, 'Test', 'Testsson', $4, '0000', '2026-01-01', $5, $6, $7)`, + // personnummer is unique per company; the ciphertext is never decrypted here. + values: [id, companyId, userId, `enc-${id}`, j.percentage, j.from, j.to], + } +} + +async function insertEmployee(seed: { companyId: string; userId: string }, j: Jamkning): Promise { + const id = randomUUID() + const q = insertSql(id, seed.companyId, seed.userId, j) + await getPool().query(q.text, q.values) + return id +} + +// A row stored before the constraint existed: percentage set, valid_to +// missing. The constraint refuses that shape on INSERT, so the seed drops it +// for exactly this statement and re-adds it NOT VALID (the migration's own +// definition, read back from the catalog), inside one transaction so the +// constraint is back before anything else can run against the table. +async function insertLegacyIncompleteEmployee(seed: { companyId: string; userId: string }): Promise { + const id = randomUUID() + const q = insertSql(id, seed.companyId, seed.userId, { percentage: 15, from: '2026-01-01', to: null }) + const client = await getClient() + try { + await client.query('BEGIN') + const def = await client.query<{ def: string }>( + `SELECT pg_get_constraintdef(oid) AS def FROM pg_constraint + WHERE conname = $1 AND conrelid = 'public.employees'::regclass`, + [CONSTRAINT], + ) + const definition = def.rows[0]?.def + if (!definition) throw new Error(`${CONSTRAINT} is missing: did the migration apply?`) + await client.query(`ALTER TABLE public.employees DROP CONSTRAINT ${CONSTRAINT}`) + await client.query(q.text, q.values) + await client.query( + `ALTER TABLE public.employees ADD CONSTRAINT ${CONSTRAINT} ${ + definition.includes('NOT VALID') ? definition : `${definition} NOT VALID` + }`, + ) + await client.query('COMMIT') + } catch (err) { + await client.query('ROLLBACK').catch(() => {}) + throw err + } finally { + client.release() + } + return id +} + +async function readJamkning(id: string): Promise { + const res = await getPool().query<{ + first_name: string + jamkning_percentage: string | null + jamkning_valid_from: string | null + jamkning_valid_to: string | null + }>( + `SELECT first_name, jamkning_percentage, + to_char(jamkning_valid_from, 'YYYY-MM-DD') AS jamkning_valid_from, + to_char(jamkning_valid_to, 'YYYY-MM-DD') AS jamkning_valid_to + FROM public.employees WHERE id = $1`, + [id], + ) + const row = res.rows[0] + return { + first_name: row.first_name, + percentage: row.jamkning_percentage === null ? null : Number(row.jamkning_percentage), + from: row.jamkning_valid_from, + to: row.jamkning_valid_to, + } +} + +async function captureError(promise: Promise): Promise { + try { + await promise + } catch (err) { + return err as PgError + } + throw new Error('expected the statement to be rejected') +} + +// What the application keys on: the SQLSTATE and the constraint name, which +// Postgres puts in the message (PostgREST forwards it verbatim) and +// node-postgres also exposes as `constraint`. +function expectConstraintRejection(err: PgError) { + expect(err.code).toBe('23514') + expect(err.constraint).toBe(CONSTRAINT) + expect(err.message).toBe(`new row for relation "employees" violates check constraint "${CONSTRAINT}"`) +} + +describe('constraint shape', () => { + it('is a CHECK on employees, added NOT VALID, commented', async () => { + const res = await getPool().query<{ + contype: string + convalidated: boolean + def: string + comment: string | null + }>( + `SELECT c.contype, c.convalidated, pg_get_constraintdef(c.oid) AS def, + obj_description(c.oid, 'pg_constraint') AS comment + FROM pg_constraint c + WHERE c.conname = $1 AND c.conrelid = 'public.employees'::regclass`, + [CONSTRAINT], + ) + expect(res.rows).toHaveLength(1) + const row = res.rows[0] + expect(row.contype).toBe('c') + // NOT VALID: rows stored before the constraint are not backfilled, they + // are checked on their next UPDATE (see the legacy block below). + expect(row.convalidated).toBe(false) + expect(row.def).toContain('NOT VALID') + expect(row.def).toContain('jamkning_valid_to >= jamkning_valid_from') + expect(row.comment).toContain('#2256') + }) +}) + +describe('INSERT', () => { + it('rejects a percentage without an end date (#2058 shape)', async () => { + const seed = await seedCompany() + const err = await captureError(insertEmployee(seed, { percentage: 20, from: '2026-01-01', to: null })) + expectConstraintRejection(err) + }) + + it('rejects a percentage without a start date', async () => { + const seed = await seedCompany() + const err = await captureError(insertEmployee(seed, { percentage: 20, from: null, to: '2026-12-31' })) + expectConstraintRejection(err) + }) + + it('rejects an end date before the start date', async () => { + const seed = await seedCompany() + const err = await captureError(insertEmployee(seed, { percentage: 20, from: '2026-06-01', to: '2026-01-31' })) + expectConstraintRejection(err) + }) + + it('rejects an inverted window even without a percentage (validator and Zod parity)', async () => { + const seed = await seedCompany() + const err = await captureError(insertEmployee(seed, { percentage: null, from: '2026-06-01', to: '2026-01-31' })) + expectConstraintRejection(err) + }) + + it('accepts a percentage with both dates', async () => { + const seed = await seedCompany() + const id = await insertEmployee(seed, { percentage: 20, from: '2026-01-01', to: '2026-12-31' }) + expect(await readJamkning(id)).toMatchObject({ percentage: 20, from: '2026-01-01', to: '2026-12-31' }) + }) + + it('accepts a one-day window (valid_to = valid_from)', async () => { + const seed = await seedCompany() + const id = await insertEmployee(seed, { percentage: 20, from: '2026-03-25', to: '2026-03-25' }) + expect(await readJamkning(id)).toMatchObject({ percentage: 20, from: '2026-03-25', to: '2026-03-25' }) + }) + + it('accepts no beslut at all, with or without stray dates', async () => { + const seed = await seedCompany() + await insertEmployee(seed, { percentage: null, from: null, to: null }) + const id = await insertEmployee(seed, { percentage: null, from: '2026-01-01', to: null }) + expect(await readJamkning(id)).toMatchObject({ percentage: null, from: '2026-01-01', to: null }) + }) +}) + +describe('UPDATE', () => { + it('rejects nulling the end date while the percentage stays set', async () => { + const seed = await seedCompany() + const id = await insertEmployee(seed, { percentage: 20, from: '2026-01-01', to: '2026-12-31' }) + const err = await captureError( + getPool().query(`UPDATE public.employees SET jamkning_valid_to = NULL WHERE id = $1`, [id]), + ) + expectConstraintRejection(err) + expect(await readJamkning(id)).toMatchObject({ percentage: 20, from: '2026-01-01', to: '2026-12-31' }) + }) + + it('rejects setting a percentage on a row that has no dates', async () => { + const seed = await seedCompany() + const id = await insertEmployee(seed, { percentage: null, from: null, to: null }) + const err = await captureError( + getPool().query(`UPDATE public.employees SET jamkning_percentage = 20 WHERE id = $1`, [id]), + ) + expectConstraintRejection(err) + expect(await readJamkning(id)).toMatchObject({ percentage: null }) + }) + + it('rejects an end date before the stored start date', async () => { + const seed = await seedCompany() + const id = await insertEmployee(seed, { percentage: 20, from: '2026-06-01', to: '2026-12-31' }) + const err = await captureError( + getPool().query(`UPDATE public.employees SET jamkning_valid_to = '2026-01-31' WHERE id = $1`, [id]), + ) + expectConstraintRejection(err) + }) + + it('accepts clearing the percentage together with the dates', async () => { + const seed = await seedCompany() + const id = await insertEmployee(seed, { percentage: 20, from: '2026-01-01', to: '2026-12-31' }) + await getPool().query( + `UPDATE public.employees + SET jamkning_percentage = NULL, jamkning_valid_from = NULL, jamkning_valid_to = NULL + WHERE id = $1`, + [id], + ) + expect(await readJamkning(id)).toMatchObject({ percentage: null, from: null, to: null }) + }) + + it('accepts clearing only the percentage (a null percentage frees the dates)', async () => { + const seed = await seedCompany() + const id = await insertEmployee(seed, { percentage: 20, from: '2026-01-01', to: '2026-12-31' }) + await getPool().query(`UPDATE public.employees SET jamkning_percentage = NULL WHERE id = $1`, [id]) + expect(await readJamkning(id)).toMatchObject({ percentage: null, from: '2026-01-01', to: '2026-12-31' }) + }) + + it('accepts replacing a complete beslut with another complete beslut', async () => { + const seed = await seedCompany() + const id = await insertEmployee(seed, { percentage: 20, from: '2026-01-01', to: '2026-06-30' }) + await getPool().query( + `UPDATE public.employees + SET jamkning_percentage = 25, jamkning_valid_from = '2026-07-01', jamkning_valid_to = '2026-12-31' + WHERE id = $1`, + [id], + ) + expect(await readJamkning(id)).toMatchObject({ percentage: 25, from: '2026-07-01', to: '2026-12-31' }) + }) +}) + +describe('the concurrent-PATCH race (#2256)', () => { + it('B blocks on A\'s row lock, then re-evaluates against A\'s committed row and fails', async () => { + const seed = await seedCompany() + const id = await insertEmployee(seed, { percentage: null, from: null, to: null }) + + // Both handlers validated the empty row (B's { jamkning_valid_to: null } + // is a no-op against it) and now issue their unconditional updates. + const a = await getClient() + const b = await getClient() + try { + await a.query('BEGIN') + await a.query( + `UPDATE public.employees + SET jamkning_percentage = 20, jamkning_valid_from = '2026-01-01', jamkning_valid_to = '2026-12-31' + WHERE id = $1`, + [id], + ) + + await b.query('BEGIN') + // Blocks on A's row lock; READ COMMITTED re-reads the row after A + // commits, so the constraint is checked against A's beslut with + // valid_to nulled. + const bUpdate = b.query(`UPDATE public.employees SET jamkning_valid_to = NULL WHERE id = $1`, [id]) + // Give B a moment to actually queue behind the lock before A commits. + await new Promise((resolve) => setTimeout(resolve, 100)) + await a.query('COMMIT') + + const err = await captureError(bUpdate) + expectConstraintRejection(err) + await b.query('ROLLBACK') + } finally { + await a.query('ROLLBACK').catch(() => {}) + await b.query('ROLLBACK').catch(() => {}) + a.release() + b.release() + } + + expect(await readJamkning(id)).toMatchObject({ percentage: 20, from: '2026-01-01', to: '2026-12-31' }) + }) +}) + +describe('legacy incomplete rows (stored before the constraint, NOT VALID)', () => { + it('seeds one by dropping and re-adding the constraint NOT VALID in one transaction', async () => { + const seed = await seedCompany() + const id = await insertLegacyIncompleteEmployee(seed) + expect(await readJamkning(id)).toMatchObject({ percentage: 15, from: '2026-01-01', to: null }) + const back = await getPool().query<{ convalidated: boolean }>( + `SELECT convalidated FROM pg_constraint WHERE conname = $1 AND conrelid = 'public.employees'::regclass`, + [CONSTRAINT], + ) + expect(back.rows).toHaveLength(1) + expect(back.rows[0].convalidated).toBe(false) + }) + + it('is refused on an unrelated edit until the beslut is completed or cleared (the NOT VALID trade-off)', async () => { + const seed = await seedCompany() + const id = await insertLegacyIncompleteEmployee(seed) + + const err = await captureError( + getPool().query(`UPDATE public.employees SET first_name = 'Ny' WHERE id = $1`, [id]), + ) + expectConstraintRejection(err) + expect(await readJamkning(id)).toMatchObject({ first_name: 'Test', percentage: 15, from: '2026-01-01', to: null }) + }) + + it('can be completed by supplying the missing end date, and is then freely editable', async () => { + const seed = await seedCompany() + const id = await insertLegacyIncompleteEmployee(seed) + await getPool().query( + `UPDATE public.employees SET first_name = 'Ny', jamkning_valid_to = '2026-12-31' WHERE id = $1`, + [id], + ) + expect(await readJamkning(id)).toMatchObject({ first_name: 'Ny', percentage: 15, from: '2026-01-01', to: '2026-12-31' }) + }) + + it('can be cleared by nulling the percentage', async () => { + const seed = await seedCompany() + const id = await insertLegacyIncompleteEmployee(seed) + await getPool().query(`UPDATE public.employees SET jamkning_percentage = NULL WHERE id = $1`, [id]) + expect(await readJamkning(id)).toMatchObject({ percentage: null, from: '2026-01-01', to: null }) + await getPool().query(`UPDATE public.employees SET first_name = 'Ny' WHERE id = $1`, [id]) + expect(await readJamkning(id)).toMatchObject({ first_name: 'Ny' }) + }) +})