fix: obscur error message when trying to use experimental features while the service is not configured for #965
Conversation
…ile the service is not configured for
WalkthroughThe error handling for the ledger creation API was updated to treat attempts to use experimental features when disabled as a validation error, returning a 400 response. The end-to-end tests were refactored to explicitly cover scenarios with and without experimental features enabled, verifying the correct error responses in both cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant System
Client->>API: POST /ledgers (with experimental features)
API->>System: Validate ledger creation request
alt Experimental features disabled
System-->>API: Return ErrExperimentalFeaturesDisabled
API-->>Client: 400 Bad Request (validation error)
else Experimental features enabled
System-->>API: Ledger created successfully
API-->>Client: 201 Created
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/api/v2/controllers_ledgers_create.go (1)
35-39: Consider extracting the validation-error list to a helper for maintainabilityThe inline
switchpredicate is starting to get long and error-prone as new validation errors are appended:case errors.Is(err, system.ErrInvalidLedgerConfiguration{}) || errors.Is(err, ledger.ErrInvalidLedgerName{}) || errors.Is(err, ledger.ErrInvalidBucketName{}) || errors.Is(err, system.ErrExperimentalFeaturesDisabled):Moving this boolean to a small helper, e.g.
isValidationError(err error) bool, will:
- Keep the handler readable.
- Avoid duplication if the same check is required elsewhere.
- Make additions in future PRs less likely to introduce precedence mistakes.
This is a low-impact refactor that improves clarity.
test/e2e/api_ledgers_create_test.go (3)
24-32: DRY up duplicated test-server bootstrapThe two contexts spin up almost identical
NewTestServerblocks, differing only by theExperimentalFeaturesflag.
Consider parameterising this setup to reduce duplication and the risk of the two paths diverging:func newServer(enableExperimental bool) *TestServer { return NewTestServer(func() Configuration { cfg := Configuration{ PostgresConfiguration: db.GetValue().ConnectionOptions(), Output: GinkgoWriter, Debug: debug, NatsURL: natsServer.GetValue().ClientURL(), } cfg.ExperimentalFeatures = enableExperimental return cfg }) }Then:
testServer := newServer(false) // or trueThis keeps the suite concise and makes feature-toggle differences explicit.
Also applies to: 64-73
45-47:specContextparameter is unused
JustBeforeEach(func(specContext SpecContext) { ... })declaresspecContextbut never references it, which triggersgo vet’sunusedparamwarning.Either use the context (e.g. for timeouts/cancellation) or drop the parameter:
JustBeforeEach(func() { err = CreateLedger(ctx, testServer.GetValue(), createLedgerRequest) })
51-59: PreferHaveOccurred()for nil-check + readabilityIn several assertions you combine
NotTo(BeNil())/To(BeNil())with error variables.
Gomega’s idiomatic helper is shorter and reads better:Expect(err).To(HaveOccurred()) // instead of NotTo(BeNil()) Expect(err).NotTo(HaveOccurred()) // instead of To(BeNil())This is stylistic, but adopting the common convention improves consistency across the codebase.
Also applies to: 91-99
📜 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 (1)
internal/api/v2/controllers_ledgers_create.go (1)
internal/controller/system/errors.go (1)
ErrExperimentalFeaturesDisabled(11-11)
⏰ 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: Add unit-test coverage for the newErrExperimentalFeaturesDisabledbranchThe additional
errors.Is(err, system.ErrExperimentalFeaturesDisabled)check correctly classifies the error as a validation error.
However, the controller now has an extra branch that is not covered by the existing unit test suite for this handler.
Please add (or extend) a fast unit test – separate from the e2e tests – that:
- Mocks
system.Controller.CreateLedgerto returnsystem.ErrExperimentalFeaturesDisabled.- Verifies the handler responds with HTTP 400 and a validation error payload.
This guards against future regressions without relying solely on the heavier e2e flow.
Fixes LX-58