fix(db): concurrency-safe set_user_resource_selection + enforce OpenCRE (Part of #586) - #1008
Conversation
…RE in selection (Part of OWASP#586)
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughThe user-resource PUT endpoint adds ChangesUser resource selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
application/tests/user_resources_api_test.py (1)
273-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest empty selection replacement from existing data.
Line 276 creates a user with no saved selection. A handler that treats
[]as a no-op would still pass this test. Seed a non-empty selection before the PUT. Then issue a GET and assert that it returns[].Proposed test update
def test_put_empty_selection_stays_empty(self) -> None: - # Empty PUT must remain [] (PR3 treats [] as "show all") — OpenCRE must - # NOT be injected, or that would become "show only OpenCRE". self.collection.upsert_user( google_sub="sub-1", email="a@x.com", display_name="U" ) + user = self.collection.get_user_by_sub("sub-1") + self.collection.set_user_resource_selection(user.id, ["ASVS"]) with patch.dict( os.environ, { @@ resp = client.put("/rest/v1/user/resources", json={"selected": []}) self.assertEqual(resp.status_code, 200) self.assertEqual(json.loads(resp.data)["selected"], []) + get = client.get("/rest/v1/user/resources") + self.assertEqual(json.loads(get.data)["selected"], [])As per coding guidelines, “Use test-first development for new behavior and importers.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/tests/user_resources_api_test.py` around lines 273 - 291, Update test_put_empty_selection_stays_empty to seed the user with a non-empty resource selection before the PUT, then replace it with {"selected": []}. Afterward, issue a GET to /rest/v1/user/resources and assert its selected value is [], proving the empty PUT replaces existing data rather than being treated as a no-op.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@application/database/db.py`:
- Around line 1356-1378: Update the replacement flow containing _replace to lock
the parent User row with a row-level write lock before deleting or inserting
UserResourceSelection records, and retain that lock through commit so concurrent
replacements serialize. Capture the replacement’s selected rows before
committing and use that snapshot for the response instead of rereading
selections afterward. Add a PostgreSQL test using two independent sessions with
disjoint selections to verify replacement semantics and serialized writes.
---
Nitpick comments:
In `@application/tests/user_resources_api_test.py`:
- Around line 273-291: Update test_put_empty_selection_stays_empty to seed the
user with a non-empty resource selection before the PUT, then replace it with
{"selected": []}. Afterward, issue a GET to /rest/v1/user/resources and assert
its selected value is [], proving the empty PUT replaces existing data rather
than being treated as a no-op.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: ffcf7ee0-d6d8-45c2-8475-05c4c260d890
📒 Files selected for processing (4)
application/database/db.pyapplication/tests/user_model_test.pyapplication/tests/user_resources_api_test.pyapplication/web/web_main.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/tests/user_model_test.py (1)
165-206: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the required retry invariants.
The transient test does not assert that
rollback()occurs before the retry. Its mockedIntegrityErrorleaves the session usable, so the test can pass without a rollback. The persistent-error test does not assert thatcommit()runs twice, so it can pass if the first error is re-raised without a retry.Count
rollback()in the transient case. Count two commit attempts in the persistent case.Proposed test assertions
+ real_rollback = self.collection.session.rollback + rollbacks = {"n": 0} + + def recording_rollback() -> None: + rollbacks["n"] += 1 + real_rollback() + - with patch.object(self.collection.session, "commit", side_effect=flaky_commit): + with patch.object( + self.collection.session, "commit", side_effect=flaky_commit + ), patch.object( + self.collection.session, "rollback", side_effect=recording_rollback + ): result = self.collection.set_user_resource_selection( user.id, ["ASVS", "CWE"] ) self.assertEqual(sorted(result), ["ASVS", "CWE"]) self.assertEqual(calls["n"], 2) # retried exactly once + self.assertEqual(rollbacks["n"], 1) + calls = {"n": 0} def always_raise(*args: Any, **kwargs: Any) -> None: + calls["n"] += 1 raise IntegrityError("stmt", {}, Exception("uq_user_resource_selection")) with patch.object(self.collection.session, "commit", side_effect=always_raise): with self.assertRaises(IntegrityError): self.collection.set_user_resource_selection(user.id, ["ASVS"]) + self.assertEqual(calls["n"], 2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/tests/user_model_test.py` around lines 165 - 206, Strengthen the tests for set_user_resource_selection: in test_set_resource_selection_recovers_from_integrity_error, mock and count session.rollback() and assert it occurs before the second commit attempt; in test_set_resource_selection_reraises_on_persistent_integrity_error, count commit invocations and assert exactly two attempts occur before the IntegrityError propagates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@application/tests/user_model_test.py`:
- Line 241: Retain the broad exception handler in the worker test flow and add a
targeted BLE001 suppression on the except clause, including a brief reason that
the failure is propagated to the parent test thread.
---
Outside diff comments:
In `@application/tests/user_model_test.py`:
- Around line 165-206: Strengthen the tests for set_user_resource_selection: in
test_set_resource_selection_recovers_from_integrity_error, mock and count
session.rollback() and assert it occurs before the second commit attempt; in
test_set_resource_selection_reraises_on_persistent_integrity_error, count commit
invocations and assert exactly two attempts occur before the IntegrityError
propagates.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 840997a0-ed63-457f-85aa-38268892a61c
📒 Files selected for processing (2)
application/database/db.pyapplication/tests/user_model_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- application/database/db.py
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Review — concurrency-safe resource selection (#1008 / #586 follow-up)
Looks good. Addresses the post-#1005 ask cleanly.
What works
- Per-user serialization:
UserrowFOR UPDATEbefore delete/insert so two disjoint concurrent PUTs cannot merge selections. Documented SQLite no-op is fine for local/tests. - IntegrityError recovery: rollback + exactly one retry; persistent failure re-raises. Matches
upsert_userstyle. - Return under lock:
get_user_resource_selectionbeforecommitso the response cannot race a later writer. - OpenCRE injection: non-empty selections always include
OPENCRE_STANDARD_NAME; empty[]stays empty (“show all”) — correct, and covered by API tests. - Tests: flaky-commit retry, persistent IntegrityError, Postgres lock-blocking test (skip on SQLite), OpenCRE inject/dedupe/empty cases.
CodeRabbit’s serialize note is addressed by the row lock; BLE001 noqa on the worker is justified.
Approve. ~11 behind main — light rebase optional before merge; unlikely to conflict on these files. Happy to rebase-merge when you want it landed.
|
Merged (rebase) after updating onto |
2 participants
What & why
The small follow-up @northdpole asked for on the #586 resource-selection work.
Two things, both scoped to the selection write path:
Concurrency recovery in
set_user_resource_selection. The method does adelete-then-insert; two concurrent PUTs for the same user could both delete
then insert the same
(user_id, standard_name), and the second commit raisedIntegrityErroronuq_user_resource_selection→ a 500. It now rolls back andretries the replace exactly once (after the competing txn has committed,
the retry deletes its rows and inserts cleanly). A second failure propagates —
no retry loop. Mirrors the existing
upsert_userrecovery pattern."OpenCRE is always included" enforced at write. A non-empty selection now
always persists
OPENCRE_STANDARD_NAME, soGET /user/resourcesreflects theinvariant the read filter and UI copy already assume. Done in the PUT handler
(API layer) — not in
db.py, to avoid a circular import viagap_analysis.An empty PUT stays
[](which the/standardsfilter treats as "showeverything"); injecting OpenCRE there would wrongly narrow it to "OpenCRE only".
Scope
db.py(recovery) +web_main.pyPUT handler (OpenCRE) + tests. No migration,no model change, no frontend, no auth-route change.
Tests
IntegrityErroronce → themethod rolls back, retries once, returns the correct deduped selection.
empty PUT stays empty. Existing exact-list PUT tests updated for the invariant.
uq_user_resource_selectionconstraint is exercised by theexisting unique-constraint test.
Verification
Verified on real Postgres (constraint-driven): 30 tests green incl. the recovery
and OpenCRE cases;
blackclean; no new mypy errors.