LSIF: Do not ensure revision for LSIF result commit resolution
Created by: efritz
This fixes the following scenario:
- A user uploads an LSIF dump for some repo R at commit C. The upload proxy ensures this commit exists and proxies the upload to the lsif-server (we can hold the invariant that if an LSIF upload exists, the commit existed at some point in time on gitserver).
- The user force-pushes such that commit C is no longer on the codehost.
- The LSIF-server returns a result with commit C in a reference result (or similar query). This includes a Repository.Commit resolver to fire, which will try to fetch from the codehost when the commit isn't in gitserver.
This operation is too expensive for this particular code path and should simply fail-fast on this edge case of the resolution. After chatting with @keegancsmith and @mrnugget a bit, it seems like it's safe to say that this commit will exist on gitserver if it had existed on gitserver in the past and it also exists on the codehost.
Before this change such conditions cause a multi-second GraphQL query in the API explorer. After this change the results are filtered without a noticeable delay.
Reviwers: This PR simply threads additional variables to expose the NoEnsureCommit
option to the RepositoryResolver.Commit
method. If there is a cleaner solution I had not found, please suggest it!
Merge request reports
Activity
Created by: mrnugget
Reviwers: This PR simply threads additional variables to expose the
NoEnsureCommit
option to theRepositoryResolver.Commit
method. If there is a cleaner solution I had not found, please suggest it!The only other way that comes up is to introduce another method for the "NoEnsure"/with-opts path:
func (s *repos) ResolveRev(ctx context.Context, repo *types.Repo, rev string) (commitID api.CommitID, err error) { return s.ResolveRev(ctx, repo, rev, nil) } func (s *repos) ResolveRevWithOpts(ctx context.Context, repo *types.Repo, rev string, opt *git.ResolveRevisionOptions) (commitID api.CommitID, err error) { // [...] }
That way you don't have to pass in
nil
if you don't need options (which seems to be the majority of usecases).But I don't know how Go-ey that naming is :)
Created by: codecov[bot]
Codecov Report
Merging #7488 into master will increase coverage by
<.01%
. The diff coverage is100%
.@@ Coverage Diff @@ ## master #7488 +/- ## ========================================= + Coverage 40.09% 40.1% +<.01% ========================================= Files 1252 1252 Lines 65825 65827 +2 Branches 6165 6165 ========================================= + Hits 26391 26397 +6 + Misses 37032 37029 -3 + Partials 2402 2401 -1
Impacted Files Coverage Δ cmd/frontend/graphqlbackend/repository.go 25.57% <100%> (+0.68%)
cmd/frontend/internal/goroutine/goroutine.go 100% <0%> (+57.14%)