Fixes Client.Delete() behaves opposite to spec. Deprecates Client.Del…

…ete() in favor of Cliente.DeleteDocument() for RediSearch >=v2.0 (#80) * [fix] fixed Delete() test for redisearch >= v2.0. * [add] Added DeleteDocument() * [add] increased overall coverage Co-authored-by: filipecosta90 <[email protected]>
RediSearch · Aug 17, 2020 · b71ddb1 · b71ddb1
1 parent 9b11c55
commit b71ddb1
Show file tree

Hide file tree

Showing 5 changed files with 144 additions and 65 deletions.
diff --git a/Makefile b/Makefile
@@ -45,11 +45,11 @@ fmt:
 	$(GOFMT) ./...
 
 godoc_examples: get fmt
-	$(GOTEST) -race -covermode=atomic -v ./redisearch
+	$(GOTEST) -race -covermode=atomic ./redisearch
 
 test: get fmt
-	$(GOTEST) -race -covermode=atomic -run "Test" -v ./redisearch
+	$(GOTEST) -race -covermode=atomic -run "Test" ./redisearch
 
 coverage: get
-	$(GOTEST) -race -coverprofile=coverage.txt -covermode=atomic -v ./redisearch
+	$(GOTEST) -race -coverprofile=coverage.txt -covermode=atomic ./redisearch
 
diff --git a/README.md b/README.md
@@ -87,7 +87,7 @@ func ExampleClient() {
 | [FT.AGGREGATE](https://oss.redislabs.com/redisearch/Commands.html#ftaggregate) |   [Aggregate](https://godoc.org/github.com/RediSearch/redisearch-go/redisearch#Client.Aggregate)          |
 | [FT.CURSOR](https://oss.redislabs.com/redisearch/Aggregations.html#cursor_api) |   [Aggregate](https://godoc.org/github.com/RediSearch/redisearch-go/redisearch#Client.Aggregate) + (*WithCursor option set to True)         |
 | [FT.EXPLAIN](https://oss.redislabs.com/redisearch/Commands.html#ftexplain) |   [Explain](https://godoc.org/github.com/RediSearch/redisearch-go/redisearch#Client.Explain)        |
-| [FT.DEL](https://oss.redislabs.com/redisearch/Commands.html#ftdel) |   [Delete](https://godoc.org/github.com/RediSearch/redisearch-go/redisearch#Client.Delete)        |
+| [FT.DEL](https://oss.redislabs.com/redisearch/Commands.html#ftdel) |   [DeleteDocument](https://godoc.org/github.com/RediSearch/redisearch-go/redisearch#Client.DeleteDocument)        |
 | [FT.GET](https://oss.redislabs.com/redisearch/Commands.html#ftget) |    [Get](https://godoc.org/github.com/RediSearch/redisearch-go/redisearch#Client.Get) |
 | [FT.MGET](https://oss.redislabs.com/redisearch/Commands.html#ftmget) |    [MultiGet](https://godoc.org/github.com/RediSearch/redisearch-go/redisearch#Client.Multi) |
 | [FT.DROP](https://oss.redislabs.com/redisearch/Commands.html#ftdrop) |   [Drop](https://godoc.org/github.com/RediSearch/redisearch-go/redisearch#Client.Drop)        |

diff --git a/redisearch/client.go b/redisearch/client.go
@@ -364,16 +364,26 @@ func (i *Client) Drop() error {
 }
 
 // Delete the document from the index, optionally delete the actual document
+// WARNING: As of RediSearch 2.0 and above, FT.DEL always deletes the underlying document.
+// Deprecated: This function  is deprecated on RediSearch 2.0 and above, use DeleteDocument() instead
 func (i *Client) Delete(docId string, deleteDocument bool) (err error) {
+	return i.delDoc(docId, deleteDocument)
+}
+
+// Delete the document from the index and also delete the HASH key in which the document is stored
+func (i *Client) DeleteDocument(docId string) (err error) {
+	return i.delDoc(docId, true)
+}
+
+// Internal method to be used by Delete() and DeleteDocument()
+func (i *Client) delDoc(docId string, deleteDocument bool) (err error) {
 	conn := i.pool.Get()
 	defer conn.Close()
-
 	if deleteDocument {
-		_, err = conn.Do("FT.DEL", i.name, docId)
-	} else {
 		_, err = conn.Do("FT.DEL", i.name, docId, "DD")
+	} else {
+		_, err = conn.Do("FT.DEL", i.name, docId)
 	}
-
 	return
 }
 

diff --git a/redisearch/client_test.go b/redisearch/client_test.go
@@ -724,3 +724,129 @@ func TestClient_SynUpdate(t *testing.T) {
 		})
 	}
 }
+
+func TestClient_Delete(t *testing.T) {
+	c := createClient("ft.del-test")
+	sc := NewSchema(DefaultOptions).
+		AddField(NewTextField("name")).
+		AddField(NewTextField("addr"))
+	version, err := c.getRediSearchVersion()
+	assert.Nil(t, err)
+
+	type args struct {
+		docId          string
+		deleteDocument bool
+	}
+	tests := []struct {
+		name                string
+		args                args
+		wantErr             bool
+		documentShouldExist bool
+	}{
+		{"persist-doc", args{"doc1", false}, false, true},
+		{"delete-doc", args{"doc1", true}, false, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c.Drop()
+			err := c.CreateIndex(sc)
+			assert.Nil(t, err)
+			err = c.Index(NewDocument(tt.args.docId, 1.0).Set("name", "Jon Doe"))
+			assert.Nil(t, err)
+			if err := c.Delete(tt.args.docId, tt.args.deleteDocument); (err != nil) != tt.wantErr {
+				t.Errorf("Delete() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			docExists, err := redis.Bool(c.pool.Get().Do("EXISTS", tt.args.docId))
+			assert.Nil(t, err)
+			if version <= 10699 {
+				assert.Equal(t, tt.documentShouldExist, docExists)
+			} else {
+				assert.Equal(t, false, docExists)
+			}
+			teardown(c)
+		})
+	}
+}
+
+func TestClient_DeleteDocument(t *testing.T) {
+	c := createClient("ft.DeleteDocument-test")
+	sc := NewSchema(DefaultOptions).
+		AddField(NewTextField("name")).
+		AddField(NewTextField("addr"))
+
+	type args struct {
+		docId          string
+		docIdsToAddIdx []string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{"doc-exists", args{"doc1", []string{"doc1", "doc2"}}, false},
+		{"doc-not-exists", args{"doc3", []string{"doc1", "doc2"}}, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c.Drop()
+			err := c.CreateIndex(sc)
+			assert.Nil(t, err)
+			for _, docId := range tt.args.docIdsToAddIdx {
+				err = c.Index(NewDocument(docId, 1.0).Set("name", "Jon Doe"))
+				assert.Nil(t, err)
+			}
+			if err := c.DeleteDocument(tt.args.docId); (err != nil) != tt.wantErr {
+				t.Errorf("DeleteDocument() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			docExists, err := redis.Bool(c.pool.Get().Do("EXISTS", tt.args.docId))
+			assert.Nil(t, err)
+			assert.False(t, docExists)
+			teardown(c)
+		})
+	}
+}
+
+func TestClient_CreateIndexWithIndexDefinition1(t *testing.T) {
+	c := createClient("index-definition-test")
+	version, err := c.getRediSearchVersion()
+	assert.Nil(t, err)
+	if version <= 10699 {
+		// IndexDefinition is available for RediSearch 2.0+
+		return
+	}
+	// Create a schema
+	sc := NewSchema(DefaultOptions).
+		AddField(NewTextFieldOptions("name", TextFieldOptions{Sortable: true})).
+		AddField(NewTextFieldOptions("description", TextFieldOptions{Weight: 5.0, Sortable: true})).
+		AddField(NewNumericField("price"))
+
+	type args struct {
+		schema     *Schema
+		definition *IndexDefinition
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{"default", args{sc, NewIndexDefinition()}, false},
+		{"default+async", args{sc, NewIndexDefinition().SetAsync(true)}, false},
+		{"default+score", args{sc, NewIndexDefinition().SetScore(0.75)}, false},
+		{"default+score_field", args{sc, NewIndexDefinition().SetScoreField("myscore")}, false},
+		{"default+language", args{sc, NewIndexDefinition().SetLanguage("portuguese")}, false},
+		{"default+language_field", args{sc, NewIndexDefinition().SetLanguageField("mylanguage")}, false},
+		{"default+prefix", args{sc, NewIndexDefinition().AddPrefix("products:*")}, false},
+		{"default+payload_field", args{sc, NewIndexDefinition().SetPayloadField("products_description")}, false},
+		{"default+filter", args{sc, NewIndexDefinition().SetFilterExpression("@score >= 0")}, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c.Drop()
+			if err := c.CreateIndexWithIndexDefinition(tt.args.schema, tt.args.definition); (err != nil) != tt.wantErr {
+				t.Errorf("CreateIndexWithIndexDefinition() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+		teardown(c)
+
+	}
+}
diff --git a/redisearch/redisearch_test.go b/redisearch/redisearch_test.go
@@ -358,63 +358,6 @@ func TestTags(t *testing.T) {
 	teardown(c)
 }
 
-func TestDelete(t *testing.T) {
-	c := createClient("TestDelete-testung")
-
-	sc := NewSchema(DefaultOptions).
-		AddField(NewTextField("foo"))
-
-	err := c.Drop()
-
-	assert.Nil(t, c.CreateIndex(sc))
-
-	var info *IndexInfo
-
-	// validate that the index is empty
-	info, err = c.Info()
-	assert.Nil(t, err)
-	if !info.IsIndexing {
-		assert.Equal(t, uint64(0), info.DocCount)
-	}
-
-	doc := NewDocument("TestDelete-doc1", 1.0)
-	doc.Set("foo", "Hello world")
-
-	err = c.IndexOptions(DefaultIndexingOptions, doc)
-	assert.Nil(t, err)
-
-	// Wait for all documents to be indexed
-	info, err = c.Info()
-	assert.Nil(t, err)
-	for info.IsIndexing {
-		time.Sleep(time.Second)
-		info, err = c.Info()
-		assert.Nil(t, err)
-	}
-
-	// now we should have 1 document (id = doc1)
-	info, err = c.Info()
-	assert.Nil(t, err)
-	assert.Equal(t, uint64(1), info.DocCount)
-
-	// delete the document from the index
-	err = c.Delete("TestDelete-doc1", true)
-	assert.Nil(t, err)
-
-	// Wait for all documents to be indexed
-	info, err = c.Info()
-	assert.Nil(t, err)
-	for info.IsIndexing {
-		time.Sleep(time.Second)
-		info, err = c.Info()
-		assert.Nil(t, err)
-	}
-
-	assert.Nil(t, err)
-	assert.Equal(t, uint64(0), info.DocCount)
-	teardown(c)
-}
-
 func TestSpellCheck(t *testing.T) {
 	c := createClient("testung")
 	countries := []string{"Spain", "Israel", "Portugal", "France", "England", "Angola"}