From 508f0aeeacaa5d4456a4d5588a57c2689e9a6d8b Mon Sep 17 00:00:00 2001 From: Andrew Williams Date: Sat, 8 Aug 2020 12:18:45 +1200 Subject: [PATCH 1/4] Client.Delete() behaves opposite to spec --- redisearch/client.go | 4 ++-- redisearch/redisearch_test.go | 29 ++++++++++++++++++++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/redisearch/client.go b/redisearch/client.go index 8cce654..5024c5e 100644 --- a/redisearch/client.go +++ b/redisearch/client.go @@ -356,9 +356,9 @@ func (i *Client) Delete(docId string, deleteDocument bool) (err error) { 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/redisearch_test.go b/redisearch/redisearch_test.go index a3e0787..880f8b5 100644 --- a/redisearch/redisearch_test.go +++ b/redisearch/redisearch_test.go @@ -365,7 +365,8 @@ func TestDelete(t *testing.T) { assert.Nil(t, err) assert.Equal(t, uint64(0), info.DocCount) - doc := NewDocument("TestDelete-doc1", 1.0) + docName := "TestDelete-doc1" + doc := NewDocument(docName, 1.0) doc.Set("foo", "Hello world") err = c.IndexOptions(DefaultIndexingOptions, doc) @@ -376,14 +377,36 @@ func TestDelete(t *testing.T) { assert.Nil(t, err) assert.Equal(t, uint64(1), info.DocCount) - // delete the document from the index - err = c.Delete("TestDelete-doc1", true) + // delete the document reference from the index + // and the document itself + err = c.Delete(docName, true) assert.Nil(t, err) // validate that the index is empty again info, err = c.Info() assert.Nil(t, err) assert.Equal(t, uint64(0), info.DocCount) + // and that the document was deleted + conn := c.pool.Get() + defer conn.Close() + docExists, err := redis.Bool(conn.Do("EXISTS", docName)) + assert.Nil(t, err) + assert.False(t, docExists) + + // Re-add the document to the index + // This time, only remove the reference + err = c.IndexOptions(DefaultIndexingOptions, doc) + assert.Nil(t, err) + err = c.Delete(docName, false) + assert.Nil(t, err) + // validate that the index is empty again + info, err = c.Info() + assert.Nil(t, err) + assert.Equal(t, uint64(0), info.DocCount) + // and that the document remains + docExists, err = redis.Bool(conn.Do("EXISTS", docName)) + assert.Nil(t, err) + assert.True(t, docExists) } func TestSpellCheck(t *testing.T) { From 86fe1274c125dddc0ebeb431740af66ca3057175 Mon Sep 17 00:00:00 2001 From: filipecosta90 Date: Mon, 17 Aug 2020 20:23:43 +0100 Subject: [PATCH 2/4] [fix] fixed Delete() test for redisearch >=0. --- Makefile | 6 +-- redisearch/client.go | 3 +- redisearch/client_test.go | 42 ++++++++++++++++++ redisearch/redisearch_test.go | 80 ----------------------------------- 4 files changed, 46 insertions(+), 85 deletions(-) diff --git a/Makefile b/Makefile index 8a732df..ac95ded 100644 --- 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/redisearch/client.go b/redisearch/client.go index 852cb96..81f6e52 100644 --- a/redisearch/client.go +++ b/redisearch/client.go @@ -364,16 +364,15 @@ 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. func (i *Client) Delete(docId string, deleteDocument bool) (err error) { conn := i.pool.Get() defer conn.Close() - if deleteDocument { _, 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 index d928279..0a97084 100644 --- a/redisearch/client_test.go +++ b/redisearch/client_test.go @@ -724,3 +724,45 @@ 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) + } + }) + } +} diff --git a/redisearch/redisearch_test.go b/redisearch/redisearch_test.go index e694196..a20ba55 100644 --- a/redisearch/redisearch_test.go +++ b/redisearch/redisearch_test.go @@ -358,86 +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) - } - - docName := "TestDelete-doc1" - doc := NewDocument(docName, 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 reference from the index - // and the document itself - err = c.Delete(docName, 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) - // and that the document was deleted - conn := c.pool.Get() - defer conn.Close() - docExists, err := redis.Bool(conn.Do("EXISTS", docName)) - assert.Nil(t, err) - assert.False(t, docExists) - - // Re-add the document to the index - // This time, only remove the reference - err = c.IndexOptions(DefaultIndexingOptions, doc) - assert.Nil(t, err) - err = c.Delete(docName, false) - assert.Nil(t, err) - // validate that the index is empty again - info, err = c.Info() - assert.Nil(t, err) - assert.Equal(t, uint64(0), info.DocCount) - // and that the document remains - docExists, err = redis.Bool(conn.Do("EXISTS", docName)) - assert.Nil(t, err) - assert.True(t, docExists) - teardown(c) -} - func TestSpellCheck(t *testing.T) { c := createClient("testung") countries := []string{"Spain", "Israel", "Portugal", "France", "England", "Angola"} From f9109759768f3634fd3ec00015f2ceb9e2a5efda Mon Sep 17 00:00:00 2001 From: filipecosta90 Date: Mon, 17 Aug 2020 21:38:03 +0100 Subject: [PATCH 3/4] [add] Added DeleteDocument() --- README.md | 2 +- redisearch/client.go | 11 +++++++++++ redisearch/client_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 565e890..65abe6e 100644 --- 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 index 81f6e52..5125efb 100644 --- a/redisearch/client.go +++ b/redisearch/client.go @@ -365,7 +365,18 @@ 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 { diff --git a/redisearch/client_test.go b/redisearch/client_test.go index 0a97084..13a7d15 100644 --- a/redisearch/client_test.go +++ b/redisearch/client_test.go @@ -763,6 +763,45 @@ func TestClient_Delete(t *testing.T) { } 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) }) } } From 50056c32b5ad43077947b4c9e8c71be17f2e7888 Mon Sep 17 00:00:00 2001 From: filipecosta90 Date: Mon, 17 Aug 2020 22:03:23 +0100 Subject: [PATCH 4/4] [add] increased overall coverage --- redisearch/client_test.go | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/redisearch/client_test.go b/redisearch/client_test.go index 13a7d15..88bc119 100644 --- a/redisearch/client_test.go +++ b/redisearch/client_test.go @@ -805,3 +805,48 @@ func TestClient_DeleteDocument(t *testing.T) { }) } } + +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) + + } +}