fix(#28): reject unknown fields in POST and PATCH /items endpoints - #29
fix(#28): reject unknown fields in POST and PATCH /items endpoints#29fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
Both create_item() and update_item() silently dropped unknown JSON
fields while returning HTTP 200, causing callers to believe updates
persisted when they did not. This is a data-loss risk for clients
sending fields like warehouse_bin that the API does not recognize.
Add allowed-field validation to both endpoints: POST /items accepts
only {"name"}, PATCH /items/:id accepts only {"name", "done"}.
Requests with unknown fields now return HTTP 400 with an error
message listing the unrecognized field names.
Add tests covering unknown-only fields, mixed known/unknown fields,
and valid-only fields for both endpoints.
Note: pre-existing ruff lint errors (I001 import sorting, UP017
timezone.utc alias) exist in unchanged lines and are not addressed
by this change.
Closes #28
|
🤖 Finished Review · ✅ Success · Started 7:10 PM UTC · Completed 7:33 PM UTC Commit: |
ReviewFindingsMedium
Low
Labels: PR modifies API endpoint request validation logic and fixes a reported bug Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
|
||
| data = request.get_json() | ||
| allowed = {"name", "done"} | ||
| unknown = set(data.keys()) - allowed |
There was a problem hiding this comment.
[medium] nil/null handling
In update_item, request.get_json() can return None (empty body, wrong content type). The new validation at line 52 calls set(data.keys()) without a null guard, causing an unhandled AttributeError and HTTP 500. Unlike create_item, which guards with 'if not data or "name" not in data:' before the field validation block, update_item has no equivalent check.
Suggested fix: Add 'if not data: return jsonify(error="request body is required"), 400' after 'data = request.get_json()' and before the 'allowed' set definition.
| if not data or "name" not in data: | ||
| return jsonify(error="name is required"), 400 | ||
|
|
||
| allowed = {"name"} |
There was a problem hiding this comment.
[medium] scope-creep
POST /items validation logic was added but is not authorized by issue #28. The issue exclusively describes the PATCH /items/:id bug. While POST exhibits the same silent-drop behavior, the fix extends beyond the linked issue's scope.
Suggested fix: Update issue #28 to include POST /items, or split the POST changes into a separate issue and PR.
| allowed = {"name"} | ||
| unknown = set(data.keys()) - allowed | ||
| if unknown: | ||
| return jsonify(error=f"Unknown fields: {', '.join(sorted(unknown))}"), 400 |
There was a problem hiding this comment.
[low] error-message-format
Error messages at lines 32 and 54 begin with uppercase 'Unknown fields' but the established codebase pattern uses lowercase (e.g., 'name is required', 'item not found').
Suggested fix: Change both to lowercase: error=f"unknown fields: {', '.join(sorted(unknown))}"
fullsend-ai-review
Bot
added
area/api
1 participant
Summary
POST /itemsandPATCH /items/:idendpoints so requests containing unknown JSON fields return HTTP 400 with a descriptive error instead of silently dropping the fields and returning 200POST /itemsaccepts onlyname;PATCH /items/:idaccepts onlynameanddoneRoot cause
Both
create_item()andupdate_item()inapp.pyonly checked for known fields (name,done) and ignored any additional keys in the request body. This caused silent data loss — callers believed updates persisted when they did not.Testing
All 13 tests pass (
pytest -v). Pre-existing ruff lint errors (import sorting,timezone.utcalias) are on unchanged lines and are not addressed by this change.Closes #28
Post-script verification
agent/28-reject-unknown-fields)88e54cc8611dff32b36a33934937eb8ab394eeb7..HEAD)