Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Commit c38e0b1

Browse files
author
Christoph Hegemann
authored
refactor(codeintel): Extracts a MappedIndex abstraction over uploads (#63781)
Precursor for https://linear.app/sourcegraph/issue/GRAPH-735/test-syntactic-usages This PR introduces the `MappedIndex` abstraction, which wraps up an upload with a target commit. Its APIs then take care of mapping upload relative paths and repo relative paths, and ranges across commits. My main motivation for making this change is that I can now test this logic in isolation (which this PR does), and also have an interface that is easy to fake and use to test syntactic usages. ## Test plan Added unit tests for the `MappedIndex` component, manual testing of the GraphQL APIs show no changes in the syntactic usages output between this PR and master.
1 parent dfb55a2 commit c38e0b1

File tree

12 files changed

+722
-301
lines changed

12 files changed

+722
-301
lines changed

internal/codeintel/codenav/BUILD.bazel

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ go_library(
99
"gittree_translator.go",
1010
"iface.go",
1111
"init.go",
12+
"mapped_index.go",
1213
"mocks_temp.go",
1314
"observability.go",
1415
"request_state.go",
@@ -52,6 +53,7 @@ go_library(
5253
"@com_github_sourcegraph_scip//bindings/go/scip",
5354
"@com_github_wk8_go_ordered_map_v2//:go-ordered-map",
5455
"@io_opentelemetry_go_otel//attribute",
56+
"@org_golang_google_protobuf//proto",
5557
],
5658
)
5759

@@ -60,6 +62,7 @@ go_test(
6062
timeout = "short",
6163
srcs = [
6264
"gittree_translator_test.go",
65+
"mapped_index_test.go",
6366
"scip_utils_test.go",
6467
"service_closest_uploads_test.go",
6568
"service_definitions_test.go",
@@ -80,6 +83,7 @@ go_test(
8083
"//internal/actor",
8184
"//internal/api",
8285
"//internal/authz",
86+
"//internal/codeintel/codenav/internal/lsifstore",
8387
"//internal/codeintel/codenav/shared",
8488
"//internal/codeintel/core",
8589
"//internal/codeintel/uploads/shared",

internal/codeintel/codenav/internal/lsifstore/lsifstore_documents.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import (
1616
"github.com/sourcegraph/sourcegraph/internal/observation"
1717
)
1818

19-
func (s *store) SCIPDocument(ctx context.Context, uploadID int, path core.UploadRelPath) (_ *scip.Document, err error) {
19+
func (s *store) SCIPDocument(ctx context.Context, uploadID int, path core.UploadRelPath) (_ core.Option[*scip.Document], err error) {
2020
ctx, _, endObservation := s.operations.scipDocument.With(ctx, &err, observation.Args{Attrs: []attribute.KeyValue{
2121
attribute.String("path", path.RawValue()),
2222
attribute.Int("uploadID", uploadID),
@@ -40,8 +40,11 @@ func (s *store) SCIPDocument(ctx context.Context, uploadID int, path core.Upload
4040
}
4141
return &document, nil
4242
})
43-
doc, _, err := scanner(s.db.Query(ctx, sqlf.Sprintf(fetchSCIPDocumentQuery, uploadID, path.RawValue())))
44-
return doc, err
43+
doc, ok, err := scanner(s.db.Query(ctx, sqlf.Sprintf(fetchSCIPDocumentQuery, uploadID, path.RawValue())))
44+
if err != nil || !ok {
45+
return core.None[*scip.Document](), err
46+
}
47+
return core.Some(doc), nil
4548
}
4649

4750
const fetchSCIPDocumentQuery = `

internal/codeintel/codenav/internal/lsifstore/store.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ type LsifStore interface {
2424
// Whole-document data
2525
GetStencil(ctx context.Context, bundleID int, path core.UploadRelPath) ([]shared.Range, error)
2626
GetRanges(ctx context.Context, bundleID int, path core.UploadRelPath, startLine, endLine int) ([]shared.CodeIntelligenceRange, error)
27-
SCIPDocument(ctx context.Context, uploadID int, path core.UploadRelPath) (_ *scip.Document, err error)
27+
SCIPDocument(ctx context.Context, uploadID int, path core.UploadRelPath) (core.Option[*scip.Document], error)
2828

2929
// Fetch symbol names by position
3030
GetMonikersByPosition(ctx context.Context, uploadID int, path core.UploadRelPath, line, character int) ([][]precise.MonikerData, error)
Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
package codenav
2+
3+
import (
4+
"context"
5+
"sync"
6+
7+
genslices "github.com/life4/genesis/slices"
8+
"github.com/sourcegraph/scip/bindings/go/scip"
9+
"google.golang.org/protobuf/proto"
10+
11+
"github.com/sourcegraph/sourcegraph/internal/api"
12+
"github.com/sourcegraph/sourcegraph/internal/codeintel/codenav/internal/lsifstore"
13+
"github.com/sourcegraph/sourcegraph/internal/codeintel/codenav/shared"
14+
"github.com/sourcegraph/sourcegraph/internal/codeintel/core"
15+
)
16+
17+
type MappedIndex interface {
18+
// GetDocument returns None if the index does not contain a document at the given path.
19+
// There is no caching here, every call to GetDocument re-fetches the full document from the database.
20+
GetDocument(context.Context, core.RepoRelPath) (core.Option[MappedDocument], error)
21+
GetUploadSummary() core.UploadSummary
22+
// TODO: Should there be a bulk-API for getting multiple documents?
23+
}
24+
25+
var _ MappedIndex = mappedIndex{}
26+
27+
// MappedDocument wraps a SCIP document from an uploaded index.
28+
// All methods accept and return ranges in the context of the target commit.
29+
type MappedDocument interface {
30+
// GetOccurrences returns shared slices. Do not modify the returned slice or Occurrences without copying them first
31+
GetOccurrences(context.Context) ([]*scip.Occurrence, error)
32+
// GetOccurrencesAtRange returns shared slices. Do not modify the returned slice or Occurrences without copying them first
33+
GetOccurrencesAtRange(context.Context, scip.Range) ([]*scip.Occurrence, error)
34+
}
35+
36+
var _ MappedDocument = &mappedDocument{}
37+
38+
// NewMappedIndexFromTranslator creates a MappedIndex using the given GitTreeTranslator.
39+
// The translators SourceCommit has to be the targetCommit, which will be mapped to upload.GetCommit()
40+
func NewMappedIndexFromTranslator(
41+
lsifStore lsifstore.LsifStore,
42+
gitTreeTranslator GitTreeTranslator,
43+
upload core.UploadLike,
44+
) MappedIndex {
45+
return mappedIndex{
46+
lsifStore: lsifStore,
47+
gitTreeTranslator: gitTreeTranslator,
48+
upload: upload,
49+
targetCommit: gitTreeTranslator.GetSourceCommit(),
50+
}
51+
}
52+
53+
type mappedIndex struct {
54+
lsifStore lsifstore.LsifStore
55+
gitTreeTranslator GitTreeTranslator
56+
upload core.UploadLike
57+
targetCommit api.CommitID
58+
}
59+
60+
func (i mappedIndex) GetUploadSummary() core.UploadSummary {
61+
return core.UploadSummary{
62+
ID: i.upload.GetID(),
63+
Root: i.upload.GetRoot(),
64+
Commit: i.upload.GetCommit(),
65+
}
66+
}
67+
68+
func (i mappedIndex) GetDocument(ctx context.Context, path core.RepoRelPath) (core.Option[MappedDocument], error) {
69+
optDocument, err := i.lsifStore.SCIPDocument(ctx, i.upload.GetID(), core.NewUploadRelPath(i.upload, path))
70+
if err != nil {
71+
return core.None[MappedDocument](), err
72+
}
73+
if document, ok := optDocument.Get(); ok {
74+
return core.Some[MappedDocument](&mappedDocument{
75+
gitTreeTranslator: i.gitTreeTranslator,
76+
indexCommit: i.upload.GetCommit(),
77+
targetCommit: i.targetCommit,
78+
path: path,
79+
document: &lockedDocument{
80+
inner: document,
81+
isMapped: false,
82+
mapErrored: nil,
83+
lock: sync.RWMutex{},
84+
},
85+
mapOnce: sync.Once{},
86+
}), nil
87+
} else {
88+
return core.None[MappedDocument](), nil
89+
}
90+
}
91+
92+
type mappedDocument struct {
93+
gitTreeTranslator GitTreeTranslator
94+
indexCommit api.CommitID
95+
targetCommit api.CommitID
96+
path core.RepoRelPath
97+
98+
document *lockedDocument
99+
mapOnce sync.Once
100+
}
101+
102+
type lockedDocument struct {
103+
inner *scip.Document
104+
isMapped bool
105+
mapErrored error
106+
lock sync.RWMutex
107+
}
108+
109+
func cloneOccurrence(occ *scip.Occurrence) *scip.Occurrence {
110+
occCopy, ok := proto.Clone(occ).(*scip.Occurrence)
111+
if !ok {
112+
panic("impossible! proto.Clone changed the type of message")
113+
}
114+
return occCopy
115+
}
116+
117+
// Concurrency: Only call this while you're holding a read lock on the document
118+
func (d *mappedDocument) mapAllOccurrences(ctx context.Context) ([]*scip.Occurrence, error) {
119+
newOccurrences := make([]*scip.Occurrence, 0)
120+
for _, occ := range d.document.inner.Occurrences {
121+
sharedRange := shared.TranslateRange(scip.NewRangeUnchecked(occ.Range))
122+
mappedRange, ok, err := d.gitTreeTranslator.GetTargetCommitRangeFromSourceRange(ctx, string(d.indexCommit), d.path.RawValue(), sharedRange, true)
123+
if err != nil {
124+
return nil, err
125+
}
126+
if !ok {
127+
continue
128+
}
129+
newOccurrence := cloneOccurrence(occ)
130+
newOccurrence.Range = mappedRange.ToSCIPRange().SCIPRange()
131+
newOccurrences = append(newOccurrences, newOccurrence)
132+
}
133+
return newOccurrences, nil
134+
}
135+
136+
func (d *mappedDocument) GetOccurrences(ctx context.Context) ([]*scip.Occurrence, error) {
137+
// It's important we don't remap the occurrences twice
138+
d.mapOnce.Do(func() {
139+
d.document.lock.RLock()
140+
newOccurrences, err := d.mapAllOccurrences(ctx)
141+
d.document.lock.RUnlock()
142+
143+
d.document.lock.Lock()
144+
defer d.document.lock.Unlock()
145+
if err != nil {
146+
d.document.mapErrored = err
147+
return
148+
}
149+
d.document.inner.Occurrences = newOccurrences
150+
d.document.isMapped = true
151+
})
152+
153+
if d.document.mapErrored != nil {
154+
return nil, d.document.mapErrored
155+
}
156+
return d.document.inner.Occurrences, nil
157+
}
158+
159+
func (d *mappedDocument) GetOccurrencesAtRange(ctx context.Context, range_ scip.Range) ([]*scip.Occurrence, error) {
160+
d.document.lock.RLock()
161+
occurrences := d.document.inner.Occurrences
162+
if d.document.isMapped {
163+
d.document.lock.RUnlock()
164+
return FindOccurrencesWithEqualRange(occurrences, range_), nil
165+
}
166+
d.document.lock.RUnlock()
167+
168+
mappedRg, ok, err := d.gitTreeTranslator.GetTargetCommitRangeFromSourceRange(
169+
ctx, string(d.indexCommit), d.path.RawValue(), shared.TranslateRange(range_), false,
170+
)
171+
if err != nil {
172+
return nil, err
173+
}
174+
if !ok {
175+
// The range was changed/removed in the target commit, so return no occurrences
176+
return nil, nil
177+
}
178+
pastMatchingOccurrences := FindOccurrencesWithEqualRange(occurrences, mappedRg.ToSCIPRange())
179+
scipRange := range_.SCIPRange()
180+
return genslices.Map(pastMatchingOccurrences, func(occ *scip.Occurrence) *scip.Occurrence {
181+
newOccurrence := cloneOccurrence(occ)
182+
newOccurrence.Range = scipRange
183+
return newOccurrence
184+
}), nil
185+
}

0 commit comments

Comments
 (0)