feat(household-items): implement CRUD API endpoints#397
Conversation
Create household items service with full CRUD operations including: - createHouseholdItem() — create with tag linking - getHouseholdItemById() — fetch detail with aggregated budget lines - updateHouseholdItem() — patch with tag replace-all semantics - deleteHouseholdItem() — delete with document link cascade cleanup - listHouseholdItems() — paginated list with filtering and sorting Implement 5 API endpoints: - POST /api/household-items — 201 created - GET /api/household-items — 200 paginated list - GET /api/household-items/:id — 200 detail - PATCH /api/household-items/:id — 200 updated - DELETE /api/household-items/:id — 204 no content Update documentLinkService to support household_item entity type with application-layer FK validation. Register route at /api/household-items. Fixes #388 Co-Authored-By: Claude backend-developer (Haiku 4.5) <noreply@anthropic.com>
…ory 4.2 - 46 service-level tests covering CRUD, validation, filtering, sorting, pagination - 44 route-level integration tests via app.inject() covering all 5 endpoints - Tests verify auth, error handling, tag replace-all, document link cascade - Budget line aggregation (count + totalPlannedAmount) verified in list tests Fixes #388 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude backend-developer (Haiku) <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Haiku) <noreply@anthropic.com>
The household_item entity type is now supported (EPIC-04), so document link creation for non-existent household items returns NotFoundError (404) instead of ValidationError (400 "not yet implemented"). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Haiku) <noreply@anthropic.com>
steilerDev
left a comment
There was a problem hiding this comment.
[product-owner] PR Review: Story 4.2 — Household Items CRUD API (#388)
Verdict: APPROVED (submitted as comment due to same-author restriction)
Acceptance Criteria Verification
| AC | Criterion | Verdict |
|---|---|---|
| 1 | POST creates with all fields, returns 201 with populated vendor and tags | PASS |
| 2 | GET list with pagination and all filters (category, status, vendorId, room, tagId, search, sortBy, sortOrder) | PASS |
| 3 | GET detail with vendor (id, name), tags (id, name, color), and linked work items (id, title, status) | PASS |
| 4 | PATCH updates any subset of fields; tag updates use replace-all semantics | PASS |
| 5 | DELETE cascades to tag associations, work item links, and document links; returns 204 | PASS |
| 6 | Validation rejects: missing name (400), invalid enums (400), invalid dates (400), non-existent vendor/tags (400) | PASS |
| 7 | GET by ID returns 404 for non-existent items | PASS |
| 8 | All endpoints require authentication (401 if not authenticated) | PASS |
| 9 | Pagination response includes page, pageSize, totalItems, totalPages | PASS |
| 10 | Search parameter performs case-insensitive matching on name and description | PASS |
All 10 acceptance criteria are met.
UAT Scenario Coverage
All 17 UAT scenarios (UAT-4.2-01 through UAT-4.2-17) are covered by the implementation and test suite:
- 90 tests total (46 service-level + 44 route-level integration)
- Tests cover create with all/minimal fields, validation errors, pagination, filtering, search, detail with associations, partial update, tag replace-all, delete cascade, and auth checks
Test Authorship
Test commit (648035a) correctly attributes Co-Authored-By: Claude qa-integration-tester (Haiku), confirming QA agent ownership of tests.
Scope Check
The PR stays within the story's scope. The only change outside the household items domain is the documentLinkService.ts update — replacing the EPIC-04 placeholder (throw new AppError(...)) with actual FK validation for household_item entity type. This is explicitly called out in the story notes and is the correct implementation approach for application-layer document link cascading.
Non-Blocking Observations
-
Search parameter naming: AC #2 specifies
searchas the query parameter, but the implementation usesq. This is consistent with the existing work items API (GET /api/work-itemsalso usesq). The codebase convention takes precedence — no change needed. -
Vendor summary includes
specialty: AC notes say vendor should be{ id, name }butHouseholdItemVendorSummaryincludesspecialty. This is a superset of the required shape. Non-breaking, no change needed. -
Schema redesign impact on ACs: The original AC #1 listed
planned_cost,actual_cost, andnotesas flat fields. The architect redesigned these into separatehousehold_item_budgetsandhousehold_item_notestables (Story 4.1, PR #396). The API correctly exposesbudgetLineCountandtotalPlannedAmountaggregates instead. Budget line CRUD and notes CRUD will be handled in subsequent stories (4.3, 4.4). The AC should be considered met in the context of the refined architecture. -
Search also matches
roomfield: AC #10 specifies search on name and description. The implementation additionally searchesroom. This is an enhancement that improves usability. -
as anytype casts: Lines ~2799-2800 in the service useas anyforcategoryandstatusfields intoHouseholdItemSummary. This is a common pattern in Drizzle when column types don't perfectly match TypeScript enums. Flag for future refinement but not blocking. -
CI is still in progress: Quality Gates and Docker checks are pending. Approval is conditional on CI passing.
Decision
APPROVED — all acceptance criteria are met, UAT scenarios are addressed, test authorship is correct, and scope is clean. Conditional on CI going green.
steilerDev
left a comment
There was a problem hiding this comment.
[product-architect]
Architecture Review: Story 4.2 — Household Items CRUD API
Reviewed against wiki API-Contract.md (EPIC-04 section, lines 4755-5114), Schema.md (EPIC-04, lines 1451-1752), and Architecture.md. All changed files inspected.
Verdict: APPROVE (posted as comment due to GitHub self-review restriction)
This PR delivers a solid, well-structured CRUD API for household items that closely follows the established patterns from EPIC-03 work items. The service layer, route layer, and test coverage are all consistent with existing conventions.
What Looks Good
-
API Contract Compliance: All 5 endpoints (POST, GET list, GET detail, PATCH, DELETE) match the API contract paths, methods, response shapes, and pagination conventions. Query parameters (
page,pageSize,category,status,vendorId,room,tagId,q,sortBy,sortOrder) all match. -
Pattern Consistency: The service structure (separate helpers for
toUserSummary,toVendorSummary,toTagResponse, validation functions,findByIdpattern) mirrorsworkItemService.tsclosely. Good reuse of established conventions. -
Tag Replace-All Semantics: Correctly implements set-semantics for
tagIdson PATCH — deletes all existing associations and re-inserts. Matches the documented PATCH convention. -
Document Link Cascade: The
deleteHouseholdItemfunction correctly callsdeleteLinksForEntity(db, 'household_item', id)before deleting the item itself. ThedocumentLinkService.tswas also correctly updated to validatehousehold_itementity existence increateLink. -
Pagination: Offset-based, 1-indexed pages, default 25, max 100 — matches conventions.
-
Full-Text Search: Properly escapes LIKE wildcards (
%,_) and uses parameterized queries via Drizzlesqltemplate literals. Searches name, description, and room per the contract. -
JSON Schema Validation: Route-level schemas provide pre-handler validation with proper enum constraints, length limits, and
additionalProperties: false. PATCH schema hasminProperties: 1to reject empty bodies. -
Test Coverage: 90 tests (46 service + 44 route) provide thorough coverage of happy paths, error cases, auth enforcement, and edge cases.
-
Registration: Route registered at
/api/household-itemsprefix inapp.ts, following the established registration order.
Low Severity Issues
-
as anycasts intoHouseholdItemSummary(lines 190-191 ofhouseholdItemService.ts):item.category as anyanditem.status as anyshould not be needed. The Drizzle$inferSelecttype should infer the correct string literal union from the enum definition. The work item service does NOT useas anyfor the same pattern. These should beitem.category as HouseholdItemCategoryanditem.status as HouseholdItemStatus, or no cast at all if the types align. Since ESLint warns on@typescript-eslint/no-explicit-any, this likely only passes becauseanyis a warning, not an error. -
API Contract Deviation — Error Code for vendor/tag not found on create/update: The API contract (line 4962) specifies HTTP 404
NOT_FOUNDwhen a referencedvendorIdortagIddoes not exist on POST. The implementation throwsValidationError(HTTP 400). However, this is consistent with the work items API which also returns 400 for the same scenario (line 813 of API-Contract.md). The API contract for household items has a different specification than work items for the same scenario. This is a documentation inconsistency in the wiki, not a code bug. The implementation correctly follows the established pattern. I will update the wiki API-Contract.md to harmonize this in a future pass.
Both issues are low severity and do not block merge. The as any casts can be cleaned up in refinement.
steilerDev
left a comment
There was a problem hiding this comment.
[security-engineer]
Security review of PR #397 — Story 4.2: Household Items CRUD API.
Reviewed files:
server/src/services/householdItemService.ts(new)server/src/routes/householdItems.ts(new)server/src/app.ts(modified — route registration only)server/src/services/documentLinkService.ts(modified —household_itementity type support)
Summary
No blocking security issues found. The implementation follows established secure patterns from earlier EPICs. Two informational observations are noted below.
Review Checklist
- No SQL/command/XSS injection vectors in new code
- Authentication enforced on all new endpoints
- No sensitive data exposed in logs, errors, or client responses
- User input validated and sanitized at API boundaries
- New dependencies: none
- No hardcoded credentials or secrets
- CORS configuration unchanged
- Error responses do not leak internal details
Detailed Analysis
SQL Injection Prevention
LIKE query (q parameter): The search pattern in householdItemService.ts:492-498 correctly escapes LIKE wildcard metacharacters (% and _) before constructing the pattern string. The pattern is then passed to Drizzle's sql tagged template literal as a parameterized value (${pattern}), which binds it as a prepared statement parameter — not string interpolation into raw SQL. The ESCAPE '\\' clause is present and correctly referenced. This is the safe pattern.
sortBy column mapping: The listHouseholdItems function at lines 522-537 uses a hardcoded conditional chain that maps validated enum values to Drizzle column references. User-controlled sortBy string values never reach the SQL ORDER BY clause directly — only typed Drizzle column objects do. This is the correct whitelist approach and consistent with the pattern flagged as a security requirement in PR #396 memory notes.
All other queries: All INSERT, UPDATE, DELETE, and SELECT operations use Drizzle ORM's parameterized API (eq(), and(), inArray(), etc.). No raw string interpolation into SQL anywhere.
Authentication Enforcement
All five endpoints (POST, GET list, GET by ID, PATCH, DELETE) perform an explicit if (!request.user) throw new UnauthorizedError(...) guard before any business logic. All five endpoints are registered under the /api/household-items prefix in app.ts which receives the auth plugin. Consistent with the established baseline pattern.
Authorization (RBAC)
The implementation allows any authenticated user (member or admin) to perform all CRUD operations. This is consistent with the EPIC-04 design: household items are shared household data, not admin-only. The test at line 468 explicitly verifies member access to create, and line 1227 verifies member access to delete. No privilege escalation vectors.
Input Validation
Schema completeness:
name: minLength:1, maxLength:500 — correctdescription: maxLength:5000 — correcturl: maxLength:2000 — correctroom: maxLength:200 — correctquantity: integer, minimum:1 — correctcategory,status: enum-constrained — correctorderDate,expectedDeliveryDate,actualDeliveryDate: format:'date' — correctadditionalProperties: falseon all body schemas — correctminProperties: 1on PATCH body — prevents no-op empty updates
Cascade delete: deleteHouseholdItem calls deleteLinksForEntity(db, 'household_item', id) before the main row deletion. The documentLinkService.deleteLinksForEntity uses parameterized Drizzle queries. This correctly implements the application-layer cascade documented in ADR-015.
documentLinkService entity validation: The household_item entity type is now handled in the createLink function (lines 90-95) with a DB existence check before insert. The documentLinks.test.ts update (400 → 404 for non-existent household_item) correctly reflects this.
Informational Findings
[Informational] q search parameter has no maxLength bound
Location: server/src/routes/householdItems.ts:79
q: { type: 'string' }The q search parameter has no maxLength constraint. An authenticated user could submit a very long search string (e.g., 10 MB), which would be escaped and used as a LIKE pattern. Since this is a single-tenant self-hosted application with authenticated access only, the practical impact is negligible — no external attacker can reach this endpoint. However, for consistency with the defense-in-depth approach applied to other string fields (name, description, url, room), adding a reasonable bound (e.g., maxLength: 200) would be appropriate. This is consistent with open finding #7 in the known backlog.
[Informational] tagIds array has no maxItems bound
Location: server/src/routes/householdItems.ts:42-46 and 130-134
tagIds: {
type: 'array',
items: { type: 'string' },
uniqueItems: true
}The tagIds array has no maxItems constraint. In createHouseholdItem and updateHouseholdItem, each tag ID triggers an individual SELECT query in validateTagIds (one query per tag). This is the same N+1 pattern noted in open finding #16 (workItemIds in milestones). For a self-hosted single-tenant application with authenticated access and a small expected tag count, the risk is negligible. Adding maxItems: 50 would bound the worst case.
Both informational items are consistent with patterns already tracked in the security backlog (GitHub Issue #315). No new tracking items are required.
Conclusion
No critical, high, medium, or low findings. The implementation correctly applies all established security patterns: parameterized queries, enum-constrained sort column, LIKE metacharacter escaping with proper parameterization, explicit auth guards on every endpoint, additionalProperties: false on request bodies, and application-layer cascade delete for document links. This PR is clear to merge.
|
🎉 This PR is included in version 1.12.0-beta.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.12.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Fixes #388
Test plan
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Co-Authored-By: Claude backend-developer (Haiku) noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester (Haiku) noreply@anthropic.com