Repository: genjidb/genji
Pull request #103 (2020-06-24 06:13:20)

Better INSERT parsing
For INSERT, some checks were done during execution time while they could have been done during parsing time. This PR fixes that for the two following errors:

```sql
// not the same number of elements, len(a, b)!= len(1, 2, 3)
INSERT INTO foo (a, b) VALUES (1, 2, 3)

// bad input:

// list of fields (a, b) with document value {a: 1}
INSERT INTO foo (a, b) VALUES (1, 2), {a: 1}
// expecting documents, got expression list (1, 2)
INSERT INTO foo VALUES (1, 2)
```


Patch:
diff --git a/sql/parser/expr.go b/sql/parser/expr.go
index 23574c82..26741578 100644
--- a/sql/parser/expr.go
+++ b/sql/parser/expr.go
@@ -203,7 +203,7 @@ func (p *Parser) parseUnaryExpr() (expr.Expr, error) {
 		return expr.DurationValue(d), nil
 	case scanner.LBRACKET:
 		p.Unscan()
-		e, _, err := p.parseDocument()
+		e, err := p.parseDocument()
 		return e, err
 	case scanner.LSBRACKET:
 		p.Unscan()
@@ -310,11 +310,10 @@ func (p *Parser) parseType() document.ValueType {
 }
 
 // parseDocument parses a document
-func (p *Parser) parseDocument() (expr.Expr, bool, error) {
+func (p *Parser) parseDocument() (expr.Expr, error) {
 	// Parse { token.
-	if tok, _, _ := p.ScanIgnoreWhitespace(); tok != scanner.LBRACKET {
-		p.Unscan()
-		return nil, false, nil
+	if tok, pos, lit := p.ScanIgnoreWhitespace(); tok != scanner.LBRACKET {
+		return nil, newParseError(scanner.Tokstr(tok, lit), []string{"{"}, pos)
 	}
 
 	var pairs expr.KVPairs
@@ -338,10 +337,10 @@ func (p *Parser) parseDocument() (expr.Expr, bool, error) {
 
 	// Parse required } token.
 	if tok, pos, lit := p.ScanIgnoreWhitespace(); tok != scanner.RBRACKET {
-		return nil, true, newParseError(scanner.Tokstr(tok, lit), []string{"}"}, pos)
+		return nil, newParseError(scanner.Tokstr(tok, lit), []string{"}"}, pos)
 	}
 
-	return pairs, true, nil
+	return pairs, nil
 }
 
 // parseKV parses a key-value pair in the form IDENT : Expr.
diff --git a/sql/parser/insert.go b/sql/parser/insert.go
index d0ba5794..4a70b3d9 100644
--- a/sql/parser/insert.go
+++ b/sql/parser/insert.go
@@ -1,6 +1,8 @@
 package parser
 
 import (
+	"fmt"
+
 	"github.com/genjidb/genji/sql/query"
 	"github.com/genjidb/genji/sql/query/expr"
 	"github.com/genjidb/genji/sql/scanner"
@@ -23,21 +25,35 @@ func (p *Parser) parseInsertStatement() (query.InsertStmt, error) {
 		return stmt, err
 	}
 
+	valueParser := p.parseParamOrDocument
+
 	// Parse field list: (a, b, c)
-	fields, ok, err := p.parseFieldList()
+	fields, withFields, err := p.parseFieldList()
 	if err != nil {
 		return stmt, err
 	}
-	if ok {
+	if withFields {
+		valueParser = p.parseParamOrExprList
 		stmt.FieldNames = fields
 	}
 
 	// Parse VALUES (v1, v2, v3)
-	stmt.Values, err = p.parseValues()
+	values, err := p.parseValues(valueParser)
 	if err != nil {
 		return stmt, err
 	}
 
+	// ensure the length of field list is the same as the length of values
+	if withFields {
+		for _, l := range values {
+			el := l.(expr.LiteralExprList)
+			if len(el) != len(stmt.FieldNames) {
+				return stmt, fmt.Errorf("%d values for %d fields", len(el), len(stmt.FieldNames))
+			}
+		}
+	}
+
+	stmt.Values = values
 	return stmt, nil
 }
 
@@ -65,7 +81,7 @@ func (p *Parser) parseFieldList() ([]string, bool, error) {
 }
 
 // parseValues parses the "VALUES" clause of the query, if it exists.
-func (p *Parser) parseValues() (expr.LiteralExprList, error) {
+func (p *Parser) parseValues(valueParser func() (expr.Expr, error)) (expr.LiteralExprList, error) {
 	// Check if the VALUES token exists.
 	if tok, pos, lit := p.ScanIgnoreWhitespace(); tok != scanner.VALUES {
 		return nil, newParseError(scanner.Tokstr(tok, lit), []string{"VALUES"}, pos)
@@ -73,7 +89,7 @@ func (p *Parser) parseValues() (expr.LiteralExprList, error) {
 
 	var valuesList expr.LiteralExprList
 	// Parse first (required) value list.
-	d, err := p.parseValue()
+	d, err := valueParser()
 	if err != nil {
 		return nil, err
 	}
@@ -87,7 +103,7 @@ func (p *Parser) parseValues() (expr.LiteralExprList, error) {
 			break
 		}
 
-		d, err := p.parseValue()
+		d, err := valueParser()
 		if err != nil {
 			return nil, err
 		}
@@ -98,8 +114,8 @@ func (p *Parser) parseValues() (expr.LiteralExprList, error) {
 	return valuesList, nil
 }
 
-// parseValue parses either a parameter, a JSON document or a list of expressions.
-func (p *Parser) parseValue() (expr.Expr, error) {
+// parseParamOrDocument parses either a parameter or a document.
+func (p *Parser) parseParamOrDocument() (expr.Expr, error) {
 	// Parse a param first
 	prm, err := p.parseParam()
 	if err != nil {
@@ -112,25 +128,24 @@ func (p *Parser) parseValue() (expr.Expr, error) {
 	// If not a param, start over
 	p.Unscan()
 
-	// check if it's a json document
-	expr, ok, err := p.parseDocument()
-	if err != nil || ok {
-		return expr, err
-	}
-
-	// if not a document, start over
-	p.Unscan()
+	// Expect a document
+	return p.parseDocument()
+}
 
-	// check if it's an expression list
-	if tok, _, _ := p.ScanIgnoreWhitespace(); tok != scanner.LPAREN {
-		tok, pos, lit := p.ScanIgnoreWhitespace()
-		return nil, newParseError(scanner.Tokstr(tok, lit), []string{"expression list or JSON"}, pos)
-	}
-	p.Unscan()
-	expr, err = p.parseExprList(scanner.LPAREN, scanner.RPAREN)
+// parseParamOrExprList parses either a parameter or a list of expressions.
+func (p *Parser) parseParamOrExprList() (expr.Expr, error) {
+	// Parse a param first
+	prm, err := p.parseParam()
 	if err != nil {
 		return nil, err
 	}
+	if prm != nil {
+		return prm, nil
+	}
+
+	// If not a param, start over
+	p.Unscan()
 
-	return expr, nil
+	// expect an expression list
+	return p.parseExprList(scanner.LPAREN, scanner.RPAREN)
 }
diff --git a/sql/query/insert.go b/sql/query/insert.go
index dd821424..7da0bef9 100644
--- a/sql/query/insert.go
+++ b/sql/query/insert.go
@@ -2,7 +2,6 @@ package query
 
 import (
 	"errors"
-	"fmt"
 
 	"github.com/genjidb/genji/database"
 	"github.com/genjidb/genji/document"
@@ -60,10 +59,6 @@ func (stmt InsertStmt) insertDocuments(t *database.Table, stack expr.EvalStack)
 			return res, err
 		}
 
-		if v.Type != document.DocumentValue {
-			return res, errors.New("values must be a list of documents if field list is empty")
-		}
-
 		d, err := v.ConvertToDocument()
 		if err != nil {
 			return res, err
@@ -94,24 +89,11 @@ func (stmt InsertStmt) insertExprList(t *database.Table, stack expr.EvalStack) (
 
 		// each document must be a list of expressions
 		// (e1, e2, e3, ...) or [e1, e2, e2, ....]
-		if v.Type != document.ArrayValue {
-			return res, errors.New("invalid values")
-		}
-
 		vlist, err := v.ConvertToArray()
 		if err != nil {
 			return res, err
 		}
 
-		lenv, err := document.ArrayLength(vlist)
-		if err != nil {
-			return res, err
-		}
-
-		if len(stmt.FieldNames) != lenv {
-			return res, fmt.Errorf("%d values for %d fields", lenv, len(stmt.FieldNames))
-		}
-
 		// iterate over each value
 		vlist.Iterate(func(i int, v document.Value) error {
 			// get the field name


Test patch:
diff --git a/sql/parser/insert_test.go b/sql/parser/insert_test.go
index 53dd0c75..30f9a3c0 100644
--- a/sql/parser/insert_test.go
+++ b/sql/parser/insert_test.go
@@ -52,14 +52,16 @@ func TestParserInsert(t *testing.T) {
 				Values:    expr.LiteralExprList{expr.NamedParam("foo"), expr.NamedParam("bar")},
 			},
 			false},
-		{"Values / With columns", "INSERT INTO test (a, b) VALUES ('c', 'd', 'e')",
+		{"Values / With fields", "INSERT INTO test (a, b) VALUES ('c', 'd')",
 			query.InsertStmt{
 				TableName:  "test",
 				FieldNames: []string{"a", "b"},
 				Values: expr.LiteralExprList{
-					expr.LiteralExprList{expr.TextValue("c"), expr.TextValue("d"), expr.TextValue("e")},
+					expr.LiteralExprList{expr.TextValue("c"), expr.TextValue("d")},
 				},
 			}, false},
+		{"Values / With too many values", "INSERT INTO test (a, b) VALUES ('c', 'd', 'e')",
+			nil, true},
 		{"Values / Multiple", "INSERT INTO test (a, b) VALUES ('c', 'd'), ('e', 'f')",
 			query.InsertStmt{
 				TableName:  "test",
@@ -69,6 +71,10 @@ func TestParserInsert(t *testing.T) {
 					expr.LiteralExprList{expr.TextValue("e"), expr.TextValue("f")},
 				},
 			}, false},
+		{"Values / With fields / Wrong values", "INSERT INTO test (a, b) VALUES {a: 1}, ('e', 'f')",
+			nil, true},
+		{"Values / Without fields / Wrong values", "INSERT INTO test VALUES {a: 1}, ('e', 'f')",
+			nil, true},
 	}
 
 	for _, test := range tests {