fix(events): return 409 instead of 500 when a slug is taken
Correction to the reported cause: both create paths already loop
`while (await db('events').where({ slug }).first())` before inserting, so a
sequential duplicate never 500s -- it gets -1 appended. The 500 is purely the
read-then-insert race: two concurrent creates for the same name+date both
clear the check and the loser's INSERT trips events_slug_unique.
isDuplicateSlugError(), built on the existing utils/dbErrors.isUniqueViolation,
is wired into the catch of POST / and POST /:id/duplicate ->
409 { code: 'EVENT_SLUG_TAKEN' }. The predicate is deliberately narrower than
isUniqueViolation: on PG it matches err.constraint, on SQLite the specific
"UNIQUE constraint failed: ... events.slug" text. A loose message test would
misfire because knex prefixes the whole INSERT -- which always names slug --
to err.message, and events has other unique columns (share_token).
PUT /:id cannot collide: slug is in IMMUTABLE_EVENT_COLUMNS. No other
adminEvents sub-router writes slug. CreateEventPage already toasts data.error,
so no frontend change is needed.
The test makes the race deterministic without timers: it hooks knex's `query`
event and injects the colliding row the instant the route issues its
slug-existence SELECT. The route then spends a full bcrypt hash before its own
INSERT, so the injected row always lands first.
Refs testplan REPORT.md B10.
This commit is contained in:
@@ -117,6 +117,46 @@ describe('admin events CRUD endpoints (smoke)', () => {
|
||||
expect(queued).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('409s (not 500) when the slug uniqueness race is lost', async () => {
|
||||
// The route mints the slug with a read-then-insert, so two concurrent
|
||||
// creates for the same name + date both clear the existence check and
|
||||
// the loser's INSERT trips events_slug_unique. Reproduce it without a
|
||||
// timer: slip the colliding row in the instant that existence SELECT is
|
||||
// issued — the route then spends a bcrypt hash before its own INSERT.
|
||||
const { slugify } = require('../../src/utils/slug');
|
||||
const collidingSlug = `wedding-${slugify('Race Wedding')}-2026-09-02`;
|
||||
let injected = null;
|
||||
const onQuery = (q) => {
|
||||
if (injected) return;
|
||||
if (!/from\s+.?events.?\s+where\s+.?slug.?\s*=/i.test(q.sql)) return;
|
||||
injected = insertEvent(db, adminId, { slug: collidingSlug, event_name: 'Race Wedding' });
|
||||
};
|
||||
db.on('query', onQuery);
|
||||
|
||||
try {
|
||||
const res = await auth(request(app).post('/api/admin/events')).send({
|
||||
event_type: 'wedding',
|
||||
event_name: 'Race Wedding',
|
||||
event_date: '2026-09-02',
|
||||
customer_name: 'Client Person',
|
||||
customer_email: '[email protected]',
|
||||
admin_email: '[email protected]',
|
||||
require_password: false,
|
||||
is_draft: true,
|
||||
});
|
||||
|
||||
expect(injected).not.toBeNull(); // the race was actually injected
|
||||
await injected;
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.code).toBe('EVENT_SLUG_TAKEN');
|
||||
expect(res.body.error).toMatch(/already exists/i);
|
||||
// Only the injected row survives — no half-created duplicate.
|
||||
expect(await db('events').where({ slug: collidingSlug })).toHaveLength(1);
|
||||
} finally {
|
||||
db.removeListener('query', onQuery);
|
||||
}
|
||||
});
|
||||
|
||||
it('400s on an invalid event type', async () => {
|
||||
const res = await auth(request(app).post('/api/admin/events')).send({
|
||||
event_type: 'not-a-real-type',
|
||||
|
||||
@@ -18,6 +18,7 @@ const { validatePasswordInContext, getBcryptRounds } = require('../../utils/pass
|
||||
const logger = require('../../utils/logger');
|
||||
const { sanitizeForLog, sanitizeValidationErrors } = require('../../utils/sanitizeForLog');
|
||||
const { errorResponse } = require('../../utils/routeHelpers');
|
||||
const { isUniqueViolation } = require('../../utils/dbErrors');
|
||||
const { buildShareLinkVariants } = require('../../services/shareLinkService');
|
||||
const { parseBooleanInput } = require('../../utils/parsers');
|
||||
const eventTypeService = require('../../services/eventTypeService');
|
||||
@@ -31,6 +32,31 @@ const downloadZipService = require('../../services/downloadZipService');
|
||||
const { resolveEventFeedbackDefaults, applyFeedbackDefaults, KEYBIND_MODES } = require('../../services/feedbackDefaults');
|
||||
const { validateHeroImageAnchor, getEventFieldRequirements, readBooleanSetting, getDownloadProtectionDefaults, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, getCustomerPhoneFromPayload, isPhoneFieldEnabled, mapEventForApi, hasCustomerContactColumns, deleteEventCascade, SLIDESHOW_TRANSITIONS, SLIDESHOW_COLORFILTERS } = require('./helpers');
|
||||
|
||||
/**
|
||||
* `events.slug` is UNIQUE, and both routes that mint one do a read-then-insert
|
||||
* (`while (await db('events').where({ slug }).first())`) — a check two
|
||||
* concurrent requests for the same name + date can both pass. The loser's
|
||||
* INSERT then raises the unique violation, which surfaced as a raw 500 with
|
||||
* "duplicate key value violates unique constraint events_slug_unique".
|
||||
*
|
||||
* Detect that one constraint specifically so the client gets an actionable
|
||||
* 409 instead. Deliberately narrower than isUniqueViolation alone: the events
|
||||
* table carries other unique columns (share_token), and a loose message test
|
||||
* would misfire because knex prefixes the whole INSERT — which always mentions
|
||||
* `slug` — to err.message.
|
||||
*/
|
||||
function isDuplicateSlugError(error) {
|
||||
if (!isUniqueViolation(error)) return false;
|
||||
// Postgres names the constraint; sqlite names the offending column.
|
||||
if (error.constraint) return /slug/i.test(error.constraint);
|
||||
return /unique constraint failed:.*\bevents\.slug\b/i.test(String(error.message || ''));
|
||||
}
|
||||
|
||||
const DUPLICATE_SLUG_RESPONSE = {
|
||||
error: 'An event with this name already exists. Change the event name or date and try again.',
|
||||
code: 'EVENT_SLUG_TAKEN',
|
||||
};
|
||||
|
||||
/**
|
||||
* Validate a gallery password the admin re-typed, against the SAME policy
|
||||
* event creation applies.
|
||||
@@ -810,6 +836,10 @@ module.exports = (router) => {
|
||||
created_at: new Date().toISOString()
|
||||
});
|
||||
} catch (error) {
|
||||
if (isDuplicateSlugError(error)) {
|
||||
logger.warn('Event creation lost the slug race', { error: error.message });
|
||||
return res.status(409).json(DUPLICATE_SLUG_RESPONSE);
|
||||
}
|
||||
errorResponse(res, error, 500, 'Failed to create event');
|
||||
}
|
||||
});
|
||||
@@ -1470,6 +1500,10 @@ module.exports = (router) => {
|
||||
is_draft: true,
|
||||
});
|
||||
} catch (error) {
|
||||
if (isDuplicateSlugError(error)) {
|
||||
logger.warn('Event duplication lost the slug race', { error: error.message });
|
||||
return res.status(409).json(DUPLICATE_SLUG_RESPONSE);
|
||||
}
|
||||
errorResponse(res, error, 500, 'Failed to duplicate event');
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user