fix: add validations on grant input #3433
Conversation
📝 WalkthroughWalkthroughIntroduces validation errors and implements input validation for credit grants. Adds new error codes and validation issue variables for amount positivity and required effective-at timestamp. Integrates a Validate method into CreateGrantInput and invokes it in CreateGrant to fail early on invalid input. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
openmeter/credit/grant.go (1)
36-44: Validation is good; consider finite-number check and consistent field keys.
- Add a guard for NaN/Inf (NaN currently passes the >0 check due to comparison semantics).
- Align attribute key with API field casing (“effectiveAt” vs “effective_at”) for consistency with JSON/SDKs.
Apply this diff to extend validation and align the attribute key:
func (i CreateGrantInput) Validate() error { - if i.Amount <= 0 { - return ErrGrantAmountMustBePositive.WithAttr("amount", i.Amount) - } - if i.EffectiveAt.IsZero() { - return ErrGrantEffectiveAtMustBeSet.WithAttr("effective_at", i.EffectiveAt) - } + if i.Amount <= 0 { + return ErrGrantAmountMustBePositive.WithAttr("amount", i.Amount) + } + // Reject non-finite values + if math.IsNaN(i.Amount) || math.IsInf(i.Amount, 0) { + return models.NewGenericValidationError(fmt.Errorf("amount must be a finite number")) + } + if i.EffectiveAt.IsZero() { + return ErrGrantEffectiveAtMustBeSet.WithAttr("effectiveAt", i.EffectiveAt) + } return nil }Additionally add the import:
import "math"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openmeter/credit/errors.go(1 hunks)openmeter/credit/grant.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
openmeter/credit/grant.go (3)
openmeter/credit/errors.go (2)
ErrGrantAmountMustBePositive(7-12)ErrGrantEffectiveAtMustBeSet(16-21)openmeter/credit/grant/grant.go (1)
Grant(14-58)openmeter/ent/schema/grant.go (5)
Grant(15-17)Grant(19-26)Grant(28-59)Grant(61-66)Grant(68-77)
openmeter/credit/errors.go (2)
pkg/models/errors.go (1)
ErrorCode(294-294)pkg/models/validationissue.go (3)
NewValidationIssue(219-230)WithFieldString(196-202)WithWarningSeverity(214-216)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Artifacts / Container image
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
openmeter/credit/grant.go (1)
50-52: Early fail on invalid input looks right.
Keep this short‑circuit to avoid unnecessary DB work; ensure these validation errors map to a 4xx at the API boundary with structured issues.Can you confirm the HTTP mapping and response shape for these validation issues (status code, body schema)?
openmeter/credit/errors.go (1)
5-21: Ensure With methods on ValidationIssue return a new copy*
Verify that all option methods (WithFieldString, WithWarningSeverity, etc.) use value receivers and don’t mutate the original ValidationIssue; if they mutate in place, package-level vars (ErrGrantAmountMustBePositive, ErrGrantEffectiveAtMustBeSet) will share state and race.
| var ErrGrantAmountMustBePositive = models.NewValidationIssue( | ||
| ErrCodeGrantAmountMustBePositive, | ||
| "amount must be positive", | ||
| models.WithFieldString("amount"), | ||
| models.WithWarningSeverity(), | ||
| ) |
There was a problem hiding this comment.
Severity mismatch: returning “warning” issues to fail the request.
You’re using WithWarningSeverity yet Validate() returns these as errors that block creation. Align severity with behavior.
Option A (preferred if available): switch to error severity.
var ErrGrantAmountMustBePositive = models.NewValidationIssue(
ErrCodeGrantAmountMustBePositive,
"amount must be positive",
models.WithFieldString("amount"),
- models.WithWarningSeverity(),
+ models.WithErrorSeverity(),
)
var ErrGrantEffectiveAtMustBeSet = models.NewValidationIssue(
ErrCodeEffectiveAtMustBeSet,
"effective at must be set",
- models.WithFieldString("effective_at"),
- models.WithWarningSeverity(),
+ models.WithFieldString("effectiveAt"),
+ models.WithErrorSeverity(),
)Option B (if no WithErrorSeverity exists): drop the explicit warning so it uses the default error severity.
- models.WithWarningSeverity(),
+ // default to error severityAlso applies to: 16-21
🤖 Prompt for AI Agents
In openmeter/credit/errors.go around lines 7-12 (and similarly for lines 16-21),
the validation issues are created with WithWarningSeverity but Validate() treats
them as blocking errors; change the severity to an error by replacing
WithWarningSeverity() with WithErrorSeverity(), or if WithErrorSeverity() is not
available, remove the explicit WithWarningSeverity() call so the default (error)
severity is used, ensuring the declaration matches the runtime behavior.
| var ErrGrantEffectiveAtMustBeSet = models.NewValidationIssue( | ||
| ErrCodeEffectiveAtMustBeSet, | ||
| "effective at must be set", | ||
| models.WithFieldString("effective_at"), | ||
| models.WithWarningSeverity(), | ||
| ) |
There was a problem hiding this comment.
Field selector casing likely mismatched with API (“effective_at” vs “effectiveAt”).
Use the JSON field to help clients pinpoint issues.
Apply this diff if you keep Option A above:
- models.WithFieldString("effective_at"),
+ models.WithFieldString("effectiveAt"),🤖 Prompt for AI Agents
In openmeter/credit/errors.go around lines 16 to 21, the field selector uses
snake_case "effective_at" which likely mismatches the API JSON field; update the
validation issue to use the JSON field name "effectiveAt" (e.g., change
models.WithFieldString("effective_at") to models.WithFieldString("effectiveAt"))
so clients receive the correct field path for errors; keep the rest of the error
construction and warning severity unchanged.
1 participant
Overview
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit