OSIDB-4748: Allow partial creation of affects via /bulk endpoint#1254
OSIDB-4748: Allow partial creation of affects via /bulk endpoint#1254
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe Changes
Sequence DiagramsequenceDiagram
actor Client
participant Endpoint as AffectView.bulk_post
participant Validator
participant DB as Database
participant Effects
participant Serializer
Client->>Endpoint: POST /affects/bulk [entries]
Endpoint->>Endpoint: Pre-scan: verify all entries target same flaw
loop For each entry (index)
Endpoint->>Validator: Validate entry fields
alt Validation fails
Endpoint->>Endpoint: Store error in failed[index]
else Validation succeeds
Endpoint->>Endpoint: _prepare_affect_for_bulk<br/>(derived fields)
end
end
Endpoint->>DB: bulk_create(prepared_affects, ignore_conflicts=True)
DB->>Endpoint: Return created Affect instances
Endpoint->>DB: Refetch via Q-filter<br/>(flaw + component/purl)
DB->>Endpoint: Return refetched instances
Endpoint->>Endpoint: Compare submitted vs. created<br/>to detect duplicates/skipped
Endpoint->>Effects: _run_post_save_effects_for_bulk<br/>(FlawCollaborator labels)
Effects->>DB: Create labels
Endpoint->>Serializer: Serialize results + failed
Serializer->>Client: Return {results[], failed[]}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Elkasitu
force-pushed
the
improve-bulk-affects-creation
branch
from
63c174e to
a305563
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
osidb/tests/endpoints/test_affects.py (1)
880-911: Minor: method name doesn't match what it asserts.The method is named
test_affect_create_bulk_returns_errors_keybut the body and docstring check thefailedkey (the new API shape). Consider renaming totest_affect_create_bulk_returns_failed_keyfor consistency with the contract this test enforces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osidb/tests/endpoints/test_affects.py` around lines 880 - 911, The test function name test_affect_create_bulk_returns_errors_key is inconsistent with its assertions and docstring which check for the "failed" key; rename the function to test_affect_create_bulk_returns_failed_key and update the docstring to reflect "failed" instead of "errors" (and adjust any references to the old name in the test suite if present) so the method name matches the contract enforced by the assertions inside the test_affect_create_bulk_returns_errors_key/test_affect_create_bulk_returns_failed_key function.osidb/api_views.py (2)
1386-1398: Reject empty request bodies consistently.If
request.datais[], the pre-scan loop doesn't execute,flaw_uuidsstays empty, and the code raisesValidationError({"flaw": "This field is required."})at line 1398. Message-wise this is misleading (the request has no items, not a missing field). Consider returning a clearer error such as"Request body must contain at least one affect."to help API consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osidb/api_views.py` around lines 1386 - 1398, The loop that collects flaw_uuids from request.data must first reject an empty list with a clearer message: before iterating over request.data check if not request.data (or len(request.data) == 0) and raise ValidationError({"non_field_errors": "Request body must contain at least one affect."}) (or ValidationError({"request": "Request body must contain at least one affect."}) depending on the API error shape) instead of letting flaw_uuids remain empty and producing the misleading {"flaw": "This field is required."}; keep the existing per-item KeyError/TypeError handling and the multi-flaw check (flaw_uuids) unchanged.
1262-1273: Minor: prefer module-level imports unless avoiding a cycle.
WorkflowModelis already imported at the top of the file (line 53); reimporting it inside the function is redundant.FlawCollaboratorcan also move to the top-levelfrom osidb.models import (...)block unless there's a circular-import reason. Inline-imports inside a hot-path helper like this obscure dependencies and incur a tiny repeated cost per call.♻️ Proposed change
from osidb.models import ( Affect, AffectCVSS, AffectV1, Flaw, + FlawCollaborator, FlawLabel, PsUpdateStream, Tracker, )def _run_post_save_effects_for_bulk(created_affects, flaw): """ Run the post_save side-effects that are normally triggered by signals after each individual Affect.save(). Called once after bulk_create. """ - from apps.workflows.workflow import WorkflowModel - from osidb.models import FlawCollaborator - # FlawCollaborator labels for PRE_SECONDARY_ASSESSMENT flaws if flaw.workflow_state == WorkflowModel.WorkflowState.PRE_SECONDARY_ASSESSMENT: for affect in created_affects: FlawCollaborator.objects.create_from_affect(affect)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osidb/api_views.py` around lines 1262 - 1273, The function _run_post_save_effects_for_bulk currently performs inline imports of WorkflowModel and FlawCollaborator; move these to the module-level import block (add WorkflowModel and FlawCollaborator to the top-level imports that already import other models) and remove the in-function import statements in _run_post_save_effects_for_bulk to avoid redundant imports on each call—only keep the inline imports if a circular-import prevents moving them to the module level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openapi.yml`:
- Around line 14011-14018: The current response schema uses an optional,
loosely-typed "failed" array; introduce a concrete AffectBulkFailure schema and
update the response to use it and mark "failed" required: add a new schema
AffectBulkFailure with required properties index (integer), input (object with
additionalProperties: {}), and errors (object with additionalProperties: {}),
then change the existing response's failed property to type: array items: $ref:
'#/components/schemas/AffectBulkFailure' and include "failed" in that response's
required list (update the response object that currently contains "failed" and
"results" so "failed" is required).
In `@osidb/api_views.py`:
- Around line 1397-1401: The code currently calls
Flaw.objects.get(uuid=flaw_uuid) which raises Flaw.DoesNotExist and results in a
500; change this to catch that case and raise a DRF ValidationError instead so
the bulk endpoint returns a 4xx like the single-item POST. Specifically, around
the block using flaw_uuids, flaw_uuid and Flaw.objects.get, wrap the get in a
try/except for Flaw.DoesNotExist (or use Flaw.objects.filter(...).first() and
validate) and raise ValidationError({"flaw": f"Flaw with uuid {flaw_uuid} does
not exist."}) (or similar message); apply the same pattern to the bulk_put
handling referenced (where Flaw.objects.get is used) to ensure consistent
validation behavior.
- Around line 1449-1494: The refetch uses (flaw_id, ps_update_stream,
ps_component) and thus picks up pre-existing rows; fix by capturing the
pre-generated primary keys/UUIDs from prepared_instances before the bulk_create
(e.g. ids = {inst.pk for inst in prepared_instances}), then call
Affect.objects.bulk_create(prepared_instances, ignore_conflicts=True) and
refetch only those exact rows via Affect.objects.filter(pk__in=ids) (or the UUID
field if different). Keep the rest of the logic (created_by_component,
created_by_purl, prepared_meta loop, and errors appending) but base them on the
refetched set named created so only actually-inserted instances are considered
created.
- Around line 1207-1259: The bulk-prep helper _prepare_affect_for_bulk bypasses
post_save signal handlers and pghistory tracking (so create_flaw_labels,
update_local_updated_dt_tracker, flaw_dependant_update_local_updated_dt and
pghistory events are not run), so either (A) explicitly perform those side
effects after bulk_create by invoking the same logic used by the post_save
handlers (call create_flaw_labels for each created Affect's parent Flaw or
collect affected Flaw ids and run the collaborator-assignment routine once, and
call update_local_updated_dt_tracker / flaw_dependant_update_local_updated_dt or
update Flaw.updated_dt for affected flaws), and ensure pghistory events are
recorded (invoke the equivalent history recording API), or (B) stop using
bulk_create and save each Affect normally so Django signals and `@pghistory.track`
fire; update callers of _prepare_affect_for_bulk to reflect the chosen approach
and add tests verifying collaborator creation, parent Flaw.updated_dt changes,
and pghistory entries for created Affects.
---
Nitpick comments:
In `@osidb/api_views.py`:
- Around line 1386-1398: The loop that collects flaw_uuids from request.data
must first reject an empty list with a clearer message: before iterating over
request.data check if not request.data (or len(request.data) == 0) and raise
ValidationError({"non_field_errors": "Request body must contain at least one
affect."}) (or ValidationError({"request": "Request body must contain at least
one affect."}) depending on the API error shape) instead of letting flaw_uuids
remain empty and producing the misleading {"flaw": "This field is required."};
keep the existing per-item KeyError/TypeError handling and the multi-flaw check
(flaw_uuids) unchanged.
- Around line 1262-1273: The function _run_post_save_effects_for_bulk currently
performs inline imports of WorkflowModel and FlawCollaborator; move these to the
module-level import block (add WorkflowModel and FlawCollaborator to the
top-level imports that already import other models) and remove the in-function
import statements in _run_post_save_effects_for_bulk to avoid redundant imports
on each call—only keep the inline imports if a circular-import prevents moving
them to the module level.
In `@osidb/tests/endpoints/test_affects.py`:
- Around line 880-911: The test function name
test_affect_create_bulk_returns_errors_key is inconsistent with its assertions
and docstring which check for the "failed" key; rename the function to
test_affect_create_bulk_returns_failed_key and update the docstring to reflect
"failed" instead of "errors" (and adjust any references to the old name in the
test suite if present) so the method name matches the contract enforced by the
assertions inside the
test_affect_create_bulk_returns_errors_key/test_affect_create_bulk_returns_failed_key
function.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 00d2ba31-41ed-4d65-9611-cd2ea27cabd1
📒 Files selected for processing (5)
docs/CHANGELOG.mdopenapi.ymlosidb/api_views.pyosidb/serializer.pyosidb/tests/endpoints/test_affects.py
Elkasitu
force-pushed
the
improve-bulk-affects-creation
branch
2 times, most recently
from
4dbf223 to
ca9208b
Compare
dmonzonis
left a comment
dmonzonis
left a comment
There was a problem hiding this comment.
Not really a fan of repeating all the pre-save/signal logic within the view, but I guess otherwise it would require a lot of refactoring. We will just have to remember to modify the logic in the view too if it ever changes in the model.
MrMarble
left a comment
MrMarble
left a comment
There was a problem hiding this comment.
LGTM! Just a note that OSIM might need to be updated as well, iirc it parsed the response to show the errors in the affect table.
Should we do the same handling in the put?
|
@MrMarble @dmonzonis I've taken your feedback into consideration and will create the appropriate tickets, thanks! |
Before this commit, submitting a batch of affects to be created via the /bulk endpoint could fail for all affects if only a single affect contained erroneous / duplicate data. With this commit, the server will attempt to create all valid affects and return a list of affect entries which yielded errors and could not be created. This commit also changes the way these affects are created by leveraging prefetching and bulk_create, this should make the creation of hundreds or thousands of affects much faster. Assisted-By: Cursor (claude-4.6-opus-high)
Elkasitu
force-pushed
the
improve-bulk-affects-creation
branch
from
ca9208b to
4f9a1b6
Compare
4 participants
Before this commit, submitting a batch of affects to be created via the /bulk endpoint could fail for all affects if only a single affect contained erroneous / duplicate data.
With this commit, the server will attempt to create all valid affects and return a list of affect entries which yielded errors and could not be created.
This commit also changes the way these affects are created by leveraging prefetching and bulk_create, this should make the creation of hundreds or thousands of affects much faster.
With this commit, the performance of affect creation in bulk is ~8 times faster.