From abe8ac8184dd825e355d7244f5985401b1f5cd49 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 17:17:29 -0500 Subject: [PATCH] test(filters): merge the duplicate itemFilters import (#308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit backend/tests/unit/itemFilters.test.ts had two separate import statements from ../../src/itemFilters; merged into one, with nothing else in the file changed. A companion fix to backend/src/routes/items.ts — reading the by-id route's id with the existing readId helper instead of Number(req.params.id), to close the leniency Number() introduced toward inputs like '5.0', '1e2' and '0x10' — was tried and then reverted, because backend/tests/integration/errorHandling.integration.test.ts deliberately drives that exact route with a non-numeric id to prove that asyncRoute plus the error middleware turn a rejected handler into a 500 rather than hanging the request, and readId's stricter parse would answer 404 before that mechanism ever runs, leaving the test green while silently deleting the coverage it exists for; the route now carries a comment explaining why Number() stays and pointing at #307 for giving that test another trigger before making the switch. Co-Authored-By: Claude Opus 5 --- backend/src/routes/items.ts | 8 ++++++++ backend/tests/unit/itemFilters.test.ts | 3 +-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/backend/src/routes/items.ts b/backend/src/routes/items.ts index c94064b..6cb826c 100755 --- a/backend/src/routes/items.ts +++ b/backend/src/routes/items.ts @@ -93,6 +93,14 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => { })); router.get('/:id', asyncRoute(async (req: Request, res: Response) => { + // Number() rather than readId(), and that is deliberate rather than an + // oversight. readId would be stricter and would match every other id-taking + // route (#207) — but errorHandling.integration.test.ts drives this exact + // route with a non-numeric id to prove that asyncRoute plus the error + // middleware answer 500 rather than leaving the request hanging, and a + // stricter parse here would leave that test green while removing the thing + // it tests. Switching this over means giving that test another trigger in + // the same change. See #307. const rows = await publicItemQuery() .where('i.id', '=', Number(req.params.id)) .where((eb) => notPending(eb)) diff --git a/backend/tests/unit/itemFilters.test.ts b/backend/tests/unit/itemFilters.test.ts index 4986357..eda7a28 100644 --- a/backend/tests/unit/itemFilters.test.ts +++ b/backend/tests/unit/itemFilters.test.ts @@ -1,6 +1,5 @@ -import { parseItemFilters, FilterError } from '../../src/itemFilters'; +import { parseItemFilters, FilterError, itemFilterExpressions, ItemFilters } from '../../src/itemFilters'; import { db } from '../../src/db'; -import { itemFilterExpressions, ItemFilters } from '../../src/itemFilters'; describe('parseItemFilters', () => { it('returns empty filters for an empty query', () => {