Fix RHT.Remove and add test code (#752)

Changes:
- Fixed nil pointer dereference error in RHT.Remove.
- Fixed RHT.Len to exclude the removed elements.
- Fixed RHT.Nodes to exclude the removed elements.
- Fixed an error in RHT.Set when modifying an already deleted key.

---------

Co-authored-by: Youngteac Hong <susukang98@gmail.com>

pkg/document/crdt/rht.go

@@ -29,28 +29,18 @@ type RHTNode struct {
 	key       string
 	val       string
 	updatedAt *time.Ticket
-	removedAt *time.Ticket
+	isRemoved bool
 }
 
-func newRHTNode(key, val string, updatedAt *time.Ticket) *RHTNode {
+func newRHTNode(key, val string, updatedAt *time.Ticket, isRemoved bool) *RHTNode {
 	return &RHTNode{
 		key:       key,
 		val:       val,
 		updatedAt: updatedAt,
+		isRemoved: isRemoved,
 	}
 }
 
-// Remove removes this node. It only marks the deleted time (tombstone).
-func (n *RHTNode) Remove(removedAt *time.Ticket) {
-	if n.removedAt == nil || removedAt.After(n.removedAt) {
-		n.removedAt = removedAt
-	}
-}
-
-func (n *RHTNode) isRemoved() bool {
-	return n.removedAt != nil
-}
-
 // Key returns the key of this node.
 func (n *RHTNode) Key() string {
 	return n.key
@@ -66,28 +56,26 @@ func (n *RHTNode) UpdatedAt() *time.Ticket {
 	return n.updatedAt
 }
 
-// RemovedAt returns the deletion time of this node.
-func (n *RHTNode) RemovedAt() *time.Ticket {
-	return n.removedAt
-}
-
 // RHT is a hashtable with logical clock(Replicated hashtable).
 // For more details about RHT: http://csl.skku.edu/papers/jpdc11.pdf
+// NOTE(justiceHui): RHT and ElementRHT has duplicated functions.
 type RHT struct {
-	nodeMapByKey map[string]*RHTNode
+	nodeMapByKey           map[string]*RHTNode
+	numberOfRemovedElement int
 }
 
 // NewRHT creates a new instance of RHT.
 func NewRHT() *RHT {
 	return &RHT{
-		nodeMapByKey: make(map[string]*RHTNode),
+		nodeMapByKey:           make(map[string]*RHTNode),
+		numberOfRemovedElement: 0,
 	}
 }
 
 // Get returns the value of the given key.
 func (rht *RHT) Get(key string) string {
 	if node, ok := rht.nodeMapByKey[key]; ok {
-		if node.isRemoved() {
+		if node.isRemoved {
 			return ""
 		}
 		return node.val
@@ -99,7 +87,7 @@ func (rht *RHT) Get(key string) string {
 // Has returns whether the element exists of the given key or not.
 func (rht *RHT) Has(key string) bool {
 	if node, ok := rht.nodeMapByKey[key]; ok {
-		return node != nil && !node.isRemoved()
+		return node != nil && !node.isRemoved
 	}
 
 	return false
@@ -108,15 +96,27 @@ func (rht *RHT) Has(key string) bool {
 // Set sets the value of the given key.
 func (rht *RHT) Set(k, v string, executedAt *time.Ticket) {
 	if node, ok := rht.nodeMapByKey[k]; !ok || executedAt.After(node.updatedAt) {
-		newNode := newRHTNode(k, v, executedAt)
+		if node != nil && node.isRemoved {
+			rht.numberOfRemovedElement--
+		}
+		newNode := newRHTNode(k, v, executedAt, false)
 		rht.nodeMapByKey[k] = newNode
 	}
 }
 
 // Remove removes the Element of the given key.
 func (rht *RHT) Remove(k string, executedAt *time.Ticket) string {
-	if node, ok := rht.nodeMapByKey[k]; ok && executedAt.After(node.removedAt) {
-		node.Remove(executedAt)
+	if node, ok := rht.nodeMapByKey[k]; ok && executedAt.After(node.updatedAt) {
+		alreadyRemoved := node.isRemoved
+		if !alreadyRemoved {
+			rht.numberOfRemovedElement++
+		}
+		newNode := newRHTNode(k, node.val, executedAt, true)
+		rht.nodeMapByKey[k] = newNode
+
+		if alreadyRemoved {
+			return ""
+		}
 		return node.val
 	}
 
@@ -128,7 +128,7 @@ func (rht *RHT) Remove(k string, executedAt *time.Ticket) string {
 func (rht *RHT) Elements() map[string]string {
 	members := make(map[string]string)
 	for _, node := range rht.nodeMapByKey {
-		if !node.isRemoved() {
+		if !node.isRemoved {
 			members[node.key] = node.val
 		}
 	}
@@ -141,15 +141,17 @@ func (rht *RHT) Elements() map[string]string {
 func (rht *RHT) Nodes() []*RHTNode {
 	var nodes []*RHTNode
 	for _, node := range rht.nodeMapByKey {
-		nodes = append(nodes, node)
+		if !node.isRemoved {
+			nodes = append(nodes, node)
+		}
 	}
 
 	return nodes
 }
 
 // Len returns the number of elements.
 func (rht *RHT) Len() int {
-	return len(rht.nodeMapByKey)
+	return len(rht.nodeMapByKey) - rht.numberOfRemovedElement
 }
 
 // DeepCopy copies itself deeply.

pkg/document/crdt/rht_test.go

@@ -1,23 +1,246 @@
-package crdt
+package crdt_test
 
 import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+
+	"github.com/yorkie-team/yorkie/pkg/document/crdt"
+	"github.com/yorkie-team/yorkie/test/helper"
 )
 
-func TestMarshal(t *testing.T) {
-	t.Run("marshal test", func(t *testing.T) {
-		key1 := `hello\\\t`
-		value1 := "world\"\f\b"
-		key2 := "hi"
-		value2 := `test\r`
-		expected := `{"hello\\\\\\t":"world\"\f\b","hi":"test\\r"}`
-
-		rht := NewRHT()
-		rht.Set(key1, value1, nil)
-		rht.Set(key2, value2, nil)
-		actual := rht.Marshal()
-		assert.Equal(t, expected, actual)
-	})
+func TestRHT_Marshal(t *testing.T) {
+	tests := []struct {
+		desc      string
+		insertKey string
+		insertVal string
+		expectStr string
+	}{
+		{
+			desc:      `1. empty hash table`,
+			insertKey: ``,
+			insertVal: ``,
+			expectStr: `{}`,
+		},
+		{
+			desc:      `2. only one element`,
+			insertKey: "hello\\\\\\t",
+			insertVal: "world\"\f\b",
+			expectStr: `{"hello\\\\\\t":"world\"\f\b"}`,
+		},
+		{
+			desc:      `3. non-empty hash table`,
+			insertKey: "hi",
+			insertVal: `test\r`,
+			expectStr: `{"hello\\\\\\t":"world\"\f\b","hi":"test\\r"}`,
+		},
+	}
+
+	root := helper.TestRoot()
+	ctx := helper.TextChangeContext(root)
+
+	rht := crdt.NewRHT()
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			if len(tt.insertKey) > 0 {
+				rht.Set(tt.insertKey, tt.insertVal, ctx.IssueTimeTicket())
+			}
+			assert.Equal(t, tt.expectStr, rht.Marshal())
+		})
+	}
+}
+
+func TestRHT_ToXML(t *testing.T) {
+	tests := []struct {
+		desc      string
+		insertKey string
+		insertVal string
+		expectStr string
+	}{
+		{
+			desc:      `1. empty hash table`,
+			insertKey: ``,
+			insertVal: ``,
+			expectStr: ``,
+		},
+		{
+			desc:      `2. only one element`,
+			insertKey: "hello\\\\\\t",
+			insertVal: "world\"\f\b",
+			expectStr: `hello\\\t="world\"\f\b"`,
+		},
+		{
+			desc:      `3. non-empty hash table`,
+			insertKey: "hi",
+			insertVal: `test\r`,
+			expectStr: `hello\\\t="world\"\f\b" hi="test\\r"`,
+		},
+	}
+
+	root := helper.TestRoot()
+	ctx := helper.TextChangeContext(root)
+
+	rht := crdt.NewRHT()
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			if len(tt.insertKey) > 0 {
+				rht.Set(tt.insertKey, tt.insertVal, ctx.IssueTimeTicket())
+			}
+			assert.Equal(t, tt.expectStr, rht.ToXML())
+		})
+	}
+}
+
+func TestRHT_Set(t *testing.T) {
+	key1, val1 := `key1`, `value1`
+	key2, val2 := `key2`, `value2`
+
+	tests := []struct {
+		desc       string
+		insertKey  []string
+		insertVal  []string
+		expectStr  string
+		expectSize int
+	}{
+		{
+			desc:       `1. set elements`,
+			insertKey:  []string{key1, key2},
+			insertVal:  []string{val1, val2},
+			expectStr:  `{"key1":"value1","key2":"value2"}`,
+			expectSize: 2,
+		},
+		{
+			desc:       `2. change elements`,
+			insertKey:  []string{key1, key2},
+			insertVal:  []string{val2, val1},
+			expectStr:  `{"key1":"value2","key2":"value1"}`,
+			expectSize: 2,
+		},
+	}
+
+	root := helper.TestRoot()
+	ctx := helper.TextChangeContext(root)
+
+	rht := crdt.NewRHT()
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			for i, key := range tt.insertKey {
+				rht.Set(key, tt.insertVal[i], ctx.IssueTimeTicket())
+			}
+			assert.Equal(t, tt.expectStr, rht.Marshal())
+			assert.Equal(t, tt.expectSize, rht.Len())
+		})
+	}
+}
+
+func TestRHT_Remove(t *testing.T) {
+	key1, val1, val11 := `key1`, `value1`, `value11`
+	key2, val2, val22 := `key2`, `value2`, `value22`
+
+	tests := []struct {
+		desc       string
+		insertKey  []string
+		insertVal  []string
+		deleteKey  []string
+		deleteVal  []string
+		expectXML  string
+		expectJSON string
+		expectSize int
+	}{
+		{
+			desc:       `1. set elements`,
+			insertKey:  []string{key1, key2},
+			insertVal:  []string{val1, val2},
+			deleteKey:  []string{},
+			deleteVal:  []string{},
+			expectXML:  `key1="value1" key2="value2"`,
+			expectJSON: `{"key1":"value1","key2":"value2"}`,
+			expectSize: 2,
+		},
+		{
+			desc:       `2. remove element`,
+			insertKey:  []string{},
+			insertVal:  []string{},
+			deleteKey:  []string{key1},
+			deleteVal:  []string{val1},
+			expectXML:  `key2="value2"`,
+			expectJSON: `{"key2":"value2"}`,
+			expectSize: 1,
+		},
+		{
+			desc:       `3. set after remove`,
+			insertKey:  []string{key1},
+			insertVal:  []string{val11},
+			deleteKey:  []string{},
+			deleteVal:  []string{},
+			expectXML:  `key1="value11" key2="value2"`,
+			expectJSON: `{"key1":"value11","key2":"value2"}`,
+			expectSize: 2,
+		},
+		{
+			desc:       `4. remove element`,
+			insertKey:  []string{key2},
+			insertVal:  []string{val22},
+			deleteKey:  []string{key1},
+			deleteVal:  []string{val11},
+			expectXML:  `key2="value22"`,
+			expectJSON: `{"key2":"value22"}`,
+			expectSize: 1,
+		},
+		{
+			desc:       `5. remove element again`,
+			insertKey:  []string{},
+			insertVal:  []string{},
+			deleteKey:  []string{key1},
+			deleteVal:  []string{``},
+			expectXML:  `key2="value22"`,
+			expectJSON: `{"key2":"value22"}`,
+			expectSize: 1,
+		},
+		{
+			desc:       `6. remove element(cleared)`,
+			insertKey:  []string{},
+			insertVal:  []string{},
+			deleteKey:  []string{key2},
+			deleteVal:  []string{val22},
+			expectXML:  ``,
+			expectJSON: `{}`,
+			expectSize: 0,
+		},
+		{
+			desc:       `7. remove not exist key`,
+			insertKey:  []string{},
+			insertVal:  []string{},
+			deleteKey:  []string{`not-exist-key`},
+			deleteVal:  []string{``},
+			expectXML:  ``,
+			expectJSON: `{}`,
+			expectSize: 0,
+		},
+	}
+
+	root := helper.TestRoot()
+	ctx := helper.TextChangeContext(root)
+
+	rht := crdt.NewRHT()
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			for i, key := range tt.insertKey {
+				rht.Set(key, tt.insertVal[i], ctx.IssueTimeTicket())
+			}
+			for i, key := range tt.deleteKey {
+				removedElement := rht.Remove(key, ctx.IssueTimeTicket())
+				assert.Equal(t, tt.deleteVal[i], removedElement)
+			}
+			assert.Equal(t, tt.expectXML, rht.ToXML())
+			assert.Equal(t, tt.expectJSON, rht.Marshal())
+			assert.Equal(t, tt.expectSize, rht.Len())
+			assert.Equal(t, tt.expectSize, len(rht.Nodes()))
+			assert.Equal(t, tt.expectSize, len(rht.Elements()))
+		})
+	}
 }