Repository: genjidb/genji
Pull request #425 (2021-07-25 16:14:28)

Handle integer conversion errors
When converting a double larger or equal to `math.MaxInt64` to an integer, it previously overflowed silently. It now returns an explicit error. 

This fixes along the way, the `math.abs(-9223372036854775808)` issue. 

codecov-commenter: # [Codecov](https://codecov.io/gh/genjidb/genji/pull/425?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb) Report
> Merging [#425](https://codecov.io/gh/genjidb/genji/pull/425?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb) (bd29ce8) into [main](https://codecov.io/gh/genjidb/genji/commit/daf4f79e9f133bf09d8dbe66f655e621bbc85f1b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb) (daf4f79) will **decrease** coverage by `0.09%`.
> The diff coverage is `22.22%`.

[![Impacted file tree graph](https://codecov.io/gh/genjidb/genji/pull/425/graphs/tree.svg?width=650&height=150&src=pr&token=nwR1GpvtyH&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb)](https://codecov.io/gh/genjidb/genji/pull/425?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb)

```diff
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
- Coverage   68.88%   68.79%   -0.10%     
==========================================
  Files          93       93              
  Lines        9633     9648      +15     
==========================================
+ Hits         6636     6637       +1     
- Misses       2281     2289       +8     
- Partials      716      722       +6     
```


| [Impacted Files](https://codecov.io/gh/genjidb/genji/pull/425?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb) | Coverage Δ | |
|---|---|---|
| [document/cast.go](https://codecov.io/gh/genjidb/genji/pull/425/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb#diff-ZG9jdW1lbnQvY2FzdC5nbw==) | `84.29% <22.22%> (-9.10%)` | :arrow_down: |
| [driver/driver.go](https://codecov.io/gh/genjidb/genji/pull/425/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb#diff-ZHJpdmVyL2RyaXZlci5nbw==) | `66.27% <0.00%> (-1.17%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/genjidb/genji/pull/425?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/genjidb/genji/pull/425?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb). Last update [daf4f79...bd29ce8](https://codecov.io/gh/genjidb/genji/pull/425?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb).


Patch:
diff --git a/document/cast.go b/document/cast.go
index 2c6271e4..d40ded02 100644
--- a/document/cast.go
+++ b/document/cast.go
@@ -14,11 +14,6 @@ func CastAs(v types.Value, t types.ValueType) (types.Value, error) {
 		return v, nil
 	}
 
-	// Null values always remain null.
-	if v.Type() == types.NullValue {
-		return v, nil
-	}
-
 	switch t {
 	case types.BoolValue:
 		return CastAsBool(v)
@@ -45,6 +40,11 @@ func CastAs(v types.Value, t types.ValueType) (types.Value, error) {
 // it fails if the text doesn't contain a valid boolean.
 // Any other type is considered an invalid cast.
 func CastAsBool(v types.Value) (types.Value, error) {
+	// Null values always remain null.
+	if v.Type() == types.NullValue {
+		return v, nil
+	}
+
 	switch v.Type() {
 	case types.BoolValue:
 		return v, nil
@@ -70,6 +70,11 @@ func CastAsBool(v types.Value) (types.Value, error) {
 // It fails if the text doesn't contain a valid float value.
 // Any other type is considered an invalid cast.
 func CastAsInteger(v types.Value) (types.Value, error) {
+	// Null values always remain null.
+	if v.Type() == types.NullValue {
+		return v, nil
+	}
+
 	switch v.Type() {
 	case types.IntegerValue:
 		return v, nil
@@ -79,7 +84,11 @@ func CastAsInteger(v types.Value) (types.Value, error) {
 		}
 		return types.NewIntegerValue(0), nil
 	case types.DoubleValue:
-		return types.NewIntegerValue(int64(v.V().(float64))), nil
+		f := v.V().(float64)
+		if f > 0 && int64(f) < 0 {
+			return nil, stringutil.Errorf("integer out of range")
+		}
+		return types.NewIntegerValue(int64(f)), nil
 	case types.TextValue:
 		i, err := strconv.ParseInt(v.V().(string), 10, 64)
 		if err != nil {
@@ -102,6 +111,11 @@ func CastAsInteger(v types.Value) (types.Value, error) {
 // it fails if the text doesn't contain a valid float value.
 // Any other type is considered an invalid cast.
 func CastAsDouble(v types.Value) (types.Value, error) {
+	// Null values always remain null.
+	if v.Type() == types.NullValue {
+		return v, nil
+	}
+
 	switch v.Type() {
 	case types.DoubleValue:
 		return v, nil
@@ -121,6 +135,11 @@ func CastAsDouble(v types.Value) (types.Value, error) {
 // CastAsText returns a JSON representation of v.
 // If the representation is a string, it gets unquoted.
 func CastAsText(v types.Value) (types.Value, error) {
+	// Null values always remain null.
+	if v.Type() == types.NullValue {
+		return v, nil
+	}
+
 	switch v.Type() {
 	case types.TextValue:
 		return v, nil
@@ -142,6 +161,11 @@ func CastAsText(v types.Value) (types.Value, error) {
 // Text: decodes a base64 string, otherwise fails.
 // Any other type is considered an invalid cast.
 func CastAsBlob(v types.Value) (types.Value, error) {
+	// Null values always remain null.
+	if v.Type() == types.NullValue {
+		return v, nil
+	}
+
 	if v.Type() == types.BlobValue {
 		return v, nil
 	}
@@ -164,6 +188,11 @@ func CastAsBlob(v types.Value) (types.Value, error) {
 // Text: decodes a JSON array, otherwise fails.
 // Any other type is considered an invalid cast.
 func CastAsArray(v types.Value) (types.Value, error) {
+	// Null values always remain null.
+	if v.Type() == types.NullValue {
+		return v, nil
+	}
+
 	if v.Type() == types.ArrayValue {
 		return v, nil
 	}
@@ -185,6 +214,11 @@ func CastAsArray(v types.Value) (types.Value, error) {
 // Text: decodes a JSON object, otherwise fails.
 // Any other type is considered an invalid cast.
 func CastAsDocument(v types.Value) (types.Value, error) {
+	// Null values always remain null.
+	if v.Type() == types.NullValue {
+		return v, nil
+	}
+
 	if v.Type() == types.DocumentValue {
 		return v, nil
 	}


Test patch:
diff --git a/document/cast_test.go b/document/cast_test.go
index 9785bbc4..9873dd4f 100644
--- a/document/cast_test.go
+++ b/document/cast_test.go
@@ -1,6 +1,7 @@
 package document
 
 import (
+	"math"
 	"testing"
 
 	"github.com/genjidb/genji/types"
@@ -70,6 +71,7 @@ func TestCastAs(t *testing.T) {
 			{blobV, nil, true},
 			{arrayV, nil, true},
 			{docV, nil, true},
+			{types.NewDoubleValue(math.MaxInt64 + 1), nil, true},
 		})
 	})
 
diff --git a/internal/expr/functions/testdata/math_functions.sql b/internal/expr/functions/testdata/math_functions.sql
index d6a44684..1e5d9da9 100644
--- a/internal/expr/functions/testdata/math_functions.sql
+++ b/internal/expr/functions/testdata/math_functions.sql
@@ -17,6 +17,8 @@ NULL
 2.0
 ! math.abs('foo')
 'cannot cast "foo" as double'
+! math.abs(-9223372036854775808)
+'integer out of range'
 
 -- test: math.acos
 > math.acos(NULL)
diff --git a/internal/testutil/expr.go b/internal/testutil/expr.go
index 6a552e05..3a5e3949 100644
--- a/internal/testutil/expr.go
+++ b/internal/testutil/expr.go
@@ -152,7 +152,7 @@ func ExprRunner(t *testing.T, testfile string) {
 						require.NoError(t, err)
 
 						// eval it to get a proper Value
-						want, err := e.Eval(emptyEnv)
+						want, err := e.Eval(environment.New(nil))
 						require.NoError(t, err)
 
 						// parse the given expr
@@ -160,7 +160,7 @@ func ExprRunner(t *testing.T, testfile string) {
 						require.NoError(t, err)
 
 						// eval it to get a proper Value
-						got, err := e.Eval(emptyEnv)
+						got, err := e.Eval(environment.New(nil))
 						require.NoError(t, err)
 
 						// finally, compare those two
@@ -170,12 +170,14 @@ func ExprRunner(t *testing.T, testfile string) {
 					t.Run("NOK "+stmt.Expr, func(t *testing.T) {
 						// parse the given epxr
 						e, err := parser.NewParser(strings.NewReader(stmt.Expr)).ParseExpr()
-						require.NoError(t, err)
-
-						// eval it, it should return an error
-						_, err = e.Eval(emptyEnv)
-						require.NotNilf(t, err, "expected expr `%s` to return an error, got nil", stmt.Expr)
-						require.Regexp(t, regexp.MustCompile(regexp.QuoteMeta(stmt.Res)), err.Error())
+						if err != nil {
+							require.Regexp(t, regexp.MustCompile(regexp.QuoteMeta(stmt.Res)), err.Error())
+						} else {
+							// eval it, it should return an error
+							_, err = e.Eval(environment.New(nil))
+							require.NotNilf(t, err, "expected expr `%s` to return an error, got nil", stmt.Expr)
+							require.Regexp(t, regexp.MustCompile(regexp.QuoteMeta(stmt.Res)), err.Error())
+						}
 					})
 				}
 			}