Skip to content

fix(cocktails): add timeout, UA header, clearer upstream errors and request logging #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ModulosPB merged 1 commit into from
Apr 17, 2026
Merged

fix(cocktails): add timeout, UA header, clearer upstream errors and request logging #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,27 @@ import { errorHandler } from './middleware/errorHandler.js';
const __dirname = path.dirname(fileURLToPath(import.meta.url));
const publicDir = path.resolve(__dirname, '..', 'public');

export function createApp() {
function requestLogger(req, res, next) {
const start = Date.now();
res.on('finish', () => {
const ms = Date.now() - start;
console.log(
`[${new Date().toISOString()}] ${req.method} ${req.originalUrl} -> ${res.statusCode} ${ms}ms`,
);
});
next();
}

export function createApp({ logRequests = process.env.NODE_ENV !== 'test' } = {}) {
const app = express();

app.disable('x-powered-by');
app.use(express.json());

if (logRequests) {
app.use(requestLogger);
}

app.use(express.static(publicDir));

app.use('/api', rootRouter);
Expand Down
87 changes: 65 additions & 22 deletions src/routes/cocktails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ const router = Router();

const BASE = 'https://www.thecocktaildb.com/api/json/v1/1';
const TTL_MS = 10 * 60 * 1000;
const FETCH_TIMEOUT_MS = 10_000;
const USER_AGENT = 'api00/0.1.0 (+https://github.com/ModulosPB/api00)';

const cache = new Map();

export function _clearCocktailsCache() {
Expand Down Expand Up @@ -32,35 +35,75 @@ function parseDrink(drink) {
};
}

router.get('/', async (req, res, next) => {
function isTimeoutError(err) {
return err?.name === 'TimeoutError' || err?.name === 'AbortError';
}

router.get('/', async (req, res) => {
Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Removal of outer try/catch + next(err) in Express 4 async handler causes unhandled promise rejections

The old code wrapped the entire route handler body in try { ... } catch (err) { return next(err); }, which is the standard pattern for safely handling errors in Express 4 async route handlers (Express 4 does not automatically catch rejected promises from async handlers). The new code removes this outer safety net and also removes the next parameter entirely. Any unexpected runtime error thrown outside the two specific try/catch blocks (lines 63-83 and 94-101) — such as in parseDrink at line 104 or res.json() at line 106 — will become an unhandled promise rejection instead of being routed to the errorHandler middleware (src/middleware/errorHandler.js).

A concrete scenario: if the upstream API returns { drinks: [null] }, drinks.map(parseDrink) will call parseDrink(null), which accesses null['strIngredient1'] at src/routes/cocktails.js:19, throwing a TypeError. With no outer catch, this crashes or leaves the request hanging.

Prompt for agents 
The route handler at src/routes/cocktails.js:42 is an async function mounted on Express 4 (which does NOT auto-catch rejected promises from async handlers). The old code had try { ... } catch (err) { return next(err); } wrapping all logic, forwarding unexpected errors to the Express error handler middleware. The new code removed this pattern and also dropped the `next` parameter.

To fix: either (1) re-add the `next` parameter and wrap the handler body in a top-level try/catch that calls `next(err)`, or (2) use an async error-wrapping utility (e.g. express-async-errors or a custom wrapper). The specific try/catch blocks for fetch and upstream.json can remain, but an outer catch is needed for everything else — particularly lines 103-106 where parseDrink could throw on malformed upstream data (e.g. null elements in the drinks array).
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

const letter = String(req.query.letter ?? 'a').toLowerCase();
if (!/^[a-z]$/.test(letter)) {
return res.status(400).json({
error: 'Query parameter "letter" must be a single character a-z.',
});
}

const cached = cache.get(letter);
if (cached && Date.now() - cached.at < TTL_MS) {
return res.json(cached.data);
}

if (typeof fetch !== 'function') {
return res.status(500).json({
error: 'Global fetch is not available. Node.js >= 18 is required.',
});
}

const url = `${BASE}/search.php?f=${letter}`;
let upstream;
try {
const letter = String(req.query.letter ?? 'a').toLowerCase();
if (!/^[a-z]$/.test(letter)) {
return res.status(400).json({
error: 'Query parameter "letter" must be a single character a-z.',
upstream = await fetch(url, {
signal: AbortSignal.timeout(FETCH_TIMEOUT_MS),
headers: {
Accept: 'application/json',
'User-Agent': USER_AGENT,
},
});
} catch (err) {
if (isTimeoutError(err)) {
console.error(`[cocktails] upstream timed out after ${FETCH_TIMEOUT_MS}ms: ${url}`);
return res.status(504).json({
error: `Upstream thecocktaildb timed out after ${FETCH_TIMEOUT_MS}ms.`,
});
}
console.error(`[cocktails] upstream fetch failed: ${err?.message || err}`);
return res.status(502).json({
error: 'Upstream thecocktaildb unreachable.',
detail: err?.message || String(err),
});
}

const cached = cache.get(letter);
if (cached && Date.now() - cached.at < TTL_MS) {
return res.json(cached.data);
}
if (!upstream.ok) {
console.error(`[cocktails] upstream returned ${upstream.status} for ${url}`);
return res.status(502).json({
error: 'Upstream thecocktaildb error',
status: upstream.status,
});
}

const upstream = await fetch(`${BASE}/search.php?f=${letter}`);
if (!upstream.ok) {
return res.status(502).json({
error: 'Upstream thecocktaildb error',
status: upstream.status,
});
}
const body = await upstream.json();
const drinks = Array.isArray(body?.drinks) ? body.drinks : [];
const data = drinks.map(parseDrink);
cache.set(letter, { at: Date.now(), data });
return res.json(data);
let body;
try {
body = await upstream.json();
} catch (err) {
return next(err);
console.error(`[cocktails] upstream returned non-JSON body: ${err?.message || err}`);
return res.status(502).json({
error: 'Upstream thecocktaildb returned an invalid JSON response.',
});
}

const drinks = Array.isArray(body?.drinks) ? body.drinks : [];
const data = drinks.map(parseDrink);
cache.set(letter, { at: Date.now(), data });
return res.json(data);
});

export default router;
58 changes: 54 additions & 4 deletions tests/cocktails.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('GET /api/cocktails', () => {
const res = await request(app).get('/api/cocktails?letter=m');

expect(res.status).toBe(200);
expect(fetchMock).toHaveBeenCalledWith(
expect(fetchMock.mock.calls[0][0]).toBe(
'https://www.thecocktaildb.com/api/json/v1/1/search.php?f=m',
);
expect(res.body).toHaveLength(1);
Expand Down Expand Up @@ -73,7 +73,7 @@ describe('GET /api/cocktails', () => {
const app = createApp();
const res = await request(app).get('/api/cocktails');
expect(res.status).toBe(200);
expect(fetchMock).toHaveBeenCalledWith(
expect(fetchMock.mock.calls[0][0]).toBe(
'https://www.thecocktaildb.com/api/json/v1/1/search.php?f=a',
);
});
Expand All @@ -99,15 +99,65 @@ describe('GET /api/cocktails', () => {
expect(res.body.error).toMatch(/thecocktaildb/i);
});

it('returns 504 when upstream fetch times out / is aborted', async () => {
const abortErr = new Error('The operation was aborted due to timeout');
abortErr.name = 'TimeoutError';
vi.stubGlobal('fetch', vi.fn().mockRejectedValue(abortErr));
const app = createApp();
const res = await request(app).get('/api/cocktails?letter=d');
expect(res.status).toBe(504);
expect(res.body.error).toMatch(/timed out/i);
});

it('returns 502 when upstream fetch throws a generic network error', async () => {
vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('ENOTFOUND')));
const app = createApp();
const res = await request(app).get('/api/cocktails?letter=e');
expect(res.status).toBe(502);
expect(res.body.error).toMatch(/unreachable/i);
expect(res.body.detail).toBe('ENOTFOUND');
});

it('returns 502 when upstream body is not valid JSON', async () => {
vi.stubGlobal(
'fetch',
vi.fn().mockResolvedValue({
ok: true,
json: async () => {
throw new SyntaxError('Unexpected token < in JSON');
},
}),
);
const app = createApp();
const res = await request(app).get('/api/cocktails?letter=f');
expect(res.status).toBe(502);
expect(res.body.error).toMatch(/invalid json/i);
});

it('sends a User-Agent header and an AbortSignal to the upstream', async () => {
const fetchMock = vi.fn().mockResolvedValue({
ok: true,
json: async () => ({ drinks: [] }),
});
vi.stubGlobal('fetch', fetchMock);
const app = createApp();
await request(app).get('/api/cocktails?letter=g');
expect(fetchMock).toHaveBeenCalledTimes(1);
const [, options] = fetchMock.mock.calls[0];
expect(options.headers['User-Agent']).toMatch(/api00/);
expect(options.headers.Accept).toBe('application/json');
expect(options.signal).toBeDefined();
});

it('caches responses per letter (upstream called once for repeated requests)', async () => {
const fetchMock = vi.fn().mockResolvedValue({
ok: true,
json: async () => ({ drinks: [] }),
});
vi.stubGlobal('fetch', fetchMock);
const app = createApp();
await request(app).get('/api/cocktails?letter=c');
await request(app).get('/api/cocktails?letter=c');
await request(app).get('/api/cocktails?letter=h');
await request(app).get('/api/cocktails?letter=h');
expect(fetchMock).toHaveBeenCalledTimes(1);
});
});
Loading