Repository: genjidb/genji
Pull request #167 (2020-08-25 21:09:11)

Revert "Allow dot notation for update set."
This PR reverts the changes introduced by genjidb/genji#127 as they still contain a few bugs.

Issue https://github.com/genjidb/genji/issues/84 will be postponed to release v0.8


Patch:
diff --git a/document/document.go b/document/document.go
index 30e66044..e985d2ae 100644
--- a/document/document.go
+++ b/document/document.go
@@ -86,101 +86,16 @@ func (fb FieldBuffer) GetByField(field string) (Value, error) {
 	return Value{}, ErrFieldNotFound
 }
 
-// setFieldValue replaces a field if it already exists or creates one if not.
-func (fb *FieldBuffer) setFieldValue(field string, reqValue Value) error {
-	_, err := fb.GetByField(field)
-	switch err {
-	case ErrFieldNotFound:
-		fb.Add(field, reqValue)
-		return nil
-	case nil:
-		_ = fb.Replace(field, reqValue)
-		return nil
-	}
-
-	return err
-}
-
-// setValueAtPath deep replaces or creates a field
-// using the value path
-func (fb *FieldBuffer) setValueAtPath(v Value, p ValuePath, newValue Value) (Value, error) {
-
-	switch v.Type {
-	case DocumentValue:
-		var buf FieldBuffer
-		err := buf.ScanDocument(v.V.(Document))
-		if err != nil {
-			return v, err
-		}
-
-		if len(p) == 1 {
-			err = buf.setFieldValue(p[0], newValue)
-			return NewDocumentValue(&buf), err
-		}
-
-		va, err := buf.GetByField(p[0])
-		if err != nil {
-			return v, err
-		}
-
-		va, err = buf.setValueAtPath(va, p[1:], newValue)
-		if err != nil {
-			return v, err
-		}
-
-		err = buf.setFieldValue(p[0], va)
-		return NewDocumentValue(&buf), err
-	case ArrayValue:
-		var vb ValueBuffer
-		err := vb.ScanArray(v.V.(Array))
-		if err != nil {
-			return v, err
-		}
-
-		index, err := strconv.Atoi(p[0])
-		if err != nil {
-			return v, err
-		}
-
-		va, err := vb.GetByIndex(index)
-		if err != nil {
-			return v, err
-		}
-
-		if len(p) == 1 {
-			err = vb.Replace(index, newValue)
-			return NewArrayValue(&vb), err
-		}
-
-		va, err = fb.setValueAtPath(va, p[1:], newValue)
-		err = vb.Replace(index, va)
-		return NewArrayValue(&vb), err
-	}
-
-	return v, nil
-}
-
 // Set replaces a field if it already exists or creates one if not.
-func (fb *FieldBuffer) Set(path ValuePath, reqValue Value) error {
-	// check if the ValuePath contains only one field to set or replace
-	if len(path) == 1 {
-		// Set or replace the unique field
-		return fb.setFieldValue(path[0], reqValue)
-	}
-
-	for i, field := range fb.fields {
-		if path[0] == field.Field {
-			v, err := fb.setValueAtPath(field.Value, path[1:], reqValue)
-			if err != nil {
-				return err
-			}
+func (fb *FieldBuffer) Set(f string, v Value) {
+	for i := range fb.fields {
+		if fb.fields[i].Field == f {
 			fb.fields[i].Value = v
-			return nil
+			return
 		}
 	}
 
-	// return an error if the first element of the path is not found
-	return ErrFieldNotFound
+	fb.Add(f, v)
 }
 
 // Iterate goes through all the fields of the document and calls the given function by passing each one of them.
diff --git a/sql/parser/update.go b/sql/parser/update.go
index a89a5f93..eb8243c7 100644
--- a/sql/parser/update.go
+++ b/sql/parser/update.go
@@ -1,8 +1,6 @@
 package parser
 
 import (
-	"strings"
-
 	"github.com/genjidb/genji/sql/planner"
 	"github.com/genjidb/genji/sql/query/expr"
 	"github.com/genjidb/genji/sql/scanner"
@@ -58,24 +56,24 @@ func (p *Parser) parseSetClause() ([]updateSetPair, error) {
 			}
 		}
 
-		ref, err := p.parseFieldRef()
-		if err != nil {
-			return nil, err
+		// Scan the identifier for the field name.
+		tok, pos, lit := p.ScanIgnoreWhitespace()
+		if tok != scanner.IDENT {
+			return nil, newParseError(scanner.Tokstr(tok, lit), []string{"identifier"}, pos)
 		}
 
-		lit := strings.Join(ref, ".")
 		// Scan the eq sign
 		if tok, pos, lit := p.ScanIgnoreWhitespace(); tok != scanner.EQ {
 			return nil, newParseError(scanner.Tokstr(tok, lit), []string{"="}, pos)
 		}
 
 		// Scan the expr for the value.
-		parsedExpr, _, err := p.ParseExpr()
+		expr, _, err := p.ParseExpr()
 		if err != nil {
 			return nil, err
 		}
+		pairs = append(pairs, updateSetPair{lit, expr})
 
-		pairs = append(pairs, updateSetPair{lit, parsedExpr})
 		firstPair = false
 	}
 
diff --git a/sql/planner/tree.go b/sql/planner/tree.go
index 5e574372..57dd0ac8 100644
--- a/sql/planner/tree.go
+++ b/sql/planner/tree.go
@@ -370,10 +370,20 @@ func (n *setNode) toStream(st document.Stream) (document.Stream, error) {
 			return nil, err
 		}
 
-		path := document.NewValuePath(n.field)
-		err = fb.Set(path, ev)
+		_, err = fb.GetByField(n.field)
+
+		switch err {
+		case nil:
+			// If no error, it means that the field already exists
+			// and it should be replaced.
+			_ = fb.Replace(n.field, ev)
+		case document.ErrFieldNotFound:
+			// If the field doesn't exist,
+			// it should be added to the document.
+			fb.Set(n.field, ev)
+		}
 
-		return &fb, err
+		return &fb, nil
 	}), nil
 }
 


Test patch:
diff --git a/document/document_test.go b/document/document_test.go
index 341ae16e..2aa48b78 100644
--- a/document/document_test.go
+++ b/document/document_test.go
@@ -1,7 +1,6 @@
 package document_test
 
 import (
-	"bytes"
 	"encoding/json"
 	"errors"
 	"testing"
@@ -80,78 +79,20 @@ func TestFieldBuffer(t *testing.T) {
 	})
 
 	t.Run("Set", func(t *testing.T) {
-		d, err := document.NewFromJSON([]byte(`{"friends": [{"name": "Bar","address": {"city": "Paris","zipcode": "75001"}}]}`))
-		require.NoError(t, err)
 		var buf document.FieldBuffer
-		err = buf.Copy(d)
-		require.NoError(t, err)
+		buf.Add("a", document.NewIntegerValue(10))
+		buf.Add("b", document.NewTextValue("hello"))
 
-		var fb document.FieldBuffer
-		d, err = document.NewFromJSON([]byte(`{"phone":{"type": "cell", "number":"111-222-333"}}`))
-		require.NoError(t, err)
-		err = fb.Copy(d)
+		buf.Set("a", document.NewDoubleValue(11))
+		v, err := buf.GetByField("a")
 		require.NoError(t, err)
+		require.Equal(t, document.NewDoubleValue(11), v)
 
-		var vb document.ValueBuffer
-		vb = vb.Append(document.NewIntegerValue(1))
-		vb = vb.Append(document.NewIntegerValue(0))
-		vb = vb.Append(document.NewIntegerValue(0))
-
-		tests := []struct {
-			name string
-			p    document.ValuePath
-			v    document.Value
-			want string
-		}{
-			{"Set replace field textValue",
-				document.NewValuePath("friends.0.name"),
-				document.NewTextValue("Baz"),
-				`{"friends": [{"name": "Baz", "address": {"city": "Paris", "zipcode": "75001"}}]}`,
-			},
-			{"Set array",
-				document.NewValuePath("friends.0.a"),
-				document.NewArrayValue(&vb),
-				`{"friends": [{"name": "Baz", "address": {"city": "Paris", "zipcode": "75001"}, "a": [1, 0, 0]}]}`,
-			},
-			{"Nested array",
-				document.NewValuePath("friends.0.a.0"),
-				document.NewArrayValue(&vb),
-				`{"friends": [{"name": "Baz", "address": {"city": "Paris", "zipcode": "75001"}, "a": [[1, 0, 0], 0, 0]}]}`,
-			},
-			{"Set at index",
-				document.NewValuePath("friends.0.a.1"),
-				document.NewIntegerValue(99),
-				`{"friends": [{"name": "Baz", "address": {"city": "Paris", "zipcode": "75001"}, "a": [[1, 0, 0], 99, 0]}]}`,
-			},
-			{
-				"Set Document",
-				document.NewValuePath("contact"),
-				document.NewDocumentValue(&fb),
-				`{"friends": [{"name": "Baz", "address": {"city": "Paris", "zipcode": "75001"}, "a": [[1, 0, 0], 99, 0]}], "contact": {"phone": {"type": "cell", "number": "111-222-333"}}}`,
-			},
-			{
-				"Set Nested Document",
-				document.NewValuePath("contact.phone.type"),
-				document.NewTextValue("fix"),
-				`{"friends": [{"name": "Baz", "address": {"city": "Paris", "zipcode": "75001"}, "a": [[1, 0, 0], 99, 0]}], "contact": {"phone": {"type": "fix", "number": "111-222-333"}}}`,
-			},
-			{"Set add field into nested document",
-				document.NewValuePath("contact.email"),
-				document.NewTextValue("foo@example.com"),
-				`{"friends": [{"name": "Baz", "address": {"city": "Paris", "zipcode": "75001"}, "a": [[1, 0, 0], 99, 0]}], "contact": {"phone": {"type": "fix", "number": "111-222-333"}, "email": "foo@example.com"}}`,
-			},
-		}
-
-		for _, test := range tests {
-			t.Run(test.name, func(t *testing.T) {
-				var bufBytes bytes.Buffer
-				require.NoError(t, err, buf.Set(test.p, test.v))
-				err := document.ToJSON(&bufBytes, buf)
-				require.NoError(t, err)
-				require.Equal(t, test.want, bufBytes.String())
-			})
-		}
-
+		buf.Set("c", document.NewIntegerValue(12))
+		require.Equal(t, 3, buf.Len())
+		v, err = buf.GetByField("c")
+		require.NoError(t, err)
+		require.Equal(t, document.NewIntegerValue(12), v)
 	})
 
 	t.Run("Delete", func(t *testing.T) {
diff --git a/sql/parser/update_test.go b/sql/parser/update_test.go
index e93c6522..ef936852 100644
--- a/sql/parser/update_test.go
+++ b/sql/parser/update_test.go
@@ -25,29 +25,6 @@ func TestParserUpdate(t *testing.T) {
 					"test",
 				)),
 			false},
-		{"SET/Dot notation", "UPDATE test SET a.2.a = 1",
-			planner.NewTree(
-				planner.NewReplacementNode(
-					planner.NewSetNode(
-						planner.NewTableInputNode("test"),
-						"a.2.a", expr.IntegerValue(1),
-					),
-					"test",
-				)),
-			false},
-		{"SET/Dot notation with cond", "UPDATE test SET a.2.a = 1 WHERE age = 10",
-			planner.NewTree(
-				planner.NewReplacementNode(
-					planner.NewSetNode(
-						planner.NewSelectionNode(
-							planner.NewTableInputNode("test"),
-							expr.Eq(expr.FieldSelector([]string{"age"}), expr.IntegerValue(10)),
-						),
-						"a.2.a", expr.IntegerValue(1),
-					),
-					"test",
-				)),
-			false},
 		{"SET/With cond", "UPDATE test SET a = 1, b = 2 WHERE age = 10",
 			planner.NewTree(
 				planner.NewReplacementNode(
diff --git a/sql/query/update_test.go b/sql/query/update_test.go
index 0d217218..d7ff9aef 100644
--- a/sql/query/update_test.go
+++ b/sql/query/update_test.go
@@ -77,46 +77,22 @@ func TestUpdateStmt(t *testing.T) {
 	}
 
 	t.Run("with arrays", func(t *testing.T) {
-		tests := []struct {
-			name     string
-			query    string
-			fails    bool
-			expected string
-			params   []interface{}
-		}{
-			{"SET / No cond add field ", `UPDATE foo set b = 0`, false, `[{"a": [1, 0, 0], "b": 0}, {"a": [2, 0], "b": 0}]`, nil},
-			{"SET / No cond with dot notation at existing index only", `UPDATE foo SET a.2 = 10`, false, `[{"a": [1, 0, 10]}, {"a": [2, 0]}]`, nil},
-			{"SET / No cond with dot notation", `UPDATE foo SET a.1 = 10`, false, `[{"a": [1, 10, 0]}, {"a": [2, 10]}]`, nil},
-			{"SET / No cond with dot notation non existing field", `UPDATE foo SET a.foo.1 = 10`, true, "", nil},
-			{"SET / With cond with dot notation", `UPDATE foo SET a.0 = 1 WHERE a.0 = 2`, false, `[{"a": [1, 0, 0]}, {"a": [1, 0]}]`, nil},
-		}
-
-		for _, tt := range tests {
-			db, err := genji.Open(":memory:")
-			require.NoError(t, err)
-			defer db.Close()
-
-			err = db.Exec(`CREATE TABLE foo;`)
-			require.NoError(t, err)
-			err = db.Exec(`INSERT INTO foo (a) VALUES ([1, 0, 0]), ([2, 0]);`)
-			require.NoError(t, err)
-
-			err = db.Exec(tt.query, tt.params...)
-			if tt.fails {
-				require.Error(t, err)
-				continue
-			}
-
-			require.NoError(t, err)
-			st, err := db.Query("SELECT * FROM foo")
-			require.NoError(t, err)
-			defer st.Close()
-
-			var buf bytes.Buffer
-
-			err = document.IteratorToJSONArray(&buf, st)
-			require.NoError(t, err)
-			require.JSONEq(t, tt.expected, buf.String())
-		}
+		db, err := genji.Open(":memory:")
+		require.NoError(t, err)
+		defer db.Close()
+
+		res, err := db.Query(`
+			CREATE TABLE foo;
+			INSERT INTO foo (a) VALUES ([1, 0, 0]), ([2, 0]);
+			UPDATE foo set b = 0;
+			SELECT * FROM foo; 
+		`)
+		require.NoError(t, err)
+		defer res.Close()
+
+		var buf bytes.Buffer
+		err = document.IteratorToJSONArray(&buf, res)
+		require.NoError(t, err)
+		require.JSONEq(t, `[{"a": [1, 0, 0], "b": 0}, {"a": [2, 0], "b": 0}]`, buf.String())
 	})
 }