-
Notifications
You must be signed in to change notification settings - Fork 0
fix(#7): use auto-incrementing counter for item IDs #10
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,3 +63,48 @@ def test_delete_item(client): | |
| def test_delete_item_not_found(client): | ||
| resp = client.delete("/items/999") | ||
| assert resp.status_code == 404 | ||
|
|
||
|
|
||
| def test_no_id_collision_after_delete(client): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] test-adequacy The new test validates the primary bug scenario (single delete then create) but does not cover multi-delete scenarios, which would provide stronger regression protection. Suggested fix: Add a test that deletes multiple items then creates multiple new ones, verifying no ID collisions occur. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [info] test-docstring-convention test_no_id_collision_after_delete() includes a docstring, but none of the existing 8 test functions use docstrings. Minor convention deviation. |
||
| """IDs must never be reused after deletion.""" | ||
| resp1 = client.post("/items", json={"name": "A"}) | ||
| resp2 = client.post("/items", json={"name": "B"}) | ||
| id_a = resp1.get_json()["id"] | ||
| id_b = resp2.get_json()["id"] | ||
|
|
||
| # Delete first item | ||
| client.delete(f"/items/{id_a}") | ||
|
|
||
| # Create a new item — should NOT collide with B's id | ||
| resp3 = client.post("/items", json={"name": "C"}) | ||
| id_c = resp3.get_json()["id"] | ||
| assert id_c != id_b, "New item ID collides with existing item" | ||
|
|
||
| # Verify both remaining items exist in the list | ||
| all_items = client.get("/items").get_json()["items"] | ||
| all_ids = [item["id"] for item in all_items] | ||
| assert id_b in all_ids | ||
| assert id_c in all_ids | ||
| assert len(all_items) == 2 | ||
|
|
||
|
|
||
| def test_no_id_reuse_after_multiple_deletes(client): | ||
| """IDs stay unique after several deletions followed by several creates.""" | ||
| # Create three items | ||
| ids = [] | ||
| for name in ("X", "Y", "Z"): | ||
| resp = client.post("/items", json={"name": name}) | ||
| ids.append(resp.get_json()["id"]) | ||
|
|
||
| # Delete all three | ||
| for item_id in ids: | ||
| client.delete(f"/items/{item_id}") | ||
|
|
||
| # Create two more items — their IDs must not collide with any prior ID | ||
| new_ids = [] | ||
| for name in ("P", "Q"): | ||
| resp = client.post("/items", json={"name": name}) | ||
| new_ids.append(resp.get_json()["id"]) | ||
|
|
||
| all_assigned = ids + new_ids | ||
| assert len(all_assigned) == len(set(all_assigned)), "ID reuse detected" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] race-condition
The _new_id() function uses a non-atomic read-increment-return on a global counter, which is not thread-safe under threaded WSGI servers. However, this is a pre-existing concern — the original len(_get_items()) + 1 was equally non-thread-safe. This PR does not regress thread safety.
Suggested fix: Use itertools.count(1) with next() for an effectively atomic counter under CPython, or protect with threading.Lock if broader runtime support is needed.