feat(products): bulk import endpoint (EVO-1555 S1)#147
Open
marcelogorutuba wants to merge 1 commit into
Open
Conversation
POST /api/v1/products/bulk accepting up to 500 products in a single ACID transaction. Reuses existing Product validations, Labelable, and RBAC (products.create). Pre-validates item types + intra-batch SKU dupes before opening the transaction so the client gets the full error set in one 422. - Rate limit: 10 req/min/key via Rack::Attack, discriminator hashes the Authorization Bearer (or api_access_token) so credentials never land in Redis keys; falls back to IP. - Blank SKU normalized to nil to avoid PG::UniqueViolation on the partial unique index (WHERE sku IS NOT NULL). - Non-Hash array elements yield a structured 422, not a 500. - Service object (Products::BulkImporter) keeps controller thin and rubocop-clean without disable comments. v1 ships only the 'fail' SKU-conflict policy (rollback total); skip/overwrite are out-of-scope per tech-spec — fast-follow if real demand surfaces. Tests: 18 request specs covering AC1-AC13 + M2/M5 hardening. AC12 driven via Rack::MockRequest because Rails request specs bypass Rack::Attack throttles at the middleware level. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer's GuideImplements a new POST /api/v1/products/bulk endpoint that ingests up to 500 products per request in a single database transaction, with a dedicated BulkImporter service handling pre-validation, intra-batch SKU deduplication, normalization, and error aggregation, plus a Rack::Attack rate limit and request specs covering success, validation, and throttling behavior. Sequence diagram for POST /api/v1/products/bulk success and failure flowssequenceDiagram
actor Client
participant RackAttack
participant ProductsController
participant BulkImporter
participant ActiveRecord
Client->>RackAttack: POST /api/v1/products/bulk
RackAttack->>RackAttack: throttle api_v1_products_bulk
RackAttack-->>ProductsController: forward request
ProductsController->>ProductsController: extract_bulk_items
alt [items invalid or limit exceeded]
ProductsController-->>Client: error_response VALIDATION_ERROR or LIMIT_EXCEEDED (422)
else [items valid]
ProductsController->>BulkImporter: call
BulkImporter->>BulkImporter: pre_validate_items
alt [pre validation errors]
BulkImporter-->>ProductsController: raise BulkImportError
ProductsController-->>Client: error_response VALIDATION_ERROR (422)
else [pre validation OK]
BulkImporter->>ActiveRecord: transaction
loop each item
BulkImporter->>ActiveRecord: save Product
end
alt [any product invalid]
ActiveRecord-->>BulkImporter: rollback
BulkImporter-->>ProductsController: raise BulkImportError
ProductsController-->>Client: error_response VALIDATION_ERROR (422)
else [all products valid]
ActiveRecord-->>BulkImporter: commit
BulkImporter-->>ProductsController: created products
ProductsController-->>Client: success_response created (201)
end
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
BulkImporter#callpre-validates non-hash items but still passes all@itemstoimport_one, so a non-hash element will hititem_paramsand callraw_item.to_h(e.g. on a String) and raise; consider either filtering invalid entries out afterpre_validate_itemsor short-circuiting the second pass when any pre-validation errors exist so non-hash items never reachimport_one.- The controller’s
extract_bulk_itemsallowsparams[:products]to be anActionController::Parametersand then callsArray(raw_items), which for a params hash gives[ [key, value], ... ]pairs rather than the expected product hashes; it would be safer to explicitly handle the Rails array payload shape (e.g.params.require(:products).to_a) instead of relying onArray()for both arrays and params. - The max-items invariant is currently enforced only in
Api::V1::ProductsController#extract_bulk_items; ifProducts::BulkImportermight ever be reused from other entry points, consider enforcingMAX_ITEMSinside the service as well to keep the limit close to the logic it protects.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `BulkImporter#call` pre-validates non-hash items but still passes all `@items` to `import_one`, so a non-hash element will hit `item_params` and call `raw_item.to_h` (e.g. on a String) and raise; consider either filtering invalid entries out after `pre_validate_items` or short-circuiting the second pass when any pre-validation errors exist so non-hash items never reach `import_one`.
- The controller’s `extract_bulk_items` allows `params[:products]` to be an `ActionController::Parameters` and then calls `Array(raw_items)`, which for a params hash gives `[ [key, value], ... ]` pairs rather than the expected product hashes; it would be safer to explicitly handle the Rails array payload shape (e.g. `params.require(:products).to_a`) instead of relying on `Array()` for both arrays and params.
- The max-items invariant is currently enforced only in `Api::V1::ProductsController#extract_bulk_items`; if `Products::BulkImporter` might ever be reused from other entry points, consider enforcing `MAX_ITEMS` inside the service as well to keep the limit close to the logic it protects.
## Individual Comments
### Comment 1
<location path="app/controllers/api/v1/products_controller.rb" line_range="109-111" />
<code_context>
)
end
+ def extract_bulk_items
+ raw_items = params[:products]
+ items = raw_items.is_a?(Array) || raw_items.is_a?(ActionController::Parameters) ? Array(raw_items) : []
+ return reject_bulk(ApiErrorCodes::VALIDATION_ERROR, 'products array is required and must not be empty') if items.empty?
+ return reject_bulk_limit(items.size) if items.size > Products::BulkImporter::MAX_ITEMS
</code_context>
<issue_to_address>
**issue (bug_risk):** The coercion of `params[:products]` when it is `ActionController::Parameters` may not yield the intended array of items.
When `raw_items` is an `ActionController::Parameters` with numeric keys, `Array(raw_items)` returns `[key, value]` pairs rather than product hashes. That means downstream code receives arrays instead of the expected hashes/JSON objects, and you lose a clear validation error on invalid input shape. Consider explicitly normalizing here, e.g.:
```ruby
def extract_bulk_items
raw_items = params[:products]
items =
if raw_items.is_a?(Array)
raw_items
elsif raw_items.is_a?(ActionController::Parameters)
raw_items.values
else
[]
end
return reject_bulk(ApiErrorCodes::VALIDATION_ERROR, 'products array is required and must not be empty') if items.empty?
return reject_bulk_limit(items.size) if items.size > Products::BulkImporter::MAX_ITEMS
end
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+109
to
+111
| def extract_bulk_items | ||
| raw_items = params[:products] | ||
| items = raw_items.is_a?(Array) || raw_items.is_a?(ActionController::Parameters) ? Array(raw_items) : [] |
There was a problem hiding this comment.
issue (bug_risk): The coercion of params[:products] when it is ActionController::Parameters may not yield the intended array of items.
When raw_items is an ActionController::Parameters with numeric keys, Array(raw_items) returns [key, value] pairs rather than product hashes. That means downstream code receives arrays instead of the expected hashes/JSON objects, and you lose a clear validation error on invalid input shape. Consider explicitly normalizing here, e.g.:
def extract_bulk_items
raw_items = params[:products]
items =
if raw_items.is_a?(Array)
raw_items
elsif raw_items.is_a?(ActionController::Parameters)
raw_items.values
else
[]
end
return reject_bulk(ApiErrorCodes::VALIDATION_ERROR, 'products array is required and must not be empty') if items.empty?
return reject_bulk_limit(items.size) if items.size > Products::BulkImporter::MAX_ITEMS
end
dpaes
approved these changes
Jun 15, 2026
dpaes
left a comment
dpaes
left a comment
Contributor
There was a problem hiding this comment.
✅ Approved (code) — EVO-1555 S1
Deep re-review across service+controller, rate-limit security, and spec coverage. No code blockers — the bulk endpoint is atomic, RBAC-gated, and the rate-limit's core security property holds.
AC coverage (S1 backend)
- AC1 (single transaction, full rollback): ✅ met & well-tested.
Products::BulkImporterwraps the whole batch in oneActiveRecord::Base.transaction; any rowsavefailure raisesBulkImportErrorand rolls the batch back. Specs assert zero persistence on a mid-batch failure, including tagging rollback (ActsAsTaggableOn::Tagging.countunchanged). - AC4 (validation mirrors the
Productmodel): ✅ correct in code — builds a realProduct.new+save, so all 9 model validations run identically to single-create. But the spec exercises only 2 of them (see caveat 1). - AC5 (RBAC
products.create): ✅ met both directions — 403 (permission denied), 401 (unauthenticated), success (granted). Reuses the samebefore_actionas single create. - AC2 (dry-run) / AC3 (skip-overwrite policy): correctly out of scope for S1.
Rate-limit security
Core claim CONFIRMED — the Bearer/api_token is SHA256-hashed before it becomes the Redis throttle key (never raw in the key; not logged — Authorization isn't logged, api_token is masked). Path+POST scope is exact (== '/api/v1/products/bulk' && req.post?), and the endpoint is auth-gated independently of the throttle, so the IP fallback opens no useful bypass.
Non-blocking caveats (worth firming up while the PR is parked for the bundle)
- AC4 under-tested. The code mirrors the model, but the spec covers only
namepresence +skuuniqueness —kind/status/currency/default_price/stock_quantity/purchase_url/length are untested. If a permitted attr were dropped fromSCALAR_ATTRSor a model rule diverged, no test would catch it. ~6 cases close the gap. - Response has no created/updated/skipped counts. The success payload is
{ data: [...], message }— the structured counts the future S2 preview UI ("create X / update Y / skip Z") will consume don't exist yet. Worth aligning that contract now so S2 isn't blocked later. - Rate-limit tested outside the real Rails stack (
Rack::MockRequest, not the middleware chain) and only via theapi_tokendiscriminator branch — thebearer:branch is untested. Recommend the manual 11× smoke (or a real integration test). - CI runs no RSpec here — "18/18 green" is self-reported (local Docker), and there's no Product factory (first Products spec). Recommend an independent
bundle exec rspecbefore relying on the count. - Low / follow-up: L4 (SKU uniqueness race → generic 409 via the global
rescue_from, not a 500), L5 (variants/images dropped — scalar-only by design; contract diverges from single-create), L6 (reload+ N+1 on the success path;reloadis removable), M1 (uncappedmetadatajsonb per row), 429 without the API error envelope /Retry-After(pre-existing repo convention).
Merge held
Per the product decision (Davidson — deliver the complete experience together) + the bundle plan, this S1 is not merged standalone — it ships bundled with S1.1 (EVO-1736) + S2 (EVO-1734), with a rebase when the batch is assembled. Approving the code; merge stays gated on the bundle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
POST /api/v1/products/bulkaccepting up to 500 products in a single ACID transaction (S1 da EVO-1555). Reaproveita 100% da modelagem entregue na EVO-1109 — apenas adiciona o pipeline de ingestão em lote.Api::V1::ProductsController(require_permissions(bulk: 'products.create'))Products::BulkImporterencapsula a transação, dedup intra-batch de SKU, normalização e tratamento de errosv1 entrega só
failna política de conflito de SKUA AC original do card Linear cita
skip / overwrite / failcomo política configurável. Esta story (S1) entrega apenasfail(rollback total em qualquer conflito de SKU vs DB ou intra-batch).skipeoverwritesão out-of-scope por decisão do tech-spec — fast-follow se virar demanda real. CSV UI (S2) e webhook ERP (S3) também ficam para stories filhas.Hardening rounds 1+2 (revisão antes do PR)
sku: ''normalizado pranilantes do save (evitaPG::UniqueViolationno índice parcial)Validation
evo-ai-crm-community: docker exec evo-crm-community-evo-crm-1 sh -lc "cd /app && DATABASE_NAME=evo_community_test POSTGRES_DATABASE=evo_community_test bundle exec rspec spec/requests/api/v1/products_bulk_spec.rb"→ 18/18 verde em 8.81s (cobre AC1–AC13 + M2/M5)evo-ai-crm-community: bundle exec rubocop app/controllers/api/v1/products_controller.rb app/services/products/bulk_importer.rb config/initializers/rack_attack.rb config/routes.rb spec/requests/api/v1/products_bulk_spec.rb→ limpo no diff (2 offenses pré-existentes emrack_attack.rb:187,200fora do escopo)Smoke manual (a fazer pré-merge)
Endpoint requer container rodando + auth-service. Comandos sugeridos:
curl POST /api/v1/products/bulkcom 1 item válido → 201curl POST /api/v1/products/bulkcom 501 itens → 422LIMIT_EXCEEDEDcurl POST /api/v1/products/bulk11x em <1min → 11ª deve voltar 429 (Rack::Attack)Changed Files
app/controllers/api/v1/products_controller.rbapp/services/products/bulk_importer.rb(NEW)config/routes.rbconfig/initializers/rack_attack.rbspec/requests/api/v1/products_bulk_spec.rb(NEW — primeiro spec de Products no repo)Linked Issue
cc @davidson
🤖 Generated with Claude Code
Summary by Sourcery
Add a bulk products import API endpoint that creates multiple products in a single transactional operation with structured validation errors and rate limiting.
New Features:
Enhancements:
Tests: