fix(bulk): invalid status code (500) when using atomic + parallel #960
Conversation
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
WalkthroughA new error variable was introduced to represent conflicts when both atomic and parallel options are set in bulk operations. Error handling in the API was updated to return a specific HTTP status code for this case. Related unit and end-to-end tests were adjusted and expanded to verify this behavior and improve response validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Controller
participant Bulker
Client->>API_Controller: POST /bulk?atomic=true¶llel=true
API_Controller->>Bulker: Run(bulkOps, options{atomic, parallel})
Bulker->>API_Controller: Return ErrAtomicParallelConflict
API_Controller-->>Client: HTTP 412 Precondition Failed (Validation Error)
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Too many requests...We've been getting too many requests from your IP address and rate limiting has been triggered. Please try again later. We apologize for the inconvenience. ✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
gfyrag
force-pushed
the
fix/bulk-atomic-parallel-invalid-status-code
branch
from
a8d10d9 to
386e797
Compare
flemzord
approved these changes
Jun 11, 2025
![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 (6)
internal/api/v2/controllers_bulk.go (1)
46-51: HTTP status selection might be misleading – consider 409 Conflict (or document 412)Returning
412 Precondition Failedtechnically works, but the more common idiom for mutually-exclusive / state-conflict situations is409 Conflict.
If you decide to keep 412, add a short comment explaining why 409/400 are not used to avoid future churn.- api.WriteErrorResponse(w, http.StatusPreconditionFailed, common.ErrValidation, err) + // 409 would also make sense here; we explicitly pick 412 to avoid colliding + // with 400 which is already used for partial-error responses. + api.WriteErrorResponse(w, http.StatusPreconditionFailed, common.ErrValidation, err)internal/api/bulking/bulker.go (2)
19-20: Exported error should be documented
ErrAtomicParallelConflictis exported; add a short godoc comment so it’s picked up by tooling.-var ErrAtomicParallelConflict = errors.New("atomic and parallel options are mutually exclusive") +// ErrAtomicParallelConflict is returned when both Atomic and Parallel flags are set. +var ErrAtomicParallelConflict = errors.New("atomic and parallel options are mutually exclusive")
290-293: Validation covers only one illegal combination – extend tests later
Validate()now checksAtomic && Parallel. If other mutually-exclusive flags appear later we’ll forget to add them. A table-driven unit test aroundBulkingOptions.Validate()would freeze expected behaviour.test/e2e/api_bulk_test.go (1)
184-193: E2E test asserts error code but not HTTP statusThe controller returns 412; asserting the status strengthens the test and catches regressions.
- Expect(err).To(HaveOccurred()) - Expect(err).To(HaveErrorCode(string(components.V2ErrorsEnumValidation))) + Expect(err).To(HaveOccurred()) + Expect(err).To(HaveErrorCode(string(components.V2ErrorsEnumValidation))) + Expect(err).To(HaveHTTPStatus(http.StatusPreconditionFailed))internal/api/v2/controllers_bulk_test.go (2)
453-483: Missing assertion on error payload for 412 scenarioThe test only checks status; assert that the body carries
VALIDATIONaserrorCodeto ensure controller wiring.expectations: func(mockLedger *LedgerController) {}, expectStatusCode: http.StatusPreconditionFailed, + expectResults: nil, // body should be api.ErrorResponseLater, when
expectStatusCode≠ 200/400, decode and compareerrResponse.ErrorCodetocommon.ErrValidation.
566-586: Body decoding branch could assert on 412 payloadYou already decode
api.ErrorResponse; add minimal validation to catch regressions:- err := json.NewDecoder(rec.Body).Decode(&errResponse) - require.NoError(t, err) + require.NoError(t, json.NewDecoder(rec.Body).Decode(&errResponse)) + if expectedStatusCode == http.StatusPreconditionFailed { + require.Equal(t, common.ErrValidation, errResponse.ErrorCode) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/api/bulking/bulker.go(3 hunks)internal/api/v2/controllers_bulk.go(1 hunks)internal/api/v2/controllers_bulk_test.go(6 hunks)test/e2e/api_bulk_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/api/v2/controllers_bulk.go (2)
internal/api/bulking/bulker.go (1)
ErrAtomicParallelConflict(19-19)internal/api/common/errors.go (2)
ErrValidation(17-17)InternalServerError(55-60)
test/e2e/api_bulk_test.go (1)
pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumValidation(15-15)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (3)
internal/api/bulking/bulker.go (1)
98-99: Nice: switched to %w – makeserrors.Is/Aswork
The new wrapping keeps the original error chain intact – good catch.internal/api/v2/controllers_bulk_test.go (2)
299-300: Good: switched failing-bulk test to explicit 400
Makes intent clearer.
383-384: Ditto – explicit status improves readability
Comment on lines
+36
to
43
| name string | ||
| queryParams url.Values | ||
| body string | ||
| expectations func(mockLedger *LedgerController) | ||
| expectStatusCode int | ||
| expectResults []bulking.APIResult | ||
| headers http.Header | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Defaulting to 200 hides forgotten expectations
Silently replacing zero with 200 can mask missing assertions. Require every test to set expectStatusCode explicitly.
- expectStatusCode int
+ expectStatusCode int // must be set in every test case; defaults removedAnd drop the fallback below.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| name string | |
| queryParams url.Values | |
| body string | |
| expectations func(mockLedger *LedgerController) | |
| expectStatusCode int | |
| expectResults []bulking.APIResult | |
| headers http.Header | |
| } | |
| name string | |
| queryParams url.Values | |
| body string | |
| expectations func(mockLedger *LedgerController) | |
| expectStatusCode int // must be set in every test case; defaults removed | |
| expectResults []bulking.APIResult | |
| headers http.Header | |
| } |
🤖 Prompt for AI Agents
In internal/api/v2/controllers_bulk_test.go around lines 36 to 43, the test
struct field expectStatusCode currently defaults to 200 when not set, which can
hide missing assertions. Remove any defaulting logic that sets expectStatusCode
to 200 automatically and require each test case to explicitly set
expectStatusCode to ensure all tests have clear expected status codes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #960 +/- ##
==========================================
+ Coverage 82.41% 82.44% +0.03%
==========================================
Files 141 141
Lines 7835 7886 +51
==========================================
+ Hits 6457 6502 +45
- Misses 1054 1059 +5
- Partials 324 325 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As the status code 400 is already used for partial errored response, I have used the error code 412.
Not beautiful but, no choice.
If you want to suggest another code, I'm listening.
Fixes LX-55