feat(api): per-user resource selection endpoint — Part of #586 - #981
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds feature-gated GET and PUT endpoints for per-user resource selections. The change adds session-based user resolution, validation, normalization, persistence behavior, no-store headers, OpenAPI definitions, documentation, and tests. ChangesUser resource selection API
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (2)
application/web/web_main.py (2)
1233-1246: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider disabling caching for per-user selection responses.
Neither
GETnorPUT /rest/v1/user/resourcessetsCache-Control, so a per-user list could be cached by an intermediary proxy or browser (e.g., shared-computer back/forward cache) and served to a different session.Also applies to: 1249-1269
🤖 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/web/web_main.py` around lines 1233 - 1246, Disable caching for both get_user_resources and the corresponding PUT user-resources handler by adding an explicit no-cache/no-store Cache-Control response header to their JSON responses. Ensure the header applies to authenticated per-user selections, including the feature-disabled fallback where applicable.
846-882: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten type hints for mypy compliance.
feature_enabled_or_default(is_enabled: Any, default_factory: Any) -> Anyand_resolve_current_user(database):useAny/no annotations, unlike the fully-typedupsert_user/get_user_by_subindb.py. As per coding guidelines,**/*.pyshould "Runmake mypyfor Python type checking" — looseAnytyping here defeats that check for callable misuse (e.g. a non-callabledefault_factory).def feature_enabled_or_default( is_enabled: Callable[[], bool], default_factory: Callable[[], Any] ) -> Callable[[Callable], Callable]: ... def _resolve_current_user(database: "db.Node_collection") -> Optional["db.User"]: ...🤖 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/web/web_main.py` around lines 846 - 882, Tighten the annotations on feature_enabled_or_default by replacing Any with callable types for is_enabled and default_factory and an appropriately typed decorator return, importing the required typing symbols. Annotate _resolve_current_user’s database parameter with db.Node_collection and its return value with Optional[db.User], preserving the existing user lookup and creation behavior.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 1101-1125: Update set_user_resource_selection to handle concurrent
writes using the same IntegrityError recovery pattern established by
upsert_user: catch the insert/commit failure, roll back the session, and retry
the replacement operation so concurrent PUTs do not surface an unhandled error.
Preserve deduplication and return the refreshed selection after a successful
commit.
In `@application/web/openapi_registry.py`:
- Around line 333-340: Update put_user_resources in
application/web/openapi_registry.py to configure PathSpec request-body
generation for a required JSON object containing a required selected array of
non-empty strings, then update docs/api/openapi.yaml at lines 516-533 with the
matching required application/json requestBody schema.
In `@application/web/web_main.py`:
- Around line 1256-1263: Normalize each string in selected by trimming leading
and trailing whitespace before passing it to set_user_resource_selection in
put_user_resources. Validate and persist the normalized values so equivalent
entries such as whitespace-padded and unpadded names deduplicate consistently.
---
Nitpick comments:
In `@application/web/web_main.py`:
- Around line 1233-1246: Disable caching for both get_user_resources and the
corresponding PUT user-resources handler by adding an explicit no-cache/no-store
Cache-Control response header to their JSON responses. Ensure the header applies
to authenticated per-user selections, including the feature-disabled fallback
where applicable.
- Around line 846-882: Tighten the annotations on feature_enabled_or_default by
replacing Any with callable types for is_enabled and default_factory and an
appropriately typed decorator return, importing the required typing symbols.
Annotate _resolve_current_user’s database parameter with db.Node_collection and
its return value with Optional[db.User], preserving the existing user lookup and
creation behavior.
🪄 Autofix (Beta)
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
Run ID: 688f202a-5958-4507-a9d8-43f0df37825c
📒 Files selected for processing (9)
application/database/db.pyapplication/tests/user_model_test.pyapplication/tests/user_resources_api_test.pyapplication/tests/web_main_test.pyapplication/web/openapi_registry.pyapplication/web/web_main.pydocs/api/openapi.yamlmigrations/versions/a1b2c3d4e5f6_add_users_and_resource_selection.pymigrations/versions/f0e1d2c3b4a5_merge_heads_before_users.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
application/web/openapi_registry.py (1)
307-334: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the
401response for both resource endpoints.
web_main.pyreturns401for anonymous requests when both feature flags are enabled, but thesePathSpecs document only200and, forPUT,400. Add a401response to both entries and keepdocs/api/openapi.yamlsynchronized so clients can discover the authentication failure.Also applies to: 336-343
🤖 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/web/openapi_registry.py` around lines 307 - 334, Add a documented 401 response to both resource endpoint PathSpec entries, including the GET entry identified by get_user_resources and the corresponding PUT entry, while preserving their existing responses. Regenerate or update docs/api/openapi.yaml so the OpenAPI specification remains synchronized and exposes the authentication failure.docs/api/openapi.yaml (2)
510-515: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRequire
selectedin both response schemas.The documented response is
{"selected": [...]}, but omittingrequired: [selected]allows{}according to OpenAPI.Proposed schema fix
type: object + required: + - selected properties: selected:Apply this to both the GET and PUT response schemas.
Also applies to: 542-547
🤖 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 `@docs/api/openapi.yaml` around lines 510 - 515, Add `required: [selected]` to both GET and PUT response object schemas containing the `selected` property in the OpenAPI definition, ensuring each documented response requires that field while preserving its existing array-of-string type.
501-505: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the conditional 401 behavior.
The endpoint contract says anonymous users receive
401when both login and MyOpenCRE flags are enabled, but neither operation documents that response. Clarify the GET description and add a401response to both GET and PUT; when either flag is disabled, document the empty-selection fallback instead.Also applies to: 536-549
🤖 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 `@docs/api/openapi.yaml` around lines 501 - 505, Update the selected-standards GET and PUT OpenAPI definitions to document that authenticated access is required when both login and MyOpenCRE are enabled, including a 401 response for anonymous users. Clarify each GET/PUT description to state that the endpoint returns an empty selection when either feature flag is disabled, and add the corresponding 401 response entries to both operations.
🤖 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.
Outside diff comments:
In `@application/web/openapi_registry.py`:
- Around line 307-334: Add a documented 401 response to both resource endpoint
PathSpec entries, including the GET entry identified by get_user_resources and
the corresponding PUT entry, while preserving their existing responses.
Regenerate or update docs/api/openapi.yaml so the OpenAPI specification remains
synchronized and exposes the authentication failure.
In `@docs/api/openapi.yaml`:
- Around line 510-515: Add `required: [selected]` to both GET and PUT response
object schemas containing the `selected` property in the OpenAPI definition,
ensuring each documented response requires that field while preserving its
existing array-of-string type.
- Around line 501-505: Update the selected-standards GET and PUT OpenAPI
definitions to document that authenticated access is required when both login
and MyOpenCRE are enabled, including a 401 response for anonymous users. Clarify
each GET/PUT description to state that the endpoint returns an empty selection
when either feature flag is disabled, and add the corresponding 401 response
entries to both operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 55780973-4784-49cb-b6fd-8379a657de19
📒 Files selected for processing (5)
application/database/db.pyapplication/tests/user_resources_api_test.pyapplication/web/openapi_registry.pyapplication/web/web_main.pydocs/api/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- application/tests/user_resources_api_test.py
- application/web/web_main.py
- application/database/db.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/database/db.py (1)
2697-2706: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftFix inconsistent return type and race condition on concurrent inserts.
There are two issues in this segment:
- Inconsistent return type: The function returns
emb(a singleEmbeddingsobject) when inserting a new record, but returnsexisting(a list ofEmbeddingsobjects, sinceget_embeddinguses.all()) at line 2714. This inconsistency will likely causeAttributeErrororTypeErrorin callers.- Race condition: If two requests concurrently add an embedding for the same document, both will evaluate
if not existing:as true and attempt to insert. If there is a unique constraint, this will raise an unhandledIntegrityError(unlikeupsert_userwhich safely recovers). If there is no constraint, duplicate rows will be created.Consider returning
existing[0]in the update branch, and wrapping theself.session.commit()in atry/except IntegrityErrorblock to safely handle concurrent inserts.🤖 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/database/db.py` around lines 2697 - 2706, Update the embedding upsert function so both insert and update paths return a single Embeddings object, specifically existing[0] after updating rather than the existing list. Protect the new-record self.session.commit() with IntegrityError handling for concurrent inserts, roll back the session, retrieve the now-existing embedding, and return it instead of propagating the conflict or creating duplicates.
🤖 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.
Outside diff comments:
In `@application/database/db.py`:
- Around line 2697-2706: Update the embedding upsert function so both insert and
update paths return a single Embeddings object, specifically existing[0] after
updating rather than the existing list. Protect the new-record
self.session.commit() with IntegrityError handling for concurrent inserts, roll
back the session, retrieve the now-existing embedding, and return it instead of
propagating the conflict or creating duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8f68c470-27f2-48c4-9d20-0469da758dd2
📒 Files selected for processing (5)
application/database/db.pyapplication/tests/web_main_test.pyapplication/web/openapi_registry.pyapplication/web/web_main.pydocs/api/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- application/tests/web_main_test.py
- application/web/openapi_registry.py
- docs/api/openapi.yaml
- application/web/web_main.py
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Thanks for the clean API surface — feature_enabled_or_default + login/MyOpenCRE gate, validation/trim, OpenAPI coverage, and the mutation-proven flag-off tests look solid.
Blocking before approve: rebase onto current main (now includes #980). This branch still carries the pre-#980 DB layer:
- Obsolete Alembic merge — still has
f0e1d2c3b4a5anda1b2c3d4e5f6revising it. Onmain, users tables land viaa1b2c3d4e5f6→c7d8e9f0a1b2(nof0e1…). After rebase this PR should be API-only (routes, OpenAPI,user_resources_api_test.py) with no migration files / no re-introducing the DB models. - Stale login paths vs #980 — this branch’s
/rest/v1/loginNO_LOGINpath and broadexcept Exceptionon callback persistence are behind what landed in #980. Rebase will pick those up; don’t re-apply the old versions.
Non-blocking (nice after rebase):
- Global
@app.after_requestsetscache_control.max_age = 300; considerCache-Control: no-storeon the per-user GET/PUT responses. - Prefer
session['user_id']when present in_resolve_current_userbefore upsert-by-sub (avoids an extra lookup on the happy path after #980).
Once rebased green, ping for re-review — then #963 (auth route migration) can open cleanly on top of the persisted-user foundation.
cd16ffa to
5512979
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
application/tests/user_resources_api_test.py (2)
137-147: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the required GET ordering directly.
The test submits
["CWE", "ASVS"]but sorts the GET response before comparison. An endpoint that returns insertion order would pass although the API contract requires sorted values.Assert
json.loads(get.data)["selected"] == ["ASVS", "CWE"]without sorting.🤖 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 137 - 147, Update the GET response assertion in the user resources test to compare json.loads(get.data)["selected"] directly with ["ASVS", "CWE"] instead of sorting it, while leaving the PUT assertion unchanged.
216-247: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify that invalid PUT requests do not replace saved selections.
These cases only assert
400against a user with no selection. Seed a selection before each invalid request, then GET the resource and assert that the original values remain. This detects validation that occurs after replacement begins.🤖 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 216 - 247, The test_put_400_on_invalid_body test must verify that rejected PUT requests preserve existing selections. Seed a known selection before each invalid payload, send the PUT, assert a 400 response, then GET the resource and assert the seeded values remain unchanged; repeat this setup for the payloads already covered.
🤖 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_resources_api_test.py`:
- Line 20: Preserve the pre-existing NO_LOAD_GRAPH_DB environment value across
the test lifecycle: update the test case’s setUp and tearDown methods to save
and restore it, or manage the override with a patch.dict patcher started and
stopped for the test. Ensure cleanup does not remove a value that was already
configured by the test runner.
---
Nitpick comments:
In `@application/tests/user_resources_api_test.py`:
- Around line 137-147: Update the GET response assertion in the user resources
test to compare json.loads(get.data)["selected"] directly with ["ASVS", "CWE"]
instead of sorting it, while leaving the PUT assertion unchanged.
- Around line 216-247: The test_put_400_on_invalid_body test must verify that
rejected PUT requests preserve existing selections. Seed a known selection
before each invalid payload, send the PUT, assert a 400 response, then GET the
resource and assert the seeded values remain unchanged; repeat this setup for
the payloads already covered.
🪄 Autofix (Beta)
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: d8966307-9cc7-4d5e-8a9f-dafdf21534be
📒 Files selected for processing (4)
application/tests/user_resources_api_test.pyapplication/web/openapi_registry.pyapplication/web/web_main.pydocs/api/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- application/web/openapi_registry.py
- docs/api/openapi.yaml
- application/web/web_main.py
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Re-reviewed after the rebase onto main/#980.
Blocking items from the earlier review are addressed: API-only diff (no migrations / no reintroduced DB layer), current login paths preserved, and the non-blocking no-store + session['user_id'] preferences landed. CI is green.
Optional follow-ups only (not blocking): assert GET ordering without re-sorting in the persist test, and seed a selection before invalid PUT 400s to prove no partial replace.
Approving and merging with rebase.
2 participants
What & why
Second increment of #586. Builds on PR1's user persistence to expose the
per-user resource selection over the API, so the frontend (PR4) and server-side
filtering (PR3) have a read/write surface. No new tables or migrations — pure
HTTP + validation over PR1's
Node_collectionmethods.Endpoints
/rest/v1/user/resources(canonical/rest/v1surface):GET→{"selected": [...]}— the current user's saved standard names, sorted.PUT{"selected": ["ASVS","CWE"]}→ dedup + replace, persists via PR1'sset_user_resource_selection, returns the stored list.Gating
Requires
login_required+is_login_enabled()+is_myopencre_enabled()(per the #966 decision):
{"selected": []}, 200, no auth, no writes.any DB read/write.
selected, not a list, empty/non-string entries) → 400.Implemented via a generalized
feature_enabled_or_defaulthelper composed overlogin_required(a generalization of PR1's flag-gating; PR1's behaviour isunchanged).
OpenAPI
Endpoint added to the spec properly — two
PathSpecentries +@openapi_documenteddecorators,docs/api/openapi.yamlregenerated (nothand-edited). The OpenAPI guardrail passes all four checks (documented views,
freshness, validity, route coverage).
Testing
12 new tests, verified on real Postgres (prod parity): flag-off default,
myopencre-off default + no-write, anonymous 401, dedup, replace against the live
UNIQUE(user_id, standard_name), and body-validation 400s. The myopencre-offtests are mutation-proven — removing the capability from the gate makes them fail
(GET leaks the saved selection, PUT persists), restoring it makes them pass — so
they genuinely detect a bypassed gate rather than passing incidentally.
blackclean; no new mypy errors vs. baseline.Scope boundaries
No server-side filtering (PR3), no frontend (PR4), no new DB surface. Reuses
PR1's methods and
login_required.Files (4)
application/web/web_main.py— routes + gate +feature_enabled_or_default.application/web/openapi_registry.py— twoPathSpecentries.docs/api/openapi.yaml— regenerated.application/tests/user_resources_api_test.py— new, 12 tests.