Add range check for numbers to prevent DynamoDB overflow#1488
Add range check for numbers to prevent DynamoDB overflow#1488rowanseymour merged 2 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1488 +/- ##
=======================================
Coverage 69.59% 69.59%
=======================================
Files 306 306
Lines 18060 18094 +34
=======================================
+ Hits 12568 12592 +24
- Misses 5059 5067 +8
- Partials 433 435 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rowanseymour
force-pushed
the
number_fix
branch
from
ff773d8 to
31ac6d8
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds range validation for numbers to prevent DynamoDB persistence failures caused by numbers exceeding the 38-digit limit of DynamoDB's N type. The implementation enforces a maximum of 36 significant digits and a magnitude range of ±1E100 for all number creation points in the codebase.
Changes:
- Added
CheckDecimalRangefunction to validate numbers have at most 36 significant digits and magnitude within ±1E100 - Modified
NewXNumberto returnXValueinstead of*XNumber, allowing it to return errors for out-of-range values - Added
ErrorLiteraltype to represent parse errors in expression trees, enabling number literals that exceed limits to be caught during parsing
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| excellent/types/number.go | Core implementation of range checking logic and API changes to NewXNumber |
| excellent/types/number_test.go | Comprehensive test coverage for range validation including edge cases |
| excellent/visitor.go | Updated number literal parsing to use NewXNumberFromString and return ErrorLiteral on failure |
| excellent/tree.go | Added ErrorLiteral type to represent parse errors in expression trees |
| excellent/operators/builtin.go | Updated documentation for Exponent operator to show overflow example |
| excellent/operators/builtin_test.go | Added test cases for arithmetic overflow scenarios |
| excellent/functions/builtin_test.go | Added test case for sum function overflow |
| flows/routers/cases/utils.go | Integrated CheckDecimalRange into ParseDecimal function |
| flows/routers/cases/utils_test.go | Added test for oversized number rejection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return x.Value.Describe() | ||
| } | ||
|
|
||
| // ErrorLiteral is a literal error value |
There was a problem hiding this comment.
Consider adding a more descriptive comment for ErrorLiteral explaining its purpose. For example: "ErrorLiteral represents a literal that failed to parse, such as a number literal that exceeds the maximum allowed digits or magnitude. This allows parse errors to be represented in the expression tree and evaluated to XError values." This would help developers understand when and why this type is used.
| // ErrorLiteral is a literal error value | |
| // ErrorLiteral represents a literal that failed to parse (for example, a number | |
| // literal that exceeds the maximum allowed digits or magnitude). This allows | |
| // parse errors to be represented in the expression tree and evaluated to XError | |
| // values. |
excellent/visitor.go
Outdated
| return &NumberLiteral{Value: types.RequireXNumberFromString(ctx.GetText())} | ||
| num, err := types.NewXNumberFromString(ctx.GetText()) | ||
| if err != nil { | ||
| return &ErrorLiteral{Err: types.NewXErrorf("invalid number")} |
There was a problem hiding this comment.
The error message "invalid number" is not specific enough. NewXNumberFromString can fail for multiple reasons: invalid format (e.g., "abc"), too many significant digits (e.g., 37+ digits), or magnitude out of range (e.g., 1E200 or 1E-200). Consider including the actual error from NewXNumberFromString in the error message to provide users with more specific feedback about why their number literal failed to parse.
| return &ErrorLiteral{Err: types.NewXErrorf("invalid number")} | |
| return &ErrorLiteral{Err: types.NewXErrorf("invalid number: %v", err)} |
| func (x *XNumber) UnmarshalJSON(data []byte) error { | ||
| return jsonx.Unmarshal(data, &x.native) | ||
| } |
There was a problem hiding this comment.
The UnmarshalJSON method directly unmarshals into x.native without performing range validation. This could allow numbers with more than 36 significant digits or magnitude outside ±1E100 to be deserialized from JSON, bypassing the range checks enforced elsewhere in the codebase. Consider adding CheckDecimalRange validation after unmarshaling to ensure consistency with other number creation paths.
| // only parse numbers like 123 or 123.456 or .456 | ||
| var decimalRegexp = regexp.MustCompile(`^-?(([0-9]+)|([0-9]+\.[0-9]+)|(\.[0-9]+))$`) | ||
|
|
||
| // MaxNumberDigits is the maximum number of significant digits in a number |
There was a problem hiding this comment.
The comment for MaxNumberDigits should clarify what "significant digits" means in this context. Consider expanding it to explain that this limit applies to the coefficient after removing trailing zeros, and that the limit exists to ensure numbers can be persisted to DynamoDB (which has a 38-digit limit for numeric types). This will help developers understand the rationale behind the constraint.
| // MaxNumberDigits is the maximum number of significant digits in a number | |
| // MaxNumberDigits is the maximum number of significant digits in a number's coefficient, | |
| // after removing any trailing zeros. This limit is chosen so that all numbers we accept | |
| // can be safely persisted to DynamoDB, which supports up to 38 total digits for numeric types. |
Numbers in expressions could grow arbitrarily large via parsing or arithmetic, causing persist failures when stored as DynamoDB N type (max 38 digits). This adds a range check (max 36 significant digits, magnitude ±1E100) enforced at all number creation points. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rowanseymour
force-pushed
the
number_fix
branch
from
31ac6d8 to
e845fba
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
rowanseymour
force-pushed
the
number_fix
branch
from
f2e52be to
dbfb89b
Compare
Numbers in expressions could grow arbitrarily large via parsing or arithmetic, causing persist failures when stored as DynamoDB N type (max 38 digits). This adds a range check (max 36 significant digits, magnitude ±1E100) enforced at all number creation points.