perf: skip unnecessary shared user queries#1968
Conversation
Avoid loading shared_users during partial updates unless the request actually changes sharing, and reuse the pre-update shared user snapshot in adapter updates instead of querying it twice. Add regression tests covering the non-sharing update path and the adapter query reuse path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughViewset partial_update handlers were refactored to compute a single ChangesShared-user update gating and query optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
for more information, see https://pre-commit.ci
|
| Filename | Overview |
|---|---|
| backend/adapter_processor_v2/views.py | Skips M2M snapshot when shared_users not in request; reuses snapshot to avoid duplicate query for current_users; adds notification_plugin guard (bug-fix side-effect) |
| backend/workflow_manager/workflow_v2/views.py | Guards M2M snapshot behind shared_users_updated flag; no other logic changes |
| backend/connector_v2/views.py | Mirrors the same shared_users_updated guard pattern; no other logic changes |
| backend/pipeline_v2/views.py | Mirrors the same shared_users_updated guard pattern with minor formatting cleanup on the outer if condition |
| backend/prompt_studio/prompt_studio_core_v2/views.py | Applies the same skip guard; notification_plugin is lazily loaded inside the block (different pattern but correct) |
| backend/api_v2/api_deployment_views.py | Applies the same skip guard; notification_plugin reformatted inline but semantically unchanged |
| backend/utils/tests/test_shared_user_query_optimizations.py | New regression tests covering skip-path for WorkflowViewSet and query-reuse for AdapterInstanceViewSet; four other changed views lack skip-path coverage |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[partial_update called] --> B{shared_users in request.data}
B -- No --> C[Skip M2M query]
B -- Yes --> D[Fetch current_shared_users from DB]
D --> E[Compute removed_users from snapshot]
C --> F[Call super partial_update]
E --> F
F --> G{status 200 AND shared_users_updated AND notification_plugin}
G -- No --> H[Return response]
G -- Yes --> I[refresh_from_db and fetch new_shared_users]
I --> J[Send sharing notifications]
J --> H
Reviews (2): Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from types import SimpleNamespace | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from rest_framework import status, viewsets | ||
| from rest_framework.response import Response | ||
|
|
||
| from adapter_processor_v2.views import AdapterInstanceViewSet | ||
| from workflow_manager.workflow_v2.views import WorkflowViewSet | ||
|
|
There was a problem hiding this comment.
The test file only exercises WorkflowViewSet and AdapterInstanceViewSet, but ConnectorInstanceViewSet, PipelineViewSet, PromptStudioCoreView, and APIDeploymentViewSet all received the same guard change. A regression in any of those four (e.g. accidentally removing the shared_users_updated guard) would go undetected. The skip-path test for WorkflowViewSet is a single-function template that could be copy-adapted for each missing view in a few lines.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/utils/tests/test_shared_user_query_optimizations.py
Line: 1-9
Comment:
**Missing skip-path coverage for four changed views**
The test file only exercises `WorkflowViewSet` and `AdapterInstanceViewSet`, but `ConnectorInstanceViewSet`, `PipelineViewSet`, `PromptStudioCoreView`, and `APIDeploymentViewSet` all received the same guard change. A regression in any of those four (e.g. accidentally removing the `shared_users_updated` guard) would go undetected. The skip-path test for `WorkflowViewSet` is a single-function template that could be copy-adapted for each missing view in a few lines.
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/utils/tests/test_shared_user_query_optimizations.py (1)
1-60: ⚡ Quick winConsider expanding test coverage to all refactored viewsets.
The tests verify the optimization for
WorkflowViewSetandAdapterInstanceViewSet, but four other viewsets received the same refactor:PromptStudioCoreView,APIDeploymentViewSet,PipelineViewSet, andConnectorInstanceViewSet. Adding similar test coverage for these would provide stronger regression protection.🤖 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 `@backend/utils/tests/test_shared_user_query_optimizations.py` around lines 1 - 60, Add tests mirroring the two existing cases for the four refactored viewsets (PromptStudioCoreView, APIDeploymentViewSet, PipelineViewSet, ConnectorInstanceViewSet): for each viewset write one test like test_workflow_partial_update_skips_shared_user_lookup_without_share_changes that patches <View>.get_object to return a MagicMock whose shared_users.all raises an AssertionError and asserts shared_users.all.assert_not_called() when request.data has no shared_users, and another test like test_adapter_partial_update_reuses_pre_update_shared_users that sets shared_users.all to return the same list twice, patches <View>.get_object and viewsets.ModelViewSet.partial_update, invokes <View>.partial_update(request) with request.data containing shared_users, then asserts response.status_code is HTTP_200_OK and that shared_users.all.call_count == 2 to verify reuse; follow the same mocking/patch pattern used for WorkflowViewSet and AdapterInstanceViewSet.
🤖 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.
Nitpick comments:
In `@backend/utils/tests/test_shared_user_query_optimizations.py`:
- Around line 1-60: Add tests mirroring the two existing cases for the four
refactored viewsets (PromptStudioCoreView, APIDeploymentViewSet,
PipelineViewSet, ConnectorInstanceViewSet): for each viewset write one test like
test_workflow_partial_update_skips_shared_user_lookup_without_share_changes that
patches <View>.get_object to return a MagicMock whose shared_users.all raises an
AssertionError and asserts shared_users.all.assert_not_called() when
request.data has no shared_users, and another test like
test_adapter_partial_update_reuses_pre_update_shared_users that sets
shared_users.all to return the same list twice, patches <View>.get_object and
viewsets.ModelViewSet.partial_update, invokes <View>.partial_update(request)
with request.data containing shared_users, then asserts response.status_code is
HTTP_200_OK and that shared_users.all.call_count == 2 to verify reuse; follow
the same mocking/patch pattern used for WorkflowViewSet and
AdapterInstanceViewSet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: add5b873-feb1-4a4e-bc8b-7234cad3999f
📒 Files selected for processing (7)
backend/adapter_processor_v2/views.pybackend/api_v2/api_deployment_views.pybackend/connector_v2/views.pybackend/pipeline_v2/views.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/utils/tests/test_shared_user_query_optimizations.pybackend/workflow_manager/workflow_v2/views.py
The other 4 resource viewsets (workflow, pipeline, connector, api_deployment) all gate their post-update shared_users comparison block on notification_plugin being truthy. The adapter viewset was missing this check, causing an extra refresh_from_db() + shared_users.all() round-trip on every share/unshare even when no notification plugin is installed. Update the test to patch notification_plugin=None explicitly and assert call_count==1 (pre-update snapshot only), which precisely pins the elimination of the old duplicate ID-extraction query. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
for more information, see https://pre-commit.ci
|
2 participants
Summary
shared_userslookups in shareable resourcepartial_updatehandlers unless the request actually updates sharingshared_userssnapshot instead of querying the same M2M relation twice before the updateWhy
This was the highest-impact, lowest-effort performance win from the audit: these endpoints were paying for M2M reads on every partial update, even when the request changed unrelated fields, and adapters were issuing the same pre-update query twice.
Validation
python3 -m py_compile backend/adapter_processor_v2/views.py backend/workflow_manager/workflow_v2/views.py backend/pipeline_v2/views.py backend/connector_v2/views.py backend/prompt_studio/prompt_studio_core_v2/views.py backend/api_v2/api_deployment_views.py backend/utils/tests/test_shared_user_query_optimizations.pyuv run ...local lint/test execution is currently blocked by repositoryuv.lockparse errors (bankswheel version mismatch and ambiguousoffice365-rest-python-clientsource)