Fix comparison of numeric jsonb columns

Since we use ->> on jsonb fields we always get a string back.
Comparisons such as < and > were done on string values which gives
unexpected results.

Author: erikdubbelboer

examples/readme_test.go

@@ -33,6 +33,6 @@ func ExampleNewConverter_readme() {
 	fmt.Println(conditions)
 	fmt.Printf("%#v\n", values)
 	// Output:
-	// ((("meta"->>'map' ~* $1) OR ("meta"->>'map' ~* $2)) AND ("meta"->>'password' = $3) AND (("meta"->>'playerCount' >= $4) AND ("meta"->>'playerCount' < $5)))
+	// ((("meta"->>'map' ~* $1) OR ("meta"->>'map' ~* $2)) AND ("meta"->>'password' = $3) AND ((("meta"->>'playerCount')::numeric >= $4) AND (("meta"->>'playerCount')::numeric < $5)))
 	// []interface {}{"aztec", "nuke", "", 2, 10}
 }

filter/converter.go

@@ -10,11 +10,14 @@ import (
 	"sync"
 )
 
-var basicOperatorMap = map[string]string{
-	"$gt":    ">",
-	"$gte":   ">=",
-	"$lt":    "<",
-	"$lte":   "<=",
+var numericOperatorMap = map[string]string{
+	"$gt":  ">",
+	"$gte": ">=",
+	"$lt":  "<",
+	"$lte": "<=",
+}
+
+var textOperatorMap = map[string]string{
 	"$eq":    "=",
 	"$ne":    "!=",
 	"$regex": "~*",
@@ -200,14 +203,7 @@ func (c *Converter) convertFilter(filter map[string]any, paramIndex int) (string
 						values = append(values, v[operator])
 					case "$exists":
 						// $exists only works on jsonb columns, so we need to check if the key is in the JSONB data first.
-						isNestedColumn := c.nestedColumn != ""
-						for _, exemption := range c.nestedExemptions {
-							if exemption == key {
-								isNestedColumn = false
-								break
-							}
-						}
-						if !isNestedColumn {
+						if !c.isNestedColumn(key) {
 							// There is no way in Postgres to check if a column exists on a table.
 							return "", nil, fmt.Errorf("$exists operator not supported on non-nested jsonb columns")
 						}
@@ -217,20 +213,14 @@ func (c *Converter) convertFilter(filter map[string]any, paramIndex int) (string
 						}
 						inner = append(inner, fmt.Sprintf("(%sjsonb_path_match(%s, 'exists($.%s)'))", neg, c.nestedColumn, key))
 					case "$elemMatch":
-						// $elemMatch needs a different implementation depending on if the column is in JSONB or not.
-						isNestedColumn := c.nestedColumn != ""
-						for _, exemption := range c.nestedExemptions {
-							if exemption == key {
-								isNestedColumn = false
-								break
-							}
-						}
 						innerConditions, innerValues, err := c.convertFilter(map[string]any{c.placeholderName: v[operator]}, paramIndex)
 						if err != nil {
 							return "", nil, err
 						}
 						paramIndex += len(innerValues)
-						if isNestedColumn {
+
+						// $elemMatch needs a different implementation depending on if the column is in JSONB or not.
+						if c.isNestedColumn(key) {
 							// This will for example become:
 							//
 							//   EXISTS (SELECT 1 FROM jsonb_array_elements("meta"->'foo') AS __filter_placeholder WHERE ("__filter_placeholder"::text = $1))
@@ -247,11 +237,29 @@ func (c *Converter) convertFilter(filter map[string]any, paramIndex int) (string
 						values = append(values, innerValues...)
 					default:
 						value := v[operator]
-						op, ok := basicOperatorMap[operator]
+						isNumericOperatorMap := false
+						op, ok := textOperatorMap[operator]
 						if !ok {
-							return "", nil, fmt.Errorf("unknown operator: %s", operator)
+							op, ok = numericOperatorMap[operator]
+							if !ok {
+								return "", nil, fmt.Errorf("unknown operator: %s", operator)
+							}
+							isNumericOperatorMap = true
+						}
+
+						if !isScalar(value) {
+							return "", nil, fmt.Errorf("invalid comparison value (must be a primitive): %v", value)
+						}
+
+						if isNumericOperatorMap && isNumeric(value) {
+							if c.isNestedColumn(key) {
+								inner = append(inner, fmt.Sprintf("((%s)::numeric %s $%d)", c.columnName(key), op, paramIndex))
+							} else {
+								inner = append(inner, fmt.Sprintf("(%s %s $%d)", c.columnName(key), op, paramIndex))
+							}
+						} else {
+							inner = append(inner, fmt.Sprintf("(%s %s $%d)", c.columnName(key), op, paramIndex))
 						}
-						inner = append(inner, fmt.Sprintf("(%s %s $%d)", c.columnName(key), op, paramIndex))
 						paramIndex++
 						values = append(values, value)
 					}
@@ -308,3 +316,15 @@ func (c *Converter) columnName(column string) string {
 	}
 	return fmt.Sprintf(`%q->>'%s'`, c.nestedColumn, column)
 }
+
+func (c *Converter) isNestedColumn(column string) bool {
+	if c.nestedColumn == "" {
+		return false
+	}
+	for _, exemption := range c.nestedExemptions {
+		if exemption == column {
+			return false
+		}
+	}
+	return true
+}

filter/converter_test.go

@@ -358,6 +358,14 @@ func TestConverter_Convert(t *testing.T) {
 			[]any{float64(18)},
 			nil,
 		},
+		{
+			"numeric comparison bug with jsonb column",
+			filter.WithNestedJSONB("meta"),
+			`{"foo": {"$gt": 0}}`,
+			`(("meta"->>'foo')::numeric > $1)`,
+			[]any{float64(0)},
+			nil,
+		},
 	}
 
 	for _, tt := range tests {

filter/util.go

@@ -1,5 +1,18 @@
 package filter
 
+func isNumeric(v any) bool {
+	if v == nil {
+		return true
+	}
+
+	switch v.(type) {
+	case float64:
+		return true
+	default:
+		return false
+	}
+}
+
 func isScalar(v any) bool {
 	if v == nil {
 		return true

integration/postgres_test.go

@@ -286,10 +286,16 @@ func TestIntegration_BasicOperators(t *testing.T) {
 			nil,
 		},
 		{
-			`invalid value`,
+			`invalid value type 1`,
 			`{"level": "town1"}`, // Level is an integer column, but the value is a string.
 			nil,
-			errors.New("pq: invalid input syntax for type integer: \"town1\""),
+			errors.New(`pq: invalid input syntax for type integer: "town1"`),
+		},
+		{
+			`invalid value type 2`,
+			`{"name": 123}`, // Name is a string column, but the value is an integer.
+			[]int{},
+			nil,
 		},
 		{
 			`empty object`,
@@ -381,6 +387,12 @@ func TestIntegration_BasicOperators(t *testing.T) {
 			[]int{3},
 			nil,
 		},
+		{
+			"numeric comparison bug bug with jsonb column",
+			`{"guild_id": {"$lt": 100}}`,
+			[]int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}, // wrong
+			nil,
+		},
 	}
 
 	for _, tt := range tests {