Merge pull request #3361 from fschade/fix-search-opensearch-queries

fix(search): fix open-search kql field compilation

services/search/pkg/opensearch/backend.go

@@ -9,6 +9,7 @@ import (
 	storageProvider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	opensearchgoAPI "github.com/opensearch-project/opensearch-go/v4/opensearchapi"
 
+	"github.com/opencloud-eu/reva/v2/pkg/errtypes"
 	"github.com/opencloud-eu/reva/v2/pkg/storagespace"
 	"github.com/opencloud-eu/reva/v2/pkg/utils"
 
@@ -17,6 +18,7 @@ import (
 	searchService "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/search/v0"
 	"github.com/opencloud-eu/opencloud/services/search/pkg/opensearch/internal/convert"
 	"github.com/opencloud-eu/opencloud/services/search/pkg/opensearch/internal/osu"
+	searchQuery "github.com/opencloud-eu/opencloud/services/search/pkg/query"
 	"github.com/opencloud-eu/opencloud/services/search/pkg/search"
 )
 
@@ -68,7 +70,10 @@ func NewBackend(index string, client *opensearchgoAPI.Client) (*Backend, error)
 
 func (b *Backend) Search(ctx context.Context, sir *searchService.SearchIndexRequest) (*searchService.SearchIndexResponse, error) {
 	boolQuery, err := convert.KQLToOpenSearchBoolQuery(sir.Query)
-	if err != nil {
+	switch {
+	case searchQuery.IsValidationError(err):
+		return nil, errtypes.BadRequest(err.Error())
+	case err != nil:
 		return nil, fmt.Errorf("failed to convert KQL query to OpenSearch bool query: %w", err)
 	}
 

services/search/pkg/opensearch/backend_test.go

@@ -11,6 +11,8 @@ import (
 	opensearchgo "github.com/opensearch-project/opensearch-go/v4"
 	opensearchgoAPI "github.com/opensearch-project/opensearch-go/v4/opensearchapi"
 
+	"github.com/opencloud-eu/reva/v2/pkg/errtypes"
+
 	searchService "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/search/v0"
 	"github.com/opencloud-eu/opencloud/services/search/pkg/opensearch"
 	opensearchtest "github.com/opencloud-eu/opencloud/services/search/pkg/opensearch/internal/test"
@@ -490,4 +492,122 @@ var _ = Describe("Backend", func() {
 			Expect(resourceByID(tc, indexName, other.ID).Path).To(Equal(other.Path), "resource in a different root must not be moved")
 		})
 	})
+
+	Describe("SearchInAnalyzedFields", func() {
+		const indexName = "opencloud-test-engine-search-analyzed-fields"
+
+		var (
+			tc      *opensearchtest.TestClient
+			backend *opensearch.Backend
+		)
+
+		BeforeEach(func() {
+			dashed := opensearchtest.Testdata.Resources.Folder
+			dashed.ID = "1$1!10"
+			dashed.Name = "new-folder"
+			dashed.Path = "./new-folder"
+			dashed.Title = "quarterly report"
+
+			plain := opensearchtest.Testdata.Resources.Folder
+			plain.ID = "1$1!11"
+			plain.Name = "documents"
+			plain.Path = "./documents"
+			plain.Title = "notes"
+
+			spaced := opensearchtest.Testdata.Resources.Folder
+			spaced.ID = "1$1!12"
+			spaced.Name = "foo bar"
+			spaced.Path = "./foo bar"
+			spaced.Title = "spaced out"
+
+			backend, tc = newBackend(indexName, dashed, plain, spaced)
+			deleteIndexOnCleanup(tc, indexName)
+			tc.Require.IndicesRefresh([]string{indexName}, nil)
+		})
+
+		DescribeTable("finds what the analyzer made of the value",
+			func(query string, want []string) {
+				resp, err := backend.Search(context.Background(), &searchService.SearchIndexRequest{Query: query})
+				Expect(err).ToNot(HaveOccurred())
+
+				names := make([]string, 0, len(resp.Matches))
+				for _, match := range resp.Matches {
+					names = append(names, match.Entity.Name)
+				}
+				Expect(names).To(ConsistOf(want))
+			},
+			Entry("the full name with the dash", "new-folder", []string{"new-folder"}),
+			Entry("one token of it", "new", []string{"new-folder"}),
+			Entry("a name without a dash", "documents", []string{"documents"}),
+			Entry("a wildcard", "*folder*", []string{"new-folder"}),
+			// the shape the web client sends for every name search
+			Entry("a wildcard around the whole dashed name", `name:"*new-folder*"`, []string{"new-folder"}),
+			Entry("a wildcard spanning the dash", `name:"*w-fol*"`, []string{"new-folder"}),
+			Entry("a wildcard in a different case", `name:"*NEW-FOLDER*"`, []string{"new-folder"}),
+			Entry("a wildcard spanning a space", `name:"*oo ba*"`, []string{"foo bar"}),
+			Entry("a wildcard around a name with a space", `name:"*foo bar*"`, []string{"foo bar"}),
+			Entry("a name with a space", `name:"foo bar"`, []string{"foo bar"}),
+			Entry("a title of two words", `Title:"quarterly report"`, []string{"new-folder"}),
+			Entry("one token of a title", "Title:quarterly", []string{"new-folder"}),
+		)
+	})
+
+	Describe("SearchByTag", func() {
+		const indexName = "opencloud-test-engine-search-by-tag"
+
+		var (
+			tc      *opensearchtest.TestClient
+			backend *opensearch.Backend
+		)
+
+		BeforeEach(func() {
+			tagged := opensearchtest.Testdata.Resources.Folder
+			tagged.ID = "1$1!20"
+			tagged.Name = "tagged"
+			tagged.Path = "./tagged"
+			tagged.Tags = []string{"foo-bar"}
+
+			other := opensearchtest.Testdata.Resources.Folder
+			other.ID = "1$1!21"
+			other.Name = "other"
+			other.Path = "./other"
+			other.Tags = []string{"foo"}
+
+			backend, tc = newBackend(indexName, tagged, other)
+			deleteIndexOnCleanup(tc, indexName)
+			tc.Require.IndicesRefresh([]string{indexName}, nil)
+		})
+
+		// a tag is one label, not prose, so it matches as a whole or not at all
+		DescribeTable("matches a tag as a whole",
+			func(query string, want []string) {
+				resp, err := backend.Search(context.Background(), &searchService.SearchIndexRequest{Query: query})
+				Expect(err).ToNot(HaveOccurred())
+
+				names := make([]string, 0, len(resp.Matches))
+				for _, match := range resp.Matches {
+					names = append(names, match.Entity.Name)
+				}
+				Expect(names).To(ConsistOf(want))
+			},
+			Entry("the whole tag", `tag:("foo-bar")`, []string{"tagged"}),
+			Entry("a token of a tag does not match it", `tag:("foo")`, []string{"other"}),
+			Entry("a tag in a different case", `tag:("FOO-BAR")`, []string{"tagged"}),
+			Entry("a wildcard reaches both", `tag:("*foo*")`, []string{"tagged", "other"}),
+		)
+	})
+
+	Describe("SearchWithAnInvalidQuery", func() {
+		const indexName = "opencloud-test-engine-search-invalid-query"
+
+		It("answers with a bad request", func() {
+			backend, tc := newBackend(indexName)
+			deleteIndexOnCleanup(tc, indexName)
+
+			_, err := backend.Search(context.Background(), &searchService.SearchIndexRequest{Query: "AND mediatype:document"})
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(BeAssignableToTypeOf(errtypes.BadRequest("")))
+			Expect(err.Error()).To(Equal(`error: bad request: the expression can't begin from a binary operator: 'AND'`))
+		})
+	})
 })

services/search/pkg/opensearch/internal/convert/kql_query.go

@@ -14,7 +14,7 @@ var (
 func KQLToOpenSearchBoolQuery(kqlQuery string) (*osu.BoolQuery, error) {
 	kqlAst, err := kql.Builder{}.Build(kqlQuery)
 	if err != nil {
-		return nil, fmt.Errorf("failed to build query: %w", err)
+		return nil, err
 	}
 
 	kqlNodes, err := ExpandKQL(kqlAst.Nodes)

services/search/pkg/opensearch/internal/convert/kql_transpile.go

@@ -3,6 +3,7 @@ package convert
 import (
 	"errors"
 	"fmt"
+	"slices"
 	"strings"
 	"time"
 
@@ -93,47 +94,13 @@ func (t kqlOpensearchTranspiler) getOperatorValueAt(nodes []ast.Node, i int) str
 }
 
 func (t kqlOpensearchTranspiler) toBuilder(node ast.Node) (osu.Builder, error) {
-	var builder osu.Builder
-
 	switch node := node.(type) {
 	case *ast.BooleanNode:
 		return osu.NewTermQuery[bool](node.Key).Value(node.Value), nil
 	case *ast.StringNode:
-		isWildcard := strings.Contains(node.Value, "*")
-		if isWildcard {
-			return osu.NewWildcardQuery(node.Key).Value(node.Value), nil
-		}
-
-		totalTerms := strings.Split(node.Value, " ")
-		isSingleTerm := len(totalTerms) == 1
-		isMultiTerm := len(totalTerms) >= 1
-		switch {
-		case isSingleTerm:
-			return osu.NewTermQuery[string](node.Key).Value(node.Value), nil
-		case isMultiTerm:
-			return osu.NewMatchPhraseQuery(node.Key).Query(node.Value), nil
-		}
-
-		return nil, fmt.Errorf("unsupported string node value: %s", node.Value)
+		return stringNodeQuery(node), nil
 	case *ast.DateTimeNode:
-		if node.Operator == nil {
-			return builder, fmt.Errorf("date time node without operator: %w", ErrUnsupportedNodeType)
-		}
-
-		query := osu.NewRangeQuery[time.Time](node.Key)
-
-		switch node.Operator.Value {
-		case ">":
-			return query.Gt(node.Value), nil
-		case ">=":
-			return query.Gte(node.Value), nil
-		case "<":
-			return query.Lt(node.Value), nil
-		case "<=":
-			return query.Lte(node.Value), nil
-		}
-
-		return nil, fmt.Errorf("unsupported operator %s for date time node: %w", node.Operator.Value, ErrUnsupportedNodeType)
+		return dateTimeNodeQuery(node)
 	case *ast.GroupNode:
 		group, err := t.transpile(node.Nodes)
 		if err != nil {
@@ -145,3 +112,56 @@ func (t kqlOpensearchTranspiler) toBuilder(node ast.Node) (osu.Builder, error) {
 
 	return nil, fmt.Errorf("%w: %T", ErrUnsupportedNodeType, node)
 }
+
+// stringNodeQuery picks the query a string node turns into.
+func stringNodeQuery(node *ast.StringNode) osu.Builder {
+	isWildcard := strings.Contains(node.Value, "*")
+
+	switch {
+	// Name: "*oo-bar", "*oo ba*", "*OO*"
+	// Title: "*rterly rep*"
+	// Tags: "*spaced tag*"
+	case isWildcard && slices.Contains([]string{"Name", "Title", "Tags"}, node.Key):
+		return osu.NewWildcardQuery(node.Key + ".keyword").
+			Value(node.Value).
+			Params(&osu.WildcardQueryParams{CaseInsensitive: true})
+	// Path: "./foo*", MimeType: "*plain"
+	case isWildcard:
+		return osu.NewWildcardQuery(node.Key).Value(node.Value)
+	// Tags: "foo-bar", "spaced tag", "FOO-BAR"
+	case node.Key == "Tags":
+		return osu.NewTermQuery[string](node.Key + ".keyword").
+			Value(node.Value).
+			Params(&osu.TermQueryParams{CaseInsensitive: true})
+	// Name: "foo-bar", "foo bar"
+	// Title: "quarterly report"
+	// Content: "foo bar"
+	case slices.Contains([]string{"Name", "Title", "Content"}, node.Key):
+		return osu.NewMatchPhraseQuery(node.Key).Query(node.Value)
+	// Path: "./foo bar", MimeType: "text/plain"
+	default:
+		return osu.NewTermQuery[string](node.Key).Value(node.Value)
+	}
+}
+
+// dateTimeNodeQuery turns a date time node into a range query.
+func dateTimeNodeQuery(node *ast.DateTimeNode) (osu.Builder, error) {
+	if node.Operator == nil {
+		return nil, fmt.Errorf("date time node without operator: %w", ErrUnsupportedNodeType)
+	}
+
+	query := osu.NewRangeQuery[time.Time](node.Key)
+
+	switch node.Operator.Value {
+	case ">":
+		return query.Gt(node.Value), nil
+	case ">=":
+		return query.Gte(node.Value), nil
+	case "<":
+		return query.Lt(node.Value), nil
+	case "<=":
+		return query.Lte(node.Value), nil
+	}
+
+	return nil, fmt.Errorf("unsupported operator %s for date time node: %w", node.Operator.Value, ErrUnsupportedNodeType)
+}

services/search/pkg/opensearch/internal/convert/kql_transpile_test.go

@@ -16,13 +16,13 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 	tests := []opensearchtest.TableTest[*ast.Ast, osu.Builder]{
 		// kql to os dsl - type tests
 		{
-			Name: "term query - string node",
+			Name: "match phrase query - string node on an analyzed field",
 			Got: &ast.Ast{
 				Nodes: []ast.Node{
 					&ast.StringNode{Key: "Name", Value: "openCloud"},
 				},
 			},
-			Want: osu.NewTermQuery[string]("Name").Value("openCloud"),
+			Want: osu.NewMatchPhraseQuery("Name").Query("openCloud"),
 		},
 		{
 			Name: "term query - boolean node - true",
@@ -58,7 +58,18 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 					&ast.StringNode{Key: "Name", Value: "open*"},
 				},
 			},
-			Want: osu.NewWildcardQuery("Name").Value("open*"),
+			Want: osu.NewWildcardQuery("Name.keyword").
+				Value("open*").
+				Params(&osu.WildcardQueryParams{CaseInsensitive: true}),
+		},
+		{
+			Name: "wildcard query - string node without a keyword sub field",
+			Got: &ast.Ast{
+				Nodes: []ast.Node{
+					&ast.StringNode{Key: "Content", Value: "open*"},
+				},
+			},
+			Want: osu.NewWildcardQuery("Content").Value("open*"),
 		},
 		{
 			Name: "bool query",
@@ -71,8 +82,8 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 				},
 			},
 			Want: osu.NewBoolQuery().Must(
-				osu.NewTermQuery[string]("Name").Value("a"),
-				osu.NewTermQuery[string]("Name").Value("b"),
+				osu.NewMatchPhraseQuery("Name").Query("a"),
+				osu.NewMatchPhraseQuery("Name").Query("b"),
 			),
 		},
 		{
@@ -84,7 +95,7 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 					}},
 				},
 			},
-			Want: osu.NewTermQuery[string]("Name").Value("any"),
+			Want: osu.NewMatchPhraseQuery("Name").Query("any"),
 		},
 		{
 			Name: "range query >",
@@ -146,7 +157,7 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 					&ast.StringNode{Key: "Name", Value: "openCloud"},
 				},
 			},
-			Want: osu.NewTermQuery[string]("Name").Value("openCloud"),
+			Want: osu.NewMatchPhraseQuery("Name").Query("openCloud"),
 		},
 		{
 			Name: "[* *]",
@@ -158,7 +169,7 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 			},
 			Want: osu.NewBoolQuery().
 				Must(
-					osu.NewTermQuery[string]("Name").Value("openCloud"),
+					osu.NewMatchPhraseQuery("Name").Query("openCloud"),
 					osu.NewTermQuery[string]("age").Value("32"),
 				),
 		},
@@ -173,7 +184,7 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 			},
 			Want: osu.NewBoolQuery().
 				Must(
-					osu.NewTermQuery[string]("Name").Value("openCloud"),
+					osu.NewMatchPhraseQuery("Name").Query("openCloud"),
 					osu.NewTermQuery[string]("age").Value("32"),
 				),
 		},
@@ -189,7 +200,7 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 			Want: osu.NewBoolQuery().
 				Params(&osu.BoolQueryParams{MinimumShouldMatch: 1}).
 				Should(
-					osu.NewTermQuery[string]("Name").Value("openCloud"),
+					osu.NewMatchPhraseQuery("Name").Query("openCloud"),
 					osu.NewTermQuery[string]("age").Value("32"),
 				),
 		},
@@ -217,7 +228,7 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 			},
 			Want: osu.NewBoolQuery().
 				Must(
-					osu.NewTermQuery[string]("Name").Value("openCloud"),
+					osu.NewMatchPhraseQuery("Name").Query("openCloud"),
 				).
 				MustNot(
 					osu.NewTermQuery[string]("age").Value("32"),
@@ -237,7 +248,7 @@ func TestTranspileKQLToOpenSearch(t *testing.T) {
 			Want: osu.NewBoolQuery().
 				Params(&osu.BoolQueryParams{MinimumShouldMatch: 1}).
 				Should(
-					osu.NewTermQuery[string]("Name").Value("openCloud"),
+					osu.NewMatchPhraseQuery("Name").Query("openCloud"),
 					osu.NewTermQuery[string]("age").Value("32"),
 					osu.NewTermQuery[string]("age").Value("44"),
 				),

services/search/pkg/query/error.go

@@ -1,6 +1,7 @@
 package query
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/opencloud-eu/opencloud/pkg/ast"
@@ -38,10 +39,16 @@ func (e UnsupportedTimeRangeError) Error() string {
 	return fmt.Sprintf("unable to convert '%v' to a time range", e.Value)
 }
 
+// IsValidationError says whether the query itself is at fault, which makes it a
+// bad request and not an error of ours.
 func IsValidationError(err error) bool {
-	switch err.(type) {
-	case *StartsWithBinaryOperatorError, *NamedGroupInvalidNodesError, *UnsupportedTimeRangeError:
-		return true
-	}
-	return false
+	var (
+		startsWithBinaryOperator *StartsWithBinaryOperatorError
+		namedGroupInvalidNodes   *NamedGroupInvalidNodesError
+		unsupportedTimeRange     *UnsupportedTimeRangeError
+	)
+
+	return errors.As(err, &startsWithBinaryOperator) ||
+		errors.As(err, &namedGroupInvalidNodes) ||
+		errors.As(err, &unsupportedTimeRange)
 }

tests/acceptance/features/apiSearch1/search.feature

@@ -97,7 +97,7 @@ Feature: Search
       | new              |
       | spaces           |
 
-  @issue-10329 @issue-3310
+  @issue-10329
   Scenario Outline: user can search hidden files
     Given using <dav-path-version> DAV path
     And user "Alice" has created a folder ".space" in space "project101"
@@ -320,7 +320,7 @@ Feature: Search
       | name:*der2   | subFOLDER2    | patern 'name:''                 |
       | name:"*der2" | subFOLDER2    | pattern 'name:""' (with quotes) |
 
-  @issue-7812 @issue-8442 @issue-10329 @issue-3312
+  @issue-7812 @issue-8442 @issue-10329
   Scenario: try to search with invalid patterns
     Given using spaces DAV path
     And user "Alice" has uploaded file with content "test file" to "testFile.txt"
@@ -395,7 +395,7 @@ Feature: Search
       | new              |
       | spaces           |
 
-  @issue-10329 @issue-3310
+  @issue-10329
   Scenario Outline: search for entry with emoji by pattern
     Given using <dav-path-version> DAV path
     And user "Alice" has uploaded file with content "hello world" to "upload😀 😁.txt"

tests/acceptance/features/apiSearchContent/contentSearch.feature

@@ -26,7 +26,7 @@ Feature: content search
       | new              |
       | spaces           |
 
-  @issue-10329 @issue-3310
+  @issue-10329
   Scenario Outline: search files by different content types
     Given using <dav-path-version> DAV path
     And user "Alice" has uploaded file with content "Using k6, you can test the reliability and performance of your systems" to "wordWithNumber.md"