artin/sourcegraph
1
0
Fork 0
mirror of https://github.com/sourcegraph/sourcegraph.git synced 2026-02-06 19:21:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

Search chore: tidy up dependencies on symbol search (#57576)

Browse Source 
This contains a bunch of cleanups of search symbols code. Basically, the goal was to take all the global clients used by symbols and explicitly declare them in a new ZoektSymbolsClient.

In order to limit the scope of this change, I added a new DefaultZoektSymbolsClient so that the things that expect to be able to call this stuff without dependencies still can.
This commit is contained in:
Camden Cheek 2023-10-13 11:11:00 -05:00 committed by GitHub
parent a9a1cef5df
commit f72eaa70f0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 148 additions and 153 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
cmd/frontend/backend/BUILD.bazel
View File

@ -13,7 +13,6 @@ go_library(
"mocks_temp.go",
"repos.go",
"repos_mock.go",
"symbols.go",
"trace.go",
"user_emails.go",
"users.go",
@ -56,9 +55,6 @@ go_library(
"//internal/repos",
"//internal/repoupdater",
"//internal/repoupdater/protocol",
"//internal/search",
"//internal/search/result",
"//internal/symbols",
"//internal/trace",
"//internal/txemail",
"//internal/txemail/txtypes",

26
cmd/frontend/backend/symbols.go
View File

@ -1,26 +0,0 @@
package backend
import (
"context"
"github.com/sourcegraph/sourcegraph/internal/search"
"github.com/sourcegraph/sourcegraph/internal/search/result"
symbolsclient "github.com/sourcegraph/sourcegraph/internal/symbols"
)
// Symbols backend.
var Symbols = &symbols{}
type symbols struct{}
// ListTags returns symbols in a repository from ctags.
func (symbols) ListTags(ctx context.Context, args search.SymbolsParameters) (result.Symbols, error) {
symbols, err := symbolsclient.DefaultClient.Search(ctx, args)
if err != nil {
return nil, err
}
for i := range symbols {
symbols[i].Line += 1 // callers expect 1-indexed lines
}
return symbols, nil
}

2
cmd/frontend/graphqlbackend/repository_stats.go
View File

@ -71,7 +71,7 @@ func (r *repositoryStatsResolver) IndexedLinesCount(ctx context.Context) (BigInt
func (r *repositoryStatsResolver) computeIndexedStats(ctx context.Context) (int32, int64, error) {
r.indexedStatsOnce.Do(func() {
repos, err := search.ListAllIndexed(ctx)
repos, err := search.ListAllIndexed(ctx, search.Indexed())
if err != nil {
r.indexedStatsErr = err
return

7
cmd/frontend/graphqlbackend/symbols.go
View File

@ -6,7 +6,6 @@ import (
"github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend/graphqlutil"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/authz"
"github.com/sourcegraph/sourcegraph/internal/database"
"github.com/sourcegraph/sourcegraph/internal/gitserver"
"github.com/sourcegraph/sourcegraph/internal/search/result"
@ -20,7 +19,7 @@ type symbolsArgs struct {
}
func (r *GitTreeEntryResolver) Symbols(ctx context.Context, args *symbolsArgs) (*symbolConnectionResolver, error) {
symbols, err := symbol.Compute(ctx, authz.DefaultSubRepoPermsChecker, r.commit.repoResolver.RepoMatch.RepoName(), api.CommitID(r.commit.oid), r.commit.inputRev, args.Query, args.First, args.IncludePatterns)
symbols, err := symbol.DefaultZoektSymbolsClient().Compute(ctx, r.commit.repoResolver.RepoMatch.RepoName(), api.CommitID(r.commit.oid), r.commit.inputRev, args.Query, args.First, args.IncludePatterns)
if err != nil && len(symbols) == 0 {
return nil, err
}
@ -34,7 +33,7 @@ func (r *GitTreeEntryResolver) Symbol(ctx context.Context, args *struct {
Line int32
Character int32
}) (*symbolResolver, error) {
symbolMatch, err := symbol.GetMatchAtLineCharacter(ctx, authz.DefaultSubRepoPermsChecker, r.commit.repoResolver.RepoMatch.RepoName(), api.CommitID(r.commit.oid), r.Path(), int(args.Line), int(args.Character))
symbolMatch, err := symbol.DefaultZoektSymbolsClient().GetMatchAtLineCharacter(ctx, r.commit.repoResolver.RepoMatch.RepoName(), api.CommitID(r.commit.oid), r.Path(), int(args.Line), int(args.Character))
if err != nil || symbolMatch == nil {
return nil, err
}
@ -42,7 +41,7 @@ func (r *GitTreeEntryResolver) Symbol(ctx context.Context, args *struct {
}
func (r *GitCommitResolver) Symbols(ctx context.Context, args *symbolsArgs) (*symbolConnectionResolver, error) {
symbols, err := symbol.Compute(ctx, authz.DefaultSubRepoPermsChecker, r.repoResolver.RepoMatch.RepoName(), api.CommitID(r.oid), r.inputRev, args.Query, args.First, args.IncludePatterns)
symbols, err := symbol.DefaultZoektSymbolsClient().Compute(ctx, r.repoResolver.RepoMatch.RepoName(), api.CommitID(r.oid), r.inputRev, args.Query, args.First, args.IncludePatterns)
if err != nil && len(symbols) == 0 {
return nil, err
}

1
cmd/frontend/internal/app/ui/BUILD.bazel
View File

@ -36,7 +36,6 @@ go_library(
"//cmd/frontend/internal/search",
"//internal/api",
"//internal/auth/userpasswd",
"//internal/authz",
"//internal/conf",
"//internal/conf/deploy",
"//internal/cookie",

4
cmd/frontend/internal/app/ui/handlers.go
View File

@ -28,7 +28,6 @@ import (
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/routevar"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/auth/userpasswd"
"github.com/sourcegraph/sourcegraph/internal/authz"
"github.com/sourcegraph/sourcegraph/internal/conf"
"github.com/sourcegraph/sourcegraph/internal/conf/deploy"
"github.com/sourcegraph/sourcegraph/internal/cookie"
@ -276,9 +275,8 @@ func newCommon(w http.ResponseWriter, r *http.Request, db database.DB, title str
ctx, cancel := context.WithTimeout(r.Context(), time.Second*1)
defer cancel()
if symbolMatch, _ := symbol.GetMatchAtLineCharacter(
if symbolMatch, _ := symbol.DefaultZoektSymbolsClient().GetMatchAtLineCharacter(
ctx,
authz.DefaultSubRepoPermsChecker,
types.MinimalRepo{ID: common.Repo.ID, Name: common.Repo.Name},
common.CommitID,
strings.TrimLeft(blobPath, "/"),

2
cmd/worker/internal/zoektrepos/zoektrepos.go
View File

@ -62,7 +62,7 @@ var (
)
func (h *handler) Handle(ctx context.Context) error {
indexed, err := search.ListAllIndexed(ctx)
indexed, err := search.ListAllIndexed(ctx, search.Indexed())
if err != nil {
return err
}

9
internal/search/env.go
View File

@ -30,8 +30,6 @@ var (
indexedDialerOnce sync.Once
indexedDialer backend.ZoektDialer
IndexedMock zoekt.Streamer
)
func SearcherURLs() *endpoint.Map {
@ -53,9 +51,6 @@ func SearcherGRPCConnectionCache() *defaults.ConnectionCache {
}
func Indexed() zoekt.Streamer {
if IndexedMock != nil {
return IndexedMock
}
indexedSearchOnce.Do(func() {
indexedSearch = backend.NewCachedSearcher(conf.Get().ServiceConnections().ZoektListTTL, backend.NewMeteredSearcher(
"", // no hostname means its the aggregator
@ -79,11 +74,11 @@ type ZoektAllIndexed struct {
}
// ListAllIndexed lists all indexed repositories.
func ListAllIndexed(ctx context.Context) (*ZoektAllIndexed, error) {
func ListAllIndexed(ctx context.Context, zs zoekt.Searcher) (*ZoektAllIndexed, error) {
q := &query.Const{Value: true}
opts := &zoekt.ListOptions{Field: zoekt.RepoListFieldReposMap}
repos, err := Indexed().List(ctx, q, opts)
repos, err := zs.List(ctx, q, opts)
if err != nil {
return nil, err
}

2
internal/search/searcher/BUILD.bazel
View File

@ -13,7 +13,6 @@ go_library(
importpath = "github.com/sourcegraph/sourcegraph/internal/search/searcher",
visibility = ["//:__subpackages__"],
deps = [
"//cmd/frontend/backend",
"//cmd/searcher/protocol",
"//internal/api",
"//internal/conf",
@ -29,6 +28,7 @@ go_library(
"//internal/search/streaming",
"//internal/search/streaming/http",
"//internal/searcher/v1:searcher",
"//internal/symbols",
"//internal/trace",
"//internal/types",
"//lib/errors",

11
internal/search/searcher/symbol_search_job.go
View File

@ -7,7 +7,6 @@ import (
"github.com/sourcegraph/conc/pool"
"go.opentelemetry.io/otel/attribute"
"github.com/sourcegraph/sourcegraph/cmd/frontend/backend"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/conf"
"github.com/sourcegraph/sourcegraph/internal/gitserver"
@ -15,6 +14,7 @@ import (
"github.com/sourcegraph/sourcegraph/internal/search/job"
"github.com/sourcegraph/sourcegraph/internal/search/result"
"github.com/sourcegraph/sourcegraph/internal/search/streaming"
"github.com/sourcegraph/sourcegraph/internal/symbols"
"github.com/sourcegraph/sourcegraph/internal/trace"
"github.com/sourcegraph/sourcegraph/internal/types"
)
@ -103,7 +103,7 @@ func searchInRepo(ctx context.Context, gitserverClient gitserver.Client, repoRev
}
tr.SetAttributes(commitID.Attr())
symbols, err := backend.Symbols.ListTags(ctx, search.SymbolsParameters{
symbols, err := symbols.DefaultClient.Search(ctx, search.SymbolsParameters{
Repo: repoRevs.Repo.Name,
CommitID: commitID,
Query: patternInfo.Pattern,
@ -114,6 +114,13 @@ func searchInRepo(ctx context.Context, gitserverClient gitserver.Client, repoRev
// Ask for limit + 1 so we can detect whether there are more results than the limit.
First: limit + 1,
})
if err != nil {
return nil, err
}
for i := range symbols {
symbols[i].Line += 1 // callers expect 1-indexed lines
}
// All symbols are from the same repo, so we can just partition them by path
// to build file matches

4
internal/search/symbol/BUILD.bazel
View File

@ -7,13 +7,14 @@ go_library(
importpath = "github.com/sourcegraph/sourcegraph/internal/search/symbol",
visibility = ["//:__subpackages__"],
deps = [
"//cmd/frontend/backend",
"//internal/actor",
"//internal/api",
"//internal/authz",
"//internal/search",
"//internal/search/result",
"//internal/search/zoekt",
"//internal/symbols",
"//internal/syncx",
"//internal/trace/policy",
"//internal/types",
"//lib/errors",
@ -37,7 +38,6 @@ go_test(
"//internal/api",
"//internal/authz/subrepoperms",
"//internal/conf",
"//internal/search",
"//internal/search/result",
"//internal/types",
"//lib/errors",

218
internal/search/symbol/symbol.go
View File

@ -10,13 +10,14 @@ import (
"github.com/sourcegraph/zoekt"
zoektquery "github.com/sourcegraph/zoekt/query"
"github.com/sourcegraph/sourcegraph/cmd/frontend/backend"
"github.com/sourcegraph/sourcegraph/internal/actor"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/authz"
"github.com/sourcegraph/sourcegraph/internal/search"
"github.com/sourcegraph/sourcegraph/internal/search/result"
zoektutil "github.com/sourcegraph/sourcegraph/internal/search/zoekt"
"github.com/sourcegraph/sourcegraph/internal/symbols"
"github.com/sourcegraph/sourcegraph/internal/syncx"
"github.com/sourcegraph/sourcegraph/internal/trace/policy"
"github.com/sourcegraph/sourcegraph/internal/types"
"github.com/sourcegraph/sourcegraph/lib/errors"
@ -24,14 +25,122 @@ import (
const DefaultSymbolLimit = 100
// NOTE: this lives inside a syncx.OnceValue because search.Indexed depends on
// conf.Get, and running conf.Get() at init time can cause a deadlock. So,
// we construct it lazily instead.
var DefaultZoektSymbolsClient = syncx.OnceValue(func() *ZoektSymbolsClient {
return &ZoektSymbolsClient{
subRepoPermsChecker: authz.DefaultSubRepoPermsChecker,
zoektStreamer: search.Indexed(),
symbols: symbols.DefaultClient,
}
})
type ZoektSymbolsClient struct {
subRepoPermsChecker authz.SubRepoPermissionChecker
zoektStreamer zoekt.Streamer
symbols *symbols.Client
}
func (s *ZoektSymbolsClient) Compute(ctx context.Context, repoName types.MinimalRepo, commitID api.CommitID, inputRev *string, query *string, first *int32, includePatterns *[]string) (res []*result.SymbolMatch, err error) {
// TODO(keegancsmith) we should be able to use indexedSearchRequest here
// and remove indexedSymbolsBranch.
if branch := indexedSymbolsBranch(ctx, s.zoektStreamer, &repoName, string(commitID)); branch != "" {
results, err := searchZoekt(ctx, s.zoektStreamer, repoName, commitID, inputRev, branch, query, first, includePatterns)
if err != nil {
return nil, errors.Wrap(err, "zoekt symbol search")
}
results, err = filterZoektResults(ctx, s.subRepoPermsChecker, repoName.Name, results)
if err != nil {
return nil, errors.Wrap(err, "checking permissions")
}
return results, nil
}
serverTimeout := 5 * time.Second
clientTimeout := 2 * serverTimeout
ctx, done := context.WithTimeout(ctx, clientTimeout)
defer done()
defer func() {
if ctx.Err() != nil && len(res) == 0 {
err = errors.Newf("The symbols service appears unresponsive, check the logs for errors.")
}
}()
var includePatternsSlice []string
if includePatterns != nil {
includePatternsSlice = *includePatterns
}
searchArgs := search.SymbolsParameters{
CommitID: commitID,
First: limitOrDefault(first) + 1, // add 1 so we can determine PageInfo.hasNextPage
Repo: repoName.Name,
IncludePatterns: includePatternsSlice,
Timeout: serverTimeout,
}
if query != nil {
searchArgs.Query = *query
}
symbols, err := s.symbols.Search(ctx, searchArgs)
if err != nil {
return nil, err
}
for i := range symbols {
symbols[i].Line += 1 // callers expect 1-indexed lines
}
fileWithPath := func(path string) *result.File {
return &result.File{
Path: path,
Repo: repoName,
InputRev: inputRev,
CommitID: commitID,
}
}
matches := make([]*result.SymbolMatch, 0, len(symbols))
for _, symbol := range symbols {
matches = append(matches, &result.SymbolMatch{
Symbol: symbol,
File: fileWithPath(symbol.Path),
})
}
return matches, err
}
// GetMatchAtLineCharacter retrieves the shortest matching symbol (if exists) defined
// at a specific line number and character offset in the provided file.
func (s *ZoektSymbolsClient) GetMatchAtLineCharacter(ctx context.Context, repo types.MinimalRepo, commitID api.CommitID, filePath string, line int, character int) (*result.SymbolMatch, error) {
// Should be large enough to include all symbols from a single file
first := int32(999999)
emptyString := ""
includePatterns := []string{regexp.QuoteMeta(filePath)}
symbolMatches, err := s.Compute(ctx, repo, commitID, &emptyString, &emptyString, &first, &includePatterns)
if err != nil {
return nil, err
}
var match *result.SymbolMatch
for _, symbolMatch := range symbolMatches {
symbolRange := symbolMatch.Symbol.Range()
isWithinRange := line >= symbolRange.Start.Line && character >= symbolRange.Start.Character && line <= symbolRange.End.Line && character <= symbolRange.End.Character
if isWithinRange && (match == nil || len(symbolMatch.Symbol.Name) < len(match.Symbol.Name)) {
match = symbolMatch
}
}
return match, nil
}
// indexedSymbols checks to see if Zoekt has indexed symbols information for a
// repository at a specific commit. If it has it returns the branch name (for
// use when querying zoekt). Otherwise an empty string is returned.
func indexedSymbolsBranch(ctx context.Context, repo *types.MinimalRepo, commit string) string {
func indexedSymbolsBranch(ctx context.Context, zs zoekt.Searcher, repo *types.MinimalRepo, commit string) string {
// We use ListAllIndexed since that is cached.
ctx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
list, err := search.ListAllIndexed(ctx)
list, err := search.ListAllIndexed(ctx, zs)
if err != nil {
return ""
}
@ -50,7 +159,7 @@ func indexedSymbolsBranch(ctx context.Context, repo *types.MinimalRepo, commit s
return ""
}
func FilterZoektResults(ctx context.Context, checker authz.SubRepoPermissionChecker, repo api.RepoName, results []*result.SymbolMatch) ([]*result.SymbolMatch, error) {
func filterZoektResults(ctx context.Context, checker authz.SubRepoPermissionChecker, repo api.RepoName, results []*result.SymbolMatch) ([]*result.SymbolMatch, error) {
if !authz.SubRepoEnabled(checker) {
return results, nil
}
@ -69,7 +178,17 @@ func FilterZoektResults(ctx context.Context, checker authz.SubRepoPermissionChec
return filtered, nil
}
func searchZoekt(ctx context.Context, repoName types.MinimalRepo, commitID api.CommitID, inputRev *string, branch string, queryString *string, first *int32, includePatterns *[]string) (res []*result.SymbolMatch, err error) {
func searchZoekt(
ctx context.Context,
z zoekt.Searcher,
repoName types.MinimalRepo,
commitID api.CommitID,
inputRev *string,
branch string,
queryString *string,
first *int32,
includePatterns *[]string,
) (res []*result.SymbolMatch, err error) {
var raw string
if queryString != nil {
raw = *queryString
@ -114,7 +233,7 @@ func searchZoekt(ctx context.Context, repoName types.MinimalRepo, commitID api.C
final := zoektquery.Simplify(zoektquery.NewAnd(ands...))
match := limitOrDefault(first) + 1
resp, err := search.Indexed().Search(ctx, final, &zoekt.SearchOptions{
resp, err := z.Search(ctx, final, &zoekt.SearchOptions{
Trace: policy.ShouldTrace(ctx),
MaxWallTime: 3 * time.Second,
ShardMaxMatchCount: match * 25,
@ -188,93 +307,6 @@ func searchZoekt(ctx context.Context, repoName types.MinimalRepo, commitID api.C
return
}
func Compute(ctx context.Context, checker authz.SubRepoPermissionChecker, repoName types.MinimalRepo, commitID api.CommitID, inputRev *string, query *string, first *int32, includePatterns *[]string) (res []*result.SymbolMatch, err error) {
// TODO(keegancsmith) we should be able to use indexedSearchRequest here
// and remove indexedSymbolsBranch.
if branch := indexedSymbolsBranch(ctx, &repoName, string(commitID)); branch != "" {
results, err := searchZoekt(ctx, repoName, commitID, inputRev, branch, query, first, includePatterns)
if err != nil {
return nil, errors.Wrap(err, "zoekt symbol search")
}
results, err = FilterZoektResults(ctx, checker, repoName.Name, results)
if err != nil {
return nil, errors.Wrap(err, "checking permissions")
}
return results, nil
}
serverTimeout := 5 * time.Second
clientTimeout := 2 * serverTimeout
ctx, done := context.WithTimeout(ctx, clientTimeout)
defer done()
defer func() {
if ctx.Err() != nil && len(res) == 0 {
err = errors.Newf("The symbols service appears unresponsive, check the logs for errors.")
}
}()
var includePatternsSlice []string
if includePatterns != nil {
includePatternsSlice = *includePatterns
}
searchArgs := search.SymbolsParameters{
CommitID: commitID,
First: limitOrDefault(first) + 1, // add 1 so we can determine PageInfo.hasNextPage
Repo: repoName.Name,
IncludePatterns: includePatternsSlice,
Timeout: serverTimeout,
}
if query != nil {
searchArgs.Query = *query
}
symbols, err := backend.Symbols.ListTags(ctx, searchArgs)
if err != nil {
return nil, err
}
fileWithPath := func(path string) *result.File {
return &result.File{
Path: path,
Repo: repoName,
InputRev: inputRev,
CommitID: commitID,
}
}
matches := make([]*result.SymbolMatch, 0, len(symbols))
for _, symbol := range symbols {
matches = append(matches, &result.SymbolMatch{
Symbol: symbol,
File: fileWithPath(symbol.Path),
})
}
return matches, err
}
// GetMatchAtLineCharacter retrieves the shortest matching symbol (if exists) defined
// at a specific line number and character offset in the provided file.
func GetMatchAtLineCharacter(ctx context.Context, checker authz.SubRepoPermissionChecker, repo types.MinimalRepo, commitID api.CommitID, filePath string, line int, character int) (*result.SymbolMatch, error) {
// Should be large enough to include all symbols from a single file
first := int32(999999)
emptyString := ""
includePatterns := []string{regexp.QuoteMeta(filePath)}
symbolMatches, err := Compute(ctx, checker, repo, commitID, &emptyString, &emptyString, &first, &includePatterns)
if err != nil {
return nil, err
}
var match *result.SymbolMatch
for _, symbolMatch := range symbolMatches {
symbolRange := symbolMatch.Symbol.Range()
isWithinRange := line >= symbolRange.Start.Line && character >= symbolRange.Start.Character && line <= symbolRange.End.Line && character <= symbolRange.End.Character
if isWithinRange && (match == nil || len(symbolMatch.Symbol.Name) < len(match.Symbol.Name)) {
match = symbolMatch
}
}
return match, nil
}
func limitOrDefault(first *int32) int {
if first == nil {
return DefaultSymbolLimit

9
internal/search/symbol/symbol_test.go
View File

@ -10,7 +10,6 @@ import (
"github.com/sourcegraph/sourcegraph/internal/api"
srp "github.com/sourcegraph/sourcegraph/internal/authz/subrepoperms"
"github.com/sourcegraph/sourcegraph/internal/conf"
"github.com/sourcegraph/sourcegraph/internal/search"
"github.com/sourcegraph/sourcegraph/internal/search/result"
"github.com/sourcegraph/sourcegraph/internal/types"
"github.com/sourcegraph/sourcegraph/lib/errors"
@ -23,12 +22,8 @@ func TestSearchZoektDoesntPanicWithNilQuery(t *testing.T) {
mockStreamer := NewMockStreamer()
expectedErr := errors.New("short circuit")
mockStreamer.SearchFunc.SetDefaultReturn(nil, expectedErr)
search.IndexedMock = mockStreamer
t.Cleanup(func() {
search.IndexedMock = nil
})
_, err := searchZoekt(context.Background(), types.MinimalRepo{ID: 1}, "commitID", nil, "branch", nil, nil, nil)
_, err := searchZoekt(context.Background(), mockStreamer, types.MinimalRepo{ID: 1}, "commitID", nil, "branch", nil, nil, nil)
assert.ErrorIs(t, err, expectedErr)
}
@ -65,7 +60,7 @@ func TestFilterZoektResults(t *testing.T) {
},
},
}
filtered, err := FilterZoektResults(ctx, checker, repoName, results)
filtered, err := filterZoektResults(ctx, checker, repoName, results)
if err != nil {
t.Fatal(err)
}

2
internal/usagestats/repositories.go
View File

@ -45,7 +45,7 @@ func GetRepositories(ctx context.Context, db database.DB) (*Repositories, error)
}
total.GitDirBytes = uint64(gitDirSize)
repos, err := search.ListAllIndexed(ctx)
repos, err := search.ListAllIndexed(ctx, search.Indexed())
if err != nil {
return nil, err
}