fix: obscur error message when trying to use experimental features while the service is not configured for #964
Conversation
…ile the service is not configured for
WalkthroughThe error handling in the ledger creation API was updated to treat the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant System
Client->>API: POST /ledgers (createLedger)
API->>System: Validate ledger config
alt Invalid config or experimental features disabled
System-->>API: Return validation error
API-->>Client: 400 Bad Request (ValidationError)
else Valid config
System-->>API: Success
API-->>Client: 201 Created
end
Suggested reviewers
Poem
✨ 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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/api_ledgers_create_test.go (2)
135-136: EmptyItblock – assertions belong inside the specThe
It("should fail", func() {})contains no expectations; failures are asserted inJustBeforeEach.
This passes linting but obscures intent. Move the expectations into theItor convert the hook into anIt-block for clarity.
30-80: High duplication between the two contextsBoth “without experimental features” and “with experimental features” spin up almost identical test-server scaffolding. Consider parameterising the feature flag and using
DescribeTable/BeforeAllto cut repetition and speed up suite execution.Also applies to: 71-110
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/api/v2/controllers_ledgers_create.go(1 hunks)test/e2e/api_ledgers_create_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/api/v2/controllers_ledgers_create.go (1)
internal/controller/system/errors.go (1)
ErrExperimentalFeaturesDisabled(11-11)
test/e2e/api_ledgers_create_test.go (5)
pkg/client/models/operations/v2createledger.go (1)
V2CreateLedgerRequest(9-13)pkg/features/features.go (2)
MinimalFeatureSet(35-41)FeatureMovesHistoryPostCommitEffectiveVolumes(18-18)pkg/client/models/components/v2errorsenum.go (2)
V2ErrorsEnumValidation(15-15)V2ErrorsEnumLedgerAlreadyExists(29-29)pkg/testserver/server.go (1)
ExperimentalFeaturesInstrumentation(29-34)pkg/client/models/operations/v2getledger.go (2)
V2GetLedgerRequest(9-12)V2GetLedgerResponse(21-25)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (1)
internal/api/v2/controllers_ledgers_create.go (1)
34-40:ErrExperimentalFeaturesDisabledcorrectly mapped to 400, but double-check semanticsTreating a “feature disabled by config” situation as a validation error (400) is consistent with the existing mapping, yet it can be argued that this is more an authorization / capability issue (often 403).
Please ensure that:
- API docs & OpenAPI definition are updated accor 8000 dingly.
- Other endpoints that can raise the same error are also aligned, otherwise clients will face inconsistent status codes.
| JustBeforeEach(func(specContext SpecContext) { | ||
| _, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateLedger(ctx, operations.V2CreateLedgerRequest{ | ||
| Ledger: createLedgerRequest.Ledger, | ||
| }) | ||
| Expect(err).NotTo(BeNil()) | ||
| Expect(err).To(HaveErrorCode(string(components.V2ErrorsEnumLedgerAlreadyExists))) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Shadowed err hides outer variable
:= creates a new err, so the outer-scoped err checked by the (empty) It block is never updated.
Replace the short declaration with assignment to keep a single source of truth:
- _, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateLedger(ctx, operations.V2CreateLedgerRequest{
+ _, err = Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateLedger(ctx, operations.V2CreateLedgerRequest{📝 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.
| JustBeforeEach(func(specContext SpecContext) { | |
| _, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateLedger(ctx, operations.V2CreateLedgerRequest{ | |
| Ledger: createLedgerRequest.Ledger, | |
| }) | |
| Expect(err).NotTo(BeNil()) | |
| Expect(err).To(HaveErrorCode(string(components.V2ErrorsEnumLedgerAlreadyExists))) | |
| }) | |
| JustBeforeEach(func(specContext SpecContext) { | |
| _, err = Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateLedger(ctx, operations.V2CreateLedgerRequest{ | |
| Ledger: createLedgerRequest.Ledger, | |
| }) | |
| Expect(err).NotTo(BeNil()) | |
| Expect(err).To(HaveErrorCode(string(components.V2ErrorsEnumLedgerAlreadyExists))) | |
| }) |
🤖 Prompt for AI Agents
In test/e2e/api_ledgers_create_test.go around lines 128 to 134, the variable err
is redeclared using := inside the JustBeforeEach block, which shadows the outer
err variable and prevents it from being updated. Replace the short variable
declaration := with a simple assignment = to update the existing err variable
instead of creating a new one, ensuring the outer err reflects the result of the
CreateLedger call.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #964 +/- ##
==========================================
- Coverage 82.82% 82.78% -0.04%
==========================================
Files 142 142
Lines 8028 8028
==========================================
- Hits 6649 6646 -3
- Misses 1054 1056 +2
- Partials 325 326 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.