symbols: respect limits, merge properly (#61056)

fixes #60698

This fixes the following bugs for unindexed symbol search:

Now we respect the limit given by "frontend". Before, the upper limit of unindexed symbol search was hard coded to 500. This limit could not be overwritten with the count: filter.

Merge symbol matches properly, applying the same logic as we do for indexed search: Before, symbol matches within the same file were simply appended which created duplicates for AND/OR queries if separate terms of the query matched the same symbol.

Test plan:
New unit test
Partially covered by existing tests

Author: stefanhengl

cmd/symbols/internal/api/handler.go

@@ -11,13 +11,10 @@ import (
 	internalgrpc "github.com/sourcegraph/sourcegraph/internal/grpc"
 	"github.com/sourcegraph/sourcegraph/internal/grpc/defaults"
 	"github.com/sourcegraph/sourcegraph/internal/search"
-	"github.com/sourcegraph/sourcegraph/internal/search/result"
 	proto "github.com/sourcegraph/sourcegraph/internal/symbols/v1"
 	internaltypes "github.com/sourcegraph/sourcegraph/internal/types"
 )
 
-const maxNumSymbolResults = 500
-
 type grpcService struct {
 	searchFunc   types.SearchFunc
 	readFileFunc func(context.Context, internaltypes.RepoCommitPath) ([]byte, error)
@@ -60,21 +57,12 @@ func NewHandler(
 	handleStatus func(http.ResponseWriter, *http.Request),
 	ctagsBinary string,
 ) http.Handler {
-	searchFuncWrapper := func(ctx context.Context, args search.SymbolsParameters) (result.Symbols, error) {
-		// Massage the arguments to ensure that First is set to a reasonable value.
-		if args.First < 0 || args.First > maxNumSymbolResults {
-			args.First = maxNumSymbolResults
-		}
-
-		return searchFunc(ctx, args)
-	}
-
 	rootLogger := logger.Scoped("symbolsServer")
 
 	// Initialize the gRPC server
 	grpcServer := defaults.NewServer(rootLogger)
 	proto.RegisterSymbolsServiceServer(grpcServer, &grpcService{
-		searchFunc:   searchFuncWrapper,
+		searchFunc:   searchFunc,
 		readFileFunc: readFileFunc,
 		ctagsBinary:  ctagsBinary,
 		logger:       rootLogger.Scoped("grpc"),

cmd/symbols/internal/database/store/search.go

@@ -44,7 +44,16 @@ func scanSymbols(rows *sql.Rows, queryErr error) (symbols []result.Symbol, err e
 	return symbols, nil
 }
 
+// This limit prevents users from accidentally running a query that returns an
+// extremely large number of results. It is arbitrary, but it should be at least
+// as high as the default limit frontend sends to the symbol service.
+const maxSymbolLimit = 50_000
+
 func (s *store) Search(ctx context.Context, args search.SymbolsParameters) ([]result.Symbol, error) {
+	if args.First < 0 || args.First > maxSymbolLimit {
+		return nil, errors.Newf("limit %d out of bounds [0, %d]", args.First, maxSymbolLimit)
+	}
+
 	return scanSymbols(s.Query(ctx, sqlf.Sprintf(
 		`
 			SELECT

internal/search/result/file.go

@@ -3,10 +3,12 @@ package result
 import (
 	"net/url"
 	"path"
+	"slices"
 	"strings"
 	"unicode/utf8"
 
 	"github.com/sourcegraph/log"
+
 	"github.com/sourcegraph/sourcegraph/internal/api"
 	"github.com/sourcegraph/sourcegraph/internal/search/filter"
 	"github.com/sourcegraph/sourcegraph/internal/types"
@@ -164,15 +166,19 @@ func (fm *FileMatch) Select(selectPath filter.SelectPath) Match {
 	return nil
 }
 
-// AppendMatches appends the line matches from src as well as updating match
-// counts and limit.
 func (fm *FileMatch) AppendMatches(src *FileMatch) {
 	// TODO merge hunk matches smartly
 	fm.ChunkMatches = append(fm.ChunkMatches, src.ChunkMatches...)
-	fm.Symbols = append(fm.Symbols, src.Symbols...)
+	fm.mergeSymbols(src)
 	fm.LimitHit = fm.LimitHit || src.LimitHit
 }
 
+func (fm *FileMatch) mergeSymbols(src *FileMatch) {
+	fm.Symbols = append(fm.Symbols, src.Symbols...)
+	slices.SortFunc(fm.Symbols, compareSymbolMatches)
+	fm.Symbols = DedupSymbols(fm.Symbols)
+}
+
 // Limit will mutate fm such that it only has limit results. limit is a number
 // greater than 0.
 //

internal/search/result/file_test.go

@@ -6,6 +6,107 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func mkSymbolMatch(name string, line int) *SymbolMatch {
+	return &SymbolMatch{
+		Symbol: Symbol{
+			Name: name,
+			Line: line,
+		},
+	}
+}
+
+func TestAppendSymbols(t *testing.T) {
+	cases := []struct {
+		name   string
+		input1 *FileMatch
+		input2 *FileMatch
+		output *FileMatch
+	}{
+		{
+			name: "duplicate symbol",
+			input1: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym2", 42),
+					mkSymbolMatch("sym1", 41),
+				},
+			},
+			input2: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym2", 42),
+					mkSymbolMatch("sym3", 43),
+				},
+			},
+			output: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym1", 41),
+					mkSymbolMatch("sym2", 42),
+					mkSymbolMatch("sym3", 43),
+				},
+			},
+		},
+		{
+			name: "same line, different symbol",
+			input1: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym1", 41),
+				},
+			},
+			input2: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym2", 41),
+				},
+			},
+			output: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym1", 41),
+					mkSymbolMatch("sym2", 41),
+				},
+			},
+		},
+		{
+			name:   "empty left side",
+			input1: &FileMatch{},
+			input2: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym1", 41),
+				},
+			},
+			output: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym1", 41),
+				},
+			},
+		},
+		{
+			name: "empty right side",
+			input1: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym1", 41),
+				},
+			},
+			input2: &FileMatch{},
+			output: &FileMatch{
+				Symbols: []*SymbolMatch{
+					mkSymbolMatch("sym1", 41),
+				},
+			},
+		},
+		{
+			name:   "both empty",
+			input1: &FileMatch{},
+			input2: &FileMatch{},
+			output: &FileMatch{},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			tc.input1.mergeSymbols(tc.input2)
+			require.Equal(t, tc.output, tc.input1)
+		})
+	}
+}
+
 func TestConvertMatches(t *testing.T) {
 	t.Run("AsLineMatches", func(t *testing.T) {
 		cases := []struct {

internal/search/result/symbol.go

@@ -1,6 +1,7 @@
 package result
 
 import (
+	"cmp"
 	"fmt"
 	"net/url"
 	"strconv"
@@ -142,6 +143,45 @@ func (s *SymbolMatch) URL() *url.URL {
 	return base
 }
 
+func compareSymbolMatches(a, b *SymbolMatch) int {
+	if v := cmp.Compare(a.Symbol.Line, b.Symbol.Line); v != 0 {
+		return v
+	}
+
+	if v := cmp.Compare(a.Symbol.Name, b.Symbol.Name); v != 0 {
+		return v
+	}
+
+	if v := cmp.Compare(a.Symbol.Kind, b.Symbol.Kind); v != 0 {
+		return v
+	}
+
+	if v := cmp.Compare(a.Symbol.Parent, b.Symbol.Parent); v != 0 {
+		return v
+	}
+
+	return cmp.Compare(a.Symbol.ParentKind, b.Symbol.ParentKind)
+}
+
+// DedupSymbols removes duplicate symbols from the list. We use a heuristic to
+// determine duplicate matches. We regard a match as the same if they have the
+// same symbol info and appear on the same line. I am unaware of a language
+// where you can break that assumption.
+func DedupSymbols(symbols []*SymbolMatch) []*SymbolMatch {
+	if len(symbols) <= 1 {
+		return symbols
+	}
+	dedup := symbols[:1]
+	for _, sym := range symbols[1:] {
+		last := dedup[len(dedup)-1]
+		if compareSymbolMatches(sym, last) == 0 {
+			continue
+		}
+		dedup = append(dedup, sym)
+	}
+	return dedup
+}
+
 func urlFragmentFromRange(lspRange lsp.Range) string {
 	if lspRange.Start == lspRange.End {
 		return "L" + lineSpecFromPosition(lspRange.Start, false)

internal/search/zoekt/indexed_search.go

@@ -532,28 +532,8 @@ func zoektFileMatchToSymbolResults(repoName types.MinimalRepo, inputRev string,
 
 	// We deduplicate symbol matches. For example searching for "foo AND bar"
 	// will return the symbol "foobar" twice. However, sourcegraph's result
-	// type for symbol is modeled as a result per symbol. Additionally we use
-	// a heuristic to determine duplicate matches. We regard a match as the
-	// same if they have the same symbol info and appear on the same line. I
-	// am unaware of a language where you can break that assumption.
-	if len(symbols) <= 1 {
-		return symbols
-	}
-	dedup := symbols[:1]
-	for _, sym := range symbols[1:] {
-		last := dedup[len(dedup)-1]
-		if (sym.Symbol.Line == last.Symbol.Line) &&
-			(sym.Symbol.Name == last.Symbol.Name) &&
-			(sym.Symbol.Kind == last.Symbol.Kind) &&
-			(sym.Symbol.Parent == last.Symbol.Parent) &&
-			(sym.Symbol.ParentKind == last.Symbol.ParentKind) {
-			continue
-		}
-		dedup = append(dedup, sym)
-	}
-	symbols = dedup
-
-	return symbols
+	// type for symbol is modeled as a result per symbol.
+	return result.DedupSymbols(symbols)
 }
 
 // contextWithoutDeadline returns a context which will cancel if the cOld is