fix: add validations on grant input #3433
Conversation
📝 WalkthroughWalkthroughAdds two new validation error constants/variables in package credit and implements CreateGrantInput.Validate() with checks for positive Amount and presence of EffectiveAt; CreateGrant now calls Validate and returns early on validation failure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openmeter/credit/errors.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-21T08:32:31.689Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
Applied to files:
openmeter/credit/errors.go
⏰ 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). (9)
- GitHub Check: Artifacts / Container image
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Repository Scan
- GitHub Check: Lint
- GitHub Check: Analyze (go)
- GitHub Check: SAST (semgrep)
| const ErrCodeGrantAmountMustBePositive models.ErrorCode = "grant_amount_must_be_positive" | ||
|
|
||
| var ErrGrantAmountMustBePositive = models.NewValidationIssue( | ||
| ErrCodeGrantAmountMustBePositive, | ||
| "amount must be positive", | ||
| models.WithFieldString("amount"), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Validation error for amount looks well-structured.
The error code, message, and field selector are clear and follow conventions. The absence of an explicit severity option means it defaults to error severity, which correctly aligns with the blocking validation behavior.
Verify that the field selector "amount" matches the JSON tag in CreateGrantInput:
🏁 Script executed:
#!/bin/bash
# Description: Verify the field selector matches the CreateGrantInput struct JSON tag
# Search for CreateGrantInput struct definition and check the Amount field's JSON tag
ast-grep --pattern $'type CreateGrantInput struct {
$$$
}'Length of output: 570
I need to see the actual JSON tags on the struct fields to verify the field selectors are correct. Let me retrieve the complete struct definition:
I need to see the actual struct definition with its JSON tags. Let me search for the complete struct definition:
Wait, I need to use the actual codebase. Let me correct this:
🏁 Script executed:
#!/bin/bash
# Extract the complete CreateGrantInput struct definition with JSON tags
cat openmeter/credit/grant.go | sed -n '25,34p'Length of output: 280
🏁 Script executed:
#!/bin/bash
# Extract the complete CreateGrantInput struct definition with JSON tags from the file
sed -n '25,34p' openmeter/credit/grant.goLength of output: 280
I need to see if struct tags exist. Let me use a more detailed search:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for CreateGrantInput struct with full context including any struct tags
rg -A 10 'type CreateGrantInput struct' openmeter/credit/grant.go
Length of output: 281
I see the struct has no visible JSON tags. The field names are in PascalCase (Amount, EffectiveAt), but the error selectors use lowercase/camelCase ("amount", "effectiveAt"). This is a potential mismatch. Let me verify if there's custom marshaling or validation field mapping logic:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for WithFieldString implementation to understand how field names are mapped
rg -B 5 -A 15 'func WithFieldString' --type=go
Length of output: 1123
Let me check the NewFieldSelector implementation and verify if CreateGrantInput has custom JSON marshaling:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for NewFieldSelector implementation
rg -B 2 -A 10 'func NewFieldSelector' --type=go
Length of output: 823
Let me check if CreateGrantInput has any custom JSON marshaling methods:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for CreateGrantInput methods and JSON marshaling
rg -B 2 -A 8 'func.*CreateGrantInput' --type=go | head -50
Length of output: 2889
I found a critical issue! The Validate() method uses WithAttr("effective_at", ...) but the error is defined with field selector "effectiveAt". Let me verify the error definitions:
---
🏁 Script executed:
```shell
#!/bin/bash
# Show the complete errors.go file to verify field selectors
cat openmeter/credit/errors.go
Length of output: 573
Field selector "effectiveAt" must be changed to "effective_at" to match usage in CreateGrantInput.Validate().
In openmeter/credit/grant.go, the Validate() method uses WithAttr("effective_at", ...) when returning the error, but the field selector in errors.go is defined as "effectiveAt" (camelCase). This mismatch will cause validation errors to report an incorrect field name. The "amount" field selector is correct and matches its usage.
Change line 14 in openmeter/credit/errors.go from:
models.WithFieldString("effectiveAt"),to:
models.WithFieldString("effective_at"),🤖 Prompt for AI Agents
In openmeter/credit/errors.go around lines 5 to 11, the field selector for the
effective date is using camelCase ("effectiveAt") which mismatches the validator
that expects "effective_at"; update the field selector to "effective_at" so it
aligns with CreateGrantInput.Validate() (i.e., replace the
models.WithFieldString("effectiveAt") entry with
models.WithFieldString("effective_at")).
| const ErrCodeEffectiveAtMustBeSet models.ErrorCode = "grant_effective_at_must_be_set" | ||
|
|
||
| var ErrGrantEffectiveAtMustBeSet = models.NewValidationIssue( | ||
| ErrCodeEffectiveAtMustBeSet, | ||
| "effective at must be set", | ||
| models.WithFieldString("effectiveAt"), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Validation error for effectiveAt is correctly implemented.
The error follows the same well-structured pattern. The field selector uses "effectiveAt" (camelCase), which aligns with the previous review feedback.
Verify that the field selector "effectiveAt" matches the JSON tag in CreateGrantInput:
🏁 Script executed:
#!/bin/bash
# Description: Verify the field selector matches the CreateGrantInput struct JSON tag for EffectiveAt
# Search for CreateGrantInput struct and examine EffectiveAt field's JSON tag
rg -n -A2 -B2 'type CreateGrantInput struct' --type=goLength of output: 262
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full CreateGrantInput struct definition with more context
rg -n -A30 'type CreateGrantInput struct' --type=go openmeter/credit/grant.go
Length of output: 1070
---
🏁 Script executed:
```shell
#!/bin/bash
# Read the raw source to see the struct tags clearly
cat openmeter/credit/grant.go | sed -n '25,34p'
Length of output: 280
---
🏁 Script executed:
```shell
#!/bin/bash
# Check other errors in the same file to see the pattern for WithFieldString
rg -n 'WithFieldString' openmeter/credit/errors.go
echo "---"
# Check the Validate method to see what attribute names are used
rg -n 'WithAttr' openmeter/credit/grant.go
Length of output: 235
Inconsistent field names in error definition and validation: "effectiveAt" vs "effective_at"
The error definition at line 18 of openmeter/credit/errors.go uses WithFieldString("effectiveAt") (camelCase), but the validation method in openmeter/credit/grant.go:41 calls .WithAttr("effective_at", ...) (snake_case) when using this error. This mismatch must be corrected—the field names in both locations must be identical:
- Either change errors.go line 18 to use
"effective_at"(snake_case), or - Change grant.go line 41 to use
"effectiveAt"(camelCase)
For reference, the ErrGrantAmountMustBePositive error follows the correct pattern: both the error definition and Validate() method consistently use "amount".
🤖 Prompt for AI Agents
In openmeter/credit/errors.go around lines 13 to 19, the field name used in
ErrGrantEffectiveAtMustBeSet is "effectiveAt" (camelCase) but the validation in
openmeter/credit/grant.go uses "effective_at" (snake_case); make them identical
by updating errors.go to use "effective_at" in the models.WithFieldString call
so the error definition matches the validator, keeping the error code and
message unchanged.
Overview
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.